Re: [PATCH v4 06/10] iio: adc: ti-ads7924 Drop unnecessary function parameters

[Matti Vaittinen <mazziesaccount@gmail.com>]

Hi dee Ho again Jonathan (and all),

On 02/03/2025 05:46, Jonathan Cameron wrote:
> On Mon, 24 Feb 2025 20:34:01 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Device pointer is the only variable which is used by the
>> ads7924_get_channels_config() and which is declared outside this
>> function. Still, the function gets the iio_device and i2c_client as
>> parameters. The sole caller of this function (probe) already has the
>> device pointer which it can directly pass to the function.
>>
>> Simplify code by passing the device pointer directly as a parameter
>> instead of digging it from the iio_device's private data.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Looking again at this function it doesn't seem to be doing anything
> useful at all.  It checks the channel nodes are in range, but
> does nothing with that data. I'd just drop it entirely.
> 
> Ah. I see David suggested the same.
> 
> We can't really 'fix' what this was perhaps intended to do now
> as what it does has become ABI :(


I took another look at this.
The logic in the ads7924 driver (without this patch) is actually:

ads7924_get_channels_config(...)
{
	device_for_each_child_node(dev, node) {
		if (fwnode_property_read_u32(node, "reg", &pval)) ..
			continue;

		if (channel >= ADS7924_CHANNELS)
			continue;

		num_channels++;
	}

if (!num_channels)
	return -EINVAL;
}

...

ads7924_probe()
{
	ret = ads7924_get_channels_config(...);
	if (ret < 0)
		return dev_err_probe(...);
}

So, it still returns an error, if no channels with valid 'reg' property 
were found from the DT. It will also fail the probe().

Thus, this change is not quite as likely to cause things to break as it 
seemed. Still, for now anything with even single valid 'channel' has 
been Ok, even if all the rest were garbage. This new variant would fail 
if any of the 'channel' nodes contained no or bad 'reg'. Thus this can 
still break things.

Anyways, I'll follow your suggestion and drop this patch (unless you 
have second thoughts) - but I will keep the function so it still 
requires at least 1 valid channel node to be found.

Yours,
	-- Matti