Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170

[Marcelo Schmitt]

Hi Jonathan,

Thank you for reviewing this set.
Clarifying some bits inline. Will apply all other suggested changes.

Thanks,
Marcelo

On 04/12, Jonathan Cameron wrote:
> On Wed, 9 Apr 2025 09:24:35 -0300
> Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote:
> 
> > From: Ana-Maria Cusco <ana-maria.cusco@xxxxxxxxxx>
> > 
> > Add support for the AD4170 ADC with the following features:
> > - Single-shot read.
> > - Analog front end PGA configuration.
> > - Digital filter and sampling frequency configuration.
> > - Calibration gain and offset configuration.
> > - Differential and pseudo-differential input configuration.
> > 
> > Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@xxxxxxxxxx>
> > Co-developed-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> Hi.
> 
> Clearly this is a massive driver, even with it broken up like this.
> So it might take a few cycles to review enough that we don't find
> new things :(

No worries. Yeah, I tried my best to make it concise but this is still not small
piece of code.

> 
> I can't think of a good way to split it up further though
> 
> Jonathan
> 
> > diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> > new file mode 100644
> > index 000000000000..0d24286ac2ab
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4170.c
> 
> 
...
> > +static int ad4170_regulator_setup(struct ad4170_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	int ret;
> > +
> > +	/* Required regulators */
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to get AVDD voltage.\n");
> > +
> > +	st->vrefs_uv[AD4170_AVDD_SUP] = ret;
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "iovdd");
> 
> If no channel uses this reference is it not optional?  Maybe not worth the
> complexity of handling that.  We have sometime bothered to do so in the past
> by first figuring out which references are in use, then trying to get the
> appropriate regulators with small changes for cases like this where
> it needs to be enabled but we might not need the voltage.

We can set the channel multiplexer to use IOVDD reference as diff chan negative
input. Similar thing can be done for the other reference supplies. I think
the examples in dt-binding don't use IOVDD but they could. Since the driver is
supporting other regulators, maybe support IOVDD too?

> 
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to get IOVDD voltage.\n");
> > +
> > +	st->vrefs_uv[AD4170_IOVDD_SUP] = ret;
> > +
> > +	/* Optional regulators */
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "avss");
> > +	if (ret < 0 && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get AVSS voltage.\n");
> > +