[PATCH 0/8] rtc: rx8010: use regmap instead of i2c smbus API

[PATCH 0/8] rtc: rx8010: use regmap instead of i2c smbus API

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I want to use this driver on a platform where the i2c controller doesn't
speak SMBUS. This series converts the driver to i2c regmap which can
figure out the correct protocol to use.

First 7 patches are just cleanups and some refactoring. The actual
conversion happens in patch 8.

Bartosz Golaszewski (8):
  rtc: rx8010: remove unnecessary parentheses
  rtc: rx8010: consolidate local variables of the same type
  rtc: rx8010: use tabs for instead of spaces for code formatting
  rtc: rx8010: rename ret to err in rx8010_set_time()
  rtc: rx8010: don't use magic values for time buffer length
  rtc: rx8010: drop unnecessary initialization
  rtc: rx8010: fix indentation in probe()
  rtc: rx8010: convert to using regmap

 drivers/rtc/rtc-rx8010.c | 284 +++++++++++++++++----------------------
 1 file changed, 120 insertions(+), 164 deletions(-)

-- 
2.26.1

[PATCH 1/8] rtc: rx8010: remove unnecessary parentheses

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Remove parentheses whenever they guard a single line.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index fe010151ec8f..2faf5357a3a5 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -181,9 +181,8 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 		return ret;
 
 	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
-	if (flagreg < 0) {
+	if (flagreg < 0)
 		return flagreg;
-	}
 
 	if (flagreg & RX8010_FLAG_VLF)
 		ret = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG,
@@ -284,17 +283,15 @@ static int rx8010_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	int err;
 
 	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
-	if (flagreg < 0) {
+	if (flagreg < 0)
 		return flagreg;
-	}
 
 	if (rx8010->ctrlreg & (RX8010_CTRL_AIE | RX8010_CTRL_UIE)) {
 		rx8010->ctrlreg &= ~(RX8010_CTRL_AIE | RX8010_CTRL_UIE);
 		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
 						rx8010->ctrlreg);
-		if (err < 0) {
+		if (err < 0)
 			return err;
-		}
 	}
 
 	flagreg &= ~RX8010_FLAG_AF;
-- 
2.26.1

[PATCH 2/8] rtc: rx8010: consolidate local variables of the same type

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Move local variables of the same type into a single line for better
readability.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index 2faf5357a3a5..4c790d33f589 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -109,8 +109,7 @@ static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 date[7];
-	int flagreg;
-	int err;
+	int flagreg, err;
 
 	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
 	if (flagreg < 0)
@@ -141,8 +140,7 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 date[7];
-	int ctrl, flagreg;
-	int ret;
+	int ctrl, flagreg, ret;
 
 	if ((dt->tm_year < 100) || (dt->tm_year > 199))
 		return -EINVAL;
@@ -250,8 +248,7 @@ static int rx8010_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	struct i2c_client *client = rx8010->client;
 	u8 alarmvals[3];
-	int flagreg;
-	int err;
+	int flagreg, err;
 
 	err = i2c_smbus_read_i2c_block_data(client, RX8010_ALMIN, 3, alarmvals);
 	if (err != 3)
@@ -279,8 +276,7 @@ static int rx8010_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 alarmvals[3];
-	int extreg, flagreg;
-	int err;
+	int extreg, flagreg, err;
 
 	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
 	if (flagreg < 0)
@@ -346,9 +342,8 @@ static int rx8010_alarm_irq_enable(struct device *dev,
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	int flagreg;
+	int flagreg, err;
 	u8 ctrl;
-	int err;
 
 	ctrl = rx8010->ctrlreg;
 
@@ -387,8 +382,7 @@ static int rx8010_alarm_irq_enable(struct device *dev,
 static int rx8010_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	int tmp;
-	int flagreg;
+	int tmp, flagreg;
 
 	switch (cmd) {
 	case RTC_VL_READ:
-- 
2.26.1

[PATCH 3/8] rtc: rx8010: use tabs for instead of spaces for code formatting

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The define values in this driver are close to their names and they are
separated by spaces. Use tabs instead and align all defines.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 58 ++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index 4c790d33f589..2038700a3e8e 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -13,40 +13,40 @@
 #include <linux/module.h>
 #include <linux/rtc.h>
 
-#define RX8010_SEC     0x10
-#define RX8010_MIN     0x11
-#define RX8010_HOUR    0x12
-#define RX8010_WDAY    0x13
-#define RX8010_MDAY    0x14
-#define RX8010_MONTH   0x15
-#define RX8010_YEAR    0x16
-#define RX8010_RESV17  0x17
-#define RX8010_ALMIN   0x18
-#define RX8010_ALHOUR  0x19
-#define RX8010_ALWDAY  0x1A
-#define RX8010_TCOUNT0 0x1B
-#define RX8010_TCOUNT1 0x1C
-#define RX8010_EXT     0x1D
-#define RX8010_FLAG    0x1E
-#define RX8010_CTRL    0x1F
+#define RX8010_SEC		0x10
+#define RX8010_MIN		0x11
+#define RX8010_HOUR		0x12
+#define RX8010_WDAY		0x13
+#define RX8010_MDAY		0x14
+#define RX8010_MONTH		0x15
+#define RX8010_YEAR		0x16
+#define RX8010_RESV17		0x17
+#define RX8010_ALMIN		0x18
+#define RX8010_ALHOUR		0x19
+#define RX8010_ALWDAY		0x1A
+#define RX8010_TCOUNT0		0x1B
+#define RX8010_TCOUNT1		0x1C
+#define RX8010_EXT		0x1D
+#define RX8010_FLAG		0x1E
+#define RX8010_CTRL		0x1F
 /* 0x20 to 0x2F are user registers */
-#define RX8010_RESV30  0x30
-#define RX8010_RESV31  0x31
-#define RX8010_IRQ     0x32
+#define RX8010_RESV30		0x30
+#define RX8010_RESV31		0x31
+#define RX8010_IRQ		0x32
 
-#define RX8010_EXT_WADA  BIT(3)
+#define RX8010_EXT_WADA		BIT(3)
 
-#define RX8010_FLAG_VLF  BIT(1)
-#define RX8010_FLAG_AF   BIT(3)
-#define RX8010_FLAG_TF   BIT(4)
-#define RX8010_FLAG_UF   BIT(5)
+#define RX8010_FLAG_VLF		BIT(1)
+#define RX8010_FLAG_AF		BIT(3)
+#define RX8010_FLAG_TF		BIT(4)
+#define RX8010_FLAG_UF		BIT(5)
 
-#define RX8010_CTRL_AIE  BIT(3)
-#define RX8010_CTRL_UIE  BIT(5)
-#define RX8010_CTRL_STOP BIT(6)
-#define RX8010_CTRL_TEST BIT(7)
+#define RX8010_CTRL_AIE		BIT(3)
+#define RX8010_CTRL_UIE		BIT(5)
+#define RX8010_CTRL_STOP	BIT(6)
+#define RX8010_CTRL_TEST	BIT(7)
 
-#define RX8010_ALARM_AE  BIT(7)
+#define RX8010_ALARM_AE		BIT(7)
 
 static const struct i2c_device_id rx8010_id[] = {
 	{ "rx8010", 0 },
-- 
2.26.1

[PATCH 4/8] rtc: rx8010: rename ret to err in rx8010_set_time()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

All other functions in this driver use 'err' for integer return values.
Do the same in rx8010_set_time() for consistency.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index 2038700a3e8e..67ff06a76629 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -140,7 +140,7 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 date[7];
-	int ctrl, flagreg, ret;
+	int ctrl, flagreg, err;
 
 	if ((dt->tm_year < 100) || (dt->tm_year > 199))
 		return -EINVAL;
@@ -150,10 +150,10 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 	if (ctrl < 0)
 		return ctrl;
 	rx8010->ctrlreg = ctrl | RX8010_CTRL_STOP;
-	ret = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
+	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
 					rx8010->ctrlreg);
-	if (ret < 0)
-		return ret;
+	if (err < 0)
+		return err;
 
 	date[RX8010_SEC - RX8010_SEC] = bin2bcd(dt->tm_sec);
 	date[RX8010_MIN - RX8010_SEC] = bin2bcd(dt->tm_min);
@@ -163,27 +163,27 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 	date[RX8010_YEAR - RX8010_SEC] = bin2bcd(dt->tm_year - 100);
 	date[RX8010_WDAY - RX8010_SEC] = bin2bcd(1 << dt->tm_wday);
 
-	ret = i2c_smbus_write_i2c_block_data(rx8010->client,
+	err = i2c_smbus_write_i2c_block_data(rx8010->client,
 					     RX8010_SEC, 7, date);
-	if (ret < 0)
-		return ret;
+	if (err < 0)
+		return err;
 
 	/* clear STOP bit after changing clock/calendar */
 	ctrl = i2c_smbus_read_byte_data(rx8010->client, RX8010_CTRL);
 	if (ctrl < 0)
 		return ctrl;
 	rx8010->ctrlreg = ctrl & ~RX8010_CTRL_STOP;
-	ret = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
+	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
 					rx8010->ctrlreg);
-	if (ret < 0)
-		return ret;
+	if (err < 0)
+		return err;
 
 	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
 	if (flagreg < 0)
 		return flagreg;
 
 	if (flagreg & RX8010_FLAG_VLF)
-		ret = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG,
+		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG,
 						flagreg & ~RX8010_FLAG_VLF);
 
 	return 0;
-- 
2.26.1

[PATCH 5/8] rtc: rx8010: don't use magic values for time buffer length

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The time buffer len is used directly in this driver. For readability
it's better to define a constant.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index 67ff06a76629..f3bed7be2533 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -48,6 +48,8 @@
 
 #define RX8010_ALARM_AE		BIT(7)
 
+#define RX8010_TIME_BUF_LEN	7
+
 static const struct i2c_device_id rx8010_id[] = {
 	{ "rx8010", 0 },
 	{ }
@@ -108,7 +110,7 @@ static irqreturn_t rx8010_irq_1_handler(int irq, void *dev_id)
 static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	u8 date[7];
+	u8 date[RX8010_TIME_BUF_LEN];
 	int flagreg, err;
 
 	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
@@ -121,8 +123,8 @@ static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
 	}
 
 	err = i2c_smbus_read_i2c_block_data(rx8010->client, RX8010_SEC,
-					    7, date);
-	if (err != 7)
+					    RX8010_TIME_BUF_LEN, date);
+	if (err != RX8010_TIME_BUF_LEN)
 		return err < 0 ? err : -EIO;
 
 	dt->tm_sec = bcd2bin(date[RX8010_SEC - RX8010_SEC] & 0x7f);
@@ -139,7 +141,7 @@ static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
 static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	u8 date[7];
+	u8 date[RX8010_TIME_BUF_LEN];
 	int ctrl, flagreg, err;
 
 	if ((dt->tm_year < 100) || (dt->tm_year > 199))
@@ -164,7 +166,8 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 	date[RX8010_WDAY - RX8010_SEC] = bin2bcd(1 << dt->tm_wday);
 
 	err = i2c_smbus_write_i2c_block_data(rx8010->client,
-					     RX8010_SEC, 7, date);
+					     RX8010_SEC, RX8010_TIME_BUF_LEN,
+					     date);
 	if (err < 0)
 		return err;
 
-- 
2.26.1

[PATCH 6/8] rtc: rx8010: drop unnecessary initialization

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The 'err' local variable in rx8010_init_client() doesn't need to be
initialized.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index f3bed7be2533..181fc21cefa8 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -196,7 +196,7 @@ static int rx8010_init_client(struct i2c_client *client)
 {
 	struct rx8010_data *rx8010 = i2c_get_clientdata(client);
 	u8 ctrl[2];
-	int need_clear = 0, err = 0;
+	int need_clear = 0, err;
 
 	/* Initialize reserved registers as specified in datasheet */
 	err = i2c_smbus_write_byte_data(client, RX8010_RESV17, 0xD8);
-- 
2.26.1

[PATCH 7/8] rtc: rx8010: fix indentation in probe()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Align the arguments passed to devm_rtc_device_register() with the upper
line.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index 181fc21cefa8..ed8ba38b4991 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client,
 	}
 
 	rx8010->rtc = devm_rtc_device_register(&client->dev, client->name,
-		&rx8010_rtc_ops, THIS_MODULE);
+					       &rx8010_rtc_ops, THIS_MODULE);
 
 	if (IS_ERR(rx8010->rtc)) {
 		dev_err(&client->dev, "unable to register the class device\n");
-- 
2.26.1

[PATCH 8/8] rtc: rx8010: convert to using regmap

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This driver requires SMBUS to work. We can relax this requirement if we
switch to using i2c regmap and let the regmap sub-system figure out how
to talk to the bus.

This also has the advantage of shrinking the code for register updates.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 200 ++++++++++++++++-----------------------
 1 file changed, 81 insertions(+), 119 deletions(-)

diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
index ed8ba38b4991..9be3ea88e86d 100644
--- a/drivers/rtc/rtc-rx8010.c
+++ b/drivers/rtc/rtc-rx8010.c
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/rtc.h>
 
 #define RX8010_SEC		0x10
@@ -63,7 +64,7 @@ static const struct of_device_id rx8010_of_match[] = {
 MODULE_DEVICE_TABLE(of, rx8010_of_match);
 
 struct rx8010_data {
-	struct i2c_client *client;
+	struct regmap *regs;
 	struct rtc_device *rtc;
 	u8 ctrlreg;
 };
@@ -72,13 +73,12 @@ static irqreturn_t rx8010_irq_1_handler(int irq, void *dev_id)
 {
 	struct i2c_client *client = dev_id;
 	struct rx8010_data *rx8010 = i2c_get_clientdata(client);
-	int flagreg;
+	int flagreg, err;
 
 	mutex_lock(&rx8010->rtc->ops_lock);
 
-	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
-
-	if (flagreg <= 0) {
+	err = regmap_read(rx8010->regs, RX8010_FLAG, &flagreg);
+	if (err) {
 		mutex_unlock(&rx8010->rtc->ops_lock);
 		return IRQ_NONE;
 	}
@@ -101,10 +101,9 @@ static irqreturn_t rx8010_irq_1_handler(int irq, void *dev_id)
 		rtc_update_irq(rx8010->rtc, 1, RTC_UF | RTC_IRQF);
 	}
 
-	i2c_smbus_write_byte_data(client, RX8010_FLAG, flagreg);
-
+	err = regmap_write(rx8010->regs, RX8010_FLAG, flagreg);
 	mutex_unlock(&rx8010->rtc->ops_lock);
-	return IRQ_HANDLED;
+	return err ? IRQ_NONE : IRQ_HANDLED;
 }
 
 static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
@@ -113,19 +112,19 @@ static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
 	u8 date[RX8010_TIME_BUF_LEN];
 	int flagreg, err;
 
-	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
-	if (flagreg < 0)
-		return flagreg;
+	err = regmap_read(rx8010->regs, RX8010_FLAG, &flagreg);
+	if (err)
+		return err;
 
 	if (flagreg & RX8010_FLAG_VLF) {
 		dev_warn(dev, "Frequency stop detected\n");
 		return -EINVAL;
 	}
 
-	err = i2c_smbus_read_i2c_block_data(rx8010->client, RX8010_SEC,
-					    RX8010_TIME_BUF_LEN, date);
-	if (err != RX8010_TIME_BUF_LEN)
-		return err < 0 ? err : -EIO;
+	err = regmap_bulk_read(rx8010->regs, RX8010_SEC,
+			       date, RX8010_TIME_BUF_LEN);
+	if (err)
+		return err;
 
 	dt->tm_sec = bcd2bin(date[RX8010_SEC - RX8010_SEC] & 0x7f);
 	dt->tm_min = bcd2bin(date[RX8010_MIN - RX8010_SEC] & 0x7f);
@@ -142,19 +141,14 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 date[RX8010_TIME_BUF_LEN];
-	int ctrl, flagreg, err;
+	int err;
 
 	if ((dt->tm_year < 100) || (dt->tm_year > 199))
 		return -EINVAL;
 
 	/* set STOP bit before changing clock/calendar */
-	ctrl = i2c_smbus_read_byte_data(rx8010->client, RX8010_CTRL);
-	if (ctrl < 0)
-		return ctrl;
-	rx8010->ctrlreg = ctrl | RX8010_CTRL_STOP;
-	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
-					rx8010->ctrlreg);
-	if (err < 0)
+	err = regmap_set_bits(rx8010->regs, RX8010_CTRL, RX8010_CTRL_STOP);
+	if (err)
 		return err;
 
 	date[RX8010_SEC - RX8010_SEC] = bin2bcd(dt->tm_sec);
@@ -165,66 +159,55 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
 	date[RX8010_YEAR - RX8010_SEC] = bin2bcd(dt->tm_year - 100);
 	date[RX8010_WDAY - RX8010_SEC] = bin2bcd(1 << dt->tm_wday);
 
-	err = i2c_smbus_write_i2c_block_data(rx8010->client,
-					     RX8010_SEC, RX8010_TIME_BUF_LEN,
-					     date);
-	if (err < 0)
+	err = regmap_bulk_write(rx8010->regs, RX8010_SEC,
+				date, RX8010_TIME_BUF_LEN);
+	if (err)
 		return err;
 
 	/* clear STOP bit after changing clock/calendar */
-	ctrl = i2c_smbus_read_byte_data(rx8010->client, RX8010_CTRL);
-	if (ctrl < 0)
-		return ctrl;
-	rx8010->ctrlreg = ctrl & ~RX8010_CTRL_STOP;
-	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
-					rx8010->ctrlreg);
-	if (err < 0)
+	err = regmap_clear_bits(rx8010->regs, RX8010_CTRL, RX8010_CTRL_STOP);
+	if (err)
 		return err;
 
-	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
-	if (flagreg < 0)
-		return flagreg;
-
-	if (flagreg & RX8010_FLAG_VLF)
-		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG,
-						flagreg & ~RX8010_FLAG_VLF);
+	err = regmap_clear_bits(rx8010->regs, RX8010_FLAG, RX8010_FLAG_VLF);
+	if (err)
+		return err;
 
 	return 0;
 }
 
-static int rx8010_init_client(struct i2c_client *client)
+static int rx8010_init_client(struct device *dev)
 {
-	struct rx8010_data *rx8010 = i2c_get_clientdata(client);
+	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 ctrl[2];
 	int need_clear = 0, err;
 
 	/* Initialize reserved registers as specified in datasheet */
-	err = i2c_smbus_write_byte_data(client, RX8010_RESV17, 0xD8);
-	if (err < 0)
+	err = regmap_write(rx8010->regs, RX8010_RESV17, 0xD8);
+	if (err)
 		return err;
 
-	err = i2c_smbus_write_byte_data(client, RX8010_RESV30, 0x00);
-	if (err < 0)
+	err = regmap_write(rx8010->regs, RX8010_RESV30, 0x00);
+	if (err)
 		return err;
 
-	err = i2c_smbus_write_byte_data(client, RX8010_RESV31, 0x08);
-	if (err < 0)
+	err = regmap_write(rx8010->regs, RX8010_RESV31, 0x08);
+	if (err)
 		return err;
 
-	err = i2c_smbus_write_byte_data(client, RX8010_IRQ, 0x00);
-	if (err < 0)
+	err = regmap_write(rx8010->regs, RX8010_IRQ, 0x00);
+	if (err)
 		return err;
 
-	err = i2c_smbus_read_i2c_block_data(rx8010->client, RX8010_FLAG,
-					    2, ctrl);
-	if (err != 2)
-		return err < 0 ? err : -EIO;
+	err = regmap_bulk_read(rx8010->regs, RX8010_FLAG, ctrl, 2);
+	if (err)
+		return err;
 
 	if (ctrl[0] & RX8010_FLAG_VLF)
-		dev_warn(&client->dev, "Frequency stop was detected\n");
+		dev_warn(dev, "Frequency stop was detected\n");
 
 	if (ctrl[0] & RX8010_FLAG_AF) {
-		dev_warn(&client->dev, "Alarm was detected\n");
+		dev_warn(dev, "Alarm was detected\n");
 		need_clear = 1;
 	}
 
@@ -236,8 +219,8 @@ static int rx8010_init_client(struct i2c_client *client)
 
 	if (need_clear) {
 		ctrl[0] &= ~(RX8010_FLAG_AF | RX8010_FLAG_TF | RX8010_FLAG_UF);
-		err = i2c_smbus_write_byte_data(client, RX8010_FLAG, ctrl[0]);
-		if (err < 0)
+		err = regmap_write(rx8010->regs, RX8010_FLAG, ctrl[0]);
+		if (err)
 			return err;
 	}
 
@@ -249,17 +232,16 @@ static int rx8010_init_client(struct i2c_client *client)
 static int rx8010_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	struct i2c_client *client = rx8010->client;
 	u8 alarmvals[3];
 	int flagreg, err;
 
-	err = i2c_smbus_read_i2c_block_data(client, RX8010_ALMIN, 3, alarmvals);
-	if (err != 3)
-		return err < 0 ? err : -EIO;
+	err = regmap_bulk_read(rx8010->regs, RX8010_ALMIN, alarmvals, 3);
+	if (err)
+		return err;
 
-	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
-	if (flagreg < 0)
-		return flagreg;
+	err = regmap_read(rx8010->regs, RX8010_FLAG, &flagreg);
+	if (err)
+		return err;
 
 	t->time.tm_sec = 0;
 	t->time.tm_min = bcd2bin(alarmvals[0] & 0x7f);
@@ -276,52 +258,38 @@ static int rx8010_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 
 static int rx8010_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
 	u8 alarmvals[3];
-	int extreg, flagreg, err;
-
-	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
-	if (flagreg < 0)
-		return flagreg;
+	int err;
 
 	if (rx8010->ctrlreg & (RX8010_CTRL_AIE | RX8010_CTRL_UIE)) {
 		rx8010->ctrlreg &= ~(RX8010_CTRL_AIE | RX8010_CTRL_UIE);
-		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
-						rx8010->ctrlreg);
-		if (err < 0)
+		err = regmap_write(rx8010->regs, RX8010_CTRL, rx8010->ctrlreg);
+		if (err)
 			return err;
 	}
 
-	flagreg &= ~RX8010_FLAG_AF;
-	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG, flagreg);
-	if (err < 0)
+	err = regmap_clear_bits(rx8010->regs, RX8010_FLAG, RX8010_FLAG_AF);
+	if (err)
 		return err;
 
 	alarmvals[0] = bin2bcd(t->time.tm_min);
 	alarmvals[1] = bin2bcd(t->time.tm_hour);
 	alarmvals[2] = bin2bcd(t->time.tm_mday);
 
-	err = i2c_smbus_write_i2c_block_data(rx8010->client, RX8010_ALMIN,
-					     2, alarmvals);
-	if (err < 0)
+	err = regmap_bulk_write(rx8010->regs, RX8010_ALMIN, alarmvals, 2);
+	if (err)
 		return err;
 
-	extreg = i2c_smbus_read_byte_data(client, RX8010_EXT);
-	if (extreg < 0)
-		return extreg;
-
-	extreg |= RX8010_EXT_WADA;
-	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_EXT, extreg);
-	if (err < 0)
+	err = regmap_clear_bits(rx8010->regs, RX8010_EXT, RX8010_EXT_WADA);
+	if (err)
 		return err;
 
 	if (alarmvals[2] == 0)
 		alarmvals[2] |= RX8010_ALARM_AE;
 
-	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_ALWDAY,
-					alarmvals[2]);
-	if (err < 0)
+	err = regmap_write(rx8010->regs, RX8010_ALWDAY, alarmvals[2]);
+	if (err)
 		return err;
 
 	if (t->enabled) {
@@ -331,9 +299,8 @@ static int rx8010_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 			rx8010->ctrlreg |=
 				(RX8010_CTRL_AIE | RX8010_CTRL_UIE);
 
-		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
-						rx8010->ctrlreg);
-		if (err < 0)
+		err = regmap_write(rx8010->regs, RX8010_CTRL, rx8010->ctrlreg);
+		if (err)
 			return err;
 	}
 
@@ -343,9 +310,8 @@ static int rx8010_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 static int rx8010_alarm_irq_enable(struct device *dev,
 				   unsigned int enabled)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	int flagreg, err;
+	int err;
 	u8 ctrl;
 
 	ctrl = rx8010->ctrlreg;
@@ -362,20 +328,14 @@ static int rx8010_alarm_irq_enable(struct device *dev,
 			ctrl &= ~RX8010_CTRL_AIE;
 	}
 
-	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
-	if (flagreg < 0)
-		return flagreg;
-
-	flagreg &= ~RX8010_FLAG_AF;
-	err = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG, flagreg);
-	if (err < 0)
+	err = regmap_clear_bits(rx8010->regs, RX8010_FLAG, RX8010_FLAG_AF);
+	if (err)
 		return err;
 
 	if (ctrl != rx8010->ctrlreg) {
 		rx8010->ctrlreg = ctrl;
-		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
-						rx8010->ctrlreg);
-		if (err < 0)
+		err = regmap_write(rx8010->regs, RX8010_CTRL, rx8010->ctrlreg);
+		if (err)
 			return err;
 	}
 
@@ -385,13 +345,13 @@ static int rx8010_alarm_irq_enable(struct device *dev,
 static int rx8010_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 {
 	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
-	int tmp, flagreg;
+	int tmp, flagreg, err;
 
 	switch (cmd) {
 	case RTC_VL_READ:
-		flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
-		if (flagreg < 0)
-			return flagreg;
+		err = regmap_read(rx8010->regs, RX8010_FLAG, &flagreg);
+		if (err)
+			return err;
 
 		tmp = flagreg & RX8010_FLAG_VLF ? RTC_VL_DATA_INVALID : 0;
 		return put_user(tmp, (unsigned int __user *)arg);
@@ -407,28 +367,30 @@ static struct rtc_class_ops rx8010_rtc_ops = {
 	.ioctl = rx8010_ioctl,
 };
 
+static const struct regmap_config rx8010_regmap_config = {
+	.name = "rx8010-rtc",
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
 static int rx8010_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct i2c_adapter *adapter = client->adapter;
 	struct rx8010_data *rx8010;
 	int err = 0;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA
-		| I2C_FUNC_SMBUS_I2C_BLOCK)) {
-		dev_err(&adapter->dev, "doesn't support required functionality\n");
-		return -EIO;
-	}
-
 	rx8010 = devm_kzalloc(&client->dev, sizeof(struct rx8010_data),
 			      GFP_KERNEL);
 	if (!rx8010)
 		return -ENOMEM;
 
-	rx8010->client = client;
 	i2c_set_clientdata(client, rx8010);
 
-	err = rx8010_init_client(client);
+	rx8010->regs = devm_regmap_init_i2c(client, &rx8010_regmap_config);
+	if (IS_ERR(rx8010->regs))
+		return PTR_ERR(rx8010->regs);
+
+	err = rx8010_init_client(&client->dev);
 	if (err)
 		return err;
 
-- 
2.26.1

Re: [PATCH 1/8] rtc: rx8010: remove unnecessary parentheses

[Alexandre Belloni]

On 04/09/2020 17:21:09+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Remove parentheses whenever they guard a single line.

Those would be braces or curly brackets, not parentheses ;)

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/rtc/rtc-rx8010.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> index fe010151ec8f..2faf5357a3a5 100644
> --- a/drivers/rtc/rtc-rx8010.c
> +++ b/drivers/rtc/rtc-rx8010.c
> @@ -181,9 +181,8 @@ static int rx8010_set_time(struct device *dev, struct rtc_time *dt)
>  		return ret;
>  
>  	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
> -	if (flagreg < 0) {
> +	if (flagreg < 0)
>  		return flagreg;
> -	}
>  
>  	if (flagreg & RX8010_FLAG_VLF)
>  		ret = i2c_smbus_write_byte_data(rx8010->client, RX8010_FLAG,
> @@ -284,17 +283,15 @@ static int rx8010_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>  	int err;
>  
>  	flagreg = i2c_smbus_read_byte_data(client, RX8010_FLAG);
> -	if (flagreg < 0) {
> +	if (flagreg < 0)
>  		return flagreg;
> -	}
>  
>  	if (rx8010->ctrlreg & (RX8010_CTRL_AIE | RX8010_CTRL_UIE)) {
>  		rx8010->ctrlreg &= ~(RX8010_CTRL_AIE | RX8010_CTRL_UIE);
>  		err = i2c_smbus_write_byte_data(rx8010->client, RX8010_CTRL,
>  						rx8010->ctrlreg);
> -		if (err < 0) {
> +		if (err < 0)
>  			return err;
> -		}
>  	}
>  
>  	flagreg &= ~RX8010_FLAG_AF;
> -- 
> 2.26.1
> 

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

Re: [PATCH 5/8] rtc: rx8010: don't use magic values for time buffer length

[Alexandre Belloni]

On 04/09/2020 17:21:13+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The time buffer len is used directly in this driver. For readability
> it's better to define a constant.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/rtc/rtc-rx8010.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> index 67ff06a76629..f3bed7be2533 100644
> --- a/drivers/rtc/rtc-rx8010.c
> +++ b/drivers/rtc/rtc-rx8010.c
> @@ -48,6 +48,8 @@
>  
>  #define RX8010_ALARM_AE		BIT(7)
>  
> +#define RX8010_TIME_BUF_LEN	7
> +
>  static const struct i2c_device_id rx8010_id[] = {
>  	{ "rx8010", 0 },
>  	{ }
> @@ -108,7 +110,7 @@ static irqreturn_t rx8010_irq_1_handler(int irq, void *dev_id)
>  static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
>  {
>  	struct rx8010_data *rx8010 = dev_get_drvdata(dev);
> -	u8 date[7];

I'm usually fine with a magic value here...

> +	u8 date[RX8010_TIME_BUF_LEN];
>  	int flagreg, err;
>  
>  	flagreg = i2c_smbus_read_byte_data(rx8010->client, RX8010_FLAG);
> @@ -121,8 +123,8 @@ static int rx8010_get_time(struct device *dev, struct rtc_time *dt)
>  	}
>  
>  	err = i2c_smbus_read_i2c_block_data(rx8010->client, RX8010_SEC,
> -					    7, date);
> -	if (err != 7)
> +					    RX8010_TIME_BUF_LEN, date);

..as long as sizeof(date) is used here.

Even better, you could have date[RX8010_YEAR - RX8010_SEC + 1] and then
use sizeof. Or also have #define RX8010_TIME_BUF_LEN (RX8010_YEAR -
RX8010_SEC + 1) which would be safer than 7.

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

Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

[Alexandre Belloni]

On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Align the arguments passed to devm_rtc_device_register() with the upper
> line.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/rtc/rtc-rx8010.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> index 181fc21cefa8..ed8ba38b4991 100644
> --- a/drivers/rtc/rtc-rx8010.c
> +++ b/drivers/rtc/rtc-rx8010.c
> @@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client,
>  	}
>  
>  	rx8010->rtc = devm_rtc_device_register(&client->dev, client->name,
> -		&rx8010_rtc_ops, THIS_MODULE);
> +					       &rx8010_rtc_ops, THIS_MODULE);
>  

You have bonus points if you replace that patch by switching from
devm_rtc_device_register to devm_rtc_allocate_device and
rtc_register_device.

More bonus points if you also set range_min and range_max and then get
rid of the range checking in set_time.

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

Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Align the arguments passed to devm_rtc_device_register() with the upper
> > line.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  drivers/rtc/rtc-rx8010.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> > index 181fc21cefa8..ed8ba38b4991 100644
> > --- a/drivers/rtc/rtc-rx8010.c
> > +++ b/drivers/rtc/rtc-rx8010.c
> > @@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client,
> >       }
> >
> >       rx8010->rtc = devm_rtc_device_register(&client->dev, client->name,
> > -             &rx8010_rtc_ops, THIS_MODULE);
> > +                                            &rx8010_rtc_ops, THIS_MODULE);
> >
>
> You have bonus points if you replace that patch by switching from
> devm_rtc_device_register to devm_rtc_allocate_device and
> rtc_register_device.
>
> More bonus points if you also set range_min and range_max and then get
> rid of the range checking in set_time.
>

Hi Alexandre!

I've just looked at the code and wondered why there's no devm
counterpart for rtc_register_device(). Then I noticed that the release
callback for devm_rtc_allocate_device() takes care of unregistering
the device. This looks like serious devres abuse to me. In general the
idea is for the release callback to only undo whatever the devres
function did and this should be opaque to the concerned resources.

In this case I believe there's no need for the 'registered' field in
struct rtc_device - this structure should *not* care about this - and
there should be devm_rtc_register_device() whose release callback
would take care of the unregistering. Since this function would be
called after devm_rtc_allocate_device(), it would be released before
so the ordering should be fine.

Let me know your thoughts.

Best regards,
Bartosz Golaszewski

Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

[Alexandre Belloni]

On 07/09/2020 11:34:59+0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Align the arguments passed to devm_rtc_device_register() with the upper
> > > line.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  drivers/rtc/rtc-rx8010.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> > > index 181fc21cefa8..ed8ba38b4991 100644
> > > --- a/drivers/rtc/rtc-rx8010.c
> > > +++ b/drivers/rtc/rtc-rx8010.c
> > > @@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client,
> > >       }
> > >
> > >       rx8010->rtc = devm_rtc_device_register(&client->dev, client->name,
> > > -             &rx8010_rtc_ops, THIS_MODULE);
> > > +                                            &rx8010_rtc_ops, THIS_MODULE);
> > >
> >
> > You have bonus points if you replace that patch by switching from
> > devm_rtc_device_register to devm_rtc_allocate_device and
> > rtc_register_device.
> >
> > More bonus points if you also set range_min and range_max and then get
> > rid of the range checking in set_time.
> >
> 
> Hi Alexandre!
> 
> I've just looked at the code and wondered why there's no devm
> counterpart for rtc_register_device(). Then I noticed that the release
> callback for devm_rtc_allocate_device() takes care of unregistering
> the device. This looks like serious devres abuse to me. In general the
> idea is for the release callback to only undo whatever the devres
> function did and this should be opaque to the concerned resources.
> 
> In this case I believe there's no need for the 'registered' field in
> struct rtc_device - this structure should *not* care about this - and
> there should be devm_rtc_register_device() whose release callback
> would take care of the unregistering. Since this function would be
> called after devm_rtc_allocate_device(), it would be released before
> so the ordering should be fine.
> 

Note that the input subsystem is also doing it that way which is
probably not a good reason alone to do it like that. But, IIRC, there
was an actual reason this was done this way and it was the ordering of
the rtc_nvmem_register/rtc_nvmem_unregister with rtc_device_unregister.
I'm not sure this is still necessary though.

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

Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

[Bartosz Golaszewski]

On Fri, Sep 11, 2020 at 2:28 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 07/09/2020 11:34:59+0200, Bartosz Golaszewski wrote:
> > On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > >
> > > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > Align the arguments passed to devm_rtc_device_register() with the upper
> > > > line.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > ---
> > > >  drivers/rtc/rtc-rx8010.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> > > > index 181fc21cefa8..ed8ba38b4991 100644
> > > > --- a/drivers/rtc/rtc-rx8010.c
> > > > +++ b/drivers/rtc/rtc-rx8010.c
> > > > @@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client,
> > > >       }
> > > >
> > > >       rx8010->rtc = devm_rtc_device_register(&client->dev, client->name,
> > > > -             &rx8010_rtc_ops, THIS_MODULE);
> > > > +                                            &rx8010_rtc_ops, THIS_MODULE);
> > > >
> > >
> > > You have bonus points if you replace that patch by switching from
> > > devm_rtc_device_register to devm_rtc_allocate_device and
> > > rtc_register_device.
> > >
> > > More bonus points if you also set range_min and range_max and then get
> > > rid of the range checking in set_time.
> > >
> >
> > Hi Alexandre!
> >
> > I've just looked at the code and wondered why there's no devm
> > counterpart for rtc_register_device(). Then I noticed that the release
> > callback for devm_rtc_allocate_device() takes care of unregistering
> > the device. This looks like serious devres abuse to me. In general the
> > idea is for the release callback to only undo whatever the devres
> > function did and this should be opaque to the concerned resources.
> >
> > In this case I believe there's no need for the 'registered' field in
> > struct rtc_device - this structure should *not* care about this - and
> > there should be devm_rtc_register_device() whose release callback
> > would take care of the unregistering. Since this function would be
> > called after devm_rtc_allocate_device(), it would be released before
> > so the ordering should be fine.
> >
>
> Note that the input subsystem is also doing it that way which is
> probably not a good reason alone to do it like that. But, IIRC, there

I'm seeing this pattern elsewhere in the kernel too and I just
recently fixed this for MDIO. I think it's just a matter of people
copy-pasting a bad implementation.

> was an actual reason this was done this way and it was the ordering of
> the rtc_nvmem_register/rtc_nvmem_unregister with rtc_device_unregister.
> I'm not sure this is still necessary though.
>

To me - each of these should have their own 'unregister' function and
appropriate devres helpers *OR* RTC-related nvmem structures could be
set up and assigned to struct rtc_device after
devm_rtc_allocate_device() and picked up by the registration function
(and also undone by rtc_unregister_device()).

I'll try to allocate some time to look into this but it's not like
it's urgent or anything - it's just a potential improvement.

Bartosz

Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

[Alexandre Belloni]

On 11/09/2020 14:33:46+0200, Bartosz Golaszewski wrote:
> I'm seeing this pattern elsewhere in the kernel too and I just
> recently fixed this for MDIO. I think it's just a matter of people
> copy-pasting a bad implementation.
> 
> > was an actual reason this was done this way and it was the ordering of
> > the rtc_nvmem_register/rtc_nvmem_unregister with rtc_device_unregister.
> > I'm not sure this is still necessary though.
> >
> 
> To me - each of these should have their own 'unregister' function and
> appropriate devres helpers *OR* RTC-related nvmem structures could be
> set up and assigned to struct rtc_device after
> devm_rtc_allocate_device() and picked up by the registration function
> (and also undone by rtc_unregister_device()).
> 
> I'll try to allocate some time to look into this but it's not like
> it's urgent or anything - it's just a potential improvement.
> 

Well, this could simply be done by adding a devres_add in
__rtc_register_device. I'm planning to remove rtc_nvmem_unregister after
the next LTS which will make that even easier.

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