Message-ID: <CAMknhBFZKKim4JyXLpHY=EoyW4ZPp03aHte5xy0ZkDnW16sfeA@mail.gmail.com>
Date: Wed, 3 Apr 2024 11:37:48 -0500
From: David Lechner <dlechner@...libre.com>
To: "Ceclan, Dumitru" <mitrutzceclan@...il.com>
Cc: dumitru.ceclan@...log.com, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Jonathan Cameron <jic23@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] iio: adc: ad7173: Add support for AD411x devices

On Wed, Apr 3, 2024 at 4:53 AM Ceclan, Dumitru <mitrutzceclan@...ilcom> wrote:
>
>
> On 01/04/2024 22:45, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <devnull+dumitru.ceclan.analog.com@...nel.org> wrote:
> >>
> >> From: Dumitru Ceclan <dumitru.ceclan@...log.com>
> >>
>
> ...
>
> >>  #define AD7175_2_ID                    0x0cd0
> >>  #define AD7172_4_ID                    0x2050
> >>  #define AD7173_ID                      0x30d0
> >> +#define AD4111_ID                      0x30d0
> >> +#define AD4112_ID                      0x30d0
> >> +#define AD4114_ID                      0x30d0
> >
> > It might make it a bit more obvious that not all chips have a unique
> > ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather
> > than introducing multiple macros with the same value.
> >
> > Or leave it as AD7173_ID to keep it short and add a comment where it
> > is used with 411x chips in ad7173_device_info[].
> >
>
> Sure
>
> >> +#define AD4116_ID                      0x34d0
> >> +#define AD4115_ID                      0x38d0
> >>  #define AD7175_8_ID                    0x3cd0
> >>  #define AD7177_ID                      0x4fd0
> >>  #define AD7173_ID_MASK                 GENMASK(15, 4)
>
> ...
>
> >>  struct ad7173_device_info {
> >>         const unsigned int *sinc5_data_rates;
> >>         unsigned int num_sinc5_data_rates;
> >>         unsigned int odr_start_value;
> >> +       unsigned int num_inputs_with_divider;
> >>         unsigned int num_channels;
> >>         unsigned int num_configs;
> >>         unsigned int num_inputs;
> >
> > Probably a good idea to change num_inputs to num_voltage_inputs so it
> > isn't confused with the total number of inputs.
> >
> > Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider.
> >
> > Also could use a comment to make it clear if num_voltage_inputs
> > includes num_voltage_inputs_with_divider or not. And that it doesn't
> > include VINCOM.
> >
>
> Alright for these 3 statements above.
>
> > Probably also need some flag here to differentiate ADCINxx voltage
> > inputs on AD4116.
> >
>
> That is the purpose of num_inputs_with_divider. Mangled some changes
> when splitting into individual patches. Will include in V2.
> "
> if (ain[1] == AD411X_VCOM_INPUT &&
>                     ain[0] >= st->info->num_inputs_with_divider)
>                         return dev_err_probe(dev, -EINVAL,
>                 "VCOM must be paired with inputs having divider.\n");
> "
>
> ...
>
> >>
> >> +static unsigned int ad4111_current_channel_config[] = {
> >> +       [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
> >> +       [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
> >> +       [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
> >> +       [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
> >> +};
> >
> > As mentioned in the DT bindings review, it would make more sense to
> > just use the datasheet numbers for the current input channels in the
> > diff-channels DT property, then we don't need this lookup table.
> >
> Yet, the datasheet does not specify the numbers, just a single bitfield
> for each pair. It is too much of a churn to need to decode that bitfield
> into individual values when the user just wants to select a single pair.
>
> ...
>
> >> +               case IIO_CURRENT:
> >> +                       *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> >> +                       *val /= AD4111_SHUNT_RESISTOR_OHM;
> >> +                       *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> >
> > Static analysis tools like to complain about using bool as int.
> > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway.
> >
> Maybe it does not apply here, but i followed this advice:
>
> Andy Shevchenko V1 of AD7173 (named initially ad717x)
> "
> > +     return (bool)(value & mask);
>
> This is weird. You have int which you get from bool, wouldn't be better
> to use
> !!(...) as other GPIO drivers do?

As long as the build bots don't complain, there isn't a reason to
change it. It is just a matter of personal preference at that point.

I got a sparse warning for something like this recently [1], but maybe
that case was just because it was inside of a FIELD_PREP() using it as
bit logic instead of addition and we won't get any warnings here.

[1]: https://lore.kernel.org/linux-iio/20240129195611.701611-3-dlechner@baylibre.com/

>
> "
>
>
> >> +               case IIO_CURRENT:
> >>                         *val = -BIT(chan->scan_type.realbits - 1);
> >
> > Expecting a special case here, at least when ADCIN15 is configured for
> > pseudo-differential inputs.
> >
>
> And what configuration would that be?
> The only configurable part is the BI_UNIPOLARx bit in the channel
> register, which is addressed here.
>
> There seems to be a confusion similar to what we had with single-ended
> channels. The ADC is differential. Pseudo-differential in this datasheet
> just means that you wired a fixed voltage(higher than 0) to the negative
> analog input.
>
>  Which you can also do on the other inputs with a divider.
>

As discussed elsewhere, you can disregard this suggestion.

> ...
>
> >> -               chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> >> +               if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
> >> +                       chan->type = IIO_CURRENT;
> >> +                       chan->channel = ain[0];
> >> +                       chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
> >> +               } else {
> >> +                       chan->channel = ain[0];
> >> +                       chan->channel2 = ain[1];
> >> +                       chan->differential = true;
> >
> > Expecting chan->differential = false when ADCIN15 is configured for
> > pseudo-differential inputs.
> >
> > Also, perhaps missed in previous reviews, I would expect
> > chan->differential = false when channels are used as single-ended.
> >
> Why?
> Also, how would one detect if you are using single-ended channels?
>
> The ADC is still differential. Single ended is represented as connecting
> AVSS(or another fixed voltage) and only letting the AIN+ input to fluctuate.
>
> In the IIO framework the only difference this makes is in the naming of
> the channel:
>  voltage0-voltage1 vs just voltage0
>
> All channels are differential. Pseudo differential: still differential.

In the discussions on the AD7380 patch series, we came to the
conclusion that pseduo-differential is technically not differential in
the context of the .differential flag in IIO.

But as mentioned in my follow up, for this driver it is going to make
things far simpler if we just ignore that and treat
pseudo-differential the same as fully differential.

Powered by blists - more mailing lists