| Message ID | 1460392435-23803-1-git-send-email-sudipm.mukherjee@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi, On 11/04/2016 at 22:03:55 +0530, Sudip Mukherjee wrote : > stmp3xxx_wdt_register() can fail as platform_device_alloc() or > platform_device_add() can fail. Lets check for the return value from > both platform_device_alloc() and platform_device_add() and return the > error value accordingly which is now propagated from probe. > Well, this may have been intentional. The RTC doesn't need the watchdog driver to be present to work correctly. However, I agree that failing silently is not nice. Did you have any particular issue? Maybe simply adding a dev_err() or two would be fine. > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk> > --- > drivers/rtc/rtc-stmp3xxx.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c > index ca54d03..e02e827 100644 > --- a/drivers/rtc/rtc-stmp3xxx.c > +++ b/drivers/rtc/rtc-stmp3xxx.c > @@ -105,20 +105,22 @@ static struct stmp3xxx_wdt_pdata wdt_pdata = { > .wdt_set_timeout = stmp3xxx_wdt_set_timeout, > }; > > -static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev) > +static int stmp3xxx_wdt_register(struct platform_device *rtc_pdev) > { > struct platform_device *wdt_pdev = > platform_device_alloc("stmp3xxx_rtc_wdt", rtc_pdev->id); > > - if (wdt_pdev) { > - wdt_pdev->dev.parent = &rtc_pdev->dev; > - wdt_pdev->dev.platform_data = &wdt_pdata; > - platform_device_add(wdt_pdev); > - } > + if (!wdt_pdev) > + return -ENOMEM; > + > + wdt_pdev->dev.parent = &rtc_pdev->dev; > + wdt_pdev->dev.platform_data = &wdt_pdata; > + return platform_device_add(wdt_pdev); > } > #else > -static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev) > +static int stmp3xxx_wdt_register(struct platform_device *rtc_pdev) > { > + return 0; > } > #endif /* CONFIG_STMP3XXX_RTC_WATCHDOG */ > > @@ -357,8 +359,7 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev) > return err; > } > > - stmp3xxx_wdt_register(pdev); > - return 0; > + return stmp3xxx_wdt_register(pdev); > } > > #ifdef CONFIG_PM_SLEEP > -- > 1.9.1 >
On Fri, Apr 15, 2016 at 11:07:15PM +0200, Alexandre Belloni wrote: > Hi, > > On 11/04/2016 at 22:03:55 +0530, Sudip Mukherjee wrote : > > stmp3xxx_wdt_register() can fail as platform_device_alloc() or > > platform_device_add() can fail. Lets check for the return value from > > both platform_device_alloc() and platform_device_add() and return the > > error value accordingly which is now propagated from probe. > > > > Well, this may have been intentional. The RTC doesn't need the watchdog > driver to be present to work correctly. However, I agree that failing > silently is not nice. > > Did you have any particular issue? Maybe simply adding a dev_err() or > two would be fine. I didnot have any particular issue. I will send a new patch with some error message there. regards sudip
diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c index ca54d03..e02e827 100644 --- a/drivers/rtc/rtc-stmp3xxx.c +++ b/drivers/rtc/rtc-stmp3xxx.c @@ -105,20 +105,22 @@ static struct stmp3xxx_wdt_pdata wdt_pdata = { .wdt_set_timeout = stmp3xxx_wdt_set_timeout, }; -static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev) +static int stmp3xxx_wdt_register(struct platform_device *rtc_pdev) { struct platform_device *wdt_pdev = platform_device_alloc("stmp3xxx_rtc_wdt", rtc_pdev->id); - if (wdt_pdev) { - wdt_pdev->dev.parent = &rtc_pdev->dev; - wdt_pdev->dev.platform_data = &wdt_pdata; - platform_device_add(wdt_pdev); - } + if (!wdt_pdev) + return -ENOMEM; + + wdt_pdev->dev.parent = &rtc_pdev->dev; + wdt_pdev->dev.platform_data = &wdt_pdata; + return platform_device_add(wdt_pdev); } #else -static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev) +static int stmp3xxx_wdt_register(struct platform_device *rtc_pdev) { + return 0; } #endif /* CONFIG_STMP3XXX_RTC_WATCHDOG */ @@ -357,8 +359,7 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev) return err; } - stmp3xxx_wdt_register(pdev); - return 0; + return stmp3xxx_wdt_register(pdev); } #ifdef CONFIG_PM_SLEEP
stmp3xxx_wdt_register() can fail as platform_device_alloc() or platform_device_add() can fail. Lets check for the return value from both platform_device_alloc() and platform_device_add() and return the error value accordingly which is now propagated from probe. Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk> --- drivers/rtc/rtc-stmp3xxx.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)