Message ID | 20240413151152.165682-1-alisa.roman@analog.com |
---|---|
Headers | show |
Series | iio: adc: ad7192: Add AD7194 support | expand |
On Sat, 13 Apr 2024 18:11:50 +0300 Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > AINCOM should actually be a supply. If present and it has a non-zero > voltage, the pseudo-differential channels are configured as single-ended > with an offset. Otherwise, they are configured as differential channels > between AINx and AINCOM pins. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> Hi Alisa, Main comment here is that you can't modify the channel arrays in place because there may be more that one instance of the hardware. My preference would be to pick between static const iio_chan_spec arrays depending on whether the aincom is provided or not. Jonathan > --- > drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index ac737221beae..a9eb4fab39ca 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -175,7 +175,7 @@ enum { > struct ad7192_chip_info { > unsigned int chip_id; > const char *name; > - const struct iio_chan_spec *channels; > + struct iio_chan_spec *channels; > u8 num_channels; > const struct iio_info *info; > }; > @@ -186,6 +186,7 @@ struct ad7192_state { > struct regulator *vref; > struct clk *mclk; > u16 int_vref_mv; > + u16 aincom_mv; > u32 fclk; > u32 mode; > u32 conf; > @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, > /* Kelvin to Celsius */ > if (chan->type == IIO_TEMP) > *val -= 273 * ad7192_get_temp_scale(unipolar); > + else if (st->aincom_mv && chan->channel2 == -1) > + *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000, > + st->scale_avail[gain][1]); > return IIO_VAL_INT; > case IIO_CHAN_INFO_SAMP_FREQ: > *val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024); > @@ -982,7 +986,7 @@ static const struct iio_info ad7195_info = { > #define AD7193_CHANNEL(_si, _channel1, _address) \ > AD7193_DIFF_CHANNEL(_si, _channel1, -1, _address) > > -static const struct iio_chan_spec ad7192_channels[] = { > +static 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), > @@ -994,7 +998,7 @@ static const struct iio_chan_spec ad7192_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(8), > }; > > -static const struct iio_chan_spec ad7193_channels[] = { > +static struct iio_chan_spec ad7193_channels[] = { No to this. Imagine you have 2 devices wired in parallel to the same host but only one of them is using vincomm. Which setting you get will depend on which order the probe happens in. You need to take a copy of the channels array or have enough variants that you can pick between static const arrays depending on the setting. If that works here that is the preferred path to take. Have ad7193_channels + ad7193_channels_with_vincomm or something like that. > AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M), > AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M), > AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > @@ -1048,11 +1052,27 @@ static void ad7192_reg_disable(void *reg) > regulator_disable(reg); > } > > +static int ad7192_config_channels(struct ad7192_state *st) > +{ > + struct iio_chan_spec *channels = st->chip_info->channels; > + int i; > + > + if (!st->aincom_mv) > + for (i = 0; i < st->chip_info->num_channels; i++) > + if (channels[i].channel2 == -1) { > + channels[i].differential = 1; > + channels[i].channel2 = 0; > + } > + > + return 0; > +} > + > static int ad7192_probe(struct spi_device *spi) > { > struct ad7192_state *st; > struct iio_dev *indio_dev; > - int ret; > + struct regulator *aincom; > + int ret = 0; > > if (!spi->irq) { > dev_err(&spi->dev, "no IRQ?\n"); > @@ -1067,6 +1087,26 @@ static int ad7192_probe(struct spi_device *spi) > > mutex_init(&st->lock); > > + aincom = devm_regulator_get_optional(&spi->dev, "aincom"); > + if (!IS_ERR(aincom)) { > + ret = regulator_enable(aincom); > + if (ret) { > + dev_err(&spi->dev, "Failed to enable specified AINCOM supply\n"); > + return ret; > + } > + > + ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(aincom); > + if (ret < 0) > + return dev_err_probe(&spi->dev, ret, > + "Device tree error, AINCOM voltage undefined\n"); > + } > + > + st->aincom_mv = ret / 1000; I think I'd make the two paths more explicit ret = regulator_get_voltage(aincom); if (ret < 0) return dev_err_probe(&spi->dev, ret, "Device tree error, AINCOM voltage undefined\n"); st->aincom_mv = ret / 1000; } else { st->aincom_mv = 0; } or rely on default for st->aincom_mv already being zero and don't bother adding the else, just move aincom_mv setting into the path with the voltage being read and not the other path. Setting it unconditionally by dividing 0 / 1000 is correct of course, but not particularly obvious! > + > st->avdd = devm_regulator_get(&spi->dev, "avdd"); > if (IS_ERR(st->avdd)) > return PTR_ERR(st->avdd); > @@ -1113,6 +1153,11 @@ static int ad7192_probe(struct spi_device *spi) > st->int_vref_mv = ret / 1000; > > st->chip_info = spi_get_device_match_data(spi); > + > + ret = ad7192_config_channels(st); > + if (ret) > + return ret; > + > indio_dev->name = st->chip_info->name; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = st->chip_info->channels;
On Sat, 13 Apr 2024 18:11:52 +0300 Alisa-Dariana Roman <alisadariana@gmail.com> 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(). > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> The rest of this series looks good to me. Jonathan
On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > AINCOM should actually be a supply. If present and it has a non-zero > voltage, the pseudo-differential channels are configured as single-ended > with an offset. Otherwise, they are configured as differential channels > between AINx and AINCOM pins. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > --- > drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index ac737221beae..a9eb4fab39ca 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -175,7 +175,7 @@ enum { > struct ad7192_chip_info { > unsigned int chip_id; > const char *name; > - const struct iio_chan_spec *channels; > + struct iio_chan_spec *channels; > u8 num_channels; > const struct iio_info *info; > }; > @@ -186,6 +186,7 @@ struct ad7192_state { > struct regulator *vref; > struct clk *mclk; > u16 int_vref_mv; > + u16 aincom_mv; u32? (In case we have a future chip that can go above 6.5535V? > u32 fclk; > u32 mode; > u32 conf; > @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, > /* Kelvin to Celsius */ Not related to this patch, but I'm not a fan of the way the temperature case writes over *val (maybe clean that up using a switch statement instead in another patch while we are working on this?). Adding the else if to this makes it even harder to follow. > if (chan->type == IIO_TEMP) > *val -= 273 * ad7192_get_temp_scale(unipolar); > + else if (st->aincom_mv && chan->channel2 == -1) I think the logic should be !chan->differential instead of chan->channel2 = -1 (more explanation on this below). > + *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000, > + st->scale_avail[gain][1]); > return IIO_VAL_INT; > case IIO_CHAN_INFO_SAMP_FREQ: > *val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024); ... > > +static int ad7192_config_channels(struct ad7192_state *st) > +{ > + struct iio_chan_spec *channels = st->chip_info->channels; > + int i; > + > + if (!st->aincom_mv) As mentioned in my reply to the DT bindings patch, I don't think this logic is correct. AINx/AINCOM input pairs are always pseudo-differential regardless if AINCOM is tied to GND or is a non-zero voltage. Also, to be clear, for pseudo-differential inputs, we want .differential = 0, so the existing channel specs look correct to me and therefore we don't need any changes like this. > + for (i = 0; i < st->chip_info->num_channels; i++) > + if (channels[i].channel2 == -1) { > + channels[i].differential = 1; > + channels[i].channel2 = 0; > + } > + > + return 0; > +} > +
On Sat, Apr 13, 2024 at 10:13 AM Alisa-Dariana Roman <alisadariana@gmail.com> 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@analog.com> > --- > 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@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 >
On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > Replace custom attribute filter_low_pass_3db_frequency_available with > standard attribute. > > Store the available values in ad7192_state struct. > > The function that used to compute those values replaced by > ad7192_update_filter_freq_avail(). > > Function ad7192_show_filter_avail() is no longer needed. > > Note that the initial available values are hardcoded. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > --- With the question below addressed: Reviewed-by: David Lechner <dlechner@baylibre.com> > drivers/iio/adc/ad7192.c | 67 ++++++++++++++++++---------------------- > 1 file changed, 30 insertions(+), 37 deletions(-) > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index 7bcc7e2aa2a2..ac737221beae 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -190,6 +190,7 @@ struct ad7192_state { > u32 mode; > u32 conf; > u32 scale_avail[8][2]; > + u32 filter_freq_avail[4][2]; > u32 oversampling_ratio_avail[4]; > u8 gpocon; > u8 clock_sel; > @@ -473,6 +474,16 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev) > st->oversampling_ratio_avail[2] = 8; > st->oversampling_ratio_avail[3] = 16; > > + st->filter_freq_avail[0][0] = 600; > + st->filter_freq_avail[1][0] = 800; > + st->filter_freq_avail[2][0] = 2300; > + st->filter_freq_avail[3][0] = 2720; > + > + st->filter_freq_avail[0][1] = 1000; > + st->filter_freq_avail[1][1] = 1000; > + st->filter_freq_avail[2][1] = 1000; > + st->filter_freq_avail[3][1] = 1000; > + > return 0; > } > > @@ -586,48 +597,24 @@ static int ad7192_get_f_adc(struct ad7192_state *st) > f_order * FIELD_GET(AD7192_MODE_RATE_MASK, st->mode)); > } > > -static void ad7192_get_available_filter_freq(struct ad7192_state *st, > - int *freq) > +static void ad7192_update_filter_freq_avail(struct ad7192_state *st) > { > unsigned int fadc; > > /* Formulas for filter at page 25 of the datasheet */ > fadc = ad7192_compute_f_adc(st, false, true); > - freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024); > + st->filter_freq_avail[0][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024); > > fadc = ad7192_compute_f_adc(st, true, true); > - freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024); > + st->filter_freq_avail[1][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024); > > fadc = ad7192_compute_f_adc(st, false, false); > - freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024); > + st->filter_freq_avail[2][0] = DIV_ROUND_CLOSEST(fadc * 230, 1024); > > fadc = ad7192_compute_f_adc(st, true, false); > - freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024); > + st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024); > } > > -static ssize_t ad7192_show_filter_avail(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct ad7192_state *st = iio_priv(indio_dev); > - unsigned int freq_avail[4], i; > - size_t len = 0; > - > - ad7192_get_available_filter_freq(st, freq_avail); > - > - for (i = 0; i < ARRAY_SIZE(freq_avail); i++) > - len += sysfs_emit_at(buf, len, "%d.%03d ", freq_avail[i] / 1000, > - freq_avail[i] % 1000); > - > - buf[len - 1] = '\n'; > - > - return len; > -} > - > -static IIO_DEVICE_ATTR(filter_low_pass_3db_frequency_available, > - 0444, ad7192_show_filter_avail, NULL, 0); > - > static IIO_DEVICE_ATTR(bridge_switch_en, 0644, > ad7192_show_bridge_switch, ad7192_set, > AD7192_REG_GPOCON); > @@ -637,7 +624,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, 0644, > AD7192_REG_CONF); > > static struct attribute *ad7192_attributes[] = { > - &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr, > &iio_dev_attr_bridge_switch_en.dev_attr.attr, > NULL > }; > @@ -647,7 +633,6 @@ static const struct attribute_group ad7192_attribute_group = { > }; > > static struct attribute *ad7195_attributes[] = { > - &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr, > &iio_dev_attr_bridge_switch_en.dev_attr.attr, > &iio_dev_attr_ac_excitation_en.dev_attr.attr, > NULL > @@ -665,17 +650,15 @@ static unsigned int ad7192_get_temp_scale(bool unipolar) > static int ad7192_set_3db_filter_freq(struct ad7192_state *st, > int val, int val2) > { > - int freq_avail[4], i, ret, freq; > + int i, ret, freq; > unsigned int diff_new, diff_old; > int idx = 0; > > diff_old = U32_MAX; > freq = val * 1000 + val2; > > - ad7192_get_available_filter_freq(st, freq_avail); > - > - for (i = 0; i < ARRAY_SIZE(freq_avail); i++) { > - diff_new = abs(freq - freq_avail[i]); > + for (i = 0; i < ARRAY_SIZE(st->filter_freq_avail); i++) { > + diff_new = abs(freq - st->filter_freq_avail[i][0]); > if (diff_new < diff_old) { > diff_old = diff_new; > idx = i; > @@ -826,6 +809,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, > st->mode &= ~AD7192_MODE_RATE_MASK; > st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div); > ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); > + ad7192_update_filter_freq_avail(st); > break; > case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > ret = ad7192_set_3db_filter_freq(st, val, val2 / 1000); > @@ -846,6 +830,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, > break; > } > mutex_unlock(&st->lock); > + ad7192_update_filter_freq_avail(st); Does this need to go inside of the mutex guard to avoid potential race conditions? > break; > default: > ret = -EINVAL; > @@ -888,6 +873,12 @@ static int ad7192_read_avail(struct iio_dev *indio_dev, > /* Values are stored in a 2D matrix */ > *length = ARRAY_SIZE(st->scale_avail) * 2; > > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > + *vals = (int *)st->filter_freq_avail; > + *type = IIO_VAL_FRACTIONAL; > + *length = ARRAY_SIZE(st->filter_freq_avail) * 2; > + > return IIO_AVAIL_LIST; > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > *vals = (int *)st->oversampling_ratio_avail; > @@ -956,7 +947,9 @@ static const struct iio_info ad7195_info = { > BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \ > (_mask_all), \ > .info_mask_shared_by_type_available = (_mask_type_av), \ > - .info_mask_shared_by_all_available = (_mask_all_av), \ > + .info_mask_shared_by_all_available = \ > + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \ > + (_mask_all_av), \ > .ext_info = (_ext_info), \ > .scan_index = (_si), \ > .scan_type = { \ > -- > 2.34.1 >
On 13.04.2024 22:10, David Lechner wrote: > On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman > <alisadariana@gmail.com> wrote: >> >> AINCOM should actually be a supply. If present and it has a non-zero >> voltage, the pseudo-differential channels are configured as single-ended >> with an offset. Otherwise, they are configured as differential channels >> between AINx and AINCOM pins. >> >> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> >> --- >> drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 49 insertions(+), 4 deletions(-) ... >> @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, >> /* Kelvin to Celsius */ > > Not related to this patch, but I'm not a fan of the way the > temperature case writes over *val (maybe clean that up using a switch > statement instead in another patch while we are working on this?). > Adding the else if to this makes it even harder to follow. > >> if (chan->type == IIO_TEMP) >> *val -= 273 * ad7192_get_temp_scale(unipolar); >> + else if (st->aincom_mv && chan->channel2 == -1) > > I think the logic should be !chan->differential instead of > chan->channel2 = -1 (more explanation on this below). > >> + *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000, >> + st->scale_avail[gain][1]); >> return IIO_VAL_INT; Hi David, I am very grateful for your suggestions! case IIO_CHAN_INFO_OFFSET: if (!unipolar) *val = -(1 << (chan->scan_type.realbits - 1)); else *val = 0; switch(chan->type) { case IIO_VOLTAGE: if (st->aincom_mv && !chan->differential) *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000, st->scale_avail[gain][1]); return IIO_VAL_INT; /* Kelvin to Celsius */ case IIO_TEMP: *val -= 273 * ad7192_get_temp_scale(unipolar); return IIO_VAL_INT; default: return -EINVAL; } I added a switch because it looks neater indeed. But I would keep the if else for the unipolar in order not to have duplicate code. Is this alright?
On 13.04.2024 23:05, David Lechner wrote: > On Sat, Apr 13, 2024 at 10:13 AM Alisa-Dariana Roman > <alisadariana@gmail.com> 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@analog.com> >> --- >> drivers/iio/adc/Kconfig | 11 ++- >> drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++--- >> 2 files changed, 138 insertions(+), 13 deletions(-) ... > >> + 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. I wrote the channels like this to match the other chips: static const struct iio_chan_spec ad7193_channels[] = { AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M), AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M), AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), AD7193_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), AD7193_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), AD7193_CHANNEL(6, 1, AD7193_CH_AIN1), AD7193_CHANNEL(7, 2, AD7193_CH_AIN2), AD7193_CHANNEL(8, 3, AD7193_CH_AIN3), AD7193_CHANNEL(9, 4, AD7193_CH_AIN4), AD7193_CHANNEL(10, 5, AD7193_CH_AIN5), AD7193_CHANNEL(11, 6, AD7193_CH_AIN6), AD7193_CHANNEL(12, 7, AD7193_CH_AIN7), AD7193_CHANNEL(13, 8, AD7193_CH_AIN8), IIO_CHAN_SOFT_TIMESTAMP(14), }; Kind regards, Alisa-Dariana Roman
On 4/14/24 8:58 AM, Alisa-Dariana Roman wrote: > On 13.04.2024 22:10, David Lechner wrote: >> On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman >> <alisadariana@gmail.com> wrote: >>> >>> AINCOM should actually be a supply. If present and it has a non-zero >>> voltage, the pseudo-differential channels are configured as single-ended >>> with an offset. Otherwise, they are configured as differential channels >>> between AINx and AINCOM pins. >>> >>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> >>> --- >>> drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 49 insertions(+), 4 deletions(-) > > ... > >>> @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, >>> /* Kelvin to Celsius */ >> >> Not related to this patch, but I'm not a fan of the way the >> temperature case writes over *val (maybe clean that up using a switch >> statement instead in another patch while we are working on this?). >> Adding the else if to this makes it even harder to follow. >> >>> if (chan->type == IIO_TEMP) >>> *val -= 273 * ad7192_get_temp_scale(unipolar); >>> + else if (st->aincom_mv && chan->channel2 == -1) >> >> I think the logic should be !chan->differential instead of >> chan->channel2 = -1 (more explanation on this below). >> >>> + *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000, >>> + st->scale_avail[gain][1]); >>> return IIO_VAL_INT; > > Hi David, > > I am very grateful for your suggestions! > > case IIO_CHAN_INFO_OFFSET: > if (!unipolar) > *val = -(1 << (chan->scan_type.realbits - 1)); > else > *val = 0; > switch(chan->type) { > case IIO_VOLTAGE: > if (st->aincom_mv && !chan->differential) > *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000, > st->scale_avail[gain][1]); > return IIO_VAL_INT; > /* Kelvin to Celsius */ > case IIO_TEMP: > *val -= 273 * ad7192_get_temp_scale(unipolar); > return IIO_VAL_INT; > default: > return -EINVAL; > } > > I added a switch because it looks neater indeed. But I would keep the if else for the unipolar in order not to have duplicate code. Is this alright? > I didn't notice before that the temperature channel could also be unipolor or bipolar, so yes this seems fine.
On 4/14/24 3:14 PM, Alisa-Dariana Roman wrote: > On 13.04.2024 23:05, David Lechner wrote: >> On Sat, Apr 13, 2024 at 10:13 AM Alisa-Dariana Roman >> <alisadariana@gmail.com> 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@analog.com> >>> --- >>> drivers/iio/adc/Kconfig | 11 ++- >>> drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++--- >>> 2 files changed, 138 insertions(+), 13 deletions(-) > > ... > >> >>> + 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. > > I wrote the channels like this to match the other chips: > > static const struct iio_chan_spec ad7193_channels[] = { > AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M), > AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M), > AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > AD7193_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), > AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), > AD7193_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), > AD7193_CHANNEL(6, 1, AD7193_CH_AIN1), > AD7193_CHANNEL(7, 2, AD7193_CH_AIN2), > AD7193_CHANNEL(8, 3, AD7193_CH_AIN3), > AD7193_CHANNEL(9, 4, AD7193_CH_AIN4), > AD7193_CHANNEL(10, 5, AD7193_CH_AIN5), > AD7193_CHANNEL(11, 6, AD7193_CH_AIN6), > AD7193_CHANNEL(12, 7, AD7193_CH_AIN7), > AD7193_CHANNEL(13, 8, AD7193_CH_AIN8), > IIO_CHAN_SOFT_TIMESTAMP(14), > }; > > Kind regards, > Alisa-Dariana Roman > Consistency is good too. ;-)
On Sat, Apr 13, 2024 at 11:05 PM David Lechner <dlechner@baylibre.com> wrote: > On Sat, Apr 13, 2024 at 10:13 AM Alisa-Dariana Roman > <alisadariana@gmail.com> wrote: ... > > +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. Quite unlikely we have the same chip to be supported by somewhere else, so I would just name it simpler, i.e. ad7194_parse_channels()