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

Andy Shevchenko
2025-05-02T10:07:32Z

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