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 |
