Re: [PATCH 07/10] watchdog: xilinx: Fix OF binding

[Michal Simek]

On 01/31/2014 06:33 PM, Rob Herring wrote:
> On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>
>>  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

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Attachment: signature.asc
Description: OpenPGP digital signature