From mboxrd@z Thu Jan 1 00:00:00 1970 From: arno@natisbad.org (Arnaud Ebalard) Date: Fri, 07 Nov 2014 00:34:41 +0100 Subject: [PATCHv0 1/3] rtc: rtc-isl12057: fix masking of register values References: <917b6e5e869fce697dec8a1597dfd2f671bf5c46.1415222752.git.arno@natisbad.org> <20141106082946.GN8316@pengutronix.de> Message-ID: <87ioirnbse.fsf@natisbad.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, Uwe Kleine-K?nig writes: > Hello Arnaud, > > On Wed, Nov 05, 2014 at 10:42:25PM +0100, Arnaud Ebalard wrote: >> When Intersil ISL12057 support was added by commit 70e123373c05 ("rtc: >> Add support for Intersil ISL12057 I2C RTC chip"), two masks for time >> registers values imported from the device were either wrong or >> omitted, leading to additional bits from those registers to impact >> read values: >> >> - mask for hour register value when reading it in AM/PM mode. As >> AM/PM mode is not the usual mode used by the driver, this error >> would only have an impact on an externally configured RTC hour >> later read by the driver. >> - mask for month value. The lack of masking would provide an >> erroneous value if century bit is set. >> >> This patch fixes those two masks. >> >> Fixes: 70e123373c05 ("rtc: Add support for Intersil ISL12057 I2C RTC chip") >> Signed-off-by: Arnaud Ebalard >> --- >> drivers/rtc/rtc-isl12057.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c >> index 455b601d731d..8132fbc7e10a 100644 >> --- a/drivers/rtc/rtc-isl12057.c >> +++ b/drivers/rtc/rtc-isl12057.c >> @@ -88,7 +88,7 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs) >> tm->tm_min = bcd2bin(regs[ISL12057_REG_RTC_MN]); >> >> if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_MIL) { /* AM/PM */ >> - tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x0f); >> + tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x1f); >> if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_PM) >> tm->tm_hour += 12; >> } else { /* 24 hour mode */ >> @@ -96,8 +96,8 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs) >> } >> >> tm->tm_mday = bcd2bin(regs[ISL12057_REG_RTC_DT]); >> - tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts at 1 */ >> + tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts at 1 */ > unrelated change? yep, will remove it. >> - tm->tm_mon = bcd2bin(regs[ISL12057_REG_RTC_MO]) - 1; /* starts at 1 */ >> + tm->tm_mon = bcd2bin(regs[ISL12057_REG_RTC_MO] & 0x1f) - 1; /* ditto */ >> tm->tm_year = bcd2bin(regs[ISL12057_REG_RTC_YR]) + 100; >> } > This patch definitly improves the driver, still I see room for more > improvement (all but the first issue are unrelated to this patch): > > - There is a century bit in the RTC_MO driver that is now correctly > masked to determine the month. Shouldn't it be used to flag if year > is >= 2100? That seems to match the hardware leap year handling. > (I.e. it considers RTC_YR=0 not to be a leap year iff RTC_MO_CENTURY > is set.) > > - I wonder how tm_wday should be handled. It seems to be hardly used by > callers of rtc_read_time. Is it better to report the hardware state > here or the weekday matching year/month/day? > > - IMHO the probe function shouldn't clear the OSCILLATOR FAILURE BIT > (OSF). It's an indicator that the stored time is wrong. Instead this > bit should be evaluated in the read_time callback. (If it's set, > return -ENODATA.) And only clear the bit in .set_time when the > registers were updated. I think most drivers get that wrong. Thanks for your feedback. I will take a look at those after alarm support is ok. Cheers, a+ From mboxrd@z Thu Jan 1 00:00:00 1970 From: arno@natisbad.org (Arnaud Ebalard) Subject: Re: [PATCHv0 1/3] rtc: rtc-isl12057: fix masking of register values Date: Fri, 07 Nov 2014 00:34:41 +0100 Message-ID: <87ioirnbse.fsf@natisbad.org> References: <917b6e5e869fce697dec8a1597dfd2f671bf5c46.1415222752.git.arno@natisbad.org> <20141106082946.GN8316@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Sender: linux-doc-owner@vger.kernel.org To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Cc: Mark Rutland , Alessandro Zummo , Peter Huewe , Linus Walleij , Thierry Reding , Mark Brown , devicetree@vger.kernel.org, rtc-linux@googlegroups.com, Pawel Moll , Ian Campbell , Stephen Warren , linux-doc@vger.kernel.org, Rob Herring , Jason Gunthorpe , Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , linux-arm-kernel@lists.infradead.org, Rob Landley , Kumar Gala , Grant Likely , Guenter Roeck , Jason Cooper List-Id: devicetree@vger.kernel.org Hi, Uwe Kleine-K=C3=B6nig writes: > Hello Arnaud, > > On Wed, Nov 05, 2014 at 10:42:25PM +0100, Arnaud Ebalard wrote: >> When Intersil ISL12057 support was added by commit 70e123373c05 ("rt= c: >> Add support for Intersil ISL12057 I2C RTC chip"), two masks for time >> registers values imported from the device were either wrong or >> omitted, leading to additional bits from those registers to impact >> read values: >>=20 >> - mask for hour register value when reading it in AM/PM mode. As >> AM/PM mode is not the usual mode used by the driver, this error >> would only have an impact on an externally configured RTC hour >> later read by the driver. >> - mask for month value. The lack of masking would provide an >> erroneous value if century bit is set. >>=20 >> This patch fixes those two masks. >>=20 >> Fixes: 70e123373c05 ("rtc: Add support for Intersil ISL12057 I2C RTC= chip") >> Signed-off-by: Arnaud Ebalard >> --- >> drivers/rtc/rtc-isl12057.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >>=20 >> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c >> index 455b601d731d..8132fbc7e10a 100644 >> --- a/drivers/rtc/rtc-isl12057.c >> +++ b/drivers/rtc/rtc-isl12057.c >> @@ -88,7 +88,7 @@ static void isl12057_rtc_regs_to_tm(struct rtc_tim= e *tm, u8 *regs) >> tm->tm_min =3D bcd2bin(regs[ISL12057_REG_RTC_MN]); >> =20 >> if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_MIL) { /* AM/P= M */ >> - tm->tm_hour =3D bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x0f); >> + tm->tm_hour =3D bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x1f); >> if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_PM) >> tm->tm_hour +=3D 12; >> } else { /* 24 hour mode */ >> @@ -96,8 +96,8 @@ static void isl12057_rtc_regs_to_tm(struct rtc_tim= e *tm, u8 *regs) >> } >> =20 >> tm->tm_mday =3D bcd2bin(regs[ISL12057_REG_RTC_DT]); >> - tm->tm_wday =3D bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts = at 1 */ >> + tm->tm_wday =3D bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts= at 1 */ > unrelated change? yep, will remove it. >> - tm->tm_mon =3D bcd2bin(regs[ISL12057_REG_RTC_MO]) - 1; /* starts = at 1 */ >> + tm->tm_mon =3D bcd2bin(regs[ISL12057_REG_RTC_MO] & 0x1f) - 1; /* = ditto */ >> tm->tm_year =3D bcd2bin(regs[ISL12057_REG_RTC_YR]) + 100; >> } > This patch definitly improves the driver, still I see room for more > improvement (all but the first issue are unrelated to this patch): > > - There is a century bit in the RTC_MO driver that is now correctly > masked to determine the month. Shouldn't it be used to flag if yea= r > is >=3D 2100? That seems to match the hardware leap year handling. > (I.e. it considers RTC_YR=3D0 not to be a leap year iff RTC_MO_CEN= TURY > is set.) > > - I wonder how tm_wday should be handled. It seems to be hardly used= by > callers of rtc_read_time. Is it better to report the hardware stat= e > here or the weekday matching year/month/day? > > - IMHO the probe function shouldn't clear the OSCILLATOR FAILURE BIT > (OSF). It's an indicator that the stored time is wrong. Instead th= is > bit should be evaluated in the read_time callback. (If it's set, > return -ENODATA.) And only clear the bit in .set_time when the > registers were updated. I think most drivers get that wrong. Thanks for your feedback. I will take a look at those after alarm support is ok. Cheers, a+