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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121219151054.GB14217@axis.com>
Date:	Wed, 19 Dec 2012 16:10:54 +0100
From:	Jesper Nilsson <jesper.nilsson@...s.com>
To:	Thierry Reding <thierry.reding@...onic-design.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	rtc-linux@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rtc-pcf8523: Add low battery voltage support

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@...s.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@...s.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/