Re: [PATCH v3 2/2] iio: adc: ti-ads1298: Add driver

[Mike Looijmans <mike.looijmans@topic.nl>]

On 06-02-2024 13:57, Andy Shevchenko wrote:
> On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote:
>> Skeleton driver for the TI ADS1298 medical ADC. This device is
>> typically used for ECG and similar measurements. Supports data
>> acquisition at configurable scale and sampling frequency.
> Thanks for an update, I have only a few style comments and a single one about
> comparison (see below). If you are going to address them as suggested, feel
> free to add
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> to the next version.
>
> ...

Thanks for reviewing...


>> +/* Registers */
>> +#define ADS1298_REG_ID		0x00
>> +#define ADS1298_MASK_ID_FAMILY			GENMASK(7, 3)
>> +#define ADS1298_MASK_ID_CHANNELS		GENMASK(2, 0)
>> +#define ADS1298_ID_FAMILY_ADS129X		0x90
>> +#define ADS1298_ID_FAMILY_ADS129XR		0xd0
> + Blank line? (And so on for all registers that have bitfields defined)

Makes sense... Looks too cluttered as it is now.


>
>> +#define ADS1298_REG_CONFIG1	0x01
>> +#define ADS1298_MASK_CONFIG1_HR			BIT(7)
>> +#define ADS1298_MASK_CONFIG1_DR			GENMASK(2, 0)
>> +#define ADS1298_SHIFT_DR_HR			6
>> +#define ADS1298_SHIFT_DR_LP			7
>> +#define ADS1298_LOWEST_DR			0x06
> ...
>
>> +	factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
>> +	if (factor >= 128)
> I just realized that this comparison is probably better in a form
>
> 	if (factor >= ADS1298_MASK_CONFIG1_HR)
>
> as it points out why this is a special case in comparison to 'if (factor)'
> below. What do you think?

The "HR" bit sets the device to high-res mode (which apparently doubles 
the internal sample rate).

But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the 
max oversampling factor.


> ...
>
>> +	wasbusy = --priv->rdata_xfer_busy;
> Why preincrement? How would it be different from postincrement?

Maybe better write this as:

--priv->rdata_xfer_busy;

wasbusy = priv->rdata_xfer_busy;

I want the value after decrementing it.


>> +	if (wasbusy) {
> To me more robust code would look like
>
> 	if (wasbusy < 1)
> 		return;
> 	...
> 	if (wasbusy > 1)
> 		...

wasbusy could have been unsigned.

This code will only ever execute with rdata_xfer_busy > 0 (or the SPI 
driver called our completion callback without us calling spi_async first)

>
>> +		/*
>> +		 * DRDY interrupt occurred before SPI completion. Start a new
>> +		 * SPI transaction now to retrieve the data that wasn't latched
>> +		 * into the ADS1298 chip's transfer buffer yet.
>> +		 */
>> +		spi_async(priv->spi, &priv->rdata_msg);
>> +		/*
>> +		 * If more than one DRDY took place, there was an overrun. Since
>> +		 * the sample is already lost, reset the counter to 1 so that
>> +		 * we will wait for a DRDY interrupt after this SPI transaction.
>> +		 */
>> +		if (wasbusy > 1)
>> +			priv->rdata_xfer_busy = 1;
>> +	}
> ...
>
>> +		/*
>> +		 * for a single transfer mode we're kept in direct_mode until
> For
>
>> +		 * completion, avoiding a race with buffered IO.
>> +		 */
> ...
>
>> +	wasbusy = priv->rdata_xfer_busy++;
> So, it starts from negative?
>
>> +	/* When no SPI transfer in transit, start one now */
>> +	if (!wasbusy)
> To be compatible with above perhaps
>
> 	if (wasbusy < 1)
>
> also makes it more robust (all negative numbers will be considered the same.
>
>> +		spi_async(priv->spi, &priv->rdata_msg);

The "rdata_xfer_busy" starts at 0.

Increments when a DRDY occurs.

Decrements when SPI completion is reported.

So the meaning of "rdata_xfer_busy" is:

0 = Waiting for DRDY interrupt

1 = SPI transfer in progress

2 = DRDY occured during SPI transfer, should start another on completion

 >2 = Multiple DRDY during SPI transfer, overflow, we 
lost rdata_xfer_busy - 2 samples


> ...
>
>
>> +	dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val),
>> +		(unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS)));
> Castings in printf() is a big red flag usually (it's rarely we need them).
> Why is it here?

Compiler complains that the expression is "unsigned long". Probably 
because of ADS1298_MASK_ID_CHANNELS being so.


> ...
>
>> +	if (reset_gpio) {
>> +		/* Minimum reset pulsewidth is 2 clock cycles */
>> +		udelay(ADS1298_CLOCKS_TO_USECS(2));
>> +		gpiod_set_value(reset_gpio, 0);
> I would rewrite it as
>
> 		/* Minimum reset pulsewidth is 2 clock cycles */
> 		gpiod_set_value(reset_gpio, 1);
> 		udelay(ADS1298_CLOCKS_TO_USECS(2));
> 		gpiod_set_value(reset_gpio, 0);
>
> to be sure we have a reset done correctly, and the comment will make more
> sense.

If used, the reset must be asserted *before* the voltages and clocks are 
activated. This would obfuscate that, and add a redundant call to set_value.

I did forget to use "cansleep" here, will add that.


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl