linux-kernel - Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f0d0f114-3953-46b5-b9f6-9b35537e6f8e@baylibre.com>
Date: Tue, 25 Feb 2025 18:09:04 -0600
From: David Lechner <dlechner@...libre.com>
To: Matti Vaittinen <mazziesaccount@...il.com>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Daniel Scally <djrscally@...il.com>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Hugo Villeneuve <hvilleneuve@...onoff.com>, Nuno Sa <nuno.sa@...log.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Guillaume Stols <gstols@...libre.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>,
 Dumitru Ceclan <mitrutzceclan@...il.com>,
 Trevor Gamblin <tgamblin@...libre.com>,
 Matteo Martelli <matteomartelli3@...il.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>,
 Ramona Alexandra Nechita <ramona.nechita@...log.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

On 2/24/25 12:34 PM, Matti Vaittinen wrote:
> The ti-ads7924 driver ignores the device-tree ADC channel specification
> and always exposes all 4 channels to users whether they are present in
> the device-tree or not. Additionally, the "reg" values in the channel
> nodes are ignored, although an error is printed if they are out of range.
> 
> Register only the channels described in the device-tree, and use the reg
> property as a channel ID.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> 
> ---
> Revision history:
> v3 => v4:
>  - Adapt to 'drop diff-channel support' changes to ADC-helpers
>  - select ADC helpers in the Kconfig
> v2 => v3: New patch
> 
> Please note that this is potentially breaking existing users if they
> have wrong values in the device-tree. I believe the device-tree should
> ideally be respected, and if it says device X has only one channel, then
> we should believe it and not register 4. Well, we don't live in the
> ideal world, so even though I believe this is TheRightThingToDo - it may
> cause havoc because correct device-tree has not been required from the
> day 1. So, please review and test and apply at your own risk :)

The DT bindings on this one are a little weird. Usually, if we don't
use any extra properties from adc.yaml, we leave out the channels. In
this case it does seem kind of like the original intention was to work
like you are suggesting, but hard to say since the driver wasn't actually
implemented that way. I would be more inclined to actually not make the
breaking change here and instead relax the bindings to make channel nodes
optional and just have the driver ignore the channel nodes by dropping
the ads7924_get_channels_config() function completely. This would make
the driver simpler instead of more complex like this patch does.

> 
> As a side note, this might warrant a fixes tag but the adc-helper -stuff
> is hardly worth to be backported... (And I've already exceeded my time
> budget with this series - hence I'll leave crafting backportable fix to
> TI people ;) )
> 
> This has only been compile tested! All testing is highly appreciated.
> ---

...

> -static int ads7924_get_channels_config(struct device *dev)
> +static int ads7924_get_channels_config(struct iio_dev *indio_dev,
> +				       struct device *dev)

Could get dev from indio_dev->dev.parent and keep only one parameter
to this function.

>  {
> -	struct fwnode_handle *node;
> -	int num_channels = 0;
> +	struct iio_chan_spec *chan_array;
> +	int num_channels = 0, i;

Don't need initialization here.

> +	static const char * const datasheet_names[] = {
> +		"AIN0", "AIN1", "AIN2", "AIN3"
> +	};
>