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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 13 Apr 2024 14:10:08 -0500
From: David Lechner <dlechner@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>
Cc: michael.hennerich@...log.com, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	alexandru.tachici@...log.com, lars@...afoo.de, jic23@...nel.org, 
	robh@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	lgirdwood@...il.com, broonie@...nel.org, andy@...nel.org, nuno.sa@...log.com, 
	marcelo.schmitt@...log.com, bigunclemax@...il.com, okan.sahin@...log.com, 
	fr0st61te@...il.com, alisa.roman@...log.com, marcus.folkesson@...il.com, 
	schnelle@...ux.ibm.com, liambeguin@...il.com
Subject: Re: [PATCH v5 3/5] iio: adc: ad7192: Add aincom supply

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