| Message ID | 20211019145919.7327-1-luca@lucaceresoli.net |
|---|---|
| Headers | show |
| Series | Add MAX77714 PMIC minimal driver (RTC and watchdog only) | expand |
On 10/19/21 7:59 AM, 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> FWIW, this patch should probably be handled separately, not as part of this series. Anyway, Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/Kconfig | 48 ++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index bf59faeb3de1..a24385099a91 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -694,10 +694,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" > @@ -1453,26 +1453,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" > @@ -1758,10 +1758,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" > @@ -2122,12 +2122,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 10/19/21 7:59 AM, 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> Acked-by: Guenter Roeck <linux@roeck-us.net> The driver needs the include file introduced with the mfd driver, so I assume it will be submitted through the mfd branch. Thanks, Guenter > > --- > > 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 abd9de8a9d99..71c3d8513ba0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11391,6 +11391,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 a24385099a91..b9b575acd690 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -699,6 +699,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 1bd2d6f37c53..268a942311a0 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -215,6 +215,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"); >
On 19/10/2021 16:59, 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 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 | 165 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77714.h | 60 +++++++++++++ > 5 files changed, 242 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 19/10/2021 16:59:13+0200, 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 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 2ad554b921d9..b1048ab25120 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 19/10/2021 16:59:14+0200, 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 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 >
On Tue, 19 Oct 2021, Luca Ceresoli wrote: > Some lines have an extra tab, remove them for proper visual alignment as > present on the rest of this file. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > > --- > > Changes in v2: none > --- > include/linux/mfd/max77686-private.h | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) Applied, thanks.
Hi Guenter, On 19/10/21 17:06, Guenter Roeck wrote: > On 10/19/21 7:59 AM, 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> > > FWIW, this patch should probably be handled separately, not as part > of this series. That was the initial idea. But this patch patch and patch 8 touch nearby lines, so adding the patch to this series looked like a good idea to avoid wasting maintainer time. It looks like I got the opposite effect... :( > Anyway, > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks.
On Tue, 19 Oct 2021, 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 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 | 165 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77714.h | 60 +++++++++++++ > 5 files changed, 242 insertions(+) > create mode 100644 drivers/mfd/max77714.c > create mode 100644 include/linux/mfd/max77714.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 514ff4a735e5..abd9de8a9d99 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11390,6 +11390,8 @@ MAXIM MAX77714 PMIC MFD DRIVER > M: Luca Ceresoli <luca@lucaceresoli.net> > S: Maintained > F: Documentation/devicetree/bindings/mfd/maxim,max77714.yaml > +F: drivers/mfd/max77714.c > +F: include/linux/mfd/max77714.h > > MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER > M: Javier Martinez Canillas <javier@dowhile0.org> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index ca0edab91aeb..295a04b479c6 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -853,6 +853,20 @@ config MFD_MAX77693 > additional drivers must be enabled in order to use the functionality > of the device. > > +config MFD_MAX77714 > + tristate "Maxim Semiconductor MAX77714 PMIC Support" > + depends on I2C > + depends on OF || COMPILE_TEST > + select MFD_CORE > + select REGMAP_I2C > + help > + Say yes here to add support for Maxim Semiconductor MAX77714. > + This is a Power Management IC with 4 buck regulators, 9 > + low-dropout regulators, 8 GPIOs, RTC, watchdog etc. This driver > + provides common support for accessing the device; additional > + drivers must be enabled in order to use each functionality of the > + device. > + > config MFD_MAX77843 > bool "Maxim Semiconductor MAX77843 PMIC Support" > depends on I2C=y > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 2ba6646e874c..fe43f2fdd5cb 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -163,6 +163,7 @@ obj-$(CONFIG_MFD_MAX77620) += max77620.o > obj-$(CONFIG_MFD_MAX77650) += max77650.o > obj-$(CONFIG_MFD_MAX77686) += max77686.o > obj-$(CONFIG_MFD_MAX77693) += max77693.o > +obj-$(CONFIG_MFD_MAX77714) += max77714.o > obj-$(CONFIG_MFD_MAX77843) += max77843.o > obj-$(CONFIG_MFD_MAX8907) += max8907.o > max8925-objs := max8925-core.o max8925-i2c.o > diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c > new file mode 100644 > index 000000000000..4b49d16fe199 > --- /dev/null > +++ b/drivers/mfd/max77714.c > @@ -0,0 +1,165 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Maxim MAX77714 MFD Driver > + * > + * Copyright (C) 2021 Luca Ceresoli > + * Author: Luca Ceresoli <luca@lucaceresoli.net> > + */ > + > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/max77714.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/regmap.h> > + > +struct max77714 { > + struct device *dev; > + struct regmap *regmap; > + struct regmap_irq_chip_data *irq_data; Is this used outside of .probe()? > + int irq; > +}; > + > +static const struct regmap_range max77714_readable_ranges[] = { > + regmap_reg_range(MAX77714_INT_TOP, MAX77714_INT_TOP), > + regmap_reg_range(MAX77714_INT_TOPM, MAX77714_INT_TOPM), > + regmap_reg_range(MAX77714_32K_STATUS, MAX77714_32K_CONFIG), > + regmap_reg_range(MAX77714_CNFG_GLBL2, MAX77714_CNFG2_ONOFF), > +}; > + > +static const struct regmap_range max77714_writable_ranges[] = { > + regmap_reg_range(MAX77714_INT_TOPM, MAX77714_INT_TOPM), > + regmap_reg_range(MAX77714_32K_CONFIG, MAX77714_32K_CONFIG), > + regmap_reg_range(MAX77714_CNFG_GLBL2, MAX77714_CNFG2_ONOFF), > +}; > + > +static const struct regmap_access_table max77714_readable_table = { > + .yes_ranges = max77714_readable_ranges, > + .n_yes_ranges = ARRAY_SIZE(max77714_readable_ranges), > +}; > + > +static const struct regmap_access_table max77714_writable_table = { > + .yes_ranges = max77714_writable_ranges, > + .n_yes_ranges = ARRAY_SIZE(max77714_writable_ranges), > +}; > + > +static const struct regmap_config max77714_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77714_CNFG2_ONOFF, > + .rd_table = &max77714_readable_table, > + .wr_table = &max77714_writable_table, > +}; > + > +static const struct regmap_irq max77714_top_irqs[] = { > + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_ONOFF, 0, MAX77714_INT_TOP_ONOFF), > + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_RTC, 0, MAX77714_INT_TOP_RTC), > + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_GPIO, 0, MAX77714_INT_TOP_GPIO), > + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_LDO, 0, MAX77714_INT_TOP_LDO), > + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_SD, 0, MAX77714_INT_TOP_SD), > + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_GLBL, 0, MAX77714_INT_TOP_GLBL), > +}; > + > +static const struct regmap_irq_chip max77714_irq_chip = { > + .name = "max77714-pmic", > + .status_base = MAX77714_INT_TOP, > + .mask_base = MAX77714_INT_TOPM, > + .num_regs = 1, > + .irqs = max77714_top_irqs, > + .num_irqs = ARRAY_SIZE(max77714_top_irqs), > +}; > + > +static const struct mfd_cell max77714_cells[] = { > + { .name = "max77714-watchdog" }, > + { .name = "max77714-rtc" }, > +}; Please place this at the top of the file. > +/* > + * MAX77714 initially uses the internal, low precision oscillator. Enable > + * the external oscillator by setting the XOSC_RETRY bit. If the external > + * oscillator is not OK (probably not installed) this has no effect. > + */ > +static int max77714_setup_xosc(struct max77714 *chip) May as well just pass 'dev' and 'regmap' to this function and do away with the superfluous struct along with all of it's memory management handling requirements. > +{ > + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */ > + static const unsigned int load_cap[4] = {0, 10, 12, 22}; Probably best to define these magic numbers. > + unsigned int load_cap_idx; > + unsigned int status; > + int err; > + > + err = regmap_update_bits(chip->regmap, MAX77714_32K_CONFIG, > + MAX77714_32K_CONFIG_XOSC_RETRY, > + MAX77714_32K_CONFIG_XOSC_RETRY); > + if (err) > + return dev_err_probe(chip->dev, err, "cannot configure XOSC\n"); Error messages should be clear: "Failed to configure the external oscillator" Same for the messages below. > + err = regmap_read(chip->regmap, MAX77714_32K_STATUS, &status); > + if (err) > + return dev_err_probe(chip->dev, err, "cannot read XOSC status\n"); > + > + load_cap_idx = (status >> MAX77714_32K_STATUS_32KLOAD_SHF) > + & MAX77714_32K_STATUS_32KLOAD_MSK; > + > + dev_info(chip->dev, "Using %s oscillator, %d pF load cap\n", > + status & MAX77714_32K_STATUS_32KSOURCE ? "internal" : "external", > + load_cap[load_cap_idx]); > + > + return 0; > +} > + > +static int max77714_probe(struct i2c_client *client) > +{ > + struct max77714 *chip; > + int err; > + > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + i2c_set_clientdata(client, chip); Where else is this used? The definition appears to be local. > + chip->dev = &client->dev; > + > + chip->regmap = devm_regmap_init_i2c(client, &max77714_regmap_config); > + if (IS_ERR(chip->regmap)) > + return dev_err_probe(chip->dev, PTR_ERR(chip->regmap), > + "failed to initialise regmap\n"); > + > + err = max77714_setup_xosc(chip); > + if (err) > + return err; > + > + err = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client->irq, > + IRQF_ONESHOT | IRQF_SHARED, 0, > + &max77714_irq_chip, &chip->irq_data); > + if (err) > + return dev_err_probe(chip->dev, err, "failed to add PMIC irq chip\n"); IRQ > + err = devm_mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE, > + max77714_cells, ARRAY_SIZE(max77714_cells), > + NULL, 0, NULL); > + if (err) > + return dev_err_probe(chip->dev, err, "failed adding MFD children\n"); "Failed to register child devices" > + return 0; > +} > + > +static const struct of_device_id max77714_dt_match[] = { > + { .compatible = "maxim,max77714" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, max77714_dt_match); > + > +static struct i2c_driver max77714_driver = { > + .driver = { > + .name = "max77714", > + .of_match_table = max77714_dt_match, > + }, > + .probe_new = max77714_probe, > +}; > +module_i2c_driver(max77714_driver); > + > +MODULE_DESCRIPTION("Maxim MAX77714 MFD core driver"); > +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/max77714.h b/include/linux/mfd/max77714.h > new file mode 100644 > index 000000000000..4a274592d4f2 > --- /dev/null > +++ b/include/linux/mfd/max77714.h > @@ -0,0 +1,60 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Maxim MAX77714 Register and data structures definition. > + * > + * Copyright (C) 2021 Luca Ceresoli > + * Author: Luca Ceresoli <luca@lucaceresoli.net> > + */ > + > +#ifndef __LINUX_MFD_MAX77714_H_ > +#define __LINUX_MFD_MAX77714_H_ > + > +#include <linux/bits.h> > + > +#define MAX77714_INT_TOP 0x00 > +#define MAX77714_INT_TOPM 0x07 /* Datasheet says "read only", but it is RW */ > + > +#define MAX77714_INT_TOP_ONOFF BIT(1) > +#define MAX77714_INT_TOP_RTC BIT(3) > +#define MAX77714_INT_TOP_GPIO BIT(4) > +#define MAX77714_INT_TOP_LDO BIT(5) > +#define MAX77714_INT_TOP_SD BIT(6) > +#define MAX77714_INT_TOP_GLBL BIT(7) > + > +#define MAX77714_32K_STATUS 0x30 > +#define MAX77714_32K_STATUS_SIOSCOK BIT(5) > +#define MAX77714_32K_STATUS_XOSCOK BIT(4) > +#define MAX77714_32K_STATUS_32KSOURCE BIT(3) > +#define MAX77714_32K_STATUS_32KLOAD_MSK 0x3 > +#define MAX77714_32K_STATUS_32KLOAD_SHF 1 > +#define MAX77714_32K_STATUS_CRYSTAL_CFG BIT(0) > + > +#define MAX77714_32K_CONFIG 0x31 > +#define MAX77714_32K_CONFIG_XOSC_RETRY BIT(4) > + > +#define MAX77714_CNFG_GLBL2 0x91 > +#define MAX77714_WDTEN BIT(2) > +#define MAX77714_WDTSLPC BIT(3) > +#define MAX77714_TWD_MASK 0x3 > +#define MAX77714_TWD_2s 0x0 > +#define MAX77714_TWD_16s 0x1 > +#define MAX77714_TWD_64s 0x2 > +#define MAX77714_TWD_128s 0x3 > + > +#define MAX77714_CNFG_GLBL3 0x92 > +#define MAX77714_WDTC BIT(0) > + > +#define MAX77714_CNFG2_ONOFF 0x94 > +#define MAX77714_WD_RST_WK BIT(5) > + > +/* Interrupts */ > +enum { > + MAX77714_IRQ_TOP_ONOFF, > + MAX77714_IRQ_TOP_RTC, /* Real-time clock */ > + MAX77714_IRQ_TOP_GPIO, /* GPIOs */ > + MAX77714_IRQ_TOP_LDO, /* Low-dropout regulators */ > + MAX77714_IRQ_TOP_SD, /* Step-down regulators */ > + MAX77714_IRQ_TOP_GLBL, /* "Global resources": Low-Battery, overtemp... */ > +}; > + > +#endif /* __LINUX_MFD_MAX77714_H_ */
Hi Lee, On 21/10/21 20:43, Lee Jones wrote: > On Tue, 19 Oct 2021, Luca Ceresoli wrote: [...] >> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c >> new file mode 100644 >> index 000000000000..4b49d16fe199 >> --- /dev/null >> +++ b/drivers/mfd/max77714.c >> @@ -0,0 +1,165 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Maxim MAX77714 MFD Driver >> + * >> + * Copyright (C) 2021 Luca Ceresoli >> + * Author: Luca Ceresoli <luca@lucaceresoli.net> >> + */ >> + >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/mfd/core.h> >> +#include <linux/mfd/max77714.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/regmap.h> >> + >> +struct max77714 { >> + struct device *dev; >> + struct regmap *regmap; >> + struct regmap_irq_chip_data *irq_data; > > Is this used outside of .probe()? No. >> +/* >> + * MAX77714 initially uses the internal, low precision oscillator. Enable >> + * the external oscillator by setting the XOSC_RETRY bit. If the external >> + * oscillator is not OK (probably not installed) this has no effect. >> + */ >> +static int max77714_setup_xosc(struct max77714 *chip) > > May as well just pass 'dev' and 'regmap' to this function and do away > with the superfluous struct along with all of it's memory management > handling requirements. Good idea! >> +{ >> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */ >> + static const unsigned int load_cap[4] = {0, 10, 12, 22}; > > Probably best to define these magic numbers. Since these numbers do not appear anywhere else I don't find added value in: #define MAX77714_LOAD_CAP_0 0 #define MAX77714_LOAD_CAP_10 10 #define MAX77714_LOAD_CAP_12 12 #define MAX77714_LOAD_CAP_22 22 [...] static const unsigned int load_cap[4] = { MAX77714_LOAD_CAP_0, MAX77714_LOAD_CAP_10, MAX77714_LOAD_CAP_12, MAX77714_LOAD_CAP_12, }; besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even worse, there is potential for copy-paste errors -- can you spot it? ;) Finally, consider this is not even global but local to a small function. But I'd rather add the unit ("pF") to either the comment line of the array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you? Apart from this coding style topic I'm OK with all the other improvements you proposed to this patch, all of them will be in v3. Thank you for the detailed review!
On Wed, 27 Oct 2021, Luca Ceresoli wrote: > Hi Lee, > > On 21/10/21 20:43, Lee Jones wrote: > > On Tue, 19 Oct 2021, Luca Ceresoli wrote: > [...] > >> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c > >> new file mode 100644 > >> index 000000000000..4b49d16fe199 > >> --- /dev/null > >> +++ b/drivers/mfd/max77714.c > >> @@ -0,0 +1,165 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +/* > >> + * Maxim MAX77714 MFD Driver > >> + * > >> + * Copyright (C) 2021 Luca Ceresoli > >> + * Author: Luca Ceresoli <luca@lucaceresoli.net> > >> + */ > >> + > >> +#include <linux/i2c.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/mfd/core.h> > >> +#include <linux/mfd/max77714.h> > >> +#include <linux/module.h> > >> +#include <linux/of.h> > >> +#include <linux/regmap.h> > >> + > >> +struct max77714 { > >> + struct device *dev; > >> + struct regmap *regmap; > >> + struct regmap_irq_chip_data *irq_data; > > > > Is this used outside of .probe()? > > No. Then you don't need to store it in a struct. [...] > >> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */ > >> + static const unsigned int load_cap[4] = {0, 10, 12, 22}; > > > > Probably best to define these magic numbers. > > Since these numbers do not appear anywhere else I don't find added value in: > > #define MAX77714_LOAD_CAP_0 0 > #define MAX77714_LOAD_CAP_10 10 > #define MAX77714_LOAD_CAP_12 12 > #define MAX77714_LOAD_CAP_22 22 > [...] > static const unsigned int load_cap[4] = { > MAX77714_LOAD_CAP_0, > MAX77714_LOAD_CAP_10, > MAX77714_LOAD_CAP_12, > MAX77714_LOAD_CAP_12, > }; I don't find value in that nomenclature either! :) I was suggesting that you used better, more forthcoming names. LOAD_CAPACITANCE_00_pF LOAD_CAPACITANCE_10_pF LOAD_CAPACITANCE_12_pF LOAD_CAPACITANCE_22_pF > besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even > worse, there is potential for copy-paste errors -- can you spot it? ;) Yes. Straight away. > Finally, consider this is not even global but local to a small function. > > But I'd rather add the unit ("pF") to either the comment line of the > array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you? I did have to read the code again to get a handle on things (probably not a good sign). To keep things simple, just add "/* pF */" onto the end of the load_cap line for now. That should clear things up at first glance.
Hi Lee, On 27/10/21 15:44, Lee Jones wrote: > On Wed, 27 Oct 2021, Luca Ceresoli wrote: > >> Hi Lee, >> >> On 21/10/21 20:43, Lee Jones wrote: >>> On Tue, 19 Oct 2021, Luca Ceresoli wrote: >> [...] >>>> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c >>>> new file mode 100644 >>>> index 000000000000..4b49d16fe199 >>>> --- /dev/null >>>> +++ b/drivers/mfd/max77714.c >>>> @@ -0,0 +1,165 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/* >>>> + * Maxim MAX77714 MFD Driver >>>> + * >>>> + * Copyright (C) 2021 Luca Ceresoli >>>> + * Author: Luca Ceresoli <luca@lucaceresoli.net> >>>> + */ >>>> + >>>> +#include <linux/i2c.h> >>>> +#include <linux/interrupt.h> >>>> +#include <linux/mfd/core.h> >>>> +#include <linux/mfd/max77714.h> >>>> +#include <linux/module.h> >>>> +#include <linux/of.h> >>>> +#include <linux/regmap.h> >>>> + >>>> +struct max77714 { >>>> + struct device *dev; >>>> + struct regmap *regmap; >>>> + struct regmap_irq_chip_data *irq_data; >>> >>> Is this used outside of .probe()? >> >> No. > > Then you don't need to store it in a struct. > > [...] > >>>> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */ >>>> + static const unsigned int load_cap[4] = {0, 10, 12, 22}; >>> >>> Probably best to define these magic numbers. >> >> Since these numbers do not appear anywhere else I don't find added value in: >> >> #define MAX77714_LOAD_CAP_0 0 >> #define MAX77714_LOAD_CAP_10 10 >> #define MAX77714_LOAD_CAP_12 12 >> #define MAX77714_LOAD_CAP_22 22 >> [...] >> static const unsigned int load_cap[4] = { >> MAX77714_LOAD_CAP_0, >> MAX77714_LOAD_CAP_10, >> MAX77714_LOAD_CAP_12, >> MAX77714_LOAD_CAP_12, >> }; > > I don't find value in that nomenclature either! :) > > I was suggesting that you used better, more forthcoming names. > > LOAD_CAPACITANCE_00_pF > LOAD_CAPACITANCE_10_pF > LOAD_CAPACITANCE_12_pF > LOAD_CAPACITANCE_22_pF > >> besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even >> worse, there is potential for copy-paste errors -- can you spot it? ;) > > Yes. Straight away. > >> Finally, consider this is not even global but local to a small function. >> >> But I'd rather add the unit ("pF") to either the comment line of the >> array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you? > > I did have to read the code again to get a handle on things (probably > not a good sign). To keep things simple, just add "/* pF */" onto the > end of the load_cap line for now. That should clear things up at > first glance. Ok, let's see how it works. I'm sending v3 in the next few days.
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-4 + 7 are trivial cleanups to the max77686 drivers and can probably be applied easily. Patches 5, 6, 8 and 9 add: dt bindings, mfd driver, watchdog driver and rtc driver. 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): mfd: max77686: Correct tab-based alignment of register addresses 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 | 58 ++++++ MAINTAINERS | 8 + drivers/mfd/Kconfig | 14 ++ drivers/mfd/Makefile | 1 + drivers/mfd/max77686.c | 2 +- drivers/mfd/max77714.c | 165 ++++++++++++++++ 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 | 28 +-- include/linux/mfd/max77714.h | 60 ++++++ 13 files changed, 580 insertions(+), 70 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