Re: [PATCH v3 2/2] iio: adc: Add TI ADS131M0x ADC driver

[Andy Shevchenko]

On Fri, Nov 14, 2025 at 11:20 AM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
>
> From: David Jander <david@xxxxxxxxxxx>
>
> Add a new IIO ADC driver for Texas Instruments ADS131M0x devices
> (ADS131M02/03/04/06/08). These are 24-bit, up to 64 kSPS, simultaneous-
> sampling delta-sigma ADCs accessed via SPI.
>
> Highlights:
> - Supports 2/3/4/6/8-channel variants with per-channel RAW and SCALE.
> - Implements device-required full-duplex fixed-frame transfers.
> - Handles both input and output CRC
>
> Note: Despite the almost identical name, this hardware is not
> compatible with the ADS131E0x series handled by
> drivers/iio/adc/ti-ads131e08.c.

...

> +config TI_ADS131M02
> +       tristate "Texas Instruments ADS131M02"
> +       depends on SPI && COMMON_CLK && REGULATOR

Hmm... The COMMON_CLK looks strange here. Why?

> +       select CRC_ITU_T

Btw, why does it not use regmap?

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/crc-itu-t.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>

> +#include <linux/device.h>

Is it used? I haven't found what API or data structure is required from here.

> +#include <linux/device/devres.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/lockdep.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>

...

> +#define ADS131M_CMD_RREG_OP            0xa000
> +#define ADS131M_CMD_WREG_OP            0x6000

These two have bit 13 always set. What is the meaning of that bit?

> +#define ADS131M_CMD_RREG(a, n) \
> +       (ADS131M_CMD_RREG_OP | \
> +        FIELD_PREP(ADS131M_CMD_ADDR_MASK, a) | \
> +        FIELD_PREP(ADS131M_CMD_NUM_MASK, n))
> +#define ADS131M_CMD_WREG(a, n) \
> +       (ADS131M_CMD_WREG_OP | \
> +        FIELD_PREP(ADS131M_CMD_ADDR_MASK, a) | \
> +        FIELD_PREP(ADS131M_CMD_NUM_MASK, n))

...

> +/**
> + * ads131m_tx_frame_unlocked - Sends a command frame with Input CRC
> + * @priv: Device private data structure.
> + * @command: The 16-bit command to send (e.g., NULL, RREG, RESET).
> + *
> + * This function sends a command in Word 0, and its calculated 16-bit
> + * CRC in Word 1, as required when Input CRC is enabled.
> + *
> + * Return: 0 on success, or a negative error code from spi_sync.

spi_sync()

But I would drop it as it makes dependency on the code changes and it
will deviate easily if code grows and something else becomes a call
that returns an error, also this simply doesn't scale: are you going
to list whole bunch of APIs in the kernel doc? (rhetorical Q) Ditto
for other similar cases.

> + */

...

> +/**
> + * ads131m_check_status_crc_err - Checks for an Input CRC error.
> + * @priv: Device private data structure.
> + *
> + * Sends a NULL command to fetch the STATUS register and checks the
> + * CRC_ERR bit. This is used to verify the integrity of the previous
> + * command (like RREG or WREG).
> + *
> + * Return: 0 on success, -EIO if CRC_ERR bit is set.

Note, this kernel-doc line is good as it doesn't rely on the code,
rather on the HW programming flow.

> + */

...

> +static int ads131m_rmw_reg(struct ads131m_priv *priv, u8 reg, u16 clear, u16 set)
> +{
> +       u16 old_val, new_val;
> +       int ret;
> +
> +       guard(mutex)(&priv->lock);
> +
> +       ret = ads131m_read_reg_unlocked(priv, reg, &old_val);
> +       if (ret < 0)
> +               return ret;
> +
> +       new_val = (old_val & ~clear) | set;
> +       if (new_val == old_val)
> +               return 0;
> +
> +       return ads131m_write_reg_unlocked(priv, reg, new_val);
> +}

...

> +static int ads131m_hw_reset(struct ads131m_priv *priv)
> +{
> +       struct device *dev = &priv->spi->dev;
> +       int ret;
> +
> +       /* Datasheet: Hold /RESET low for > 2 f_CLKIN cycles. 1us is ample. */
> +       ret = gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "Failed to assert reset GPIO\n");


> +       fsleep(1);

Hmm... Is it needed? I think the GPIO is slow enough to avoid delays
like this, but okay.

> +       ret = gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "Failed to deassert reset GPIO\n");
> +
> +       /* Wait t_REGACQ (5us) for registers to be accessible */
> +       fsleep(ADS131M_RESET_DELAY_US);
> +
> +       return 0;
> +}

Can you use the reset-gpio driver instead of a custom approach?

...

> +       /*
> +        * Get the optional external reference. This schedules regulator_put()
> +        * automatically.
> +        */
> +       priv->refin_supply = devm_regulator_get_optional(dev, "refin");
> +       ret = PTR_ERR_OR_ZERO(priv->refin_supply);
> +       if (ret == -ENODEV)
> +               priv->refin_supply = NULL;
> +       else if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to get refin regulator\n");

So, will the refin_supply be ever an error pointer? I think no, hence
why IS_ERR_OR_NULL() in the user somewhere above in the code?

...

> +static int ads131m_parse_clock(struct ads131m_priv *priv, bool *is_xtal)
> +{
> +       struct device *dev = &priv->spi->dev;
> +       int ret;
> +
> +       priv->clk = devm_clk_get_enabled(dev, NULL);
> +       if (IS_ERR(priv->clk))
> +               return dev_err_probe(dev, PTR_ERR(priv->clk), "clk get enabled failed\n");
> +
> +       ret = device_property_match_string(dev, "clock-names", "xtal");
> +       if (ret == 0) {
> +               if (!priv->config->supports_xtal)
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "'xtal' clock not supported on this device");

> +               *is_xtal = true;
> +
> +               return 0;

This...

> +       } else if (ret > 0) {
> +               return dev_err_probe(dev, -EINVAL, "'xtal' must be the only or first clock name");

> +       } else if (ret == -ENODATA) {
> +               *is_xtal = false;
> +
> +               return 0;
> +       }
> +
> +       return dev_err_probe(dev, ret, "failed to read 'clock-names' property");

...and this can be deduplicated, so the first one becomes just a check
for !supports_xtal.

  if (ret == 0) && !supports_xtal)
    return dev_err_probe(...);
  else if (ret > 0)
    return dev_err_probe(...);

This one will be modified to

  else if (ret != -ENODATA)
    return dev_err_probe(...);

  *is_xtal = !ret;
  return ret;

> +}

...

> +       config = spi_get_device_match_data(spi);

> +       if (!config)
> +               return dev_err_probe(dev, -EINVAL, "No device configuration data found\n");

Without this code will crash, right? So, I consider this check is
redundant because any support of any new chip requires this, and if
one didn't add the driver data, means it wasn't tested (which is a
good trap on itself during code review).

...

> +       { } /* Fixed sentinel */

No comment needed.

-- 
With Best Regards,
Andy Shevchenko