Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

[Marcelo Schmitt]

On 06/26, Nuno Sá wrote:
> On Wed, 2024-06-26 at 10:17 -0300, Marcelo Schmitt wrote:
> > On 06/26, Nuno Sá wrote:
> > > On Tue, 2024-06-25 at 18:55 -0300, Marcelo Schmitt wrote:
> > > > Add support for AD4000 series of low noise, low power, high speed,
> > > > successive approximation register (SAR) ADCs.
> > > > 
> > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> > > > ---
...
> > > > +	ret = devm_regulator_get_enable(dev, "vdd");
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "Failed to enable VDD
> > > > supply\n");
> > > > +
> > > > +	ret = devm_regulator_get_enable(dev, "vio");
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "Failed to enable VIO
> > > > supply\n");
> > > 
> > > devm_regulator_bulk_get_enable()? Do we have any ordering constrains?
> > 
> > No ordering constraints, but vdd and vio are optional while ref is required
> > and
> > we need to get the voltage of ref.
> > devm_regulator_bulk_get_enable_read_voltage()? and discard vdd and vio
> > voltages?
> 
> Hmmm, vdd and vio do not look like optional to me :). Anyways I meant
> devm_regulator_bulk_get_enable() only for vdd and vio and still treat ref
> separately.
> 

I've mistaken these supplies with supplies for a different device.
Yes, vdd and vio are required and devm_regulator_bulk_get_enable() is useful
to init them.

> > 
> > > 
> > > > +
...
> > > > +		/*
> > > > +		 * In "3-wire mode", the ADC SDI line must be kept high
> > > > when
> > > > +		 * data is not being clocked out of the controller.
> > > > +		 * Request the SPI controller to make MOSI idle high.
> > > > +		 */
> > > > +		spi->mode |= SPI_MOSI_IDLE_HIGH;
> > > > +		ret = spi_setup(spi);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +
> > > > +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev-
> > > > > channels);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		ret = ad4000_config(st);
> > > > +		if (ret < 0)
> > > > +			dev_warn(dev, "Failed to config device\n");
> > > > +
> > > 
> > > Should this be a warning? Very suspicious :)
> > 
> > This devices have some many possible wiring configurations.
> > I didn't want to fail just because reg access fail.
> > Maybe ADC SDI was wired to VIO but dt don't have adi,sdi-pin = "high".
> > Reg access will fail but sample read should work.
> 
> Well, to me that really is a configuration failure and we should treat it as
> such. If we are in the so called "reg_access_info" which I read as "we can
> access registers", failing to do so should be treated as an error. 
> 
> So, setting scale would also fail and we then have a broken interface :)

Drat, that's right. 
Okay, will make probe fail if config fails.

Thanks,
Marcelo

> 
> - Nuno Sá
> >