Re: [PATCH v3] Watchdog: New module for ITE 5632 watchdog

[Mark Pearson]

Thanks Guenter

On Fri, Feb 23, 2024, at 3:21 PM, Guenter Roeck wrote:
> On 2/23/24 11:58, Mark Pearson wrote:
>> On Fri, Jul 21, 2023, at 8:29 AM, David Ober wrote:
>>> This modules is to allow for the new ITE 5632 EC chip
>>> to support the watchdog for initial use in the Lenovo SE10
>>>
>>> Signed-off-by: David Ober <dober6023@xxxxxxxxx>
>>>
>>> V2 Fix stop to deactivate wdog on unload of module
>>> V2 Remove extra logging that is not needed
>>> V2 change udelays to usleep_range
>>> V2 Changed to now request region on start and release on stop instead
>>>     of for every ping and read/write
>>> V3 add counter to while loops so it will not hang
>>> V3 rework code to use platform_device_register_simple
>>> V3 rework getting the Chip ID to remove duplicate code and close IO
>>> V3 change some extra logging to be debug only
>>> ---
> [ ... ]
>>> +config ITE5632_WDT
>>> +        tristate "ITE 5632"
>>> +        select WATCHDOG_CORE
>>> +        help
>>> +          If you say yes here you get support for the watchdog
>>> +          functionality of the ITE 5632 eSIO chip.
>>> +
>>> +          This driver can also be built as a module. If so, the module
>>> +          will be called ite5632_wdt.
>>> +
>
> [ ... ]
>
>>
>> 
>> Please let us know if there is anything else needed to get this accepted. Happy to address any feedback.
>> 
>
> I am sure I commented on this before. The fact that the Lenovo SE10 uses an
> ITE 5632 controller is completely irrelevant. Lenovo could decide tomorrow to
> replace the ITE chip with a Nuvoton chip, use the same API to talk with it,
> and the watchdog would work perfectly fine.
>
> This is a driver for the watchdog implemented in the embedded controller
> on Lenovo SE10. It is not a watchdog driver for ITE5632. Again, the EC chip
> used in that Lenovo system is completely irrelevant, even more so since
> this seems to be one of those undocumented ITE chips which officially
> don't even exist. Claiming that this would be a watchdog driver for ITE5632
> would be not only misleading but simply wrong.
>
> It seems that we can not agree on this. That means that, from my perspective,
> there is no real path to move forward. Wim will have to decide if and how
> to proceed.
>
My apologies - I hadn't realised that was the issue (my fault for missing it). Appreciate the clarification.

Is this as simple as renaming this driver as (for example) a lenovo_se_wdt device, and adding in the appropriate checking during the init that it is only used on Lenovo SE10 platforms?

I don't understand the concern if a different chip was used - wouldn't that need a different driver at that point?

If it's more subtle than that, is there something you propose instead?

Thanks
Mark