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.
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 ?
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.
Also ok with me.
Guenter
