[PATCH v4 0/9] Add MAX77714 PMIC minimal driver (RTC and watchdog only)

[PATCH v4 0/9] Add MAX77714 PMIC minimal driver (RTC and watchdog only)

[Luca Ceresoli]

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

-- 
2.25.1

[PATCH v4 1/9] rtc: max77686: convert comments to kernel-doc format

[Luca Ceresoli]

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>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

---

Changes in v4: none

Changes in v3: none

Changes in v2: none
---
 drivers/rtc/rtc-max77686.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index eae7cb9faf1e..bac52cdea97d 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -61,24 +61,27 @@ enum {
 	RTC_NR_TIME
 };
 
+/**
+ * struct max77686_rtc_driver_data - model-specific configuration
+ * @delay: Minimum usecs needed for a RTC update
+ * @mask: Mask used to read RTC registers value
+ * @map: Registers offset to I2C addresses map
+ * @alarm_enable_reg: Has a separate alarm enable register?
+ * @rtc_i2c_addr: I2C address for RTC block
+ * @rtc_irq_from_platform: RTC interrupt via platform resource
+ * @alarm_pending_status_reg: Pending alarm status register
+ * @rtc_irq_chip: RTC IRQ CHIP for regmap
+ * @regmap_config: regmap configuration for the chip
+ */
 struct max77686_rtc_driver_data {
-	/* Minimum usecs needed for a RTC update */
 	unsigned long		delay;
-	/* Mask used to read RTC registers value */
 	u8			mask;
-	/* Registers offset to I2C addresses map */
 	const unsigned int	*map;
-	/* Has a separate alarm enable register? */
 	bool			alarm_enable_reg;
-	/* I2C address for RTC block */
 	int			rtc_i2c_addr;
-	/* RTC interrupt via platform resource */
 	bool			rtc_irq_from_platform;
-	/* Pending alarm status register */
 	int			alarm_pending_status_reg;
-	/* RTC IRQ CHIP for regmap */
 	const struct regmap_irq_chip *rtc_irq_chip;
-	/* regmap configuration for the chip */
 	const struct regmap_config *regmap_config;
 };
 
-- 
2.25.1

[PATCH v4 2/9] rtc: max77686: rename day-of-month defines

[Luca Ceresoli]

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 v4: none

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

[PATCH v4 3/9] rtc: max77686: remove unused code to read in 12-hour mode

[Luca Ceresoli]

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 v4: none

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

[PATCH v4 4/9] dt-bindings: mfd: add Maxim MAX77714 PMIC

[Luca Ceresoli]

Add bindings for the MAX77714 PMIC with GPIO, RTC and watchdog.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Reviewed-by: Rob Herring <robh@kernel.org>

---

Changes in v4: none

Changes in v3:
 - add 'regulators' node (Krzysztof Kozlowski, Rob Herring)

Changes in v2: none
---
 .../bindings/mfd/maxim,max77714.yaml          | 68 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max77714.yaml

diff --git a/Documentation/devicetree/bindings/mfd/maxim,max77714.yaml b/Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
new file mode 100644
index 000000000000..74a6867d3c82
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/maxim,max77714.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MAX77714 PMIC with GPIO, RTC and watchdog from Maxim Integrated.
+
+maintainers:
+  - Luca Ceresoli <luca@lucaceresoli.net>
+
+description: |
+  MAX77714 is a Power Management IC with 4 buck regulators, 9
+  low-dropout regulators, 8 GPIOs, RTC and watchdog.
+
+properties:
+  compatible:
+    const: maxim,max77714
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+    description:
+      The first cell is the IRQ number, the second cell is the trigger type.
+
+  regulators:
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      '^(buck[0-3]|ldo[0-8])$':
+        type: object
+        unevaluatedProperties: false
+        $ref: /schemas/regulator/regulator.yaml#
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pmic@1c {
+            compatible = "maxim,max77714";
+            reg = <0x1c>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+            interrupt-controller;
+            #interrupt-cells = <2>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 10c8ae3a8c73..8e084f9a1d9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11572,6 +11572,11 @@ F:	drivers/power/supply/max77650-charger.c
 F:	drivers/regulator/max77650-regulator.c
 F:	include/linux/mfd/max77650.h
 
+MAXIM MAX77714 PMIC MFD DRIVER
+M:	Luca Ceresoli <luca@lucaceresoli.net>
+S:	Maintained
+F:	Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
+
 MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
 M:	Javier Martinez Canillas <javier@dowhile0.org>
 L:	linux-kernel@vger.kernel.org
-- 
2.25.1

[PATCH v4 5/9] mfd: max77714: Add driver for Maxim MAX77714 PMIC

[Luca Ceresoli]

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e084f9a1d9f..a40db6e6a86b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11576,6 +11576,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 3fb480818599..1b9d772bdae6 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -855,6 +855,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 0b1b629aef3e..03115cf1336b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -162,6 +162,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..08dfb69bc6e8
--- /dev/null
+++ b/drivers/mfd/max77714.c
@@ -0,0 +1,152 @@
+// 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>
+
+static const struct mfd_cell max77714_cells[] = {
+	{ .name = "max77714-watchdog" },
+	{ .name = "max77714-rtc" },
+};
+
+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),
+};
+
+/*
+ * 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 device *dev, struct regmap *regmap)
+{
+	/* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
+	static const unsigned int load_cap[4] = {0, 10, 12, 22}; /* pF */
+	unsigned int load_cap_idx;
+	unsigned int status;
+	int err;
+
+	err = regmap_update_bits(regmap, MAX77714_32K_CONFIG,
+				 MAX77714_32K_CONFIG_XOSC_RETRY,
+				 MAX77714_32K_CONFIG_XOSC_RETRY);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to configure the external oscillator\n");
+
+	err = regmap_read(regmap, MAX77714_32K_STATUS, &status);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to read external oscillator status\n");
+
+	load_cap_idx = (status >> MAX77714_32K_STATUS_32KLOAD_SHF)
+		& MAX77714_32K_STATUS_32KLOAD_MSK;
+
+	dev_info(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 device *dev = &client->dev;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *irq_data;
+	int err;
+
+	regmap = devm_regmap_init_i2c(client, &max77714_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialise regmap\n");
+
+	err = max77714_setup_xosc(dev, regmap);
+	if (err)
+		return err;
+
+	err = devm_regmap_add_irq_chip(dev, regmap, client->irq,
+				       IRQF_ONESHOT | IRQF_SHARED, 0,
+				       &max77714_irq_chip, &irq_data);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to add PMIC IRQ chip\n");
+
+	err =  devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    max77714_cells, ARRAY_SIZE(max77714_cells),
+				    NULL, 0, NULL);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to register child devices\n");
+
+	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_ */
-- 
2.25.1

[PATCH v4 6/9] watchdog: Kconfig: fix help text indentation

[Luca Ceresoli]

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 v4: none

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

[PATCH v4 7/9] watchdog: max77620: add support for the max77714 variant

[Luca Ceresoli]

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,
+	},
+	.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, &regval);
+	ret = regmap_read(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, &regval);
 	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");
-- 
2.25.1

[PATCH v4 8/9] watchdog: max77620: add comment to clarify set_timeout procedure

[Luca Ceresoli]

Clarify why we need to ping the watchdog before changing the timeout by
quoting the MAX77714 datasheet.

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)
-- 
2.25.1

[PATCH v4 9/9] rtc: max77686: add MAX77714 support

[Luca Ceresoli]

The RTC included in the MAX77714 PMIC is very similar to the one in the
MAX77686. Reuse the rtc-max77686.c driver with the minimum required changes
for the MAX77714 RTC.

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 v4: none

Changes in v3: none

Changes in v2: none
---
 drivers/rtc/Kconfig        |  2 +-
 drivers/rtc/rtc-max77686.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 058e56a10ab8..4242cca5e560 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -375,7 +375,7 @@ config RTC_DRV_MAX8997
 
 config RTC_DRV_MAX77686
 	tristate "Maxim MAX77686"
-	depends on MFD_MAX77686 || MFD_MAX77620 || COMPILE_TEST
+	depends on MFD_MAX77686 || MFD_MAX77620 || MFD_MAX77714 || COMPILE_TEST
 	help
 	  If you say yes here you will get support for the
 	  RTC of Maxim MAX77686/MAX77620/MAX77802 PMIC.
diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 5c64d08c0732..b0250d91fb00 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -19,6 +19,7 @@
 
 #define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
 #define MAX77620_I2C_ADDR_RTC		0x68
+#define MAX77714_I2C_ADDR_RTC		0x48
 #define MAX77686_INVALID_I2C_ADDR	(-1)
 
 /* Define non existing register */
@@ -200,6 +201,28 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.regmap_config = &max77686_rtc_regmap_config,
 };
 
+static const struct regmap_irq_chip max77714_rtc_irq_chip = {
+	.name		= "max77714-rtc",
+	.status_base	= MAX77686_RTC_INT,
+	.mask_base	= MAX77686_RTC_INTM,
+	.num_regs	= 1,
+	.irqs		= max77686_rtc_irqs,
+	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs) - 1, /* no WTSR on 77714 */
+};
+
+static const struct max77686_rtc_driver_data max77714_drv_data = {
+	.delay = 16000,
+	.mask  = 0x7f,
+	.map   = max77686_map,
+	.alarm_enable_reg = false,
+	.rtc_irq_from_platform = false,
+	/* On MAX77714 RTCA1 is BIT 1 of RTCINT (0x00). Not supported by this driver. */
+	.alarm_pending_status_reg = MAX77686_INVALID_REG,
+	.rtc_i2c_addr = MAX77714_I2C_ADDR_RTC,
+	.rtc_irq_chip = &max77714_rtc_irq_chip,
+	.regmap_config = &max77686_rtc_regmap_config,
+};
+
 static const struct regmap_config max77620_rtc_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -843,6 +866,7 @@ static const struct platform_device_id rtc_id[] = {
 	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
 	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
 	{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
+	{ "max77714-rtc", .driver_data = (kernel_ulong_t)&max77714_drv_data, },
 	{},
 };
 MODULE_DEVICE_TABLE(platform, rtc_id);
-- 
2.25.1

Re: [PATCH v4 4/9] dt-bindings: mfd: add Maxim MAX77714 PMIC

[Krzysztof Kozlowski]

On 20/11/2021 16:57, Luca Ceresoli wrote:
> Add bindings for the MAX77714 PMIC with GPIO, RTC and watchdog.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Changes in v4: none
> 
> Changes in v3:
>  - add 'regulators' node (Krzysztof Kozlowski, Rob Herring)
> 
> Changes in v2: none
> ---
>  .../bindings/mfd/maxim,max77714.yaml          | 68 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v4 5/9] mfd: max77714: Add driver for Maxim MAX77714 PMIC

[Krzysztof Kozlowski]

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

Re: [PATCH v4 7/9] watchdog: max77620: add support for the max77714 variant

[Guenter Roeck]

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, &regval);
> +	ret = regmap_read(wdt->rmap, wdt->drv_data->reg_cnfg_glbl2, &regval);
>  	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");

Re: [PATCH v4 8/9] watchdog: max77620: add comment to clarify set_timeout procedure

[Guenter Roeck]

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)

Re: [PATCH v4 8/9] watchdog: max77620: add comment to clarify set_timeout procedure

[Luca Ceresoli]

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)
	 */

?

-- 
Luca

Re: [PATCH v4 8/9] watchdog: max77620: add comment to clarify set_timeout procedure

[Guenter Roeck]

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

Re: [PATCH v4 7/9] watchdog: max77620: add support for the max77714 variant

[Luca Ceresoli]

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.
-- 
Luca (slightly confused, but very open to learning)

Re: [PATCH v4 7/9] watchdog: max77620: add support for the max77714 variant

[Guenter Roeck]

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

Re: [PATCH v4 7/9] watchdog: max77620: add support for the max77714 variant

[Luca Ceresoli]

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.

-- 
Luca