Hello,

On 16/10/2021 22:21:17+0300, Alexey Firago wrote:

+#define MAX31343_REG_TIMER_CFG	(0x05)
+#define  TIMER_CFG_TFS		GENMASK(1, 0) /* Timer frequency */
+#define  TIMER_CFG_TRPT		BIT(2) /* Timer repeat mode */
+#define  TIMER_CFG_TPAUSE	BIT(3) /* Timer Pause */
+#define  TIMER_CFG_TE		BIT(4) /* Timer enable */
+
+/* RTC section */
+#define MAX31343_REG_SEC	(0x06)
+#define  SEC10_MASK	GENMASK(6, 4) /* RTC seconds in multiples of 10 */
+#define  SEC_MASK	GENMASK(3, 0) /* RTC seconds value */

I'm not convinced having separate masks is useful here, was that
automatically generated?

+static int max31343_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct max31343_rtc_data *max31343 = dev_get_drvdata(dev);
+	u8 date[7];
+	int ret;
+
+	dev_dbg(dev, "RTC set time %04d-%02d-%02d %02d/%02d/%02d\n",
+		tm->tm_year + 1900, tm->tm_mon, tm->tm_mday,
+		tm->tm_hour, tm->tm_min, tm->tm_sec);
+

This could use %ptR

+	date[0] = bin2bcd(tm->tm_sec);
+	date[1] = bin2bcd(tm->tm_min);
+	date[2] = bin2bcd(tm->tm_hour);
+	date[3] = tm->tm_wday;
+	date[4] = bin2bcd(tm->tm_mday);
+	date[5] = bin2bcd(tm->tm_mon + 1);
+
+	if (tm->tm_year >= 200)
+		date[5] |= CENTURY;
+	date[6] = bin2bcd(tm->tm_year % 100);
+
+	ret = regmap_bulk_write(max31343->regmap, MAX31343_REG_SEC, date,
+				sizeof(date));
+	return ret;
+}
+

[...]

+static int
+max31343_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct max31343_rtc_data *max31343 = NULL;
+	int ret, status;
+	struct nvmem_config nvmem_cfg = {
+		.name = "max31343_nvram",
+		.word_size = 1,
+		.stride = 1,
+		.size = MAX31343_RAM_SIZE,
+		.type = NVMEM_TYPE_BATTERY_BACKED,
+		.reg_read = max31343_nvram_read,
+		.reg_write = max31343_nvram_write,
+	};
+
+	max31343 = devm_kzalloc(&client->dev, sizeof(struct max31343_rtc_data),
+				GFP_KERNEL);
+	if (!max31343)
+		return -ENOMEM;
+
+	max31343->regmap = devm_regmap_init_i2c(client, &max31343_regmap_config);
+	if (IS_ERR(max31343->regmap))
+		return PTR_ERR(max31343->regmap);
+
+	i2c_set_clientdata(client, max31343);
+
+	ret = regmap_read(max31343->regmap, MAX31343_REG_STATUS, &status);
+	if (ret < 0)
+		return ret;
+
+	max31343->rtc = devm_rtc_allocate_device(&client->dev);
+	if (IS_ERR(max31343->rtc))
+		return PTR_ERR(max31343->rtc);
+
+	max31343->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	max31343->rtc->range_max = RTC_TIMESTAMP_END_2199;

For my information, did you check the time continuity in this interval?

+	max31343->rtc->ops = &max31343_rtc_ops;
+	ret = devm_rtc_register_device(max31343->rtc);
+	if (ret)
+		return ret;
+
+	nvmem_cfg.priv = max31343->regmap;
+	devm_rtc_nvmem_register(max31343->rtc, &nvmem_cfg);
+	max31343_hwmon_register(&client->dev);

The whole driver seems ok, I'd like to get a review from the hwmon
maintainers on the hwmon part as it is quite large.

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