[PATCH] rtc-pcf8523: Add low battery voltage support

[PATCH] rtc-pcf8523: Add low battery voltage support

[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 <jesper.nilsson@axis.com>
---
diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index be05a64..82a9895 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -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
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH] rtc-pcf8523: Add low battery voltage support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

On Wed, Dec 19, 2012 at 03:04:56PM +0100, Jesper Nilsson wrote:
> 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 <jesper.nilsson@axis.com>
> ---
> diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
> index be05a64..82a9895 100644
> --- a/drivers/rtc/rtc-pcf8523.c
> +++ b/drivers/rtc/rtc-pcf8523.c
> @@ -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.

>  
>  #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;
> +}
> +
> +

That's one blank line too much.

>  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.

> +	.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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] rtc-pcf8523: Add low battery voltage support

[Jesper Nilsson]

On Wed, Dec 19, 2012 at 03:42:26PM +0100, Thierry Reding wrote:
> On Wed, Dec 19, 2012 at 03:04:56PM +0100, Jesper Nilsson wrote:
> > 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 <jesper.nilsson@axis.com>
> > ---
> > diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
> > index be05a64..82a9895 100644
> > --- a/drivers/rtc/rtc-pcf8523.c
> > +++ b/drivers/rtc/rtc-pcf8523.c
> > @@ -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.

> >  #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;
> > +}
> > +
> > +
> 
> That's one blank line too much.

Will fix.

> >  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. :-)

> > +	.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.

> Thierry

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH] rtc-pcf8523: Add low battery voltage support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Wed, Dec 19, 2012 at 04:10:54PM +0100, Jesper Nilsson wrote:
> On Wed, Dec 19, 2012 at 03:42:26PM +0100, Thierry Reding wrote:
> > On Wed, Dec 19, 2012 at 03:04:56PM +0100, Jesper Nilsson wrote:
[...]
> > > +	.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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] rtc-pcf8523: Add low battery voltage support

[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 <jesper.nilsson@axis.com>
---
diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index be05a64..62905fb 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -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
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH v2] rtc-pcf8523: Add low battery voltage support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On Wed, Dec 19, 2012 at 04:34:32PM +0100, Jesper Nilsson wrote:
> 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 <jesper.nilsson@axis.com>
> ---
> diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
> index be05a64..62905fb 100644
> --- a/drivers/rtc/rtc-pcf8523.c
> +++ b/drivers/rtc/rtc-pcf8523.c
> @@ -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)

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)
{
	...
}

> +{
> +	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 <thierry.reding@avionic-design.de>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3] rtc-pcf8523: Add low battery voltage support

[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 <jesper.nilsson@axis.com>
Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
---
diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index be05a64..acede49 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -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
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH v3] rtc-pcf8523: Add low battery voltage support

[Andrew Morton]

On Wed, 19 Dec 2012 17:00:33 +0100
Jesper Nilsson <jesper.nilsson@axis.com> wrote:

> 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.
> 
> ...
>
> --- a/drivers/rtc/rtc-pcf8523.c
> +++ b/drivers/rtc/rtc-pcf8523.c
> @@ -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,
>  };

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

--- a/drivers/rtc/rtc-pcf8523.c~rtc-pcf8523-add-low-battery-voltage-support-fix
+++ a/drivers/rtc/rtc-pcf8523.c
@@ -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,
_