* [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
@ 2016-12-17 12:10 Russell King
2016-12-17 18:30 ` Wolfram Sang
0 siblings, 1 reply; 10+ messages in thread
From: Russell King @ 2016-12-17 12:10 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Peter Rosin, linux-i2c
smbus functions return -ve on error, 0 on success. However,
__i2c_transfer() have a different return signature - -ve on error, or
number of buffers transferred (which may be zero or greater.)
The upshot of this is that the sense of the test is reversed when using
the mux on a bus supporting the master_xfer method: we cache the value
and never retry if we fail to transfer any buffers, but if we succeed,
we clear the cached value.
Fix this by making pca954x_reg_write() return a negative error code for
all failure cases.
Fixes: 463e8f845cbf ("i2c: mux: pca954x: retry updating the mux selection on failure")
Acked-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8bc3d36d2837..9c4ac26c014e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -151,6 +151,9 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
buf[0] = val;
msg.buf = buf;
ret = __i2c_transfer(adap, &msg, 1);
+
+ if (ret >= 0 && ret != 1)
+ ret = -EREMOTEIO;
} else {
union i2c_smbus_data data;
ret = adap->algo->smbus_xfer(adap, client->addr,
@@ -179,7 +182,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
/* Only select the channel if its different from the last channel */
if (data->last_chan != regval) {
ret = pca954x_reg_write(muxc->parent, client, regval);
- data->last_chan = ret ? 0 : regval;
+ data->last_chan = ret < 0 ? 0 : regval;
}
return ret;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
2016-12-17 12:10 [PATCH] i2c: mux: pca954x: fix i2c mux selection caching Russell King
@ 2016-12-17 18:30 ` Wolfram Sang
0 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2016-12-17 18:30 UTC (permalink / raw)
To: Russell King; +Cc: Peter Rosin, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 857 bytes --]
On Sat, Dec 17, 2016 at 12:10:56PM +0000, Russell King wrote:
> smbus functions return -ve on error, 0 on success. However,
> __i2c_transfer() have a different return signature - -ve on error, or
> number of buffers transferred (which may be zero or greater.)
>
> The upshot of this is that the sense of the test is reversed when using
> the mux on a bus supporting the master_xfer method: we cache the value
> and never retry if we fail to transfer any buffers, but if we succeed,
> we clear the cached value.
>
> Fix this by making pca954x_reg_write() return a negative error code for
> all failure cases.
>
> Fixes: 463e8f845cbf ("i2c: mux: pca954x: retry updating the mux selection on failure")
> Acked-by: Peter Rosin <peda@axentia.se>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
2016-12-16 23:23 ` Russell King - ARM Linux
@ 2016-12-17 6:50 ` Peter Rosin
-1 siblings, 0 replies; 10+ messages in thread
From: Peter Rosin @ 2016-12-17 6:50 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Vivien Didelot, linux-i2c, linux-arm-kernel, Wolfram Sang
On 2016-12-17 00:23, Russell King - ARM Linux wrote:
> On Fri, Dec 16, 2016 at 10:20:35PM +0100, Peter Rosin wrote:
>> On 2016-12-16 21:06, Russell King wrote:
>>> smbus functions return -ve on error, 0 on success. However,
>>> __i2c_transfer() have a different return signature - -ve on error, or
>>> number of buffers transferred (which may be zero or greater.)
>>>
>>> The upshot of this is that the sense of the test is reversed when using
>>> the mux on a bus supporting the master_xfer method: we cache the value
>>> and never retry if we fail to transfer any buffers, but if we succeed,
>>> we clear the cached value.
>>
>> Ouch! Thanks for catching this.
>>
>>> Fix this.
>>
>> But lets fix the corner case of __i2c_transfer returning 0 instead of
>> the expected 1 as well (not sure if that's even possible, but lets close
>> the possibility just in case), so I'd prefer if you could fix
>> pca954x_reg_write() to return 0 iff __i2c_transfer(...) returns 1
>> instead, and -EREMOTEIO on other non-negative return values. Thanks!
>
> So you want something like this instead?
Yes, but I was originally thinking that the second hunk was no
longer needed...
Either way,
Acked-by: Peter Rosin <peda@axentia.se>
Cheers,
peda
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8bc3d36d2837..9c4ac26c014e 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -151,6 +151,9 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
> buf[0] = val;
> msg.buf = buf;
> ret = __i2c_transfer(adap, &msg, 1);
> +
> + if (ret >= 0 && ret != 1)
> + ret = -EREMOTEIO;
> } else {
> union i2c_smbus_data data;
> ret = adap->algo->smbus_xfer(adap, client->addr,
> @@ -179,7 +182,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> /* Only select the channel if its different from the last channel */
> if (data->last_chan != regval) {
> ret = pca954x_reg_write(muxc->parent, client, regval);
> - data->last_chan = ret ? 0 : regval;
> + data->last_chan = ret < 0 ? 0 : regval;
> }
>
> return ret;
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
@ 2016-12-17 6:50 ` Peter Rosin
0 siblings, 0 replies; 10+ messages in thread
From: Peter Rosin @ 2016-12-17 6:50 UTC (permalink / raw)
To: linux-arm-kernel
On 2016-12-17 00:23, Russell King - ARM Linux wrote:
> On Fri, Dec 16, 2016 at 10:20:35PM +0100, Peter Rosin wrote:
>> On 2016-12-16 21:06, Russell King wrote:
>>> smbus functions return -ve on error, 0 on success. However,
>>> __i2c_transfer() have a different return signature - -ve on error, or
>>> number of buffers transferred (which may be zero or greater.)
>>>
>>> The upshot of this is that the sense of the test is reversed when using
>>> the mux on a bus supporting the master_xfer method: we cache the value
>>> and never retry if we fail to transfer any buffers, but if we succeed,
>>> we clear the cached value.
>>
>> Ouch! Thanks for catching this.
>>
>>> Fix this.
>>
>> But lets fix the corner case of __i2c_transfer returning 0 instead of
>> the expected 1 as well (not sure if that's even possible, but lets close
>> the possibility just in case), so I'd prefer if you could fix
>> pca954x_reg_write() to return 0 iff __i2c_transfer(...) returns 1
>> instead, and -EREMOTEIO on other non-negative return values. Thanks!
>
> So you want something like this instead?
Yes, but I was originally thinking that the second hunk was no
longer needed...
Either way,
Acked-by: Peter Rosin <peda@axentia.se>
Cheers,
peda
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8bc3d36d2837..9c4ac26c014e 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -151,6 +151,9 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
> buf[0] = val;
> msg.buf = buf;
> ret = __i2c_transfer(adap, &msg, 1);
> +
> + if (ret >= 0 && ret != 1)
> + ret = -EREMOTEIO;
> } else {
> union i2c_smbus_data data;
> ret = adap->algo->smbus_xfer(adap, client->addr,
> @@ -179,7 +182,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> /* Only select the channel if its different from the last channel */
> if (data->last_chan != regval) {
> ret = pca954x_reg_write(muxc->parent, client, regval);
> - data->last_chan = ret ? 0 : regval;
> + data->last_chan = ret < 0 ? 0 : regval;
> }
>
> return ret;
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
2016-12-16 21:20 ` Peter Rosin
@ 2016-12-16 23:23 ` Russell King - ARM Linux
-1 siblings, 0 replies; 10+ messages in thread
From: Russell King - ARM Linux @ 2016-12-16 23:23 UTC (permalink / raw)
To: Peter Rosin; +Cc: Wolfram Sang, linux-i2c, linux-arm-kernel
On Fri, Dec 16, 2016 at 10:20:35PM +0100, Peter Rosin wrote:
> On 2016-12-16 21:06, Russell King wrote:
> > smbus functions return -ve on error, 0 on success. However,
> > __i2c_transfer() have a different return signature - -ve on error, or
> > number of buffers transferred (which may be zero or greater.)
> >
> > The upshot of this is that the sense of the test is reversed when using
> > the mux on a bus supporting the master_xfer method: we cache the value
> > and never retry if we fail to transfer any buffers, but if we succeed,
> > we clear the cached value.
>
> Ouch! Thanks for catching this.
>
> > Fix this.
>
> But lets fix the corner case of __i2c_transfer returning 0 instead of
> the expected 1 as well (not sure if that's even possible, but lets close
> the possibility just in case), so I'd prefer if you could fix
> pca954x_reg_write() to return 0 iff __i2c_transfer(...) returns 1
> instead, and -EREMOTEIO on other non-negative return values. Thanks!
So you want something like this instead?
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8bc3d36d2837..9c4ac26c014e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -151,6 +151,9 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
buf[0] = val;
msg.buf = buf;
ret = __i2c_transfer(adap, &msg, 1);
+
+ if (ret >= 0 && ret != 1)
+ ret = -EREMOTEIO;
} else {
union i2c_smbus_data data;
ret = adap->algo->smbus_xfer(adap, client->addr,
@@ -179,7 +182,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
/* Only select the channel if its different from the last channel */
if (data->last_chan != regval) {
ret = pca954x_reg_write(muxc->parent, client, regval);
- data->last_chan = ret ? 0 : regval;
+ data->last_chan = ret < 0 ? 0 : regval;
}
return ret;
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
@ 2016-12-16 23:23 ` Russell King - ARM Linux
0 siblings, 0 replies; 10+ messages in thread
From: Russell King - ARM Linux @ 2016-12-16 23:23 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2016 at 10:20:35PM +0100, Peter Rosin wrote:
> On 2016-12-16 21:06, Russell King wrote:
> > smbus functions return -ve on error, 0 on success. However,
> > __i2c_transfer() have a different return signature - -ve on error, or
> > number of buffers transferred (which may be zero or greater.)
> >
> > The upshot of this is that the sense of the test is reversed when using
> > the mux on a bus supporting the master_xfer method: we cache the value
> > and never retry if we fail to transfer any buffers, but if we succeed,
> > we clear the cached value.
>
> Ouch! Thanks for catching this.
>
> > Fix this.
>
> But lets fix the corner case of __i2c_transfer returning 0 instead of
> the expected 1 as well (not sure if that's even possible, but lets close
> the possibility just in case), so I'd prefer if you could fix
> pca954x_reg_write() to return 0 iff __i2c_transfer(...) returns 1
> instead, and -EREMOTEIO on other non-negative return values. Thanks!
So you want something like this instead?
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8bc3d36d2837..9c4ac26c014e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -151,6 +151,9 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
buf[0] = val;
msg.buf = buf;
ret = __i2c_transfer(adap, &msg, 1);
+
+ if (ret >= 0 && ret != 1)
+ ret = -EREMOTEIO;
} else {
union i2c_smbus_data data;
ret = adap->algo->smbus_xfer(adap, client->addr,
@@ -179,7 +182,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
/* Only select the channel if its different from the last channel */
if (data->last_chan != regval) {
ret = pca954x_reg_write(muxc->parent, client, regval);
- data->last_chan = ret ? 0 : regval;
+ data->last_chan = ret < 0 ? 0 : regval;
}
return ret;
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently@9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
2016-12-16 20:06 ` Russell King
@ 2016-12-16 21:20 ` Peter Rosin
-1 siblings, 0 replies; 10+ messages in thread
From: Peter Rosin @ 2016-12-16 21:20 UTC (permalink / raw)
To: Russell King; +Cc: Wolfram Sang, linux-i2c, linux-arm-kernel
On 2016-12-16 21:06, Russell King wrote:
> smbus functions return -ve on error, 0 on success. However,
> __i2c_transfer() have a different return signature - -ve on error, or
> number of buffers transferred (which may be zero or greater.)
>
> The upshot of this is that the sense of the test is reversed when using
> the mux on a bus supporting the master_xfer method: we cache the value
> and never retry if we fail to transfer any buffers, but if we succeed,
> we clear the cached value.
Ouch! Thanks for catching this.
> Fix this.
But lets fix the corner case of __i2c_transfer returning 0 instead of
the expected 1 as well (not sure if that's even possible, but lets close
the possibility just in case), so I'd prefer if you could fix
pca954x_reg_write() to return 0 iff __i2c_transfer(...) returns 1
instead, and -EREMOTEIO on other non-negative return values. Thanks!
Cheers,
peda
> Fixes: 463e8f845cbf ("i2c: mux: pca954x: retry updating the mux selection on failure")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8bc3d36d2837..b6d62ecbd5b6 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -179,7 +179,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> /* Only select the channel if its different from the last channel */
> if (data->last_chan != regval) {
> ret = pca954x_reg_write(muxc->parent, client, regval);
> - data->last_chan = ret ? 0 : regval;
> + data->last_chan = ret >= 0 ? regval : 0;
> }
>
> return ret;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
@ 2016-12-16 21:20 ` Peter Rosin
0 siblings, 0 replies; 10+ messages in thread
From: Peter Rosin @ 2016-12-16 21:20 UTC (permalink / raw)
To: linux-arm-kernel
On 2016-12-16 21:06, Russell King wrote:
> smbus functions return -ve on error, 0 on success. However,
> __i2c_transfer() have a different return signature - -ve on error, or
> number of buffers transferred (which may be zero or greater.)
>
> The upshot of this is that the sense of the test is reversed when using
> the mux on a bus supporting the master_xfer method: we cache the value
> and never retry if we fail to transfer any buffers, but if we succeed,
> we clear the cached value.
Ouch! Thanks for catching this.
> Fix this.
But lets fix the corner case of __i2c_transfer returning 0 instead of
the expected 1 as well (not sure if that's even possible, but lets close
the possibility just in case), so I'd prefer if you could fix
pca954x_reg_write() to return 0 iff __i2c_transfer(...) returns 1
instead, and -EREMOTEIO on other non-negative return values. Thanks!
Cheers,
peda
> Fixes: 463e8f845cbf ("i2c: mux: pca954x: retry updating the mux selection on failure")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8bc3d36d2837..b6d62ecbd5b6 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -179,7 +179,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> /* Only select the channel if its different from the last channel */
> if (data->last_chan != regval) {
> ret = pca954x_reg_write(muxc->parent, client, regval);
> - data->last_chan = ret ? 0 : regval;
> + data->last_chan = ret >= 0 ? regval : 0;
> }
>
> return ret;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
@ 2016-12-16 20:06 ` Russell King
0 siblings, 0 replies; 10+ messages in thread
From: Russell King @ 2016-12-16 20:06 UTC (permalink / raw)
To: Peter Rosin; +Cc: Wolfram Sang, linux-i2c, linux-arm-kernel
smbus functions return -ve on error, 0 on success. However,
__i2c_transfer() have a different return signature - -ve on error, or
number of buffers transferred (which may be zero or greater.)
The upshot of this is that the sense of the test is reversed when using
the mux on a bus supporting the master_xfer method: we cache the value
and never retry if we fail to transfer any buffers, but if we succeed,
we clear the cached value.
Fix this.
Fixes: 463e8f845cbf ("i2c: mux: pca954x: retry updating the mux selection on failure")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8bc3d36d2837..b6d62ecbd5b6 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -179,7 +179,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
/* Only select the channel if its different from the last channel */
if (data->last_chan != regval) {
ret = pca954x_reg_write(muxc->parent, client, regval);
- data->last_chan = ret ? 0 : regval;
+ data->last_chan = ret >= 0 ? regval : 0;
}
return ret;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: pca954x: fix i2c mux selection caching
@ 2016-12-16 20:06 ` Russell King
0 siblings, 0 replies; 10+ messages in thread
From: Russell King @ 2016-12-16 20:06 UTC (permalink / raw)
To: linux-arm-kernel
smbus functions return -ve on error, 0 on success. However,
__i2c_transfer() have a different return signature - -ve on error, or
number of buffers transferred (which may be zero or greater.)
The upshot of this is that the sense of the test is reversed when using
the mux on a bus supporting the master_xfer method: we cache the value
and never retry if we fail to transfer any buffers, but if we succeed,
we clear the cached value.
Fix this.
Fixes: 463e8f845cbf ("i2c: mux: pca954x: retry updating the mux selection on failure")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8bc3d36d2837..b6d62ecbd5b6 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -179,7 +179,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
/* Only select the channel if its different from the last channel */
if (data->last_chan != regval) {
ret = pca954x_reg_write(muxc->parent, client, regval);
- data->last_chan = ret ? 0 : regval;
+ data->last_chan = ret >= 0 ? regval : 0;
}
return ret;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-12-17 18:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-17 12:10 [PATCH] i2c: mux: pca954x: fix i2c mux selection caching Russell King
2016-12-17 18:30 ` Wolfram Sang
-- strict thread matches above, loose matches on Subject: below --
2016-12-16 20:06 Russell King
2016-12-16 20:06 ` Russell King
2016-12-16 21:20 ` Peter Rosin
2016-12-16 21:20 ` Peter Rosin
2016-12-16 23:23 ` Russell King - ARM Linux
2016-12-16 23:23 ` Russell King - ARM Linux
2016-12-17 6:50 ` Peter Rosin
2016-12-17 6:50 ` Peter Rosin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.