rtc: rs5c372: Add RTC_VL_READ, RTC_VL_CLR ioctls

PATCH Superseded Camel Guo 3 participants 7 messages Nov 10, 2021 – Nov 11, 2021 Real time clock (RTC)

lore

Share & tools

lore

Camel Guo

author

2021-11-10 11:55 UTC #0 lore raw

PATCH 1/1 rtc: rs5c372: Add RTC_VL_READ, RTC_VL_CLR ioctls

From: Camel Guo <[email protected]>

In order to make it possible to get battery voltage status, this commit adds RTC_VL_READ, RTC_VL_CLR ioctl commands to rtc-rs5c372.

Signed-off-by Camel Guo <[email protected]>

---
 drivers/rtc/rtc-rs5c372.c | 66 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

drivers/rtc/rtc-rs5c372.c +66 -0

index 80980414890c..5a96e5d3663a 100644
@@ -485,6 +485,71 @@ static int rs5c372_rtc_proc(struct device *dev, struct seq_file *seq)
 #define	rs5c372_rtc_proc	NULL
 #endif
 
+#ifdef CONFIG_RTC_INTF_DEV
+static int rs5c372_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct rs5c372	*rs5c = i2c_get_clientdata(to_i2c_client(dev));
+	unsigned char	ctrl2;
+	int		addr;
+	unsigned int	flags;
+
+	dev_dbg(dev, "%s: cmd=%x\n", __func__, cmd);
+
+	addr = RS5C_ADDR(RS5C_REG_CTRL2);
+	ctrl2 = i2c_smbus_read_byte_data(rs5c->client, addr);
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		flags = 0;
+
+		switch (rs5c->type) {
+		case rtc_r2025sd:
+		case rtc_r2221tl:
+			if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) ||
+				(rs5c->type == rtc_r2221tl &&  (ctrl2 & R2x2x_CTRL2_XSTP))) {
+				flags |= RTC_VL_DATA_INVALID;
+			}
+			if (ctrl2 & R2x2x_CTRL2_VDET)
+				flags |= RTC_VL_ACCURACY_LOW;
+			break;
+		default:
+			if (ctrl2 & RS5C_CTRL2_XSTP)
+				flags |= RTC_VL_DATA_INVALID;
+			break;
+		}
+
+		return put_user(flags, (unsigned int __user *)arg);
+	case RTC_VL_CLR:
+		/* clear rtc VDET, PON and XSTP bits */
+		switch (rs5c->type) {
+		case rtc_r2025sd:
+		case rtc_r2221tl:
+			ctrl2 &= ~(R2x2x_CTRL2_VDET | R2x2x_CTRL2_PON);
+			if (rs5c->type == rtc_r2025sd)
+				ctrl2 |= R2x2x_CTRL2_XSTP;
+			else
+				ctrl2 &= ~R2x2x_CTRL2_XSTP;
+			break;
+		default:
+			ctrl2 &= ~RS5C_CTRL2_XSTP;
+			break;
+		}
+
+		if (i2c_smbus_write_byte_data(rs5c->client, addr, ctrl2) < 0) {
+			dev_dbg(&rs5c->client->dev, "%s: write error in line %i\n",
+					__func__, __LINE__);
+			return -EIO;
+		}
+		return 0;
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}
+#else
+#define rs5c372_ioctl	NULL
+#endif
+
 static const struct rtc_class_ops rs5c372_rtc_ops = {
 	.proc		= rs5c372_rtc_proc,
 	.read_time	= rs5c372_rtc_read_time,

@@ -492,6 +557,7 @@ static const struct rtc_class_ops rs5c372_rtc_ops = {
 	.read_alarm	= rs5c_read_alarm,
 	.set_alarm	= rs5c_set_alarm,
 	.alarm_irq_enable = rs5c_rtc_alarm_irq_enable,
+	.ioctl		= rs5c372_ioctl,
 };
 
 #if IS_ENABLED(CONFIG_RTC_INTF_SYSFS)

Signature · 1 line

2.20.1

Alexandre Belloni

maintainer

2021-11-10 13:54 UTC #1 lore raw

Hello,

Quoted 34 lines · Camel Guo

From: Camel Guo <[email protected]>

In order to make it possible to get battery voltage status, this commit adds RTC_VL_READ, RTC_VL_CLR ioctl commands to rtc-rs5c372.

Signed-off-by Camel Guo <[email protected]>

---
 drivers/rtc/rtc-rs5c372.c | 66 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

drivers/rtc/rtc-rs5c372.c +16 -0

index 80980414890c..5a96e5d3663a 100644
@@ -485,6 +485,71 @@ static int rs5c372_rtc_proc(struct device *dev, struct seq_file *seq)
 #define	rs5c372_rtc_proc	NULL
 #endif
 
+#ifdef CONFIG_RTC_INTF_DEV
+static int rs5c372_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct rs5c372	*rs5c = i2c_get_clientdata(to_i2c_client(dev));
+	unsigned char	ctrl2;
+	int		addr;
+	unsigned int	flags;
+
+	dev_dbg(dev, "%s: cmd=%x\n", __func__, cmd);
+
+	addr = RS5C_ADDR(RS5C_REG_CTRL2);
+	ctrl2 = i2c_smbus_read_byte_data(rs5c->client, addr);
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		flags = 0;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_proc(struct device *dev, struct seq_file *seq) · line 513

Quoted 10 lines

drivers/rtc/rtc-rs5c372.c +10 -0

+
+		switch (rs5c->type) {
+		case rtc_r2025sd:
+		case rtc_r2221tl:
+			if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) ||
+				(rs5c->type == rtc_r2221tl &&  (ctrl2 & R2x2x_CTRL2_XSTP))) {
+				flags |= RTC_VL_DATA_INVALID;
+			}
+			if (ctrl2 & R2x2x_CTRL2_VDET)
+				flags |= RTC_VL_ACCURACY_LOW;

Shouldn't that be RTC_VL_BACKUP_LOW?

Quoted 11 lines

drivers/rtc/rtc-rs5c372.c +11 -0

+			break;
+		default:
+			if (ctrl2 & RS5C_CTRL2_XSTP)
+				flags |= RTC_VL_DATA_INVALID;
+			break;
+		}
+
+		return put_user(flags, (unsigned int __user *)arg);
+	case RTC_VL_CLR:
+		/* clear rtc VDET, PON and XSTP bits */
+		switch (rs5c->type) {

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_proc(struct device *dev, struct seq_file *seq) · line 534

Quoted 10 lines

drivers/rtc/rtc-rs5c372.c +10 -0

+		case rtc_r2025sd:
+		case rtc_r2221tl:
+			ctrl2 &= ~(R2x2x_CTRL2_VDET | R2x2x_CTRL2_PON);
+			if (rs5c->type == rtc_r2025sd)
+				ctrl2 |= R2x2x_CTRL2_XSTP;
+			else
+				ctrl2 &= ~R2x2x_CTRL2_XSTP;
+			break;
+		default:
+			ctrl2 &= ~RS5C_CTRL2_XSTP;

You can clear VDET but you must nt clear PON or XSTP as they are used to know whether the time on the RTC has been set correctly. Clearing those can only be done in .set_time.

Quoted 32 lines

drivers/rtc/rtc-rs5c372.c +19 -0

+			break;
+		}
+
+		if (i2c_smbus_write_byte_data(rs5c->client, addr, ctrl2) < 0) {
+			dev_dbg(&rs5c->client->dev, "%s: write error in line %i\n",
+					__func__, __LINE__);
+			return -EIO;
+		}
+		return 0;
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}
+#else
+#define rs5c372_ioctl	NULL
+#endif
+
 static const struct rtc_class_ops rs5c372_rtc_ops = {
 	.proc		= rs5c372_rtc_proc,
 	.read_time	= rs5c372_rtc_read_time,

@@ -492,6 +557,7 @@ static const struct rtc_class_ops rs5c372_rtc_ops = {
 	.read_alarm	= rs5c_read_alarm,
 	.set_alarm	= rs5c_set_alarm,
 	.alarm_irq_enable = rs5c_rtc_alarm_irq_enable,
+	.ioctl		= rs5c372_ioctl,
 };
 
 #if IS_ENABLED(CONFIG_RTC_INTF_SYSFS)

Signature · 1 line

2.20.1

Signature · 3 lines

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

Some drivers (e.g: rv3029_ioctl and rv8803_ioctl) use RTC_VL_ACCURACY_LOW, but some other drivers (e.g: abx80x_ioctl, pcf2127_rtc_ioctl and pcf8523_rtc_ioctl) use RTC_VL_BACKUP_LOW instead. Is there any guideline or document telling the differences between them?

I can change it to RTC_VL_BACKUP_LOW of course.

Quoted 23 lines

Quoted 12 lines

drivers/rtc/rtc-rs5c372.c +11 -0

+                     break;
+             default:
+                     if (ctrl2 & RS5C_CTRL2_XSTP)
+                             flags |= RTC_VL_DATA_INVALID;
+                     break;
+             }
+
+             return put_user(flags, (unsigned int __user *)arg);
+     case RTC_VL_CLR:
+             /* clear rtc VDET, PON and XSTP bits */
+             switch (rs5c->type) {

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_proc(struct device *dev, struct seq_file *seq) · line 534

Quoted 10 lines

drivers/rtc/rtc-rs5c372.c +10 -0

+             case rtc_r2025sd:
+             case rtc_r2221tl:
+                     ctrl2 &= ~(R2x2x_CTRL2_VDET | R2x2x_CTRL2_PON);
+                     if (rs5c->type == rtc_r2025sd)
+                             ctrl2 |= R2x2x_CTRL2_XSTP;
+                     else
+                             ctrl2 &= ~R2x2x_CTRL2_XSTP;
+                     break;
+             default:
+                     ctrl2 &= ~RS5C_CTRL2_XSTP;

Quoted 3 lines

You can clear VDET but you must nt clear PON or XSTP as they are used to know whether the time on the RTC has been set correctly. Clearing those can only be done in .set_time.

Will update shortly.

Quoted 38 lines

Quoted 33 lines

drivers/rtc/rtc-rs5c372.c +19 -0

+                     break;
+             }
+
+             if (i2c_smbus_write_byte_data(rs5c->client, addr, ctrl2) < 0) {
+                     dev_dbg(&rs5c->client->dev, "%s: write error in line %i\n",
+                                     __func__, __LINE__);
+                     return -EIO;
+             }
+             return 0;
+     default:
+             return -ENOIOCTLCMD;
+     }
+     return 0;
+}
+#else
+#define rs5c372_ioctl        NULL
+#endif
+
   static const struct rtc_class_ops rs5c372_rtc_ops = {
         .proc           = rs5c372_rtc_proc,
         .read_time      = rs5c372_rtc_read_time,

@@ -492,6 +557,7 @@ static const struct rtc_class_ops rs5c372_rtc_ops = {
         .read_alarm     = rs5c_read_alarm,
         .set_alarm      = rs5c_set_alarm,
         .alarm_irq_enable = rs5c_rtc_alarm_irq_enable,
+     .ioctl          = rs5c372_ioctl,
   };
 
   #if IS_ENABLED(CONFIG_RTC_INTF_SYSFS)

Signature · 1 line

2.20.1

Signature · 3 lines

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

Alexandre Belloni

maintainer

2021-11-10 14:27 UTC #3 lore raw

Quoted 16 lines · Camel Guo

Quoted 16 lines · Camel Guo
Quoted 14 lines
From: Camel Guo <[email protected]>
+     switch (cmd) {
+     case RTC_VL_READ:
+             flags = 0;
+
+             switch (rs5c->type) {
+             case rtc_r2025sd:
+             case rtc_r2221tl:
+                     if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) || +                             (rs5c->type == rtc_r2221tl && (ctrl2 & R2x2x_CTRL2_XSTP))) { +                             flags |= RTC_VL_DATA_INVALID;
+                     }
+                     if (ctrl2 & R2x2x_CTRL2_VDET)
+                             flags |= RTC_VL_ACCURACY_LOW;

Quoted 7 lines

Quoted 1 line
Shouldn't that be RTC_VL_BACKUP_LOW?
Some drivers (e.g: rv3029_ioctl and rv8803_ioctl) use RTC_VL_ACCURACY_LOW, but some other drivers (e.g: abx80x_ioctl, pcf2127_rtc_ioctl and pcf8523_rtc_ioctl) use RTC_VL_BACKUP_LOW instead. Is there any guideline or document telling the differences between them?

RTC_VL_BACKUP_LOW: The backup voltage is low
RTC_VL_ACCURACY_LOW: the primary or backup voltage is low, temperature compensation (or similar) has stopped

Signature · 3 lines

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

Camel Guo

2021-11-10 14:30 UTC #4 lore raw

Hello,

Quoted 25 lines · Alexandre Belloni

Quoted 16 lines · Camel Guo
Quoted 16 lines · Camel Guo
Quoted 14 lines
From: Camel Guo <[email protected]>
+     switch (cmd) {
+     case RTC_VL_READ:
+             flags = 0;
+
+             switch (rs5c->type) {
+             case rtc_r2025sd:
+             case rtc_r2221tl:
+                     if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) || +                             (rs5c->type == rtc_r2221tl && (ctrl2 & R2x2x_CTRL2_XSTP))) { +                             flags |= RTC_VL_DATA_INVALID;
+                     }
+                     if (ctrl2 & R2x2x_CTRL2_VDET)
+                             flags |= RTC_VL_ACCURACY_LOW;
Quoted 7 lines
Quoted 1 line
Shouldn't that be RTC_VL_BACKUP_LOW?
Some drivers (e.g: rv3029_ioctl and rv8803_ioctl) use RTC_VL_ACCURACY_LOW, but some other drivers (e.g: abx80x_ioctl, pcf2127_rtc_ioctl and pcf8523_rtc_ioctl) use RTC_VL_BACKUP_LOW instead. Is there any guideline or document telling the differences between them?

Quoted 3 lines

RTC_VL_BACKUP_LOW: The backup voltage is low
RTC_VL_ACCURACY_LOW: the primary or backup voltage is low, temperature compensation (or similar) has stopped

Then I agree that we should go for RTC_VL_BACKUP_LOW.

Quoted 5 lines

Signature · 4 lines

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

Camel Guo

2021-11-10 15:01 UTC #5 lore raw

Patch V2 has been uploaded. Please review patch v2 instead.

Quoted 39 lines · Camel Guo

Hello,
Quoted 25 lines · Alexandre Belloni
Quoted 16 lines · Camel Guo
Quoted 16 lines · Camel Guo
Quoted 14 lines
From: Camel Guo <[email protected]>
+     switch (cmd) {
+     case RTC_VL_READ:
+             flags = 0;
+
+             switch (rs5c->type) {
+             case rtc_r2025sd:
+             case rtc_r2221tl:
+                     if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) || +                             (rs5c->type == rtc_r2221tl && (ctrl2 & R2x2x_CTRL2_XSTP))) { +                             flags |= RTC_VL_DATA_INVALID;
+                     }
+                     if (ctrl2 & R2x2x_CTRL2_VDET)
+                             flags |= RTC_VL_ACCURACY_LOW;
Quoted 7 lines
Quoted 1 line
Shouldn't that be RTC_VL_BACKUP_LOW?
Some drivers (e.g: rv3029_ioctl and rv8803_ioctl) use RTC_VL_ACCURACY_LOW, but some other drivers (e.g: abx80x_ioctl, pcf2127_rtc_ioctl and pcf8523_rtc_ioctl) use RTC_VL_BACKUP_LOW instead. Is there any guideline or document telling the differences between them?
Quoted 3 lines
RTC_VL_BACKUP_LOW: The backup voltage is low
RTC_VL_ACCURACY_LOW: the primary or backup voltage is low, temperature compensation (or similar) has stopped
Then I agree that we should go for RTC_VL_BACKUP_LOW.
Quoted 5 lines
Signature · 4 lines
-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Camel Guo

2021-11-11 08:38 UTC #6 lore raw

In reply to Camel Guo

Quoted 21 lines · Camel Guo

Hello,
Quoted 18 lines · Alexandre Belloni
Quoted 17 lines · Camel Guo
Quoted 17 lines · Camel Guo
Quoted 6 lines
From: Camel Guo <[email protected]>
+     switch (cmd) {
+     case RTC_VL_READ:
+             flags = 0;
+
+             switch (rs5c->type) {
Quoted 8 lines
+             case rtc_r2025sd:
+             case rtc_r2221tl:
+                     if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) || +                             (rs5c->type == rtc_r2221tl && (ctrl2 & R2x2x_CTRL2_XSTP))) { +                             flags |= RTC_VL_DATA_INVALID;
+                     }
+                     if (ctrl2 & R2x2x_CTRL2_VDET)
+                             flags |= RTC_VL_ACCURACY_LOW;
Shouldn't that be RTC_VL_BACKUP_LOW?

Fixed in V3.

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
Some drivers (e.g: rv3029_ioctl and rv8803_ioctl) use RTC_VL_ACCURACY_LOW, but some other drivers (e.g: abx80x_ioctl, pcf2127_rtc_ioctl and pcf8523_rtc_ioctl) use RTC_VL_BACKUP_LOW instead. Is there any guideline or document telling the differences between them?

Quoted 5 lines

Quoted 3 lines
RTC_VL_BACKUP_LOW: The backup voltage is low
RTC_VL_ACCURACY_LOW: the primary or backup voltage is low, temperature compensation (or similar) has stopped
Then I agree that we should go for RTC_VL_BACKUP_LOW.

Fixed in V3.

Quoted 6 lines

Signature · 4 lines

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

All comments are fixed in V3, please review it again.