Re: [PATCH v4 7/9] watchdog: max77620: add support for the max77714 variant

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Guenter,

On 29/11/21 22:56, Guenter Roeck wrote:
> On 11/29/21 1:24 PM, Luca Ceresoli wrote:
> [ ... ]
>>>>   +static const struct max77620_variant max77620_wdt_data = {
>>>> +    .wdt_info = {
>>>> +        .identity = "max77620-watchdog",
>>>> +        .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>>>> WDIOF_MAGICCLOSE,
>>>> +    },
>>>
>>> That does not have to be, and should not be, part of device specific
>>> data,
>>> just because of the identity string.
>>
>> Ok, no problem, will fix, but I have two questions.
>>
>> First, what's the reason? Coding style or a functional difference?
>> Usually const data is preferred to runtime assignment.
>>
> 
> wdt_info is not chip specific variant information as nothing but the
> identity
> string is different, and there is no technical need for that difference.
> 
>> Second: it's not clear how you expect it to be done. Looking into the
> 
> I gave you three options to pick from.

Perhaps it's because I'm not a native English speaker, but I thought
"either" introduces an alternative between two options (wiktionary
confirms, FWIW). Reading your sentence in that perspective gave it a
different meaning.

>> kernel it looks like almost all drivers set a constant string. I could
>> find only one exception, f71808e_wdt:
>> https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/watchdog/f71808e_wdt.c#L471
>>
>>
>>> Either keep the current identity string,
>>> mark max77620_wdt_info as __ro_after_init and overwrite the identity
>>> string
>>> there during probe
>>
>> And also remove 'static' I guess. Hm, I don't love this, as above I tend
>> to prefer static const when possible for file-scoped data.
>>
> 
> Where did I suggest to remove 'static', and what would be the benefit of
> making
> the variable global ?

You didn't. But since max77620_wdt_info is currently file-scoped, should
it be modified during probe it would generate a weird situation in case
one has both a max77714 and a max77620 on the same board (unlikely but
possible), as two instances of the same driver would modify the same
(static) data.

But all of this discussion is getting quite theoretical as...

>>> or add the structure to max77620_wdt and fill it out there.
>>
>> Do you mean like the following, untested, kind-of-pseudocode?
>>
>>   struct max77620_wdt {
>>       struct device            *dev;
>>       struct regmap            *rmap;
>>     const struct max77620_variant    *drv_data;
>> +    struct watchdog_info        info;     /* not a pointer! */
>>       struct watchdog_device        wdt_dev;
>>   };
>>
>> and then, in probe:
>>
>>     wdt->dev = dev;
>>     wdt->drv_data = (const struct max77620_variant *)id->driver_data;
>>     /* ... assign other wdt fields ... */
>> +  strlcpy(wdt_dev->info.identity, id->name, \
>> +          sizeof(wdt_dev->info.identity));
>> +  wdt_dev->info.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | \
>> +                          WDIOF_MAGICCLOSE;
>>
> For example, yes.
> 
>> Finally, what about simply:
>>
>>   static const struct max77620_variant max77620_wdt_data = {
>>     .wdt_info = {
>> -        .identity = "max77620-watchdog",
>> +        .identity = "max77xxx-watchdog",
>>         .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | ...
>>     },
>>
>> and always use that struct unconditionally? The max63xx_wdt.c driver
>> seems to do that. Or, if this is an issue for backward compatibility (is
>> it?), just leave max77620_wdt_data and the .identity field will always
>> be "max77620-watchdog" even when using a MAX77714.

...I'll go for this last, simplest option: same struct, same string, always.

-- 
Luca