Re: [PATCH v5 3/5] iio: adc: ad7192: Add aincom supply

[David Lechner]

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;
> +}
> +