Re: [PATCH v2] iio: adc: ad7606: remove frstdata check for serial mode

[Guillaume Stols <gstols@baylibre.com>]

On 6/23/24 17:54, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 13:45:00 +0000
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> Frstdata pin is set high during the first sample's transmission and then
>> set low.  This code chunk attempts to recover from an eventual glitch in
>> the clock by checking frstdata state after reading the first channel's
>> sample.  Currently, in serial mode, this check happens AFTER the 16th
>> pulse, and if frstdata is not set it resets the device and returns EINVAL.
>> According to the datasheet, "The FRSTDATA output returns to a logic low
>> following the 16th SCLK falling edge.", thus after the 16th pulse, the
>> check will always be true, and the driver will not work as expected.  Thus
>> it must be removed for serial mode.
> when you say will not work as expected, is this is normal circumstances, or
> when dealing with a clock glitch?  i.e. should this have a fixes tag and
> got upstream asap or is it just cleaning up a corner case and can wait for
> now?
>
> One trivial comment inline.
>
> Jonathan

It completely prevents the driver to work when adi, first-data 
(optional) is defined in the DT.

So I guess anyone having the driver working with the serial interface 
right now did not define frstdata in the DT.

However, for someone new that sets adi, first-data in the DT, it is not 
very straightforward to spot where the issue is.

>
>> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
>> ---
>> Changes in v2:
>>   - Remove frstdata check only for the serial interface as suggested by
>>     Michael Hennerich.
>>   - Link to v1: https://lore.kernel.org/r/20240417-cleanup-ad7606-v1-1-5c2a29662c0a@baylibre.com
>> ---
>>   drivers/iio/adc/ad7606.c     | 28 ++-----------------------
>>   drivers/iio/adc/ad7606.h     |  2 ++
>>   drivers/iio/adc/ad7606_par.c | 49 +++++++++++++++++++++++++++++++++++++++++---
>>   3 files changed, 50 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
>> index 3a417595294f..c321c6ef48df 100644
>> --- a/drivers/iio/adc/ad7606.c
>> +++ b/drivers/iio/adc/ad7606.c
>> @@ -49,7 +49,7 @@ static const unsigned int ad7616_oversampling_avail[8] = {
>>   	1, 2, 4, 8, 16, 32, 64, 128,
>>   };
>>   
>> -static int ad7606_reset(struct ad7606_state *st)
>> +int ad7606_reset(struct ad7606_state *st)
>>   {
>>   	if (st->gpio_reset) {
>>   		gpiod_set_value(st->gpio_reset, 1);
>> @@ -60,6 +60,7 @@ static int ad7606_reset(struct ad7606_state *st)
>>   
>>   	return -ENODEV;
>>   }
>> +EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
>>   
>>   static int ad7606_reg_access(struct iio_dev *indio_dev,
>>   			     unsigned int reg,
>> @@ -88,31 +89,6 @@ static int ad7606_read_samples(struct ad7606_state *st)
>>   {
>>   	unsigned int num = st->chip_info->num_channels - 1;
>>   	u16 *data = st->data;
>> -	int ret;
>> -
>> -	/*
>> -	 * The frstdata signal is set to high while and after reading the sample
>> -	 * of the first channel and low for all other channels. This can be used
>> -	 * to check that the incoming data is correctly aligned. During normal
>> -	 * operation the data should never become unaligned, but some glitch or
>> -	 * electrostatic discharge might cause an extra read or clock cycle.
>> -	 * Monitoring the frstdata signal allows to recover from such failure
>> -	 * situations.
>> -	 */
>> -
>> -	if (st->gpio_frstdata) {
>> -		ret = st->bops->read_block(st->dev, 1, data);
>> -		if (ret)
>> -			return ret;
>> -
>> -		if (!gpiod_get_value(st->gpio_frstdata)) {
>> -			ad7606_reset(st);
>> -			return -EIO;
>> -		}
>> -
>> -		data++;
>> -		num--;
>> -	}
>>   
>>   	return st->bops->read_block(st->dev, num, data);
>>   }
>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>> index 0c6a88cc4695..6649e84d25de 100644
>> --- a/drivers/iio/adc/ad7606.h
>> +++ b/drivers/iio/adc/ad7606.h
>> @@ -151,6 +151,8 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>>   		 const char *name, unsigned int id,
>>   		 const struct ad7606_bus_ops *bops);
>>   
>> +int ad7606_reset(struct ad7606_state *st);
>> +
>>   enum ad7606_supported_device_ids {
>>   	ID_AD7605_4,
>>   	ID_AD7606_8,
>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>> index d8408052262e..1f7050297b64 100644
>> --- a/drivers/iio/adc/ad7606_par.c
>> +++ b/drivers/iio/adc/ad7606_par.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>> +#include <linux/gpio/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/types.h>
>>   #include <linux/err.h>
>> @@ -21,8 +22,30 @@ static int ad7606_par16_read_block(struct device *dev,
>>   	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>   	struct ad7606_state *st = iio_priv(indio_dev);
>>   
>> -	insw((unsigned long)st->base_address, buf, count);
>>   
>> +	/*
>> +	 * On the parallel interface, the frstdata signal is set to high while
>> +	 * and after reading the sample of the first channel and low for all
>> +	 * other channels.  This can be used to check that the incoming data is
>> +	 * correctly aligned.  During normal operation the data should never
>> +	 * become unaligned, but some glitch or electrostatic discharge might
>> +	 * cause an extra read or clock cycle.  Monitoring the frstdata signal
>> +	 * allows to recover from such failure situations.
>> +	 */
>> +	int num = count;
>> +	u16 *_buf = buf;
>> +
>> +	if (st->gpio_frstdata) {
>> +		insw((unsigned long)st->base_address, _buf, 1);
>> +		if (!gpiod_get_value(st->gpio_frstdata)) {
>> +			ad7606_reset(st);
>> +			return -EIO;
>> +		}
>> +		_buf++;
>> +		num--;
>> +	}
>> +	insw((unsigned long)st->base_address, _buf, num)
>> +;
> Seems this slipped onto the next line.
> Make sure to run checkpatch which I would have thought would catch this.
>
>>   	return 0;
>>   }
>>   
>> @@ -35,8 +58,28 @@ static int ad7606_par8_read_block(struct device *dev,
>>   {
>>   	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>   	struct ad7606_state *st = iio_priv(indio_dev);
>> -
>> -	insb((unsigned long)st->base_address, buf, count * 2);
>> +	/*
>> +	 * On the parallel interface, the frstdata signal is set to high while
>> +	 * and after reading the sample of the first channel and low for all
>> +	 * other channels.  This can be used to check that the incoming data is
>> +	 * correctly aligned.  During normal operation the data should never
>> +	 * become unaligned, but some glitch or electrostatic discharge might
>> +	 * cause an extra read or clock cycle.  Monitoring the frstdata signal
>> +	 * allows to recover from such failure situations.
>> +	 */
>> +	int num = count;
>> +	u16 *_buf = buf;
>> +
>> +	if (st->gpio_frstdata) {
>> +		insb((unsigned long)st->base_address, _buf, 2);
>> +		if (!gpiod_get_value(st->gpio_frstdata)) {
>> +			ad7606_reset(st);
>> +			return -EIO;
>> +		}
>> +		_buf++;
>> +		num--;
>> +	}
>> +	insb((unsigned long)st->base_address, _buf, num * 2);
>>   
>>   	return 0;
>>   }
>>
>> ---
>> base-commit: 07d4d0bb4a8ddcc463ed599b22f510d5926c2495
>> change-id: 20240416-cleanup-ad7606-161e2ed9818b
>>
>> Best regards,