Re: [RESEND PATCH v3 2/3] watchdog: f71808e_wdt: migrate to new kernel watchdog API

[Ahmad Fatoum]

Hello Guenter,

On 03.05.21 17:28, Guenter Roeck wrote:
> On Mon, May 03, 2021 at 04:39:25PM +0200, Ahmad Fatoum wrote:
>> Hello Guenter,
>>
>> On 03.05.21 16:31, Guenter Roeck wrote:
>>>> -static struct fintek_wdt watchdog = {
>>>> -	.lock = __MUTEX_INITIALIZER(watchdog.lock),
>>>> -};
>>>> +static struct fintek_wdt watchdog;
>>>
>>> This should really be allocated, and "watchdog" is a terrible variable name
>>> even if static, especially given the previous patch.
>>
>> I can add a follow up patch to change this. I maintained the old
>> state of things here to make review easier.
>>
> Odd argument. You changed all the function names (unnecessarily,
> if you ask me) in the first patch of the series because they were
> too generic in your opinion.

I find it surprising to have a function called watchdog_set_timeout
defined locally in a driver.

But yes, the first patch was more useful in v2, where new functions
were added for pinmuxing each variant. Having f71808e_ for generic
code and f8186fg_ for a variant would not help code comprehension.

> That by itself added a lot of unnecessary complexity to the review.

This was not my intention. I figured having a separate patch that just
does the rename should make review of follow-up commits easier, not harder...

> And pretty much everything else changed
> in this patch anyway.

Feedback on v2 was that I change too much and that this made review harder.
I thus left name changes in a separate patch and limited changes in follow-up
commit to signatures where appropriate.

Thanks,
Ahmad

> 
> Guenter
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |