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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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.

...

> +/* 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)

> +#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?

> +		cfg = ADS1298_LOWEST_DR;
> +	else if (factor)
> +		cfg = ADS1298_MASK_CONFIG1_HR | ilog2(factor); /* Use HR mode */
> +	else
> +		cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */

...

> +		*val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));

Outer parentheses are redundant.

...

> +	wasbusy = --priv->rdata_xfer_busy;

Why preincrement? How would it be different from postincrement?

> +	if (wasbusy) {

To me more robust code would look like

	if (wasbusy < 1)
		return;
	...
	if (wasbusy > 1)
		...

> +		/*
> +		 * 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);

...

> +	struct device *dev = &priv->spi->dev;
> +	int ret;
> +	unsigned int val;

Better ordering is so called reversed xmas tree (longest lines first):

	struct device *dev = &priv->spi->dev;
	unsigned int val;
	int ret;

...

> +	/* Device initializes into RDATAC mode, which we don't want. */

No period at the end of one-line comments (be consistent in the style over
comments of the same class).

...

> +	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?

...

> +	/* VREF can be supplied externally. Otherwise use internal reference */

Better to use comma as it's one-line comment. Or make it multi-line with
respective periods.

...

> +		ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> +					       priv->reg_vref);

Can be one line.

> +		if (ret)
> +			return ret;

...

> +	ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> +				       priv->reg_avdd);

One line.

> +	if (ret)
> +		return ret;

...

> +	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.

> +	} else {
> +		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "RESET failed\n");
> +	}
> +	/* Wait 18 clock cycles for reset command to complete */
> +	udelay(ADS1298_CLOCKS_TO_USECS(18));

-- 
With Best Regards,
Andy Shevchenko