rtc-pcf8523: Add low battery voltage support

PATCH Superseded Jesper Nilsson 3 participants 8 messages Dec 19, 2012 Real time clock (RTC)

lore

Share & tools

lore

Jesper Nilsson

author

2012-12-19 14:05 UTC #0 lore raw

PATCH 1/1 rtc-pcf8523: Add low battery voltage support

This patch implements reading of the battery voltage low signal for rtc-pcf8523.

The bit is read-only and cannot be cleared by software, so no clear-function is implemented.

Signed-off-by Jesper Nilsson <[email protected]>

---

drivers/rtc/rtc-pcf8523.c +23 -2

index be05a64..82a9895 100644
@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* Battery low bit, read-only */
 
 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

@@ -250,9 +252,29 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int pcf8523_rtc_read_vl(struct device *dev, int *vl)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int err;
+
+	err = pcf8523_read(client, REG_CONTROL3, &value);
+	if (err < 0)
+		return err;
+
+	if (value & REG_CONTROL3_BLF)
+		*vl = 1;
+	else
+		*vl = 0;
+
+	return 0;
+}
+
+
 static const struct rtc_class_ops pcf8523_rtc_ops = {
-	.read_time = pcf8523_rtc_read_time,
-	.set_time = pcf8523_rtc_set_time,
+	.read_time	= pcf8523_rtc_read_time,
+	.set_time	= pcf8523_rtc_set_time,
+	.read_vl	= pcf8523_rtc_read_vl,
 };
 
 static int pcf8523_probe(struct i2c_client *client,

/^JN - Jesper Nilsson

Signature · 1 line

               Jesper Nilsson -- [email protected]

Thierry Reding

2012-12-19 14:42 UTC #1 lore raw

Quoted 13 lines · Jesper Nilsson

This patch implements reading of the battery voltage low signal for rtc-pcf8523.
The bit is read-only and cannot be cleared by software, so no clear-function is implemented.
Signed-off-by Jesper Nilsson <[email protected]>
---
drivers/rtc/rtc-pcf8523.c +0 -0
index be05a64..82a9895 100644

drivers/rtc/rtc-pcf8523.c line 26

Quoted 4 lines

drivers/rtc/rtc-pcf8523.c +1 -0

@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* Battery low bit, read-only */

Nit: "battery" since you don't have a full sentence.

Quoted 16 lines

drivers/rtc/rtc-pcf8523.c +9 -0

 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

@@ -250,9 +252,29 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int pcf8523_rtc_read_vl(struct device *dev, int *vl)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int err;
+
+	err = pcf8523_read(client, REG_CONTROL3, &value);
+	if (err < 0)
+		return err;

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 273

Quoted 10 lines

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

+
+	if (value & REG_CONTROL3_BLF)
+		*vl = 1;
+	else
+		*vl = 0;
+
+	return 0;
+}
+
+

That's one blank line too much.

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 276

Quoted 5 lines

drivers/rtc/rtc-pcf8523.c +2 -2

 static const struct rtc_class_ops pcf8523_rtc_ops = {
-	.read_time = pcf8523_rtc_read_time,
-	.set_time = pcf8523_rtc_set_time,
+	.read_time	= pcf8523_rtc_read_time,
+	.set_time	= pcf8523_rtc_set_time,

Maybe you shouldn't reindent these, but rather adopt the existing style instead.

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 277

Quoted 1 line

drivers/rtc/rtc-pcf8523.c +1 -0
+	.read_vl	= pcf8523_rtc_read_vl,

What tree is this based on? None of the trees I have contains .read_vl in rtc_class_ops.

Thierry

Jesper Nilsson

author

2012-12-19 15:11 UTC #2 lore raw

Quoted 7 lines · Thierry Reding

Quoted 6 lines · Jesper Nilsson
This patch implements reading of the battery voltage low signal for rtc-pcf8523.
The bit is read-only and cannot be cleared by software, so no clear-function is implemented.

Quoted 13 lines

drivers/rtc/rtc-pcf8523.c line 26
Quoted 11 lines
Signed-off-by Jesper Nilsson <[email protected]>
---
drivers/rtc/rtc-pcf8523.c +1 -0
index be05a64..82a9895 100644
@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* Battery low bit, read-only */
Nit: "battery" since you don't have a full sentence.

Right.

Quoted 19 lines

drivers/rtc/rtc-pcf8523.c +13 -0

 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

@@ -250,9 +252,29 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int pcf8523_rtc_read_vl(struct device *dev, int *vl)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int err;
+
+	err = pcf8523_read(client, REG_CONTROL3, &value);
+	if (err < 0)
+		return err;
+
+	if (value & REG_CONTROL3_BLF)
+		*vl = 1;
+	else

Quoted 8 lines

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 273
Quoted 6 lines
drivers/rtc/rtc-pcf8523.c +6 -0
+		*vl = 0;
+
+	return 0;
+}
+
+
That's one blank line too much.

Will fix.

Quoted 8 lines

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 276
Quoted 5 lines
drivers/rtc/rtc-pcf8523.c +2 -2
 static const struct rtc_class_ops pcf8523_rtc_ops = {
-	.read_time = pcf8523_rtc_read_time,
-	.set_time = pcf8523_rtc_set_time,
+	.read_time	= pcf8523_rtc_read_time,
+	.set_time	= pcf8523_rtc_set_time,
Maybe you shouldn't reindent these, but rather adopt the existing style instead.

Ok. :-)

Quoted 4 lines

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 277
Quoted 1 line
drivers/rtc/rtc-pcf8523.c +1 -0
+	.read_vl	= pcf8523_rtc_read_vl,
What tree is this based on? None of the trees I have contains .read_vl in rtc_class_ops.

Hm, I've tested this against a local 3.4 kernel since that is what my device had, I didn't realize that we already had some local changes for the voltage low stuff.

I'm guessing the right way is to do it as the pcf8563 in the mainline kernel, I'll respin my patch and resend.

Quoted 1 line

Thierry

/^JN - Jesper Nilsson

Signature · 1 line

               Jesper Nilsson -- [email protected]

Thierry Reding

2012-12-19 15:19 UTC #3 lore raw

Quoted 2 lines · Jesper Nilsson

Quoted 1 line · Thierry Reding
On Wed, Dec 19, 2012 at 03:04:56PM +0100, Jesper Nilsson wrote:

[...]

Quoted 11 lines

Quoted 4 lines
drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 277
Quoted 1 line
drivers/rtc/rtc-pcf8523.c +1 -0
+	.read_vl	= pcf8523_rtc_read_vl,
What tree is this based on? None of the trees I have contains .read_vl in rtc_class_ops.
Hm, I've tested this against a local 3.4 kernel since that is what my device had, I didn't realize that we already had some local changes for the voltage low stuff.
I'm guessing the right way is to do it as the pcf8563 in the mainline kernel, I'll respin my patch and resend.

From a quick search through the mainline kernel history, there's no trace of this field.

Thierry

Jesper Nilsson

author

2012-12-19 15:34 UTC #4 lore raw

PATCH v2 1/1 rtc-pcf8523: Add low battery voltage support

This patch implements reading of the battery voltage low signal for rtc-pcf8523.

The bit is read-only and cannot be cleared by software, so no clear-function is implemented.

Signed-off-by Jesper Nilsson <[email protected]>

---

drivers/rtc/rtc-pcf8523.c +28 -0

index be05a64..62905fb 100644
@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* battery low bit, read-only */
 
 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

@@ -250,9 +252,36 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int
+pcf8523_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int err;
+	int ret = 0;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		err = pcf8523_read(client, REG_CONTROL3, &value);
+		if (err < 0)
+			return err;
+
+		if (value & REG_CONTROL3_BLF)
+			ret = 1;
+
+		if (copy_to_user((void __user *)arg, &ret, sizeof(int)))
+			return -EFAULT;
+
+		return 0;
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
 static const struct rtc_class_ops pcf8523_rtc_ops = {
 	.read_time = pcf8523_rtc_read_time,
 	.set_time = pcf8523_rtc_set_time,
+	.ioctl = pcf8523_rtc_ioctl,
 };
 
 static int pcf8523_probe(struct i2c_client *client,

/^JN - Jesper Nilsson

Signature · 1 line

               Jesper Nilsson -- [email protected]

Thierry Reding

Reviewed-by on v2 1/1

2012-12-19 15:49 UTC #5 lore raw

Quoted 21 lines · Jesper Nilsson

This patch implements reading of the battery voltage low signal for rtc-pcf8523.
The bit is read-only and cannot be cleared by software, so no clear-function is implemented.
Signed-off-by Jesper Nilsson <[email protected]>
---
drivers/rtc/rtc-pcf8523.c +1 -0
index be05a64..62905fb 100644
@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* battery low bit, read-only */
 
 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 256

Quoted 5 lines

drivers/rtc/rtc-pcf8523.c +2 -0

@@ -250,9 +252,36 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int
+pcf8523_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)

You should keep the modifier and return type on the same line as the function name and wrap the arguments instead. That is:

static int pcf8523_rtc_ioctl(struct device *dev, unsigned int cmd,
unsigned long arg)

{
	...
}

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 261

Quoted 5 lines

drivers/rtc/rtc-pcf8523.c +5 -0

+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int err;
+	int ret = 0;

You can probably collapse the two integers in one line, like so:

	int ret = 0, err;

Other than that, looks good:

Reviewed-by Thierry Reding <[email protected]>

Jesper Nilsson

author

2012-12-19 16:00 UTC #6 lore raw

PATCH v3 1/1 rtc-pcf8523: Add low battery voltage support

This patch implements reading of the battery voltage low signal for rtc-pcf8523.

The bit is read-only and cannot be cleared by software, so no clear-function is implemented.

Signed-off-by Jesper Nilsson <[email protected]>

Reviewed-by Thierry Reding <[email protected]>

---

drivers/rtc/rtc-pcf8523.c +27 -0

index be05a64..acede49 100644
@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* battery low bit, read-only */
 
 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

@@ -250,9 +252,35 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int pcf8523_rtc_ioctl(struct device *dev, unsigned int cmd,
+			     unsigned long arg)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int ret = 0, err;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		err = pcf8523_read(client, REG_CONTROL3, &value);
+		if (err < 0)
+			return err;
+
+		if (value & REG_CONTROL3_BLF)
+			ret = 1;
+
+		if (copy_to_user((void __user *)arg, &ret, sizeof(int)))
+			return -EFAULT;
+
+		return 0;
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
 static const struct rtc_class_ops pcf8523_rtc_ops = {
 	.read_time = pcf8523_rtc_read_time,
 	.set_time = pcf8523_rtc_set_time,
+	.ioctl = pcf8523_rtc_ioctl,
 };
 
 static int pcf8523_probe(struct i2c_client *client,

/^JN - Jesper Nilsson

Signature · 1 line

               Jesper Nilsson -- [email protected]

Andrew Morton

2012-12-19 22:04 UTC #7 lore raw

Quoted 42 lines · On Wed, 19 Dec 2012 17:00:33 +0100 Jesper Nilsson <[email protected]>…

This patch implements reading of the battery voltage low signal for rtc-pcf8523.

The bit is read-only and cannot be cleared by software, so no clear-function is implemented.

...

drivers/rtc/rtc-pcf8523.c +21 -0

@@ -23,6 +23,7 @@
 #define REG_CONTROL3_PM_VDD (1 << 6) /* switch-over disabled */
 #define REG_CONTROL3_PM_DSM (1 << 5) /* direct switching mode */
 #define REG_CONTROL3_PM_MASK 0xe0
+#define REG_CONTROL3_BLF (1 << 2) /* battery low bit, read-only */
 
 #define REG_SECONDS  0x03
 #define REG_SECONDS_OS (1 << 7)

@@ -250,9 +252,35 @@ static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8523_start_rtc(client);
 }
 
+static int pcf8523_rtc_ioctl(struct device *dev, unsigned int cmd,
+			     unsigned long arg)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 value;
+	int ret = 0, err;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		err = pcf8523_read(client, REG_CONTROL3, &value);
+		if (err < 0)
+			return err;
+
+		if (value & REG_CONTROL3_BLF)
+			ret = 1;
+
+		if (copy_to_user((void __user *)arg, &ret, sizeof(int)))
+			return -EFAULT;
+
+		return 0;

drivers/rtc/rtc-pcf8523.c static int pcf8523_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 284

Quoted 10 lines

drivers/rtc/rtc-pcf8523.c +6 -0

+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
 static const struct rtc_class_ops pcf8523_rtc_ops = {
 	.read_time = pcf8523_rtc_read_time,
 	.set_time = pcf8523_rtc_set_time,
+	.ioctl = pcf8523_rtc_ioctl,
 };

Other drivers make the ioctl conditional on CONFIG_RTC_INTF_DEV, presumably to avoid bloat.

drivers/rtc/rtc-pcf8523.c renamed from drivers/rtc/rtc-pcf8523.c~rtc-pcf8523-add-low-battery-voltage-support-fix +4 -0

@@ -251,6 +251,7 @@ static int pcf8523_rtc_set_time(struct d
 	return pcf8523_start_rtc(client);
 }
 
+#ifdef CONFIG_RTC_INTF_DEV
 static int pcf8523_rtc_ioctl(struct device *dev, unsigned int cmd,
 			     unsigned long arg)
 {

@@ -275,6 +276,9 @@ static int pcf8523_rtc_ioctl(struct devi
 		return -ENOIOCTLCMD;
 	}
 }
+#else
+#define pcf8523_rtc_ioctl NULL
+#endif
 
 static const struct rtc_class_ops pcf8523_rtc_ops = {
 	.read_time = pcf8523_rtc_read_time,