| Message ID | 20230306090919.2206871-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: s3c2410_wdt: Simplify using dev_err_probe() | expand |
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
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 >
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,
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(-)