On Sat, Apr 13, 2024 at 10:13 AM Alisa-Dariana Roman <alisadariana@xxxxxxxxx> wrote: > > Unlike the other AD719Xs, AD7194 has configurable differential > channels. The user can dynamically configure them in the devicetree. > > Also modify config AD7192 description for better scaling. > > Moved ad7192_chip_info struct definition to allow use of callback > function parse_channels(). It looks like this no longer needs to be moved in this revision. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx> > --- > drivers/iio/adc/Kconfig | 11 ++- > drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++--- > 2 files changed, 138 insertions(+), 13 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 a9eb4fab39ca..646ab56b87e3 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. > */ > @@ -128,10 +128,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_DIFF(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 18 > +#define AD7194_CH_AIN_START 1 > +#define AD7194_CH_AIN_NR 16 > +#define AD7194_CH_DIFF_NR_MAX 256 > + > /* 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) > > @@ -169,17 +180,10 @@ enum { > ID_AD7190, > ID_AD7192, > ID_AD7193, > + ID_AD7194, > ID_AD7195, > }; > > -struct ad7192_chip_info { > - unsigned int chip_id; > - const char *name; > - 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; > @@ -201,6 +205,15 @@ struct ad7192_state { > struct ad_sigma_delta sd; > }; > > +struct ad7192_chip_info { > + unsigned int chip_id; > + const char *name; > + struct iio_chan_spec *channels; > + u8 num_channels; > + const struct iio_info *info; > + int (*parse_channels)(struct iio_dev *indio_dev); > +}; > + > static const char * const ad7192_syscalib_modes[] = { > [AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale", > [AD7192_SYSCALIB_FULL_SCALE] = "full_scale", > @@ -925,6 +938,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, > @@ -1016,6 +1038,90 @@ static struct iio_chan_spec ad7193_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(14), > }; > > +static int ad7192_parse_channel(struct fwnode_handle *child, > + struct iio_chan_spec *ad7194_channels) nit: this is only requiring one "channel" and is not used as "channels" array, so I would leave out the "s". > +{ > + u32 ain[2]; > + int ret; > + > + 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->channel = ain[0]; > + ad7194_channels->channel2 = ain[1]; > + ad7194_channels->address = AD7194_CH_DIFF(ain[0], ain[1]); > + > + return 0; > +} > + > +static int ad7192_parse_channels(struct iio_dev *indio_dev) Better name might be ad7194_parse_channels() or ad7192_parse_ad7194_channels() since this is specific to the ad7194 chip. > +{ > + struct ad7192_state *st = iio_priv(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, ain_chan; > + int ret; > + > + num_channels = device_get_child_node_count(dev); > + if (num_channels > AD7194_CH_DIFF_NR_MAX) > + return -EINVAL; > + > + num_channels += AD7194_CH_BASE_NR; > + > + ad7194_channels = devm_kcalloc(dev, sizeof(*ad7194_channels), > + num_channels, GFP_KERNEL); nit: technically, the argument order is supposed to be count then size > + if (!ad7194_channels) > + return -ENOMEM; > + > + indio_dev->channels = ad7194_channels; > + indio_dev->num_channels = num_channels; > + > + device_for_each_child_node(dev, child) { > + *ad7194_channels = ad7194_chan_diff; > + ad7194_channels->scan_index = index++; > + ret = ad7192_parse_channel(child, ad7194_channels); > + if (ret) { > + fwnode_handle_put(child); > + return ret; > + } > + ad7194_channels++; > + } > + > + *ad7194_channels = ad7194_chan_temp; > + ad7194_channels->scan_index = index++; > + ad7194_channels->address = AD7194_CH_TEMP; > + ad7194_channels++; nit: It would seem more natural to have all voltage channels altogether rather than having the temperature channel in between. > + > + for (ain_chan = 1; ain_chan <= 16; ain_chan++) { > + if (st->aincom_mv) { > + *ad7194_channels = ad7194_chan; > + } else { > + *ad7194_channels = ad7194_chan_diff; > + ad7194_channels->channel2 = 0; > + } Same comment as on [PATCH 3/5] pseudo-differential channels have differential = 0 and so nothing should depend on st->aincom_mv being 0 or not. > + ad7194_channels->scan_index = index++; > + ad7194_channels->channel = ain_chan; > + ad7194_channels->address = AD7194_CH_DIFF(ain_chan, 0); > + 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, > @@ -1038,6 +1144,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 = ad7192_parse_channels, > + }, > [ID_AD7195] = { > .chip_id = CHIPID_AD7195, > .name = "ad7195", > @@ -1164,6 +1276,12 @@ static int ad7192_probe(struct spi_device *spi) > 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; > + } Take it or leave it, but I think it would be nice to move indio_dev->channels = st->chip_info->channels; indio_dev->num_channels = st->chip_info->num_channels; into an else { } here to make it clear what parse_channels is doing. > + > ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info); > if (ret) > return ret; > @@ -1201,6 +1319,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] }, > {} > }; > @@ -1210,6 +1329,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] }, > {} > }; > @@ -1226,6 +1346,6 @@ static struct spi_driver ad7192_driver = { > module_spi_driver(ad7192_driver); > > MODULE_AUTHOR("Michael Hennerich <michael.hennerich@xxxxxxxxxx>"); > -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 >