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

[Li, Meng]

> -----Original Message-----
> From: Felix Knopf <knopf@xxxxxxx>
> Sent: Tuesday, May 25, 2021 4:23 PM
> To: Li, Meng <Meng.Li@xxxxxxxxxxxxx>; Uwe Kleine-König <u.kleine-
> koenig@xxxxxxxxxxxxxx>
> Cc: lars@xxxxxxxxxx; Michael.Hennerich@xxxxxxxxxx; jic23@xxxxxxxxxx;
> pmeerw@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> iio@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx
> 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;
>  }