[PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[Maciej W. Rozycki]

Complement commit 85d77047c4ea ("drivers/rtc/rtc-m41t80.c: propagate 
error value from smbus functions") and correct the remaining places that 
fail to propagate the error code from SMBus calls.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
References: 85d77047c4ea ("drivers/rtc/rtc-m41t80.c: propagate error value from smbus functions")
---
Hi,

 I think this does not qualify for backporting, but please feel free to 
decide otherwise.

 This change I did verify at run time to the extent I was able to, but I 
didn't try to trigger artificial errors.  Also the M41T81, which is the 
device I've been using this driver with, does not have battery failure 
indication implemented, so I could not execute the procfs handling path 
(again without making some artificial changes).  These changes should be 
obvious regardless.

 I'll be posting further patches over the coming weeks, based on my 
original effort as archived here: <https://lkml.org/lkml/2008/5/12/385>,
<https://lore.kernel.org/patchwork/project/lkml/list/?series=73524&archive=both> 
However I have just realised they'll need another iteration before I post 
them.  So for now just these two obvious fixes.

 Please apply.

  Maciej
---
 drivers/rtc/rtc-m41t80.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

linux-rtc-m41t80-err.diff
Index: linux-20181008-swarm64-eb/drivers/rtc/rtc-m41t80.c
===================================================================
--- linux-20181008-swarm64-eb.orig/drivers/rtc/rtc-m41t80.c
+++ linux-20181008-swarm64-eb/drivers/rtc/rtc-m41t80.c
@@ -217,7 +217,7 @@ static int m41t80_rtc_read_time(struct d
 					    sizeof(buf), buf);
 	if (err < 0) {
 		dev_err(&client->dev, "Unable to read date\n");
-		return -EIO;
+		return err;
 	}
 
 	tm->tm_sec = bcd2bin(buf[M41T80_REG_SEC] & 0x7f);
@@ -274,10 +274,11 @@ static int m41t80_rtc_set_time(struct de
 	if (flags < 0)
 		return flags;
 
-	if (i2c_smbus_write_byte_data(client, M41T80_REG_FLAGS,
-				      flags & ~M41T80_FLAGS_OF)) {
+	err = i2c_smbus_write_byte_data(client, M41T80_REG_FLAGS,
+					flags & ~M41T80_FLAGS_OF);
+	if (err < 0) {
 		dev_err(&client->dev, "Unable to write flags register\n");
-		return -EIO;
+		return err;
 	}
 
 	return err;
@@ -287,10 +288,12 @@ static int m41t80_rtc_proc(struct device
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct m41t80_data *clientdata = i2c_get_clientdata(client);
-	u8 reg;
+	int reg;
 
 	if (clientdata->features & M41T80_FEATURE_BL) {
 		reg = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
+		if (reg < 0)
+			return reg;
 		seq_printf(seq, "battery\t\t: %s\n",
 			   (reg & M41T80_FLAGS_BATT_LOW) ? "exhausted" : "ok");
 	}

Re: [PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[Alexandre Belloni]

On 07/11/2018 02:39:51+0000, Maciej W. Rozycki wrote:
> Complement commit 85d77047c4ea ("drivers/rtc/rtc-m41t80.c: propagate 
> error value from smbus functions") and correct the remaining places that 
> fail to propagate the error code from SMBus calls.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> References: 85d77047c4ea ("drivers/rtc/rtc-m41t80.c: propagate error value from smbus functions")
> ---
> Hi,
> 
>  I think this does not qualify for backporting, but please feel free to 
> decide otherwise.
> 
>  This change I did verify at run time to the extent I was able to, but I 
> didn't try to trigger artificial errors.  Also the M41T81, which is the 
> device I've been using this driver with, does not have battery failure 
> indication implemented, so I could not execute the procfs handling path 
> (again without making some artificial changes).  These changes should be 
> obvious regardless.
> 
>  I'll be posting further patches over the coming weeks, based on my 
> original effort as archived here: <https://lkml.org/lkml/2008/5/12/385>,
> <https://lore.kernel.org/patchwork/project/lkml/list/?series=73524&archive=both> 
> However I have just realised they'll need another iteration before I post 
> them.  So for now just these two obvious fixes.
> 
>  Please apply.
> 
>   Maciej
> ---
>  drivers/rtc/rtc-m41t80.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[Alexandre Belloni]

On 07/11/2018 02:39:51+0000, Maciej W. Rozycki wrote:
> Complement commit 85d77047c4ea ("drivers/rtc/rtc-m41t80.c: propagate 
> error value from smbus functions") and correct the remaining places that 
> fail to propagate the error code from SMBus calls.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> References: 85d77047c4ea ("drivers/rtc/rtc-m41t80.c: propagate error value from smbus functions")
> ---
> Hi,
> 
>  I think this does not qualify for backporting, but please feel free to 
> decide otherwise.
> 
>  This change I did verify at run time to the extent I was able to, but I 
> didn't try to trigger artificial errors.  Also the M41T81, which is the 
> device I've been using this driver with, does not have battery failure 
> indication implemented, so I could not execute the procfs handling path 
> (again without making some artificial changes).  These changes should be 
> obvious regardless.
> 
>  I'll be posting further patches over the coming weeks, based on my 
> original effort as archived here: <https://lkml.org/lkml/2008/5/12/385>,
> <https://lore.kernel.org/patchwork/project/lkml/list/?series=73524&archive=both> 
> However I have just realised they'll need another iteration before I post 
> them.  So for now just these two obvious fixes.
> 

Regarding the persistent part, do you really need more than what is
provided? As far as I know, the timekeeping core is already taking care
of using the best source to get the suspended time.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[Maciej W. Rozycki]

On Wed, 14 Nov 2018, Alexandre Belloni wrote:

> >  I'll be posting further patches over the coming weeks, based on my 
> > original effort as archived here: <https://lkml.org/lkml/2008/5/12/385>,
> > <https://lore.kernel.org/patchwork/project/lkml/list/?series=73524&archive=both> 
> > However I have just realised they'll need another iteration before I post 
> > them.  So for now just these two obvious fixes.
> > 
> 
> Regarding the persistent part, do you really need more than what is
> provided? As far as I know, the timekeeping core is already taking care
> of using the best source to get the suspended time.

 For that we have platform code duplicating what the RTC framework 
already does.  Some of that cannot be made to work reasonably.  See e.g. 
arch/mips/dec/time.c or arch/mips/sibyte/swarm/rtc_*.c.

 The DEC code is technically sound, but spreads the same function (RTC 
r/w access) across another place, in addition to drivers/rtc/rtc-cmos.c.  
The SiByte code bypasses the I2C driver and therefore cannot be 
converted to use I2C interrupts, which means system latency problems.  
The write part used for NTP can be removed right away by making it 
return -ENODEV, and I have separate patches to do that for these 
platforms.  The read part cannot AFAICT.

 I think we can discuss that when I post the patches.  The m41t80 driver 
currently does not work for me anyway and has to be fixed because of:

i2c /dev entries driver
i2c-sibyte: i2c SMBus adapter module for SiByte board
i2c i2c-1: doesn't support I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK

and the persistent part is only one patch in the upcoming number of 
changes.

 Thanks for taking the other three patches.

  Maciej

Re: [PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[Alexandre Belloni]

On 14/11/2018 12:05:14+0000, Maciej W. Rozycki wrote:
> On Wed, 14 Nov 2018, Alexandre Belloni wrote:
> 
> > >  I'll be posting further patches over the coming weeks, based on my 
> > > original effort as archived here: <https://lkml.org/lkml/2008/5/12/385>,
> > > <https://lore.kernel.org/patchwork/project/lkml/list/?series=73524&archive=both> 
> > > However I have just realised they'll need another iteration before I post 
> > > them.  So for now just these two obvious fixes.
> > > 
> > 
> > Regarding the persistent part, do you really need more than what is
> > provided? As far as I know, the timekeeping core is already taking care
> > of using the best source to get the suspended time.
> 
>  For that we have platform code duplicating what the RTC framework 
> already does.  Some of that cannot be made to work reasonably.  See e.g. 
> arch/mips/dec/time.c or arch/mips/sibyte/swarm/rtc_*.c.
> 
>  The DEC code is technically sound, but spreads the same function (RTC 
> r/w access) across another place, in addition to drivers/rtc/rtc-cmos.c.  
> The SiByte code bypasses the I2C driver and therefore cannot be 
> converted to use I2C interrupts, which means system latency problems.  
> The write part used for NTP can be removed right away by making it 
> return -ENODEV, and I have separate patches to do that for these 
> platforms.  The read part cannot AFAICT.
> 
>  I think we can discuss that when I post the patches.  The m41t80 driver 
> currently does not work for me anyway and has to be fixed because of:
> 
> i2c /dev entries driver
> i2c-sibyte: i2c SMBus adapter module for SiByte board
> i2c i2c-1: doesn't support I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK
> 
> and the persistent part is only one patch in the upcoming number of 
> changes.
> 

Well, one of the solution for that (and tis is on my todo list) is to
convert the driver to use regmap which would take care of using the
proper i2c transfers. However, one of the concern when not having bock
accesses is that the registers are not latched (as you seem to know).
One thing I would like is then to avoid the multiple SEC register read
when not necessary.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: m41t80: Complete error propagation from SMBus calls

[Maciej W. Rozycki]

On Wed, 14 Nov 2018, Alexandre Belloni wrote:

> >  I think we can discuss that when I post the patches.  The m41t80 driver 
> > currently does not work for me anyway and has to be fixed because of:
> > 
> > i2c /dev entries driver
> > i2c-sibyte: i2c SMBus adapter module for SiByte board
> > i2c i2c-1: doesn't support I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK
> > 
> > and the persistent part is only one patch in the upcoming number of 
> > changes.
> 
> Well, one of the solution for that (and tis is on my todo list) is to
> convert the driver to use regmap which would take care of using the
> proper i2c transfers. However, one of the concern when not having bock
> accesses is that the registers are not latched (as you seem to know).
> One thing I would like is then to avoid the multiple SEC register read
> when not necessary.

 Unfortunately the SMBus host does not give much choice here.  It does 
have some extensions for block transfers, but writes are limited to 5 
bytes and reads to 7 bytes.  The usual solution is to read repeatedly 
until the seconds match.  For writes it is not a problem, because it 
takes less than 1 second to write all the clock registers, so if you 
start with seconds, then the data written will be consistent.

 The Xicor chip is worse as it uses 16-bit addresses and that is not 
handled by SMBus support in our I2C core, however apparently that can be 
simulated by byte writes with that particular chip.  The SMBus host 
implements a protocol extension for 16-bit addressing, but I think it's 
not worth the hassle adding to SMBus support in our I2C core given how 
rare the Xicor setup are.

 Finally the SMBus host does support raw I2C transfers, but only with a 
polled bit-banged interface, where you need to time the loop correctly 
to get clocking of the individual bits right.  I don't think we want to 
go down that path.

  Maciej