Received-SPF: pass (google.com: domain of linux-kernel+bounces-79372-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Feedback-ID: ic2b14614:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-153-g7e3bb84806-fm-20240215.007-g7e3bb848
Precedence: bulk
MIME-Version: 1.0
Message-Id: <4209014c-1730-4c31-87d8-4192d68bcbc6@app.fastmail.com>
In-Reply-To: <6b0373a2-7750-4d57-8839-95c6fa30c6b8@roeck-us.net>
References: <20230721122931.505957-1-dober6023@gmail.com>
 <a361ce91-beba-43d8-b969-285063658da5@app.fastmail.com>
 <6b0373a2-7750-4d57-8839-95c6fa30c6b8@roeck-us.net>
Date: Fri, 23 Feb 2024 19:43:17 -0500
From: "Mark Pearson" <mpearson@squebb.ca>
To: "Guenter Roeck" <linux@roeck-us.net>, "David Ober" <dober6023@gmail.com>,
 wim@linux-watchdog.org
Cc: linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
 "David Ober" <dober@lenovo.com>
Subject: Re: [PATCH v3] Watchdog: New module for ITE 5632 watchdog
Content-Type: text/plain

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@gmail.com>
>>>
>>> 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