s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the
implementation of the former can move to the latter.
scripts/bloat-o-meter reports for this change (on an ARCH=arm
allmodconfig build):
add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125)
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/watchdog/s3c2410_wdt.c | 70 +++++++++++++++-------------------
1 file changed, 30 insertions(+), 40 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 58b262ca4e88..7382bf038161 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -579,11 +579,27 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
return 0;
}
-static inline const struct s3c2410_wdt_variant *
-s3c2410_get_wdt_drv_data(struct platform_device *pdev)
+static void s3c2410wdt_wdt_disable_action(void *data)
+{
+ s3c2410wdt_enable(data, false);
+}
+
+static int s3c2410wdt_probe(struct platform_device *pdev)
{
- const struct s3c2410_wdt_variant *variant;
struct device *dev = &pdev->dev;
+ const struct s3c2410_wdt_variant *variant;
+ struct s3c2410_wdt *wdt;
+ unsigned int wtcon;
+ int wdt_irq;
+ int ret;
+
+ wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
+
+ wdt->dev = dev;
+ spin_lock_init(&wdt->lock);
+ wdt->wdt_device = s3c2410_wdd;
variant = of_device_get_match_data(dev);
if (!variant) {
@@ -597,56 +613,30 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
if (variant == &drv_data_exynos850_cl0 ||
variant == &drv_data_exynosautov9_cl0) {
u32 index;
- int err;
- err = of_property_read_u32(dev->of_node,
+ ret = of_property_read_u32(dev->of_node,
"samsung,cluster-index", &index);
- if (err) {
+ if (ret) {
dev_err(dev, "failed to get cluster index\n");
- return NULL;
+ return -EINVAL;
}
switch (index) {
case 0:
- return variant;
+ break;
case 1:
- return (variant == &drv_data_exynos850_cl0) ?
- &drv_data_exynos850_cl1 :
- &drv_data_exynosautov9_cl1;
+ if (variant == &drv_data_exynos850_cl0)
+ variant = &drv_data_exynos850_cl1;
+ else
+ variant = &drv_data_exynosautov9_cl1;
+ break;
default:
dev_err(dev, "wrong cluster index: %u\n", index);
- return NULL;
+ return -EINVAL;
}
}
#endif
-
- return variant;
-}
-
-static void s3c2410wdt_wdt_disable_action(void *data)
-{
- s3c2410wdt_enable(data, false);
-}
-
-static int s3c2410wdt_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct s3c2410_wdt *wdt;
- unsigned int wtcon;
- int wdt_irq;
- int ret;
-
- wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
- if (!wdt)
- return -ENOMEM;
-
- wdt->dev = dev;
- spin_lock_init(&wdt->lock);
- wdt->wdt_device = s3c2410_wdd;
-
- wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
- if (!wdt->drv_data)
- return -EINVAL;
+ wdt->drv_data = variant;
if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
--
2.39.1
On 3/6/23 01:09, Uwe Kleine-König wrote: > s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the > implementation of the former can move to the latter. > > scripts/bloat-o-meter reports for this change (on an ARCH=arm > allmodconfig build): > > add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125) > The reason for separating functions in this case wasn't that the separate function would be called several times. It was to improve code readability. If anything, I would argue that it might sense to split the already lengthy probe function further instead of combining it. Maybe I am old fashioned. Maybe the old "split your code into multiple functions if the function size is larger than X lines" no longer applies, and it is now "never split functions unless the separated function is called more than once". Still, I am quite concerned that accepting this patch would result in a flurry of similar patches which would all do nothing but hurt readability, using the same set of arguments. I really don't like where this is going. I am going to leave it up to Wim to decide if and how to proceed. Guenter
On Mon, Mar 06, 2023 at 09:17:23AM -0800, Guenter Roeck wrote: > On 3/6/23 01:09, Uwe Kleine-König wrote: > > s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the > > implementation of the former can move to the latter. > > > > scripts/bloat-o-meter reports for this change (on an ARCH=arm > > allmodconfig build): > > > > add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125) > > > > The reason for separating functions in this case wasn't that the separate function > would be called several times. It was to improve code readability. If anything, > I would argue that it might sense to split the already lengthy probe function > further instead of combining it. Agreed. For dev_err_probe() to work the following would be alternatively possible: diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index 58b262ca4e88..564919717761 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -579,8 +579,8 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt) return 0; } -static inline const struct s3c2410_wdt_variant * -s3c2410_get_wdt_drv_data(struct platform_device *pdev) +static inline int +s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt) { const struct s3c2410_wdt_variant *variant; struct device *dev = &pdev->dev; @@ -603,24 +603,26 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev) "samsung,cluster-index", &index); if (err) { dev_err(dev, "failed to get cluster index\n"); - return NULL; + return -EINVAL; } switch (index) { case 0: - return variant; + break; case 1: - return (variant == &drv_data_exynos850_cl0) ? + variant = (variant == &drv_data_exynos850_cl0) ? &drv_data_exynos850_cl1 : &drv_data_exynosautov9_cl1; + break; default: dev_err(dev, "wrong cluster index: %u\n", index); - return NULL; + return -EINVAL; } } #endif + wdt->drv_data = variant; - return variant; + return 0; } static void s3c2410wdt_wdt_disable_action(void *data) @@ -644,9 +646,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) spin_lock_init(&wdt->lock); wdt->wdt_device = s3c2410_wdd; - wdt->drv_data = s3c2410_get_wdt_drv_data(pdev); - if (!wdt->drv_data) - return -EINVAL; + ret = s3c2410_get_wdt_drv_data(pdev, wdt); + if (ret) + return ret; if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) { wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, I didn't check if it's the same as for my initial patch #1, but for an allmodconfig ARCH=arm build this also has a nice bloatometer output: add/remove: 1/1 grow/shrink: 0/1 up/down: 4/-104 (-100) This would allow to make use of dev_err_probe() in s3c2410_get_wdt_drv_data() and still maintain the split into two functions. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On 3/6/23 09:47, Uwe Kleine-König wrote: > On Mon, Mar 06, 2023 at 09:17:23AM -0800, Guenter Roeck wrote: >> On 3/6/23 01:09, Uwe Kleine-König wrote: >>> s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the >>> implementation of the former can move to the latter. >>> >>> scripts/bloat-o-meter reports for this change (on an ARCH=arm >>> allmodconfig build): >>> >>> add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125) >>> >> >> The reason for separating functions in this case wasn't that the separate function >> would be called several times. It was to improve code readability. If anything, >> I would argue that it might sense to split the already lengthy probe function >> further instead of combining it. > > Agreed. For dev_err_probe() to work the following would be alternatively > possible: > That looks ok to me. Thanks, Guenter > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 58b262ca4e88..564919717761 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -579,8 +579,8 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt) > return 0; > } > > -static inline const struct s3c2410_wdt_variant * > -s3c2410_get_wdt_drv_data(struct platform_device *pdev) > +static inline int > +s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt) > { > const struct s3c2410_wdt_variant *variant; > struct device *dev = &pdev->dev; > @@ -603,24 +603,26 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev) > "samsung,cluster-index", &index); > if (err) { > dev_err(dev, "failed to get cluster index\n"); > - return NULL; > + return -EINVAL; > } > > switch (index) { > case 0: > - return variant; > + break; > case 1: > - return (variant == &drv_data_exynos850_cl0) ? > + variant = (variant == &drv_data_exynos850_cl0) ? > &drv_data_exynos850_cl1 : > &drv_data_exynosautov9_cl1; > + break; > default: > dev_err(dev, "wrong cluster index: %u\n", index); > - return NULL; > + return -EINVAL; > } > } > #endif > + wdt->drv_data = variant; > > - return variant; > + return 0; > } > > static void s3c2410wdt_wdt_disable_action(void *data) > @@ -644,9 +646,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > spin_lock_init(&wdt->lock); > wdt->wdt_device = s3c2410_wdd; > > - wdt->drv_data = s3c2410_get_wdt_drv_data(pdev); > - if (!wdt->drv_data) > - return -EINVAL; > + ret = s3c2410_get_wdt_drv_data(pdev, wdt); > + if (ret) > + return ret; > > if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) { > wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, > > I didn't check if it's the same as for my initial patch #1, but for an > allmodconfig ARCH=arm build this also has a nice bloatometer output: > > add/remove: 1/1 grow/shrink: 0/1 up/down: 4/-104 (-100) > > This would allow to make use of dev_err_probe() in > s3c2410_get_wdt_drv_data() and still maintain the split into two > functions. > > Best regards > Uwe >
© 2016 - 2025 Red Hat, Inc.