| Message ID | 38d39befcca110132fd4349c87fcb5d7ff51a2c8.1740421248.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Support ROHM BD79124 ADC | expand |
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 :( Jonathan > > --- > This commit is compile-tested only! All further testing is appreciated. > --- > drivers/iio/adc/ti-ads7924.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c > index 66b54c0d75aa..b1f745f75dbe 100644 > --- a/drivers/iio/adc/ti-ads7924.c > +++ b/drivers/iio/adc/ti-ads7924.c > @@ -251,11 +251,8 @@ static const struct iio_info ads7924_info = { > .read_raw = ads7924_read_raw, > }; > > -static int ads7924_get_channels_config(struct i2c_client *client, > - struct iio_dev *indio_dev) > +static int ads7924_get_channels_config(struct device *dev) > { > - struct ads7924_data *priv = iio_priv(indio_dev); > - struct device *dev = priv->dev; > struct fwnode_handle *node; > int num_channels = 0; > > @@ -380,7 +377,7 @@ static int ads7924_probe(struct i2c_client *client) > indio_dev->num_channels = ARRAY_SIZE(ads7924_channels); > indio_dev->info = &ads7924_info; > > - ret = ads7924_get_channels_config(client, indio_dev); > + ret = ads7924_get_channels_config(dev); > if (ret < 0) > return dev_err_probe(dev, ret, > "failed to get channels configuration\n");
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
diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c index 66b54c0d75aa..b1f745f75dbe 100644 --- a/drivers/iio/adc/ti-ads7924.c +++ b/drivers/iio/adc/ti-ads7924.c @@ -251,11 +251,8 @@ static const struct iio_info ads7924_info = { .read_raw = ads7924_read_raw, }; -static int ads7924_get_channels_config(struct i2c_client *client, - struct iio_dev *indio_dev) +static int ads7924_get_channels_config(struct device *dev) { - struct ads7924_data *priv = iio_priv(indio_dev); - struct device *dev = priv->dev; struct fwnode_handle *node; int num_channels = 0; @@ -380,7 +377,7 @@ static int ads7924_probe(struct i2c_client *client) indio_dev->num_channels = ARRAY_SIZE(ads7924_channels); indio_dev->info = &ads7924_info; - ret = ads7924_get_channels_config(client, indio_dev); + ret = ads7924_get_channels_config(dev); if (ret < 0) return dev_err_probe(dev, ret, "failed to get channels configuration\n");
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> --- This commit is compile-tested only! All further testing is appreciated. --- drivers/iio/adc/ti-ads7924.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)