From: Bartosz Golaszewski <[email protected]>
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
From: Bartosz Golaszewski <[email protected]>
Align the arguments passed to devm_rtc_device_register() with the upper
line.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
The 'err' local variable in rx8010_init_client() doesn't need to be
initialized.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Remove parentheses whenever they guard a single line.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Move local variables of the same type into a single line for better
readability.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
The time buffer len is used directly in this driver. For readability
it's better to define a constant.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
On 04/09/2020 17:21:09+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Remove parentheses whenever they guard a single line.
Those would be braces or curly brackets, not parentheses ;)
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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
On 04/09/2020 17:21:13+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> The time buffer len is used directly in this driver. For readability
> it's better to define a constant.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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
On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Align the arguments passed to devm_rtc_device_register() with the upper
> line.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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
On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
<[email protected]> wrote:
>
> On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Align the arguments passed to devm_rtc_device_register() with the upper
> > line.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > 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
On 07/09/2020 11:34:59+0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
> <[email protected]> wrote:
> >
> > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > Align the arguments passed to devm_rtc_device_register() with the upper
> > > line.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > ---
> > > 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
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
On Fri, Sep 11, 2020 at 2:28 PM Alexandre Belloni
<[email protected]> wrote:
>
> On 07/09/2020 11:34:59+0200, Bartosz Golaszewski wrote:
> > On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
> > <[email protected]> wrote:
> > >
> > > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <[email protected]>
> > > >
> > > > Align the arguments passed to devm_rtc_device_register() with the upper
> > > > line.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > > ---
> > > > 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