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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251207193313.794ea339@jic23-huawei>
Date: Sun, 7 Dec 2025 19:33:13 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, David Jander <david@...tonic.nl>,
 kernel@...gutronix.de, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org, Andy Shevchenko
 <andy@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno
 Sá <nuno.sa@...log.com>
Subject: Re: [PATCH v4 2/2] iio: adc: Add TI ADS131M0x ADC driver

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

> From: David Jander <david@...tonic.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@...tonic.nl>
> Co-developed-by: Oleksij Rempel <o.rempel@...gutronix.de>
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.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;
> +}