| Message ID | 20240208172459.280189-6-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7192: Add support for AD7194 | expand |
On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > Unlike the other AD719Xs, AD7194 has configurable differential > channels. The default configuration for these channels can be changed > from the devicetree. > > The default configuration is hardcoded in order to have a stable number > of channels. > > Also modify config AD7192 description for better scaling. > > Moved ad7192_chip_info struct definition to allow use of callback > function parse_channels(). > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/iio/adc/Kconfig | 11 ++- > drivers/iio/adc/ad7192.c | 150 ++++++++++++++++++++++++++++++++++++--- > 2 files changed, 148 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 59ae1d17b50d..8062a4d1cbe7 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -71,12 +71,17 @@ config AD7124 > called ad7124. > > 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 d8393ac048e7..a3ff60ed6f63 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. > */ > @@ -125,10 +125,39 @@ > #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */ > #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */ > > +#define AD7194_CH_TEMP 0x100 /* Temp sensor */ > +#define AD7194_CH_AIN1 0x400 /* AIN1 - AINCOM */ > +#define AD7194_CH_AIN2 0x410 /* AIN2 - AINCOM */ > +#define AD7194_CH_AIN3 0x420 /* AIN3 - AINCOM */ > +#define AD7194_CH_AIN4 0x430 /* AIN4 - AINCOM */ > +#define AD7194_CH_AIN5 0x440 /* AIN5 - AINCOM */ > +#define AD7194_CH_AIN6 0x450 /* AIN6 - AINCOM */ > +#define AD7194_CH_AIN7 0x460 /* AIN7 - AINCOM */ > +#define AD7194_CH_AIN8 0x470 /* AIN8 - AINCOM */ > +#define AD7194_CH_AIN9 0x480 /* AIN9 - AINCOM */ > +#define AD7194_CH_AIN10 0x490 /* AIN10 - AINCOM */ > +#define AD7194_CH_AIN11 0x4A0 /* AIN11 - AINCOM */ > +#define AD7194_CH_AIN12 0x4B0 /* AIN12 - AINCOM */ > +#define AD7194_CH_AIN13 0x4C0 /* AIN13 - AINCOM */ > +#define AD7194_CH_AIN14 0x4D0 /* AIN14 - AINCOM */ > +#define AD7194_CH_AIN15 0x4E0 /* AIN15 - AINCOM */ > +#define AD7194_CH_AIN16 0x4F0 /* AIN16 - AINCOM */ > +#define AD7194_CH_POS_MASK GENMASK(7, 4) > +#define AD7194_CH_POS(x) FIELD_PREP(AD7194_CH_POS_MASK, (x)) AD7194_CH_POS_MASK isn't used elsewhere, so maybe just this? #define AD7194_CH_POS(x) FIELD_PREP(GENMASK(7, 4), (x)) > +#define AD7194_CH_NEG_MASK GENMASK(3, 0) > +#define AD7194_CH_NEG(x) FIELD_PREP(AD7194_CH_NEG_MASK, (x)) > +#define AD7194_CH_DIFF(pos, neg) \ > + (AD7194_CH_POS(pos) | AD7194_CH_NEG(neg)) You could also add `((neg) == 0 ? BIT(10) : 0) | ...` and then use this macro to define AD7194_CH_AINx. #define AD7194_CH_AIN1 AD7194_CH_DIFF(1, 0) > +#define AD7194_CH_DIFF_START 0 > +#define AD7194_CH_DIFF_NR 8 > +#define AD7194_CH_AIN_START 1 > +#define AD7194_CH_AIN_NR 16 > + > /* 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) > > @@ -166,17 +195,10 @@ enum { > ID_AD7190, > ID_AD7192, > ID_AD7193, > + ID_AD7194, > ID_AD7195, > }; > > -struct ad7192_chip_info { > - unsigned int chip_id; > - const char *name; > - const struct iio_chan_spec *channels; > - u8 num_channels; > - const struct iio_info *info; > -}; > - > struct ad7192_state { > const struct ad7192_chip_info *chip_info; > struct regulator *avdd; > @@ -197,6 +219,15 @@ struct ad7192_state { > struct ad_sigma_delta sd; > }; > > +struct ad7192_chip_info { > + unsigned int chip_id; > + const char *name; > + const struct iio_chan_spec *channels; > + u8 num_channels; > + const struct iio_info *info; > + int (*parse_channels)(struct ad7192_state *st); > +}; > + > static const char * const ad7192_syscalib_modes[] = { > [AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale", > [AD7192_SYSCALIB_FULL_SCALE] = "full_scale", > @@ -918,6 +949,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, > +}; Isn't this identical to ad7192_info and ad7195_info now that .attrs is removed? It seems like we could consolidate here. > + > static const struct iio_info ad7195_info = { > .read_raw = ad7192_read_raw, > .write_raw = ad7192_write_raw, > @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(14), > }; > > +static struct iio_chan_spec ad7194_channels[] = { > + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), > + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), > + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), > + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), > + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), > + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), > + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), > + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), > + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), > + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), > + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), > + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), > + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), > + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), > + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), > + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), > + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), > + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), > + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), > + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), > + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), > + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), > + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), > + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), > + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), Shouldn't these be differential channels since they are pseudo-differential inputs measuring the difference between AINx and AINCOM? > + IIO_CHAN_SOFT_TIMESTAMP(25), > +}; i.e. like this (where AINCOM is voltage0 AINx is voltagex) static struct iio_chan_spec ad7194_channels[] = { AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1), AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2), AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3), AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4), AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5), AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6), AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7), AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8), AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9), AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10), AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11), AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12), AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13), AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14), AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15), AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16), AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP), IIO_CHAN_SOFT_TIMESTAMP(17), }; > + > +static int ad7192_parse_channel(struct fwnode_handle *child) > +{ > + u32 reg, ain[2]; > + int ret; > + > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) > + return ret; > + > + if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR)) > + return -EINVAL; > + > + ret = fwnode_property_read_u32_array(child, "diff-channels", ain, > + ARRAY_SIZE(ain)); > + if (ret) > + return ret; > + > + if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) || > + !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR)) > + return -EINVAL; > + > + ad7194_channels[reg].channel = ain[0]; > + ad7194_channels[reg].channel2 = ain[1]; > + ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]); ... then the suggested change to AD7194_CH_DIFF above also make this work when ain[1] is zero which should be allowed in the range check immediately before this. > + > + return 0; > +} > + > +static int ad7192_parse_channels(struct ad7192_state *st) > +{ > + struct device *dev = &st->sd.spi->dev; > + struct fwnode_handle *child; > + int ret; > + > + device_for_each_child_node(dev, child) { > + ret = ad7192_parse_channel(child); > + if (ret) { > + fwnode_handle_put(child); > + return ret; > + } > + } > + > + return 0; > +} > + > static const struct ad7192_chip_info ad7192_chip_info_tbl[] = { > [ID_AD7190] = { > .chip_id = CHIPID_AD7190, > @@ -1031,6 +1145,14 @@ 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", > + .channels = ad7194_channels, > + .num_channels = ARRAY_SIZE(ad7194_channels), > + .info = &ad7194_info, > + .parse_channels = ad7192_parse_channels, > + }, > [ID_AD7195] = { > .chip_id = CHIPID_AD7195, > .name = "ad7195", > @@ -1142,6 +1264,12 @@ static int ad7192_probe(struct spi_device *spi) > } > } > > + if (st->chip_info->parse_channels) { > + ret = st->chip_info->parse_channels(st); > + if (ret) > + return ret; > + } > + > ret = ad7192_setup(st); > if (ret) > return ret; > @@ -1153,6 +1281,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] }, > {} > }; > @@ -1162,6 +1291,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] }, > {} > }; > @@ -1178,6 +1308,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); > -- > 2.34.1 >
Hello and thank you for the feedback! On 09.02.2024 00:27, David Lechner wrote: > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman > <alisadariana@gmail.com> wrote: >> >> Unlike the other AD719Xs, AD7194 has configurable differential >> channels. The default configuration for these channels can be changed >> from the devicetree. ... >> >> +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, >> +}; > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is > removed? It seems like we could consolidate here. Those are not exactly identical since: 92 has bridge switch attribute, 95 has bridge switch and ac excitation attributes and 94 has no custom attributes. I used a different info structure for 94 in order to avoid showing extra attributes. > >> + >> static const struct iio_info ad7195_info = { >> .read_raw = ad7192_read_raw, >> .write_raw = ad7192_write_raw, >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { >> IIO_CHAN_SOFT_TIMESTAMP(14), >> }; >> >> +static struct iio_chan_spec ad7194_channels[] = { >> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), >> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), >> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), >> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), >> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), >> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), >> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), >> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), >> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), >> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), >> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), >> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), >> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), >> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), >> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), >> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), >> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), >> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), >> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), >> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), >> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), >> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), >> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), >> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), >> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), > > Shouldn't these be differential channels since they are > pseudo-differential inputs measuring the difference between AINx and > AINCOM? > >> + IIO_CHAN_SOFT_TIMESTAMP(25), >> +}; > > i.e. like this (where AINCOM is voltage0 AINx is voltagex) > > static struct iio_chan_spec ad7194_channels[] = { > AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1), > AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2), > AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3), > AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4), > AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5), > AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6), > AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7), > AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8), > AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9), > AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10), > AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11), > AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12), > AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13), > AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14), > AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15), > AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16), > AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP), > IIO_CHAN_SOFT_TIMESTAMP(17), > }; > I tried to follow the existing style of the driver: for each pseudo-differential channel(AINx - AINCOM) there is an iio channel like this in_voltagex_raw; and for each differential channel(AINx - AINy) there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194 has 16 pseudo-differential channels/8 fully differential channels so I thought the (AINx - AINCOM) channels should be static and only the 8 differential ones could be configured by the user from the devicetree by choosing the input pins. The existing style of the driver, AD7192 has 4 pseudo differential channels and 2 (non configurable) differential channels: static const struct iio_chan_spec ad7192_channels[] = { AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), AD719x_CHANNEL(7, 4, AD7192_CH_AIN4), IIO_CHAN_SOFT_TIMESTAMP(8), }; Would it be better to respect the existing style or to do like you suggested and have a total of 16 differential channels that are configurable from the device tree? Kind regards, Alisa-Dariana Roman
On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > Hello and thank you for the feedback! > > On 09.02.2024 00:27, David Lechner wrote: > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman > > <alisadariana@gmail.com> wrote: > >> > >> Unlike the other AD719Xs, AD7194 has configurable differential > >> channels. The default configuration for these channels can be changed > >> from the devicetree. > > ... > > >> > >> +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, > >> +}; > > > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is > > removed? It seems like we could consolidate here. > > Those are not exactly identical since: 92 has bridge switch attribute, > 95 has bridge switch and ac excitation attributes and 94 has no custom > attributes. I used a different info structure for 94 in order to avoid > showing extra attributes. > Ah, I see what you mean. I didn't look close enough at the other patch removing one attribute to see that were still other attributes. > > > >> + > >> static const struct iio_info ad7195_info = { > >> .read_raw = ad7192_read_raw, > >> .write_raw = ad7192_write_raw, > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { > >> IIO_CHAN_SOFT_TIMESTAMP(14), > >> }; > >> > >> +static struct iio_chan_spec ad7194_channels[] = { > >> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), > >> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), > >> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), > >> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), > >> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), > >> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), > >> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), > >> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), > >> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), > >> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), > >> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), > >> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), > >> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), > >> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), > >> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), > >> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), > >> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), > >> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), > >> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), > >> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), > >> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), > >> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), > >> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), > >> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), > >> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), > > > > Shouldn't these be differential channels since they are > > pseudo-differential inputs measuring the difference between AINx and > > AINCOM? > > > >> + IIO_CHAN_SOFT_TIMESTAMP(25), > >> +}; > > > > i.e. like this (where AINCOM is voltage0 AINx is voltagex) > > > > static struct iio_chan_spec ad7194_channels[] = { > > AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1), > > AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2), > > AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3), > > AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4), > > AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5), > > AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6), > > AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7), > > AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8), > > AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9), > > AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10), > > AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11), > > AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12), > > AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13), > > AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14), > > AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15), > > AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16), > > AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP), > > IIO_CHAN_SOFT_TIMESTAMP(17), > > }; > > > > I tried to follow the existing style of the driver: for each > pseudo-differential channel(AINx - AINCOM) there is an iio channel like > this in_voltagex_raw; and for each differential channel(AINx - AINy) > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194 > has 16 pseudo-differential channels/8 fully differential channels so I > thought the (AINx - AINCOM) channels should be static and only the 8 > differential ones could be configured by the user from the devicetree by > choosing the input pins. > > The existing style of the driver, AD7192 has 4 pseudo differential > channels and 2 (non configurable) differential channels: > static const struct iio_chan_spec ad7192_channels[] = { > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > AD719x_CHANNEL(7, 4, AD7192_CH_AIN4), > IIO_CHAN_SOFT_TIMESTAMP(8), > }; > > Would it be better to respect the existing style or to do like you > suggested and have a total of 16 differential channels that are > configurable from the device tree? Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was done this way since only certain combinations of inputs can be used together. (Although I think indexes 4 to 7 should really be differential because they are the difference between the input and AINCOM which may not be GND, but it is probably too late to fix that.) Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is much more configurable than AD7192 when it comes to assigning channels. There are basically no restrictions on which inputs can be used together. So I am still confident that my suggestion is the way to go for AD7194. (Although I didn't actually try it on hardware, so can't be 100% confident. But at least 90% confident :-p)
On Thu, 15 Feb 2024 11:13:19 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman > <alisadariana@gmail.com> wrote: > > > > Hello and thank you for the feedback! > > > > On 09.02.2024 00:27, David Lechner wrote: > > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman > > > <alisadariana@gmail.com> wrote: > > >> > > >> Unlike the other AD719Xs, AD7194 has configurable differential > > >> channels. The default configuration for these channels can be changed > > >> from the devicetree. > > > > ... > > > > >> > > >> +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, > > >> +}; > > > > > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is > > > removed? It seems like we could consolidate here. > > > > Those are not exactly identical since: 92 has bridge switch attribute, > > 95 has bridge switch and ac excitation attributes and 94 has no custom > > attributes. I used a different info structure for 94 in order to avoid > > showing extra attributes. > > > > Ah, I see what you mean. I didn't look close enough at the other patch > removing one attribute to see that were still other attributes. > > > > > > >> + > > >> static const struct iio_info ad7195_info = { > > >> .read_raw = ad7192_read_raw, > > >> .write_raw = ad7192_write_raw, > > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { > > >> IIO_CHAN_SOFT_TIMESTAMP(14), > > >> }; > > >> > > >> +static struct iio_chan_spec ad7194_channels[] = { > > >> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), > > >> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), > > >> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), > > >> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), > > >> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), > > >> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), > > >> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), > > >> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), > > >> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), > > >> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), > > >> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), > > >> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), > > >> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), > > >> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), > > >> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), > > >> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), > > >> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), > > >> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), > > >> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), > > >> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), > > >> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), > > >> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), > > >> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), > > >> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), > > >> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), > > > > > > Shouldn't these be differential channels since they are > > > pseudo-differential inputs measuring the difference between AINx and > > > AINCOM? > > > > > >> + IIO_CHAN_SOFT_TIMESTAMP(25), > > >> +}; > > > > > > i.e. like this (where AINCOM is voltage0 AINx is voltagex) > > > > > > static struct iio_chan_spec ad7194_channels[] = { > > > AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1), > > > AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2), > > > AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3), > > > AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4), > > > AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5), > > > AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6), > > > AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7), > > > AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8), > > > AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9), > > > AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10), > > > AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11), > > > AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12), > > > AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13), > > > AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14), > > > AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15), > > > AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16), > > > AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP), > > > IIO_CHAN_SOFT_TIMESTAMP(17), > > > }; > > > > > > > I tried to follow the existing style of the driver: for each > > pseudo-differential channel(AINx - AINCOM) there is an iio channel like > > this in_voltagex_raw; and for each differential channel(AINx - AINy) > > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194 > > has 16 pseudo-differential channels/8 fully differential channels so I > > thought the (AINx - AINCOM) channels should be static and only the 8 > > differential ones could be configured by the user from the devicetree by > > choosing the input pins. > > > > The existing style of the driver, AD7192 has 4 pseudo differential > > channels and 2 (non configurable) differential channels: > > static const struct iio_chan_spec ad7192_channels[] = { > > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > > AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > > AD719x_CHANNEL(7, 4, AD7192_CH_AIN4), > > IIO_CHAN_SOFT_TIMESTAMP(8), > > }; > > > > Would it be better to respect the existing style or to do like you > > suggested and have a total of 16 differential channels that are > > configurable from the device tree? > > Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was > done this way since only certain combinations of inputs can be used > together. (Although I think indexes 4 to 7 should really be > differential because they are the difference between the input and > AINCOM which may not be GND, but it is probably too late to fix that.) Ground is never absolute anyway, but we could indeed be relative to something that changes. Ah well - no one has asked for it on that part I guess so not important. > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is > much more configurable than AD7192 when it comes to assigning > channels. There are basically no restrictions on which inputs can be > used together. So I am still confident that my suggestion is the way > to go for AD7194. (Although I didn't actually try it on hardware, so > can't be 100% confident. But at least 90% confident :-p) You would have to define a channel number for aincom. There is an explicit example in the datasheet of it being at 2.5V using a reference supply. I wonder what expectation here is. Allways a reference regulator on that pin, or an actually varying input? Maybe in long term we want to support both options - so if aincom-supply is provided these are single ended with an offset, but if not they are differential channels between channel X and channel AINCOM. Note though that this mode is described a pseudo differential which normally means a fixed voltage on the negative. So gut feeling from me is treat them as single ended and add an aincom-supply + the offsets that result if that is provided in DT and voltage from it is non 0. What fun. Jonathan
On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 15 Feb 2024 11:13:19 -0600 > David Lechner <dlechner@baylibre.com> wrote: > ... > > > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is > > much more configurable than AD7192 when it comes to assigning > > channels. There are basically no restrictions on which inputs can be > > used together. So I am still confident that my suggestion is the way > > to go for AD7194. (Although I didn't actually try it on hardware, so > > can't be 100% confident. But at least 90% confident :-p) > > You would have to define a channel number for aincom. There is an explicit > example in the datasheet of it being at 2.5V using a reference supply. > > I wonder what expectation here is. Allways a reference regulator on that pin, or > an actually varying input? Maybe in long term we want to support both > options - so if aincom-supply is provided these are single ended with > an offset, but if not they are differential channels between channel X and > channel AINCOM. > > Note though that this mode is described a pseudo differential which normally > means a fixed voltage on the negative. > > So gut feeling from me is treat them as single ended and add an > aincom-supply + the offsets that result if that is provided in DT and > voltage from it is non 0. Calling AINCOM a supply doesn't sound right to me since usually this signal is coming somewhere external, i.e. you have a twisted pair connected to AIN1 and AINCOM going to some signal source that may be hot-pluggable and not known at compile time. As an example, if AINCOM was modeled as a supply, then we would have to change the device tree every time we changed the voltage offset on the signal generator while we are testing using an evaluation board.
On Fri, 16 Feb 2024 10:57:33 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 15 Feb 2024 11:13:19 -0600 > > David Lechner <dlechner@baylibre.com> wrote: > > > > ... > > > > > > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is > > > much more configurable than AD7192 when it comes to assigning > > > channels. There are basically no restrictions on which inputs can be > > > used together. So I am still confident that my suggestion is the way > > > to go for AD7194. (Although I didn't actually try it on hardware, so > > > can't be 100% confident. But at least 90% confident :-p) > > > > You would have to define a channel number for aincom. There is an explicit > > example in the datasheet of it being at 2.5V using a reference supply. > > > > I wonder what expectation here is. Allways a reference regulator on that pin, or > > an actually varying input? Maybe in long term we want to support both > > options - so if aincom-supply is provided these are single ended with > > an offset, but if not they are differential channels between channel X and > > channel AINCOM. > > > > Note though that this mode is described a pseudo differential which normally > > means a fixed voltage on the negative. > > > > So gut feeling from me is treat them as single ended and add an > > aincom-supply + the offsets that result if that is provided in DT and > > voltage from it is non 0. > > Calling AINCOM a supply doesn't sound right to me since usually this > signal is coming somewhere external, i.e. you have a twisted pair > connected to AIN1 and AINCOM going to some signal source that may be > hot-pluggable and not known at compile time. As an example, if AINCOM > was modeled as a supply, then we would have to change the device tree > every time we changed the voltage offset on the signal generator while > we are testing using an evaluation board. We tend to stick away from designing features to support testing with devboards where external wiring is involved because anything could be wired up there. (Examples are things like shunt resistors - normally they are DT only) So sometimes it's a bit painful to work with such boards. The main focus has to be production devices or at least stable set ups where a fixed DT is sufficient. So I'm more interested in focusing on production device use cases. Do we have an information on how this is this used in those environments? Jonathan
On Sat, Feb 17, 2024 at 10:25 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 16 Feb 2024 10:57:33 -0600 > David Lechner <dlechner@baylibre.com> wrote: > > > On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Thu, 15 Feb 2024 11:13:19 -0600 > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > > > ... > > > > > > > > > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is > > > > much more configurable than AD7192 when it comes to assigning > > > > channels. There are basically no restrictions on which inputs can be > > > > used together. So I am still confident that my suggestion is the way > > > > to go for AD7194. (Although I didn't actually try it on hardware, so > > > > can't be 100% confident. But at least 90% confident :-p) > > > > > > You would have to define a channel number for aincom. There is an explicit > > > example in the datasheet of it being at 2.5V using a reference supply. > > > > > > I wonder what expectation here is. Allways a reference regulator on that pin, or > > > an actually varying input? Maybe in long term we want to support both > > > options - so if aincom-supply is provided these are single ended with > > > an offset, but if not they are differential channels between channel X and > > > channel AINCOM. > > > > > > Note though that this mode is described a pseudo differential which normally > > > means a fixed voltage on the negative. > > > > > > So gut feeling from me is treat them as single ended and add an > > > aincom-supply + the offsets that result if that is provided in DT and > > > voltage from it is non 0. > > > > Calling AINCOM a supply doesn't sound right to me since usually this > > signal is coming somewhere external, i.e. you have a twisted pair > > connected to AIN1 and AINCOM going to some signal source that may be > > hot-pluggable and not known at compile time. As an example, if AINCOM > > was modeled as a supply, then we would have to change the device tree > > every time we changed the voltage offset on the signal generator while > > we are testing using an evaluation board. > > We tend to stick away from designing features to support testing with > devboards where external wiring is involved because anything could be > wired up there. (Examples are things like shunt resistors - normally > they are DT only) So sometimes it's a bit painful to work with such boards. > The main focus has to be production devices or at least stable set ups > where a fixed DT is sufficient. > > So I'm more interested in focusing on production device use cases. > Do we have an information on how this is this used in those environments? > Point taken. I also checked with an apps engineer at ADI and it does sound like AINCOM should be a supply.
On Thu, Feb 15, 2024 at 11:13 AM David Lechner <dlechner@baylibre.com> wrote: > > On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman > <alisadariana@gmail.com> wrote: > > > > Hello and thank you for the feedback! > > > > On 09.02.2024 00:27, David Lechner wrote: > > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman > > > <alisadariana@gmail.com> wrote: > > >> > > >> Unlike the other AD719Xs, AD7194 has configurable differential > > >> channels. The default configuration for these channels can be changed > > >> from the devicetree. > > > > ... > > > > >> > > >> +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, > > >> +}; > > > > > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is > > > removed? It seems like we could consolidate here. > > > > Those are not exactly identical since: 92 has bridge switch attribute, > > 95 has bridge switch and ac excitation attributes and 94 has no custom > > attributes. I used a different info structure for 94 in order to avoid > > showing extra attributes. > > > > Ah, I see what you mean. I didn't look close enough at the other patch > removing one attribute to see that were still other attributes. > > > > > > >> + > > >> static const struct iio_info ad7195_info = { > > >> .read_raw = ad7192_read_raw, > > >> .write_raw = ad7192_write_raw, > > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { > > >> IIO_CHAN_SOFT_TIMESTAMP(14), > > >> }; > > >> > > >> +static struct iio_chan_spec ad7194_channels[] = { > > >> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), > > >> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), > > >> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), > > >> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), > > >> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), > > >> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), > > >> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), > > >> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), > > >> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), > > >> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), > > >> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), > > >> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), > > >> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), > > >> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), > > >> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), > > >> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), > > >> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), > > >> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), > > >> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), > > >> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), > > >> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), > > >> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), > > >> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), > > >> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), > > >> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), > > > > > > Shouldn't these be differential channels since they are > > > pseudo-differential inputs measuring the difference between AINx and > > > AINCOM? > > > > > >> + IIO_CHAN_SOFT_TIMESTAMP(25), > > >> +}; > > > > > > i.e. like this (where AINCOM is voltage0 AINx is voltagex) > > > > > > static struct iio_chan_spec ad7194_channels[] = { > > > AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1), > > > AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2), > > > AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3), > > > AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4), > > > AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5), > > > AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6), > > > AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7), > > > AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8), > > > AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9), > > > AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10), > > > AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11), > > > AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12), > > > AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13), > > > AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14), > > > AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15), > > > AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16), > > > AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP), > > > IIO_CHAN_SOFT_TIMESTAMP(17), > > > }; > > > > > > > I tried to follow the existing style of the driver: for each > > pseudo-differential channel(AINx - AINCOM) there is an iio channel like > > this in_voltagex_raw; and for each differential channel(AINx - AINy) > > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194 > > has 16 pseudo-differential channels/8 fully differential channels so I > > thought the (AINx - AINCOM) channels should be static and only the 8 > > differential ones could be configured by the user from the devicetree by > > choosing the input pins. > > > > The existing style of the driver, AD7192 has 4 pseudo differential > > channels and 2 (non configurable) differential channels: > > static const struct iio_chan_spec ad7192_channels[] = { > > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > > AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > > AD719x_CHANNEL(7, 4, AD7192_CH_AIN4), > > IIO_CHAN_SOFT_TIMESTAMP(8), > > }; > > > > Would it be better to respect the existing style or to do like you > > suggested and have a total of 16 differential channels that are > > configurable from the device tree? Now that we have it sorted that AINCOM should be a supply, it does sound like we should more closely follow the pattern from AD7192. But to cover every possible programmable combination of AINx to AINy, we would need 256 differential channels (16 * 16) in addition to the other channels. Realistically, we probably don't need that many though. Since I see that AD7192 has a differential channel where uses AIN2 for both + and - I guess having 16 differential channels that are configured via device tree would be enough to allow all 16 AINx inputs to be used this way. Is there a use case where the same AINx would be assigned to multiple channels at the same time? > > Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was > done this way since only certain combinations of inputs can be used > together. (Although I think indexes 4 to 7 should really be > differential because they are the difference between the input and > AINCOM which may not be GND, but it is probably too late to fix that.) > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is > much more configurable than AD7192 when it comes to assigning > channels. There are basically no restrictions on which inputs can be > used together. So I am still confident that my suggestion is the way > to go for AD7194. (Although I didn't actually try it on hardware, so > can't be 100% confident. But at least 90% confident :-p)
On Mon, 19 Feb 2024 10:33:45 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Feb 15, 2024 at 11:13 AM David Lechner <dlechner@baylibre.com> wrote: > > > > On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman > > <alisadariana@gmail.com> wrote: > > > > > > Hello and thank you for the feedback! > > > > > > On 09.02.2024 00:27, David Lechner wrote: > > > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman > > > > <alisadariana@gmail.com> wrote: > > > >> > > > >> Unlike the other AD719Xs, AD7194 has configurable differential > > > >> channels. The default configuration for these channels can be changed > > > >> from the devicetree. > > > > > > ... > > > > > > >> > > > >> +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, > > > >> +}; > > > > > > > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is > > > > removed? It seems like we could consolidate here. > > > > > > Those are not exactly identical since: 92 has bridge switch attribute, > > > 95 has bridge switch and ac excitation attributes and 94 has no custom > > > attributes. I used a different info structure for 94 in order to avoid > > > showing extra attributes. > > > > > > > Ah, I see what you mean. I didn't look close enough at the other patch > > removing one attribute to see that were still other attributes. > > > > > > > > > >> + > > > >> static const struct iio_info ad7195_info = { > > > >> .read_raw = ad7192_read_raw, > > > >> .write_raw = ad7192_write_raw, > > > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { > > > >> IIO_CHAN_SOFT_TIMESTAMP(14), > > > >> }; > > > >> > > > >> +static struct iio_chan_spec ad7194_channels[] = { > > > >> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), > > > >> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), > > > >> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), > > > >> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), > > > >> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), > > > >> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), > > > >> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), > > > >> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), > > > >> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), > > > >> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), > > > >> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), > > > >> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), > > > >> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), > > > >> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), > > > >> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), > > > >> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), > > > >> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), > > > >> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), > > > >> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), > > > >> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), > > > >> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), > > > >> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), > > > >> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), > > > >> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), > > > >> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), > > > > > > > > Shouldn't these be differential channels since they are > > > > pseudo-differential inputs measuring the difference between AINx and > > > > AINCOM? > > > > > > > >> + IIO_CHAN_SOFT_TIMESTAMP(25), > > > >> +}; > > > > > > > > i.e. like this (where AINCOM is voltage0 AINx is voltagex) > > > > > > > > static struct iio_chan_spec ad7194_channels[] = { > > > > AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1), > > > > AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2), > > > > AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3), > > > > AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4), > > > > AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5), > > > > AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6), > > > > AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7), > > > > AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8), > > > > AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9), > > > > AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10), > > > > AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11), > > > > AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12), > > > > AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13), > > > > AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14), > > > > AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15), > > > > AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16), > > > > AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP), > > > > IIO_CHAN_SOFT_TIMESTAMP(17), > > > > }; > > > > > > > > > > I tried to follow the existing style of the driver: for each > > > pseudo-differential channel(AINx - AINCOM) there is an iio channel like > > > this in_voltagex_raw; and for each differential channel(AINx - AINy) > > > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194 > > > has 16 pseudo-differential channels/8 fully differential channels so I > > > thought the (AINx - AINCOM) channels should be static and only the 8 > > > differential ones could be configured by the user from the devicetree by > > > choosing the input pins. > > > > > > The existing style of the driver, AD7192 has 4 pseudo differential > > > channels and 2 (non configurable) differential channels: > > > static const struct iio_chan_spec ad7192_channels[] = { > > > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > > > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > > > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > > > AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > > > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > > > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > > > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > > > AD719x_CHANNEL(7, 4, AD7192_CH_AIN4), > > > IIO_CHAN_SOFT_TIMESTAMP(8), > > > }; > > > > > > Would it be better to respect the existing style or to do like you > > > suggested and have a total of 16 differential channels that are > > > configurable from the device tree? > > > Now that we have it sorted that AINCOM should be a supply, it does > sound like we should more closely follow the pattern from AD7192. But > to cover every possible programmable combination of AINx to AINy, we > would need 256 differential channels (16 * 16) in addition to the > other channels. Realistically, we probably don't need that many > though. Since I see that AD7192 has a differential channel where uses > AIN2 for both + and - I guess having 16 differential channels that are > configured via device tree would be enough to allow all 16 AINx inputs > to be used this way. Is there a use case where the same AINx would be > assigned to multiple channels at the same time? If there are very large numbers of options, common solution is to move to dynamic assignment and channel nodes so DT specifies what is wired. In theory we then allow all combinations at the same time but rely on common sense to restrict it. I don't suggest channel nodes for most drivers because it adds complexity and a few unwired channels being exposed is rarely a problem (mostly people buy the right sized ADC). For cases like this though it can reduce things to a manageable level. Also little purpose in supporting 1-2 and 2-1 which can simplify things somewhat. However that can also be left unconstrained on assumption common sense will be exercised in the DT. > > > > > Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was > > done this way since only certain combinations of inputs can be used > > together. (Although I think indexes 4 to 7 should really be > > differential because they are the difference between the input and > > AINCOM which may not be GND, but it is probably too late to fix that.) > > > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is > > much more configurable than AD7192 when it comes to assigning > > channels. There are basically no restrictions on which inputs can be > > used together. So I am still confident that my suggestion is the way > > to go for AD7194. (Although I didn't actually try it on hardware, so > > can't be 100% confident. But at least 90% confident :-p)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 59ae1d17b50d..8062a4d1cbe7 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -71,12 +71,17 @@ config AD7124 called ad7124. 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 d8393ac048e7..a3ff60ed6f63 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. */ @@ -125,10 +125,39 @@ #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */ #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */ +#define AD7194_CH_TEMP 0x100 /* Temp sensor */ +#define AD7194_CH_AIN1 0x400 /* AIN1 - AINCOM */ +#define AD7194_CH_AIN2 0x410 /* AIN2 - AINCOM */ +#define AD7194_CH_AIN3 0x420 /* AIN3 - AINCOM */ +#define AD7194_CH_AIN4 0x430 /* AIN4 - AINCOM */ +#define AD7194_CH_AIN5 0x440 /* AIN5 - AINCOM */ +#define AD7194_CH_AIN6 0x450 /* AIN6 - AINCOM */ +#define AD7194_CH_AIN7 0x460 /* AIN7 - AINCOM */ +#define AD7194_CH_AIN8 0x470 /* AIN8 - AINCOM */ +#define AD7194_CH_AIN9 0x480 /* AIN9 - AINCOM */ +#define AD7194_CH_AIN10 0x490 /* AIN10 - AINCOM */ +#define AD7194_CH_AIN11 0x4A0 /* AIN11 - AINCOM */ +#define AD7194_CH_AIN12 0x4B0 /* AIN12 - AINCOM */ +#define AD7194_CH_AIN13 0x4C0 /* AIN13 - AINCOM */ +#define AD7194_CH_AIN14 0x4D0 /* AIN14 - AINCOM */ +#define AD7194_CH_AIN15 0x4E0 /* AIN15 - AINCOM */ +#define AD7194_CH_AIN16 0x4F0 /* AIN16 - AINCOM */ +#define AD7194_CH_POS_MASK GENMASK(7, 4) +#define AD7194_CH_POS(x) FIELD_PREP(AD7194_CH_POS_MASK, (x)) +#define AD7194_CH_NEG_MASK GENMASK(3, 0) +#define AD7194_CH_NEG(x) FIELD_PREP(AD7194_CH_NEG_MASK, (x)) +#define AD7194_CH_DIFF(pos, neg) \ + (AD7194_CH_POS(pos) | AD7194_CH_NEG(neg)) +#define AD7194_CH_DIFF_START 0 +#define AD7194_CH_DIFF_NR 8 +#define AD7194_CH_AIN_START 1 +#define AD7194_CH_AIN_NR 16 + /* 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) @@ -166,17 +195,10 @@ enum { ID_AD7190, ID_AD7192, ID_AD7193, + ID_AD7194, ID_AD7195, }; -struct ad7192_chip_info { - unsigned int chip_id; - const char *name; - const struct iio_chan_spec *channels; - u8 num_channels; - const struct iio_info *info; -}; - struct ad7192_state { const struct ad7192_chip_info *chip_info; struct regulator *avdd; @@ -197,6 +219,15 @@ struct ad7192_state { struct ad_sigma_delta sd; }; +struct ad7192_chip_info { + unsigned int chip_id; + const char *name; + const struct iio_chan_spec *channels; + u8 num_channels; + const struct iio_info *info; + int (*parse_channels)(struct ad7192_state *st); +}; + static const char * const ad7192_syscalib_modes[] = { [AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale", [AD7192_SYSCALIB_FULL_SCALE] = "full_scale", @@ -918,6 +949,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, @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = { IIO_CHAN_SOFT_TIMESTAMP(14), }; +static struct iio_chan_spec ad7194_channels[] = { + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001), + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023), + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045), + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067), + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089), + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB), + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD), + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF), + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP), + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1), + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2), + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3), + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4), + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5), + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6), + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7), + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8), + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9), + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10), + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11), + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12), + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13), + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14), + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15), + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16), + IIO_CHAN_SOFT_TIMESTAMP(25), +}; + +static int ad7192_parse_channel(struct fwnode_handle *child) +{ + u32 reg, ain[2]; + int ret; + + ret = fwnode_property_read_u32(child, "reg", ®); + if (ret) + return ret; + + if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR)) + return -EINVAL; + + ret = fwnode_property_read_u32_array(child, "diff-channels", ain, + ARRAY_SIZE(ain)); + if (ret) + return ret; + + if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) || + !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR)) + return -EINVAL; + + ad7194_channels[reg].channel = ain[0]; + ad7194_channels[reg].channel2 = ain[1]; + ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]); + + return 0; +} + +static int ad7192_parse_channels(struct ad7192_state *st) +{ + struct device *dev = &st->sd.spi->dev; + struct fwnode_handle *child; + int ret; + + device_for_each_child_node(dev, child) { + ret = ad7192_parse_channel(child); + if (ret) { + fwnode_handle_put(child); + return ret; + } + } + + return 0; +} + static const struct ad7192_chip_info ad7192_chip_info_tbl[] = { [ID_AD7190] = { .chip_id = CHIPID_AD7190, @@ -1031,6 +1145,14 @@ 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", + .channels = ad7194_channels, + .num_channels = ARRAY_SIZE(ad7194_channels), + .info = &ad7194_info, + .parse_channels = ad7192_parse_channels, + }, [ID_AD7195] = { .chip_id = CHIPID_AD7195, .name = "ad7195", @@ -1142,6 +1264,12 @@ static int ad7192_probe(struct spi_device *spi) } } + if (st->chip_info->parse_channels) { + ret = st->chip_info->parse_channels(st); + if (ret) + return ret; + } + ret = ad7192_setup(st); if (ret) return ret; @@ -1153,6 +1281,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] }, {} }; @@ -1162,6 +1291,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] }, {} }; @@ -1178,6 +1308,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);