[PATCH 0/2] bq32000 fixes

[PATCH 0/2] bq32000 fixes

[Daniel Romell]

Hi all,

This is a couple of patches for the bq32000 RTC.

The first one is purely cosmetic, fixing a case where the wrong mask (but
correct value) was used when reading a register value.

The second patch adds a check for a failure flag in the read function. If the
register contents can't be trusted, an error is returned instead.
The patch also removes some code in the probe function that would clear the
failure flag without resolving the error condition. The error is now only
cleared when writing a new time to the RTC.

Jan Östlund (2):
  rtc: bq32k: Use correct mask name for 'minutes' register.
  rtc: bq32k: Fix handling of oscillator failure flag

 drivers/rtc/rtc-bq32k.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

--
2.7.4

[PATCH 1/2] rtc: bq32k: Use correct mask name for 'minutes' register.

[Daniel Romell]

From: Jan Östlund <jao@hms.se>

The BQ32K_SECONDS_MASK and BQ32K_MINUTES_MASK both has the same
value. This is no functional change.

Signed-off-by: Daniel Romell <daro@hms.se>
---
 drivers/rtc/rtc-bq32k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index 0299988..5a0c137 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -94,7 +94,7 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		return error;
 
 	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
-	tm->tm_min = bcd2bin(regs.minutes & BQ32K_SECONDS_MASK);
+	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
 	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
 	tm->tm_mday = bcd2bin(regs.date);
 	tm->tm_wday = bcd2bin(regs.day) - 1;
-- 
2.7.4

[PATCH 2/2] rtc: bq32k: Fix handling of oscillator failure flag

[Daniel Romell]

From: Jan Östlund <jao@hms.se>

While the oscillator failure flag is set, the RTC registers
should be considered invalid. bq32k_rtc_read_time() now
returns an error instead of an invalid time.

The failure flag is cleared the next time the clock is set.

Signed-off-by: Daniel Romell <daro@hms.se>
---
 drivers/rtc/rtc-bq32k.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index 5a0c137..3fc6f7c 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -93,6 +93,13 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (error)
 		return error;
 
+	/*
+	 * In case of oscillator failure, the register contents should be
+	 * considered invalid. The flag is cleared the next time the RTC is set.
+	 */
+	if (regs.minutes & BQ32K_OF)
+		return -EIO;
+
 	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
 	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
 	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
@@ -204,13 +211,10 @@ static int bq32k_probe(struct i2c_client *client,
 
 	/* Check Oscillator Failure flag */
 	error = bq32k_read(dev, &reg, BQ32K_MINUTES, 1);
-	if (!error && (reg & BQ32K_OF)) {
-		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
-		reg &= ~BQ32K_OF;
-		error = bq32k_write(dev, &reg, BQ32K_MINUTES, 1);
-	}
 	if (error)
 		return error;
+	if (reg & BQ32K_OF)
+		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
 
 	if (client->dev.of_node)
 		trickle_charger_of_init(dev, client->dev.of_node);
-- 
2.7.4

Re: [PATCH 2/2] rtc: bq32k: Fix handling of oscillator failure flag

[Alexandre Belloni]

On 11/08/2016 at 11:59:15 +0200, Daniel Romell wrote :
> From: Jan Östlund <jao@hms.se>
> 
> While the oscillator failure flag is set, the RTC registers
> should be considered invalid. bq32k_rtc_read_time() now
> returns an error instead of an invalid time.
> 
> The failure flag is cleared the next time the clock is set.
> 
> Signed-off-by: Daniel Romell <daro@hms.se>
> ---
>  drivers/rtc/rtc-bq32k.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
> index 5a0c137..3fc6f7c 100644
> --- a/drivers/rtc/rtc-bq32k.c
> +++ b/drivers/rtc/rtc-bq32k.c
> @@ -93,6 +93,13 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	if (error)
>  		return error;
>  
> +	/*
> +	 * In case of oscillator failure, the register contents should be
> +	 * considered invalid. The flag is cleared the next time the RTC is set.
> +	 */
> +	if (regs.minutes & BQ32K_OF)
> +		return -EIO;

The other drivers return -EINVAL in that case. Else, the change is fine.

> +
>  	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
>  	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
>  	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
> @@ -204,13 +211,10 @@ static int bq32k_probe(struct i2c_client *client,
>  
>  	/* Check Oscillator Failure flag */
>  	error = bq32k_read(dev, &reg, BQ32K_MINUTES, 1);
> -	if (!error && (reg & BQ32K_OF)) {
> -		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
> -		reg &= ~BQ32K_OF;
> -		error = bq32k_write(dev, &reg, BQ32K_MINUTES, 1);
> -	}
>  	if (error)
>  		return error;
> +	if (reg & BQ32K_OF)
> +		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
>  
>  	if (client->dev.of_node)
>  		trickle_charger_of_init(dev, client->dev.of_node);
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 0/2] bq32000 fixes

[Daniel Romell]

Changes since v1:
* Return -EINVAL instead of -EIO when failure flag is set.

Jan Östlund (2):
  rtc: bq32k: Use correct mask name for 'minutes' register.
  rtc: bq32k: Fix handling of oscillator failure flag

 drivers/rtc/rtc-bq32k.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

--
2.7.4

[PATCH v2 1/2] rtc: bq32k: Use correct mask name for 'minutes' register.

[Daniel Romell]

From: Jan Östlund <jao@hms.se>

The BQ32K_SECONDS_MASK and BQ32K_MINUTES_MASK both has the same
value. This is no functional change.

Signed-off-by: Daniel Romell <daro@hms.se>
---
 drivers/rtc/rtc-bq32k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index 0299988..5a0c137 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -94,7 +94,7 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		return error;
 
 	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
-	tm->tm_min = bcd2bin(regs.minutes & BQ32K_SECONDS_MASK);
+	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
 	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
 	tm->tm_mday = bcd2bin(regs.date);
 	tm->tm_wday = bcd2bin(regs.day) - 1;
-- 
2.7.4

Re: [PATCH v2 1/2] rtc: bq32k: Use correct mask name for 'minutes' register.

[Alexandre Belloni]

On 11/08/2016 at 13:31:43 +0200, Daniel Romell wrote :
> From: Jan Östlund <jao@hms.se>
> 
> The BQ32K_SECONDS_MASK and BQ32K_MINUTES_MASK both has the same
> value. This is no functional change.
> 
> Signed-off-by: Daniel Romell <daro@hms.se>

Sorry, I saw it only before trying apply but both patches also need Jan's SoB.

> ---
>  drivers/rtc/rtc-bq32k.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
> index 0299988..5a0c137 100644
> --- a/drivers/rtc/rtc-bq32k.c
> +++ b/drivers/rtc/rtc-bq32k.c
> @@ -94,7 +94,7 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  		return error;
>  
>  	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
> -	tm->tm_min = bcd2bin(regs.minutes & BQ32K_SECONDS_MASK);
> +	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
>  	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
>  	tm->tm_mday = bcd2bin(regs.date);
>  	tm->tm_wday = bcd2bin(regs.day) - 1;
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [rtc-linux] Re: [PATCH v2 1/2] rtc: bq32k: Use correct mask name for 'minutes' register.

[Alexandre Belloni]

On 11/08/2016 at 07:56:03 -0700, danielromell@gmail.com wrote :
> 
> 
> Den torsdag 11 augusti 2016 kl. 15:42:58 UTC+2 skrev alexandre.belloni:
> >
> > On 11/08/2016 at 13:31:43 +0200, Daniel Romell wrote : 
> > > From: Jan Östlund <j...@hms.se <javascript:>> 
> > > 
> > > The BQ32K_SECONDS_MASK and BQ32K_MINUTES_MASK both has the same 
> > > value. This is no functional change. 
> > > 
> > > Signed-off-by: Daniel Romell <da...@hms.se <javascript:>> 
> >
> > Sorry, I saw it only before trying apply but both patches also need Jan's 
> > SoB. 
> >
> 
> Ah, sorry, my bad. I'll ask him to add that. 
> Is it ok if he just reply to the emails with his SoB line, or do you need a 
> new patch set?  
> 

That's a bit unconventional but I'm fine with a simple mail.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

RE: [PATCH v2 1/2] rtc: bq32k: Use correct mask name for 'minutes' register.

[Jan Östlund]

Signed-off-by: Jan Östlund <jao@hms.se>

-----Original Message-----
From: Daniel Romell [mailto:danielromell@gmail.com] 
Sent: den 11 augusti 2016 13:32
To: a.zummo@towertech.it
Cc: alexandre.belloni@free-electrons.com; rtc-linux@googlegroups.com; linux-kernel@vger.kernel.org; Daniel Romell <Daro@hms.se>; Jan Östlund <jao@hms.se>; Magnus Olsson <Mago@hms.se>
Subject: [PATCH v2 1/2] rtc: bq32k: Use correct mask name for 'minutes' register.

From: Jan Östlund <jao@hms.se>

The BQ32K_SECONDS_MASK and BQ32K_MINUTES_MASK both has the same
value. This is no functional change.

Signed-off-by: Daniel Romell <daro@hms.se>
---
 drivers/rtc/rtc-bq32k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index 0299988..5a0c137 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -94,7 +94,7 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		return error;
 
 	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
-	tm->tm_min = bcd2bin(regs.minutes & BQ32K_SECONDS_MASK);
+	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
 	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
 	tm->tm_mday = bcd2bin(regs.date);
 	tm->tm_wday = bcd2bin(regs.day) - 1;
-- 
2.7.4

[PATCH v2 2/2] rtc: bq32k: Fix handling of oscillator failure flag

[Daniel Romell]

From: Jan Östlund <jao@hms.se>

While the oscillator failure flag is set, the RTC registers
should be considered invalid. bq32k_rtc_read_time() now
returns an error instead of an invalid time.

The failure flag is cleared the next time the clock is set.

Signed-off-by: Daniel Romell <daro@hms.se>
---
 drivers/rtc/rtc-bq32k.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index 5a0c137..3977424 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -93,6 +93,13 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (error)
 		return error;
 
+	/*
+	 * In case of oscillator failure, the register contents should be
+	 * considered invalid. The flag is cleared the next time the RTC is set.
+	 */
+	if (regs.minutes & BQ32K_OF)
+		return -EINVAL;
+
 	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
 	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
 	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
@@ -204,13 +211,10 @@ static int bq32k_probe(struct i2c_client *client,
 
 	/* Check Oscillator Failure flag */
 	error = bq32k_read(dev, &reg, BQ32K_MINUTES, 1);
-	if (!error && (reg & BQ32K_OF)) {
-		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
-		reg &= ~BQ32K_OF;
-		error = bq32k_write(dev, &reg, BQ32K_MINUTES, 1);
-	}
 	if (error)
 		return error;
+	if (reg & BQ32K_OF)
+		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
 
 	if (client->dev.of_node)
 		trickle_charger_of_init(dev, client->dev.of_node);
-- 
2.7.4

RE: [PATCH v2 2/2] rtc: bq32k: Fix handling of oscillator failure flag

[Jan Östlund]

Signed-off-by: Jan Östlund <jao@hms.se>

-----Original Message-----
From: Daniel Romell [mailto:danielromell@gmail.com] 
Sent: den 11 augusti 2016 13:32
To: a.zummo@towertech.it
Cc: alexandre.belloni@free-electrons.com; rtc-linux@googlegroups.com; linux-kernel@vger.kernel.org; Daniel Romell <Daro@hms.se>; Jan Östlund <jao@hms.se>; Magnus Olsson <Mago@hms.se>
Subject: [PATCH v2 2/2] rtc: bq32k: Fix handling of oscillator failure flag

From: Jan Östlund <jao@hms.se>

While the oscillator failure flag is set, the RTC registers
should be considered invalid. bq32k_rtc_read_time() now
returns an error instead of an invalid time.

The failure flag is cleared the next time the clock is set.

Signed-off-by: Daniel Romell <daro@hms.se>
---
 drivers/rtc/rtc-bq32k.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index 5a0c137..3977424 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -93,6 +93,13 @@ static int bq32k_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (error)
 		return error;
 
+	/*
+	 * In case of oscillator failure, the register contents should be
+	 * considered invalid. The flag is cleared the next time the RTC is set.
+	 */
+	if (regs.minutes & BQ32K_OF)
+		return -EINVAL;
+
 	tm->tm_sec = bcd2bin(regs.seconds & BQ32K_SECONDS_MASK);
 	tm->tm_min = bcd2bin(regs.minutes & BQ32K_MINUTES_MASK);
 	tm->tm_hour = bcd2bin(regs.cent_hours & BQ32K_HOURS_MASK);
@@ -204,13 +211,10 @@ static int bq32k_probe(struct i2c_client *client,
 
 	/* Check Oscillator Failure flag */
 	error = bq32k_read(dev, &reg, BQ32K_MINUTES, 1);
-	if (!error && (reg & BQ32K_OF)) {
-		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
-		reg &= ~BQ32K_OF;
-		error = bq32k_write(dev, &reg, BQ32K_MINUTES, 1);
-	}
 	if (error)
 		return error;
+	if (reg & BQ32K_OF)
+		dev_warn(dev, "Oscillator Failure. Check RTC battery.\n");
 
 	if (client->dev.of_node)
 		trickle_charger_of_init(dev, client->dev.of_node);
-- 
2.7.4