Message ID | 20210512045725.23390-1-Meng.Li@windriver.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | driver: adc: ltc2497: return directly after reading the adc conversion value | expand |
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,
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
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 >
> -----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 > >
> -----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/ |
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 > > > >
>> 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
> -----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; > }
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,