Re: [PATCH v3 4/6] watchdog: w83627hf_wdt: Store Super I/O configurations in platform data

["Paul Louvel" <paul.louvel@bootlin.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 9419 bytes --]

On Tue Oct 6, 2026 at 4:25 PM CEST, Tzung-Bi Shih wrote:
> On Sun, Oct 04, 2026 at 02:12:52PM +0200, Paul Louvel wrote:
>>  static int wdt_probe(struct platform_device *pdev)
>>  {
>>  	const struct platform_device_id *id = platform_get_device_id(pdev);
>> +	const struct wdt_pdata *pdata = pdev->dev.platform_data;
>
> Use dev_get_platdata().
>
>> @@ -522,12 +533,16 @@ static int wdt_probe(struct platform_device *pdev)
>>   */
>>  static int __init wdt_use_alt_key(const struct dmi_system_id *d)
>>  {
>> -	wdt_cfg_enter = 0x88;
>> -	wdt_cfg_leave = 0xBB;
>> +	struct wdt_pdata *pdata = d->driver_data;
>> +
>> +	pdata->siocfg_enter = 0x88;
>> +	pdata->siocfg_leave = 0xBB;
>>  
>>  	return 0;
>>  }
>>  
>> +static struct wdt_pdata pdata;
>> +
>>  static const struct dmi_system_id wdt_dmi_table[] __initconst = {
>>  	{
>>  		.matches = {
>> @@ -537,6 +552,7 @@ static const struct dmi_system_id wdt_dmi_table[] __initconst = {
>>  			DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"),
>>  		},
>>  		.callback = wdt_use_alt_key,
>> +		.driver_data = &pdata,
>>  	},
>>  	{}
>>  };
>> @@ -581,27 +597,33 @@ static struct platform_device *wdt_pdev;
>>  
>>  static int __init wdt_init(void)
>>  {
>> +	int sioaddr;
>>  	int ret;
>>  	int chip;
>>  
>> +	pdata.siocfg_enter = 0x87;
>> +	pdata.siocfg_leave = 0xAA;
>> +
>>  	/* Apply system-specific quirks */
>>  	dmi_check_system(wdt_dmi_table);
>>  
>> -	wdt_io = SIO_REG_CONF_ADDR0;
>> -	chip = wdt_find(SIO_REG_CONF_ADDR0);
>> +	sioaddr = SIO_REG_CONF_ADDR0;
>> +	chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave);
>>  	if (chip < 0) {
>> -		wdt_io = SIO_REG_CONF_ADDR1;
>> -		chip = wdt_find(SIO_REG_CONF_ADDR1);
>> +		sioaddr = SIO_REG_CONF_ADDR1;
>> +		chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave);
>>  		if (chip < 0)
>>  			return chip;
>>  	}
>>  
>> +	pdata.sioaddr = sioaddr;
>> +
>
> `pdata` is still global.  Given that there are only a few settings, maybe
> we can use the same method I proposed in [3/6] patch, and use
> `dmi_first_match()` to avoid the global variable.  E.g.:
>
>     struct w83627hf_sio_pdata {
>         u16 sioaddr;
>         u8 siocfg_enter;
>         u8 siocfg_leave;
>     };
>
>     static const struct w83627hf_sio_pdata default_pdata = {
>         .siocfg_enter = 0x87,
>         .siocfg_leave = 0xaa,
>     };
>
>     static const struct w83627hf_sio_pdata alt_pdata = {
>         .siocfg_enter = 0x88,
>         .siocfg_leave = 0xbb,
>     };
>
>     static const struct dmi_system_id wdt_dmi_table[] __initconst = {
>         {
>             .matches = {
>                 DMI_EXACT_MATCH(DMI_SYS_VENDOR, "INVENTEC"),
>                 DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Symphony"),
>                 DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"),
>             },
>             .driver_data = (void *)&alt_pdata,
>         },
>         {}
>     };
>
> Then, in `wdt_init()`, it can define a local `pdata` and initialize it
> based on the DMI match:
>
>     const struct dmi_system_id *match;
>     struct w83627hf_sio_pdata pdata;
>
>     match = dmi_first_match(wdt_dmi_table);
>     if (match)
>         pdata = *(const struct w83627hf_sio_pdata *)match->driver_data;
>     else
>         pdata = default_pdata;
>
>     pdata.sioaddr = SIO_REG_CONF_ADDR0;
>     chip = wdt_find(pdata.sioaddr, pdata.siocfg_enter, pdata.siocfg_leave);

Yes, pdata was still global, but the scope is limited to the init function.
Your proposition looks better tho.



-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

text:
Return-Path: <paul.louvel@bootlin.com>
Delivered-To: paul.louvel@bootlin.com
Received: from mail.galae.net ([172.22.1.253])
	by 9e7fea892943 with LMTP
	id oH2THD72yGpQfCkAtaWaaQ
	(envelope-from <paul.louvel@bootlin.com>)
	for <paul.louvel@bootlin.com>; Fri, 09 Oct 2026 16:12:14 +0200
X-Original-To: paul.louvel@bootlin.com
Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 1C92A11D70841;
	Fri,  9 Oct 2026 16:12:07 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim;
	t\x1791555130; h=from:subject:date:message-id:to:cc:mime-version:content-type:
	 content-transfer-encoding:in-reply-to:references;
	bhÍZYN8ntqvipg3FYRWOYxSDKsm2OOZGI6Qs5B7jTHyg=;
	b=OaV7x4LGSW6b2FnAKVdiks6LA9di0h6IEKNQVLYWnT0py3eLzLeIzaoC7AoA+fTFRaL/Xj
	4wWvkXky+Z5f3v295TFVfUFFM5B/QGXhLcwWFHT8e0sttThRh2jox1FXUrYz+mOUvGMzWQ
	IX9sSHcM08okc7CvmcRSP2LL+XSQsZIKXadnkCyBZZUYC6iglTZv1FT+GBnFhKSdWsr8mN
	Wy/XjjpTmUzHioiFShyML8FQ+hrgMjp2c+E8hkxv06qlu/+kQlpVsFzygTVU32jzs4rnho
	k1o0ohkJw8L3ZnpPckzPPlafRA5hkmeK/MkTpEMlqA8aaP+7lDQHVx9tgGkRiw=Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 09 Oct 2026 16:12:06 +0200
Message-Id: <DM0DAEXPSUES.3UOY3A0GN18C0@bootlin.com>
From: "Paul Louvel" <paul.louvel@bootlin.com>
To: "Tzung-Bi Shih" <tzungbi@kernel.org>, "Paul Louvel"
 <paul.louvel@bootlin.com>
Cc: "Wim Van Sebroeck" <wim@linux-watchdog.org>, "Guenter Roeck"
 <linux@roeck-us.net>, <linux-watchdog@vger.kernel.org>,
 <linux-kernel@vger.kernel.org>, "Thomas Petazzoni"
 <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v3 4/6] watchdog: w83627hf_wdt: Store Super I/O
 configurations in platform data
X-Mailer: aerc 0.21.0-0-g5549850facc2
References: <20261004-w83627hf_wdt-improvements-v3-0-8e27b518595e@bootlin.com> <20261004-w83627hf_wdt-improvements-v3-4-8e27b518595e@bootlin.com> <asUE3iFZBbl8fVcb@google.com>
In-Reply-To: <asUE3iFZBbl8fVcb@google.com>
X-Last-TLS-Session-Version: TLSv1.3

On Tue Oct 6, 2026 at 4:25 PM CEST, Tzung-Bi Shih wrote:
> On Sun, Oct 04, 2026 at 02:12:52PM +0200, Paul Louvel wrote:
>>  static int wdt_probe(struct platform_device *pdev)
>>  {
>>  	const struct platform_device_id *id = platform_get_device_id(pdev);
>> +	const struct wdt_pdata *pdata = pdev->dev.platform_data;
>
> Use dev_get_platdata().
>
>> @@ -522,12 +533,16 @@ static int wdt_probe(struct platform_device *pdev)
>>   */
>>  static int __init wdt_use_alt_key(const struct dmi_system_id *d)
>>  {
>> -	wdt_cfg_enter = 0x88;
>> -	wdt_cfg_leave = 0xBB;
>> +	struct wdt_pdata *pdata = d->driver_data;
>> +
>> +	pdata->siocfg_enter = 0x88;
>> +	pdata->siocfg_leave = 0xBB;
>>  
>>  	return 0;
>>  }
>>  
>> +static struct wdt_pdata pdata;
>> +
>>  static const struct dmi_system_id wdt_dmi_table[] __initconst = {
>>  	{
>>  		.matches = {
>> @@ -537,6 +552,7 @@ static const struct dmi_system_id wdt_dmi_table[] __initconst = {
>>  			DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"),
>>  		},
>>  		.callback = wdt_use_alt_key,
>> +		.driver_data = &pdata,
>>  	},
>>  	{}
>>  };
>> @@ -581,27 +597,33 @@ static struct platform_device *wdt_pdev;
>>  
>>  static int __init wdt_init(void)
>>  {
>> +	int sioaddr;
>>  	int ret;
>>  	int chip;
>>  
>> +	pdata.siocfg_enter = 0x87;
>> +	pdata.siocfg_leave = 0xAA;
>> +
>>  	/* Apply system-specific quirks */
>>  	dmi_check_system(wdt_dmi_table);
>>  
>> -	wdt_io = SIO_REG_CONF_ADDR0;
>> -	chip = wdt_find(SIO_REG_CONF_ADDR0);
>> +	sioaddr = SIO_REG_CONF_ADDR0;
>> +	chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave);
>>  	if (chip < 0) {
>> -		wdt_io = SIO_REG_CONF_ADDR1;
>> -		chip = wdt_find(SIO_REG_CONF_ADDR1);
>> +		sioaddr = SIO_REG_CONF_ADDR1;
>> +		chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave);
>>  		if (chip < 0)
>>  			return chip;
>>  	}
>>  
>> +	pdata.sioaddr = sioaddr;
>> +
>
> `pdata` is still global.  Given that there are only a few settings, maybe
> we can use the same method I proposed in [3/6] patch, and use
> `dmi_first_match()` to avoid the global variable.  E.g.:
>
>     struct w83627hf_sio_pdata {
>         u16 sioaddr;
>         u8 siocfg_enter;
>         u8 siocfg_leave;
>     };
>
>     static const struct w83627hf_sio_pdata default_pdata = {
>         .siocfg_enter = 0x87,
>         .siocfg_leave = 0xaa,
>     };
>
>     static const struct w83627hf_sio_pdata alt_pdata = {
>         .siocfg_enter = 0x88,
>         .siocfg_leave = 0xbb,
>     };
>
>     static const struct dmi_system_id wdt_dmi_table[] __initconst = {
>         {
>             .matches = {
>                 DMI_EXACT_MATCH(DMI_SYS_VENDOR, "INVENTEC"),
>                 DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Symphony"),
>                 DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"),
>             },
>             .driver_data = (void *)&alt_pdata,
>         },
>         {}
>     };
>
> Then, in `wdt_init()`, it can define a local `pdata` and initialize it
> based on the DMI match:
>
>     const struct dmi_system_id *match;
>     struct w83627hf_sio_pdata pdata;
>
>     match = dmi_first_match(wdt_dmi_table);
>     if (match)
>         pdata = *(const struct w83627hf_sio_pdata *)match->driver_data;
>     else
>         pdata = default_pdata;
>
>     pdata.sioaddr = SIO_REG_CONF_ADDR0;
>     chip = wdt_find(pdata.sioaddr, pdata.siocfg_enter, pdata.siocfg_leave);

Yes, pdata was still global, but the scope is limited to the init function.
Your proposition looks better tho.



-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com