| Message ID | 20240413151152.165682-4-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| 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, 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 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 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.
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[] = { 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; + 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;
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(-)