Re: [PATCH v3 3/6] watchdog: w83627hf_wdt: Move register offsets into driver data

[Paul Louvel]

On Tue Oct 6, 2026 at 4:25 PM CEST, Tzung-Bi Shih wrote:
> On Sun, Oct 04, 2026 at 02:12:51PM +0200, Paul Louvel wrote:
>> @@ -491,6 +481,21 @@ static int wdt_probe(struct platform_device *pdev)
>>  	wdd->min_timeout = 1;
>>  	wdd->max_timeout = 255;
>>  
>> +	data->reg.timeout = W83627HF_WDT_TIMEOUT;
>> +	data->reg.control = W83627HF_WDT_CONTROL;
>> +	data->reg.csr = W836X7HF_WDT_CSR;
>> +
>> +	if (chip == nct6102 || chip == nct6116 || chip == nct6126) {
>> +		data->reg.timeout = NCT6102D_WDT_TIMEOUT;
>> +		data->reg.control = NCT6102D_WDT_CONTROL;
>> +		data->reg.csr = NCT6102D_WDT_CSR;
>> +	}
>> +
>> +	if (chip == w83697hf || chip == w83697ug) {
>> +		data->reg.timeout = W83697HF_WDT_TIMEOUT;
>> +		data->reg.control = W83697HF_WDT_CONTROL;
>> +	}
>> +
>
> A switch statement would be cleaner here.
>
> Also, there are only 3 distinct register configurations: default, nct61xx,
> and w83697xx.  Rather than storing and copying mutable integer fields in
> every driver data instance, consider defining static const register tables
> and holding a pointer to them, e.g.:
>
>     struct w83627hf_regs {
> 	u8 timeout;
> 	u8 control;
> 	u8 csr;
>     };
>
>     static const struct w83627hf_regs w83627hf_default_regs = {
> 	.timeout = W83627HF_WDT_TIMEOUT,
> 	.control = W83627HF_WDT_CONTROL,
> 	.csr = W836X7HF_WDT_CSR,
>     };
>
>     ...
>
>     struct w83627hf_data {
> 	...
> 	const struct w83627hf_regs *regs;
>     };

Agreed.

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