Message-ID: <CAMknhBGmM7yt1JR1tW4SS5RLGpN9PtnMrf0WvZ-bhU-gSv3YUQ@mail.gmail.com>
Date: Mon, 25 Mar 2024 09:08:27 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Julien Stephan <jstephan@...libre.com>, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>, 
	Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v5 4/7] iio: adc: ad7380: add support for
 pseudo-differential parts

On Sun, Mar 24, 2024 at 8:01 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 19 Mar 2024 11:11:25 +0100
> Julien Stephan <jstephan@...libre.com> wrote:
>
> > From: David Lechner <dlechner@...libre.com>
> >
> > Add support for AD7383, AD7384 pseudo-differential compatible parts.
> > Pseudo differential parts require common mode voltage supplies so add
> > the support for them and add the support of IIO_CHAN_INFO_OFFSET to
> > retrieve the offset
> >
> > Signed-off-by: David Lechner <dlechner@...libre.com>
> > Signed-off-by: Julien Stephan <jstephan@...libre.com>
>
> Hi Julien,
>
> A few aditional comments inline.  The one about
> optional regulators may be something others disagree with.
> Mark, perhaps you have time to comment.
> Is this usage of devm_regulator_get_optional() to check a real regulator
> is supplied (as we are going to get the voltage) sensible?  Feels wrong
> given the regulator is the exact opposite of optional.
>
> Jonathan
>
> >  struct ad7380_state {
> >       const struct ad7380_chip_info *chip_info;
> >       struct spi_device *spi;
> >       struct regmap *regmap;
> >       unsigned int vref_mv;
> > +     unsigned int vcm_mv[2];
> >       /*
> >        * DMA (thus cache coherency maintenance) requires the
> >        * transfer buffers to live in their own cache lines.
> > @@ -304,6 +333,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> >               *val2 = chan->scan_type.realbits;
> >
> >               return IIO_VAL_FRACTIONAL_LOG2;
> > +     case IIO_CHAN_INFO_OFFSET:
> > +             *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
> > +                     / st->vref_mv;
>
> So this maths seems to be right to me, but it took me a while to figure it out.
> Perhaps a comment would help along the lines of this is transforming
>
>         (raw * scale) + vcm_mv
> to
>         (raw + vcm_mv / scale) * scale
> as IIO ABI says offset is applied before scale.
>
> > +
> > +             return IIO_VAL_INT;
> >       }
> >
> >       return -EINVAL;
> > @@ -350,7 +384,7 @@ static int ad7380_probe(struct spi_device *spi)
> >       struct iio_dev *indio_dev;
> >       struct ad7380_state *st;
> >       struct regulator *vref;
> > -     int ret;
> > +     int ret, i;
> >
> >       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >       if (!indio_dev)
> > @@ -394,6 +428,40 @@ static int ad7380_probe(struct spi_device *spi)
> >               st->vref_mv = AD7380_INTERNAL_REF_MV;
> >       }
> >
> > +     if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
> > +             return dev_err_probe(&spi->dev, -EINVAL,
> > +                                  "invalid number of VCM supplies\n");
> > +
> > +     /*
> > +      * pseudo-differential chips have common mode supplies for the negative
> > +      * input pin.
> > +      */
> > +     for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> > +             struct regulator *vcm;
> > +
> > +             vcm = devm_regulator_get_optional(&spi->dev,
>
> Why optional?
>
> > +                                               st->chip_info->vcm_supplies[i]);
> > +             if (IS_ERR(vcm))
>
> This will fail if it's not there, so I'm guessing you are using this to avoid
> getting to the regulator_get_voltage?  If it's not present I'd rely on that
> failing rather than the confusing handling here.
>
> When the read of voltage wasn't in probe this would have resulted in a problem
> much later than initial setup, now it is, we are just pushing it down a few lines.
>
> Arguably we could have a devm_regulator_get_not_dummy()
> that had same implementation to as get_optional() but whilst it's called that
> I think it's confusing to use like this.

Despite the misleading naming, I guess I am used to
devm_regulator_get_optional() by now having used it enough times.
Since it fails either way though, technically both ways seem fine so I
can't really argue for one over the other.

But given that this is a common pattern in many IIO drivers, maybe we
make a devm_regulator_get_enable_get_voltage()? This would return the
voltage on success or an error code. (If the regulator subsystem
doesn't want this maybe we could have
devm_iio_regulator_get_enable_get_voltage()).

If the dev_err_probe() calls were included in
devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
here and in many other drivers to get the regulator, enable it, add
the reset action and get the voltage could be reduced to 3 lines.

>
> > +                     return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> > +                                          "Failed to get %s regulator\n",
> > +                                          st->chip_info->vcm_supplies[i]);
> > +
> > +             ret = regulator_enable(vcm);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = devm_add_action_or_reset(&spi->dev,
> > +                                            ad7380_regulator_disable, vcm);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regulator_get_voltage(vcm);
>
> I'd let this fail if we have a dummy regulator.
>
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             st->vcm_mv[i] = ret / 1000;
> > +     }
> > +

Powered by blists - more mailing lists