Re: [PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck <linux@roeck-us.net>]

On 01/24/2015 08:48 AM, Rafał Miłecki wrote:
> On 24 January 2015 at 17:33, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/24/2015 05:58 AM, Rafał Miłecki wrote:
>>>
>>> Just like in case of other watchdog drivers, use the new kernel core
>>> API to provide restart support.
>>>
>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>> ---
>>> V2: Include changes to include/linux/bcm47xx_wdt.h
>>> ---
>>>    drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>    include/linux/bcm47xx_wdt.h    |  1 +
>>>    2 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>> b/drivers/watchdog/bcm47xx_wdt.c
>>> index 9816485..dac3c5d 100644
>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>> notifier_block *this,
>>>          return NOTIFY_DONE;
>>>    }
>>>
>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long
>>> mode,
>>> +                              void *cmd)
>>> +{
>>> +       struct bcm47xx_wdt *wdt;
>>> +
>>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>> +       wdt->timer_set(wdt, 1);
>>> +
>>> +       return NOTIFY_DONE;
>>> +}
>>> +
>>>    static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>          .owner          = THIS_MODULE,
>>>          .start          = bcm47xx_wdt_soft_start,
>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device
>>> *pdev)
>>>          watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>
>>>          wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>> -
>>
>>
>> Unnecessary whitespace change.
>
> I changed it to improve code readability. Now you have two seprated
> code blocks. First one setups notification and registers it. Second
> setups handler and registers it.
>

That is not how it works; this is a logically separate change,
which should be submitted as separate patch if at all.

Keep in mind though that this is a personal preference; if we accept
this change, someone else may come along tomorrow asking to change it
back "to improve readability by removing unnecessary empty lines".
Normally I would leave that alone unless you get a checkpatch
warning or error, and even then it is advisable to know the maintainer's
position on accepting coding style changes.

Thanks,
Guenter