| Message ID | 1554913683-25454-21-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: Expand use of device managed functions (series 3 of 3) | expand |
On Wed, 2019-04-10 at 09:28 -0700, Guenter Roeck wrote: > Introduce local variable 'struct device *dev' and use it instead of > dereferencing it repeatedly. Also, there is no call to dev_get_drvdata() > or platform_get_drvdata() in the driver, so drop the unnecessary > call to platform_set_drvdata(). Dropping platform_set_drvdata seems to me like it should be a separate patch. And are you sure no other function uses a get_drvdata call? Maybe something in watchdog_dev.c? Possibly: #ifdef CONFIG_WATCHDOG_SYSFS static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr, char *buf) { struct watchdog_device *wdd = dev_get_drvdata(dev); return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status)); } static DEVICE_ATTR_RO(nowayout); > diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c [] > @@ -189,18 +190,16 @@ static int jz4740_wdt_probe(struct platform_device *pdev) [] > - platform_set_drvdata(pdev, drvdata); > - > return 0; > } >
Hi Joe, On Wed, Apr 10, 2019 at 09:52:09AM -0700, Joe Perches wrote: > On Wed, 2019-04-10 at 09:28 -0700, Guenter Roeck wrote: > > Introduce local variable 'struct device *dev' and use it instead of > > dereferencing it repeatedly. Also, there is no call to dev_get_drvdata() > > or platform_get_drvdata() in the driver, so drop the unnecessary > > call to platform_set_drvdata(). > > Dropping platform_set_drvdata seems to me like it should > be a separate patch. > Bundling all changes into one patch per driver already resulted in more than 60 patches total in this series. Splitting that into three sets of patches over three days already earned me automated replies telling me that I am now considered to be a spammer. One logical change per patch would have resulted in hundreds of patches. I don't think that would have scaled well. I considered other splits, such as one coccinelle rule per patch, affecting multiple drivers, but that would have had the same result since it would have needed dozens of Cc: lines per patch. Ultimately, I decided to go with one patch per file. > And are you sure no other function uses a get_drvdata call? > Maybe something in watchdog_dev.c? Possibly: > > #ifdef CONFIG_WATCHDOG_SYSFS > static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct watchdog_device *wdd = dev_get_drvdata(dev); > 'dev' in nowayout_show() points to the watchdog device, not to the platform device. Its drvdata is set in drivers/base/core.c:device_create_groups_vargs(). Not all watchdog drivers are platform drivers, and the watchdog core can not depend on a watchdog device even having a parent device, much less make assumptions about its drvdata. Guenter
diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c index aa9d3523addd..d1bc7cbd4f2b 100644 --- a/drivers/watchdog/jz4740_wdt.c +++ b/drivers/watchdog/jz4740_wdt.c @@ -163,11 +163,12 @@ MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches); static int jz4740_wdt_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; struct jz4740_wdt_drvdata *drvdata; struct watchdog_device *jz4740_wdt; int ret; - drvdata = devm_kzalloc(&pdev->dev, sizeof(struct jz4740_wdt_drvdata), + drvdata = devm_kzalloc(dev, sizeof(struct jz4740_wdt_drvdata), GFP_KERNEL); if (!drvdata) return -ENOMEM; @@ -181,7 +182,7 @@ static int jz4740_wdt_probe(struct platform_device *pdev) jz4740_wdt->timeout = heartbeat; jz4740_wdt->min_timeout = 1; jz4740_wdt->max_timeout = MAX_HEARTBEAT; - jz4740_wdt->parent = &pdev->dev; + jz4740_wdt->parent = dev; watchdog_set_nowayout(jz4740_wdt, nowayout); watchdog_set_drvdata(jz4740_wdt, drvdata); @@ -189,18 +190,16 @@ static int jz4740_wdt_probe(struct platform_device *pdev) if (IS_ERR(drvdata->base)) return PTR_ERR(drvdata->base); - drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc"); + drvdata->rtc_clk = devm_clk_get(dev, "rtc"); if (IS_ERR(drvdata->rtc_clk)) { - dev_err(&pdev->dev, "cannot find RTC clock\n"); + dev_err(dev, "cannot find RTC clock\n"); return PTR_ERR(drvdata->rtc_clk); } - ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt); + ret = devm_watchdog_register_device(dev, &drvdata->wdt); if (ret < 0) return ret; - platform_set_drvdata(pdev, drvdata); - return 0; }
Introduce local variable 'struct device *dev' and use it instead of dereferencing it repeatedly. Also, there is no call to dev_get_drvdata() or platform_get_drvdata() in the driver, so drop the unnecessary call to platform_set_drvdata(). The conversion was done automatically with coccinelle using the following semantic patches. The semantic patches and the scripts used to generate this commit log are available at https://github.com/groeck/coccinelle-patches Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/watchdog/jz4740_wdt.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)