Hi Guenter,

On 29/11/21 22:56, Guenter Roeck wrote:

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

quoted

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.

quoted

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.

quoted

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 ?

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...

quoted

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.

quoted

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