[PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Meng.Li]

From: Meng Li <Meng.Li@windriver.com>

When read adc conversion value with below command:
cat /sys/.../iio:device0/in_voltage0-voltage1_raw
There is an error reported as below:
ltc2497 0-0014: i2c transfer failed: -EREMOTEIO
This i2c transfer issue is introduced by commit 69548b7c2c4f ("iio:
adc: ltc2497: split protocol independent part in a separate module").
When extract the common code into ltc2497-core.c, it change the
code logic of function ltc2497core_read(). With wrong reading
sequence, the action of enable adc channel is sent to chip again
during adc channel is in conversion status. In this way, there is
no ack from chip, and then cause i2c transfer failed.
In order to keep the code logic is the same with original ideal,
it is need to return direct after reading the adc conversion value.

v2:
According to ltc2497 datasheet, the max value of conversion time is
149.9 ms. So, add 20% to the 150msecs so that there is enough time
for data conversion.

Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
Cc: stable@vger.kernel.org
Signed-off-by: Meng Li <Meng.Li@windriver.com>
---
 drivers/iio/adc/ltc2497.c | 2 ++
 drivers/iio/adc/ltc2497.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
index 1adddf5a88a9..fd5a66860a47 100644
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 		}
 
 		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
+
+		return ret;
 	}
 
 	ret = i2c_smbus_write_byte(st->client,
diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
index d0b42dd6b8ad..e451930837d8 100644
--- a/drivers/iio/adc/ltc2497.h
+++ b/drivers/iio/adc/ltc2497.h
@@ -2,7 +2,7 @@
 
 #define LTC2497_ENABLE			0xA0
 #define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
-#define LTC2497_CONVERSION_TIME_MS	150ULL
+#define LTC2497_CONVERSION_TIME_MS	180ULL
 
 struct ltc2497core_driverdata {
 	struct regulator *ref;
-- 
2.17.1

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Jonathan Cameron]

On Tue,  1 Jun 2021 17:28:05 +0800
Meng.Li@windriver.com wrote:

> From: Meng Li <Meng.Li@windriver.com>
> 
> When read adc conversion value with below command:
> cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> There is an error reported as below:
> ltc2497 0-0014: i2c transfer failed: -EREMOTEIO
> This i2c transfer issue is introduced by commit 69548b7c2c4f ("iio:
> adc: ltc2497: split protocol independent part in a separate module").
> When extract the common code into ltc2497-core.c, it change the
> code logic of function ltc2497core_read(). With wrong reading
> sequence, the action of enable adc channel is sent to chip again
> during adc channel is in conversion status. In this way, there is
> no ack from chip, and then cause i2c transfer failed.
> In order to keep the code logic is the same with original ideal,
> it is need to return direct after reading the adc conversion value.
> 
> v2:
> According to ltc2497 datasheet, the max value of conversion time is
> 149.9 ms. So, add 20% to the 150msecs so that there is enough time
> for data conversion.

Version change logs go below the --- as we don't want to preserve them
forever in the git history.

I may have lost track of the discussion, but I thought the idea was that perhaps
the longer time period would remove the need for the early return?

Thanks,

Jonathan
> 
> Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
> Cc: stable@vger.kernel.org
> Signed-off-by: Meng Li <Meng.Li@windriver.com>
> ---
>  drivers/iio/adc/ltc2497.c | 2 ++
>  drivers/iio/adc/ltc2497.h | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> index 1adddf5a88a9..fd5a66860a47 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>  		}
>  
>  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> +
> +		return ret;
>  	}
>  
>  	ret = i2c_smbus_write_byte(st->client,
> diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> index d0b42dd6b8ad..e451930837d8 100644
> --- a/drivers/iio/adc/ltc2497.h
> +++ b/drivers/iio/adc/ltc2497.h
> @@ -2,7 +2,7 @@
>  
>  #define LTC2497_ENABLE			0xA0
>  #define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> -#define LTC2497_CONVERSION_TIME_MS	150ULL
> +#define LTC2497_CONVERSION_TIME_MS	180ULL
>  
>  struct ltc2497core_driverdata {
>  	struct regulator *ref;

RE: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Li, Meng]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Friday, June 4, 2021 12:20 AM
> To: Li, Meng <Meng.Li@windriver.com>
> Cc: lars@metafoo.de; Michael.Hennerich@analog.com;
> pmeerw@pmeerw.net; u.kleine-koenig@pengutronix.de; linux-
> kernel@vger.kernel.org; linux-iio@vger.kernel.org
> Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> conversion value
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue,  1 Jun 2021 17:28:05 +0800
> Meng.Li@windriver.com wrote:
> 
> > From: Meng Li <Meng.Li@windriver.com>
> >
> > When read adc conversion value with below command:
> > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > There is an error reported as below:
> > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > issue is introduced by commit 69548b7c2c4f ("iio:
> > adc: ltc2497: split protocol independent part in a separate module").
> > When extract the common code into ltc2497-core.c, it change the code
> > logic of function ltc2497core_read(). With wrong reading sequence, the
> > action of enable adc channel is sent to chip again during adc channel
> > is in conversion status. In this way, there is no ack from chip, and
> > then cause i2c transfer failed.
> > In order to keep the code logic is the same with original ideal, it is
> > need to return direct after reading the adc conversion value.
> >
> > v2:
> > According to ltc2497 datasheet, the max value of conversion time is
> > 149.9 ms. So, add 20% to the 150msecs so that there is enough time for
> > data conversion.
> 
> Version change logs go below the --- as we don't want to preserve them
> forever in the git history.
> 
> I may have lost track of the discussion, but I thought the idea was that
> perhaps the longer time period would remove the need for the early return?
> 

No!
I think the ret is essential.
Because when call i2c_master_recv() to receive data from chip, there is a stop bit sent to chip at the last step.
When the chip receive the stop bit, it will start the next conversion operation. 
So, if we send any command on I2C bus when the chip is in conversion status, there will be a i2c error occurred.

Thanks,
Limeng

> Thanks,
> 
> Jonathan
> >
> > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent
> > part in a separate module ")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Meng Li <Meng.Li@windriver.com>
> > ---
> >  drivers/iio/adc/ltc2497.c | 2 ++
> >  drivers/iio/adc/ltc2497.h | 2 +-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > index 1adddf5a88a9..fd5a66860a47 100644
> > --- a/drivers/iio/adc/ltc2497.c
> > +++ b/drivers/iio/adc/ltc2497.c
> > @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct
> ltc2497core_driverdata *ddata,
> >               }
> >
> >               *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > +
> > +             return ret;
> >       }
> >
> >       ret = i2c_smbus_write_byte(st->client, diff --git
> > a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h index
> > d0b42dd6b8ad..e451930837d8 100644
> > --- a/drivers/iio/adc/ltc2497.h
> > +++ b/drivers/iio/adc/ltc2497.h
> > @@ -2,7 +2,7 @@
> >
> >  #define LTC2497_ENABLE                       0xA0
> >  #define LTC2497_CONFIG_DEFAULT               LTC2497_ENABLE
> > -#define LTC2497_CONVERSION_TIME_MS   150ULL
> > +#define LTC2497_CONVERSION_TIME_MS   180ULL
> >
> >  struct ltc2497core_driverdata {
> >       struct regulator *ref;

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5774 bytes --]

Hello,

On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:
> 
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Friday, June 4, 2021 12:20 AM
> > To: Li, Meng <Meng.Li@windriver.com>
> > Cc: lars@metafoo.de; Michael.Hennerich@analog.com;
> > pmeerw@pmeerw.net; u.kleine-koenig@pengutronix.de; linux-
> > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> > conversion value
> > 
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On Tue,  1 Jun 2021 17:28:05 +0800
> > Meng.Li@windriver.com wrote:
> > 
> > > From: Meng Li <Meng.Li@windriver.com>
> > >
> > > When read adc conversion value with below command:
> > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > There is an error reported as below:
> > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > > issue is introduced by commit 69548b7c2c4f ("iio:
> > > adc: ltc2497: split protocol independent part in a separate module").
> > > When extract the common code into ltc2497-core.c, it change the code
> > > logic of function ltc2497core_read(). With wrong reading sequence, the
> > > action of enable adc channel is sent to chip again during adc channel
> > > is in conversion status. In this way, there is no ack from chip, and
> > > then cause i2c transfer failed.
> > > In order to keep the code logic is the same with original ideal, it is
> > > need to return direct after reading the adc conversion value.

As background about the choice of the .result_and_measure callback:
A difference between the ltc2497 (i2c) and ltc2496 (spi) is that for the
latter reading the result of the last conversion and starting a new is a
single bus operation and the one cannot be done without the other.

> > > v2:
> > > According to ltc2497 datasheet, the max value of conversion time is
> > > 149.9 ms. So, add 20% to the 150msecs so that there is enough time for
> > > data conversion.
> > 
> > Version change logs go below the --- as we don't want to preserve them
> > forever in the git history.
> > 
> > I may have lost track of the discussion, but I thought the idea was that
> > perhaps the longer time period would remove the need for the early return?
> > 
> 
> No!
> I think the ret is essential.

I'd like to understand why. Currently ltc2497core_read() looks as
follows (simplified by dropping error handling, and unrolling the
result_and_measure callback for the i2c case):

	ltc2497core_wait_conv()

	// result_and_measure(address, NULL)
	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);

	msleep_interruptible(LTC2497_CONVERSION_TIME_MS)

	// result_and_measure(address, val);
	i2c_master_recv(client, &buf, 3);
	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);


With the early return you suggest to introduce with your patch you save
the last i2c_smbus_write_byte call. The data sheet indeed claims to
start a new conversion at the stop condition.

So either the reading of the conversion result and programming of the
(maybe) new address has to be done in a single i2c transfer, or we
have to do something like your patch.

What I don't like about your approach is that it changes the semantic of
the callback to result_*or*_measure which is something the spi variant
cannot implement. With the current use of the function this is fine,
however if at some time in the future we implement a bulk conversion
shortcut this hurts.

So I suggest to do:

---->8----
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Fri, 4 Jun 2021 08:02:44 +0200
Subject: [PATCH] iio: ltc2497: Fix reading conversion results

After the result of the previous conversion is read the chip
automatically starts a new conversion and doesn't accept new i2c
transfers until this conversion is completed which makes the function
return failure.

So add an early return iff the programming of the new address isn't
needed. Note this will not fix the problem in general, but all cases
that are currently used. Once this changes we get the failure back, but
this can be addressed when the need arises.

Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
Reported-by: Meng Li <Meng.Li@windriver.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/adc/ltc2497.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -41,6 +41,19 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 		}
 
 		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
+
+		/*
+		 * The part started a new conversion at the end of the above i2c
+		 * transfer, so if the address didn't change since the last call
+		 * everything is fine and we can return early.
+		 * If not (which should only happen when some sort of bulk
+		 * conversion is implemented) we have to program the new
+		 * address. Note that this probably fails as the conversion that
+		 * was triggered above is like not complete yet and the two
+		 * operations have to be done in a single transfer.
+		 */
+		if (ddata->addr_prev == address)
+			return 0;
 	}
 
 	ret = i2c_smbus_write_byte(st->client,

Compared to Meng Li's patch this doesn't introduce reporting of bogus
conversion results once we implement bulk conversion.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Li, Meng]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Friday, June 4, 2021 2:13 PM
> To: Li, Meng <Meng.Li@windriver.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de;
> Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> kernel@vger.kernel.org; linux-iio@vger.kernel.org
> Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> conversion value
> 
> Hello,
> 
> On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Friday, June 4, 2021 12:20 AM
> > > To: Li, Meng <Meng.Li@windriver.com>
> > > Cc: lars@metafoo.de; Michael.Hennerich@analog.com;
> > > pmeerw@pmeerw.net; u.kleine-koenig@pengutronix.de; linux-
> > > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after
> > > reading the adc conversion value
> > >
> > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > >
> > > On Tue,  1 Jun 2021 17:28:05 +0800
> > > Meng.Li@windriver.com wrote:
> > >
> > > > From: Meng Li <Meng.Li@windriver.com>
> > > >
> > > > When read adc conversion value with below command:
> > > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > > There is an error reported as below:
> > > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > > > issue is introduced by commit 69548b7c2c4f ("iio:
> > > > adc: ltc2497: split protocol independent part in a separate module").
> > > > When extract the common code into ltc2497-core.c, it change the
> > > > code logic of function ltc2497core_read(). With wrong reading
> > > > sequence, the action of enable adc channel is sent to chip again
> > > > during adc channel is in conversion status. In this way, there is
> > > > no ack from chip, and then cause i2c transfer failed.
> > > > In order to keep the code logic is the same with original ideal,
> > > > it is need to return direct after reading the adc conversion value.
> 
> As background about the choice of the .result_and_measure callback:
> A difference between the ltc2497 (i2c) and ltc2496 (spi) is that for the latter
> reading the result of the last conversion and starting a new is a single bus
> operation and the one cannot be done without the other.
> 
> > > > v2:
> > > > According to ltc2497 datasheet, the max value of conversion time
> > > > is
> > > > 149.9 ms. So, add 20% to the 150msecs so that there is enough time
> > > > for data conversion.
> > >
> > > Version change logs go below the --- as we don't want to preserve
> > > them forever in the git history.
> > >
> > > I may have lost track of the discussion, but I thought the idea was
> > > that perhaps the longer time period would remove the need for the early
> return?
> > >
> >
> > No!
> > I think the ret is essential.
> 
> I'd like to understand why. Currently ltc2497core_read() looks as follows
> (simplified by dropping error handling, and unrolling the result_and_measure
> callback for the i2c case):
> 
> 	ltc2497core_wait_conv()
> 
> 	// result_and_measure(address, NULL)
> 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> 
> 	msleep_interruptible(LTC2497_CONVERSION_TIME_MS)
> 
> 	// result_and_measure(address, val);
> 	i2c_master_recv(client, &buf, 3);
> 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> 
> 
> With the early return you suggest to introduce with your patch you save the
> last i2c_smbus_write_byte call. The data sheet indeed claims to start a new
> conversion at the stop condition.
> 
> So either the reading of the conversion result and programming of the
> (maybe) new address has to be done in a single i2c transfer, or we have to do
> something like your patch.
> 
> What I don't like about your approach is that it changes the semantic of the
> callback to result_*or*_measure which is something the spi variant cannot
> implement. With the current use of the function this is fine, however if at
> some time in the future we implement a bulk conversion shortcut this hurts.
> 
> So I suggest to do:
> 
> ---->8----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date: Fri, 4 Jun 2021 08:02:44 +0200
> Subject: [PATCH] iio: ltc2497: Fix reading conversion results
> 
> After the result of the previous conversion is read the chip automatically
> starts a new conversion and doesn't accept new i2c transfers until this
> conversion is completed which makes the function return failure.
> 
> So add an early return iff the programming of the new address isn't needed.
> Note this will not fix the problem in general, but all cases that are currently
> used. Once this changes we get the failure back, but this can be addressed
> when the need arises.
> 
> Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a
> separate module ")
> Reported-by: Meng Li <Meng.Li@windriver.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/iio/adc/ltc2497.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -41,6 +41,19 @@ static int ltc2497_result_and_measure(struct
> ltc2497core_driverdata *ddata,
>  		}
> 
>  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> +
> +		/*
> +		 * The part started a new conversion at the end of the above
> i2c
> +		 * transfer, so if the address didn't change since the last call
> +		 * everything is fine and we can return early.
> +		 * If not (which should only happen when some sort of bulk
> +		 * conversion is implemented) we have to program the new
> +		 * address. Note that this probably fails as the conversion
> that
> +		 * was triggered above is like not complete yet and the two
> +		 * operations have to be done in a single transfer.
> +		 */
> +		if (ddata->addr_prev == address)
> +			return 0;
>  	}
> 
>  	ret = i2c_smbus_write_byte(st->client,
> 
> Compared to Meng Li's patch this doesn't introduce reporting of bogus
> conversion results once we implement bulk conversion.
> 

Ok!
Understand.
I agree you to push patch to upstream and it is more reasonable that the  original author to fix this issue.

Thanks,
Limeng

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Jonathan Cameron]

On Fri, 4 Jun 2021 06:43:20 +0000
"Li, Meng" <Meng.Li@windriver.com> wrote:

> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: Friday, June 4, 2021 2:13 PM
> > To: Li, Meng <Meng.Li@windriver.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de;
> > Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> > conversion value
> > 
> > Hello,
> > 
> > On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:  
> > >
> > >  
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Friday, June 4, 2021 12:20 AM
> > > > To: Li, Meng <Meng.Li@windriver.com>
> > > > Cc: lars@metafoo.de; Michael.Hennerich@analog.com;
> > > > pmeerw@pmeerw.net; u.kleine-koenig@pengutronix.de; linux-
> > > > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > > > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after
> > > > reading the adc conversion value
> > > >
> > > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > > >
> > > > On Tue,  1 Jun 2021 17:28:05 +0800
> > > > Meng.Li@windriver.com wrote:
> > > >  
> > > > > From: Meng Li <Meng.Li@windriver.com>
> > > > >
> > > > > When read adc conversion value with below command:
> > > > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > > > There is an error reported as below:
> > > > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > > > > issue is introduced by commit 69548b7c2c4f ("iio:
> > > > > adc: ltc2497: split protocol independent part in a separate module").
> > > > > When extract the common code into ltc2497-core.c, it change the
> > > > > code logic of function ltc2497core_read(). With wrong reading
> > > > > sequence, the action of enable adc channel is sent to chip again
> > > > > during adc channel is in conversion status. In this way, there is
> > > > > no ack from chip, and then cause i2c transfer failed.
> > > > > In order to keep the code logic is the same with original ideal,
> > > > > it is need to return direct after reading the adc conversion value.  
> > 
> > As background about the choice of the .result_and_measure callback:
> > A difference between the ltc2497 (i2c) and ltc2496 (spi) is that for the latter
> > reading the result of the last conversion and starting a new is a single bus
> > operation and the one cannot be done without the other.
> >   
> > > > > v2:
> > > > > According to ltc2497 datasheet, the max value of conversion time
> > > > > is
> > > > > 149.9 ms. So, add 20% to the 150msecs so that there is enough time
> > > > > for data conversion.  
> > > >
> > > > Version change logs go below the --- as we don't want to preserve
> > > > them forever in the git history.
> > > >
> > > > I may have lost track of the discussion, but I thought the idea was
> > > > that perhaps the longer time period would remove the need for the early  
> > return?  
> > > >  
> > >
> > > No!
> > > I think the ret is essential.  
> > 
> > I'd like to understand why. Currently ltc2497core_read() looks as follows
> > (simplified by dropping error handling, and unrolling the result_and_measure
> > callback for the i2c case):
> > 
> > 	ltc2497core_wait_conv()
> > 
> > 	// result_and_measure(address, NULL)
> > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > 
> > 	msleep_interruptible(LTC2497_CONVERSION_TIME_MS)
> > 
> > 	// result_and_measure(address, val);
> > 	i2c_master_recv(client, &buf, 3);
> > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > 
> > 
> > With the early return you suggest to introduce with your patch you save the
> > last i2c_smbus_write_byte call. The data sheet indeed claims to start a new
> > conversion at the stop condition.
> > 
> > So either the reading of the conversion result and programming of the
> > (maybe) new address has to be done in a single i2c transfer, or we have to do
> > something like your patch.
> > 
> > What I don't like about your approach is that it changes the semantic of the
> > callback to result_*or*_measure which is something the spi variant cannot
> > implement. With the current use of the function this is fine, however if at
> > some time in the future we implement a bulk conversion shortcut this hurts.
> > 
> > So I suggest to do:
> >   
> > ---->8----  
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Date: Fri, 4 Jun 2021 08:02:44 +0200
> > Subject: [PATCH] iio: ltc2497: Fix reading conversion results
> > 
> > After the result of the previous conversion is read the chip automatically
> > starts a new conversion and doesn't accept new i2c transfers until this
> > conversion is completed which makes the function return failure.
> > 
> > So add an early return iff the programming of the new address isn't needed.
> > Note this will not fix the problem in general, but all cases that are currently
> > used. Once this changes we get the failure back, but this can be addressed
> > when the need arises.
> > 
> > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a
> > separate module ")
> > Reported-by: Meng Li <Meng.Li@windriver.com>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/iio/adc/ltc2497.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > --- a/drivers/iio/adc/ltc2497.c
> > +++ b/drivers/iio/adc/ltc2497.c
> > @@ -41,6 +41,19 @@ static int ltc2497_result_and_measure(struct
> > ltc2497core_driverdata *ddata,
> >  		}
> > 
> >  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > +
> > +		/*
> > +		 * The part started a new conversion at the end of the above
> > i2c
> > +		 * transfer, so if the address didn't change since the last call
> > +		 * everything is fine and we can return early.
> > +		 * If not (which should only happen when some sort of bulk
> > +		 * conversion is implemented) we have to program the new
> > +		 * address. Note that this probably fails as the conversion
> > that
> > +		 * was triggered above is like not complete yet and the two
> > +		 * operations have to be done in a single transfer.
> > +		 */

I'm a little confused by this comment.  It seems to say it will fail if we
ever do have a different address?  That doesn't seem very helpful...

J
> > +		if (ddata->addr_prev == address)
> > +			return 0;
> >  	}
> > 
> >  	ret = i2c_smbus_write_byte(st->client,
> > 
> > Compared to Meng Li's patch this doesn't introduce reporting of bogus
> > conversion results once we implement bulk conversion.
> >   
> 
> Ok!
> Understand.
> I agree you to push patch to upstream and it is more reasonable that the  original author to fix this issue.
> 
> Thanks,
> Limeng
> 
> > Best regards
> > Uwe
> > 
> > --
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 | https://www.pengutronix.de/ |  

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8995 bytes --]

On Fri, Jun 04, 2021 at 09:20:43AM +0100, Jonathan Cameron wrote:
> On Fri, 4 Jun 2021 06:43:20 +0000
> "Li, Meng" <Meng.Li@windriver.com> wrote:
> 
> > > -----Original Message-----
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Sent: Friday, June 4, 2021 2:13 PM
> > > To: Li, Meng <Meng.Li@windriver.com>
> > > Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de;
> > > Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> > > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> > > conversion value
> > > 
> > > Hello,
> > > 
> > > On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:  
> > > >
> > > >  
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Friday, June 4, 2021 12:20 AM
> > > > > To: Li, Meng <Meng.Li@windriver.com>
> > > > > Cc: lars@metafoo.de; Michael.Hennerich@analog.com;
> > > > > pmeerw@pmeerw.net; u.kleine-koenig@pengutronix.de; linux-
> > > > > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > > > > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after
> > > > > reading the adc conversion value
> > > > >
> > > > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > > > >
> > > > > On Tue,  1 Jun 2021 17:28:05 +0800
> > > > > Meng.Li@windriver.com wrote:
> > > > >  
> > > > > > From: Meng Li <Meng.Li@windriver.com>
> > > > > >
> > > > > > When read adc conversion value with below command:
> > > > > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > > > > There is an error reported as below:
> > > > > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > > > > > issue is introduced by commit 69548b7c2c4f ("iio:
> > > > > > adc: ltc2497: split protocol independent part in a separate module").
> > > > > > When extract the common code into ltc2497-core.c, it change the
> > > > > > code logic of function ltc2497core_read(). With wrong reading
> > > > > > sequence, the action of enable adc channel is sent to chip again
> > > > > > during adc channel is in conversion status. In this way, there is
> > > > > > no ack from chip, and then cause i2c transfer failed.
> > > > > > In order to keep the code logic is the same with original ideal,
> > > > > > it is need to return direct after reading the adc conversion value.  
> > > 
> > > As background about the choice of the .result_and_measure callback:
> > > A difference between the ltc2497 (i2c) and ltc2496 (spi) is that for the latter
> > > reading the result of the last conversion and starting a new is a single bus
> > > operation and the one cannot be done without the other.
> > >   
> > > > > > v2:
> > > > > > According to ltc2497 datasheet, the max value of conversion time
> > > > > > is
> > > > > > 149.9 ms. So, add 20% to the 150msecs so that there is enough time
> > > > > > for data conversion.  
> > > > >
> > > > > Version change logs go below the --- as we don't want to preserve
> > > > > them forever in the git history.
> > > > >
> > > > > I may have lost track of the discussion, but I thought the idea was
> > > > > that perhaps the longer time period would remove the need for the early  
> > > return?  
> > > > >  
> > > >
> > > > No!
> > > > I think the ret is essential.  
> > > 
> > > I'd like to understand why. Currently ltc2497core_read() looks as follows
> > > (simplified by dropping error handling, and unrolling the result_and_measure
> > > callback for the i2c case):
> > > 
> > > 	ltc2497core_wait_conv()
> > > 
> > > 	// result_and_measure(address, NULL)
> > > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > > 
> > > 	msleep_interruptible(LTC2497_CONVERSION_TIME_MS)
> > > 
> > > 	// result_and_measure(address, val);
> > > 	i2c_master_recv(client, &buf, 3);
> > > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > > 
> > > 
> > > With the early return you suggest to introduce with your patch you save the
> > > last i2c_smbus_write_byte call. The data sheet indeed claims to start a new
> > > conversion at the stop condition.
> > > 
> > > So either the reading of the conversion result and programming of the
> > > (maybe) new address has to be done in a single i2c transfer, or we have to do
> > > something like your patch.
> > > 
> > > What I don't like about your approach is that it changes the semantic of the
> > > callback to result_*or*_measure which is something the spi variant cannot
> > > implement. With the current use of the function this is fine, however if at
> > > some time in the future we implement a bulk conversion shortcut this hurts.
> > > 
> > > So I suggest to do:
> > >   
> > > ---->8----  
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Date: Fri, 4 Jun 2021 08:02:44 +0200
> > > Subject: [PATCH] iio: ltc2497: Fix reading conversion results
> > > 
> > > After the result of the previous conversion is read the chip automatically
> > > starts a new conversion and doesn't accept new i2c transfers until this
> > > conversion is completed which makes the function return failure.
> > > 
> > > So add an early return iff the programming of the new address isn't needed.
> > > Note this will not fix the problem in general, but all cases that are currently
> > > used. Once this changes we get the failure back, but this can be addressed
> > > when the need arises.
> > > 
> > > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a
> > > separate module ")
> > > Reported-by: Meng Li <Meng.Li@windriver.com>
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/iio/adc/ltc2497.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > > --- a/drivers/iio/adc/ltc2497.c
> > > +++ b/drivers/iio/adc/ltc2497.c
> > > @@ -41,6 +41,19 @@ static int ltc2497_result_and_measure(struct
> > > ltc2497core_driverdata *ddata,
> > >  		}
> > > 
> > >  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > > +
> > > +		/*
> > > +		 * The part started a new conversion at the end of the above i2c
> > > +		 * transfer, so if the address didn't change since the last call
> > > +		 * everything is fine and we can return early.
> > > +		 * If not (which should only happen when some sort of bulk
> > > +		 * conversion is implemented) we have to program the new
> > > +		 * address. Note that this probably fails as the conversion that
> > > +		 * was triggered above is like not complete yet and the two
> > > +		 * operations have to be done in a single transfer.
> > > +		 */
> 
> I'm a little confused by this comment.  It seems to say it will fail if we
> ever do have a different address?  That doesn't seem very helpful...

It's not a real problem in the sense that it triggers today. If you want
to read out (say) the channels 1, 5, 6 and 7, you could do:

	start conversion for channel 1
	wait for the conversion to complete
	read out conversion for channel 1 and start channel 5
	wait for the conversion to complete
	read out conversion for channel 5 and start channel 6
	wait for the conversion to complete
	read out conversion for channel 6 and start channel 7
	wait for the conversion to complete
	read out conversion for channel 7

With this procedure the step "read out conversion for channel X and
start channel Y" has to (and can) be done in a single transfer. But the
status quo is, that when these channels are to be read the following
happens:

	start conversion for channel 1
	wait for the conversion to complete
	read out conversion for channel 1 and (implicitly) start another conversion for channel 1

	wait for the conversion to complete

	start conversion for channel 5
	wait for the conversion to complete
	read out conversion for channel 5 and (implicitly) start another conversion for channel 5

	wait for the conversion to complete

	...

and ltc2497_result_and_measure is well suited to handle this.

So maybe reword the comment to:

	The part started a new conversion at the end of the above i2c
	transfer. With the current implementation of how reading is
	implemented in ltc2497core it never happens that this new
	conversion should be done for a different channel which would
	require writing a new channel address. (Actually writing such a
	new address requires more effort, either another delay must be
	added or the now two transfers must be combined into a single
	one.)

	So check the assumption that the channel really didn't change
	and then return early which does the right thing today.

?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Li, Meng]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Friday, June 4, 2021 4:53 PM
> To: Jonathan Cameron <jic23@kernel.org>
> Cc: Li, Meng <Meng.Li@windriver.com>; lars@metafoo.de;
> Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> kernel@vger.kernel.org; linux-iio@vger.kernel.org
> Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> conversion value
> 
> On Fri, Jun 04, 2021 at 09:20:43AM +0100, Jonathan Cameron wrote:
> > On Fri, 4 Jun 2021 06:43:20 +0000
> > "Li, Meng" <Meng.Li@windriver.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Sent: Friday, June 4, 2021 2:13 PM
> > > > To: Li, Meng <Meng.Li@windriver.com>
> > > > Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de;
> > > > Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> > > > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > > > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after
> > > > reading the adc conversion value
> > > >
> > > > Hello,
> > > >
> > > > On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Friday, June 4, 2021 12:20 AM
> > > > > > To: Li, Meng <Meng.Li@windriver.com>
> > > > > > Cc: lars@metafoo.de; Michael.Hennerich@analog.com;
> > > > > > pmeerw@pmeerw.net; u.kleine-koenig@pengutronix.de; linux-
> > > > > > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > > > > > Subject: Re: [PATCH] driver: adc: ltc2497: return directly
> > > > > > after reading the adc conversion value
> > > > > >
> > > > > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > > > > >
> > > > > > On Tue,  1 Jun 2021 17:28:05 +0800 Meng.Li@windriver.com
> > > > > > wrote:
> > > > > >
> > > > > > > From: Meng Li <Meng.Li@windriver.com>
> > > > > > >
> > > > > > > When read adc conversion value with below command:
> > > > > > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > > > > > There is an error reported as below:
> > > > > > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c
> > > > > > > transfer issue is introduced by commit 69548b7c2c4f ("iio:
> > > > > > > adc: ltc2497: split protocol independent part in a separate
> module").
> > > > > > > When extract the common code into ltc2497-core.c, it change
> > > > > > > the code logic of function ltc2497core_read(). With wrong
> > > > > > > reading sequence, the action of enable adc channel is sent
> > > > > > > to chip again during adc channel is in conversion status. In
> > > > > > > this way, there is no ack from chip, and then cause i2c transfer
> failed.
> > > > > > > In order to keep the code logic is the same with original
> > > > > > > ideal, it is need to return direct after reading the adc conversion
> value.
> > > >
> > > > As background about the choice of the .result_and_measure callback:
> > > > A difference between the ltc2497 (i2c) and ltc2496 (spi) is that
> > > > for the latter reading the result of the last conversion and
> > > > starting a new is a single bus operation and the one cannot be done
> without the other.
> > > >
> > > > > > > v2:
> > > > > > > According to ltc2497 datasheet, the max value of conversion
> > > > > > > time is
> > > > > > > 149.9 ms. So, add 20% to the 150msecs so that there is
> > > > > > > enough time for data conversion.
> > > > > >
> > > > > > Version change logs go below the --- as we don't want to
> > > > > > preserve them forever in the git history.
> > > > > >
> > > > > > I may have lost track of the discussion, but I thought the
> > > > > > idea was that perhaps the longer time period would remove the
> > > > > > need for the early
> > > > return?
> > > > > >
> > > > >
> > > > > No!
> > > > > I think the ret is essential.
> > > >
> > > > I'd like to understand why. Currently ltc2497core_read() looks as
> > > > follows (simplified by dropping error handling, and unrolling the
> > > > result_and_measure callback for the i2c case):
> > > >
> > > > 	ltc2497core_wait_conv()
> > > >
> > > > 	// result_and_measure(address, NULL)
> > > > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > > >
> > > > 	msleep_interruptible(LTC2497_CONVERSION_TIME_MS)
> > > >
> > > > 	// result_and_measure(address, val);
> > > > 	i2c_master_recv(client, &buf, 3);
> > > > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > > >
> > > >
> > > > With the early return you suggest to introduce with your patch you
> > > > save the last i2c_smbus_write_byte call. The data sheet indeed
> > > > claims to start a new conversion at the stop condition.
> > > >
> > > > So either the reading of the conversion result and programming of
> > > > the
> > > > (maybe) new address has to be done in a single i2c transfer, or we
> > > > have to do something like your patch.
> > > >
> > > > What I don't like about your approach is that it changes the
> > > > semantic of the callback to result_*or*_measure which is something
> > > > the spi variant cannot implement. With the current use of the
> > > > function this is fine, however if at some time in the future we
> implement a bulk conversion shortcut this hurts.
> > > >
> > > > So I suggest to do:
> > > >
> > > > ---->8----
> > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Date: Fri, 4 Jun 2021 08:02:44 +0200
> > > > Subject: [PATCH] iio: ltc2497: Fix reading conversion results
> > > >
> > > > After the result of the previous conversion is read the chip
> > > > automatically starts a new conversion and doesn't accept new i2c
> > > > transfers until this conversion is completed which makes the function
> return failure.
> > > >
> > > > So add an early return iff the programming of the new address isn't
> needed.
> > > > Note this will not fix the problem in general, but all cases that
> > > > are currently used. Once this changes we get the failure back, but
> > > > this can be addressed when the need arises.
> > > >
> > > > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol
> > > > independent part in a separate module ")
> > > > Reported-by: Meng Li <Meng.Li@windriver.com>
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/iio/adc/ltc2497.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > > > --- a/drivers/iio/adc/ltc2497.c
> > > > +++ b/drivers/iio/adc/ltc2497.c
> > > > @@ -41,6 +41,19 @@ static int ltc2497_result_and_measure(struct
> > > > ltc2497core_driverdata *ddata,
> > > >  		}
> > > >
> > > >  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > > > +
> > > > +		/*
> > > > +		 * The part started a new conversion at the end of the above
> i2c
> > > > +		 * transfer, so if the address didn't change since the last call
> > > > +		 * everything is fine and we can return early.
> > > > +		 * If not (which should only happen when some sort of bulk
> > > > +		 * conversion is implemented) we have to program the new
> > > > +		 * address. Note that this probably fails as the conversion
> that
> > > > +		 * was triggered above is like not complete yet and the two
> > > > +		 * operations have to be done in a single transfer.
> > > > +		 */
> >
> > I'm a little confused by this comment.  It seems to say it will fail
> > if we ever do have a different address?  That doesn't seem very helpful...
> 
> It's not a real problem in the sense that it triggers today. If you want to read
> out (say) the channels 1, 5, 6 and 7, you could do:
> 
> 	start conversion for channel 1
> 	wait for the conversion to complete
> 	read out conversion for channel 1 and start channel 5
> 	wait for the conversion to complete
> 	read out conversion for channel 5 and start channel 6
> 	wait for the conversion to complete
> 	read out conversion for channel 6 and start channel 7
> 	wait for the conversion to complete
> 	read out conversion for channel 7
> 

 Have you tested above case on real hardware? Or only a inference based on data sheet?

Thanks.
Limeng

> With this procedure the step "read out conversion for channel X and start
> channel Y" has to (and can) be done in a single transfer. But the status quo is,
> that when these channels are to be read the following
> happens:
> 
> 	start conversion for channel 1
> 	wait for the conversion to complete
> 	read out conversion for channel 1 and (implicitly) start another
> conversion for channel 1
> 
> 	wait for the conversion to complete
> 
> 	start conversion for channel 5
> 	wait for the conversion to complete
> 	read out conversion for channel 5 and (implicitly) start another
> conversion for channel 5
> 
> 	wait for the conversion to complete
> 
> 	...
> 
> and ltc2497_result_and_measure is well suited to handle this.
> 
> So maybe reword the comment to:
> 
> 	The part started a new conversion at the end of the above i2c
> 	transfer. With the current implementation of how reading is
> 	implemented in ltc2497core it never happens that this new
> 	conversion should be done for a different channel which would
> 	require writing a new channel address. (Actually writing such a
> 	new address requires more effort, either another delay must be
> 	added or the now two transfers must be combined into a single
> 	one.)
> 
> 	So check the assumption that the channel really didn't change
> 	and then return early which does the right thing today.
> 
> ?
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5324 bytes --]

On Fri, Jun 04, 2021 at 09:04:48AM +0000, Li, Meng wrote:
> > On Fri, Jun 04, 2021 at 09:20:43AM +0100, Jonathan Cameron wrote:
> > > On Fri, 4 Jun 2021 06:43:20 +0000
> > > "Li, Meng" <Meng.Li@windriver.com> wrote:
> > > > > On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:
> > > > > > I think the ret is essential.
> > > > >
> > > > > I'd like to understand why. Currently ltc2497core_read() looks as
> > > > > follows (simplified by dropping error handling, and unrolling the
> > > > > result_and_measure callback for the i2c case):
> > > > >
> > > > > 	ltc2497core_wait_conv()
> > > > >
> > > > > 	// result_and_measure(address, NULL)
> > > > > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > > > >
> > > > > 	msleep_interruptible(LTC2497_CONVERSION_TIME_MS)
> > > > >
> > > > > 	// result_and_measure(address, val);
> > > > > 	i2c_master_recv(client, &buf, 3);
> > > > > 	i2c_smbus_write_byte(client, LTC2497_ENABLE | address);
> > > > >
> > > > >
> > > > > With the early return you suggest to introduce with your patch you
> > > > > save the last i2c_smbus_write_byte call. The data sheet indeed
> > > > > claims to start a new conversion at the stop condition.
> > > > >
> > > > > So either the reading of the conversion result and programming of
> > > > > the
> > > > > (maybe) new address has to be done in a single i2c transfer, or we
> > > > > have to do something like your patch.
> > > > >
> > > > > What I don't like about your approach is that it changes the
> > > > > semantic of the callback to result_*or*_measure which is something
> > > > > the spi variant cannot implement. With the current use of the
> > > > > function this is fine, however if at some time in the future we
> > > > > implement a bulk conversion shortcut this hurts.
> > > > >
> > > > > So I suggest to do:
> > > > >
> > > > > ---->8----
> > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > Date: Fri, 4 Jun 2021 08:02:44 +0200
> > > > > Subject: [PATCH] iio: ltc2497: Fix reading conversion results
> > > > >
> > > > > After the result of the previous conversion is read the chip
> > > > > automatically starts a new conversion and doesn't accept new i2c
> > > > > transfers until this conversion is completed which makes the function return failure.
> > > > >
> > > > > So add an early return iff the programming of the new address isn't needed.
> > > > > Note this will not fix the problem in general, but all cases that
> > > > > are currently used. Once this changes we get the failure back, but
> > > > > this can be addressed when the need arises.
> > > > >
> > > > > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol
> > > > > independent part in a separate module ")
> > > > > Reported-by: Meng Li <Meng.Li@windriver.com>
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  drivers/iio/adc/ltc2497.c | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > > > > --- a/drivers/iio/adc/ltc2497.c
> > > > > +++ b/drivers/iio/adc/ltc2497.c
> > > > > @@ -41,6 +41,19 @@ static int ltc2497_result_and_measure(struct
> > > > > ltc2497core_driverdata *ddata,
> > > > >  		}
> > > > >
> > > > >  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > > > > +
> > > > > +		/*
> > > > > +		 * The part started a new conversion at the end of the above i2c
> > > > > +		 * transfer, so if the address didn't change since the last call
> > > > > +		 * everything is fine and we can return early.
> > > > > +		 * If not (which should only happen when some sort of bulk
> > > > > +		 * conversion is implemented) we have to program the new
> > > > > +		 * address. Note that this probably fails as the conversion that
> > > > > +		 * was triggered above is like not complete yet and the two
> > > > > +		 * operations have to be done in a single transfer.
> > > > > +		 */
> > >
> > > I'm a little confused by this comment.  It seems to say it will fail
> > > if we ever do have a different address?  That doesn't seem very helpful...
> > 
> > It's not a real problem in the sense that it triggers today. If you want to read
> > out (say) the channels 1, 5, 6 and 7, you could do:
> > 
> > 	start conversion for channel 1
> > 	wait for the conversion to complete
> > 	read out conversion for channel 1 and start channel 5
> > 	wait for the conversion to complete
> > 	read out conversion for channel 5 and start channel 6
> > 	wait for the conversion to complete
> > 	read out conversion for channel 6 and start channel 7
> > 	wait for the conversion to complete
> > 	read out conversion for channel 7
> 
> Have you tested above case on real hardware? Or only a inference based on data sheet?

I never had the ltc2497 in my hands, but the customer I created my
patches for uses the ltc2496 (i.e. its spi variant) and did the above
from userspace using spidev.

And I'm confident enough to claim that if your patch fixes your problem,
then mine does it, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Meng.Li]

From: Meng Li <Meng.Li@windriver.com>

When read adc conversion value with below command:
cat /sys/.../iio:device0/in_voltage0-voltage1_raw
There is an error reported as below:
ltc2497 0-0014: i2c transfer failed: -EREMOTEIO
This i2c transfer issue is introduced by commit 69548b7c2c4f ("iio:
adc: ltc2497: split protocol independent part in a separate module").
When extract the common code into ltc2497-core.c, it change the
code logic of function ltc2497core_read(). With wrong reading
sequence, the action of enable adc channel is sent to chip again
during adc channel is in conversion status. In this way, there is
no ack from chip, and then cause i2c transfer failed.
In order to keep the code logic is the same with original ideal,
it is need to return direct after reading the adc conversion value.

Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
Cc: stable@vger.kernel.org
Signed-off-by: Meng Li <Meng.Li@windriver.com>
---
 drivers/iio/adc/ltc2497.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
index 1adddf5a88a9..fd5a66860a47 100644
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 		}
 
 		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
+
+		return ret;
 	}
 
 	ret = i2c_smbus_write_byte(st->client,
-- 
2.17.1

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Jonathan Cameron]

On Wed, 12 May 2021 12:57:25 +0800
Meng.Li@windriver.com wrote:

> From: Meng Li <Meng.Li@windriver.com>
> 
> When read adc conversion value with below command:
> cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> There is an error reported as below:
> ltc2497 0-0014: i2c transfer failed: -EREMOTEIO
> This i2c transfer issue is introduced by commit 69548b7c2c4f ("iio:
> adc: ltc2497: split protocol independent part in a separate module").
> When extract the common code into ltc2497-core.c, it change the
> code logic of function ltc2497core_read(). With wrong reading
> sequence, the action of enable adc channel is sent to chip again
> during adc channel is in conversion status. In this way, there is
> no ack from chip, and then cause i2c transfer failed.
> In order to keep the code logic is the same with original ideal,
> it is need to return direct after reading the adc conversion value.
> 
> Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
> Cc: stable@vger.kernel.org
> Signed-off-by: Meng Li <Meng.Li@windriver.com>

@Uwe,  I'm far from sure looking at the code what the intent is here.
whilst it definitely changed as Meng Li has outlined, I'm not sure
the fix is correct.

Jonathan

> ---
>  drivers/iio/adc/ltc2497.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> index 1adddf5a88a9..fd5a66860a47 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>  		}
>  
>  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> +
> +		return ret;
>  	}
>  
>  	ret = i2c_smbus_write_byte(st->client,

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com wrote:
> From: Meng Li <Meng.Li@windriver.com>
> 
> When read adc conversion value with below command:
> cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> There is an error reported as below:
> ltc2497 0-0014: i2c transfer failed: -EREMOTEIO
> This i2c transfer issue is introduced by commit 69548b7c2c4f ("iio:
> adc: ltc2497: split protocol independent part in a separate module").
> When extract the common code into ltc2497-core.c, it change the
> code logic of function ltc2497core_read(). With wrong reading
> sequence, the action of enable adc channel is sent to chip again
> during adc channel is in conversion status. In this way, there is
> no ack from chip, and then cause i2c transfer failed.
> In order to keep the code logic is the same with original ideal,
> it is need to return direct after reading the adc conversion value.
> 
> Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
> Cc: stable@vger.kernel.org
> Signed-off-by: Meng Li <Meng.Li@windriver.com>
> ---
>  drivers/iio/adc/ltc2497.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> index 1adddf5a88a9..fd5a66860a47 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>  		}
>  
>  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> +
> +		return ret;

This looks wrong for me. The idea of the function
ltc2497_result_and_measure is that it reads the result and starts a new
measurement. I guess the problem is that ltc2497_result_and_measure is
called to early, not that it does too much.

But note I don't have such a system handy to actually debug this any
more.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Jonathan Cameron]

On Wed, 19 May 2021 11:21:04 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com wrote:
> > From: Meng Li <Meng.Li@windriver.com>
> > 
> > When read adc conversion value with below command:
> > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > There is an error reported as below:
> > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO
> > This i2c transfer issue is introduced by commit 69548b7c2c4f ("iio:
> > adc: ltc2497: split protocol independent part in a separate module").
> > When extract the common code into ltc2497-core.c, it change the
> > code logic of function ltc2497core_read(). With wrong reading
> > sequence, the action of enable adc channel is sent to chip again
> > during adc channel is in conversion status. In this way, there is
> > no ack from chip, and then cause i2c transfer failed.
> > In order to keep the code logic is the same with original ideal,
> > it is need to return direct after reading the adc conversion value.
> > 
> > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent part in a separate module ")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Meng Li <Meng.Li@windriver.com>
> > ---
> >  drivers/iio/adc/ltc2497.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > index 1adddf5a88a9..fd5a66860a47 100644
> > --- a/drivers/iio/adc/ltc2497.c
> > +++ b/drivers/iio/adc/ltc2497.c
> > @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
> >  		}
> >  
> >  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > +
> > +		return ret;  
> 
> This looks wrong for me. The idea of the function
> ltc2497_result_and_measure is that it reads the result and starts a new
> measurement. I guess the problem is that ltc2497_result_and_measure is
> called to early, not that it does too much.
> 
> But note I don't have such a system handy to actually debug this any
> more.

@Meng Li,

I see from the datasheet that the device can be used with an external oscillator.
Is that the case on your boards, because if so the timing delay of 150msecs may
be far too short.  If not, perhaps the part is right at the upper end of
timings and we just need to add 20% to the 150msecs to be sure of not
hitting the limit?

Thanks,

Jonathan


> 
> Best regards
> Uwe
> 

RE: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Li, Meng]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, May 22, 2021 1:02 AM
> To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Li, Meng <Meng.Li@windriver.com>; lars@metafoo.de;
> Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> kernel@vger.kernel.org; linux-iio@vger.kernel.org
> Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> conversion value
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Wed, 19 May 2021 11:21:04 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com
> wrote:
> > > From: Meng Li <Meng.Li@windriver.com>
> > >
> > > When read adc conversion value with below command:
> > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > There is an error reported as below:
> > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > > issue is introduced by commit 69548b7c2c4f ("iio:
> > > adc: ltc2497: split protocol independent part in a separate module").
> > > When extract the common code into ltc2497-core.c, it change the code
> > > logic of function ltc2497core_read(). With wrong reading sequence,
> > > the action of enable adc channel is sent to chip again during adc
> > > channel is in conversion status. In this way, there is no ack from
> > > chip, and then cause i2c transfer failed.
> > > In order to keep the code logic is the same with original ideal, it
> > > is need to return direct after reading the adc conversion value.
> > >
> > > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent
> > > part in a separate module ")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Meng Li <Meng.Li@windriver.com>
> > > ---
> > >  drivers/iio/adc/ltc2497.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > > index 1adddf5a88a9..fd5a66860a47 100644
> > > --- a/drivers/iio/adc/ltc2497.c
> > > +++ b/drivers/iio/adc/ltc2497.c
> > > @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct
> ltc2497core_driverdata *ddata,
> > >             }
> > >
> > >             *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > > +
> > > +           return ret;
> >
> > This looks wrong for me. The idea of the function
> > ltc2497_result_and_measure is that it reads the result and starts a
> > new measurement. I guess the problem is that
> > ltc2497_result_and_measure is called to early, not that it does too much.
> >
> > But note I don't have such a system handy to actually debug this any
> > more.
> 
> @Meng Li,
> 
> I see from the datasheet that the device can be used with an external
> oscillator.
> Is that the case on your boards, because if so the timing delay of 150msecs
> may be far too short.  If not, perhaps the part is right at the upper end of
> timings and we just need to add 20% to the 150msecs to be sure of not
> hitting the limit?
> 

Hi Jonathan,

Thanks for your very professional comments.
I check my board schematics, the pin 35 f0 is connected to GND, so I use the internal oscillator.
In additional, I am not very understand your comment about the case of using internal oscillator.
Do you mean that you agree my patch and only need to change 150 into 180?
#define LTC2497_CONVERSION_TIME_MS	150ULL
=>
#define LTC2497_CONVERSION_TIME_MS	180ULL

Thanks,
Limeng

> Thanks,
> 
> Jonathan
> 
> 
> >
> > Best regards
> > Uwe
> >

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Jonathan Cameron]

On Mon, 24 May 2021 02:49:01 +0000
"Li, Meng" <Meng.Li@windriver.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, May 22, 2021 1:02 AM
> > To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Li, Meng <Meng.Li@windriver.com>; lars@metafoo.de;
> > Michael.Hennerich@analog.com; pmeerw@pmeerw.net; linux-
> > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> > conversion value
> > 
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On Wed, 19 May 2021 11:21:04 +0200
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >   
> > > On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com  
> > wrote:  
> > > > From: Meng Li <Meng.Li@windriver.com>
> > > >
> > > > When read adc conversion value with below command:
> > > > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > > > There is an error reported as below:
> > > > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > > > issue is introduced by commit 69548b7c2c4f ("iio:
> > > > adc: ltc2497: split protocol independent part in a separate module").
> > > > When extract the common code into ltc2497-core.c, it change the code
> > > > logic of function ltc2497core_read(). With wrong reading sequence,
> > > > the action of enable adc channel is sent to chip again during adc
> > > > channel is in conversion status. In this way, there is no ack from
> > > > chip, and then cause i2c transfer failed.
> > > > In order to keep the code logic is the same with original ideal, it
> > > > is need to return direct after reading the adc conversion value.
> > > >
> > > > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent
> > > > part in a separate module ")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Meng Li <Meng.Li@windriver.com>
> > > > ---
> > > >  drivers/iio/adc/ltc2497.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > > > index 1adddf5a88a9..fd5a66860a47 100644
> > > > --- a/drivers/iio/adc/ltc2497.c
> > > > +++ b/drivers/iio/adc/ltc2497.c
> > > > @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct  
> > ltc2497core_driverdata *ddata,  
> > > >             }
> > > >
> > > >             *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > > > +
> > > > +           return ret;  
> > >
> > > This looks wrong for me. The idea of the function
> > > ltc2497_result_and_measure is that it reads the result and starts a
> > > new measurement. I guess the problem is that
> > > ltc2497_result_and_measure is called to early, not that it does too much.
> > >
> > > But note I don't have such a system handy to actually debug this any
> > > more.  
> > 
> > @Meng Li,
> > 
> > I see from the datasheet that the device can be used with an external
> > oscillator.
> > Is that the case on your boards, because if so the timing delay of 150msecs
> > may be far too short.  If not, perhaps the part is right at the upper end of
> > timings and we just need to add 20% to the 150msecs to be sure of not
> > hitting the limit?
> >   
> 
> Hi Jonathan,
> 
> Thanks for your very professional comments.
> I check my board schematics, the pin 35 f0 is connected to GND, so I use the internal oscillator.

Bad guess :( 

> In additional, I am not very understand your comment about the case of using internal oscillator.
> Do you mean that you agree my patch and only need to change 150 into 180?
> #define LTC2497_CONVERSION_TIME_MS	150ULL
> =>  
> #define LTC2497_CONVERSION_TIME_MS	180ULL

Yes.  Just give a little more time in case the oscillator is running
a little slow.

Jonathan

> 
> Thanks,
> Limeng
> 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> > >
> > > Best regards
> > > Uwe
> > >  
> 

RE: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Li, Meng]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Wednesday, May 19, 2021 5:21 PM
> To: Li, Meng <Meng.Li@windriver.com>
> Cc: lars@metafoo.de; Michael.Hennerich@analog.com; jic23@kernel.org;
> pmeerw@pmeerw.net; linux-kernel@vger.kernel.org; linux-
> iio@vger.kernel.org
> Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> conversion value
> 
> On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com wrote:
> > From: Meng Li <Meng.Li@windriver.com>
> >
> > When read adc conversion value with below command:
> > cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> > There is an error reported as below:
> > ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> > issue is introduced by commit 69548b7c2c4f ("iio:
> > adc: ltc2497: split protocol independent part in a separate module").
> > When extract the common code into ltc2497-core.c, it change the code
> > logic of function ltc2497core_read(). With wrong reading sequence, the
> > action of enable adc channel is sent to chip again during adc channel
> > is in conversion status. In this way, there is no ack from chip, and
> > then cause i2c transfer failed.
> > In order to keep the code logic is the same with original ideal, it is
> > need to return direct after reading the adc conversion value.
> >
> > Fixes: 69548b7c2c4f ("iio: adc: ltc2497: split protocol independent
> > part in a separate module ")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Meng Li <Meng.Li@windriver.com>
> > ---
> >  drivers/iio/adc/ltc2497.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > index 1adddf5a88a9..fd5a66860a47 100644
> > --- a/drivers/iio/adc/ltc2497.c
> > +++ b/drivers/iio/adc/ltc2497.c
> > @@ -41,6 +41,8 @@ static int ltc2497_result_and_measure(struct
> ltc2497core_driverdata *ddata,
> >  		}
> >
> >  		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> > +
> > +		return ret;
> 
> This looks wrong for me. The idea of the function
> ltc2497_result_and_measure is that it reads the result and starts a new
> measurement. I guess the problem is that ltc2497_result_and_measure is
> called to early, not that it does too much.
> 
> But note I don't have such a system handy to actually debug this any more.
> 

Hi Uwe,

Thanks for your comments.
I would like to verify your ideal if you can offer your patch or tell me where to change code.

Thanks,
Limeng


> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Felix Knopf]

>> On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com wrote:
>>> When read adc conversion value with below command:
>>> cat /sys/.../iio:device0/in_voltage0-voltage1_raw
>>> There is an error reported as below:
>>> ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
>>> issue is introduced by commit 69548b7c2c4f ("iio:
>>> adc: ltc2497: split protocol independent part in a separate module").
>>> When extract the common code into ltc2497-core.c, it change the code
>>> logic of function ltc2497core_read(). With wrong reading sequence, the
>>> action of enable adc channel is sent to chip again during adc channel
>>> is in conversion status. In this way, there is no ack from chip, and
>>> then cause i2c transfer failed.

Hi,

I came across the same or a very similar issue with the ltc2497 but took
a different approach to solve it.  I suspect this issue is caused by a
suboptimal I2C access pattern.

The ltc2497 triggers a new conversion on the stop condition of
transactions addressed to it.  As the chip cannot communicate during a
conversion, it will not ACK until it is finished.  The current driver
produces the following sequence to read from an arbitrary channel:

ltc2497_result_and_measure(…, NULL);
1) S <ADDR> W A | <CONF> A | P    (select channel)

2) [sleep 150ms]                  (wait for conversion)

ltc2497_result_and_measure(…, val);
3) S <ADDR> R A | <data> … | P    (read data)
4) S <ADDR> W N | P               (chip is busy, error)

Transaction 3 triggers a new conversion on the previously selected
channel and causes the following channel select (4) to fail.  The
examples in the datasheet [1] make use of repeated start conditions to
prevent unintended triggers.  In our case, 3 and 4 should be combined
into one transaction.

Limeng's patch sikps 4 which solves the problem but causes issues at
high sample rates, were 1 is skipped by the core.

I attached my ad-hoc solution below.
@Limeng: Could you test this with your hardware?

If there is interest, I will prepare a proper patch.
(Should that go into a new thread then?)

Regards, Felix

[1] https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf#page=18

-- 
Felix Knopf
von Hoerner & Sulger GmbH
https://vh-s.de


diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
index 1adddf5a88a9..8968bf70859b 100644
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -34,20 +34,23 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
        int ret;
 
        if (val) {
-               ret = i2c_master_recv(st->client, (char *)&st->buf, 3);
+               ret = i2c_smbus_read_i2c_block_data(st->client,
+                                                   LTC2497_ENABLE | address, 3,
+                                                   (char *)&st->buf);
                if (ret < 0) {
-                       dev_err(&st->client->dev, "i2c_master_recv failed\n");
+                       dev_err(&st->client->dev, "i2c transfer failed\n");
                        return ret;
                }
 
                *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
+       } else {
+               ret = i2c_smbus_write_byte(st->client,
+                                          LTC2497_ENABLE | address);
+               if (ret)
+                       dev_err(&st->client->dev, "i2c write failed: %pe\n",
+                               ERR_PTR(ret));
        }
 
-       ret = i2c_smbus_write_byte(st->client,
-                                  LTC2497_ENABLE | address);
-       if (ret)
-               dev_err(&st->client->dev, "i2c transfer failed: %pe\n",
-                       ERR_PTR(ret));
        return ret;
 }

RE: [PATCH] driver: adc: ltc2497: return directly after reading the adc conversion value

[Li, Meng]

> -----Original Message-----
> From: Felix Knopf <knopf@vh-s.de>
> Sent: Tuesday, May 25, 2021 4:23 PM
> To: Li, Meng <Meng.Li@windriver.com>; Uwe Kleine-König <u.kleine-
> koenig@pengutronix.de>
> Cc: lars@metafoo.de; Michael.Hennerich@analog.com; jic23@kernel.org;
> pmeerw@pmeerw.net; linux-kernel@vger.kernel.org; linux-
> iio@vger.kernel.org
> Subject: Re: [PATCH] driver: adc: ltc2497: return directly after reading the adc
> conversion value
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> >> On Wed, May 12, 2021 at 12:57:25PM +0800, Meng.Li@windriver.com
> wrote:
> >>> When read adc conversion value with below command:
> >>> cat /sys/.../iio:device0/in_voltage0-voltage1_raw
> >>> There is an error reported as below:
> >>> ltc2497 0-0014: i2c transfer failed: -EREMOTEIO This i2c transfer
> >>> issue is introduced by commit 69548b7c2c4f ("iio:
> >>> adc: ltc2497: split protocol independent part in a separate module").
> >>> When extract the common code into ltc2497-core.c, it change the code
> >>> logic of function ltc2497core_read(). With wrong reading sequence,
> >>> the action of enable adc channel is sent to chip again during adc
> >>> channel is in conversion status. In this way, there is no ack from
> >>> chip, and then cause i2c transfer failed.
> 
> Hi,
> 
> I came across the same or a very similar issue with the ltc2497 but took a
> different approach to solve it.  I suspect this issue is caused by a suboptimal
> I2C access pattern.
> 
> The ltc2497 triggers a new conversion on the stop condition of transactions
> addressed to it.  As the chip cannot communicate during a conversion, it will
> not ACK until it is finished.  The current driver produces the following
> sequence to read from an arbitrary channel:
> 
> ltc2497_result_and_measure(…, NULL);
> 1) S <ADDR> W A | <CONF> A | P    (select channel)
> 
> 2) [sleep 150ms]                  (wait for conversion)
> 
> ltc2497_result_and_measure(…, val);
> 3) S <ADDR> R A | <data> … | P    (read data)
> 4) S <ADDR> W N | P               (chip is busy, error)
> 
> Transaction 3 triggers a new conversion on the previously selected channel
> and causes the following channel select (4) to fail.  The examples in the
> datasheet [1] make use of repeated start conditions to prevent unintended
> triggers.  In our case, 3 and 4 should be combined into one transaction.
> 
> Limeng's patch sikps 4 which solves the problem but causes issues at high
> sample rates, were 1 is skipped by the core.
> 
> I attached my ad-hoc solution below.

Hi Felix,

Thanks for your new ideal firstly. I will test your patch in later.
I had a look your patch, and I found out there is no essential difference from my patch.
You put the step 4 in the else branch, I think it is the same effective with my return solution.

In additional, about the high sample rates, you pointed out that my patch will skip the step1
But I check the ltc2497 datasheet
https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf
in page 18, there is a "Consecutive Reading with the Same Input/Configuration" mode, I think it is able to read data continuously without setting channel address every time.

Thanks,
Limeng

> @Limeng: Could you test this with your hardware?
> 
> If there is interest, I will prepare a proper patch.
> (Should that go into a new thread then?)
> 
> Regards, Felix
> 
> [1] https://www.analog.com/media/en/technical-documentation/data-
> sheets/2497fb.pdf#page=18
> 
> --
> Felix Knopf
> von Hoerner & Sulger GmbH
> https://vh-s.de
> 
> 
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c index
> 1adddf5a88a9..8968bf70859b 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -34,20 +34,23 @@ static int ltc2497_result_and_measure(struct
> ltc2497core_driverdata *ddata,
>         int ret;
> 
>         if (val) {
> -               ret = i2c_master_recv(st->client, (char *)&st->buf, 3);
> +               ret = i2c_smbus_read_i2c_block_data(st->client,
> +                                                   LTC2497_ENABLE | address, 3,
> +                                                   (char *)&st->buf);
>                 if (ret < 0) {
> -                       dev_err(&st->client->dev, "i2c_master_recv failed\n");
> +                       dev_err(&st->client->dev, "i2c transfer
> + failed\n");
>                         return ret;
>                 }
> 
>                 *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> +       } else {
> +               ret = i2c_smbus_write_byte(st->client,
> +                                          LTC2497_ENABLE | address);
> +               if (ret)
> +                       dev_err(&st->client->dev, "i2c write failed: %pe\n",
> +                               ERR_PTR(ret));
>         }
> 
> -       ret = i2c_smbus_write_byte(st->client,
> -                                  LTC2497_ENABLE | address);
> -       if (ret)
> -               dev_err(&st->client->dev, "i2c transfer failed: %pe\n",
> -                       ERR_PTR(ret));
>         return ret;
>  }