On 11/29/21 1:24 PM, Luca Ceresoli wrote:
[ ... ]

quoted

quoted

  
+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

quoted

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 ?

quoted

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