| Message ID | 26d172891694e4041e73b7dae85a1848adf38034.1391177880.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek@xilinx.com> wrote: > Use of_property_read_u32 functions to clean OF probing. The subject is a bit misleading as this doesn't really fix anything. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c > index c229cc4..475440a6 100644 > --- a/drivers/watchdog/of_xilinx_wdt.c > +++ b/drivers/watchdog/of_xilinx_wdt.c > @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev) > static int xwdt_probe(struct platform_device *pdev) > { > int rc; > - u32 *tmptr; > - u32 *pfreq; > + u32 pfreq, enable_once; > struct resource *res; > struct xwdt_device *xdev; > bool no_timeout = false; > @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev) > if (IS_ERR(xdev->base)) > return PTR_ERR(xdev->base); > > - pfreq = (u32 *)of_get_property(pdev->dev.of_node, > - "clock-frequency", NULL); > - > - if (pfreq == NULL) { > + rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq); > + if (rc) { > dev_warn(&pdev->dev, > "The watchdog clock frequency cannot be obtained\n"); > no_timeout = true; You can kill this... > } > > - tmptr = (u32 *)of_get_property(pdev->dev.of_node, > - "xlnx,wdt-interval", NULL); > - if (tmptr == NULL) { > + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval", > + &xdev->wdt_interval); > + if (rc) { > dev_warn(&pdev->dev, > "Parameter \"xlnx,wdt-interval\" not found\n"); > no_timeout = true; and this... > - } else { > - xdev->wdt_interval = *tmptr; > } > > - tmptr = (u32 *)of_get_property(pdev->dev.of_node, > - "xlnx,wdt-enable-once", NULL); > - if (tmptr == NULL) { > + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once", > + &enable_once); > + if (!rc && enable_once) { > dev_warn(&pdev->dev, > "Parameter \"xlnx,wdt-enable-once\" not found\n"); > watchdog_set_nowayout(xilinx_wdt_wdd, true); > @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev) > */ > if (!no_timeout) and use "if (pfreq && xdev->wdt_interval)" if you initialize pfreq to 0. > xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) / > - *pfreq); > + pfreq); Is the wdog really usable if the timeout properties are missing? Seems like you should fail to probe rather than warn. Rob
On 01/31/2014 06:33 PM, Rob Herring wrote: > On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek@xilinx.com> wrote: >> Use of_property_read_u32 functions to clean OF probing. > > The subject is a bit misleading as this doesn't really fix anything. fair enough. Will change it. > >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++--------------- >> 1 file changed, 10 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c >> index c229cc4..475440a6 100644 >> --- a/drivers/watchdog/of_xilinx_wdt.c >> +++ b/drivers/watchdog/of_xilinx_wdt.c >> @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev) >> static int xwdt_probe(struct platform_device *pdev) >> { >> int rc; >> - u32 *tmptr; >> - u32 *pfreq; >> + u32 pfreq, enable_once; >> struct resource *res; >> struct xwdt_device *xdev; >> bool no_timeout = false; >> @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev) >> if (IS_ERR(xdev->base)) >> return PTR_ERR(xdev->base); >> >> - pfreq = (u32 *)of_get_property(pdev->dev.of_node, >> - "clock-frequency", NULL); >> - >> - if (pfreq == NULL) { >> + rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq); >> + if (rc) { >> dev_warn(&pdev->dev, >> "The watchdog clock frequency cannot be obtained\n"); >> no_timeout = true; > > You can kill this... > >> } >> >> - tmptr = (u32 *)of_get_property(pdev->dev.of_node, >> - "xlnx,wdt-interval", NULL); >> - if (tmptr == NULL) { >> + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval", >> + &xdev->wdt_interval); >> + if (rc) { >> dev_warn(&pdev->dev, >> "Parameter \"xlnx,wdt-interval\" not found\n"); >> no_timeout = true; > > and this... > >> - } else { >> - xdev->wdt_interval = *tmptr; >> } >> >> - tmptr = (u32 *)of_get_property(pdev->dev.of_node, >> - "xlnx,wdt-enable-once", NULL); >> - if (tmptr == NULL) { >> + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once", >> + &enable_once); >> + if (!rc && enable_once) { >> dev_warn(&pdev->dev, >> "Parameter \"xlnx,wdt-enable-once\" not found\n"); >> watchdog_set_nowayout(xilinx_wdt_wdd, true); >> @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev) >> */ >> if (!no_timeout) > > and use "if (pfreq && xdev->wdt_interval)" if you initialize pfreq to 0. I just wanted to to change functions not logic in the driver. But it can be changed too. >> xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) / >> - *pfreq); >> + pfreq); > > Is the wdog really usable if the timeout properties are missing? Seems > like you should fail to probe rather than warn. There are 2 things. 1. timeout - you don't need pfreq and wdt_interval to use this driver but for that there should be module parameter timeout there. It should be added. 2. about warn - based on 1 I don't think driver should failed but I am looking at logic above which I have added there but should be different. u32 enable_once = 0; if (!rc) dev_warn if (enable_once) watchdog_set_nowayout(xilinx_wdt_wdd, true); Thanks, Michal
On 02/03/2014 08:59 AM, Michal Simek wrote: > On 01/31/2014 06:33 PM, Rob Herring wrote: >> On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek@xilinx.com> wrote: >>> Use of_property_read_u32 functions to clean OF probing. >> >> The subject is a bit misleading as this doesn't really fix anything. > > fair enough. Will change it. > >> >>> >>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>> --- >>> >>> drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++--------------- >>> 1 file changed, 10 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c >>> index c229cc4..475440a6 100644 >>> --- a/drivers/watchdog/of_xilinx_wdt.c >>> +++ b/drivers/watchdog/of_xilinx_wdt.c >>> @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev) >>> static int xwdt_probe(struct platform_device *pdev) >>> { >>> int rc; >>> - u32 *tmptr; >>> - u32 *pfreq; >>> + u32 pfreq, enable_once; >>> struct resource *res; >>> struct xwdt_device *xdev; >>> bool no_timeout = false; >>> @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev) >>> if (IS_ERR(xdev->base)) >>> return PTR_ERR(xdev->base); >>> >>> - pfreq = (u32 *)of_get_property(pdev->dev.of_node, >>> - "clock-frequency", NULL); >>> - >>> - if (pfreq == NULL) { >>> + rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq); >>> + if (rc) { >>> dev_warn(&pdev->dev, >>> "The watchdog clock frequency cannot be obtained\n"); >>> no_timeout = true; >> >> You can kill this... >> >>> } >>> >>> - tmptr = (u32 *)of_get_property(pdev->dev.of_node, >>> - "xlnx,wdt-interval", NULL); >>> - if (tmptr == NULL) { >>> + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval", >>> + &xdev->wdt_interval); >>> + if (rc) { >>> dev_warn(&pdev->dev, >>> "Parameter \"xlnx,wdt-interval\" not found\n"); >>> no_timeout = true; >> >> and this... >> >>> - } else { >>> - xdev->wdt_interval = *tmptr; >>> } >>> >>> - tmptr = (u32 *)of_get_property(pdev->dev.of_node, >>> - "xlnx,wdt-enable-once", NULL); >>> - if (tmptr == NULL) { >>> + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once", >>> + &enable_once); >>> + if (!rc && enable_once) { >>> dev_warn(&pdev->dev, >>> "Parameter \"xlnx,wdt-enable-once\" not found\n"); >>> watchdog_set_nowayout(xilinx_wdt_wdd, true); >>> @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev) >>> */ >>> if (!no_timeout) >> >> and use "if (pfreq && xdev->wdt_interval)" if you initialize pfreq to 0. > > > I just wanted to to change functions not logic in the driver. > But it can be changed too. > >>> xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) / >>> - *pfreq); >>> + pfreq); >> >> Is the wdog really usable if the timeout properties are missing? Seems >> like you should fail to probe rather than warn. > > There are 2 things. > 1. timeout - you don't need pfreq and wdt_interval to use this driver > but for that there should be module parameter timeout there. > It should be added. > > 2. about warn - based on 1 I don't think driver should failed > but I am looking at logic above which I have added there but should be different. > > u32 enable_once = 0; > if (!rc) > dev_warn > if (rc) here sorry. Thanks, Michal
On 01/31/2014 06:18 AM, Michal Simek wrote: > Use of_property_read_u32 functions to clean OF probing. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c index c229cc4..475440a6 100644 --- a/drivers/watchdog/of_xilinx_wdt.c +++ b/drivers/watchdog/of_xilinx_wdt.c @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev) static int xwdt_probe(struct platform_device *pdev) { int rc; - u32 *tmptr; - u32 *pfreq; + u32 pfreq, enable_once; struct resource *res; struct xwdt_device *xdev; bool no_timeout = false; @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev) if (IS_ERR(xdev->base)) return PTR_ERR(xdev->base); - pfreq = (u32 *)of_get_property(pdev->dev.of_node, - "clock-frequency", NULL); - - if (pfreq == NULL) { + rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq); + if (rc) { dev_warn(&pdev->dev, "The watchdog clock frequency cannot be obtained\n"); no_timeout = true; } - tmptr = (u32 *)of_get_property(pdev->dev.of_node, - "xlnx,wdt-interval", NULL); - if (tmptr == NULL) { + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval", + &xdev->wdt_interval); + if (rc) { dev_warn(&pdev->dev, "Parameter \"xlnx,wdt-interval\" not found\n"); no_timeout = true; - } else { - xdev->wdt_interval = *tmptr; } - tmptr = (u32 *)of_get_property(pdev->dev.of_node, - "xlnx,wdt-enable-once", NULL); - if (tmptr == NULL) { + rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once", + &enable_once); + if (!rc && enable_once) { dev_warn(&pdev->dev, "Parameter \"xlnx,wdt-enable-once\" not found\n"); watchdog_set_nowayout(xilinx_wdt_wdd, true); @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev) */ if (!no_timeout) xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) / - *pfreq); + pfreq); spin_lock_init(&xdev->spinlock); watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
Use of_property_read_u32 functions to clean OF probing. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) -- 1.8.2.3