| Message ID | 20211111225852.3128201-1-luca@lucaceresoli.net |
|---|---|
| Headers | show |
| Series | Add MAX77714 PMIC minimal driver (RTC and watchdog only) | expand |
On 11/11/21 2:58 PM, Luca Ceresoli wrote: > Some entries indent their help text with 1 tab + 1 space or 1 tab only > instead of 1 tab + 2 spaces. Add the missing spaces. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > --- > > Changes in v3: none > --- > drivers/watchdog/Kconfig | 48 ++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 24 deletions(-) > Acked-by: Randy Dunlap <rdunlap@infradead.org> Thanks.
On 11/11/21 2:58 PM, Luca Ceresoli wrote: > Convert the comments documenting this struct to kernel-doc format for > standardization and readability. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > Changes in v3: none > > Changes in v2: none > --- > drivers/rtc/rtc-max77686.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > Reviewed-by: Randy Dunlap <rdunlap@infradead.org> Thanks.
On 11/11/21 2:58 PM, Luca Ceresoli wrote: > Add a simple driver to support the watchdog embedded in the Maxim MAX77714 > PMIC. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > I just realized that this is effectively a rewrite of drivers/watchdog/max77620_wdt.c. The only difference I can see is is the register offsets (0x91 and 0x92 vs. 1 and 2) and some implementation details. Please add support for this watchdog to the other driver or provide a _really_ good reason why that is not possible. > --- > > Changes in v3: none > > Changes in v2: > - fix Kconfig help indentation (Randy Dunlap) > - make max77714_margin_value static const (Guenter Roeck) > - fix platform module instantiation > --- > MAINTAINERS | 1 + > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/max77714_wdt.c | 179 ++++++++++++++++++++++++++++++++ > 4 files changed, 190 insertions(+) > create mode 100644 drivers/watchdog/max77714_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 1a37b9422c5f..d182231b4bbf 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11573,6 +11573,7 @@ M: Luca Ceresoli <luca@lucaceresoli.net> > S: Maintained > F: Documentation/devicetree/bindings/mfd/maxim,max77714.yaml > F: drivers/mfd/max77714.c > +F: drivers/watchdog/max77714_wdt.c > F: include/linux/mfd/max77714.h > > MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index a6d97f30325a..f5100b731927 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -685,6 +685,15 @@ config MAX77620_WATCHDOG > MAX77620 chips. To compile this driver as a module, > choose M here: the module will be called max77620_wdt. > > +config MAX77714_WATCHDOG > + tristate "Maxim MAX77714 Watchdog Timer" > + depends on MFD_MAX77714 || COMPILE_TEST > + help > + This is the driver for watchdog timer in the MAX77714 PMIC. > + Say 'Y' here to enable the watchdog timer support for MAX77714 > + chips. To compile this driver as a module, choose M here: the > + module will be called max77714_wdt. > + > config IMX2_WDT > tristate "IMX2+ Watchdog" > depends on ARCH_MXC || ARCH_LAYERSCAPE || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 2ee97064145b..575be33c52b8 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -214,6 +214,7 @@ obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o > obj-$(CONFIG_MAX63XX_WATCHDOG) += max63xx_wdt.o > obj-$(CONFIG_MAX77620_WATCHDOG) += max77620_wdt.o > +obj-$(CONFIG_MAX77714_WATCHDOG) += max77714_wdt.o > obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o > obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o > obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o > diff --git a/drivers/watchdog/max77714_wdt.c b/drivers/watchdog/max77714_wdt.c > new file mode 100644 > index 000000000000..cce6c13d76eb > --- /dev/null > +++ b/drivers/watchdog/max77714_wdt.c > @@ -0,0 +1,179 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Maxim MAX77714 Watchdog Driver > + * > + * Copyright (C) 2021 Luca Ceresoli > + * Author: Luca Ceresoli <luca@lucaceresoli.net> > + */ > + > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/mfd/max77714.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/watchdog.h> > + > +struct max77714_wdt { > + struct device *dev; > + struct regmap *rmap; > + struct watchdog_device wd_dev; > +}; > + > +/* Timeout in seconds, indexed by TWD bits of CNFG_GLBL2 register */ > +static const unsigned int max77714_margin_value[] = { 2, 16, 64, 128 }; > + > +static int max77714_wdt_start(struct watchdog_device *wd_dev) > +{ > + struct max77714_wdt *wdt = watchdog_get_drvdata(wd_dev); > + > + return regmap_update_bits(wdt->rmap, MAX77714_CNFG_GLBL2, > + MAX77714_WDTEN, MAX77714_WDTEN); > +} > + > +static int max77714_wdt_stop(struct watchdog_device *wd_dev) > +{ > + struct max77714_wdt *wdt = watchdog_get_drvdata(wd_dev); > + > + return regmap_update_bits(wdt->rmap, MAX77714_CNFG_GLBL2, > + MAX77714_WDTEN, 0); > +} > + > +static int max77714_wdt_ping(struct watchdog_device *wd_dev) > +{ > + struct max77714_wdt *wdt = watchdog_get_drvdata(wd_dev); > + > + return regmap_update_bits(wdt->rmap, MAX77714_CNFG_GLBL3, > + MAX77714_WDTC, 1); > +} > + > +static int max77714_wdt_set_timeout(struct watchdog_device *wd_dev, > + unsigned int timeout) > +{ > + struct max77714_wdt *wdt = watchdog_get_drvdata(wd_dev); > + unsigned int new_timeout, new_twd; > + int err; > + > + for (new_twd = 0; new_twd < ARRAY_SIZE(max77714_margin_value) - 1; new_twd++) > + if (timeout <= max77714_margin_value[new_twd]) > + break; > + > + /* new_wdt is not out of bounds here due to the "- 1" in the for loop */ > + new_timeout = max77714_margin_value[new_twd]; > + > + /* > + * "If the value of TWD needs to be changed, clear the system > + * watchdog timer first [...], then change the value of TWD." > + * (MAX77714 datasheet) > + */ > + err = regmap_update_bits(wdt->rmap, MAX77714_CNFG_GLBL3, > + MAX77714_WDTC, 1); > + if (err) > + return err; > + > + err = regmap_update_bits(wdt->rmap, MAX77714_CNFG_GLBL2, > + MAX77714_TWD_MASK, new_twd); > + if (err) > + return err; > + > + wd_dev->timeout = new_timeout; > + > + dev_dbg(wdt->dev, "New timeout = %u s (WDT = 0x%x)", new_timeout, new_twd); > + > + return 0; > +} > + > +static const struct watchdog_info max77714_wdt_info = { > + .identity = "max77714-watchdog", > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > +}; > + > +static const struct watchdog_ops max77714_wdt_ops = { > + .start = max77714_wdt_start, > + .stop = max77714_wdt_stop, > + .ping = max77714_wdt_ping, > + .set_timeout = max77714_wdt_set_timeout, > +}; > + > +static int max77714_wdt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct max77714_wdt *wdt; > + struct watchdog_device *wd_dev; > + unsigned int regval; > + int err; > + > + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt->dev = dev; > + > + wd_dev = &wdt->wd_dev; > + wd_dev->info = &max77714_wdt_info; > + wd_dev->ops = &max77714_wdt_ops; > + wd_dev->min_timeout = 2; > + wd_dev->max_timeout = 128; > + > + platform_set_drvdata(pdev, wdt); > + watchdog_set_drvdata(wd_dev, wdt); > + > + wdt->rmap = dev_get_regmap(dev->parent, NULL); > + if (!wdt->rmap) > + return dev_err_probe(wdt->dev, -ENODEV, "Failed to get parent regmap\n"); > + > + /* WD_RST_WK: if 1 wdog restarts; if 0 wdog shuts down */ > + err = regmap_update_bits(wdt->rmap, MAX77714_CNFG2_ONOFF, > + MAX77714_WD_RST_WK, MAX77714_WD_RST_WK); > + if (err) > + return dev_err_probe(wdt->dev, err, "Error updating CNFG2_ONOFF\n"); > + > + err = regmap_read(wdt->rmap, MAX77714_CNFG_GLBL2, ®val); > + if (err) > + return dev_err_probe(wdt->dev, err, "Error reading CNFG_GLBL2\n"); > + > + /* enable watchdog | enable auto-clear in sleep state */ > + regval |= (MAX77714_WDTEN | MAX77714_WDTSLPC); > + > + err = regmap_write(wdt->rmap, MAX77714_CNFG_GLBL2, regval); > + if (err) > + return dev_err_probe(wdt->dev, err, "Error writing CNFG_GLBL2\n"); > + > + wd_dev->timeout = max77714_margin_value[regval & MAX77714_TWD_MASK]; > + > + dev_dbg(wdt->dev, "Timeout = %u s (WDT = 0x%x)", > + wd_dev->timeout, regval & MAX77714_TWD_MASK); > + > + set_bit(WDOG_HW_RUNNING, &wd_dev->status); > + > + watchdog_stop_on_unregister(wd_dev); > + > + err = devm_watchdog_register_device(dev, wd_dev); > + if (err) > + return dev_err_probe(dev, err, "Cannot register watchdog device\n"); > + > + dev_info(dev, "registered as /dev/watchdog%d\n", wd_dev->id); > + > + return 0; > +} > + > +static const struct platform_device_id max77714_wdt_platform_id[] = { > + { .name = "max77714-watchdog", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(platform, max77714_wdt_platform_id); > + > +static struct platform_driver max77714_wdt_driver = { > + .driver = { > + .name = "max77714-watchdog", > + }, > + .probe = max77714_wdt_probe, > + .id_table = max77714_wdt_platform_id, > +}; > + > +module_platform_driver(max77714_wdt_driver); > + > +MODULE_DESCRIPTION("MAX77714 watchdog timer driver"); > +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>"); > +MODULE_LICENSE("GPL v2"); >
Hi Guenter, On 12/11/21 15:57, Guenter Roeck wrote: > On 11/11/21 2:58 PM, Luca Ceresoli wrote: >> Add a simple driver to support the watchdog embedded in the Maxim >> MAX77714 >> PMIC. >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> > > I just realized that this is effectively a rewrite of > drivers/watchdog/max77620_wdt.c. > The only difference I can see is is the register offsets (0x91 and 0x92 > vs. 1 and 2) and some implementation details. Please add support for this > watchdog to the other driver or provide a _really_ good reason why that > is not possible. I initially started developing MAX77714 watchdog support as an addition to max77620_wdt.c as the procedures look identical at least for the basic features. But the register content seems completely different. Here are the notes I took at that time: -------------------------8<------------------------- MAX77620 has reg ONOFFCNFG1 at 0x41, ONOFFCNFG2 at 0x42. MAX77714 has reg CNFG1_ONOFF at 0x93, CNFG2_ONOFF at 0x94. OK, we can handle this with a register indirection table, indexed by chip model. MAX77620 has MAX77620_REG_FPS_CFG0 register. On MAX77714 I was unable to find any such register (I haven't looked at FPS in detail though). OK, we can handle this with some if()s or entirely disable PM on the 77714 until anybody cares. MAX77620 ONOFFCNFG1 has SFT_RST in bit 7. MAX77714 CNFG1_ONOFF has SFT_RST is bit 6. Uhm, should we have a _bit_ indirection table in addition to the _register_ indirection table? MAX77620 ONOFFCNFG2 bit 5 is SLP_LPM_MSK, involved in FPS. MAX77620 ONOFFCNFG2 bit 6 is WD_RTS_WK, configures the watchdog timer. MAX77714 CNFG2_ONOFF bit 5 is WD_RTS_WK, configures the watchdog timer. On MAX77714 I haven't found SLP_LPM_MSK. MAX77620 has 6 CID registers with "ES version" in CID5. MAX77714 has 5 CID registers with "DEVICE id" in CID3. CID registers would be useful to get the chip model directly from the chip, if only they had the same structure. Almost all of the registers I have been looking into have similar differences. -------------------------8<------------------------- When I started adding indirection tables the driver started growing bigger and uglier, and that little simple driver started being big and complex. So I opted to add a new driver. Let me know whether that sounds OK.
On 12/11/2021 17:02, Luca Ceresoli wrote: > Hi Guenter, > > On 12/11/21 15:57, Guenter Roeck wrote: >> On 11/11/21 2:58 PM, Luca Ceresoli wrote: >>> Add a simple driver to support the watchdog embedded in the Maxim >>> MAX77714 >>> PMIC. >>> >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>> >> >> I just realized that this is effectively a rewrite of >> drivers/watchdog/max77620_wdt.c. >> The only difference I can see is is the register offsets (0x91 and 0x92 >> vs. 1 and 2) and some implementation details. Please add support for this >> watchdog to the other driver or provide a _really_ good reason why that >> is not possible. > > I initially started developing MAX77714 watchdog support as an addition > to max77620_wdt.c as the procedures look identical at least for the > basic features. > > But the register content seems completely different. Here are the notes > I took at that time: > > -------------------------8<------------------------- > > MAX77620 has reg ONOFFCNFG1 at 0x41, ONOFFCNFG2 at 0x42. > MAX77714 has reg CNFG1_ONOFF at 0x93, CNFG2_ONOFF at 0x94. > OK, we can handle this with a register indirection table, indexed by > chip model. > > MAX77620 has MAX77620_REG_FPS_CFG0 register. > On MAX77714 I was unable to find any such register (I haven't looked at > FPS in detail though). > OK, we can handle this with some if()s or entirely disable PM on the > 77714 until anybody cares. > > MAX77620 ONOFFCNFG1 has SFT_RST in bit 7. > MAX77714 CNFG1_ONOFF has SFT_RST is bit 6. > Uhm, should we have a _bit_ indirection table in addition to the > _register_ indirection table? > > MAX77620 ONOFFCNFG2 bit 5 is SLP_LPM_MSK, involved in FPS. > MAX77620 ONOFFCNFG2 bit 6 is WD_RTS_WK, configures the watchdog timer. > MAX77714 CNFG2_ONOFF bit 5 is WD_RTS_WK, configures the watchdog timer. > On MAX77714 I haven't found SLP_LPM_MSK. > > MAX77620 has 6 CID registers with "ES version" in CID5. > MAX77714 has 5 CID registers with "DEVICE id" in CID3. > CID registers would be useful to get the chip model directly from the > chip, if only they had the same structure. > > Almost all of the registers I have been looking into have similar > differences. > > -------------------------8<------------------------- > > When I started adding indirection tables the driver started growing > bigger and uglier, and that little simple driver started being big and > complex. So I opted to add a new driver. > The register offset differences are trivial and we do it in several drivers. Also in rtc-max77686 used by you here. Lack of features as well - just have a variant/driver data which defines certain features (true/false) or quirk bits (see s3c2410_wdt). The second driver - s3c2410_wdt - also customizes the bits. Therefore if the generic device operating configuration is similar (same generic control flow) and differences are in bits and offsets, then it should be one driver. Best regards, Krzysztof
On 11/12/21 8:07 AM, Krzysztof Kozlowski wrote: > On 12/11/2021 17:02, Luca Ceresoli wrote: >> Hi Guenter, >> >> On 12/11/21 15:57, Guenter Roeck wrote: >>> On 11/11/21 2:58 PM, Luca Ceresoli wrote: >>>> Add a simple driver to support the watchdog embedded in the Maxim >>>> MAX77714 >>>> PMIC. >>>> >>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>>> >>> >>> I just realized that this is effectively a rewrite of >>> drivers/watchdog/max77620_wdt.c. >>> The only difference I can see is is the register offsets (0x91 and 0x92 >>> vs. 1 and 2) and some implementation details. Please add support for this >>> watchdog to the other driver or provide a _really_ good reason why that >>> is not possible. >> >> I initially started developing MAX77714 watchdog support as an addition >> to max77620_wdt.c as the procedures look identical at least for the >> basic features. >> >> But the register content seems completely different. Here are the notes >> I took at that time: >> >> -------------------------8<------------------------- >> >> MAX77620 has reg ONOFFCNFG1 at 0x41, ONOFFCNFG2 at 0x42. >> MAX77714 has reg CNFG1_ONOFF at 0x93, CNFG2_ONOFF at 0x94. >> OK, we can handle this with a register indirection table, indexed by >> chip model. >> >> MAX77620 has MAX77620_REG_FPS_CFG0 register. >> On MAX77714 I was unable to find any such register (I haven't looked at >> FPS in detail though). >> OK, we can handle this with some if()s or entirely disable PM on the >> 77714 until anybody cares. >> >> MAX77620 ONOFFCNFG1 has SFT_RST in bit 7. >> MAX77714 CNFG1_ONOFF has SFT_RST is bit 6. >> Uhm, should we have a _bit_ indirection table in addition to the >> _register_ indirection table? >> >> MAX77620 ONOFFCNFG2 bit 5 is SLP_LPM_MSK, involved in FPS. >> MAX77620 ONOFFCNFG2 bit 6 is WD_RTS_WK, configures the watchdog timer. >> MAX77714 CNFG2_ONOFF bit 5 is WD_RTS_WK, configures the watchdog timer. >> On MAX77714 I haven't found SLP_LPM_MSK. >> >> MAX77620 has 6 CID registers with "ES version" in CID5. >> MAX77714 has 5 CID registers with "DEVICE id" in CID3. >> CID registers would be useful to get the chip model directly from the >> chip, if only they had the same structure. >> >> Almost all of the registers I have been looking into have similar >> differences. >> >> -------------------------8<------------------------- >> >> When I started adding indirection tables the driver started growing >> bigger and uglier, and that little simple driver started being big and >> complex. So I opted to add a new driver. >> > > The register offset differences are trivial and we do it in several > drivers. Also in rtc-max77686 used by you here. > Lack of features as well - just have a variant/driver data which defines > certain features (true/false) or quirk bits (see s3c2410_wdt). > > The second driver - s3c2410_wdt - also customizes the bits. > > Therefore if the generic device operating configuration is similar (same > generic control flow) and differences are in bits and offsets, then it > should be one driver. > Exactly. Thanks, Guenter
Hi Guenter, Krzysztof, On 12/11/21 20:23, Guenter Roeck wrote: > On 11/12/21 8:07 AM, Krzysztof Kozlowski wrote: >> On 12/11/2021 17:02, Luca Ceresoli wrote: >>> Hi Guenter, >>> >>> On 12/11/21 15:57, Guenter Roeck wrote: >>>> On 11/11/21 2:58 PM, Luca Ceresoli wrote: >>>>> Add a simple driver to support the watchdog embedded in the Maxim >>>>> MAX77714 >>>>> PMIC. >>>>> >>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>>>> >>>> >>>> I just realized that this is effectively a rewrite of >>>> drivers/watchdog/max77620_wdt.c. >>>> The only difference I can see is is the register offsets (0x91 and 0x92 >>>> vs. 1 and 2) and some implementation details. Please add support for >>>> this >>>> watchdog to the other driver or provide a _really_ good reason why that >>>> is not possible. >>> >>> I initially started developing MAX77714 watchdog support as an addition >>> to max77620_wdt.c as the procedures look identical at least for the >>> basic features. >>> >>> But the register content seems completely different. Here are the notes >>> I took at that time: >>> >>> -------------------------8<------------------------- >>> >>> MAX77620 has reg ONOFFCNFG1 at 0x41, ONOFFCNFG2 at 0x42. >>> MAX77714 has reg CNFG1_ONOFF at 0x93, CNFG2_ONOFF at 0x94. >>> OK, we can handle this with a register indirection table, indexed by >>> chip model. >>> >>> MAX77620 has MAX77620_REG_FPS_CFG0 register. >>> On MAX77714 I was unable to find any such register (I haven't looked at >>> FPS in detail though). >>> OK, we can handle this with some if()s or entirely disable PM on the >>> 77714 until anybody cares. >>> >>> MAX77620 ONOFFCNFG1 has SFT_RST in bit 7. >>> MAX77714 CNFG1_ONOFF has SFT_RST is bit 6. >>> Uhm, should we have a _bit_ indirection table in addition to the >>> _register_ indirection table? >>> >>> MAX77620 ONOFFCNFG2 bit 5 is SLP_LPM_MSK, involved in FPS. >>> MAX77620 ONOFFCNFG2 bit 6 is WD_RTS_WK, configures the watchdog timer. >>> MAX77714 CNFG2_ONOFF bit 5 is WD_RTS_WK, configures the watchdog timer. >>> On MAX77714 I haven't found SLP_LPM_MSK. >>> >>> MAX77620 has 6 CID registers with "ES version" in CID5. >>> MAX77714 has 5 CID registers with "DEVICE id" in CID3. >>> CID registers would be useful to get the chip model directly from the >>> chip, if only they had the same structure. >>> >>> Almost all of the registers I have been looking into have similar >>> differences. >>> >>> -------------------------8<------------------------- >>> >>> When I started adding indirection tables the driver started growing >>> bigger and uglier, and that little simple driver started being big and >>> complex. So I opted to add a new driver. >>> >> >> The register offset differences are trivial and we do it in several >> drivers. Also in rtc-max77686 used by you here. >> Lack of features as well - just have a variant/driver data which defines >> certain features (true/false) or quirk bits (see s3c2410_wdt). >> >> The second driver - s3c2410_wdt - also customizes the bits. >> >> Therefore if the generic device operating configuration is similar (same >> generic control flow) and differences are in bits and offsets, then it >> should be one driver. >> > > Exactly. Ok, I'll do that and send v4. Now I realize I should have mentioned from the beginning the reasons for creating a new driver, so this discussion would have been cleared much earlier. Apologies for that. Patches 1-6 + 8 are not impacted and will need no change for this issue.
On Thu, Nov 11, 2021 at 11:58:50PM +0100, Luca Ceresoli wrote: > Some entries indent their help text with 1 tab + 1 space or 1 tab only > instead of 1 tab + 2 spaces. Add the missing spaces. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Acked-by: Randy Dunlap <rdunlap@infradead.org> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > > Changes in v3: none > --- > drivers/watchdog/Kconfig | 48 ++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 9d222ba17ec6..a6d97f30325a 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -680,10 +680,10 @@ config MAX77620_WATCHDOG > depends on MFD_MAX77620 || COMPILE_TEST > select WATCHDOG_CORE > help > - This is the driver for the Max77620 watchdog timer. > - Say 'Y' here to enable the watchdog timer support for > - MAX77620 chips. To compile this driver as a module, > - choose M here: the module will be called max77620_wdt. > + This is the driver for the Max77620 watchdog timer. > + Say 'Y' here to enable the watchdog timer support for > + MAX77620 chips. To compile this driver as a module, > + choose M here: the module will be called max77620_wdt. > > config IMX2_WDT > tristate "IMX2+ Watchdog" > @@ -1440,26 +1440,26 @@ config TQMX86_WDT > depends on X86 > select WATCHDOG_CORE > help > - This is the driver for the hardware watchdog timer in the TQMX86 IO > - controller found on some of their ComExpress Modules. > + This is the driver for the hardware watchdog timer in the TQMX86 IO > + controller found on some of their ComExpress Modules. > > - To compile this driver as a module, choose M here; the module > - will be called tqmx86_wdt. > + To compile this driver as a module, choose M here; the module > + will be called tqmx86_wdt. > > - Most people will say N. > + Most people will say N. > > config VIA_WDT > tristate "VIA Watchdog Timer" > depends on X86 && PCI > select WATCHDOG_CORE > help > - This is the driver for the hardware watchdog timer on VIA > - southbridge chipset CX700, VX800/VX820 or VX855/VX875. > + This is the driver for the hardware watchdog timer on VIA > + southbridge chipset CX700, VX800/VX820 or VX855/VX875. > > - To compile this driver as a module, choose M here; the module > - will be called via_wdt. > + To compile this driver as a module, choose M here; the module > + will be called via_wdt. > > - Most people will say N. > + Most people will say N. > > config W83627HF_WDT > tristate "Watchdog timer for W83627HF/W83627DHG and compatibles" > @@ -1745,10 +1745,10 @@ config BCM7038_WDT > depends on HAS_IOMEM > depends on ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST > help > - Watchdog driver for the built-in hardware in Broadcom 7038 and > - later SoCs used in set-top boxes. BCM7038 was made public > - during the 2004 CES, and since then, many Broadcom chips use this > - watchdog block, including some cable modem chips. > + Watchdog driver for the built-in hardware in Broadcom 7038 and > + later SoCs used in set-top boxes. BCM7038 was made public > + during the 2004 CES, and since then, many Broadcom chips use this > + watchdog block, including some cable modem chips. > > config IMGPDC_WDT > tristate "Imagination Technologies PDC Watchdog Timer" > @@ -2109,12 +2109,12 @@ config KEEMBAY_WATCHDOG > depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST) > select WATCHDOG_CORE > help > - This option enable support for an In-secure watchdog timer driver for > - Intel Keem Bay SoC. This WDT has a 32 bit timer and decrements in every > - count unit. An interrupt will be triggered, when the count crosses > - the threshold configured in the register. > + This option enable support for an In-secure watchdog timer driver for > + Intel Keem Bay SoC. This WDT has a 32 bit timer and decrements in every > + count unit. An interrupt will be triggered, when the count crosses > + the threshold configured in the register. > > - To compile this driver as a module, choose M here: the > - module will be called keembay_wdt. > + To compile this driver as a module, choose M here: the > + module will be called keembay_wdt. > > endif # WATCHDOG
On 11/11/2021 23:58:46+0100, Luca Ceresoli wrote: > RTC_DATE and REG_RTC_DATE are used for the registers holding the day of > month. Rename these constants to mean what they mean. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > > Changes in v3: none > > Changes in v2: > - fix drivers/mfd/max77686.c build failure due to missing rename > (Reported-by: kernel test robot <lkp@intel.com>) > --- > drivers/mfd/max77686.c | 2 +- > drivers/rtc/rtc-max77686.c | 16 ++++++++-------- > include/linux/mfd/max77686-private.h | 4 ++-- > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c > index f9e12ab2bc75..2ac64277fb84 100644 > --- a/drivers/mfd/max77686.c > +++ b/drivers/mfd/max77686.c > @@ -87,7 +87,7 @@ static bool max77802_rtc_is_volatile_reg(struct device *dev, unsigned int reg) > reg == MAX77802_RTC_WEEKDAY || > reg == MAX77802_RTC_MONTH || > reg == MAX77802_RTC_YEAR || > - reg == MAX77802_RTC_DATE); > + reg == MAX77802_RTC_MONTHDAY); > } > > static bool max77802_is_volatile_reg(struct device *dev, unsigned int reg) > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c > index bac52cdea97d..7e765207f28e 100644 > --- a/drivers/rtc/rtc-max77686.c > +++ b/drivers/rtc/rtc-max77686.c > @@ -57,7 +57,7 @@ enum { > RTC_WEEKDAY, > RTC_MONTH, > RTC_YEAR, > - RTC_DATE, > + RTC_MONTHDAY, > RTC_NR_TIME > }; > > @@ -119,7 +119,7 @@ enum max77686_rtc_reg_offset { > REG_RTC_WEEKDAY, > REG_RTC_MONTH, > REG_RTC_YEAR, > - REG_RTC_DATE, > + REG_RTC_MONTHDAY, > REG_ALARM1_SEC, > REG_ALARM1_MIN, > REG_ALARM1_HOUR, > @@ -150,7 +150,7 @@ static const unsigned int max77686_map[REG_RTC_END] = { > [REG_RTC_WEEKDAY] = MAX77686_RTC_WEEKDAY, > [REG_RTC_MONTH] = MAX77686_RTC_MONTH, > [REG_RTC_YEAR] = MAX77686_RTC_YEAR, > - [REG_RTC_DATE] = MAX77686_RTC_DATE, > + [REG_RTC_MONTHDAY] = MAX77686_RTC_MONTHDAY, > [REG_ALARM1_SEC] = MAX77686_ALARM1_SEC, > [REG_ALARM1_MIN] = MAX77686_ALARM1_MIN, > [REG_ALARM1_HOUR] = MAX77686_ALARM1_HOUR, > @@ -233,7 +233,7 @@ static const unsigned int max77802_map[REG_RTC_END] = { > [REG_RTC_WEEKDAY] = MAX77802_RTC_WEEKDAY, > [REG_RTC_MONTH] = MAX77802_RTC_MONTH, > [REG_RTC_YEAR] = MAX77802_RTC_YEAR, > - [REG_RTC_DATE] = MAX77802_RTC_DATE, > + [REG_RTC_MONTHDAY] = MAX77802_RTC_MONTHDAY, > [REG_ALARM1_SEC] = MAX77802_ALARM1_SEC, > [REG_ALARM1_MIN] = MAX77802_ALARM1_MIN, > [REG_ALARM1_HOUR] = MAX77802_ALARM1_HOUR, > @@ -288,7 +288,7 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, > > /* Only a single bit is set in data[], so fls() would be equivalent */ > tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1; > - tm->tm_mday = data[RTC_DATE] & 0x1f; > + tm->tm_mday = data[RTC_MONTHDAY] & 0x1f; > tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1; > tm->tm_year = data[RTC_YEAR] & mask; > tm->tm_yday = 0; > @@ -309,7 +309,7 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data, > data[RTC_MIN] = tm->tm_min; > data[RTC_HOUR] = tm->tm_hour; > data[RTC_WEEKDAY] = 1 << tm->tm_wday; > - data[RTC_DATE] = tm->tm_mday; > + data[RTC_MONTHDAY] = tm->tm_mday; > data[RTC_MONTH] = tm->tm_mon + 1; > > if (info->drv_data->alarm_enable_reg) { > @@ -565,8 +565,8 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info) > data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT); > if (data[RTC_YEAR] & info->drv_data->mask) > data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT); > - if (data[RTC_DATE] & 0x1f) > - data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT); > + if (data[RTC_MONTHDAY] & 0x1f) > + data[RTC_MONTHDAY] |= (1 << ALARM_ENABLE_SHIFT); > > ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC], > data, ARRAY_SIZE(data)); > diff --git a/include/linux/mfd/max77686-private.h b/include/linux/mfd/max77686-private.h > index b1482b3cf353..3acceeedbaba 100644 > --- a/include/linux/mfd/max77686-private.h > +++ b/include/linux/mfd/max77686-private.h > @@ -152,7 +152,7 @@ enum max77686_rtc_reg { > MAX77686_RTC_WEEKDAY = 0x0A, > MAX77686_RTC_MONTH = 0x0B, > MAX77686_RTC_YEAR = 0x0C, > - MAX77686_RTC_DATE = 0x0D, > + MAX77686_RTC_MONTHDAY = 0x0D, > MAX77686_ALARM1_SEC = 0x0E, > MAX77686_ALARM1_MIN = 0x0F, > MAX77686_ALARM1_HOUR = 0x10, > @@ -352,7 +352,7 @@ enum max77802_rtc_reg { > MAX77802_RTC_WEEKDAY = 0xCA, > MAX77802_RTC_MONTH = 0xCB, > MAX77802_RTC_YEAR = 0xCC, > - MAX77802_RTC_DATE = 0xCD, > + MAX77802_RTC_MONTHDAY = 0xCD, > MAX77802_RTC_AE1 = 0xCE, > MAX77802_ALARM1_SEC = 0xCF, > MAX77802_ALARM1_MIN = 0xD0, > -- > 2.25.1 >
On 11/11/2021 23:58:47+0100, Luca Ceresoli wrote: > The MAX77714 RTC chip is explicitly set to 24-hour mode in > max77686_rtc_probe() -> max77686_rtc_init_reg() and never changed back to > 12-hour mode. Accordingly info->rtc_24hr_mode is set to 1 in the same place > and never modified later, so it is de facto a constant. Yet there is code > to read 12-hour time, which is unreachable. > > Remove the unused variable, the unreachable code to manage 12-hour mode and > the defines that become unused due to the above changes. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > Changes in v3: none > > Changes in v2: > - remove the now-unused defines too (Alexandre Belloni) > - improve the commit message > --- > drivers/rtc/rtc-max77686.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c > index 7e765207f28e..5c64d08c0732 100644 > --- a/drivers/rtc/rtc-max77686.c > +++ b/drivers/rtc/rtc-max77686.c > @@ -34,9 +34,6 @@ > #define RTC_UDR_MASK BIT(RTC_UDR_SHIFT) > #define RTC_RBUDR_SHIFT 4 > #define RTC_RBUDR_MASK BIT(RTC_RBUDR_SHIFT) > -/* RTC Hour register */ > -#define HOUR_PM_SHIFT 6 > -#define HOUR_PM_MASK BIT(HOUR_PM_SHIFT) > /* RTC Alarm Enable */ > #define ALARM_ENABLE_SHIFT 7 > #define ALARM_ENABLE_MASK BIT(ALARM_ENABLE_SHIFT) > @@ -99,7 +96,6 @@ struct max77686_rtc_info { > > int rtc_irq; > int virq; > - int rtc_24hr_mode; > }; > > enum MAX77686_RTC_OP { > @@ -278,13 +274,7 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, > > tm->tm_sec = data[RTC_SEC] & mask; > tm->tm_min = data[RTC_MIN] & mask; > - if (info->rtc_24hr_mode) { > - tm->tm_hour = data[RTC_HOUR] & 0x1f; > - } else { > - tm->tm_hour = data[RTC_HOUR] & 0x0f; > - if (data[RTC_HOUR] & HOUR_PM_MASK) > - tm->tm_hour += 12; > - } > + tm->tm_hour = data[RTC_HOUR] & 0x1f; > > /* Only a single bit is set in data[], so fls() would be equivalent */ > tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1; > @@ -662,8 +652,6 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info) > data[0] = (1 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT); > data[1] = (0 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT); > > - info->rtc_24hr_mode = 1; > - > ret = regmap_bulk_write(info->rtc_regmap, > info->drv_data->map[REG_RTC_CONTROLM], > data, ARRAY_SIZE(data)); > -- > 2.25.1 >
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 and 8 add: dt bindings, mfd driver, watchdog driver and rtc driver. 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 (8): 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: max77714: add driver for the watchdog in the MAX77714 PMIC rtc: max77686: add MAX77714 support .../bindings/mfd/maxim,max77714.yaml | 68 +++++++ MAINTAINERS | 8 + 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 | 57 +++--- drivers/watchdog/Makefile | 1 + drivers/watchdog/max77714_wdt.c | 179 ++++++++++++++++++ include/linux/mfd/max77686-private.h | 4 +- include/linux/mfd/max77714.h | 60 ++++++ 13 files changed, 565 insertions(+), 58 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max77714.yaml create mode 100644 drivers/mfd/max77714.c create mode 100644 drivers/watchdog/max77714_wdt.c create mode 100644 include/linux/mfd/max77714.h