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; > > + } > > +