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]
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/

Powered by blists - more mailing lists