[PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Ibrahim Tilki]

changelog:
since v5:
  - dt-binding: add enum value "2" to aux-voltage-chargable
  - dt-binding: remove adi,trickle-diode-enable
  - dt-binding: change description of trickle-resistor-ohms
  - dt-binding: reorder as in example schema
  - parse "wakeup-source" when irq not requested
  - remove limitation on max31328 irq and clokout
  - remove error and warning messages during trickle charger setup

since v4:
  - dt-binding: remove interrupt names.
  - dt-binding: add description for "interrupts" property
  - dt-binding: replace deprecated property "trickle-diode-disable"
      by "aux-voltage-chargeable"
  - dt-binding: add new property "adi,trickle-diode-enable"
  - dt-binding: remove "wakeup-source"
  - use clear_bit instead of __clear_bit
  - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
  - use chip_desc pointer as driver data instead of enum.

since v3:
  - add "break" to fix warning: unannotated fall-through 
    Reported-by: kernel test robot <lkp@intel.com>

since v2:
  - dt-binding: update title and description
  - dt-binding: remove last example
  - drop watchdog support
  - support reading 12Hr format instead of forcing 24hr at probe time
  - use "tm_year % 100" instead of range check
  - refactor max313xx_init for readability

Ibrahim Tilki (2):
  drivers: rtc: add max313xx series rtc driver
  dt-bindings: rtc: add max313xx RTCs

 .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
 drivers/rtc/Kconfig                           |   11 +
 drivers/rtc/Makefile                          |    1 +
 drivers/rtc/rtc-max313xx.c                    | 1053 +++++++++++++++++
 4 files changed, 1209 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
 create mode 100644 drivers/rtc/rtc-max313xx.c

-- 
2.25.1

[PATCH v5 1/2] drivers: rtc: add max313xx series rtc driver

[Ibrahim Tilki]

Adding support for Analog Devices MAX313XX series RTCs.

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
---
 drivers/rtc/Kconfig        |   11 +
 drivers/rtc/Makefile       |    1 +
 drivers/rtc/rtc-max313xx.c | 1053 ++++++++++++++++++++++++++++++++++++
 3 files changed, 1065 insertions(+)
 create mode 100644 drivers/rtc/rtc-max313xx.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 753872408..2160619ed 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -323,6 +323,17 @@ config RTC_DRV_LP8788
 	help
 	  Say Y to enable support for the LP8788 RTC/ALARM driver.
 
+config RTC_DRV_MAX313XX
+	tristate "Analog Devices MAX313XX RTC driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you will get support for the
+	  Analog Devices MAX313XX series RTC family.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-max313xx.
+
 config RTC_DRV_MAX6900
 	tristate "Maxim MAX6900"
 	help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index ea445d1eb..880db99be 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_RTC_DRV_M41T94)	+= rtc-m41t94.o
 obj-$(CONFIG_RTC_DRV_M48T35)	+= rtc-m48t35.o
 obj-$(CONFIG_RTC_DRV_M48T59)	+= rtc-m48t59.o
 obj-$(CONFIG_RTC_DRV_M48T86)	+= rtc-m48t86.o
+obj-$(CONFIG_RTC_DRV_MAX313XX)	+= rtc-max313xx.o
 obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
 obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
 obj-$(CONFIG_RTC_DRV_MAX6916)	+= rtc-max6916.o
diff --git a/drivers/rtc/rtc-max313xx.c b/drivers/rtc/rtc-max313xx.c
new file mode 100644
index 000000000..043528d7d
--- /dev/null
+++ b/drivers/rtc/rtc-max313xx.c
@@ -0,0 +1,1053 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices MAX313XX series I2C RTC driver
+ *
+ * Copyright 2023 Analog Devices Inc.
+ */
+#include <asm-generic/unaligned.h>
+#include <linux/bcd.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/util_macros.h>
+
+/* common registers */
+#define MAX313XX_INT_ALARM1		BIT(0)
+#define MAX313XX_HRS_F_AM_PM		BIT(5)
+#define MAX313XX_HRS_F_12_24		BIT(6)
+#define MAX313XX_MONTH_CENTURY		BIT(7)
+
+#define MAX313XX_TIME_SIZE		0x07
+
+/* device specific registers */
+#define MAX3134X_CFG2_REG		0x01
+#define MAX3134X_CFG2_SET_RTC		BIT(1)
+
+#define MAX31341_TRICKLE_RES_MASK	GENMASK(1, 0)
+#define MAX31341_TRICKLE_DIODE_EN	BIT(2)
+#define MAX31341_TRICKLE_ENABLE_BIT	BIT(3)
+#define MAX31341_POWER_MGMT_REG		0x56
+#define MAX31341_POWER_MGMT_TRICKLE_BIT	BIT(0)
+
+#define MAX3133X_TRICKLE_RES_MASK	GENMASK(2, 1)
+#define MAX3133X_TRICKLE_DIODE_EN	BIT(3)
+#define MAX3133X_TRICKLE_ENABLE_BIT	BIT(0)
+
+#define MAX31329_TRICKLE_ENABLE_BIT	BIT(7)
+#define MAX31343_TRICKLE_ENABLE_MASK	GENMASK(7, 4)
+#define MAX31343_TRICKLE_ENABLE_CODE	5
+#define MAX31329_43_TRICKLE_RES_MASK	GENMASK(1, 0)
+#define MAX31329_43_TRICKLE_DIODE_EN	BIT(2)
+
+#define MAX31329_CONFIG2_REG		0x04
+#define MAX31329_CONFIG2_CLKIN_EN	BIT(2)
+#define MAX31329_CONFIG2_CLKIN_FREQ	GENMASK(1, 0)
+
+#define MAX31341_42_CONFIG1_REG		0x00
+#define MAX31341_42_CONFIG1_CLKIN_EN	BIT(7)
+#define MAX31341_42_CONFIG1_CLKIN_FREQ	GENMASK(5, 4)
+#define MAX31341_42_CONFIG1_OSC_DISABLE	BIT(3)
+#define MAX31341_42_CONFIG1_SWRST	BIT(0)
+
+enum max313xx_ids {
+	ID_MAX31328,
+	ID_MAX31329,
+	ID_MAX31331,
+	ID_MAX31334,
+	ID_MAX31341,
+	ID_MAX31342,
+	ID_MAX31343,
+	MAX313XX_ID_NR
+};
+
+struct clkout_cfg {
+	const int *freq_avail;
+	u8 freq_size;
+	u8 freq_pos;
+	u8 reg;
+	u8 en_bit;
+	bool en_invert;
+};
+
+struct chip_desc {
+	struct clkout_cfg *clkout;
+	const char *clkout_name;
+	u8 sec_reg;
+	u8 alarm1_sec_reg;
+
+	u8 int_en_reg;
+	u8 int_status_reg;
+
+	u8 ram_reg;
+	u8 ram_size;
+
+	u8 temp_reg;
+
+	u8 trickle_reg;
+};
+
+#define clk_hw_to_max313xx(_hw) container_of(_hw, struct max313xx, clkout)
+
+struct max313xx {
+	enum max313xx_ids id;
+	struct regmap *regmap;
+	struct rtc_device *rtc;
+	struct clk_hw clkout;
+	struct clk *clkin;
+	const struct chip_desc *chip;
+	int irq;
+};
+
+static const int max313xx_clkin_freq[] = { 1, 50, 60, 32768 };
+
+static const int max31328_clkout_freq[] = { 1, 1024, 4096, 8192 };
+static const int max31329_clkout_freq[] = { 1, 4096, 8192, 32768 };
+static const int max3133x_clkout_freq[] = { 1, 64, 1024, 32768 };
+static const int max31341_42_clkout_freq[] = { 1, 50, 60, 32768 };
+static const int max31343_clkout_freq[] = { 1, 2, 4, 8, 16, 32, 64, 128, 32875 };
+
+static struct clkout_cfg max31328_clkout = {
+	.freq_avail = max31328_clkout_freq,
+	.freq_size = ARRAY_SIZE(max31328_clkout_freq),
+	.freq_pos = 3,
+	.reg = 0x0E,
+	.en_bit = BIT(3),
+	.en_invert = true,
+};
+
+static struct clkout_cfg max31329_clkout = {
+	.freq_avail = max31329_clkout_freq,
+	.freq_size = ARRAY_SIZE(max31329_clkout_freq),
+	.freq_pos = 5,
+	.reg = 0x04,
+	.en_bit = BIT(7),
+};
+
+static struct clkout_cfg max3133x_clkout = {
+	.freq_avail = max3133x_clkout_freq,
+	.freq_size = ARRAY_SIZE(max3133x_clkout_freq),
+	.freq_pos = 0,
+	.reg = 0x04,
+	.en_bit = BIT(2),
+};
+
+static struct clkout_cfg max31341_42_clkout = {
+	.freq_avail = max31341_42_clkout_freq,
+	.freq_size = ARRAY_SIZE(max31341_42_clkout_freq),
+	.freq_pos = 1,
+	.reg = 0x00,
+	.en_bit = BIT(6),
+	.en_invert = true,
+};
+
+static struct clkout_cfg max31343_clkout = {
+	.freq_avail = max31343_clkout_freq,
+	.freq_size = ARRAY_SIZE(max31343_clkout_freq),
+	.freq_pos = 3,
+	.reg = 0x04,
+	.en_bit = BIT(7),
+};
+
+static const struct chip_desc chip[MAX313XX_ID_NR] = {
+	[ID_MAX31328] = {
+		.int_en_reg = 0x0E,
+		.int_status_reg = 0x0F,
+		.sec_reg = 0x00,
+		.alarm1_sec_reg = 0x07,
+		.temp_reg = 0x11,
+		.clkout = &max31328_clkout,
+		.clkout_name = "max31328-sqw",
+	},
+	[ID_MAX31329] = {
+		.int_en_reg = 0x01,
+		.int_status_reg = 0x00,
+		.sec_reg = 0x06,
+		.alarm1_sec_reg = 0x0D,
+		.ram_reg = 0x22,
+		.ram_size = 64,
+		.trickle_reg = 0x19,
+		.clkout = &max31329_clkout,
+		.clkout_name = "max31329-clkout",
+	},
+	[ID_MAX31331] = {
+		.int_en_reg = 0x01,
+		.int_status_reg = 0x00,
+		.sec_reg = 0x08,
+		.alarm1_sec_reg = 0x0F,
+		.ram_reg = 0x20,
+		.ram_size = 32,
+		.trickle_reg = 0x1B,
+		.clkout = &max3133x_clkout,
+		.clkout_name = "max31331-clkout",
+	},
+	[ID_MAX31334] = {
+		.int_en_reg = 0x01,
+		.int_status_reg = 0x00,
+		.sec_reg = 0x09,
+		.alarm1_sec_reg = 0x10,
+		.ram_reg = 0x30,
+		.ram_size = 32,
+		.trickle_reg = 0x1E,
+		.clkout = &max3133x_clkout,
+		.clkout_name = "max31334-clkout",
+	},
+	[ID_MAX31341] = {
+		.int_en_reg = 0x04,
+		.int_status_reg = 0x05,
+		.sec_reg = 0x06,
+		.alarm1_sec_reg = 0x0D,
+		.ram_reg = 0x16,
+		.ram_size = 64,
+		.trickle_reg = 0x57,
+		.clkout = &max31341_42_clkout,
+		.clkout_name = "max31341-clkout",
+	},
+	[ID_MAX31342] = {
+		.int_en_reg = 0x04,
+		.int_status_reg = 0x05,
+		.sec_reg = 0x06,
+		.alarm1_sec_reg = 0x0D,
+		.clkout = &max31341_42_clkout,
+		.clkout_name = "max31342-clkout",
+	},
+	[ID_MAX31343] = {
+		.int_en_reg = 0x01,
+		.int_status_reg = 0x00,
+		.sec_reg = 0x06,
+		.alarm1_sec_reg = 0x0D,
+		.ram_reg = 0x22,
+		.ram_size = 64,
+		.temp_reg = 0x1A,
+		.trickle_reg = 0x19,
+		.clkout = &max31343_clkout,
+		.clkout_name = "max31343-clko",
+	},
+};
+
+static const u32 max313xx_trickle_ohms[] = { 3000, 6000, 11000 };
+
+static bool max313xx_volatile_reg(struct device *dev, unsigned int reg)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	const struct chip_desc *chip = rtc->chip;
+
+	/* time keeping registers */
+	if (reg >= chip->sec_reg && reg < chip->sec_reg + MAX313XX_TIME_SIZE)
+		return true;
+
+	/* interrupt status register */
+	if (reg == chip->int_status_reg)
+		return true;
+
+	/* temperature registers */
+	return chip->temp_reg &&
+		(reg == chip->temp_reg || reg == chip->temp_reg + 1);
+}
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = max313xx_volatile_reg,
+};
+
+static int max313xx_get_hour(u8 hour_reg)
+{
+	int hour;
+
+	/* 24Hr mode */
+	if (!FIELD_GET(MAX313XX_HRS_F_12_24, hour_reg))
+		return bcd2bin(hour_reg & 0x3f);
+
+	/* 12Hr mode */
+	hour = bcd2bin(hour_reg & 0x1f);
+	if (hour == 12)
+		hour = 0;
+
+	if (FIELD_GET(MAX313XX_HRS_F_AM_PM, hour_reg))
+		hour += 12;
+
+	return hour;
+}
+
+static int max313xx_read_time(struct device *dev, struct rtc_time *t)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	u8 regs[7];
+	int ret;
+
+	ret = regmap_bulk_read(rtc->regmap, rtc->chip->sec_reg, regs, 7);
+	if (ret)
+		return ret;
+
+	t->tm_sec = bcd2bin(regs[0] & 0x7f);
+	t->tm_min = bcd2bin(regs[1] & 0x7f);
+	t->tm_hour = max313xx_get_hour(regs[2]);
+	t->tm_wday = bcd2bin(regs[3] & 0x07) - 1;
+	t->tm_mday = bcd2bin(regs[4] & 0x3f);
+	t->tm_mon = bcd2bin(regs[5] & 0x1f) - 1;
+	t->tm_year = bcd2bin(regs[6]) + 100;
+
+	if (FIELD_GET(MAX313XX_MONTH_CENTURY, regs[5]))
+		t->tm_year += 100;
+
+	return 0;
+}
+
+static int max313xx_pre_set_time(struct max313xx *rtc)
+{
+	if (!rtc->clkin)
+		return 0;
+
+	/* Clkin needs to be disabled before setting time. */
+	switch (rtc->id) {
+	case ID_MAX31341:
+	case ID_MAX31342:
+		return regmap_clear_bits(rtc->regmap,
+					 MAX31341_42_CONFIG1_REG,
+					 MAX31341_42_CONFIG1_CLKIN_EN);
+	default:
+		return 0;
+	}
+}
+
+static int max313xx_post_set_time(struct max313xx *rtc)
+{
+	int ret;
+
+	switch (rtc->id) {
+	case ID_MAX31341:
+	case ID_MAX31342:
+		ret = regmap_set_bits(rtc->regmap, MAX3134X_CFG2_REG,
+				      MAX3134X_CFG2_SET_RTC);
+		if (ret)
+			return ret;
+
+		fsleep(10000);
+
+		ret = regmap_clear_bits(rtc->regmap, MAX3134X_CFG2_REG,
+					MAX3134X_CFG2_SET_RTC);
+		if (ret)
+			return ret;
+
+		if (rtc->clkin)
+			ret = regmap_set_bits(rtc->regmap,
+					      MAX31341_42_CONFIG1_REG,
+					      MAX31341_42_CONFIG1_CLKIN_EN);
+
+		break;
+	default:
+		return 0;
+	}
+
+	return ret;
+}
+
+static int max313xx_set_time(struct device *dev, struct rtc_time *t)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	u8 regs[7];
+	int ret;
+
+	regs[0] = bin2bcd(t->tm_sec);
+	regs[1] = bin2bcd(t->tm_min);
+	regs[2] = bin2bcd(t->tm_hour);
+	regs[3] = bin2bcd(t->tm_wday + 1);
+	regs[4] = bin2bcd(t->tm_mday);
+	regs[5] = bin2bcd(t->tm_mon + 1);
+	regs[6] = bin2bcd(t->tm_year % 100);
+
+	if (t->tm_year >= 200)
+		regs[5] |= FIELD_PREP(MAX313XX_MONTH_CENTURY, 1);
+
+	ret = max313xx_pre_set_time(rtc);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_write(rtc->regmap, rtc->chip->sec_reg, regs, 7);
+	if (ret)
+		return ret;
+
+	return max313xx_post_set_time(rtc);
+}
+
+static int max313xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	unsigned int status, int_en;
+	struct rtc_time time;
+	u8 regs[6];
+	int ret;
+
+	ret = regmap_bulk_read(rtc->regmap, rtc->chip->alarm1_sec_reg, regs,
+			       sizeof(regs));
+	if (ret)
+		return ret;
+
+	t->time.tm_sec = bcd2bin(regs[0] & 0x7f);
+	t->time.tm_min = bcd2bin(regs[1] & 0x7f);
+	t->time.tm_hour = bcd2bin(regs[2] & 0x3f);
+	t->time.tm_mday = bcd2bin(regs[3] & 0x3f);
+	t->time.tm_mon = bcd2bin(regs[4] & 0x1f) - 1;
+	t->time.tm_year = bcd2bin(regs[5]) + 100;
+
+	ret = max313xx_read_time(dev, &time);
+	if (ret)
+		return ret;
+
+	if (time.tm_year >= 200)
+		t->time.tm_year += 100;
+
+	ret = regmap_read(rtc->regmap, rtc->chip->int_status_reg, &status);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(rtc->regmap, rtc->chip->int_en_reg, &int_en);
+	if (ret)
+		return ret;
+
+	t->enabled = FIELD_GET(MAX313XX_INT_ALARM1, int_en);
+	t->pending = FIELD_GET(MAX313XX_INT_ALARM1, status);
+
+	return 0;
+}
+
+static int max313xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	unsigned int reg;
+	u8 regs[6];
+	int ret;
+
+	regs[0] = bin2bcd(t->time.tm_sec);
+	regs[1] = bin2bcd(t->time.tm_min);
+	regs[2] = bin2bcd(t->time.tm_hour);
+	regs[3] = bin2bcd(t->time.tm_mday);
+	regs[4] = bin2bcd(t->time.tm_mon + 1);
+	regs[5] = bin2bcd(t->time.tm_year % 100);
+
+	ret = regmap_bulk_write(rtc->regmap, rtc->chip->alarm1_sec_reg, regs,
+				sizeof(regs));
+	if (ret)
+		return ret;
+
+	reg = FIELD_PREP(MAX313XX_INT_ALARM1, t->enabled);
+	ret = regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
+				 MAX313XX_INT_ALARM1, reg);
+	if (ret)
+		return ret;
+
+	/* Clear status register */
+	return regmap_read(rtc->regmap, rtc->chip->int_status_reg, &reg);
+}
+
+static int max313xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+
+	return regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
+				  MAX313XX_INT_ALARM1,
+				  FIELD_PREP(MAX313XX_INT_ALARM1, enabled));
+}
+
+static const struct rtc_class_ops max3133x_rtc_ops = {
+	.read_time	= max313xx_read_time,
+	.set_time	= max313xx_set_time,
+	.read_alarm	= max313xx_read_alarm,
+	.set_alarm	= max313xx_set_alarm,
+	.alarm_irq_enable = max313xx_alarm_irq_enable,
+};
+
+static irqreturn_t max313xx_irq(int irq, void *dev_id)
+{
+	struct max313xx	*rtc = dev_id;
+	struct mutex *lock = &rtc->rtc->ops_lock;
+	int stat, ret;
+
+	mutex_lock(lock);
+	ret = regmap_read(rtc->regmap, rtc->chip->int_status_reg, &stat);
+	if (ret)
+		goto out;
+
+	if (FIELD_GET(MAX313XX_INT_ALARM1, stat)) {
+		ret = regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
+					 MAX313XX_INT_ALARM1, 0);
+		if (ret)
+			goto out;
+
+		rtc_update_irq(rtc->rtc, 1, RTC_AF | RTC_IRQF);
+	}
+
+out:
+	mutex_unlock(lock);
+
+	return IRQ_HANDLED;
+}
+
+static int max313xx_nvmem_reg_read(void *priv, unsigned int offset,
+				   void *val, size_t bytes)
+{
+	struct max313xx *rtc = priv;
+	unsigned int reg = rtc->chip->ram_reg + offset;
+
+	return regmap_bulk_read(rtc->regmap, reg, val, bytes);
+}
+
+static int max313xx_nvmem_reg_write(void *priv, unsigned int offset,
+				    void *val, size_t bytes)
+{
+	struct max313xx *rtc = priv;
+	unsigned int reg = rtc->chip->ram_reg + offset;
+
+	return regmap_bulk_write(rtc->regmap, reg, val, bytes);
+}
+
+struct nvmem_config max313xx_nvmem_cfg = {
+	.reg_read = max313xx_nvmem_reg_read,
+	.reg_write = max313xx_nvmem_reg_write,
+	.word_size = 8,
+};
+
+static unsigned long max313xx_clkout_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	struct max313xx *rtc = clk_hw_to_max313xx(hw);
+	const struct clkout_cfg *clkout = rtc->chip->clkout;
+	unsigned int freq_mask;
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(rtc->regmap, clkout->reg, &reg);
+	if (ret)
+		return 0;
+
+	freq_mask = __roundup_pow_of_two(clkout->freq_size) - 1;
+
+	return clkout->freq_avail[(reg >> clkout->freq_pos) & freq_mask];
+}
+
+static long max313xx_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
+				       unsigned long *prate)
+{
+	struct max313xx *rtc = clk_hw_to_max313xx(hw);
+	struct clkout_cfg *clkout = rtc->chip->clkout;
+	int index;
+
+	index = find_closest(rate, clkout->freq_avail, clkout->freq_size);
+	return clkout->freq_avail[index];
+}
+
+static int max313xx_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long parent_rate)
+{
+	struct max313xx *rtc = clk_hw_to_max313xx(hw);
+	struct clkout_cfg *clkout = rtc->chip->clkout;
+	unsigned int freq_mask;
+	int index;
+
+	index = find_closest(rate, clkout->freq_avail, clkout->freq_size);
+	freq_mask = __roundup_pow_of_two(clkout->freq_size) - 1;
+
+	return regmap_update_bits(rtc->regmap, clkout->reg,
+				  freq_mask << clkout->freq_pos,
+				  index << clkout->freq_pos);
+}
+
+static int max313xx_clkout_enable(struct clk_hw *hw)
+{
+	struct max313xx *rtc = clk_hw_to_max313xx(hw);
+	struct clkout_cfg *clkout = rtc->chip->clkout;
+
+	if (clkout->en_invert)
+		return regmap_clear_bits(rtc->regmap, clkout->reg,
+					 clkout->en_bit);
+
+	return regmap_set_bits(rtc->regmap, clkout->reg,  clkout->en_bit);
+}
+
+static void max313xx_clkout_disable(struct clk_hw *hw)
+{
+	struct max313xx *rtc = clk_hw_to_max313xx(hw);
+	struct clkout_cfg *clkout = rtc->chip->clkout;
+
+	switch (rtc->id) {
+	case ID_MAX31331:
+	case ID_MAX31334:
+		if (rtc->irq > 0) {
+			dev_err(rtc->rtc->dev.parent,
+				"clkout cannot be disabled when IRQ is requested");
+			return;
+		}
+		break;
+	default:
+		break;
+	}
+
+	if (clkout->en_invert)
+		regmap_set_bits(rtc->regmap, clkout->reg, clkout->en_bit);
+	else
+		regmap_clear_bits(rtc->regmap, clkout->reg,  clkout->en_bit);
+}
+
+static int max313xx_clkout_is_enabled(struct clk_hw *hw)
+{
+	struct max313xx *rtc = clk_hw_to_max313xx(hw);
+	struct clkout_cfg *clkout = rtc->chip->clkout;
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(rtc->regmap, clkout->reg, &reg);
+	if (ret)
+		return ret;
+
+	return !!(reg & clkout->en_bit) ^ clkout->en_invert;
+}
+
+static const struct clk_ops max313xx_clkout_ops = {
+	.recalc_rate = max313xx_clkout_recalc_rate,
+	.round_rate = max313xx_clkout_round_rate,
+	.set_rate = max313xx_clkout_set_rate,
+	.enable = max313xx_clkout_enable,
+	.disable = max313xx_clkout_disable,
+	.is_enabled = max313xx_clkout_is_enabled,
+};
+
+struct clk_init_data max313xx_clk_init = {
+	.name = "max313xx-clkout",
+	.ops = &max313xx_clkout_ops,
+};
+
+static int max313xx_read_temp(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *val)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	u8 reg[2];
+	s16 temp;
+	int ret;
+
+	if (type != hwmon_temp || attr != hwmon_temp_input)
+		return -EOPNOTSUPP;
+
+	ret = regmap_bulk_read(rtc->regmap, rtc->chip->temp_reg, reg, 2);
+	if (ret)
+		return ret;
+
+	temp = get_unaligned_be16(reg);
+
+	*val = (temp / 64) * 250;
+
+	return 0;
+}
+
+static umode_t max313xx_is_visible(const void *data,
+				   enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	if (type == hwmon_temp && attr == hwmon_temp_input)
+		return 0444;
+
+	return 0;
+}
+
+static const struct hwmon_channel_info *max313xx_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops max313xx_hwmon_ops = {
+	.is_visible = max313xx_is_visible,
+	.read = max313xx_read_temp,
+};
+
+static const struct hwmon_chip_info max313xx_chip_info = {
+	.ops = &max313xx_hwmon_ops,
+	.info = max313xx_info,
+};
+
+static int max313xx_init(struct max313xx *rtc)
+{
+	int ret;
+
+	switch (rtc->id) {
+	case ID_MAX31341:
+	case ID_MAX31342:
+		ret = regmap_clear_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
+					MAX31341_42_CONFIG1_OSC_DISABLE);
+		if (ret)
+			return ret;
+
+		return regmap_set_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
+				       MAX31341_42_CONFIG1_SWRST);
+	default:
+		return 0;
+	}
+}
+
+static int max313xx_clkout_register(struct device *dev)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	int ret;
+
+	if (!device_property_present(dev, "#clock-cells"))
+		return 0;
+
+	max313xx_clk_init.name = rtc->chip->clkout_name;
+	device_property_read_string(dev, "clock-output-names",
+				    &max313xx_clk_init.name);
+	rtc->clkout.init = &max313xx_clk_init;
+
+	ret = devm_clk_hw_register(dev, &rtc->clkout);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot register clock\n");
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					   &rtc->clkout);
+}
+
+static int max313xx_trickle_charger_setup(struct device *dev)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	bool trickle_diode_en;
+	u32 charger_en = 0;
+	int index, reg;
+	u32 ohms = 0;
+	int ret;
+
+	device_property_read_u32(dev, "aux-voltage-chargeable", &charger_en);
+	switch (charger_en) {
+	case 0:
+		return 0;
+	case 1:
+		trickle_diode_en = 0;
+		break;
+	case 2:
+		trickle_diode_en = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	device_property_read_u32(dev, "trickle-resistor-ohms", &ohms);
+
+	if (!rtc->chip->trickle_reg)
+		return 0;
+
+	index = find_closest(ohms, max313xx_trickle_ohms,
+			     ARRAY_SIZE(max313xx_trickle_ohms)) + 1;
+
+	switch (rtc->id) {
+	case ID_MAX31329:
+		reg = FIELD_PREP(MAX31329_TRICKLE_ENABLE_BIT, 1) |
+		      FIELD_PREP(MAX31329_43_TRICKLE_RES_MASK, index) |
+		      FIELD_PREP(MAX31329_43_TRICKLE_DIODE_EN, trickle_diode_en);
+		break;
+	case ID_MAX31331:
+	case ID_MAX31334:
+		reg = FIELD_PREP(MAX3133X_TRICKLE_ENABLE_BIT, 1) |
+		      FIELD_PREP(MAX3133X_TRICKLE_RES_MASK, index) |
+		      FIELD_PREP(MAX3133X_TRICKLE_DIODE_EN, trickle_diode_en);
+		break;
+	case ID_MAX31341:
+		if (index == 1)
+			index = 0;
+
+		reg = FIELD_PREP(MAX31341_TRICKLE_ENABLE_BIT, 1) |
+		      FIELD_PREP(MAX31341_TRICKLE_RES_MASK, index) |
+		      FIELD_PREP(MAX31341_TRICKLE_DIODE_EN, trickle_diode_en);
+
+		ret = regmap_set_bits(rtc->regmap, MAX31341_POWER_MGMT_REG,
+				      MAX31341_POWER_MGMT_TRICKLE_BIT);
+		if (ret)
+			return ret;
+
+		break;
+	case ID_MAX31343:
+		reg = FIELD_PREP(MAX31329_43_TRICKLE_RES_MASK, index) |
+		      FIELD_PREP(MAX31329_43_TRICKLE_DIODE_EN, trickle_diode_en) |
+		      FIELD_PREP(MAX31343_TRICKLE_ENABLE_MASK,
+				 MAX31343_TRICKLE_ENABLE_CODE);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return regmap_write(rtc->regmap, rtc->chip->trickle_reg, reg);
+}
+
+static int max313xx_find_clkin_freq_index(struct clk *clk)
+{
+	unsigned long rate = clk_get_rate(clk);
+	int freq;
+	int i;
+
+	i = find_closest(rate, max313xx_clkin_freq,
+			 ARRAY_SIZE(max313xx_clkin_freq));
+	if (max313xx_clkin_freq[i] == rate)
+		return i;
+
+	for (i = ARRAY_SIZE(max313xx_clkin_freq) - 1; i >= 0; i--) {
+		freq = max313xx_clkin_freq[i];
+		if (freq == clk_round_rate(clk, freq))
+			return i;
+	}
+
+	/* supplied clock cannot produce one of desired frequency rate */
+	return -ENODEV;
+}
+
+static int max313xx_clkin_init(struct device *dev)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	int rate;
+	int ret;
+
+	rtc->clkin = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(rtc->clkin)) {
+		if (PTR_ERR(rtc->clkin) == -ENOENT)
+			rtc->clkin = NULL;
+		else
+			return dev_err_probe(dev, PTR_ERR(rtc->clkin),
+					     "error while clkin setup\n");
+	}
+
+	if (!rtc->clkin) {
+		switch (rtc->id) {
+		case ID_MAX31329:
+			return regmap_clear_bits(rtc->regmap,
+						 MAX31329_CONFIG2_REG,
+						 MAX31329_CONFIG2_CLKIN_EN);
+		case ID_MAX31341:
+		case ID_MAX31342:
+			return regmap_clear_bits(rtc->regmap,
+						 MAX31341_42_CONFIG1_REG,
+						 MAX31341_42_CONFIG1_CLKIN_EN);
+		default:
+			return 0;
+		}
+	}
+
+	rate = max313xx_find_clkin_freq_index(rtc->clkin);
+	if (rate < 0)
+		return dev_err_probe(dev, rate,
+				     "clkin cannot produce required frequency\n");
+
+	ret = clk_set_rate(rtc->clkin, max313xx_clkin_freq[rate]);
+	if (ret)
+		return ret;
+
+	switch (rtc->id) {
+	case ID_MAX31329:
+		ret = regmap_update_bits(rtc->regmap, MAX31329_CONFIG2_REG,
+					 MAX31329_CONFIG2_CLKIN_FREQ, rate);
+		if (ret)
+			return ret;
+
+		return regmap_set_bits(rtc->regmap, MAX31329_CONFIG2_REG,
+				       MAX31329_CONFIG2_CLKIN_EN);
+	case ID_MAX31341:
+	case ID_MAX31342:
+		ret = regmap_update_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
+					 MAX31341_42_CONFIG1_CLKIN_FREQ,
+					 FIELD_PREP(MAX31341_42_CONFIG1_CLKIN_FREQ, rate));
+		if (ret)
+			return ret;
+
+		return regmap_set_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
+				       MAX31341_42_CONFIG1_CLKIN_EN);
+	default:
+		rtc->clkin = NULL;
+		dev_warn(dev, "device does not have clock input\n");
+		return 0;
+	}
+}
+
+static int max313xx_irq_init(struct device *dev, const char *devname)
+{
+	struct max313xx *rtc = dev_get_drvdata(dev);
+	int ret;
+
+	switch (rtc->id) {
+	case ID_MAX31328:
+		break;
+	case ID_MAX31331:
+	case ID_MAX31334:
+		if (rtc->clkout.clk) {
+			/*
+			 * interrupt muxing depends on clkout so enable clkout
+			 * if configured before requesting interrupt
+			 */
+			ret = clk_prepare_enable(rtc->clkout.clk);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "cannot enable clkout\n");
+		}
+		break;
+	default:
+		if (rtc->clkin) {
+			if (rtc->clkout.clk && rtc->irq > 0)
+				return dev_err_probe(dev, -EOPNOTSUPP,
+						     "irq not possible when both clkin and clkout are configured\n");
+
+			if (rtc->irq <= 0)
+				break;
+
+			/* clkout needs to be disabled for interrupt */
+			if (rtc->chip->clkout->en_invert)
+				ret = regmap_set_bits(rtc->regmap,
+						      rtc->chip->clkout->reg,
+						      rtc->chip->clkout->en_bit);
+			else
+				ret = regmap_clear_bits(rtc->regmap,
+							rtc->chip->clkout->reg,
+							rtc->chip->clkout->en_bit);
+
+			if (ret)
+				return ret;
+		}
+		break;
+	}
+
+	if (rtc->irq > 0) {
+		return devm_request_threaded_irq(dev, rtc->irq, NULL,
+						 &max313xx_irq, IRQF_ONESHOT,
+						 devname, rtc);
+	} else if (device_property_read_bool(dev, "wakeup-source")) {
+		clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtc->features);
+		return device_init_wakeup(dev, true);
+	}
+
+	clear_bit(RTC_FEATURE_ALARM, rtc->rtc->features);
+
+	return 0;
+}
+
+static int max313xx_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct max313xx *max313xx;
+	struct device *hwmon;
+	const void *match;
+	int ret;
+
+	max313xx = devm_kzalloc(&client->dev, sizeof(*max313xx), GFP_KERNEL);
+	if (!max313xx)
+		return -ENOMEM;
+
+	dev_set_drvdata(&client->dev, max313xx);
+
+	max313xx->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(max313xx->regmap)) {
+		return dev_err_probe(dev, PTR_ERR(max313xx->regmap),
+				     "regmap init failed\n");
+	}
+
+	i2c_set_clientdata(client, max313xx);
+
+	match = device_get_match_data(dev);
+	if (match)
+		max313xx->chip = match;
+	else if (id)
+		max313xx->chip = (struct chip_desc *)id->driver_data;
+	else
+		return -ENODEV;
+
+	max313xx->id = max313xx->chip - chip;
+
+	ret = max313xx_init(max313xx);
+	if (ret)
+		return ret;
+
+	max313xx->rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(max313xx->rtc))
+		return PTR_ERR(max313xx->rtc);
+
+	max313xx->rtc->ops = &max3133x_rtc_ops;
+	max313xx->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	max313xx->rtc->range_max = RTC_TIMESTAMP_END_2199;
+
+	ret = devm_rtc_register_device(max313xx->rtc);
+	if (ret)
+		return ret;
+
+	max313xx->irq = client->irq;
+
+	ret = max313xx_clkout_register(dev);
+	if (ret)
+		return ret;
+
+	ret = max313xx_clkin_init(dev);
+	if (ret)
+		return ret;
+
+	/* IRQ wiring depends on the clock configuration so parse them first */
+	ret = max313xx_irq_init(dev, client->name);
+	if (ret)
+		return ret;
+
+	if (max313xx->chip->ram_size) {
+		max313xx_nvmem_cfg.size = max313xx->chip->ram_size;
+		max313xx_nvmem_cfg.priv = max313xx;
+
+		ret = devm_rtc_nvmem_register(max313xx->rtc, &max313xx_nvmem_cfg);
+		if (ret)
+			dev_warn(dev, "cannot register rtc nvmem\n");
+	}
+
+	if (max313xx->chip->temp_reg) {
+		hwmon = devm_hwmon_device_register_with_info(dev, client->name,
+							     max313xx,
+							     &max313xx_chip_info,
+							     NULL);
+		if (IS_ERR(hwmon))
+			dev_warn(dev, "cannot register hwmon device: %li\n",
+				 PTR_ERR(hwmon));
+	}
+
+	return max313xx_trickle_charger_setup(dev);
+}
+
+static const struct of_device_id max313xx_of_id[] = {
+	{ .compatible = "adi,max31328", .data = &chip[ID_MAX31328] },
+	{ .compatible = "adi,max31329", .data = &chip[ID_MAX31329] },
+	{ .compatible = "adi,max31331", .data = &chip[ID_MAX31331] },
+	{ .compatible = "adi,max31334", .data = &chip[ID_MAX31334] },
+	{ .compatible = "adi,max31341", .data = &chip[ID_MAX31341] },
+	{ .compatible = "adi,max31342", .data = &chip[ID_MAX31342] },
+	{ .compatible = "adi,max31343", .data = &chip[ID_MAX31343] },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max313xx_of_id);
+
+static const struct i2c_device_id max313xx_id[] = {
+	{ "max31328", (kernel_ulong_t)&chip[ID_MAX31328] },
+	{ "max31329", (kernel_ulong_t)&chip[ID_MAX31329] },
+	{ "max31331", (kernel_ulong_t)&chip[ID_MAX31331] },
+	{ "max31334", (kernel_ulong_t)&chip[ID_MAX31334] },
+	{ "max31341", (kernel_ulong_t)&chip[ID_MAX31341] },
+	{ "max31342", (kernel_ulong_t)&chip[ID_MAX31342] },
+	{ "max31343", (kernel_ulong_t)&chip[ID_MAX31343] },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max313xx_id);
+
+static struct i2c_driver max313xx_driver = {
+	.driver = {
+		.name	= "rtc-max313xx",
+		.of_match_table = max313xx_of_id,
+	},
+	.probe		= max313xx_probe,
+	.id_table	= max313xx_id,
+};
+module_i2c_driver(max313xx_driver);
+
+MODULE_DESCRIPTION("Analog Devices MAX313XX RTCs");
+MODULE_AUTHOR("Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>");
+MODULE_AUTHOR("Ibrahim Tilki <Ibrahim.Tilki@analog.com>");
+MODULE_SOFTDEP("pre: regmap-i2c");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");
-- 
2.25.1

[PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Ibrahim Tilki]

Devicetree binding documentation for Analog Devices MAX313XX RTCs

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
---
 .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml

diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
new file mode 100644
index 000000000..0c17a395e
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
@@ -0,0 +1,144 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX313XX series I2C RTCs
+
+maintainers:
+  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
+  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
+
+description: Analog Devices MAX313XX series I2C RTCs.
+
+properties:
+  compatible:
+    enum:
+      - adi,max31328
+      - adi,max31329
+      - adi,max31331
+      - adi,max31334
+      - adi,max31341
+      - adi,max31342
+      - adi,max31343
+
+  reg:
+    description: I2C address of the RTC
+    items:
+      - enum: [0x68, 0x69]
+
+  interrupts:
+    description: |
+      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
+      lines and alarm1 interrupt muxing depends on the clockin/clockout
+      configuration.
+    maxItems: 1
+
+  "#clock-cells":
+    description: |
+      RTC can be used as a clock source through its clock output pin when
+      supplied.
+    const: 0
+
+  clocks:
+    description: |
+      RTC uses this clock for clock input when supplied. Clock has to provide
+      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
+    maxItems: 1
+
+  aux-voltage-chargeable:
+    enum: [0, 1, 2]
+    description: |
+      Enables trickle charger.
+      0: Charger is disabled (default)
+      1: Charger is enabled
+      2: Charger is enabled with a diode
+
+  trickle-resistor-ohms:
+    description: Selected resistor for trickle charger.
+    enum: [3000, 6000, 11000]
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: rtc.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,max31328
+              - adi,max31342
+
+    then:
+      properties:
+        aux-voltage-chargeable: false
+        trickle-resistor-ohms: false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,max31328
+              - adi,max31331
+              - adi,max31334
+              - adi,max31343
+
+    then:
+      properties:
+        clocks: false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,max31341
+              - adi,max31342
+
+    then:
+      properties:
+        reg:
+          items:
+            - const: 0x69
+
+    else:
+      properties:
+        reg:
+          items:
+            - const: 0x68
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@68 {
+            reg = <0x68>;
+            compatible = "adi,max31329";
+            clocks = <&clkin>;
+            interrupt-parent = <&gpio>;
+            interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@68 {
+            reg = <0x68>;
+            compatible = "adi,max31331";
+            #clock-cells = <0>;
+        };
+    };
-- 
2.25.1

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 03/04/2023 17:43, Ibrahim Tilki wrote:
> Devicetree binding documentation for Analog Devices MAX313XX RTCs
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> new file mode 100644
> index 000000000..0c17a395e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX313XX series I2C RTCs
> +
> +maintainers:
> +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> +
> +description: Analog Devices MAX313XX series I2C RTCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31328
> +      - adi,max31329
> +      - adi,max31331
> +      - adi,max31334
> +      - adi,max31341
> +      - adi,max31342
> +      - adi,max31343
> +
> +  reg:
> +    description: I2C address of the RTC
> +    items:
> +      - enum: [0x68, 0x69]
> +
> +  interrupts:
> +    description: |

Do not need '|'.

> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> +      configuration.
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    description: |

Do not need '|'.

> +      RTC can be used as a clock source through its clock output pin when
> +      supplied.
> +    const: 0
> +
> +  clocks:
> +    description: |

Do not need '|'.

> +      RTC uses this clock for clock input when supplied. Clock has to provide
> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> +    maxItems: 1
> +
> +  aux-voltage-chargeable:
> +    enum: [0, 1, 2]
> +    description: |
> +      Enables trickle charger.
> +      0: Charger is disabled (default)
> +      1: Charger is enabled
> +      2: Charger is enabled with a diode

2 is not an allowed value. I asked to drop this property. It is coming
from rtc.yaml. I also do not understand "with a diode". So otherwise it
is charging with, I don't know, FET?

> +
> +  trickle-resistor-ohms:
> +    description: Selected resistor for trickle charger.
> +    enum: [3000, 6000, 11000]
> +
> +required:
> +  - compatible
> +  - reg
> +
Best regards,
Krzysztof

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Alexandre Belloni]

On 04/04/2023 08:17:40+0200, Krzysztof Kozlowski wrote:
> On 03/04/2023 17:43, Ibrahim Tilki wrote:
> > Devicetree binding documentation for Analog Devices MAX313XX RTCs
> > 
> > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > ---
> >  .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
> >  1 file changed, 144 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > new file mode 100644
> > index 000000000..0c17a395e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > @@ -0,0 +1,144 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2022 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX313XX series I2C RTCs
> > +
> > +maintainers:
> > +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > +
> > +description: Analog Devices MAX313XX series I2C RTCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max31328
> > +      - adi,max31329
> > +      - adi,max31331
> > +      - adi,max31334
> > +      - adi,max31341
> > +      - adi,max31342
> > +      - adi,max31343
> > +
> > +  reg:
> > +    description: I2C address of the RTC
> > +    items:
> > +      - enum: [0x68, 0x69]
> > +
> > +  interrupts:
> > +    description: |
> 
> Do not need '|'.
> 
> > +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> > +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> > +      configuration.
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    description: |
> 
> Do not need '|'.
> 
> > +      RTC can be used as a clock source through its clock output pin when
> > +      supplied.
> > +    const: 0
> > +
> > +  clocks:
> > +    description: |
> 
> Do not need '|'.
> 
> > +      RTC uses this clock for clock input when supplied. Clock has to provide
> > +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> > +    maxItems: 1
> > +
> > +  aux-voltage-chargeable:
> > +    enum: [0, 1, 2]
> > +    description: |
> > +      Enables trickle charger.
> > +      0: Charger is disabled (default)
> > +      1: Charger is enabled
> > +      2: Charger is enabled with a diode
> 
> 2 is not an allowed value. I asked to drop this property. It is coming
> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> is charging with, I don't know, FET?

No, what is not explained here (and maybe not unsterstood by the
submitter) is that the RTC has an extra diode so, charging will always
enable a diode, select a resistor and then have or not an extra diode.
Figure2 of the MAX31329 datasheet is great.

> 
> > +
> > +  trickle-resistor-ohms:
> > +    description: Selected resistor for trickle charger.
> > +    enum: [3000, 6000, 11000]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> Best regards,
> Krzysztof
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 04/04/2023 09:10, Alexandre Belloni wrote:
>>
>>> +      RTC can be used as a clock source through its clock output pin when
>>> +      supplied.
>>> +    const: 0
>>> +
>>> +  clocks:
>>> +    description: |
>>
>> Do not need '|'.
>>
>>> +      RTC uses this clock for clock input when supplied. Clock has to provide
>>> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
>>> +    maxItems: 1
>>> +
>>> +  aux-voltage-chargeable:
>>> +    enum: [0, 1, 2]
>>> +    description: |
>>> +      Enables trickle charger.
>>> +      0: Charger is disabled (default)
>>> +      1: Charger is enabled
>>> +      2: Charger is enabled with a diode
>>
>> 2 is not an allowed value. I asked to drop this property. It is coming
>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>> is charging with, I don't know, FET?
> 
> No, what is not explained here (and maybe not unsterstood by the
> submitter) is that the RTC has an extra diode so, 

Value of 2 is still not allowed and if the patch was tested, it would be
clearly visible. Unfortunately patch was not tested...

> charging will always
> enable a diode, select a resistor and then have or not an extra diode.
> Figure2 of the MAX31329 datasheet is great.

So the diode is in the max313xx? Then why enabling it is a property of
DT? Either this should be inferred from compatible or is even a policy,
not a DT property. Just because device has a register for something, is
not an argument that "something" should be in DT.


Best regards,
Krzysztof

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Alexandre Belloni]

On 04/04/2023 09:21:56+0200, Krzysztof Kozlowski wrote:
> On 04/04/2023 09:10, Alexandre Belloni wrote:
> >>
> >>> +      RTC can be used as a clock source through its clock output pin when
> >>> +      supplied.
> >>> +    const: 0
> >>> +
> >>> +  clocks:
> >>> +    description: |
> >>
> >> Do not need '|'.
> >>
> >>> +      RTC uses this clock for clock input when supplied. Clock has to provide
> >>> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> >>> +    maxItems: 1
> >>> +
> >>> +  aux-voltage-chargeable:
> >>> +    enum: [0, 1, 2]
> >>> +    description: |
> >>> +      Enables trickle charger.
> >>> +      0: Charger is disabled (default)
> >>> +      1: Charger is enabled
> >>> +      2: Charger is enabled with a diode
> >>
> >> 2 is not an allowed value. I asked to drop this property. It is coming
> >> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> >> is charging with, I don't know, FET?
> > 
> > No, what is not explained here (and maybe not unsterstood by the
> > submitter) is that the RTC has an extra diode so, 
> 
> Value of 2 is still not allowed and if the patch was tested, it would be
> clearly visible. Unfortunately patch was not tested...
> 
> > charging will always
> > enable a diode, select a resistor and then have or not an extra diode.
> > Figure2 of the MAX31329 datasheet is great.
> 
> So the diode is in the max313xx? Then why enabling it is a property of
> DT? Either this should be inferred from compatible or is even a policy,
> not a DT property. Just because device has a register for something, is
> not an argument that "something" should be in DT.

Well, it depends on the battery that is installed on the board so it
makes sense to have it in DT.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 04/04/2023 09:44, Alexandre Belloni wrote:
>>
>>> charging will always
>>> enable a diode, select a resistor and then have or not an extra diode.
>>> Figure2 of the MAX31329 datasheet is great.
>>
>> So the diode is in the max313xx? Then why enabling it is a property of
>> DT? Either this should be inferred from compatible or is even a policy,
>> not a DT property. Just because device has a register for something, is
>> not an argument that "something" should be in DT.
> 
> Well, it depends on the battery that is installed on the board so it
> makes sense to have it in DT.

OK, that would be a good reason, but I wonder why? Why choosing diode or
not depends on the battery? Wouldn't you always want to have the diode?

Best regards,
Krzysztof

RE: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Tilki, Ibrahim]

> On 04/04/2023 08:17:40+0200, Krzysztof Kozlowski wrote:
> > On 03/04/2023 17:43, Ibrahim Tilki wrote:
> > > Devicetree binding documentation for Analog Devices MAX313XX RTCs
> > > 
> > > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > > Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > > ---
> > > .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
> > > 1 file changed, 144 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml \
> > > b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml new file mode 100644
> > > index 000000000..0c17a395e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > > @@ -0,0 +1,144 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright 2022 Analog Devices Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices MAX313XX series I2C RTCs
> > > +
> > > +maintainers:
> > > +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > > +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > > +
> > > +description: Analog Devices MAX313XX series I2C RTCs.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,max31328
> > > +      - adi,max31329
> > > +      - adi,max31331
> > > +      - adi,max31334
> > > +      - adi,max31341
> > > +      - adi,max31342
> > > +      - adi,max31343
> > > +
> > > +  reg:
> > > +    description: I2C address of the RTC
> > > +    items:
> > > +      - enum: [0x68, 0x69]
> > > +
> > > +  interrupts:
> > > +    description: |
> > 
> > Do not need '|'.
> > 
> > > +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> > > +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> > > +      configuration.
> > > +    maxItems: 1
> > > +
> > > +  "#clock-cells":
> > > +    description: |
> > 
> > Do not need '|'.
> > 
> > > +      RTC can be used as a clock source through its clock output pin when
> > > +      supplied.
> > > +    const: 0
> > > +
> > > +  clocks:
> > > +    description: |
> > 
> > Do not need '|'.
> > 
> > > +      RTC uses this clock for clock input when supplied. Clock has to provide
> > > +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> > > +    maxItems: 1
> > > +
> > > +  aux-voltage-chargeable:
> > > +    enum: [0, 1, 2]
> > > +    description: |
> > > +      Enables trickle charger.
> > > +      0: Charger is disabled (default)
> > > +      1: Charger is enabled
> > > +      2: Charger is enabled with a diode
> > 
> > 2 is not an allowed value. I asked to drop this property. It is coming
> > from rtc.yaml. I also do not understand "with a diode". So otherwise it
> > is charging with, I don't know, FET?
> 
> No, what is not explained here (and maybe not unsterstood by the
> submitter) is that the RTC has an extra diode so, charging will always
> enable a diode, select a resistor and then have or not an extra diode.
> Figure2 of the MAX31329 datasheet is great.
> 

That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
So if I can't have "adi,trickle-diode-enable" and can't add an additional value
to "aux-voltage-chargeable", I am not sure how to add support for the extra
diode at this point.

Best regards,
Ibrahim

> > 
> > > +
> > > +  trickle-resistor-ohms:
> > > +    description: Selected resistor for trickle charger.
> > > +    enum: [3000, 6000, 11000]
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > Best regards,
> > Krzysztof
> >

RE: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Tilki, Ibrahim]

> On 04/04/2023 09:44, Alexandre Belloni wrote:
> >>
> >>> charging will always
> >>> enable a diode, select a resistor and then have or not an extra diode.
> >>> Figure2 of the MAX31329 datasheet is great.
> >>
> >> So the diode is in the max313xx? Then why enabling it is a property of
> >> DT? Either this should be inferred from compatible or is even a policy,
> >> not a DT property. Just because device has a register for something, is
> >> not an argument that "something" should be in DT.
> > 
> > Well, it depends on the battery that is installed on the board so it
> > makes sense to have it in DT.
> 
> OK, that would be a good reason, but I wonder why? Why choosing diode or
> not depends on the battery? Wouldn't you always want to have the diode?
> 
> Best regards,
> Krzysztof

The datasheet doesn't say much about the extra diode but there is always a
schottky diode. The extra diode might be there to cause voltage drop.

Best regards,
Ibrahim

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Alexandre Belloni]

On 04/04/2023 10:14:33+0200, Krzysztof Kozlowski wrote:
> On 04/04/2023 09:44, Alexandre Belloni wrote:
> >>
> >>> charging will always
> >>> enable a diode, select a resistor and then have or not an extra diode.
> >>> Figure2 of the MAX31329 datasheet is great.
> >>
> >> So the diode is in the max313xx? Then why enabling it is a property of
> >> DT? Either this should be inferred from compatible or is even a policy,
> >> not a DT property. Just because device has a register for something, is
> >> not an argument that "something" should be in DT.
> > 
> > Well, it depends on the battery that is installed on the board so it
> > makes sense to have it in DT.
> 
> OK, that would be a good reason, but I wonder why? Why choosing diode or
> not depends on the battery? Wouldn't you always want to have the diode?
> 

It limits the maximum current used to charge the battery or supercap to
not exceed what is allowed.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 04/04/2023 11:56, Alexandre Belloni wrote:
> On 04/04/2023 10:14:33+0200, Krzysztof Kozlowski wrote:
>> On 04/04/2023 09:44, Alexandre Belloni wrote:
>>>>
>>>>> charging will always
>>>>> enable a diode, select a resistor and then have or not an extra diode.
>>>>> Figure2 of the MAX31329 datasheet is great.
>>>>
>>>> So the diode is in the max313xx? Then why enabling it is a property of
>>>> DT? Either this should be inferred from compatible or is even a policy,
>>>> not a DT property. Just because device has a register for something, is
>>>> not an argument that "something" should be in DT.
>>>
>>> Well, it depends on the battery that is installed on the board so it
>>> makes sense to have it in DT.
>>
>> OK, that would be a good reason, but I wonder why? Why choosing diode or
>> not depends on the battery? Wouldn't you always want to have the diode?
>>
> 
> It limits the maximum current used to charge the battery or supercap to
> not exceed what is allowed.

and I still wonder why someone would like to allow exceeding what is
allowed. IOW, what is the use case to disable the diode?

Best regards,
Krzysztof

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 04/04/2023 11:26, Tilki, Ibrahim wrote:
>>>> +  aux-voltage-chargeable:
>>>> +    enum: [0, 1, 2]
>>>> +    description: |
>>>> +      Enables trickle charger.
>>>> +      0: Charger is disabled (default)
>>>> +      1: Charger is enabled
>>>> +      2: Charger is enabled with a diode
>>>
>>> 2 is not an allowed value. I asked to drop this property. It is coming
>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>>> is charging with, I don't know, FET?
>>
>> No, what is not explained here (and maybe not unsterstood by the
>> submitter) is that the RTC has an extra diode so, charging will always
>> enable a diode, select a resistor and then have or not an extra diode.
>> Figure2 of the MAX31329 datasheet is great.
>>
> 
> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> diode at this point.

Ask the person who asked you to remove adi,trickle-diode-enable...
Anyway even if you decided to go with such advise, the solution is not
to create binding which fails.


Best regards,
Krzysztof

RE: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Tilki, Ibrahim]

>>>>> +  aux-voltage-chargeable:
>>>>> +    enum: [0, 1, 2]
>>>>> +    description: |
>>>>> +      Enables trickle charger.
>>>>> +      0: Charger is disabled (default)
>>>>> +      1: Charger is enabled
>>>>> +      2: Charger is enabled with a diode
>>>>
>>>> 2 is not an allowed value. I asked to drop this property. It is coming
>>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>>>> is charging with, I don't know, FET?
>>>
>>> No, what is not explained here (and maybe not unsterstood by the
>>> submitter) is that the RTC has an extra diode so, charging will always
>>> enable a diode, select a resistor and then have or not an extra diode.
>>> Figure2 of the MAX31329 datasheet is great.
>>>
>> 
>> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
>> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
>> to "aux-voltage-chargeable", I am not sure how to add support for the extra
>> diode at this point.
>
> Ask the person who asked you to remove adi,trickle-diode-enable...

That was the purpose.

> Anyway even if you decided to go with such advise, the solution is not
> to create binding which fails.

I didn't see that it would fail, should have checked the binding with using
the property in examples. I'm sorry about that.

Best regards,
Ibrahim

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Alexandre Belloni]

On 04/04/2023 12:06:14+0200, Krzysztof Kozlowski wrote:
> On 04/04/2023 11:56, Alexandre Belloni wrote:
> > On 04/04/2023 10:14:33+0200, Krzysztof Kozlowski wrote:
> >> On 04/04/2023 09:44, Alexandre Belloni wrote:
> >>>>
> >>>>> charging will always
> >>>>> enable a diode, select a resistor and then have or not an extra diode.
> >>>>> Figure2 of the MAX31329 datasheet is great.
> >>>>
> >>>> So the diode is in the max313xx? Then why enabling it is a property of
> >>>> DT? Either this should be inferred from compatible or is even a policy,
> >>>> not a DT property. Just because device has a register for something, is
> >>>> not an argument that "something" should be in DT.
> >>>
> >>> Well, it depends on the battery that is installed on the board so it
> >>> makes sense to have it in DT.
> >>
> >> OK, that would be a good reason, but I wonder why? Why choosing diode or
> >> not depends on the battery? Wouldn't you always want to have the diode?
> >>
> > 
> > It limits the maximum current used to charge the battery or supercap to
> > not exceed what is allowed.
> 
> and I still wonder why someone would like to allow exceeding what is
> allowed. IOW, what is the use case to disable the diode?
> 

The battery or supercap is the part defining the current limit. why
would you want to limit the current if you can charge faster?


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Alexandre Belloni]

On 04/04/2023 10:35:53+0000, Tilki, Ibrahim wrote:
> >>>>> +  aux-voltage-chargeable:
> >>>>> +    enum: [0, 1, 2]
> >>>>> +    description: |
> >>>>> +      Enables trickle charger.
> >>>>> +      0: Charger is disabled (default)
> >>>>> +      1: Charger is enabled
> >>>>> +      2: Charger is enabled with a diode
> >>>>
> >>>> 2 is not an allowed value. I asked to drop this property. It is coming
> >>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> >>>> is charging with, I don't know, FET?
> >>>
> >>> No, what is not explained here (and maybe not unsterstood by the
> >>> submitter) is that the RTC has an extra diode so, charging will always
> >>> enable a diode, select a resistor and then have or not an extra diode.
> >>> Figure2 of the MAX31329 datasheet is great.
> >>>
> >> 
> >> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> >> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> >> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> >> diode at this point.
> >
> > Ask the person who asked you to remove adi,trickle-diode-enable...
> 
> That was the purpose.
> 

If the earlier submission was clearer my answer would have been
different but note how I had to dig up the datasheet to understand there
were two diodes. All the trickle chargers have a schottky diode so
"adi,trickle-diode-enable" nor the commit log were explicit about the
second diode (which is a regular diode).

aux-voltage-chargeable is enabling a diode on all the existing RTC
drivers so instead of trying to make me look like the bad guy you should
rather thank for taking the time trying to get better DT bindings.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Alexandre Belloni]

On 04/04/2023 14:26:18+0200, Alexandre Belloni wrote:
> On 04/04/2023 10:35:53+0000, Tilki, Ibrahim wrote:
> > >>>>> +  aux-voltage-chargeable:
> > >>>>> +    enum: [0, 1, 2]
> > >>>>> +    description: |
> > >>>>> +      Enables trickle charger.
> > >>>>> +      0: Charger is disabled (default)
> > >>>>> +      1: Charger is enabled
> > >>>>> +      2: Charger is enabled with a diode
> > >>>>
> > >>>> 2 is not an allowed value. I asked to drop this property. It is coming
> > >>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> > >>>> is charging with, I don't know, FET?
> > >>>
> > >>> No, what is not explained here (and maybe not unsterstood by the
> > >>> submitter) is that the RTC has an extra diode so, charging will always
> > >>> enable a diode, select a resistor and then have or not an extra diode.
> > >>> Figure2 of the MAX31329 datasheet is great.
> > >>>
> > >> 
> > >> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> > >> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> > >> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> > >> diode at this point.
> > >
> > > Ask the person who asked you to remove adi,trickle-diode-enable...
> > 
> > That was the purpose.
> > 
> 
> If the earlier submission was clearer my answer would have been
> different but note how I had to dig up the datasheet to understand there
> were two diodes. All the trickle chargers have a schottky diode so
> "adi,trickle-diode-enable" nor the commit log were explicit about the
> second diode (which is a regular diode).
> 
> aux-voltage-chargeable is enabling a diode on all the existing RTC
> drivers so instead of trying to make me look like the bad guy you should
> rather thank for taking the time trying to get better DT bindings.
> 

BTW, Krzysztof, you should focus more on how v5 of the driver is still
abusing the #clock-cells property to do pinmuxing after I repeatedly
explain to not do that.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 04/04/2023 14:29, Alexandre Belloni wrote:
> On 04/04/2023 14:26:18+0200, Alexandre Belloni wrote:
>> On 04/04/2023 10:35:53+0000, Tilki, Ibrahim wrote:
>>>>>>>> +  aux-voltage-chargeable:
>>>>>>>> +    enum: [0, 1, 2]
>>>>>>>> +    description: |
>>>>>>>> +      Enables trickle charger.
>>>>>>>> +      0: Charger is disabled (default)
>>>>>>>> +      1: Charger is enabled
>>>>>>>> +      2: Charger is enabled with a diode
>>>>>>>
>>>>>>> 2 is not an allowed value. I asked to drop this property. It is coming
>>>>>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>>>>>>> is charging with, I don't know, FET?
>>>>>>
>>>>>> No, what is not explained here (and maybe not unsterstood by the
>>>>>> submitter) is that the RTC has an extra diode so, charging will always
>>>>>> enable a diode, select a resistor and then have or not an extra diode.
>>>>>> Figure2 of the MAX31329 datasheet is great.
>>>>>>
>>>>>
>>>>> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
>>>>> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
>>>>> to "aux-voltage-chargeable", I am not sure how to add support for the extra
>>>>> diode at this point.
>>>>
>>>> Ask the person who asked you to remove adi,trickle-diode-enable...
>>>
>>> That was the purpose.
>>>
>>
>> If the earlier submission was clearer my answer would have been
>> different but note how I had to dig up the datasheet to understand there
>> were two diodes. All the trickle chargers have a schottky diode so
>> "adi,trickle-diode-enable" nor the commit log were explicit about the
>> second diode (which is a regular diode).
>>
>> aux-voltage-chargeable is enabling a diode on all the existing RTC
>> drivers so instead of trying to make me look like the bad guy you should
>> rather thank for taking the time trying to get better DT bindings.
>>
> 
> BTW, Krzysztof, you should focus more on how v5 of the driver is still
> abusing the #clock-cells property to do pinmuxing after I repeatedly
> explain to not do that.

Uh, I assumed #clock-cells in the binding is for its purpose - clocks. I
am rarely verifying driver implementation.

Best regards,
Krzysztof

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 03/04/2023 17:43, Ibrahim Tilki wrote:
> Devicetree binding documentation for Analog Devices MAX313XX RTCs
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> new file mode 100644
> index 000000000..0c17a395e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX313XX series I2C RTCs
> +
> +maintainers:
> +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> +
> +description: Analog Devices MAX313XX series I2C RTCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31328
> +      - adi,max31329
> +      - adi,max31331
> +      - adi,max31334
> +      - adi,max31341
> +      - adi,max31342
> +      - adi,max31343
> +
> +  reg:
> +    description: I2C address of the RTC
> +    items:
> +      - enum: [0x68, 0x69]
> +
> +  interrupts:
> +    description: |
> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> +      configuration.
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    description: |
> +      RTC can be used as a clock source through its clock output pin when
> +      supplied.

This part is correct, but your implementation is not. I don't think you
can disable or enable interrupts, based on usage of clock. Either this
is clock (gated or not) or interrupt, not both.


Best regards,
Krzysztof

Re: [PATCH v5 1/2] drivers: rtc: add max313xx series rtc driver

[Krzysztof Kozlowski]

On 03/04/2023 17:43, Ibrahim Tilki wrote:
> Adding support for Analog Devices MAX313XX series RTCs.
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>

(...)

> +
> +static int max313xx_clkout_register(struct device *dev)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_property_present(dev, "#clock-cells"))

Why would it be not present?

Best regards,
Krzysztof

RE: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Tilki, Ibrahim]

> >>>>>> +  aux-voltage-chargeable:
> >>>>>> +    enum: [0, 1, 2]
> >>>>>> +    description: |
> >>>>>> +      Enables trickle charger.
> >>>>>> +      0: Charger is disabled (default)
> >>>>>> +      1: Charger is enabled
> >>>>>> +      2: Charger is enabled with a diode
> >>>>>
> >>>>> 2 is not an allowed value. I asked to drop this property. It is coming
> >>>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> >>>>> is charging with, I don't know, FET?
> >>>>
> >>>> No, what is not explained here (and maybe not unsterstood by the
> >>>> submitter) is that the RTC has an extra diode so, charging will always
> >>>> enable a diode, select a resistor and then have or not an extra diode.
> >>>> Figure2 of the MAX31329 datasheet is great.
> >>>>
> >>> 
> >>> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> >>> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> >>> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> >>> diode at this point.
> >>
> >> Ask the person who asked you to remove adi,trickle-diode-enable...
> > 
> > That was the purpose.
> > 
> 
> If the earlier submission was clearer my answer would have been
> different but note how I had to dig up the datasheet to understand there
> were two diodes. All the trickle chargers have a schottky diode so
> "adi,trickle-diode-enable" nor the commit log were explicit about the
> second diode (which is a regular diode).
> 
> aux-voltage-chargeable is enabling a diode on all the existing RTC
> drivers so instead of trying to make me look like the bad guy you should
> rather thank for taking the time trying to get better DT bindings.

With all due respect, I am not trying to make you look like the bad guy.
It wouldn't help me in any way. It is just that this is a non-verbal
communication channel, if anything. I didn't think "That was the purpose"
would sound rude here and my apoligies if it does.

I am just trying to understand your expectations and this was my interpretation
of your comment on v4. So, "adi,trickle-diode-enable" it is or should I change
the name? The datasheet doesn't say anything but "diode".

Best regards,
Ibrahim

RE: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Tilki, Ibrahim]

> > +  interrupts:
> > +    description: |
> > +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> > +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> > +      configuration.
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    description: |
> > +      RTC can be used as a clock source through its clock output pin when
> > +      supplied.
> 
> This part is correct, but your implementation is not. I don't think you
> can disable or enable interrupts, based on usage of clock. Either this
> is clock (gated or not) or interrupt, not both.
> 

The driver doesn't enable or disable interrupts based on clock usage. It checks
whether the IRQ is possible or not. Enablement of interrupt depends on the 
"interrupts" property. The tricky part is that interrupt muxing depends on
clock configuration.

Let me briefly explain the behavior of RTCs and the driver[v4]

MAX31328:
  - Has a single pin which is either used as IRQ or clock output.
  - Driver aborts probe with "-EOPNOTSUPP" when user requests irq and clockout
    at the same time. In other words, when both "interrupts" and "#clock-cells"
    properties are present. Otherwise, we are fine.

MAX31331:
MAX31334:
  - Has two pins: INTA and INTB/CLOCKOUT. INTA pin is dedicated for interrupt.
    INTB pin is used as either interrupt or clockout. The Alarm1 interrupt is
    muxed into INTB by default. If the CLOCKOUT is enabled, Alarm1 irq is muxed
    into INTA. We don't have further control over interrupt muxing.
  - Driver checks for "#clock-cells". If it is present, it enables the clockout
    so that we can get interrupt from INTA.

The Rest:
  - Has two pins: INTA/CLKIN and INTB/CLOCKOUT. Alarm1 interrupt is muxed into
    INTA by default, muxed into INTB if and only if we enable CLKIN.
  - Driver aborts probe with -EOPNOTSUPP when user requests interrupt, clockin
    and clockout at the same time. We can't have all three with two pins.
	

Unfortunately we don't have control over the interrupt muxing other than clock
configuration. How should the driver approach this?

Best regards,
Ibrahim

Re: [PATCH v5 2/2] dt-bindings: rtc: add max313xx RTCs

[Krzysztof Kozlowski]

On 04/04/2023 17:40, Tilki, Ibrahim wrote:
>>> +  interrupts:
>>> +    description: |
>>> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
>>> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
>>> +      configuration.
>>> +    maxItems: 1
>>> +
>>> +  "#clock-cells":
>>> +    description: |
>>> +      RTC can be used as a clock source through its clock output pin when
>>> +      supplied.
>>
>> This part is correct, but your implementation is not. I don't think you
>> can disable or enable interrupts, based on usage of clock. Either this
>> is clock (gated or not) or interrupt, not both.
>>
> 
> The driver doesn't enable or disable interrupts based on clock usage. It checks
> whether the IRQ is possible or not. Enablement of interrupt depends on the 
> "interrupts" property. The tricky part is that interrupt muxing depends on
> clock configuration.

I think it is not what your driver is doing. It checks if clock-cells...

> 
> Let me briefly explain the behavior of RTCs and the driver[v4]
> 
> MAX31328:
>   - Has a single pin which is either used as IRQ or clock output.
>   - Driver aborts probe with "-EOPNOTSUPP" when user requests irq and clockout
>     at the same time. In other words, when both "interrupts" and "#clock-cells"
>     properties are present. Otherwise, we are fine.

Why? These are fixed properties of the device. It is always a clock
controller and always has interrupt line. The choice between them is
depending on pins, so pin control.

> 
> MAX31331:
> MAX31334:
>   - Has two pins: INTA and INTB/CLOCKOUT. INTA pin is dedicated for interrupt.
>     INTB pin is used as either interrupt or clockout. The Alarm1 interrupt is
>     muxed into INTB by default. If the CLOCKOUT is enabled, Alarm1 irq is muxed
>     into INTA. We don't have further control over interrupt muxing.
>   - Driver checks for "#clock-cells". If it is present, it enables the clockout
>     so that we can get interrupt from INTA.
> 
> The Rest:
>   - Has two pins: INTA/CLKIN and INTB/CLOCKOUT. Alarm1 interrupt is muxed into
>     INTA by default, muxed into INTB if and only if we enable CLKIN.
>   - Driver aborts probe with -EOPNOTSUPP when user requests interrupt, clockin
>     and clockout at the same time. We can't have all three with two pins.
> 	
> 
> Unfortunately we don't have control over the interrupt muxing other than clock
> configuration. How should the driver approach this?

Pinmux with two states - interrupt or clock.

Best regards,
Krzysztof

Re: [PATCH v5 1/2] drivers: rtc: add max313xx series rtc driver

[Chris Packham]

Hi Ibrahim,

On 4/04/23 03:43, Ibrahim Tilki wrote:
> Adding support for Analog Devices MAX313XX series RTCs.
>
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>

Just taking this version for a spin on my board with the MAX31331. 
Compared to patchset 3 I'm finding that the RTC isn't actually running 
after it is set. I haven't begun to debug why this might be. I suspect 
it's not being enabled.

Here's the snippet of dts that I'm using (which worked with patchset 3).

         i2c@1 {
                         rtc@68 {
                                 compatible = "adi,max31331";
                                 reg = <0x68>;
                         };
         };

And here's an example of it not working

root@x240-26GHXm ~]# date
Mon Apr 24 10:49:54 UTC 2023
[root@x240-26GHXm ~]# hwclock -w
[root@x240-26GHXm ~]# hwclock -r
Sat Jan  1 00:00:00 2000  0.000000 seconds

And finally a register dump which I'll start looking at shortly

[root@x240-26GHXm ~]# i2cdump -f -y 2 0x68
No size specified (using byte-data access)
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
00: 40 00 01 43 03 04 04 00 00 00 00 01 01 01 00 00 @.?C???....???..
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ...............?
20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
40: 00 00 00 00 00 00 00 00 00 ff ff ff ff ff ff ff ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
60: 00 00 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................

> ---
>   drivers/rtc/Kconfig        |   11 +
>   drivers/rtc/Makefile       |    1 +
>   drivers/rtc/rtc-max313xx.c | 1053 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 1065 insertions(+)
>   create mode 100644 drivers/rtc/rtc-max313xx.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 753872408..2160619ed 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -323,6 +323,17 @@ config RTC_DRV_LP8788
>   	help
>   	  Say Y to enable support for the LP8788 RTC/ALARM driver.
>   
> +config RTC_DRV_MAX313XX
> +	tristate "Analog Devices MAX313XX RTC driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you will get support for the
> +	  Analog Devices MAX313XX series RTC family.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-max313xx.
> +
>   config RTC_DRV_MAX6900
>   	tristate "Maxim MAX6900"
>   	help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index ea445d1eb..880db99be 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_RTC_DRV_M41T94)	+= rtc-m41t94.o
>   obj-$(CONFIG_RTC_DRV_M48T35)	+= rtc-m48t35.o
>   obj-$(CONFIG_RTC_DRV_M48T59)	+= rtc-m48t59.o
>   obj-$(CONFIG_RTC_DRV_M48T86)	+= rtc-m48t86.o
> +obj-$(CONFIG_RTC_DRV_MAX313XX)	+= rtc-max313xx.o
>   obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
>   obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
>   obj-$(CONFIG_RTC_DRV_MAX6916)	+= rtc-max6916.o
> diff --git a/drivers/rtc/rtc-max313xx.c b/drivers/rtc/rtc-max313xx.c
> new file mode 100644
> index 000000000..043528d7d
> --- /dev/null
> +++ b/drivers/rtc/rtc-max313xx.c
> @@ -0,0 +1,1053 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices MAX313XX series I2C RTC driver
> + *
> + * Copyright 2023 Analog Devices Inc.
> + */
> +#include <asm-generic/unaligned.h>
> +#include <linux/bcd.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +#include <linux/util_macros.h>
> +
> +/* common registers */
> +#define MAX313XX_INT_ALARM1		BIT(0)
> +#define MAX313XX_HRS_F_AM_PM		BIT(5)
> +#define MAX313XX_HRS_F_12_24		BIT(6)
> +#define MAX313XX_MONTH_CENTURY		BIT(7)
> +
> +#define MAX313XX_TIME_SIZE		0x07
> +
> +/* device specific registers */
> +#define MAX3134X_CFG2_REG		0x01
> +#define MAX3134X_CFG2_SET_RTC		BIT(1)
> +
> +#define MAX31341_TRICKLE_RES_MASK	GENMASK(1, 0)
> +#define MAX31341_TRICKLE_DIODE_EN	BIT(2)
> +#define MAX31341_TRICKLE_ENABLE_BIT	BIT(3)
> +#define MAX31341_POWER_MGMT_REG		0x56
> +#define MAX31341_POWER_MGMT_TRICKLE_BIT	BIT(0)
> +
> +#define MAX3133X_TRICKLE_RES_MASK	GENMASK(2, 1)
> +#define MAX3133X_TRICKLE_DIODE_EN	BIT(3)
> +#define MAX3133X_TRICKLE_ENABLE_BIT	BIT(0)
> +
> +#define MAX31329_TRICKLE_ENABLE_BIT	BIT(7)
> +#define MAX31343_TRICKLE_ENABLE_MASK	GENMASK(7, 4)
> +#define MAX31343_TRICKLE_ENABLE_CODE	5
> +#define MAX31329_43_TRICKLE_RES_MASK	GENMASK(1, 0)
> +#define MAX31329_43_TRICKLE_DIODE_EN	BIT(2)
> +
> +#define MAX31329_CONFIG2_REG		0x04
> +#define MAX31329_CONFIG2_CLKIN_EN	BIT(2)
> +#define MAX31329_CONFIG2_CLKIN_FREQ	GENMASK(1, 0)
> +
> +#define MAX31341_42_CONFIG1_REG		0x00
> +#define MAX31341_42_CONFIG1_CLKIN_EN	BIT(7)
> +#define MAX31341_42_CONFIG1_CLKIN_FREQ	GENMASK(5, 4)
> +#define MAX31341_42_CONFIG1_OSC_DISABLE	BIT(3)
> +#define MAX31341_42_CONFIG1_SWRST	BIT(0)
> +
> +enum max313xx_ids {
> +	ID_MAX31328,
> +	ID_MAX31329,
> +	ID_MAX31331,
> +	ID_MAX31334,
> +	ID_MAX31341,
> +	ID_MAX31342,
> +	ID_MAX31343,
> +	MAX313XX_ID_NR
> +};
> +
> +struct clkout_cfg {
> +	const int *freq_avail;
> +	u8 freq_size;
> +	u8 freq_pos;
> +	u8 reg;
> +	u8 en_bit;
> +	bool en_invert;
> +};
> +
> +struct chip_desc {
> +	struct clkout_cfg *clkout;
> +	const char *clkout_name;
> +	u8 sec_reg;
> +	u8 alarm1_sec_reg;
> +
> +	u8 int_en_reg;
> +	u8 int_status_reg;
> +
> +	u8 ram_reg;
> +	u8 ram_size;
> +
> +	u8 temp_reg;
> +
> +	u8 trickle_reg;
> +};
> +
> +#define clk_hw_to_max313xx(_hw) container_of(_hw, struct max313xx, clkout)
> +
> +struct max313xx {
> +	enum max313xx_ids id;
> +	struct regmap *regmap;
> +	struct rtc_device *rtc;
> +	struct clk_hw clkout;
> +	struct clk *clkin;
> +	const struct chip_desc *chip;
> +	int irq;
> +};
> +
> +static const int max313xx_clkin_freq[] = { 1, 50, 60, 32768 };
> +
> +static const int max31328_clkout_freq[] = { 1, 1024, 4096, 8192 };
> +static const int max31329_clkout_freq[] = { 1, 4096, 8192, 32768 };
> +static const int max3133x_clkout_freq[] = { 1, 64, 1024, 32768 };
> +static const int max31341_42_clkout_freq[] = { 1, 50, 60, 32768 };
> +static const int max31343_clkout_freq[] = { 1, 2, 4, 8, 16, 32, 64, 128, 32875 };
> +
> +static struct clkout_cfg max31328_clkout = {
> +	.freq_avail = max31328_clkout_freq,
> +	.freq_size = ARRAY_SIZE(max31328_clkout_freq),
> +	.freq_pos = 3,
> +	.reg = 0x0E,
> +	.en_bit = BIT(3),
> +	.en_invert = true,
> +};
> +
> +static struct clkout_cfg max31329_clkout = {
> +	.freq_avail = max31329_clkout_freq,
> +	.freq_size = ARRAY_SIZE(max31329_clkout_freq),
> +	.freq_pos = 5,
> +	.reg = 0x04,
> +	.en_bit = BIT(7),
> +};
> +
> +static struct clkout_cfg max3133x_clkout = {
> +	.freq_avail = max3133x_clkout_freq,
> +	.freq_size = ARRAY_SIZE(max3133x_clkout_freq),
> +	.freq_pos = 0,
> +	.reg = 0x04,
> +	.en_bit = BIT(2),
> +};
> +
> +static struct clkout_cfg max31341_42_clkout = {
> +	.freq_avail = max31341_42_clkout_freq,
> +	.freq_size = ARRAY_SIZE(max31341_42_clkout_freq),
> +	.freq_pos = 1,
> +	.reg = 0x00,
> +	.en_bit = BIT(6),
> +	.en_invert = true,
> +};
> +
> +static struct clkout_cfg max31343_clkout = {
> +	.freq_avail = max31343_clkout_freq,
> +	.freq_size = ARRAY_SIZE(max31343_clkout_freq),
> +	.freq_pos = 3,
> +	.reg = 0x04,
> +	.en_bit = BIT(7),
> +};
> +
> +static const struct chip_desc chip[MAX313XX_ID_NR] = {
> +	[ID_MAX31328] = {
> +		.int_en_reg = 0x0E,
> +		.int_status_reg = 0x0F,
> +		.sec_reg = 0x00,
> +		.alarm1_sec_reg = 0x07,
> +		.temp_reg = 0x11,
> +		.clkout = &max31328_clkout,
> +		.clkout_name = "max31328-sqw",
> +	},
> +	[ID_MAX31329] = {
> +		.int_en_reg = 0x01,
> +		.int_status_reg = 0x00,
> +		.sec_reg = 0x06,
> +		.alarm1_sec_reg = 0x0D,
> +		.ram_reg = 0x22,
> +		.ram_size = 64,
> +		.trickle_reg = 0x19,
> +		.clkout = &max31329_clkout,
> +		.clkout_name = "max31329-clkout",
> +	},
> +	[ID_MAX31331] = {
> +		.int_en_reg = 0x01,
> +		.int_status_reg = 0x00,
> +		.sec_reg = 0x08,
> +		.alarm1_sec_reg = 0x0F,
> +		.ram_reg = 0x20,
> +		.ram_size = 32,
> +		.trickle_reg = 0x1B,
> +		.clkout = &max3133x_clkout,
> +		.clkout_name = "max31331-clkout",
> +	},
> +	[ID_MAX31334] = {
> +		.int_en_reg = 0x01,
> +		.int_status_reg = 0x00,
> +		.sec_reg = 0x09,
> +		.alarm1_sec_reg = 0x10,
> +		.ram_reg = 0x30,
> +		.ram_size = 32,
> +		.trickle_reg = 0x1E,
> +		.clkout = &max3133x_clkout,
> +		.clkout_name = "max31334-clkout",
> +	},
> +	[ID_MAX31341] = {
> +		.int_en_reg = 0x04,
> +		.int_status_reg = 0x05,
> +		.sec_reg = 0x06,
> +		.alarm1_sec_reg = 0x0D,
> +		.ram_reg = 0x16,
> +		.ram_size = 64,
> +		.trickle_reg = 0x57,
> +		.clkout = &max31341_42_clkout,
> +		.clkout_name = "max31341-clkout",
> +	},
> +	[ID_MAX31342] = {
> +		.int_en_reg = 0x04,
> +		.int_status_reg = 0x05,
> +		.sec_reg = 0x06,
> +		.alarm1_sec_reg = 0x0D,
> +		.clkout = &max31341_42_clkout,
> +		.clkout_name = "max31342-clkout",
> +	},
> +	[ID_MAX31343] = {
> +		.int_en_reg = 0x01,
> +		.int_status_reg = 0x00,
> +		.sec_reg = 0x06,
> +		.alarm1_sec_reg = 0x0D,
> +		.ram_reg = 0x22,
> +		.ram_size = 64,
> +		.temp_reg = 0x1A,
> +		.trickle_reg = 0x19,
> +		.clkout = &max31343_clkout,
> +		.clkout_name = "max31343-clko",
> +	},
> +};
> +
> +static const u32 max313xx_trickle_ohms[] = { 3000, 6000, 11000 };
> +
> +static bool max313xx_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	const struct chip_desc *chip = rtc->chip;
> +
> +	/* time keeping registers */
> +	if (reg >= chip->sec_reg && reg < chip->sec_reg + MAX313XX_TIME_SIZE)
> +		return true;
> +
> +	/* interrupt status register */
> +	if (reg == chip->int_status_reg)
> +		return true;
> +
> +	/* temperature registers */
> +	return chip->temp_reg &&
> +		(reg == chip->temp_reg || reg == chip->temp_reg + 1);
> +}
> +
> +static const struct regmap_config regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = max313xx_volatile_reg,
> +};
> +
> +static int max313xx_get_hour(u8 hour_reg)
> +{
> +	int hour;
> +
> +	/* 24Hr mode */
> +	if (!FIELD_GET(MAX313XX_HRS_F_12_24, hour_reg))
> +		return bcd2bin(hour_reg & 0x3f);
> +
> +	/* 12Hr mode */
> +	hour = bcd2bin(hour_reg & 0x1f);
> +	if (hour == 12)
> +		hour = 0;
> +
> +	if (FIELD_GET(MAX313XX_HRS_F_AM_PM, hour_reg))
> +		hour += 12;
> +
> +	return hour;
> +}
> +
> +static int max313xx_read_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	u8 regs[7];
> +	int ret;
> +
> +	ret = regmap_bulk_read(rtc->regmap, rtc->chip->sec_reg, regs, 7);
> +	if (ret)
> +		return ret;
> +
> +	t->tm_sec = bcd2bin(regs[0] & 0x7f);
> +	t->tm_min = bcd2bin(regs[1] & 0x7f);
> +	t->tm_hour = max313xx_get_hour(regs[2]);
> +	t->tm_wday = bcd2bin(regs[3] & 0x07) - 1;
> +	t->tm_mday = bcd2bin(regs[4] & 0x3f);
> +	t->tm_mon = bcd2bin(regs[5] & 0x1f) - 1;
> +	t->tm_year = bcd2bin(regs[6]) + 100;
> +
> +	if (FIELD_GET(MAX313XX_MONTH_CENTURY, regs[5]))
> +		t->tm_year += 100;
> +
> +	return 0;
> +}
> +
> +static int max313xx_pre_set_time(struct max313xx *rtc)
> +{
> +	if (!rtc->clkin)
> +		return 0;
> +
> +	/* Clkin needs to be disabled before setting time. */
> +	switch (rtc->id) {
> +	case ID_MAX31341:
> +	case ID_MAX31342:
> +		return regmap_clear_bits(rtc->regmap,
> +					 MAX31341_42_CONFIG1_REG,
> +					 MAX31341_42_CONFIG1_CLKIN_EN);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int max313xx_post_set_time(struct max313xx *rtc)
> +{
> +	int ret;
> +
> +	switch (rtc->id) {
> +	case ID_MAX31341:
> +	case ID_MAX31342:
> +		ret = regmap_set_bits(rtc->regmap, MAX3134X_CFG2_REG,
> +				      MAX3134X_CFG2_SET_RTC);
> +		if (ret)
> +			return ret;
> +
> +		fsleep(10000);
> +
> +		ret = regmap_clear_bits(rtc->regmap, MAX3134X_CFG2_REG,
> +					MAX3134X_CFG2_SET_RTC);
> +		if (ret)
> +			return ret;
> +
> +		if (rtc->clkin)
> +			ret = regmap_set_bits(rtc->regmap,
> +					      MAX31341_42_CONFIG1_REG,
> +					      MAX31341_42_CONFIG1_CLKIN_EN);
> +
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static int max313xx_set_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	u8 regs[7];
> +	int ret;
> +
> +	regs[0] = bin2bcd(t->tm_sec);
> +	regs[1] = bin2bcd(t->tm_min);
> +	regs[2] = bin2bcd(t->tm_hour);
> +	regs[3] = bin2bcd(t->tm_wday + 1);
> +	regs[4] = bin2bcd(t->tm_mday);
> +	regs[5] = bin2bcd(t->tm_mon + 1);
> +	regs[6] = bin2bcd(t->tm_year % 100);
> +
> +	if (t->tm_year >= 200)
> +		regs[5] |= FIELD_PREP(MAX313XX_MONTH_CENTURY, 1);
> +
> +	ret = max313xx_pre_set_time(rtc);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_write(rtc->regmap, rtc->chip->sec_reg, regs, 7);
> +	if (ret)
> +		return ret;
> +
> +	return max313xx_post_set_time(rtc);
> +}
> +
> +static int max313xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	unsigned int status, int_en;
> +	struct rtc_time time;
> +	u8 regs[6];
> +	int ret;
> +
> +	ret = regmap_bulk_read(rtc->regmap, rtc->chip->alarm1_sec_reg, regs,
> +			       sizeof(regs));
> +	if (ret)
> +		return ret;
> +
> +	t->time.tm_sec = bcd2bin(regs[0] & 0x7f);
> +	t->time.tm_min = bcd2bin(regs[1] & 0x7f);
> +	t->time.tm_hour = bcd2bin(regs[2] & 0x3f);
> +	t->time.tm_mday = bcd2bin(regs[3] & 0x3f);
> +	t->time.tm_mon = bcd2bin(regs[4] & 0x1f) - 1;
> +	t->time.tm_year = bcd2bin(regs[5]) + 100;
> +
> +	ret = max313xx_read_time(dev, &time);
> +	if (ret)
> +		return ret;
> +
> +	if (time.tm_year >= 200)
> +		t->time.tm_year += 100;
> +
> +	ret = regmap_read(rtc->regmap, rtc->chip->int_status_reg, &status);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(rtc->regmap, rtc->chip->int_en_reg, &int_en);
> +	if (ret)
> +		return ret;
> +
> +	t->enabled = FIELD_GET(MAX313XX_INT_ALARM1, int_en);
> +	t->pending = FIELD_GET(MAX313XX_INT_ALARM1, status);
> +
> +	return 0;
> +}
> +
> +static int max313xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	unsigned int reg;
> +	u8 regs[6];
> +	int ret;
> +
> +	regs[0] = bin2bcd(t->time.tm_sec);
> +	regs[1] = bin2bcd(t->time.tm_min);
> +	regs[2] = bin2bcd(t->time.tm_hour);
> +	regs[3] = bin2bcd(t->time.tm_mday);
> +	regs[4] = bin2bcd(t->time.tm_mon + 1);
> +	regs[5] = bin2bcd(t->time.tm_year % 100);
> +
> +	ret = regmap_bulk_write(rtc->regmap, rtc->chip->alarm1_sec_reg, regs,
> +				sizeof(regs));
> +	if (ret)
> +		return ret;
> +
> +	reg = FIELD_PREP(MAX313XX_INT_ALARM1, t->enabled);
> +	ret = regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
> +				 MAX313XX_INT_ALARM1, reg);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear status register */
> +	return regmap_read(rtc->regmap, rtc->chip->int_status_reg, &reg);
> +}
> +
> +static int max313xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +
> +	return regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
> +				  MAX313XX_INT_ALARM1,
> +				  FIELD_PREP(MAX313XX_INT_ALARM1, enabled));
> +}
> +
> +static const struct rtc_class_ops max3133x_rtc_ops = {
> +	.read_time	= max313xx_read_time,
> +	.set_time	= max313xx_set_time,
> +	.read_alarm	= max313xx_read_alarm,
> +	.set_alarm	= max313xx_set_alarm,
> +	.alarm_irq_enable = max313xx_alarm_irq_enable,
> +};
> +
> +static irqreturn_t max313xx_irq(int irq, void *dev_id)
> +{
> +	struct max313xx	*rtc = dev_id;
> +	struct mutex *lock = &rtc->rtc->ops_lock;
> +	int stat, ret;
> +
> +	mutex_lock(lock);
> +	ret = regmap_read(rtc->regmap, rtc->chip->int_status_reg, &stat);
> +	if (ret)
> +		goto out;
> +
> +	if (FIELD_GET(MAX313XX_INT_ALARM1, stat)) {
> +		ret = regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
> +					 MAX313XX_INT_ALARM1, 0);
> +		if (ret)
> +			goto out;
> +
> +		rtc_update_irq(rtc->rtc, 1, RTC_AF | RTC_IRQF);
> +	}
> +
> +out:
> +	mutex_unlock(lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int max313xx_nvmem_reg_read(void *priv, unsigned int offset,
> +				   void *val, size_t bytes)
> +{
> +	struct max313xx *rtc = priv;
> +	unsigned int reg = rtc->chip->ram_reg + offset;
> +
> +	return regmap_bulk_read(rtc->regmap, reg, val, bytes);
> +}
> +
> +static int max313xx_nvmem_reg_write(void *priv, unsigned int offset,
> +				    void *val, size_t bytes)
> +{
> +	struct max313xx *rtc = priv;
> +	unsigned int reg = rtc->chip->ram_reg + offset;
> +
> +	return regmap_bulk_write(rtc->regmap, reg, val, bytes);
> +}
> +
> +struct nvmem_config max313xx_nvmem_cfg = {
> +	.reg_read = max313xx_nvmem_reg_read,
> +	.reg_write = max313xx_nvmem_reg_write,
> +	.word_size = 8,
> +};
> +
> +static unsigned long max313xx_clkout_recalc_rate(struct clk_hw *hw,
> +						 unsigned long parent_rate)
> +{
> +	struct max313xx *rtc = clk_hw_to_max313xx(hw);
> +	const struct clkout_cfg *clkout = rtc->chip->clkout;
> +	unsigned int freq_mask;
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(rtc->regmap, clkout->reg, &reg);
> +	if (ret)
> +		return 0;
> +
> +	freq_mask = __roundup_pow_of_two(clkout->freq_size) - 1;
> +
> +	return clkout->freq_avail[(reg >> clkout->freq_pos) & freq_mask];
> +}
> +
> +static long max313xx_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
> +				       unsigned long *prate)
> +{
> +	struct max313xx *rtc = clk_hw_to_max313xx(hw);
> +	struct clkout_cfg *clkout = rtc->chip->clkout;
> +	int index;
> +
> +	index = find_closest(rate, clkout->freq_avail, clkout->freq_size);
> +	return clkout->freq_avail[index];
> +}
> +
> +static int max313xx_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> +				    unsigned long parent_rate)
> +{
> +	struct max313xx *rtc = clk_hw_to_max313xx(hw);
> +	struct clkout_cfg *clkout = rtc->chip->clkout;
> +	unsigned int freq_mask;
> +	int index;
> +
> +	index = find_closest(rate, clkout->freq_avail, clkout->freq_size);
> +	freq_mask = __roundup_pow_of_two(clkout->freq_size) - 1;
> +
> +	return regmap_update_bits(rtc->regmap, clkout->reg,
> +				  freq_mask << clkout->freq_pos,
> +				  index << clkout->freq_pos);
> +}
> +
> +static int max313xx_clkout_enable(struct clk_hw *hw)
> +{
> +	struct max313xx *rtc = clk_hw_to_max313xx(hw);
> +	struct clkout_cfg *clkout = rtc->chip->clkout;
> +
> +	if (clkout->en_invert)
> +		return regmap_clear_bits(rtc->regmap, clkout->reg,
> +					 clkout->en_bit);
> +
> +	return regmap_set_bits(rtc->regmap, clkout->reg,  clkout->en_bit);
> +}
> +
> +static void max313xx_clkout_disable(struct clk_hw *hw)
> +{
> +	struct max313xx *rtc = clk_hw_to_max313xx(hw);
> +	struct clkout_cfg *clkout = rtc->chip->clkout;
> +
> +	switch (rtc->id) {
> +	case ID_MAX31331:
> +	case ID_MAX31334:
> +		if (rtc->irq > 0) {
> +			dev_err(rtc->rtc->dev.parent,
> +				"clkout cannot be disabled when IRQ is requested");
> +			return;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (clkout->en_invert)
> +		regmap_set_bits(rtc->regmap, clkout->reg, clkout->en_bit);
> +	else
> +		regmap_clear_bits(rtc->regmap, clkout->reg,  clkout->en_bit);
> +}
> +
> +static int max313xx_clkout_is_enabled(struct clk_hw *hw)
> +{
> +	struct max313xx *rtc = clk_hw_to_max313xx(hw);
> +	struct clkout_cfg *clkout = rtc->chip->clkout;
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(rtc->regmap, clkout->reg, &reg);
> +	if (ret)
> +		return ret;
> +
> +	return !!(reg & clkout->en_bit) ^ clkout->en_invert;
> +}
> +
> +static const struct clk_ops max313xx_clkout_ops = {
> +	.recalc_rate = max313xx_clkout_recalc_rate,
> +	.round_rate = max313xx_clkout_round_rate,
> +	.set_rate = max313xx_clkout_set_rate,
> +	.enable = max313xx_clkout_enable,
> +	.disable = max313xx_clkout_disable,
> +	.is_enabled = max313xx_clkout_is_enabled,
> +};
> +
> +struct clk_init_data max313xx_clk_init = {
> +	.name = "max313xx-clkout",
> +	.ops = &max313xx_clkout_ops,
> +};
> +
> +static int max313xx_read_temp(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *val)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	u8 reg[2];
> +	s16 temp;
> +	int ret;
> +
> +	if (type != hwmon_temp || attr != hwmon_temp_input)
> +		return -EOPNOTSUPP;
> +
> +	ret = regmap_bulk_read(rtc->regmap, rtc->chip->temp_reg, reg, 2);
> +	if (ret)
> +		return ret;
> +
> +	temp = get_unaligned_be16(reg);
> +
> +	*val = (temp / 64) * 250;
> +
> +	return 0;
> +}
> +
> +static umode_t max313xx_is_visible(const void *data,
> +				   enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	if (type == hwmon_temp && attr == hwmon_temp_input)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *max313xx_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops max313xx_hwmon_ops = {
> +	.is_visible = max313xx_is_visible,
> +	.read = max313xx_read_temp,
> +};
> +
> +static const struct hwmon_chip_info max313xx_chip_info = {
> +	.ops = &max313xx_hwmon_ops,
> +	.info = max313xx_info,
> +};
> +
> +static int max313xx_init(struct max313xx *rtc)
> +{
> +	int ret;
> +
> +	switch (rtc->id) {
> +	case ID_MAX31341:
> +	case ID_MAX31342:
> +		ret = regmap_clear_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
> +					MAX31341_42_CONFIG1_OSC_DISABLE);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_set_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
> +				       MAX31341_42_CONFIG1_SWRST);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int max313xx_clkout_register(struct device *dev)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_property_present(dev, "#clock-cells"))
> +		return 0;
> +
> +	max313xx_clk_init.name = rtc->chip->clkout_name;
> +	device_property_read_string(dev, "clock-output-names",
> +				    &max313xx_clk_init.name);
> +	rtc->clkout.init = &max313xx_clk_init;
> +
> +	ret = devm_clk_hw_register(dev, &rtc->clkout);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "cannot register clock\n");
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> +					   &rtc->clkout);
> +}
> +
> +static int max313xx_trickle_charger_setup(struct device *dev)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	bool trickle_diode_en;
> +	u32 charger_en = 0;
> +	int index, reg;
> +	u32 ohms = 0;
> +	int ret;
> +
> +	device_property_read_u32(dev, "aux-voltage-chargeable", &charger_en);
> +	switch (charger_en) {
> +	case 0:
> +		return 0;
> +	case 1:
> +		trickle_diode_en = 0;
> +		break;
> +	case 2:
> +		trickle_diode_en = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	device_property_read_u32(dev, "trickle-resistor-ohms", &ohms);
> +
> +	if (!rtc->chip->trickle_reg)
> +		return 0;
> +
> +	index = find_closest(ohms, max313xx_trickle_ohms,
> +			     ARRAY_SIZE(max313xx_trickle_ohms)) + 1;
> +
> +	switch (rtc->id) {
> +	case ID_MAX31329:
> +		reg = FIELD_PREP(MAX31329_TRICKLE_ENABLE_BIT, 1) |
> +		      FIELD_PREP(MAX31329_43_TRICKLE_RES_MASK, index) |
> +		      FIELD_PREP(MAX31329_43_TRICKLE_DIODE_EN, trickle_diode_en);
> +		break;
> +	case ID_MAX31331:
> +	case ID_MAX31334:
> +		reg = FIELD_PREP(MAX3133X_TRICKLE_ENABLE_BIT, 1) |
> +		      FIELD_PREP(MAX3133X_TRICKLE_RES_MASK, index) |
> +		      FIELD_PREP(MAX3133X_TRICKLE_DIODE_EN, trickle_diode_en);
> +		break;
> +	case ID_MAX31341:
> +		if (index == 1)
> +			index = 0;
> +
> +		reg = FIELD_PREP(MAX31341_TRICKLE_ENABLE_BIT, 1) |
> +		      FIELD_PREP(MAX31341_TRICKLE_RES_MASK, index) |
> +		      FIELD_PREP(MAX31341_TRICKLE_DIODE_EN, trickle_diode_en);
> +
> +		ret = regmap_set_bits(rtc->regmap, MAX31341_POWER_MGMT_REG,
> +				      MAX31341_POWER_MGMT_TRICKLE_BIT);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case ID_MAX31343:
> +		reg = FIELD_PREP(MAX31329_43_TRICKLE_RES_MASK, index) |
> +		      FIELD_PREP(MAX31329_43_TRICKLE_DIODE_EN, trickle_diode_en) |
> +		      FIELD_PREP(MAX31343_TRICKLE_ENABLE_MASK,
> +				 MAX31343_TRICKLE_ENABLE_CODE);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return regmap_write(rtc->regmap, rtc->chip->trickle_reg, reg);
> +}
> +
> +static int max313xx_find_clkin_freq_index(struct clk *clk)
> +{
> +	unsigned long rate = clk_get_rate(clk);
> +	int freq;
> +	int i;
> +
> +	i = find_closest(rate, max313xx_clkin_freq,
> +			 ARRAY_SIZE(max313xx_clkin_freq));
> +	if (max313xx_clkin_freq[i] == rate)
> +		return i;
> +
> +	for (i = ARRAY_SIZE(max313xx_clkin_freq) - 1; i >= 0; i--) {
> +		freq = max313xx_clkin_freq[i];
> +		if (freq == clk_round_rate(clk, freq))
> +			return i;
> +	}
> +
> +	/* supplied clock cannot produce one of desired frequency rate */
> +	return -ENODEV;
> +}
> +
> +static int max313xx_clkin_init(struct device *dev)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	int rate;
> +	int ret;
> +
> +	rtc->clkin = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(rtc->clkin)) {
> +		if (PTR_ERR(rtc->clkin) == -ENOENT)
> +			rtc->clkin = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(rtc->clkin),
> +					     "error while clkin setup\n");
> +	}
> +
> +	if (!rtc->clkin) {
> +		switch (rtc->id) {
> +		case ID_MAX31329:
> +			return regmap_clear_bits(rtc->regmap,
> +						 MAX31329_CONFIG2_REG,
> +						 MAX31329_CONFIG2_CLKIN_EN);
> +		case ID_MAX31341:
> +		case ID_MAX31342:
> +			return regmap_clear_bits(rtc->regmap,
> +						 MAX31341_42_CONFIG1_REG,
> +						 MAX31341_42_CONFIG1_CLKIN_EN);
> +		default:
> +			return 0;
> +		}
> +	}
> +
> +	rate = max313xx_find_clkin_freq_index(rtc->clkin);
> +	if (rate < 0)
> +		return dev_err_probe(dev, rate,
> +				     "clkin cannot produce required frequency\n");
> +
> +	ret = clk_set_rate(rtc->clkin, max313xx_clkin_freq[rate]);
> +	if (ret)
> +		return ret;
> +
> +	switch (rtc->id) {
> +	case ID_MAX31329:
> +		ret = regmap_update_bits(rtc->regmap, MAX31329_CONFIG2_REG,
> +					 MAX31329_CONFIG2_CLKIN_FREQ, rate);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_set_bits(rtc->regmap, MAX31329_CONFIG2_REG,
> +				       MAX31329_CONFIG2_CLKIN_EN);
> +	case ID_MAX31341:
> +	case ID_MAX31342:
> +		ret = regmap_update_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
> +					 MAX31341_42_CONFIG1_CLKIN_FREQ,
> +					 FIELD_PREP(MAX31341_42_CONFIG1_CLKIN_FREQ, rate));
> +		if (ret)
> +			return ret;
> +
> +		return regmap_set_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
> +				       MAX31341_42_CONFIG1_CLKIN_EN);
> +	default:
> +		rtc->clkin = NULL;
> +		dev_warn(dev, "device does not have clock input\n");
> +		return 0;
> +	}
> +}
> +
> +static int max313xx_irq_init(struct device *dev, const char *devname)
> +{
> +	struct max313xx *rtc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (rtc->id) {
> +	case ID_MAX31328:
> +		break;
> +	case ID_MAX31331:
> +	case ID_MAX31334:
> +		if (rtc->clkout.clk) {
> +			/*
> +			 * interrupt muxing depends on clkout so enable clkout
> +			 * if configured before requesting interrupt
> +			 */
> +			ret = clk_prepare_enable(rtc->clkout.clk);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "cannot enable clkout\n");
> +		}
> +		break;
> +	default:
> +		if (rtc->clkin) {
> +			if (rtc->clkout.clk && rtc->irq > 0)
> +				return dev_err_probe(dev, -EOPNOTSUPP,
> +						     "irq not possible when both clkin and clkout are configured\n");
> +
> +			if (rtc->irq <= 0)
> +				break;
> +
> +			/* clkout needs to be disabled for interrupt */
> +			if (rtc->chip->clkout->en_invert)
> +				ret = regmap_set_bits(rtc->regmap,
> +						      rtc->chip->clkout->reg,
> +						      rtc->chip->clkout->en_bit);
> +			else
> +				ret = regmap_clear_bits(rtc->regmap,
> +							rtc->chip->clkout->reg,
> +							rtc->chip->clkout->en_bit);
> +
> +			if (ret)
> +				return ret;
> +		}
> +		break;
> +	}
> +
> +	if (rtc->irq > 0) {
> +		return devm_request_threaded_irq(dev, rtc->irq, NULL,
> +						 &max313xx_irq, IRQF_ONESHOT,
> +						 devname, rtc);
> +	} else if (device_property_read_bool(dev, "wakeup-source")) {
> +		clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtc->features);
> +		return device_init_wakeup(dev, true);
> +	}
> +
> +	clear_bit(RTC_FEATURE_ALARM, rtc->rtc->features);
> +
> +	return 0;
> +}
> +
> +static int max313xx_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct max313xx *max313xx;
> +	struct device *hwmon;
> +	const void *match;
> +	int ret;
> +
> +	max313xx = devm_kzalloc(&client->dev, sizeof(*max313xx), GFP_KERNEL);
> +	if (!max313xx)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&client->dev, max313xx);
> +
> +	max313xx->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(max313xx->regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(max313xx->regmap),
> +				     "regmap init failed\n");
> +	}
> +
> +	i2c_set_clientdata(client, max313xx);
> +
> +	match = device_get_match_data(dev);
> +	if (match)
> +		max313xx->chip = match;
> +	else if (id)
> +		max313xx->chip = (struct chip_desc *)id->driver_data;
> +	else
> +		return -ENODEV;
> +
> +	max313xx->id = max313xx->chip - chip;
> +
> +	ret = max313xx_init(max313xx);
> +	if (ret)
> +		return ret;
> +
> +	max313xx->rtc = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(max313xx->rtc))
> +		return PTR_ERR(max313xx->rtc);
> +
> +	max313xx->rtc->ops = &max3133x_rtc_ops;
> +	max313xx->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	max313xx->rtc->range_max = RTC_TIMESTAMP_END_2199;
> +
> +	ret = devm_rtc_register_device(max313xx->rtc);
> +	if (ret)
> +		return ret;
> +
> +	max313xx->irq = client->irq;
> +
> +	ret = max313xx_clkout_register(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = max313xx_clkin_init(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* IRQ wiring depends on the clock configuration so parse them first */
> +	ret = max313xx_irq_init(dev, client->name);
> +	if (ret)
> +		return ret;
> +
> +	if (max313xx->chip->ram_size) {
> +		max313xx_nvmem_cfg.size = max313xx->chip->ram_size;
> +		max313xx_nvmem_cfg.priv = max313xx;
> +
> +		ret = devm_rtc_nvmem_register(max313xx->rtc, &max313xx_nvmem_cfg);
> +		if (ret)
> +			dev_warn(dev, "cannot register rtc nvmem\n");
> +	}
> +
> +	if (max313xx->chip->temp_reg) {
> +		hwmon = devm_hwmon_device_register_with_info(dev, client->name,
> +							     max313xx,
> +							     &max313xx_chip_info,
> +							     NULL);
> +		if (IS_ERR(hwmon))
> +			dev_warn(dev, "cannot register hwmon device: %li\n",
> +				 PTR_ERR(hwmon));
> +	}
> +
> +	return max313xx_trickle_charger_setup(dev);
> +}
> +
> +static const struct of_device_id max313xx_of_id[] = {
> +	{ .compatible = "adi,max31328", .data = &chip[ID_MAX31328] },
> +	{ .compatible = "adi,max31329", .data = &chip[ID_MAX31329] },
> +	{ .compatible = "adi,max31331", .data = &chip[ID_MAX31331] },
> +	{ .compatible = "adi,max31334", .data = &chip[ID_MAX31334] },
> +	{ .compatible = "adi,max31341", .data = &chip[ID_MAX31341] },
> +	{ .compatible = "adi,max31342", .data = &chip[ID_MAX31342] },
> +	{ .compatible = "adi,max31343", .data = &chip[ID_MAX31343] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max313xx_of_id);
> +
> +static const struct i2c_device_id max313xx_id[] = {
> +	{ "max31328", (kernel_ulong_t)&chip[ID_MAX31328] },
> +	{ "max31329", (kernel_ulong_t)&chip[ID_MAX31329] },
> +	{ "max31331", (kernel_ulong_t)&chip[ID_MAX31331] },
> +	{ "max31334", (kernel_ulong_t)&chip[ID_MAX31334] },
> +	{ "max31341", (kernel_ulong_t)&chip[ID_MAX31341] },
> +	{ "max31342", (kernel_ulong_t)&chip[ID_MAX31342] },
> +	{ "max31343", (kernel_ulong_t)&chip[ID_MAX31343] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max313xx_id);
> +
> +static struct i2c_driver max313xx_driver = {
> +	.driver = {
> +		.name	= "rtc-max313xx",
> +		.of_match_table = max313xx_of_id,
> +	},
> +	.probe		= max313xx_probe,
> +	.id_table	= max313xx_id,
> +};
> +module_i2c_driver(max313xx_driver);
> +
> +MODULE_DESCRIPTION("Analog Devices MAX313XX RTCs");
> +MODULE_AUTHOR("Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>");
> +MODULE_AUTHOR("Ibrahim Tilki <Ibrahim.Tilki@analog.com>");
> +MODULE_SOFTDEP("pre: regmap-i2c");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");

Re: [PATCH v5 1/2] drivers: rtc: add max313xx series rtc driver

[Chris Packham]

On 24/04/23 10:52, Chris Packham wrote:
> Hi Ibrahim,
>
> On 4/04/23 03:43, Ibrahim Tilki wrote:
>> Adding support for Analog Devices MAX313XX series RTCs.
>>
>> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
>> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
>
> Just taking this version for a spin on my board with the MAX31331. 
> Compared to patchset 3 I'm finding that the RTC isn't actually running 
> after it is set. I haven't begun to debug why this might be. I suspect 
> it's not being enabled.
>
> Here's the snippet of dts that I'm using (which worked with patchset 3).
>
>         i2c@1 {
>                         rtc@68 {
>                                 compatible = "adi,max31331";
>                                 reg = <0x68>;
>                         };
>         };
>
> And here's an example of it not working
>
> root@x240-26GHXm ~]# date
> Mon Apr 24 10:49:54 UTC 2023
> [root@x240-26GHXm ~]# hwclock -w
> [root@x240-26GHXm ~]# hwclock -r
> Sat Jan  1 00:00:00 2000  0.000000 seconds
>
> And finally a register dump which I'll start looking at shortly
>
> [root@x240-26GHXm ~]# i2cdump -f -y 2 0x68
> No size specified (using byte-data access)
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
> 00: 40 00 01 43 03 04 04 00 00 00 00 01 01 01 00 00 @.?C???....???..
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ...............?
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 40: 00 00 00 00 00 00 00 00 00 ff ff ff ff ff ff ff ................
> 50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 60: 00 00 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................

Doing a bit of debugging I can see that the OSF bit is set in the STATUS 
reg and that the SWRST bit is set in the RESET register. This is likely 
because builds on this device have been assuming that the RTC is a 
DS1340. Manually clearing the reset bit things start working. I haven't 
confirmed but I think patchset 3 probably would have failed in this 
situation too.

Even though this might be considered operator error I think that the 
driver needs to detect OSF and return -EINVAL in max313xx_read_time like 
other RTC drivers do. And that max313xx_set_time needs to ensure that 
the SWRST bit is cleared. I can whip up a patch to be squashed into your 
next revision if that helps.

>
>> ---
>>   drivers/rtc/Kconfig        |   11 +
>>   drivers/rtc/Makefile       |    1 +
>>   drivers/rtc/rtc-max313xx.c | 1053 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1065 insertions(+)
>>   create mode 100644 drivers/rtc/rtc-max313xx.c
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index 753872408..2160619ed 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -323,6 +323,17 @@ config RTC_DRV_LP8788
>>       help
>>         Say Y to enable support for the LP8788 RTC/ALARM driver.
>>   +config RTC_DRV_MAX313XX
>> +    tristate "Analog Devices MAX313XX RTC driver"
>> +    depends on I2C
>> +    select REGMAP_I2C
>> +    help
>> +      If you say yes here you will get support for the
>> +      Analog Devices MAX313XX series RTC family.
>> +
>> +      This driver can also be built as a module. If so, the module
>> +      will be called rtc-max313xx.
>> +
>>   config RTC_DRV_MAX6900
>>       tristate "Maxim MAX6900"
>>       help
>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> index ea445d1eb..880db99be 100644
>> --- a/drivers/rtc/Makefile
>> +++ b/drivers/rtc/Makefile
>> @@ -88,6 +88,7 @@ obj-$(CONFIG_RTC_DRV_M41T94)    += rtc-m41t94.o
>>   obj-$(CONFIG_RTC_DRV_M48T35)    += rtc-m48t35.o
>>   obj-$(CONFIG_RTC_DRV_M48T59)    += rtc-m48t59.o
>>   obj-$(CONFIG_RTC_DRV_M48T86)    += rtc-m48t86.o
>> +obj-$(CONFIG_RTC_DRV_MAX313XX)    += rtc-max313xx.o
>>   obj-$(CONFIG_RTC_DRV_MAX6900)    += rtc-max6900.o
>>   obj-$(CONFIG_RTC_DRV_MAX6902)    += rtc-max6902.o
>>   obj-$(CONFIG_RTC_DRV_MAX6916)    += rtc-max6916.o
>> diff --git a/drivers/rtc/rtc-max313xx.c b/drivers/rtc/rtc-max313xx.c
>> new file mode 100644
>> index 000000000..043528d7d
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-max313xx.c
>> @@ -0,0 +1,1053 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Analog Devices MAX313XX series I2C RTC driver
>> + *
>> + * Copyright 2023 Analog Devices Inc.
>> + */
>> +#include <asm-generic/unaligned.h>
>> +#include <linux/bcd.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/rtc.h>
>> +#include <linux/util_macros.h>
>> +
>> +/* common registers */
>> +#define MAX313XX_INT_ALARM1        BIT(0)
>> +#define MAX313XX_HRS_F_AM_PM        BIT(5)
>> +#define MAX313XX_HRS_F_12_24        BIT(6)
>> +#define MAX313XX_MONTH_CENTURY        BIT(7)
>> +
>> +#define MAX313XX_TIME_SIZE        0x07
>> +
>> +/* device specific registers */
>> +#define MAX3134X_CFG2_REG        0x01
>> +#define MAX3134X_CFG2_SET_RTC        BIT(1)
>> +
>> +#define MAX31341_TRICKLE_RES_MASK    GENMASK(1, 0)
>> +#define MAX31341_TRICKLE_DIODE_EN    BIT(2)
>> +#define MAX31341_TRICKLE_ENABLE_BIT    BIT(3)
>> +#define MAX31341_POWER_MGMT_REG        0x56
>> +#define MAX31341_POWER_MGMT_TRICKLE_BIT    BIT(0)
>> +
>> +#define MAX3133X_TRICKLE_RES_MASK    GENMASK(2, 1)
>> +#define MAX3133X_TRICKLE_DIODE_EN    BIT(3)
>> +#define MAX3133X_TRICKLE_ENABLE_BIT    BIT(0)
>> +
>> +#define MAX31329_TRICKLE_ENABLE_BIT    BIT(7)
>> +#define MAX31343_TRICKLE_ENABLE_MASK    GENMASK(7, 4)
>> +#define MAX31343_TRICKLE_ENABLE_CODE    5
>> +#define MAX31329_43_TRICKLE_RES_MASK    GENMASK(1, 0)
>> +#define MAX31329_43_TRICKLE_DIODE_EN    BIT(2)
>> +
>> +#define MAX31329_CONFIG2_REG        0x04
>> +#define MAX31329_CONFIG2_CLKIN_EN    BIT(2)
>> +#define MAX31329_CONFIG2_CLKIN_FREQ    GENMASK(1, 0)
>> +
>> +#define MAX31341_42_CONFIG1_REG        0x00
>> +#define MAX31341_42_CONFIG1_CLKIN_EN    BIT(7)
>> +#define MAX31341_42_CONFIG1_CLKIN_FREQ    GENMASK(5, 4)
>> +#define MAX31341_42_CONFIG1_OSC_DISABLE    BIT(3)
>> +#define MAX31341_42_CONFIG1_SWRST    BIT(0)
>> +
>> +enum max313xx_ids {
>> +    ID_MAX31328,
>> +    ID_MAX31329,
>> +    ID_MAX31331,
>> +    ID_MAX31334,
>> +    ID_MAX31341,
>> +    ID_MAX31342,
>> +    ID_MAX31343,
>> +    MAX313XX_ID_NR
>> +};
>> +
>> +struct clkout_cfg {
>> +    const int *freq_avail;
>> +    u8 freq_size;
>> +    u8 freq_pos;
>> +    u8 reg;
>> +    u8 en_bit;
>> +    bool en_invert;
>> +};
>> +
>> +struct chip_desc {
>> +    struct clkout_cfg *clkout;
>> +    const char *clkout_name;
>> +    u8 sec_reg;
>> +    u8 alarm1_sec_reg;
>> +
>> +    u8 int_en_reg;
>> +    u8 int_status_reg;
>> +
>> +    u8 ram_reg;
>> +    u8 ram_size;
>> +
>> +    u8 temp_reg;
>> +
>> +    u8 trickle_reg;
>> +};
>> +
>> +#define clk_hw_to_max313xx(_hw) container_of(_hw, struct max313xx, 
>> clkout)
>> +
>> +struct max313xx {
>> +    enum max313xx_ids id;
>> +    struct regmap *regmap;
>> +    struct rtc_device *rtc;
>> +    struct clk_hw clkout;
>> +    struct clk *clkin;
>> +    const struct chip_desc *chip;
>> +    int irq;
>> +};
>> +
>> +static const int max313xx_clkin_freq[] = { 1, 50, 60, 32768 };
>> +
>> +static const int max31328_clkout_freq[] = { 1, 1024, 4096, 8192 };
>> +static const int max31329_clkout_freq[] = { 1, 4096, 8192, 32768 };
>> +static const int max3133x_clkout_freq[] = { 1, 64, 1024, 32768 };
>> +static const int max31341_42_clkout_freq[] = { 1, 50, 60, 32768 };
>> +static const int max31343_clkout_freq[] = { 1, 2, 4, 8, 16, 32, 64, 
>> 128, 32875 };
>> +
>> +static struct clkout_cfg max31328_clkout = {
>> +    .freq_avail = max31328_clkout_freq,
>> +    .freq_size = ARRAY_SIZE(max31328_clkout_freq),
>> +    .freq_pos = 3,
>> +    .reg = 0x0E,
>> +    .en_bit = BIT(3),
>> +    .en_invert = true,
>> +};
>> +
>> +static struct clkout_cfg max31329_clkout = {
>> +    .freq_avail = max31329_clkout_freq,
>> +    .freq_size = ARRAY_SIZE(max31329_clkout_freq),
>> +    .freq_pos = 5,
>> +    .reg = 0x04,
>> +    .en_bit = BIT(7),
>> +};
>> +
>> +static struct clkout_cfg max3133x_clkout = {
>> +    .freq_avail = max3133x_clkout_freq,
>> +    .freq_size = ARRAY_SIZE(max3133x_clkout_freq),
>> +    .freq_pos = 0,
>> +    .reg = 0x04,
>> +    .en_bit = BIT(2),
>> +};
>> +
>> +static struct clkout_cfg max31341_42_clkout = {
>> +    .freq_avail = max31341_42_clkout_freq,
>> +    .freq_size = ARRAY_SIZE(max31341_42_clkout_freq),
>> +    .freq_pos = 1,
>> +    .reg = 0x00,
>> +    .en_bit = BIT(6),
>> +    .en_invert = true,
>> +};
>> +
>> +static struct clkout_cfg max31343_clkout = {
>> +    .freq_avail = max31343_clkout_freq,
>> +    .freq_size = ARRAY_SIZE(max31343_clkout_freq),
>> +    .freq_pos = 3,
>> +    .reg = 0x04,
>> +    .en_bit = BIT(7),
>> +};
>> +
>> +static const struct chip_desc chip[MAX313XX_ID_NR] = {
>> +    [ID_MAX31328] = {
>> +        .int_en_reg = 0x0E,
>> +        .int_status_reg = 0x0F,
>> +        .sec_reg = 0x00,
>> +        .alarm1_sec_reg = 0x07,
>> +        .temp_reg = 0x11,
>> +        .clkout = &max31328_clkout,
>> +        .clkout_name = "max31328-sqw",
>> +    },
>> +    [ID_MAX31329] = {
>> +        .int_en_reg = 0x01,
>> +        .int_status_reg = 0x00,
>> +        .sec_reg = 0x06,
>> +        .alarm1_sec_reg = 0x0D,
>> +        .ram_reg = 0x22,
>> +        .ram_size = 64,
>> +        .trickle_reg = 0x19,
>> +        .clkout = &max31329_clkout,
>> +        .clkout_name = "max31329-clkout",
>> +    },
>> +    [ID_MAX31331] = {
>> +        .int_en_reg = 0x01,
>> +        .int_status_reg = 0x00,
>> +        .sec_reg = 0x08,
>> +        .alarm1_sec_reg = 0x0F,
>> +        .ram_reg = 0x20,
>> +        .ram_size = 32,
>> +        .trickle_reg = 0x1B,
>> +        .clkout = &max3133x_clkout,
>> +        .clkout_name = "max31331-clkout",
>> +    },
>> +    [ID_MAX31334] = {
>> +        .int_en_reg = 0x01,
>> +        .int_status_reg = 0x00,
>> +        .sec_reg = 0x09,
>> +        .alarm1_sec_reg = 0x10,
>> +        .ram_reg = 0x30,
>> +        .ram_size = 32,
>> +        .trickle_reg = 0x1E,
>> +        .clkout = &max3133x_clkout,
>> +        .clkout_name = "max31334-clkout",
>> +    },
>> +    [ID_MAX31341] = {
>> +        .int_en_reg = 0x04,
>> +        .int_status_reg = 0x05,
>> +        .sec_reg = 0x06,
>> +        .alarm1_sec_reg = 0x0D,
>> +        .ram_reg = 0x16,
>> +        .ram_size = 64,
>> +        .trickle_reg = 0x57,
>> +        .clkout = &max31341_42_clkout,
>> +        .clkout_name = "max31341-clkout",
>> +    },
>> +    [ID_MAX31342] = {
>> +        .int_en_reg = 0x04,
>> +        .int_status_reg = 0x05,
>> +        .sec_reg = 0x06,
>> +        .alarm1_sec_reg = 0x0D,
>> +        .clkout = &max31341_42_clkout,
>> +        .clkout_name = "max31342-clkout",
>> +    },
>> +    [ID_MAX31343] = {
>> +        .int_en_reg = 0x01,
>> +        .int_status_reg = 0x00,
>> +        .sec_reg = 0x06,
>> +        .alarm1_sec_reg = 0x0D,
>> +        .ram_reg = 0x22,
>> +        .ram_size = 64,
>> +        .temp_reg = 0x1A,
>> +        .trickle_reg = 0x19,
>> +        .clkout = &max31343_clkout,
>> +        .clkout_name = "max31343-clko",
>> +    },
>> +};
>> +
>> +static const u32 max313xx_trickle_ohms[] = { 3000, 6000, 11000 };
>> +
>> +static bool max313xx_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    const struct chip_desc *chip = rtc->chip;
>> +
>> +    /* time keeping registers */
>> +    if (reg >= chip->sec_reg && reg < chip->sec_reg + 
>> MAX313XX_TIME_SIZE)
>> +        return true;
>> +
>> +    /* interrupt status register */
>> +    if (reg == chip->int_status_reg)
>> +        return true;
>> +
>> +    /* temperature registers */
>> +    return chip->temp_reg &&
>> +        (reg == chip->temp_reg || reg == chip->temp_reg + 1);
>> +}
>> +
>> +static const struct regmap_config regmap_config = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = max313xx_volatile_reg,
>> +};
>> +
>> +static int max313xx_get_hour(u8 hour_reg)
>> +{
>> +    int hour;
>> +
>> +    /* 24Hr mode */
>> +    if (!FIELD_GET(MAX313XX_HRS_F_12_24, hour_reg))
>> +        return bcd2bin(hour_reg & 0x3f);
>> +
>> +    /* 12Hr mode */
>> +    hour = bcd2bin(hour_reg & 0x1f);
>> +    if (hour == 12)
>> +        hour = 0;
>> +
>> +    if (FIELD_GET(MAX313XX_HRS_F_AM_PM, hour_reg))
>> +        hour += 12;
>> +
>> +    return hour;
>> +}
>> +
>> +static int max313xx_read_time(struct device *dev, struct rtc_time *t)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    u8 regs[7];
>> +    int ret;
>> +
>> +    ret = regmap_bulk_read(rtc->regmap, rtc->chip->sec_reg, regs, 7);
>> +    if (ret)
>> +        return ret;
>> +
>> +    t->tm_sec = bcd2bin(regs[0] & 0x7f);
>> +    t->tm_min = bcd2bin(regs[1] & 0x7f);
>> +    t->tm_hour = max313xx_get_hour(regs[2]);
>> +    t->tm_wday = bcd2bin(regs[3] & 0x07) - 1;
>> +    t->tm_mday = bcd2bin(regs[4] & 0x3f);
>> +    t->tm_mon = bcd2bin(regs[5] & 0x1f) - 1;
>> +    t->tm_year = bcd2bin(regs[6]) + 100;
>> +
>> +    if (FIELD_GET(MAX313XX_MONTH_CENTURY, regs[5]))
>> +        t->tm_year += 100;
>> +
>> +    return 0;
>> +}
>> +
>> +static int max313xx_pre_set_time(struct max313xx *rtc)
>> +{
>> +    if (!rtc->clkin)
>> +        return 0;
>> +
>> +    /* Clkin needs to be disabled before setting time. */
>> +    switch (rtc->id) {
>> +    case ID_MAX31341:
>> +    case ID_MAX31342:
>> +        return regmap_clear_bits(rtc->regmap,
>> +                     MAX31341_42_CONFIG1_REG,
>> +                     MAX31341_42_CONFIG1_CLKIN_EN);
>> +    default:
>> +        return 0;
>> +    }
>> +}
>> +
>> +static int max313xx_post_set_time(struct max313xx *rtc)
>> +{
>> +    int ret;
>> +
>> +    switch (rtc->id) {
>> +    case ID_MAX31341:
>> +    case ID_MAX31342:
>> +        ret = regmap_set_bits(rtc->regmap, MAX3134X_CFG2_REG,
>> +                      MAX3134X_CFG2_SET_RTC);
>> +        if (ret)
>> +            return ret;
>> +
>> +        fsleep(10000);
>> +
>> +        ret = regmap_clear_bits(rtc->regmap, MAX3134X_CFG2_REG,
>> +                    MAX3134X_CFG2_SET_RTC);
>> +        if (ret)
>> +            return ret;
>> +
>> +        if (rtc->clkin)
>> +            ret = regmap_set_bits(rtc->regmap,
>> +                          MAX31341_42_CONFIG1_REG,
>> +                          MAX31341_42_CONFIG1_CLKIN_EN);
>> +
>> +        break;
>> +    default:
>> +        return 0;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int max313xx_set_time(struct device *dev, struct rtc_time *t)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    u8 regs[7];
>> +    int ret;
>> +
>> +    regs[0] = bin2bcd(t->tm_sec);
>> +    regs[1] = bin2bcd(t->tm_min);
>> +    regs[2] = bin2bcd(t->tm_hour);
>> +    regs[3] = bin2bcd(t->tm_wday + 1);
>> +    regs[4] = bin2bcd(t->tm_mday);
>> +    regs[5] = bin2bcd(t->tm_mon + 1);
>> +    regs[6] = bin2bcd(t->tm_year % 100);
>> +
>> +    if (t->tm_year >= 200)
>> +        regs[5] |= FIELD_PREP(MAX313XX_MONTH_CENTURY, 1);
>> +
>> +    ret = max313xx_pre_set_time(rtc);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_bulk_write(rtc->regmap, rtc->chip->sec_reg, regs, 7);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return max313xx_post_set_time(rtc);
>> +}
>> +
>> +static int max313xx_read_alarm(struct device *dev, struct rtc_wkalrm 
>> *t)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    unsigned int status, int_en;
>> +    struct rtc_time time;
>> +    u8 regs[6];
>> +    int ret;
>> +
>> +    ret = regmap_bulk_read(rtc->regmap, rtc->chip->alarm1_sec_reg, 
>> regs,
>> +                   sizeof(regs));
>> +    if (ret)
>> +        return ret;
>> +
>> +    t->time.tm_sec = bcd2bin(regs[0] & 0x7f);
>> +    t->time.tm_min = bcd2bin(regs[1] & 0x7f);
>> +    t->time.tm_hour = bcd2bin(regs[2] & 0x3f);
>> +    t->time.tm_mday = bcd2bin(regs[3] & 0x3f);
>> +    t->time.tm_mon = bcd2bin(regs[4] & 0x1f) - 1;
>> +    t->time.tm_year = bcd2bin(regs[5]) + 100;
>> +
>> +    ret = max313xx_read_time(dev, &time);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (time.tm_year >= 200)
>> +        t->time.tm_year += 100;
>> +
>> +    ret = regmap_read(rtc->regmap, rtc->chip->int_status_reg, &status);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_read(rtc->regmap, rtc->chip->int_en_reg, &int_en);
>> +    if (ret)
>> +        return ret;
>> +
>> +    t->enabled = FIELD_GET(MAX313XX_INT_ALARM1, int_en);
>> +    t->pending = FIELD_GET(MAX313XX_INT_ALARM1, status);
>> +
>> +    return 0;
>> +}
>> +
>> +static int max313xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    unsigned int reg;
>> +    u8 regs[6];
>> +    int ret;
>> +
>> +    regs[0] = bin2bcd(t->time.tm_sec);
>> +    regs[1] = bin2bcd(t->time.tm_min);
>> +    regs[2] = bin2bcd(t->time.tm_hour);
>> +    regs[3] = bin2bcd(t->time.tm_mday);
>> +    regs[4] = bin2bcd(t->time.tm_mon + 1);
>> +    regs[5] = bin2bcd(t->time.tm_year % 100);
>> +
>> +    ret = regmap_bulk_write(rtc->regmap, rtc->chip->alarm1_sec_reg, 
>> regs,
>> +                sizeof(regs));
>> +    if (ret)
>> +        return ret;
>> +
>> +    reg = FIELD_PREP(MAX313XX_INT_ALARM1, t->enabled);
>> +    ret = regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
>> +                 MAX313XX_INT_ALARM1, reg);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Clear status register */
>> +    return regmap_read(rtc->regmap, rtc->chip->int_status_reg, &reg);
>> +}
>> +
>> +static int max313xx_alarm_irq_enable(struct device *dev, unsigned 
>> int enabled)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +
>> +    return regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
>> +                  MAX313XX_INT_ALARM1,
>> +                  FIELD_PREP(MAX313XX_INT_ALARM1, enabled));
>> +}
>> +
>> +static const struct rtc_class_ops max3133x_rtc_ops = {
>> +    .read_time    = max313xx_read_time,
>> +    .set_time    = max313xx_set_time,
>> +    .read_alarm    = max313xx_read_alarm,
>> +    .set_alarm    = max313xx_set_alarm,
>> +    .alarm_irq_enable = max313xx_alarm_irq_enable,
>> +};
>> +
>> +static irqreturn_t max313xx_irq(int irq, void *dev_id)
>> +{
>> +    struct max313xx    *rtc = dev_id;
>> +    struct mutex *lock = &rtc->rtc->ops_lock;
>> +    int stat, ret;
>> +
>> +    mutex_lock(lock);
>> +    ret = regmap_read(rtc->regmap, rtc->chip->int_status_reg, &stat);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (FIELD_GET(MAX313XX_INT_ALARM1, stat)) {
>> +        ret = regmap_update_bits(rtc->regmap, rtc->chip->int_en_reg,
>> +                     MAX313XX_INT_ALARM1, 0);
>> +        if (ret)
>> +            goto out;
>> +
>> +        rtc_update_irq(rtc->rtc, 1, RTC_AF | RTC_IRQF);
>> +    }
>> +
>> +out:
>> +    mutex_unlock(lock);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int max313xx_nvmem_reg_read(void *priv, unsigned int offset,
>> +                   void *val, size_t bytes)
>> +{
>> +    struct max313xx *rtc = priv;
>> +    unsigned int reg = rtc->chip->ram_reg + offset;
>> +
>> +    return regmap_bulk_read(rtc->regmap, reg, val, bytes);
>> +}
>> +
>> +static int max313xx_nvmem_reg_write(void *priv, unsigned int offset,
>> +                    void *val, size_t bytes)
>> +{
>> +    struct max313xx *rtc = priv;
>> +    unsigned int reg = rtc->chip->ram_reg + offset;
>> +
>> +    return regmap_bulk_write(rtc->regmap, reg, val, bytes);
>> +}
>> +
>> +struct nvmem_config max313xx_nvmem_cfg = {
>> +    .reg_read = max313xx_nvmem_reg_read,
>> +    .reg_write = max313xx_nvmem_reg_write,
>> +    .word_size = 8,
>> +};
>> +
>> +static unsigned long max313xx_clkout_recalc_rate(struct clk_hw *hw,
>> +                         unsigned long parent_rate)
>> +{
>> +    struct max313xx *rtc = clk_hw_to_max313xx(hw);
>> +    const struct clkout_cfg *clkout = rtc->chip->clkout;
>> +    unsigned int freq_mask;
>> +    unsigned int reg;
>> +    int ret;
>> +
>> +    ret = regmap_read(rtc->regmap, clkout->reg, &reg);
>> +    if (ret)
>> +        return 0;
>> +
>> +    freq_mask = __roundup_pow_of_two(clkout->freq_size) - 1;
>> +
>> +    return clkout->freq_avail[(reg >> clkout->freq_pos) & freq_mask];
>> +}
>> +
>> +static long max313xx_clkout_round_rate(struct clk_hw *hw, unsigned 
>> long rate,
>> +                       unsigned long *prate)
>> +{
>> +    struct max313xx *rtc = clk_hw_to_max313xx(hw);
>> +    struct clkout_cfg *clkout = rtc->chip->clkout;
>> +    int index;
>> +
>> +    index = find_closest(rate, clkout->freq_avail, clkout->freq_size);
>> +    return clkout->freq_avail[index];
>> +}
>> +
>> +static int max313xx_clkout_set_rate(struct clk_hw *hw, unsigned long 
>> rate,
>> +                    unsigned long parent_rate)
>> +{
>> +    struct max313xx *rtc = clk_hw_to_max313xx(hw);
>> +    struct clkout_cfg *clkout = rtc->chip->clkout;
>> +    unsigned int freq_mask;
>> +    int index;
>> +
>> +    index = find_closest(rate, clkout->freq_avail, clkout->freq_size);
>> +    freq_mask = __roundup_pow_of_two(clkout->freq_size) - 1;
>> +
>> +    return regmap_update_bits(rtc->regmap, clkout->reg,
>> +                  freq_mask << clkout->freq_pos,
>> +                  index << clkout->freq_pos);
>> +}
>> +
>> +static int max313xx_clkout_enable(struct clk_hw *hw)
>> +{
>> +    struct max313xx *rtc = clk_hw_to_max313xx(hw);
>> +    struct clkout_cfg *clkout = rtc->chip->clkout;
>> +
>> +    if (clkout->en_invert)
>> +        return regmap_clear_bits(rtc->regmap, clkout->reg,
>> +                     clkout->en_bit);
>> +
>> +    return regmap_set_bits(rtc->regmap, clkout->reg, clkout->en_bit);
>> +}
>> +
>> +static void max313xx_clkout_disable(struct clk_hw *hw)
>> +{
>> +    struct max313xx *rtc = clk_hw_to_max313xx(hw);
>> +    struct clkout_cfg *clkout = rtc->chip->clkout;
>> +
>> +    switch (rtc->id) {
>> +    case ID_MAX31331:
>> +    case ID_MAX31334:
>> +        if (rtc->irq > 0) {
>> +            dev_err(rtc->rtc->dev.parent,
>> +                "clkout cannot be disabled when IRQ is requested");
>> +            return;
>> +        }
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    if (clkout->en_invert)
>> +        regmap_set_bits(rtc->regmap, clkout->reg, clkout->en_bit);
>> +    else
>> +        regmap_clear_bits(rtc->regmap, clkout->reg, clkout->en_bit);
>> +}
>> +
>> +static int max313xx_clkout_is_enabled(struct clk_hw *hw)
>> +{
>> +    struct max313xx *rtc = clk_hw_to_max313xx(hw);
>> +    struct clkout_cfg *clkout = rtc->chip->clkout;
>> +    unsigned int reg;
>> +    int ret;
>> +
>> +    ret = regmap_read(rtc->regmap, clkout->reg, &reg);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return !!(reg & clkout->en_bit) ^ clkout->en_invert;
>> +}
>> +
>> +static const struct clk_ops max313xx_clkout_ops = {
>> +    .recalc_rate = max313xx_clkout_recalc_rate,
>> +    .round_rate = max313xx_clkout_round_rate,
>> +    .set_rate = max313xx_clkout_set_rate,
>> +    .enable = max313xx_clkout_enable,
>> +    .disable = max313xx_clkout_disable,
>> +    .is_enabled = max313xx_clkout_is_enabled,
>> +};
>> +
>> +struct clk_init_data max313xx_clk_init = {
>> +    .name = "max313xx-clkout",
>> +    .ops = &max313xx_clkout_ops,
>> +};
>> +
>> +static int max313xx_read_temp(struct device *dev, enum 
>> hwmon_sensor_types type,
>> +                  u32 attr, int channel, long *val)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    u8 reg[2];
>> +    s16 temp;
>> +    int ret;
>> +
>> +    if (type != hwmon_temp || attr != hwmon_temp_input)
>> +        return -EOPNOTSUPP;
>> +
>> +    ret = regmap_bulk_read(rtc->regmap, rtc->chip->temp_reg, reg, 2);
>> +    if (ret)
>> +        return ret;
>> +
>> +    temp = get_unaligned_be16(reg);
>> +
>> +    *val = (temp / 64) * 250;
>> +
>> +    return 0;
>> +}
>> +
>> +static umode_t max313xx_is_visible(const void *data,
>> +                   enum hwmon_sensor_types type,
>> +                   u32 attr, int channel)
>> +{
>> +    if (type == hwmon_temp && attr == hwmon_temp_input)
>> +        return 0444;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info *max313xx_info[] = {
>> +    HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_ops max313xx_hwmon_ops = {
>> +    .is_visible = max313xx_is_visible,
>> +    .read = max313xx_read_temp,
>> +};
>> +
>> +static const struct hwmon_chip_info max313xx_chip_info = {
>> +    .ops = &max313xx_hwmon_ops,
>> +    .info = max313xx_info,
>> +};
>> +
>> +static int max313xx_init(struct max313xx *rtc)
>> +{
>> +    int ret;
>> +
>> +    switch (rtc->id) {
>> +    case ID_MAX31341:
>> +    case ID_MAX31342:
>> +        ret = regmap_clear_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
>> +                    MAX31341_42_CONFIG1_OSC_DISABLE);
>> +        if (ret)
>> +            return ret;
>> +
>> +        return regmap_set_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
>> +                       MAX31341_42_CONFIG1_SWRST);
>> +    default:
>> +        return 0;
>> +    }
>> +}
>> +
>> +static int max313xx_clkout_register(struct device *dev)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    int ret;
>> +
>> +    if (!device_property_present(dev, "#clock-cells"))
>> +        return 0;
>> +
>> +    max313xx_clk_init.name = rtc->chip->clkout_name;
>> +    device_property_read_string(dev, "clock-output-names",
>> +                    &max313xx_clk_init.name);
>> +    rtc->clkout.init = &max313xx_clk_init;
>> +
>> +    ret = devm_clk_hw_register(dev, &rtc->clkout);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "cannot register clock\n");
>> +
>> +    return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
>> +                       &rtc->clkout);
>> +}
>> +
>> +static int max313xx_trickle_charger_setup(struct device *dev)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    bool trickle_diode_en;
>> +    u32 charger_en = 0;
>> +    int index, reg;
>> +    u32 ohms = 0;
>> +    int ret;
>> +
>> +    device_property_read_u32(dev, "aux-voltage-chargeable", 
>> &charger_en);
>> +    switch (charger_en) {
>> +    case 0:
>> +        return 0;
>> +    case 1:
>> +        trickle_diode_en = 0;
>> +        break;
>> +    case 2:
>> +        trickle_diode_en = 1;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    device_property_read_u32(dev, "trickle-resistor-ohms", &ohms);
>> +
>> +    if (!rtc->chip->trickle_reg)
>> +        return 0;
>> +
>> +    index = find_closest(ohms, max313xx_trickle_ohms,
>> +                 ARRAY_SIZE(max313xx_trickle_ohms)) + 1;
>> +
>> +    switch (rtc->id) {
>> +    case ID_MAX31329:
>> +        reg = FIELD_PREP(MAX31329_TRICKLE_ENABLE_BIT, 1) |
>> +              FIELD_PREP(MAX31329_43_TRICKLE_RES_MASK, index) |
>> +              FIELD_PREP(MAX31329_43_TRICKLE_DIODE_EN, 
>> trickle_diode_en);
>> +        break;
>> +    case ID_MAX31331:
>> +    case ID_MAX31334:
>> +        reg = FIELD_PREP(MAX3133X_TRICKLE_ENABLE_BIT, 1) |
>> +              FIELD_PREP(MAX3133X_TRICKLE_RES_MASK, index) |
>> +              FIELD_PREP(MAX3133X_TRICKLE_DIODE_EN, trickle_diode_en);
>> +        break;
>> +    case ID_MAX31341:
>> +        if (index == 1)
>> +            index = 0;
>> +
>> +        reg = FIELD_PREP(MAX31341_TRICKLE_ENABLE_BIT, 1) |
>> +              FIELD_PREP(MAX31341_TRICKLE_RES_MASK, index) |
>> +              FIELD_PREP(MAX31341_TRICKLE_DIODE_EN, trickle_diode_en);
>> +
>> +        ret = regmap_set_bits(rtc->regmap, MAX31341_POWER_MGMT_REG,
>> +                      MAX31341_POWER_MGMT_TRICKLE_BIT);
>> +        if (ret)
>> +            return ret;
>> +
>> +        break;
>> +    case ID_MAX31343:
>> +        reg = FIELD_PREP(MAX31329_43_TRICKLE_RES_MASK, index) |
>> +              FIELD_PREP(MAX31329_43_TRICKLE_DIODE_EN, 
>> trickle_diode_en) |
>> +              FIELD_PREP(MAX31343_TRICKLE_ENABLE_MASK,
>> +                 MAX31343_TRICKLE_ENABLE_CODE);
>> +        break;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    return regmap_write(rtc->regmap, rtc->chip->trickle_reg, reg);
>> +}
>> +
>> +static int max313xx_find_clkin_freq_index(struct clk *clk)
>> +{
>> +    unsigned long rate = clk_get_rate(clk);
>> +    int freq;
>> +    int i;
>> +
>> +    i = find_closest(rate, max313xx_clkin_freq,
>> +             ARRAY_SIZE(max313xx_clkin_freq));
>> +    if (max313xx_clkin_freq[i] == rate)
>> +        return i;
>> +
>> +    for (i = ARRAY_SIZE(max313xx_clkin_freq) - 1; i >= 0; i--) {
>> +        freq = max313xx_clkin_freq[i];
>> +        if (freq == clk_round_rate(clk, freq))
>> +            return i;
>> +    }
>> +
>> +    /* supplied clock cannot produce one of desired frequency rate */
>> +    return -ENODEV;
>> +}
>> +
>> +static int max313xx_clkin_init(struct device *dev)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    int rate;
>> +    int ret;
>> +
>> +    rtc->clkin = devm_clk_get_enabled(dev, NULL);
>> +    if (IS_ERR(rtc->clkin)) {
>> +        if (PTR_ERR(rtc->clkin) == -ENOENT)
>> +            rtc->clkin = NULL;
>> +        else
>> +            return dev_err_probe(dev, PTR_ERR(rtc->clkin),
>> +                         "error while clkin setup\n");
>> +    }
>> +
>> +    if (!rtc->clkin) {
>> +        switch (rtc->id) {
>> +        case ID_MAX31329:
>> +            return regmap_clear_bits(rtc->regmap,
>> +                         MAX31329_CONFIG2_REG,
>> +                         MAX31329_CONFIG2_CLKIN_EN);
>> +        case ID_MAX31341:
>> +        case ID_MAX31342:
>> +            return regmap_clear_bits(rtc->regmap,
>> +                         MAX31341_42_CONFIG1_REG,
>> +                         MAX31341_42_CONFIG1_CLKIN_EN);
>> +        default:
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    rate = max313xx_find_clkin_freq_index(rtc->clkin);
>> +    if (rate < 0)
>> +        return dev_err_probe(dev, rate,
>> +                     "clkin cannot produce required frequency\n");
>> +
>> +    ret = clk_set_rate(rtc->clkin, max313xx_clkin_freq[rate]);
>> +    if (ret)
>> +        return ret;
>> +
>> +    switch (rtc->id) {
>> +    case ID_MAX31329:
>> +        ret = regmap_update_bits(rtc->regmap, MAX31329_CONFIG2_REG,
>> +                     MAX31329_CONFIG2_CLKIN_FREQ, rate);
>> +        if (ret)
>> +            return ret;
>> +
>> +        return regmap_set_bits(rtc->regmap, MAX31329_CONFIG2_REG,
>> +                       MAX31329_CONFIG2_CLKIN_EN);
>> +    case ID_MAX31341:
>> +    case ID_MAX31342:
>> +        ret = regmap_update_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
>> +                     MAX31341_42_CONFIG1_CLKIN_FREQ,
>> +                     FIELD_PREP(MAX31341_42_CONFIG1_CLKIN_FREQ, rate));
>> +        if (ret)
>> +            return ret;
>> +
>> +        return regmap_set_bits(rtc->regmap, MAX31341_42_CONFIG1_REG,
>> +                       MAX31341_42_CONFIG1_CLKIN_EN);
>> +    default:
>> +        rtc->clkin = NULL;
>> +        dev_warn(dev, "device does not have clock input\n");
>> +        return 0;
>> +    }
>> +}
>> +
>> +static int max313xx_irq_init(struct device *dev, const char *devname)
>> +{
>> +    struct max313xx *rtc = dev_get_drvdata(dev);
>> +    int ret;
>> +
>> +    switch (rtc->id) {
>> +    case ID_MAX31328:
>> +        break;
>> +    case ID_MAX31331:
>> +    case ID_MAX31334:
>> +        if (rtc->clkout.clk) {
>> +            /*
>> +             * interrupt muxing depends on clkout so enable clkout
>> +             * if configured before requesting interrupt
>> +             */
>> +            ret = clk_prepare_enable(rtc->clkout.clk);
>> +            if (ret)
>> +                return dev_err_probe(dev, ret,
>> +                             "cannot enable clkout\n");
>> +        }
>> +        break;
>> +    default:
>> +        if (rtc->clkin) {
>> +            if (rtc->clkout.clk && rtc->irq > 0)
>> +                return dev_err_probe(dev, -EOPNOTSUPP,
>> +                             "irq not possible when both clkin and 
>> clkout are configured\n");
>> +
>> +            if (rtc->irq <= 0)
>> +                break;
>> +
>> +            /* clkout needs to be disabled for interrupt */
>> +            if (rtc->chip->clkout->en_invert)
>> +                ret = regmap_set_bits(rtc->regmap,
>> +                              rtc->chip->clkout->reg,
>> + rtc->chip->clkout->en_bit);
>> +            else
>> +                ret = regmap_clear_bits(rtc->regmap,
>> +                            rtc->chip->clkout->reg,
>> + rtc->chip->clkout->en_bit);
>> +
>> +            if (ret)
>> +                return ret;
>> +        }
>> +        break;
>> +    }
>> +
>> +    if (rtc->irq > 0) {
>> +        return devm_request_threaded_irq(dev, rtc->irq, NULL,
>> +                         &max313xx_irq, IRQF_ONESHOT,
>> +                         devname, rtc);
>> +    } else if (device_property_read_bool(dev, "wakeup-source")) {
>> +        clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtc->features);
>> +        return device_init_wakeup(dev, true);
>> +    }
>> +
>> +    clear_bit(RTC_FEATURE_ALARM, rtc->rtc->features);
>> +
>> +    return 0;
>> +}
>> +
>> +static int max313xx_probe(struct i2c_client *client,
>> +              const struct i2c_device_id *id)
>> +{
>> +    struct device *dev = &client->dev;
>> +    struct max313xx *max313xx;
>> +    struct device *hwmon;
>> +    const void *match;
>> +    int ret;
>> +
>> +    max313xx = devm_kzalloc(&client->dev, sizeof(*max313xx), 
>> GFP_KERNEL);
>> +    if (!max313xx)
>> +        return -ENOMEM;
>> +
>> +    dev_set_drvdata(&client->dev, max313xx);
>> +
>> +    max313xx->regmap = devm_regmap_init_i2c(client, &regmap_config);
>> +    if (IS_ERR(max313xx->regmap)) {
>> +        return dev_err_probe(dev, PTR_ERR(max313xx->regmap),
>> +                     "regmap init failed\n");
>> +    }
>> +
>> +    i2c_set_clientdata(client, max313xx);
>> +
>> +    match = device_get_match_data(dev);
>> +    if (match)
>> +        max313xx->chip = match;
>> +    else if (id)
>> +        max313xx->chip = (struct chip_desc *)id->driver_data;
>> +    else
>> +        return -ENODEV;
>> +
>> +    max313xx->id = max313xx->chip - chip;
>> +
>> +    ret = max313xx_init(max313xx);
>> +    if (ret)
>> +        return ret;
>> +
>> +    max313xx->rtc = devm_rtc_allocate_device(dev);
>> +    if (IS_ERR(max313xx->rtc))
>> +        return PTR_ERR(max313xx->rtc);
>> +
>> +    max313xx->rtc->ops = &max3133x_rtc_ops;
>> +    max313xx->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>> +    max313xx->rtc->range_max = RTC_TIMESTAMP_END_2199;
>> +
>> +    ret = devm_rtc_register_device(max313xx->rtc);
>> +    if (ret)
>> +        return ret;
>> +
>> +    max313xx->irq = client->irq;
>> +
>> +    ret = max313xx_clkout_register(dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = max313xx_clkin_init(dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* IRQ wiring depends on the clock configuration so parse them 
>> first */
>> +    ret = max313xx_irq_init(dev, client->name);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (max313xx->chip->ram_size) {
>> +        max313xx_nvmem_cfg.size = max313xx->chip->ram_size;
>> +        max313xx_nvmem_cfg.priv = max313xx;
>> +
>> +        ret = devm_rtc_nvmem_register(max313xx->rtc, 
>> &max313xx_nvmem_cfg);
>> +        if (ret)
>> +            dev_warn(dev, "cannot register rtc nvmem\n");
>> +    }
>> +
>> +    if (max313xx->chip->temp_reg) {
>> +        hwmon = devm_hwmon_device_register_with_info(dev, client->name,
>> +                                 max313xx,
>> +                                 &max313xx_chip_info,
>> +                                 NULL);
>> +        if (IS_ERR(hwmon))
>> +            dev_warn(dev, "cannot register hwmon device: %li\n",
>> +                 PTR_ERR(hwmon));
>> +    }
>> +
>> +    return max313xx_trickle_charger_setup(dev);
>> +}
>> +
>> +static const struct of_device_id max313xx_of_id[] = {
>> +    { .compatible = "adi,max31328", .data = &chip[ID_MAX31328] },
>> +    { .compatible = "adi,max31329", .data = &chip[ID_MAX31329] },
>> +    { .compatible = "adi,max31331", .data = &chip[ID_MAX31331] },
>> +    { .compatible = "adi,max31334", .data = &chip[ID_MAX31334] },
>> +    { .compatible = "adi,max31341", .data = &chip[ID_MAX31341] },
>> +    { .compatible = "adi,max31342", .data = &chip[ID_MAX31342] },
>> +    { .compatible = "adi,max31343", .data = &chip[ID_MAX31343] },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(of, max313xx_of_id);
>> +
>> +static const struct i2c_device_id max313xx_id[] = {
>> +    { "max31328", (kernel_ulong_t)&chip[ID_MAX31328] },
>> +    { "max31329", (kernel_ulong_t)&chip[ID_MAX31329] },
>> +    { "max31331", (kernel_ulong_t)&chip[ID_MAX31331] },
>> +    { "max31334", (kernel_ulong_t)&chip[ID_MAX31334] },
>> +    { "max31341", (kernel_ulong_t)&chip[ID_MAX31341] },
>> +    { "max31342", (kernel_ulong_t)&chip[ID_MAX31342] },
>> +    { "max31343", (kernel_ulong_t)&chip[ID_MAX31343] },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, max313xx_id);
>> +
>> +static struct i2c_driver max313xx_driver = {
>> +    .driver = {
>> +        .name    = "rtc-max313xx",
>> +        .of_match_table = max313xx_of_id,
>> +    },
>> +    .probe        = max313xx_probe,
>> +    .id_table    = max313xx_id,
>> +};
>> +module_i2c_driver(max313xx_driver);
>> +
>> +MODULE_DESCRIPTION("Analog Devices MAX313XX RTCs");
>> +MODULE_AUTHOR("Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>");
>> +MODULE_AUTHOR("Ibrahim Tilki <Ibrahim.Tilki@analog.com>");
>> +MODULE_SOFTDEP("pre: regmap-i2c");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION("1.0");

Re: [PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Chris Packham]

Hi All,

On 4/04/23 03:43, Ibrahim Tilki wrote:
> changelog:
> since v5:
>    - dt-binding: add enum value "2" to aux-voltage-chargable
>    - dt-binding: remove adi,trickle-diode-enable
>    - dt-binding: change description of trickle-resistor-ohms
>    - dt-binding: reorder as in example schema
>    - parse "wakeup-source" when irq not requested
>    - remove limitation on max31328 irq and clokout
>    - remove error and warning messages during trickle charger setup
>
> since v4:
>    - dt-binding: remove interrupt names.
>    - dt-binding: add description for "interrupts" property
>    - dt-binding: replace deprecated property "trickle-diode-disable"
>        by "aux-voltage-chargeable"
>    - dt-binding: add new property "adi,trickle-diode-enable"
>    - dt-binding: remove "wakeup-source"
>    - use clear_bit instead of __clear_bit
>    - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
>    - use chip_desc pointer as driver data instead of enum.
>
> since v3:
>    - add "break" to fix warning: unannotated fall-through
>      Reported-by: kernel test robot <lkp@intel.com>
>
> since v2:
>    - dt-binding: update title and description
>    - dt-binding: remove last example
>    - drop watchdog support
>    - support reading 12Hr format instead of forcing 24hr at probe time
>    - use "tm_year % 100" instead of range check
>    - refactor max313xx_init for readability
>
> Ibrahim Tilki (2):
>    drivers: rtc: add max313xx series rtc driver
>    dt-bindings: rtc: add max313xx RTCs
>
>   .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
>   drivers/rtc/Kconfig                           |   11 +
>   drivers/rtc/Makefile                          |    1 +
>   drivers/rtc/rtc-max313xx.c                    | 1053 +++++++++++++++++
>   4 files changed, 1209 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
>   create mode 100644 drivers/rtc/rtc-max313xx.c

What happened to this series in the end? It kind of went off my radar 
and I forgot about it.

We've been carrying a version of these changes in our local tree for a 
while (and using it quite happily I should add).

Re: [PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Nuno Sá]

On Fri, 2024-01-26 at 02:22 +0000, Chris Packham wrote:
> Hi All,
> 
> On 4/04/23 03:43, Ibrahim Tilki wrote:
> > changelog:
> > since v5:
> >    - dt-binding: add enum value "2" to aux-voltage-chargable
> >    - dt-binding: remove adi,trickle-diode-enable
> >    - dt-binding: change description of trickle-resistor-ohms
> >    - dt-binding: reorder as in example schema
> >    - parse "wakeup-source" when irq not requested
> >    - remove limitation on max31328 irq and clokout
> >    - remove error and warning messages during trickle charger setup
> > 
> > since v4:
> >    - dt-binding: remove interrupt names.
> >    - dt-binding: add description for "interrupts" property
> >    - dt-binding: replace deprecated property "trickle-diode-disable"
> >        by "aux-voltage-chargeable"
> >    - dt-binding: add new property "adi,trickle-diode-enable"
> >    - dt-binding: remove "wakeup-source"
> >    - use clear_bit instead of __clear_bit
> >    - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
> >    - use chip_desc pointer as driver data instead of enum.
> > 
> > since v3:
> >    - add "break" to fix warning: unannotated fall-through
> >      Reported-by: kernel test robot <lkp@intel.com>
> > 
> > since v2:
> >    - dt-binding: update title and description
> >    - dt-binding: remove last example
> >    - drop watchdog support
> >    - support reading 12Hr format instead of forcing 24hr at probe time
> >    - use "tm_year % 100" instead of range check
> >    - refactor max313xx_init for readability
> > 
> > Ibrahim Tilki (2):
> >    drivers: rtc: add max313xx series rtc driver
> >    dt-bindings: rtc: add max313xx RTCs
> > 
> >   .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
> >   drivers/rtc/Kconfig                           |   11 +
> >   drivers/rtc/Makefile                          |    1 +
> >   drivers/rtc/rtc-max313xx.c                    | 1053 +++++++++++++++++
> >   4 files changed, 1209 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> >   create mode 100644 drivers/rtc/rtc-max313xx.c
> 
> What happened to this series in the end? It kind of went off my radar 
> and I forgot about it.
> 
> We've been carrying a version of these changes in our local tree for a 
> while (and using it quite happily I should add).
> 

Hi Chris,

Also not sure.... In the meantime Ibrahim left ADI so if this is not in shape to
be merged he won't be able to re-spin. If there's a need for a re-spin, please
let me know so I can see internally if there's someone who can continue this
work. I would do it myself if I had the HW.

- Nuno Sá

Re: [PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Chris Packham]

On 26/01/24 20:51, Nuno Sá wrote:
> On Fri, 2024-01-26 at 02:22 +0000, Chris Packham wrote:
>> Hi All,
>>
>> On 4/04/23 03:43, Ibrahim Tilki wrote:
>>> changelog:
>>> since v5:
>>>     - dt-binding: add enum value "2" to aux-voltage-chargable
>>>     - dt-binding: remove adi,trickle-diode-enable
>>>     - dt-binding: change description of trickle-resistor-ohms
>>>     - dt-binding: reorder as in example schema
>>>     - parse "wakeup-source" when irq not requested
>>>     - remove limitation on max31328 irq and clokout
>>>     - remove error and warning messages during trickle charger setup
>>>
>>> since v4:
>>>     - dt-binding: remove interrupt names.
>>>     - dt-binding: add description for "interrupts" property
>>>     - dt-binding: replace deprecated property "trickle-diode-disable"
>>>         by "aux-voltage-chargeable"
>>>     - dt-binding: add new property "adi,trickle-diode-enable"
>>>     - dt-binding: remove "wakeup-source"
>>>     - use clear_bit instead of __clear_bit
>>>     - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
>>>     - use chip_desc pointer as driver data instead of enum.
>>>
>>> since v3:
>>>     - add "break" to fix warning: unannotated fall-through
>>>       Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> since v2:
>>>     - dt-binding: update title and description
>>>     - dt-binding: remove last example
>>>     - drop watchdog support
>>>     - support reading 12Hr format instead of forcing 24hr at probe time
>>>     - use "tm_year % 100" instead of range check
>>>     - refactor max313xx_init for readability
>>>
>>> Ibrahim Tilki (2):
>>>     drivers: rtc: add max313xx series rtc driver
>>>     dt-bindings: rtc: add max313xx RTCs
>>>
>>>    .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
>>>    drivers/rtc/Kconfig                           |   11 +
>>>    drivers/rtc/Makefile                          |    1 +
>>>    drivers/rtc/rtc-max313xx.c                    | 1053 +++++++++++++++++
>>>    4 files changed, 1209 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
>>>    create mode 100644 drivers/rtc/rtc-max313xx.c
>> What happened to this series in the end? It kind of went off my radar
>> and I forgot about it.
>>
>> We've been carrying a version of these changes in our local tree for a
>> while (and using it quite happily I should add).
>>
> Hi Chris,
>
> Also not sure.... In the meantime Ibrahim left ADI so if this is not in shape to
> be merged he won't be able to re-spin. If there's a need for a re-spin, please
> let me know so I can see internally if there's someone who can continue this
> work. I would do it myself if I had the HW.
I've got a board with a max31331 so I can test that. I don't have any of 
the interrupts hooked up so I won't be able to test that. Looks like 
there was some outstanding discussion around the trickle-charge 
devicetree properties so I'd need to figure out what was wanted there. 
I'll try to pick up the last series from the mailing list and go from there.

Re: [PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Nuno Sá]

On Mon, 2024-01-29 at 03:28 +0000, Chris Packham wrote:
> 
> On 26/01/24 20:51, Nuno Sá wrote:
> > On Fri, 2024-01-26 at 02:22 +0000, Chris Packham wrote:
> > > Hi All,
> > > 
> > > On 4/04/23 03:43, Ibrahim Tilki wrote:
> > > > changelog:
> > > > since v5:
> > > >     - dt-binding: add enum value "2" to aux-voltage-chargable
> > > >     - dt-binding: remove adi,trickle-diode-enable
> > > >     - dt-binding: change description of trickle-resistor-ohms
> > > >     - dt-binding: reorder as in example schema
> > > >     - parse "wakeup-source" when irq not requested
> > > >     - remove limitation on max31328 irq and clokout
> > > >     - remove error and warning messages during trickle charger setup
> > > > 
> > > > since v4:
> > > >     - dt-binding: remove interrupt names.
> > > >     - dt-binding: add description for "interrupts" property
> > > >     - dt-binding: replace deprecated property "trickle-diode-disable"
> > > >         by "aux-voltage-chargeable"
> > > >     - dt-binding: add new property "adi,trickle-diode-enable"
> > > >     - dt-binding: remove "wakeup-source"
> > > >     - use clear_bit instead of __clear_bit
> > > >     - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
> > > >     - use chip_desc pointer as driver data instead of enum.
> > > > 
> > > > since v3:
> > > >     - add "break" to fix warning: unannotated fall-through
> > > >       Reported-by: kernel test robot <lkp@intel.com>
> > > > 
> > > > since v2:
> > > >     - dt-binding: update title and description
> > > >     - dt-binding: remove last example
> > > >     - drop watchdog support
> > > >     - support reading 12Hr format instead of forcing 24hr at probe time
> > > >     - use "tm_year % 100" instead of range check
> > > >     - refactor max313xx_init for readability
> > > > 
> > > > Ibrahim Tilki (2):
> > > >     drivers: rtc: add max313xx series rtc driver
> > > >     dt-bindings: rtc: add max313xx RTCs
> > > > 
> > > >    .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
> > > >    drivers/rtc/Kconfig                           |   11 +
> > > >    drivers/rtc/Makefile                          |    1 +
> > > >    drivers/rtc/rtc-max313xx.c                    | 1053
> > > > +++++++++++++++++
> > > >    4 files changed, 1209 insertions(+)
> > > >    create mode 100644
> > > > Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > > >    create mode 100644 drivers/rtc/rtc-max313xx.c
> > > What happened to this series in the end? It kind of went off my radar
> > > and I forgot about it.
> > > 
> > > We've been carrying a version of these changes in our local tree for a
> > > while (and using it quite happily I should add).
> > > 
> > Hi Chris,
> > 
> > Also not sure.... In the meantime Ibrahim left ADI so if this is not in
> > shape to
> > be merged he won't be able to re-spin. If there's a need for a re-spin,
> > please
> > let me know so I can see internally if there's someone who can continue this
> > work. I would do it myself if I had the HW.
> I've got a board with a max31331 so I can test that. I don't have any of 
> the interrupts hooked up so I won't be able to test that. Looks like 
> there was some outstanding discussion around the trickle-charge 
> devicetree properties so I'd need to figure out what was wanted there. 
> I'll try to pick up the last series from the mailing list and go from there.

Thanks Chris!

- Nuno Sá

Re: [PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Chris Packham]

On 29/01/24 20:55, Nuno Sá wrote:
> On Mon, 2024-01-29 at 03:28 +0000, Chris Packham wrote:
>> On 26/01/24 20:51, Nuno Sá wrote:
>>> On Fri, 2024-01-26 at 02:22 +0000, Chris Packham wrote:
>>>> Hi All,
>>>>
>>>> On 4/04/23 03:43, Ibrahim Tilki wrote:
>>>>> changelog:
>>>>> since v5:
>>>>>      - dt-binding: add enum value "2" to aux-voltage-chargable
>>>>>      - dt-binding: remove adi,trickle-diode-enable
>>>>>      - dt-binding: change description of trickle-resistor-ohms
>>>>>      - dt-binding: reorder as in example schema
>>>>>      - parse "wakeup-source" when irq not requested
>>>>>      - remove limitation on max31328 irq and clokout
>>>>>      - remove error and warning messages during trickle charger setup
>>>>>
>>>>> since v4:
>>>>>      - dt-binding: remove interrupt names.
>>>>>      - dt-binding: add description for "interrupts" property
>>>>>      - dt-binding: replace deprecated property "trickle-diode-disable"
>>>>>          by "aux-voltage-chargeable"
>>>>>      - dt-binding: add new property "adi,trickle-diode-enable"
>>>>>      - dt-binding: remove "wakeup-source"
>>>>>      - use clear_bit instead of __clear_bit
>>>>>      - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
>>>>>      - use chip_desc pointer as driver data instead of enum.
>>>>>
>>>>> since v3:
>>>>>      - add "break" to fix warning: unannotated fall-through
>>>>>        Reported-by: kernel test robot <lkp@intel.com>
>>>>>
>>>>> since v2:
>>>>>      - dt-binding: update title and description
>>>>>      - dt-binding: remove last example
>>>>>      - drop watchdog support
>>>>>      - support reading 12Hr format instead of forcing 24hr at probe time
>>>>>      - use "tm_year % 100" instead of range check
>>>>>      - refactor max313xx_init for readability
>>>>>
>>>>> Ibrahim Tilki (2):
>>>>>      drivers: rtc: add max313xx series rtc driver
>>>>>      dt-bindings: rtc: add max313xx RTCs
>>>>>
>>>>>     .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
>>>>>     drivers/rtc/Kconfig                           |   11 +
>>>>>     drivers/rtc/Makefile                          |    1 +
>>>>>     drivers/rtc/rtc-max313xx.c                    | 1053
>>>>> +++++++++++++++++
>>>>>     4 files changed, 1209 insertions(+)
>>>>>     create mode 100644
>>>>> Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
>>>>>     create mode 100644 drivers/rtc/rtc-max313xx.c
>>>> What happened to this series in the end? It kind of went off my radar
>>>> and I forgot about it.
>>>>
>>>> We've been carrying a version of these changes in our local tree for a
>>>> while (and using it quite happily I should add).
>>>>
>>> Hi Chris,
>>>
>>> Also not sure.... In the meantime Ibrahim left ADI so if this is not in
>>> shape to
>>> be merged he won't be able to re-spin. If there's a need for a re-spin,
>>> please
>>> let me know so I can see internally if there's someone who can continue this
>>> work. I would do it myself if I had the HW.
>> I've got a board with a max31331 so I can test that. I don't have any of
>> the interrupts hooked up so I won't be able to test that. Looks like
>> there was some outstanding discussion around the trickle-charge
>> devicetree properties so I'd need to figure out what was wanted there.
>> I'll try to pick up the last series from the mailing list and go from there.

I see that in the meantime Antoniu has landed a max31335 driver. Does 
anyone know off-hand how close the max31335 is to the other max313xx 
variants? Should I leave them separate or attempt to integrate the two.

Re: [PATCH v5 0/2] drivers: rtc: add max313xx series rtc driver

[Nuno Sá]

On Fri, 2024-02-02 at 00:28 +0000, Chris Packham wrote:
> 
> On 29/01/24 20:55, Nuno Sá wrote:
> > On Mon, 2024-01-29 at 03:28 +0000, Chris Packham wrote:
> > > On 26/01/24 20:51, Nuno Sá wrote:
> > > > On Fri, 2024-01-26 at 02:22 +0000, Chris Packham wrote:
> > > > > Hi All,
> > > > > 
> > > > > On 4/04/23 03:43, Ibrahim Tilki wrote:
> > > > > > changelog:
> > > > > > since v5:
> > > > > >      - dt-binding: add enum value "2" to aux-voltage-chargable
> > > > > >      - dt-binding: remove adi,trickle-diode-enable
> > > > > >      - dt-binding: change description of trickle-resistor-ohms
> > > > > >      - dt-binding: reorder as in example schema
> > > > > >      - parse "wakeup-source" when irq not requested
> > > > > >      - remove limitation on max31328 irq and clokout
> > > > > >      - remove error and warning messages during trickle charger setup
> > > > > > 
> > > > > > since v4:
> > > > > >      - dt-binding: remove interrupt names.
> > > > > >      - dt-binding: add description for "interrupts" property
> > > > > >      - dt-binding: replace deprecated property "trickle-diode-disable"
> > > > > >          by "aux-voltage-chargeable"
> > > > > >      - dt-binding: add new property "adi,trickle-diode-enable"
> > > > > >      - dt-binding: remove "wakeup-source"
> > > > > >      - use clear_bit instead of __clear_bit
> > > > > >      - use devm_of_clk_add_hw_provider instead of of_clk_add_provider
> > > > > >      - use chip_desc pointer as driver data instead of enum.
> > > > > > 
> > > > > > since v3:
> > > > > >      - add "break" to fix warning: unannotated fall-through
> > > > > >        Reported-by: kernel test robot <lkp@intel.com>
> > > > > > 
> > > > > > since v2:
> > > > > >      - dt-binding: update title and description
> > > > > >      - dt-binding: remove last example
> > > > > >      - drop watchdog support
> > > > > >      - support reading 12Hr format instead of forcing 24hr at probe time
> > > > > >      - use "tm_year % 100" instead of range check
> > > > > >      - refactor max313xx_init for readability
> > > > > > 
> > > > > > Ibrahim Tilki (2):
> > > > > >      drivers: rtc: add max313xx series rtc driver
> > > > > >      dt-bindings: rtc: add max313xx RTCs
> > > > > > 
> > > > > >     .../devicetree/bindings/rtc/adi,max313xx.yaml |  144 +++
> > > > > >     drivers/rtc/Kconfig                           |   11 +
> > > > > >     drivers/rtc/Makefile                          |    1 +
> > > > > >     drivers/rtc/rtc-max313xx.c                    | 1053
> > > > > > +++++++++++++++++
> > > > > >     4 files changed, 1209 insertions(+)
> > > > > >     create mode 100644
> > > > > > Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > > > > >     create mode 100644 drivers/rtc/rtc-max313xx.c
> > > > > What happened to this series in the end? It kind of went off my radar
> > > > > and I forgot about it.
> > > > > 
> > > > > We've been carrying a version of these changes in our local tree for a
> > > > > while (and using it quite happily I should add).
> > > > > 
> > > > Hi Chris,
> > > > 
> > > > Also not sure.... In the meantime Ibrahim left ADI so if this is not in
> > > > shape to
> > > > be merged he won't be able to re-spin. If there's a need for a re-spin,
> > > > please
> > > > let me know so I can see internally if there's someone who can continue this
> > > > work. I would do it myself if I had the HW.
> > > I've got a board with a max31331 so I can test that. I don't have any of
> > > the interrupts hooked up so I won't be able to test that. Looks like
> > > there was some outstanding discussion around the trickle-charge
> > > devicetree properties so I'd need to figure out what was wanted there.
> > > I'll try to pick up the last series from the mailing list and go from there.
> 
> I see that in the meantime Antoniu has landed a max31335 driver. Does 
> anyone know off-hand how close the max31335 is to the other max313xx 
> variants? Should I leave them separate or attempt to integrate the two.
> 

Hi Chris,

I did not looked into the devices datasheets but typically, if the register map is
close enough we should try to integrate them in the same driver.

Thanks for continuing this work btw!
- Nuno Sa