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

[Mike Looijmans]

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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxx
W: www.topic.nl