Received-SPF: pass (google.com: domain of linux-kernel+bounces-117205-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240319-adding-new-ad738x-driver-v5-0-ce7df004ceb3@baylibre.com>
 <20240319-adding-new-ad738x-driver-v5-4-ce7df004ceb3@baylibre.com> <20240324130135.35f4b0eb@jic23-huawei>
In-Reply-To: <20240324130135.35f4b0eb@jic23-huawei>
From: David Lechner <dlechner@baylibre.com>
Date: Mon, 25 Mar 2024 09:08:27 -0500
Message-ID: <CAMknhBGmM7yt1JR1tW4SS5RLGpN9PtnMrf0WvZ-bhU-gSv3YUQ@mail.gmail.com>
Subject: Re: [PATCH v5 4/7] iio: adc: ad7380: add support for
 pseudo-differential parts
To: Jonathan Cameron <jic23@kernel.org>
Cc: Julien Stephan <jstephan@baylibre.com>, Lars-Peter Clausen <lars@metafoo.de>, 
	Michael Hennerich <Michael.Hennerich@analog.com>, =?UTF-8?B?TnVubyBTw6E=?= <nuno.sa@analog.com>, 
	Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	kernel test robot <lkp@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 24, 2024 at 8:01=E2=80=AFAM Jonathan Cameron <jic23@kernel.org>=
 wrote:
>
> On Tue, 19 Mar 2024 11:11:25 +0100
> Julien Stephan <jstephan@baylibre.com> wrote:
>
> > From: David Lechner <dlechner@baylibre.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@baylibre.com>
> > Signed-off-by: Julien Stephan <jstephan@baylibre.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_d=
ev,
> >               *val2 =3D chan->scan_type.realbits;
> >
> >               return IIO_VAL_FRACTIONAL_LOG2;
> > +     case IIO_CHAN_INFO_OFFSET:
> > +             *val =3D st->vcm_mv[chan->channel] * (1 << chan->scan_typ=
e.realbits)
> > +                     / st->vref_mv;
>
> So this maths seems to be right to me, but it took me a while to figure i=
t 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 =3D 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 =3D 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 ne=
gative
> > +      * input pin.
> > +      */
> > +     for (i =3D 0; i < st->chip_info->num_vcm_supplies; i++) {
> > +             struct regulator *vcm;
> > +
> > +             vcm =3D devm_regulator_get_optional(&spi->dev,
>
> Why optional?
>
> > +                                               st->chip_info->vcm_supp=
lies[i]);
> > +             if (IS_ERR(vcm))
>
> This will fail if it's not there, so I'm guessing you are using this to a=
void
> getting to the regulator_get_voltage?  If it's not present I'd rely on th=
at
> failing rather than the confusing handling here.
>
> When the read of voltage wasn't in probe this would have resulted in a pr=
oblem
> much later than initial setup, now it is, we are just pushing it down a f=
ew 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 =3D regulator_enable(vcm);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret =3D devm_add_action_or_reset(&spi->dev,
> > +                                            ad7380_regulator_disable, =
vcm);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret =3D regulator_get_voltage(vcm);
>
> I'd let this fail if we have a dummy regulator.
>
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             st->vcm_mv[i] =3D ret / 1000;
> > +     }
> > +