| Message ID | 20dd0e4ea72fe39b90b611f9c08dbd4bc1d5217f.1740421248.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD79124 ADC | expand |
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@gmail.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" > + }; >
On 26/02/2025 02:09, David Lechner wrote: > 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@gmail.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. I have no strong opinion on this. I see this driver says 'Supported' in MAINTAINERS. Maybe Hugo is able to provide some insight? >> 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" >> + }; Thanks for the review David! I do agree with the comments to the code. Yours, -- Matti
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 0993008a1586..d16082ac2e1f 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1476,6 +1476,7 @@ config TI_ADS7924 tristate "Texas Instruments ADS7924 ADC" depends on I2C select REGMAP_I2C + select IIO_ADC_HELPER help If you say yes here you get support for Texas Instruments ADS7924 4 channels, 12-bit I2C ADC chip. diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c index b1f745f75dbe..e3ac170c73f4 100644 --- a/drivers/iio/adc/ti-ads7924.c +++ b/drivers/iio/adc/ti-ads7924.c @@ -22,6 +22,7 @@ #include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <linux/iio/adc-helpers.h> #include <linux/iio/iio.h> #include <linux/iio/types.h> @@ -119,15 +120,12 @@ #define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \ ADS7924_CONVTIME_US) -#define ADS7924_V_CHAN(_chan, _addr) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .channel = _chan, \ - .address = _addr, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ - .datasheet_name = "AIN"#_chan, \ -} +static const struct iio_chan_spec ads7924_chan_template = { + .type = IIO_VOLTAGE, + .indexed = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), +}; struct ads7924_data { struct device *dev; @@ -182,13 +180,6 @@ static const struct regmap_config ads7924_regmap_config = { .writeable_reg = ads7924_is_writeable_reg, }; -static const struct iio_chan_spec ads7924_channels[] = { - ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG), - ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG), - ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG), - ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG), -}; - static int ads7924_get_adc_result(struct ads7924_data *data, struct iio_chan_spec const *chan, int *val) { @@ -251,32 +242,35 @@ static const struct iio_info ads7924_info = { .read_raw = ads7924_read_raw, }; -static int ads7924_get_channels_config(struct device *dev) +static int ads7924_get_channels_config(struct iio_dev *indio_dev, + struct device *dev) { - struct fwnode_handle *node; - int num_channels = 0; + struct iio_chan_spec *chan_array; + int num_channels = 0, i; + static const char * const datasheet_names[] = { + "AIN0", "AIN1", "AIN2", "AIN3" + }; - device_for_each_child_node(dev, node) { - u32 pval; - unsigned int channel; + num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev, + &ads7924_chan_template, + ARRAY_SIZE(datasheet_names) - 1, + &chan_array); - if (fwnode_property_read_u32(node, "reg", &pval)) { - dev_err(dev, "invalid reg on %pfw\n", node); - continue; - } + if (num_channels < 0) + return num_channels; - channel = pval; - if (channel >= ADS7924_CHANNELS) { - dev_err(dev, "invalid channel index %d on %pfw\n", - channel, node); - continue; - } + if (!num_channels) + return -EINVAL; - num_channels++; + for (i = 0; i < num_channels; i++) { + int ch_id = chan_array[i].channel; + + chan_array[i].address = ADS7924_DATA0_U_REG + ch_id; + chan_array[i].datasheet_name = datasheet_names[ch_id]; } - if (!num_channels) - return -EINVAL; + indio_dev->channels = chan_array; + indio_dev->num_channels = num_channels; return 0; } @@ -370,18 +364,15 @@ static int ads7924_probe(struct i2c_client *client) mutex_init(&data->lock); - indio_dev->name = "ads7924"; - indio_dev->modes = INDIO_DIRECT_MODE; - - indio_dev->channels = ads7924_channels; - indio_dev->num_channels = ARRAY_SIZE(ads7924_channels); - indio_dev->info = &ads7924_info; - - ret = ads7924_get_channels_config(dev); + ret = ads7924_get_channels_config(indio_dev, dev); if (ret < 0) return dev_err_probe(dev, ret, "failed to get channels configuration\n"); + indio_dev->name = "ads7924"; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->info = &ads7924_info; + data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config); if (IS_ERR(data->regmap)) return dev_err_probe(dev, PTR_ERR(data->regmap), @@ -469,3 +460,4 @@ module_i2c_driver(ads7924_driver); MODULE_AUTHOR("Hugo Villeneuve <hvilleneuve@dimonoff.com>"); MODULE_DESCRIPTION("Texas Instruments ADS7924 ADC I2C driver"); MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS("IIO_DRIVER");
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@gmail.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 :) 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. --- drivers/iio/adc/Kconfig | 1 + drivers/iio/adc/ti-ads7924.c | 78 ++++++++++++++++-------------------- 2 files changed, 36 insertions(+), 43 deletions(-)