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

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Fri, Nov 14, 2025 at 08:24:23PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 14, 2025 at 11:20 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > +config TI_ADS131M02
> > +       tristate "Texas Instruments ADS131M02"
> > +       depends on SPI && COMMON_CLK && REGULATOR
> 
> Hmm... The COMMON_CLK looks strange here. Why?

I can drop it, but the driver will fail without proper clock
configuration anyways.

> > +       select CRC_ITU_T
> 
> Btw, why does it not use regmap?

I already answered it here:
https://lore.kernel.org/all/aQ3J_rJV-hB2nh91@pengutronix.de/

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

Dokumentation do not provides this information.

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

ack

> > +static int ads131m_hw_reset(struct ads131m_priv *priv)
> > +       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?

I do not have strong option about this right now.

> ...
> 
> > +       /*
> > +        * 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?

Looks like error pointer:
https://elixir.bootlin.com/linux/v6.18-rc5/source/include/linux/regulator/consumer.h#L351

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

ok.

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

ok.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |