| Message ID | 20240430162946.589423-7-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7192: Add AD7194 support | expand |
On Tue, Apr 30, 2024 at 7:30 PM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > Unlike the other AD719Xs, AD7194 has configurable channels. The user can > dynamically configure them in the devicetree. > > Also modify config AD7192 description for better scaling. ... > +static int ad7194_parse_channels(struct iio_dev *indio_dev) > +{ > + struct device *dev = indio_dev->dev.parent; > + struct iio_chan_spec *ad7194_channels; > + struct fwnode_handle *child; > + struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0); > + struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0); > + struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0); > + struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0); > + unsigned int num_channels, index = 0; > + u32 ain[2]; > + int ret; > + > + num_channels = device_get_child_node_count(dev); > + if (num_channels > AD7194_CH_MAX_NR) > + return dev_err_probe(dev, -EINVAL, > + "Too many channels: %u\n", num_channels); > + > + num_channels += AD7194_CH_BASE_NR; > + > + ad7194_channels = devm_kcalloc(dev, num_channels, > + sizeof(*ad7194_channels), GFP_KERNEL); > + if (!ad7194_channels) > + return -ENOMEM; > + > + indio_dev->channels = ad7194_channels; > + indio_dev->num_channels = num_channels; > + > + device_for_each_child_node(dev, child) { You wanted _scoped version... > + ret = fwnode_property_read_u32_array(child, "diff-channels", > + ain, ARRAY_SIZE(ain)); > + if (ret == 0) { > + ret = ad7194_validate_ain_channel(dev, ain[0]); > + if (ret) ...otherwise here is the reference count leakage. > + return ret; > + > + ret = ad7194_validate_ain_channel(dev, ain[1]); > + if (ret) Or here. > + return ret; > + > + *ad7194_channels = ad7194_chan_diff; > + ad7194_channels->scan_index = index++; > + ad7194_channels->channel = ain[0]; > + ad7194_channels->channel2 = ain[1]; > + ad7194_channels->address = AD7194_CH(ain[0], ain[1]); > + } else { > + ret = fwnode_property_read_u32(child, "single-channel", > + &ain[0]); > + if (ret) { > + fwnode_handle_put(child); > + return ret; > + } > + > + ret = ad7194_validate_ain_channel(dev, ain[0]); > + if (ret) Or here. > + return ret; > + > + *ad7194_channels = ad7194_chan; > + ad7194_channels->scan_index = index++; > + ad7194_channels->channel = ain[0]; > + ad7194_channels->address = AD7194_CH(ain[0], 0); > + } > + ad7194_channels++; > + } > + > + *ad7194_channels = ad7194_chan_temp; > + ad7194_channels->scan_index = index++; > + ad7194_channels->address = AD7194_CH_TEMP; > + ad7194_channels++; > + > + *ad7194_channels = ad7194_chan_timestamp; > + ad7194_channels->scan_index = index; > + > + return 0; > +}
On Tue, Apr 30, 2024 at 11:30 AM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > Unlike the other AD719Xs, AD7194 has configurable channels. The user can > dynamically configure them in the devicetree. > > Also modify config AD7192 description for better scaling. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > --- > drivers/iio/adc/Kconfig | 11 +++- > drivers/iio/adc/ad7192.c | 129 +++++++++++++++++++++++++++++++++++++-- > 2 files changed, 133 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 8db68b80b391..74fecc284f1a 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -88,12 +88,17 @@ config AD7173 > called ad7173. > > config AD7192 > - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver" > + tristate "Analog Devices AD7192 and similar ADC driver" > depends on SPI > select AD_SIGMA_DELTA > help > - Say yes here to build support for Analog Devices AD7190, > - AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC). > + Say yes here to build support for Analog Devices SPI analog to digital > + converters (ADC): > + - AD7190 > + - AD7192 > + - AD7193 > + - AD7194 > + - AD7195 > If unsure, say N (but it's safe to say "Y"). > > To compile this driver as a module, choose M here: the > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index 3e797ff48086..0f6ecf953559 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver > + * AD7192 and similar SPI ADC driver > * > * Copyright 2011-2015 Analog Devices Inc. > */ > @@ -129,10 +129,21 @@ > #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */ > #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */ > > +#define AD7194_CH_POS(x) (((x) - 1) << 4) > +#define AD7194_CH_NEG(x) ((x) - 1) > +#define AD7194_CH(pos, neg) \ > + (((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos)) I think this needs a comment that BIT(10) is the CON18 flag for pseudo-differential channels. Also, it would probably be easier to understand if there were two different macros, one for "single-channel" and one for "diff-channels" rather than having neg==0 magically turn in to the pseudo flag. > +#define AD7194_CH_TEMP 0x100 /* Temp sensor */ > +#define AD7194_CH_BASE_NR 2 > +#define AD7194_CH_AIN_START 1 > +#define AD7194_CH_AIN_NR 16 > +#define AD7194_CH_MAX_NR 272 > + > /* ID Register Bit Designations (AD7192_REG_ID) */ > #define CHIPID_AD7190 0x4 > #define CHIPID_AD7192 0x0 > #define CHIPID_AD7193 0x2 > +#define CHIPID_AD7194 0x3 > #define CHIPID_AD7195 0x6 > #define AD7192_ID_MASK GENMASK(3, 0) > > @@ -170,6 +181,7 @@ enum { > ID_AD7190, > ID_AD7192, > ID_AD7193, > + ID_AD7194, > ID_AD7195, > }; > > @@ -179,6 +191,7 @@ struct ad7192_chip_info { > const struct iio_chan_spec *channels; > u8 num_channels; > const struct iio_info *info; > + int (*parse_channels)(struct iio_dev *indio_dev); > }; > > struct ad7192_state { > @@ -932,6 +945,15 @@ static const struct iio_info ad7192_info = { > .update_scan_mode = ad7192_update_scan_mode, > }; > > +static const struct iio_info ad7194_info = { > + .read_raw = ad7192_read_raw, > + .write_raw = ad7192_write_raw, > + .write_raw_get_fmt = ad7192_write_raw_get_fmt, > + .read_avail = ad7192_read_avail, > + .validate_trigger = ad_sd_validate_trigger, > + .update_scan_mode = ad7192_update_scan_mode, It looks like ad7192_update_scan_mode() won't work on ad7194 since the channels are specified via DT for this chip and could be anything. Each scan_index no longer corresponds a specific bit in AD7192_REG_CONF so it would end up with some random configuration. > +}; > + The rest looks fine other than what Andy mentioned already.
On Tue, 30 Apr 2024 19:29:46 +0300 Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > Unlike the other AD719Xs, AD7194 has configurable channels. The user can > dynamically configure them in the devicetree. > > Also modify config AD7192 description for better scaling. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> Hi Alisa-Dariana, I only had one minor thing to add to the other reviews, Thanks, Jonathan > To compile this driver as a module, choose M here: the > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index 3e797ff48086..0f6ecf953559 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c ... > +static int ad7194_parse_channels(struct iio_dev *indio_dev) > +{ > + struct device *dev = indio_dev->dev.parent; > + struct iio_chan_spec *ad7194_channels; > + struct fwnode_handle *child; > + struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0); I think you are only using these as templates that get copied? If so declare them const. > + struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0); > + struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0); > + struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0); > + unsigned int num_channels, index = 0; > + u32 ain[2]; > + int ret; > + > + num_channels = device_get_child_node_count(dev); > + if (num_channels > AD7194_CH_MAX_NR) > + return dev_err_probe(dev, -EINVAL, > + "Too many channels: %u\n", num_channels); > + > + num_channels += AD7194_CH_BASE_NR; > + > + ad7194_channels = devm_kcalloc(dev, num_channels, > + sizeof(*ad7194_channels), GFP_KERNEL); > + if (!ad7194_channels) > + return -ENOMEM; > + > + indio_dev->channels = ad7194_channels; > + indio_dev->num_channels = num_channels; > + > + device_for_each_child_node(dev, child) { Andy pointed out the need for scoped here to avoid leaking the fwnode in error paths. > + ret = fwnode_property_read_u32_array(child, "diff-channels", > + ain, ARRAY_SIZE(ain)); > + if (ret == 0) { > + ret = ad7194_validate_ain_channel(dev, ain[0]); > + if (ret) > + return ret; > + > + ret = ad7194_validate_ain_channel(dev, ain[1]); > + if (ret) > + return ret; > + > + *ad7194_channels = ad7194_chan_diff; > + ad7194_channels->scan_index = index++; > + ad7194_channels->channel = ain[0]; > + ad7194_channels->channel2 = ain[1]; > + ad7194_channels->address = AD7194_CH(ain[0], ain[1]); > + } else { > + ret = fwnode_property_read_u32(child, "single-channel", > + &ain[0]); > + if (ret) { > + fwnode_handle_put(child); With scoped variant above you can drop this one case of (currently correct) release of the fwnode. > + return ret; > + } > + > + ret = ad7194_validate_ain_channel(dev, ain[0]); > + if (ret) > + return ret; > + > + *ad7194_channels = ad7194_chan; > + ad7194_channels->scan_index = index++; > + ad7194_channels->channel = ain[0]; > + ad7194_channels->address = AD7194_CH(ain[0], 0); > + } > + ad7194_channels++; > + } > + > + *ad7194_channels = ad7194_chan_temp; > + ad7194_channels->scan_index = index++; > + ad7194_channels->address = AD7194_CH_TEMP; > + ad7194_channels++; > + > + *ad7194_channels = ad7194_chan_timestamp; > + ad7194_channels->scan_index = index; > + > + return 0; > +}
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 8db68b80b391..74fecc284f1a 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -88,12 +88,17 @@ config AD7173 called ad7173. config AD7192 - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver" + tristate "Analog Devices AD7192 and similar ADC driver" depends on SPI select AD_SIGMA_DELTA help - Say yes here to build support for Analog Devices AD7190, - AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC). + Say yes here to build support for Analog Devices SPI analog to digital + converters (ADC): + - AD7190 + - AD7192 + - AD7193 + - AD7194 + - AD7195 If unsure, say N (but it's safe to say "Y"). To compile this driver as a module, choose M here: the diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c index 3e797ff48086..0f6ecf953559 100644 --- a/drivers/iio/adc/ad7192.c +++ b/drivers/iio/adc/ad7192.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver + * AD7192 and similar SPI ADC driver * * Copyright 2011-2015 Analog Devices Inc. */ @@ -129,10 +129,21 @@ #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */ #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */ +#define AD7194_CH_POS(x) (((x) - 1) << 4) +#define AD7194_CH_NEG(x) ((x) - 1) +#define AD7194_CH(pos, neg) \ + (((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos)) +#define AD7194_CH_TEMP 0x100 /* Temp sensor */ +#define AD7194_CH_BASE_NR 2 +#define AD7194_CH_AIN_START 1 +#define AD7194_CH_AIN_NR 16 +#define AD7194_CH_MAX_NR 272 + /* ID Register Bit Designations (AD7192_REG_ID) */ #define CHIPID_AD7190 0x4 #define CHIPID_AD7192 0x0 #define CHIPID_AD7193 0x2 +#define CHIPID_AD7194 0x3 #define CHIPID_AD7195 0x6 #define AD7192_ID_MASK GENMASK(3, 0) @@ -170,6 +181,7 @@ enum { ID_AD7190, ID_AD7192, ID_AD7193, + ID_AD7194, ID_AD7195, }; @@ -179,6 +191,7 @@ struct ad7192_chip_info { const struct iio_chan_spec *channels; u8 num_channels; const struct iio_info *info; + int (*parse_channels)(struct iio_dev *indio_dev); }; struct ad7192_state { @@ -932,6 +945,15 @@ static const struct iio_info ad7192_info = { .update_scan_mode = ad7192_update_scan_mode, }; +static const struct iio_info ad7194_info = { + .read_raw = ad7192_read_raw, + .write_raw = ad7192_write_raw, + .write_raw_get_fmt = ad7192_write_raw_get_fmt, + .read_avail = ad7192_read_avail, + .validate_trigger = ad_sd_validate_trigger, + .update_scan_mode = ad7192_update_scan_mode, +}; + static const struct iio_info ad7195_info = { .read_raw = ad7192_read_raw, .write_raw = ad7192_write_raw, @@ -1023,6 +1045,91 @@ static const struct iio_chan_spec ad7193_channels[] = { IIO_CHAN_SOFT_TIMESTAMP(14), }; +static int ad7194_validate_ain_channel(struct device *dev, u32 ain) +{ + if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR)) + return dev_err_probe(dev, -EINVAL, + "Invalid AIN channel: %u\n", ain); + + return 0; +} + +static int ad7194_parse_channels(struct iio_dev *indio_dev) +{ + struct device *dev = indio_dev->dev.parent; + struct iio_chan_spec *ad7194_channels; + struct fwnode_handle *child; + struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0); + struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0); + struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0); + struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0); + unsigned int num_channels, index = 0; + u32 ain[2]; + int ret; + + num_channels = device_get_child_node_count(dev); + if (num_channels > AD7194_CH_MAX_NR) + return dev_err_probe(dev, -EINVAL, + "Too many channels: %u\n", num_channels); + + num_channels += AD7194_CH_BASE_NR; + + ad7194_channels = devm_kcalloc(dev, num_channels, + sizeof(*ad7194_channels), GFP_KERNEL); + if (!ad7194_channels) + return -ENOMEM; + + indio_dev->channels = ad7194_channels; + indio_dev->num_channels = num_channels; + + device_for_each_child_node(dev, child) { + ret = fwnode_property_read_u32_array(child, "diff-channels", + ain, ARRAY_SIZE(ain)); + if (ret == 0) { + ret = ad7194_validate_ain_channel(dev, ain[0]); + if (ret) + return ret; + + ret = ad7194_validate_ain_channel(dev, ain[1]); + if (ret) + return ret; + + *ad7194_channels = ad7194_chan_diff; + ad7194_channels->scan_index = index++; + ad7194_channels->channel = ain[0]; + ad7194_channels->channel2 = ain[1]; + ad7194_channels->address = AD7194_CH(ain[0], ain[1]); + } else { + ret = fwnode_property_read_u32(child, "single-channel", + &ain[0]); + if (ret) { + fwnode_handle_put(child); + return ret; + } + + ret = ad7194_validate_ain_channel(dev, ain[0]); + if (ret) + return ret; + + *ad7194_channels = ad7194_chan; + ad7194_channels->scan_index = index++; + ad7194_channels->channel = ain[0]; + ad7194_channels->address = AD7194_CH(ain[0], 0); + } + ad7194_channels++; + } + + *ad7194_channels = ad7194_chan_temp; + ad7194_channels->scan_index = index++; + ad7194_channels->address = AD7194_CH_TEMP; + ad7194_channels++; + + *ad7194_channels = ad7194_chan_timestamp; + ad7194_channels->scan_index = index; + + return 0; +} + static const struct ad7192_chip_info ad7192_chip_info_tbl[] = { [ID_AD7190] = { .chip_id = CHIPID_AD7190, @@ -1045,6 +1152,12 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = { .num_channels = ARRAY_SIZE(ad7193_channels), .info = &ad7192_info, }, + [ID_AD7194] = { + .chip_id = CHIPID_AD7194, + .name = "ad7194", + .info = &ad7194_info, + .parse_channels = ad7194_parse_channels, + }, [ID_AD7195] = { .chip_id = CHIPID_AD7195, .name = "ad7195", @@ -1152,9 +1265,15 @@ static int ad7192_probe(struct spi_device *spi) st->chip_info = spi_get_device_match_data(spi); indio_dev->name = st->chip_info->name; indio_dev->modes = INDIO_DIRECT_MODE; - indio_dev->channels = st->chip_info->channels; - indio_dev->num_channels = st->chip_info->num_channels; indio_dev->info = st->chip_info->info; + if (st->chip_info->parse_channels) { + ret = st->chip_info->parse_channels(indio_dev); + if (ret) + return ret; + } else { + indio_dev->channels = st->chip_info->channels; + indio_dev->num_channels = st->chip_info->num_channels; + } ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info); if (ret) @@ -1193,6 +1312,7 @@ static const struct of_device_id ad7192_of_match[] = { { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] }, { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] }, { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] }, + { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] }, { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] }, {} }; @@ -1202,6 +1322,7 @@ static const struct spi_device_id ad7192_ids[] = { { "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] }, { "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] }, { "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] }, + { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] }, { "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] }, {} }; @@ -1218,6 +1339,6 @@ static struct spi_driver ad7192_driver = { module_spi_driver(ad7192_driver); MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); -MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC"); +MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC"); MODULE_LICENSE("GPL v2"); MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
Unlike the other AD719Xs, AD7194 has configurable channels. The user can dynamically configure them in the devicetree. Also modify config AD7192 description for better scaling. Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> --- drivers/iio/adc/Kconfig | 11 +++- drivers/iio/adc/ad7192.c | 129 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 133 insertions(+), 7 deletions(-)