On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman
<alisadariana@xxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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;
> +}
> +
