| Message ID | 20211120155707.4019487-1-luca@lucaceresoli.net |
|---|---|
| Headers | show |
| Series | Add MAX77714 PMIC minimal driver (RTC and watchdog only) | expand |
On 20/11/2021 16:57, Luca Ceresoli wrote: > Add a simple driver for the Maxim MAX77714 PMIC, supporting RTC and > watchdog only. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > --- > > Changes in v4: none > > Changes in v3: > - Suggested by Lee Jones: > - move struct mfd_cell to top of file > - remove struct max77714 and its kmalloc, not used after probe > - reword error messages > - add "/* pF */" onto the end of the load_cap line > > Changes in v2: > - fix "watchdog" word in heading comment (Guenter Roeck) > - move struct max77714 to .c file (Krzysztof Kozlowski) > - change include guard format (Krzysztof Kozlowski) > - allow building as a module (Krzysztof Kozlowski) > - remove of_match_ptr usage (Krzysztof Kozlowski / lkp) > (Reported-by: kernel test robot <lkp@intel.com>) > --- > MAINTAINERS | 2 + > drivers/mfd/Kconfig | 14 ++++ > drivers/mfd/Makefile | 1 + > drivers/mfd/max77714.c | 152 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77714.h | 60 ++++++++++++++ > 5 files changed, 229 insertions(+) > create mode 100644 drivers/mfd/max77714.c > create mode 100644 include/linux/mfd/max77714.h > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
On Sat, Nov 20, 2021 at 04:57:05PM +0100, Luca Ceresoli wrote: > The MAX77714 is a MFD chip whose watchdog has the same programming > procedures as the MAX77620 watchdog, but most register offsets and bit > masks are different, as well as some features. > > Support the MAX77714 watchdog by adding a variant description table holding > the differences. > > All the features implemented by this driver are available on the MAX77714 > except for the lack of a WDTOFFC bit. Instead of using a "HAS_*" flag we > handle this by holding in the cnfg_glbl2_cfg_bits struct field the bits > (i.e. the features) to enable in the CNFG_GLBL2 register. These bits differ > among the two models. This implementation allows to avoid any conditional > code, keeping the execution flow unchanged. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > --- > > This patch is new in v4. It replaces v3 patch 7 ("watchdog: max77714: add > driver for the watchdog in the MAX77714 PMIC") by adding MAX77714 wdog > support to the existing MAX77620 wdog driver instead of adding a new > driver. Suggested by Guenter Roeck and Krzysztof Kozlowski. > --- > drivers/watchdog/Kconfig | 2 +- > drivers/watchdog/max77620_wdt.c | 96 +++++++++++++++++++++++++-------- > 2 files changed, 75 insertions(+), 23 deletions(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index a6d97f30325a..f920ad271dde 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -677,7 +677,7 @@ config MAX63XX_WATCHDOG > > config MAX77620_WATCHDOG > tristate "Maxim Max77620 Watchdog Timer" > - depends on MFD_MAX77620 || COMPILE_TEST > + depends on MFD_MAX77620 || MFD_MAX77714 || COMPILE_TEST > select WATCHDOG_CORE > help > This is the driver for the Max77620 watchdog timer. > diff --git a/drivers/watchdog/max77620_wdt.c b/drivers/watchdog/max77620_wdt.c > index be6a53c30002..06b48295fab6 100644 > --- a/drivers/watchdog/max77620_wdt.c > +++ b/drivers/watchdog/max77620_wdt.c > @@ -3,8 +3,10 @@ > * Maxim MAX77620 Watchdog Driver > * > * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved. > + * Copyright (C) 2021 Luca Ceresoli > * > * Author: Laxman Dewangan <ldewangan@nvidia.com> > + * Author: Luca Ceresoli <luca@lucaceresoli.net> > */ > > #include <linux/err.h> > @@ -13,6 +15,7 @@ > #include <linux/module.h> > #include <linux/mod_devicetable.h> > #include <linux/mfd/max77620.h> > +#include <linux/mfd/max77714.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/slab.h> > @@ -20,17 +23,66 @@ > > static bool nowayout = WATCHDOG_NOWAYOUT; > > +/** > + * struct max77620_variant - Data specific to a chip variant > + * @wdt_info: watchdog descriptor > + * @reg_onoff_cnfg2: ONOFF_CNFG2 register offset > + * @reg_cnfg_glbl2: CNFG_GLBL2 register offset > + * @reg_cnfg_glbl3: CNFG_GLBL3 register offset > + * @wdtc_mask: WDTC bit mask in CNFG_GLBL3 (=bits to update to ping the watchdog) > + * @bit_wd_rst_wk: WD_RST_WK bit offset within ONOFF_CNFG2 > + * @cnfg_glbl2_cfg_bits: configuration bits to enable in CNFG_GLBL2 register > + */ > +struct max77620_variant { > + const struct watchdog_info wdt_info; > + u8 reg_onoff_cnfg2; > + u8 reg_cnfg_glbl2; > + u8 reg_cnfg_glbl3; > + u8 wdtc_mask; > + u8 bit_wd_rst_wk; > + u8 cnfg_glbl2_cfg_bits; > +}; > + > struct max77620_wdt { > struct device *dev; > struct regmap *rmap; > + const struct max77620_variant *drv_data; > struct watchdog_device wdt_dev; > }; > > +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. Either keep the current identity string, mark max77620_wdt_info as __ro_after_init and overwrite the identity string there during probe, or add the structure to max77620_wdt and fill it out there. Guenter > + .reg_onoff_cnfg2 = MAX77620_REG_ONOFFCNFG2, > + .reg_cnfg_glbl2 = MAX77620_REG_CNFGGLBL2, > + .reg_cnfg_glbl3 = MAX77620_REG_CNFGGLBL3, > + .wdtc_mask = MAX77620_WDTC_MASK, > + .bit_wd_rst_wk = MAX77620_ONOFFCNFG2_WD_RST_WK, > + /* Set WDT clear in OFF and sleep mode */ > + .cnfg_glbl2_cfg_bits = MAX77620_WDTSLPC | MAX77620_WDTOFFC, > +}; > + > +static const struct max77620_variant max77714_wdt_data = { > + .wdt_info = { > + .identity = "max77714-watchdog", > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > + }, > + .reg_onoff_cnfg2 = MAX77714_CNFG2_ONOFF, > + .reg_cnfg_glbl2 = MAX77714_CNFG_GLBL2, > + .reg_cnfg_glbl3 = MAX77714_CNFG_GLBL3, > + .wdtc_mask = MAX77714_WDTC, > + .bit_wd_rst_wk = MAX77714_WD_RST_WK, > + /* Set WDT clear in sleep mode (there is no WDTOFFC on MAX77714) */ > + .cnfg_glbl2_cfg_bits = MAX77714_WDTSLPC, > +}; > + > static int max77620_wdt_start(struct watchdog_device *wdt_dev) > { > struct max77620_wdt *wdt = watchdog_get_drvdata(wdt_dev); > > - return regmap_update_bits(wdt->rmap, MAX77620_REG_CNFGGLBL2, > + return regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, > MAX77620_WDTEN, MAX77620_WDTEN); > } > > @@ -38,7 +90,7 @@ static int max77620_wdt_stop(struct watchdog_device *wdt_dev) > { > struct max77620_wdt *wdt = watchdog_get_drvdata(wdt_dev); > > - return regmap_update_bits(wdt->rmap, MAX77620_REG_CNFGGLBL2, > + return regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, > MAX77620_WDTEN, 0); > } > > @@ -46,8 +98,8 @@ static int max77620_wdt_ping(struct watchdog_device *wdt_dev) > { > struct max77620_wdt *wdt = watchdog_get_drvdata(wdt_dev); > > - return regmap_update_bits(wdt->rmap, MAX77620_REG_CNFGGLBL3, > - MAX77620_WDTC_MASK, 0x1); > + return regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl3, > + wdt->drv_data->wdtc_mask, 0x1); > } > > static int max77620_wdt_set_timeout(struct watchdog_device *wdt_dev, > @@ -80,12 +132,12 @@ static int max77620_wdt_set_timeout(struct watchdog_device *wdt_dev, > break; > } > > - ret = regmap_update_bits(wdt->rmap, MAX77620_REG_CNFGGLBL3, > - MAX77620_WDTC_MASK, 0x1); > + ret = regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl3, > + wdt->drv_data->wdtc_mask, 0x1); > if (ret < 0) > return ret; > > - ret = regmap_update_bits(wdt->rmap, MAX77620_REG_CNFGGLBL2, > + ret = regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, > MAX77620_TWD_MASK, regval); > if (ret < 0) > return ret; > @@ -95,11 +147,6 @@ static int max77620_wdt_set_timeout(struct watchdog_device *wdt_dev, > return 0; > } > > -static const struct watchdog_info max77620_wdt_info = { > - .identity = "max77620-watchdog", > - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > -}; > - > static const struct watchdog_ops max77620_wdt_ops = { > .start = max77620_wdt_start, > .stop = max77620_wdt_stop, > @@ -109,6 +156,7 @@ static const struct watchdog_ops max77620_wdt_ops = { > > static int max77620_wdt_probe(struct platform_device *pdev) > { > + const struct platform_device_id *id = platform_get_device_id(pdev); > struct device *dev = &pdev->dev; > struct max77620_wdt *wdt; > struct watchdog_device *wdt_dev; > @@ -120,6 +168,8 @@ static int max77620_wdt_probe(struct platform_device *pdev) > return -ENOMEM; > > wdt->dev = dev; > + wdt->drv_data = (const struct max77620_variant *) id->driver_data; > + > wdt->rmap = dev_get_regmap(dev->parent, NULL); > if (!wdt->rmap) { > dev_err(wdt->dev, "Failed to get parent regmap\n"); > @@ -127,7 +177,7 @@ static int max77620_wdt_probe(struct platform_device *pdev) > } > > wdt_dev = &wdt->wdt_dev; > - wdt_dev->info = &max77620_wdt_info; > + wdt_dev->info = &wdt->drv_data->wdt_info; > wdt_dev->ops = &max77620_wdt_ops; > wdt_dev->min_timeout = 2; > wdt_dev->max_timeout = 128; > @@ -136,25 +186,25 @@ static int max77620_wdt_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, wdt); > > /* Enable WD_RST_WK - WDT expire results in a restart */ > - ret = regmap_update_bits(wdt->rmap, MAX77620_REG_ONOFFCNFG2, > - MAX77620_ONOFFCNFG2_WD_RST_WK, > - MAX77620_ONOFFCNFG2_WD_RST_WK); > + ret = regmap_update_bits(wdt->rmap, wdt->drv_data->reg_onoff_cnfg2, > + wdt->drv_data->bit_wd_rst_wk, > + wdt->drv_data->bit_wd_rst_wk); > if (ret < 0) { > dev_err(wdt->dev, "Failed to set WD_RST_WK: %d\n", ret); > return ret; > } > > - /* Set WDT clear in OFF and sleep mode */ > - ret = regmap_update_bits(wdt->rmap, MAX77620_REG_CNFGGLBL2, > - MAX77620_WDTOFFC | MAX77620_WDTSLPC, > - MAX77620_WDTOFFC | MAX77620_WDTSLPC); > + /* Set the "auto WDT clear" bits available on the chip */ > + ret = regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, > + wdt->drv_data->cnfg_glbl2_cfg_bits, > + wdt->drv_data->cnfg_glbl2_cfg_bits); > if (ret < 0) { > dev_err(wdt->dev, "Failed to set WDT OFF mode: %d\n", ret); > return ret; > } > > /* Check if WDT running and if yes then set flags properly */ > - ret = regmap_read(wdt->rmap, MAX77620_REG_CNFGGLBL2, ®val); > + ret = regmap_read(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, ®val); > if (ret < 0) { > dev_err(wdt->dev, "Failed to read WDT CFG register: %d\n", ret); > return ret; > @@ -186,7 +236,8 @@ static int max77620_wdt_probe(struct platform_device *pdev) > } > > static const struct platform_device_id max77620_wdt_devtype[] = { > - { .name = "max77620-watchdog", }, > + { "max77620-watchdog", (kernel_ulong_t)&max77620_wdt_data }, > + { "max77714-watchdog", (kernel_ulong_t)&max77714_wdt_data }, > { }, > }; > MODULE_DEVICE_TABLE(platform, max77620_wdt_devtype); > @@ -208,4 +259,5 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started " > "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>"); > +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>"); > MODULE_LICENSE("GPL v2");
On Sat, Nov 20, 2021 at 04:57:06PM +0100, Luca Ceresoli wrote: > Clarify why we need to ping the watchdog before changing the timeout by > quoting the MAX77714 datasheet. > Unless I am missing something, this adds confusion instead of clarifying anything, and it is misleading. The added comment in the code makes it sound like clearing the watchdog timer is only needed for MAX77614. However, the code was in place for MAX77620, suggesting that it was needed for that chip as well and is not MAX77614 specific. Please either drop this patch or rephrase it to clarify that it applies to both chips. Guenter > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > --- > > This patch is new in v4. It adds a clarification comment to the max77620 > driver taken from v3 patch 7. > --- > drivers/watchdog/max77620_wdt.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/watchdog/max77620_wdt.c b/drivers/watchdog/max77620_wdt.c > index 06b48295fab6..f082a4ea2c03 100644 > --- a/drivers/watchdog/max77620_wdt.c > +++ b/drivers/watchdog/max77620_wdt.c > @@ -132,6 +132,11 @@ static int max77620_wdt_set_timeout(struct watchdog_device *wdt_dev, > break; > } > > + /* > + * "If the value of TWD needs to be changed, clear the system > + * watchdog timer first [...], then change the value of TWD." > + * (MAX77714 datasheet) > + */ > ret = regmap_update_bits(wdt->rmap, wdt->drv_data->reg_cnfg_glbl3, > wdt->drv_data->wdtc_mask, 0x1); > if (ret < 0)
Hi Guenter, On 29/11/21 17:04, Guenter Roeck wrote: > On Sat, Nov 20, 2021 at 04:57:06PM +0100, Luca Ceresoli wrote: >> Clarify why we need to ping the watchdog before changing the timeout by >> quoting the MAX77714 datasheet. >> > > Unless I am missing something, this adds confusion instead of clarifying > anything, and it is misleading. The added comment in the code makes it > sound like clearing the watchdog timer is only needed for MAX77614. > However, the code was in place for MAX77620, suggesting that it was needed > for that chip as well and is not MAX77614 specific. You're right, the comment comes from the max77714-only driver, but now that it is in a multi-chip driver the confusion started to exist. > Please either drop this patch or rephrase it to clarify that it applies > to both chips. What if I rephrase to: /* * "If the value of TWD needs to be changed, clear the system * watchdog timer first [...], then change the value of TWD." - * (MAX77714 datasheet) + * (MAX77714 datasheet but applies to MAX77620 too) */ ?
On 11/29/21 8:08 AM, Luca Ceresoli wrote: > Hi Guenter, > > On 29/11/21 17:04, Guenter Roeck wrote: >> On Sat, Nov 20, 2021 at 04:57:06PM +0100, Luca Ceresoli wrote: >>> Clarify why we need to ping the watchdog before changing the timeout by >>> quoting the MAX77714 datasheet. >>> >> >> Unless I am missing something, this adds confusion instead of clarifying >> anything, and it is misleading. The added comment in the code makes it >> sound like clearing the watchdog timer is only needed for MAX77614. >> However, the code was in place for MAX77620, suggesting that it was needed >> for that chip as well and is not MAX77614 specific. > > You're right, the comment comes from the max77714-only driver, but now > that it is in a multi-chip driver the confusion started to exist. > >> Please either drop this patch or rephrase it to clarify that it applies >> to both chips. > > What if I rephrase to: > > /* > * "If the value of TWD needs to be changed, clear the system > * watchdog timer first [...], then change the value of TWD." > - * (MAX77714 datasheet) > + * (MAX77714 datasheet but applies to MAX77620 too) > */ > Sounds good. Guenter
Hi Guenter, thanks for you review! On 29/11/21 16:53, Guenter Roeck wrote: > On Sat, Nov 20, 2021 at 04:57:05PM +0100, Luca Ceresoli wrote: >> The MAX77714 is a MFD chip whose watchdog has the same programming >> procedures as the MAX77620 watchdog, but most register offsets and bit >> masks are different, as well as some features. >> >> Support the MAX77714 watchdog by adding a variant description table holding >> the differences. >> >> All the features implemented by this driver are available on the MAX77714 >> except for the lack of a WDTOFFC bit. Instead of using a "HAS_*" flag we >> handle this by holding in the cnfg_glbl2_cfg_bits struct field the bits >> (i.e. the features) to enable in the CNFG_GLBL2 register. These bits differ >> among the two models. This implementation allows to avoid any conditional >> code, keeping the execution flow unchanged. >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> --- >> >> This patch is new in v4. It replaces v3 patch 7 ("watchdog: max77714: add >> driver for the watchdog in the MAX77714 PMIC") by adding MAX77714 wdog >> support to the existing MAX77620 wdog driver instead of adding a new >> driver. Suggested by Guenter Roeck and Krzysztof Kozlowski. >> --- >> drivers/watchdog/Kconfig | 2 +- >> drivers/watchdog/max77620_wdt.c | 96 +++++++++++++++++++++++++-------- >> 2 files changed, 75 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index a6d97f30325a..f920ad271dde 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -677,7 +677,7 @@ config MAX63XX_WATCHDOG >> >> config MAX77620_WATCHDOG >> tristate "Maxim Max77620 Watchdog Timer" >> - depends on MFD_MAX77620 || COMPILE_TEST >> + depends on MFD_MAX77620 || MFD_MAX77714 || COMPILE_TEST >> select WATCHDOG_CORE >> help >> This is the driver for the Max77620 watchdog timer. >> diff --git a/drivers/watchdog/max77620_wdt.c b/drivers/watchdog/max77620_wdt.c >> index be6a53c30002..06b48295fab6 100644 >> --- a/drivers/watchdog/max77620_wdt.c >> +++ b/drivers/watchdog/max77620_wdt.c >> @@ -3,8 +3,10 @@ >> * Maxim MAX77620 Watchdog Driver >> * >> * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved. >> + * Copyright (C) 2021 Luca Ceresoli >> * >> * Author: Laxman Dewangan <ldewangan@nvidia.com> >> + * Author: Luca Ceresoli <luca@lucaceresoli.net> >> */ >> >> #include <linux/err.h> >> @@ -13,6 +15,7 @@ >> #include <linux/module.h> >> #include <linux/mod_devicetable.h> >> #include <linux/mfd/max77620.h> >> +#include <linux/mfd/max77714.h> >> #include <linux/platform_device.h> >> #include <linux/regmap.h> >> #include <linux/slab.h> >> @@ -20,17 +23,66 @@ >> >> static bool nowayout = WATCHDOG_NOWAYOUT; >> >> +/** >> + * struct max77620_variant - Data specific to a chip variant >> + * @wdt_info: watchdog descriptor >> + * @reg_onoff_cnfg2: ONOFF_CNFG2 register offset >> + * @reg_cnfg_glbl2: CNFG_GLBL2 register offset >> + * @reg_cnfg_glbl3: CNFG_GLBL3 register offset >> + * @wdtc_mask: WDTC bit mask in CNFG_GLBL3 (=bits to update to ping the watchdog) >> + * @bit_wd_rst_wk: WD_RST_WK bit offset within ONOFF_CNFG2 >> + * @cnfg_glbl2_cfg_bits: configuration bits to enable in CNFG_GLBL2 register >> + */ >> +struct max77620_variant { >> + const struct watchdog_info wdt_info; >> + u8 reg_onoff_cnfg2; >> + u8 reg_cnfg_glbl2; >> + u8 reg_cnfg_glbl3; >> + u8 wdtc_mask; >> + u8 bit_wd_rst_wk; >> + u8 cnfg_glbl2_cfg_bits; >> +}; >> + >> struct max77620_wdt { >> struct device *dev; >> struct regmap *rmap; >> + const struct max77620_variant *drv_data; >> struct watchdog_device wdt_dev; >> }; >> >> +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. Second: it's not clear how you expect it to be done. Looking into the 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. > 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; 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. Thanks for you patience in reading so far.
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
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.
Hi, this series adds minimal drivers for the Maxim Semiconductor MAX77714 (https://www.maximintegrated.com/en/products/power/power-management-ics/MAX77714.html). Only RTC and watchdog are implemented by these patches. All implemented functionality is tested and working: RTC read/write, watchdog start/stop/ping/set_timeout. Patches 1-3 + 6 are trivial cleanups to the max77686 drivers and Kconfig indentation and can probably be applied easily. Patches 4, 5, 7, 8 and 9 add: dt bindings, mfd driver, watchdog driver and rtc driver. Changes in v4: - do not add a new wdog driver for MAX77714, extend the MAX77620 wdog driver; this means removing v3 patch 7, now replaced by patches 7+8 - added review tags Changes in v3: - fixed all issues reported on v1 patches - removed patch 1 of v2, already applied ("mfd: max77686: Correct tab-based alignment of register addresses") Changes in v2: - fixed all issues reported on v1 patches - added patch 7 ("watchdog: Kconfig: fix help text indentation") - additional minor improvements Luca Luca Ceresoli (9): rtc: max77686: convert comments to kernel-doc format rtc: max77686: rename day-of-month defines rtc: max77686: remove unused code to read in 12-hour mode dt-bindings: mfd: add Maxim MAX77714 PMIC mfd: max77714: Add driver for Maxim MAX77714 PMIC watchdog: Kconfig: fix help text indentation watchdog: max77620: add support for the max77714 variant watchdog: max77620: add comment to clarify set_timeout procedure rtc: max77686: add MAX77714 support .../bindings/mfd/maxim,max77714.yaml | 68 ++++++++ MAINTAINERS | 7 + drivers/mfd/Kconfig | 14 ++ drivers/mfd/Makefile | 1 + drivers/mfd/max77686.c | 2 +- drivers/mfd/max77714.c | 152 ++++++++++++++++++ drivers/rtc/Kconfig | 2 +- drivers/rtc/rtc-max77686.c | 75 +++++---- drivers/watchdog/Kconfig | 50 +++--- drivers/watchdog/max77620_wdt.c | 101 +++++++++--- include/linux/mfd/max77686-private.h | 4 +- include/linux/mfd/max77714.h | 60 +++++++ 12 files changed, 455 insertions(+), 81 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max77714.yaml create mode 100644 drivers/mfd/max77714.c create mode 100644 include/linux/mfd/max77714.h