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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 18 Nov 2025 15:18:21 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> From: David Jander <david@protonic.nl>
> 
> 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.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> Co-developed-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Hi Oleksij,

Series applied, but one comment inline on some code that surprised
me. I'm curious whether anyone else agrees the devm_clk_get_enabled()
stub returning NULL is an odd choice?

For now applied to my local branch. I will push it out only as testing until
I can rebase on rc1.

> +/**
> + * ads131m_parse_clock - enable clock and detect "xtal" selection
> + * @priv: Device private data structure.
> + * @is_xtal: result flag (true if "xtal", false if default "clkin")
> + *
> + * Return: 0 on success, or a negative error code.
> + */
> +static int ads131m_parse_clock(struct ads131m_priv *priv, bool *is_xtal)
> +{
> +	struct device *dev = &priv->spi->dev;
> +	struct clk *clk;
> +	int ret;
> +
> +	clk = devm_clk_get_enabled(dev, NULL);

This surprised me, so I went digging.  Anyone know why
the stub returns NULL?  Given that the normal function doesn't have
that as an allowed return value that seems really odd.

Still, it does, so this code is fine if odd.

> +	if (IS_ERR_OR_NULL(clk)) {
> +		if (IS_ERR(clk))
> +			ret = PTR_ERR(clk);
> +		else
> +			ret = -ENODEV;
> +
> +		return dev_err_probe(dev, ret, "clk get enabled failed\n");
> +	}
> +
> +	ret = device_property_match_string(dev, "clock-names", "xtal");
> +	if (ret > 0)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "'xtal' must be the only or first clock name");
> +
> +	if (ret < 0 && ret != -ENODATA)
> +		return dev_err_probe(dev, ret,
> +				     "failed to read 'clock-names' property");
> +
> +	if (ret == 0 && !priv->config->supports_xtal)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "'xtal' clock not supported on this device");
> +
> +	*is_xtal = !ret;
> +
> +	return 0;
> +}