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

[Uwe Kleine-König]

Hello,

On Fri, Jun 04, 2021 at 02:16:39AM +0000, Li, Meng wrote:
> 
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Sent: Friday, June 4, 2021 12:20 AM
> > To: Li, Meng <Meng.Li@xxxxxxxxxxxxx>
> > Cc: lars@xxxxxxxxxx; Michael.Hennerich@xxxxxxxxxx;
> > pmeerw@xxxxxxxxxx; u.kleine-koenig@xxxxxxxxxxxxxx; 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 Tue,  1 Jun 2021 17:28:05 +0800
> > Meng.Li@xxxxxxxxxxxxx wrote:
> > 
> > > From: Meng Li <Meng.Li@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
---
 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: signature.asc
Description: PGP signature