Re: [RFC PATCH 1/5] iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC

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

On Thu, May 01, 2025 at 12:37:30PM -0500, David Lechner wrote:
> On 5/1/25 5:00 AM, Sayyad Abid wrote:
> > Add the core driver file `ti-ads1262.c` for the TI ADS1262 ADC.
> > This initial version implements basic IIO functionality for device
> > probe via SPI and reading raw voltage samples from input channels.

...

> > +#include <linux/kernel.h>
> 
> This header includes too much, please use more specific headers.
> 
> > +#include <linux/device.h>

Ditto for this one. Include it only if really required, otherwise we have often
used dev_printk.h, device/devres.h.

> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/delay.h>
> 
> Alphabetical order is preferred.

> > +#include <linux/spi/spi.h>
> > +#include <linux/unaligned.h>

Also many headers are missing (probably due to inclusion of kernel.h).

...

> > +#define ADS1262_SETTLE_TIME_USECS	10000

_US is fine (no need to have longer _USECS, which is not so standard).

Also
(10 * USEC_PER_MSEC)

...

> > +/* The Read/Write commands require 4 tCLK to encode and decode, for speeds
> > + * 2x the clock rate, these commands would require extra time between the
> > + * command byte and the data. A simple way to tacke this issue is by
> > + * limiting the SPI bus transfer speed while accessing registers.
> > + */

/*
 * Wrong style for multi-line comments, please use
 * this as an example. Fix all comments in the file
 * accordingly.
 */

...

> > +/* For reading and writing we need a buffer of size 3bytes*/

Missing space.

...

> > +/**
> > + * struct ads1262_private - ADS1262 ADC private data structure
> > + * @spi: SPI device structure
> > + * @reset_gpio: GPIO descriptor for reset pin
> > + * @prev_channel: Previously selected channel for MUX configuration
> > + * @cmd_buffer: Buffer for SPI command transfers
> > + * @rx_buffer: Buffer for SPI data reception
> > + */
> > +struct ads1262_private {
> > +	struct spi_device *spi;

Is it really used? Or is struct device *dev just enough?

> > +	struct gpio_desc *reset_gpio;
> > +	u8 prev_channel;
> > +	u8 cmd_buffer[ADS1262_SPI_CMD_BUFFER_SIZE];
> > +	u8 rx_buffer[ADS1262_SPI_RDATA_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> 
> cmd_buffer is also used with SPI, so __aligned(IIO_DMA_MINALIGN); needs to go
> there instead.
> 
> > +};

...

> > +#define ADS1262_CHAN(index)						\
> > +{									\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = index,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.scan_index = index,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> > +		.storagebits = 32,					\
> > +		.endianness = IIO_CPU					\

Leave trailing comma here and in the similar cases (when it's not a clear
terminator entry).

> > +	},								\
> > +}

...

> > +static int ads1262_write_cmd(struct ads1262_private *priv, u8 command)
> > +{
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = priv->cmd_buffer,
> > +		.rx_buf = priv->rx_buffer,
> > +		.len = ADS1262_SPI_RDATA_BUFFER_SIZE,
> > +		.speed_hz = ADS1262_CLK_RATE_HZ,
> > +	};
> > +
> > +	priv->cmd_buffer[0] = command;
> > +
> > +	return spi_sync_transfer(priv->spi, &xfer, 1);
> > +}
> > +
> > +static int ads1262_reg_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > +	struct ads1262_private *priv = context;
> > +
> > +	priv->cmd_buffer[0] = ADS1262_CMD_WREG | reg;
> > +	priv->cmd_buffer[1] = 0;
> > +	priv->cmd_buffer[2] = val;
> > +
> > +	return spi_write(priv->spi, &priv->cmd_buffer[0], 3);
> > +}

Can't you use regmap SPI instead?

...

> > +static int ads1262_reg_read(void *context, unsigned int reg)
> > +{
> > +	struct ads1262_private *priv = context;
> > +	struct spi_transfer reg_read_xfer = {
> > +		.tx_buf = priv->cmd_buffer,
> > +		.rx_buf = priv->cmd_buffer,
> > +		.len = 3,
> > +		.speed_hz = ADS1262_CLK_RATE_HZ,
> > +	};
> > +	int ret;
> > +
> > +	priv->cmd_buffer[0] = ADS1262_CMD_RREG | reg;
> > +	priv->cmd_buffer[1] = 0;
> > +	priv->cmd_buffer[2] = 0;
> > +
> > +	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> 
> Why not use regmap? You will still custom read/write functions similar to these
> because of needing the lower SCLK rate, but it will give you a bunch of other
> nice features for free.

Ah, same comment above :-)

...

> > +static int ads1262_reset(struct iio_dev *indio_dev)
> > +{
> > +	struct ads1262_private *priv = iio_priv(indio_dev);
> > +
> > +	if (priv->reset_gpio) {
> > +		gpiod_set_value(priv->reset_gpio, 0);
> > +		usleep_range(200, 300);
> 
> Use fsleep(). Also, could make this clear that it is 4 tCLK cycles (the hard-
> coded value would have to be changed if external clock support was added).
> 
> > +		gpiod_set_value(priv->reset_gpio, 1);
> 
> The DT bindings will take care of active low, so this looks backwards. Also
> st->reset_gpio is never assigned, so this is dead code.
> 
> > +	} else {

Redundant else. Just return from the conditional and have the below outside of
it.

> > +		return ads1262_write_cmd(priv, ADS1262_CMD_RESET);
> > +	}

Missing blank line, but see above.

> > +	return 0;
> > +}

-- 
With Best Regards,
Andy Shevchenko