On Sun, Mar 24, 2024 at 8:01 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 19 Mar 2024 11:11:25 +0100
> Julien Stephan <jstephan@xxxxxxxxxxxx> wrote:
>
> > From: David Lechner <dlechner@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
>
> 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;
> > + }
> > +
