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

[Jesper Nilsson <jesper.nilsson@axis.com>]

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