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

["Nuno Sá" <noname.nuno@gmail.com>]

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@analog.com>
> > > ---
> > >  MAINTAINERS              |   1 +
> > >  drivers/iio/adc/Kconfig  |  12 +
> > >  drivers/iio/adc/Makefile |   1 +
> > >  drivers/iio/adc/ad4000.c | 711 +++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 725 insertions(+)
> > >  create mode 100644 drivers/iio/adc/ad4000.c
> > > 
> 

...

> > 
> > nit: you could reduce the scope of the above prepare functions...
> 
> Not sure I got what you mean with this comment Nuno.
> Would it be preferable to prepare the 3-wire/4-wire transfers within the
> switch
> cases in probe?
> 

These functions are only called from probe() right? So they could closer to the
probe function. Anyways a nitpick comment :)

...

> 
> > 
> > 
> > iio_device_claim_direct_scoped()?
> 
> I had iio_device_claim_direct_scoped() in v4 but was asked to use a local
> lock to protect the read modify write cycle here.
> > 
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		mutex_lock(&st->lock);
> > 
> > guard()?
> 
> This guard() stuff is somewhat new to me.
> Will check out if can use it here.

should be doable... 

> 
> > 
> > > +		ret = ad4000_read_reg(st, &reg_val);
> > > +		if (ret < 0)
> > > +			goto err_unlock;
> > > +
> > > +		span_comp_en = val2 == st->scale_tbl[1][1];
> > > +		reg_val &= ~AD4000_CFG_SPAN_COMP;
> > > +		reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP,
> > > span_comp_en);
> > > +
> > > +		ret = ad4000_write_reg(st, reg_val);
> > > +		if (ret < 0)
> > > +			goto err_unlock;
> > > +
> > > +		st->span_comp = span_comp_en;
> > > +err_unlock:
> > > +		iio_device_release_direct_mode(indio_dev);
> > > +		mutex_unlock(&st->lock);
> > > +		return ret;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> ...
> > > +
> > > +static int ad4000_probe(struct spi_device *spi)
> > > +{
> > > +	const struct ad4000_chip_info *chip;
> > > +	struct device *dev = &spi->dev;
> > > +	struct iio_dev *indio_dev;
> > > +	struct ad4000_state *st;
> > > +	int ret;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	chip = spi_get_device_match_data(spi);
> > > +	if (!chip)
> > > +		return -EINVAL;
> > > +
> > > +	st = iio_priv(indio_dev);
> > > +	st->spi = spi;
> > > +
> > > +	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.

> 
> > 
> > > +
> > > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "Failed to get ref regulator
> > > reference\n");
> > > +	st->vref_mv = ret / 1000;
> > > +
> > > +	st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv",
> > > GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(st->cnv_gpio))
> > > +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> > > +				     "Failed to get CNV GPIO");
> > > +
> > > +	ret = device_property_match_property_string(dev, "adi,sdi-pin",
> > > +						    ad4000_sdi_pin,
> > > +						   
> > > ARRAY_SIZE(ad4000_sdi_pin));
> > > +	if (ret < 0 && ret != -EINVAL)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "getting adi,sdi-pin property
> > > failed\n");
> > > +
> > > +	/* Default to usual SPI connections if pin properties are not
> > > present
> > > */
> > > +	st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
> > > +	switch (st->sdi_pin) {
> > > +	case AD4000_SDI_MOSI:
> > > +		indio_dev->info = &ad4000_reg_access_info;
> > > +		indio_dev->channels = &chip->reg_access_chan_spec;
> > > +
> > > +		/*
> > > +		 * 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 :)

- Nuno Sá
>