Message-ID: <ZnwU3MovTWfrovrE@debian-BULLSEYE-live-builder-AMD64>
Date: Wed, 26 Jun 2024 10:17:16 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
	lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
	robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	conor+dt@...nel.org, nuno.sa@...log.com, dlechner@...libre.com,
	corbet@....net, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-spi@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

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@...log.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
> > 
...
> ...
> 
> > +
> > +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> > +{
> > +	struct spi_transfer t = {
> > +		.tx_buf = st->tx_buf,
> > +		.rx_buf = st->rx_buf,
> > +		.len = 2,
> > +	};
> > +	int ret;
> > +
> > +	st->tx_buf[0] = AD4000_READ_COMMAND;
> > +	ret = spi_sync_transfer(st->spi, &t, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = st->tx_buf[1];
> 
> I'm puzzled... tx_buf?
> 
Oh my, I must have messed up when changing to array buffers.
Looks like v6 will be coming :)

> > +	return ret;
> > +}
> > +
> > +/*
> > + * This executes a data sample transfer for when the device connections are
> > + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> > + * absent or set to "high". In this connection mode, the ADC SDI pin is
> > + * connected to MOSI or to VIO and ADC CNV pin is connected either to a SPI
> > + * controller CS or to a GPIO.
> > + * AD4000 series of devices initiate conversions on the rising edge of CNV
> > pin.
> > + *
> > + * If the CNV pin is connected to an SPI controller CS line (which is by
> > default
> > + * active low), the ADC readings would have a latency (delay) of one read.
> > + * Moreover, since we also do ADC sampling for filling the buffer on
> > triggered
> > + * buffer mode, the timestamps of buffer readings would be disarranged.
> > + * To prevent the read latency and reduce the time discrepancy between the
> > + * sample read request and the time of actual sampling by the ADC, do a
> > + * preparatory transfer to pulse the CS/CNV line.
> > + */
> > +static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
> > +					     const struct iio_chan_spec
> > *chan)
> > +{
> > +	unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
> > +						     : AD4000_TCONV_NS;
> > +	struct spi_transfer *xfers = st->xfers;
> > +
> > +	xfers[0].cs_change = 1;
> > +	xfers[0].cs_change_delay.value = cnv_pulse_time;
> > +	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> > +
> > +	xfers[1].rx_buf = &st->scan.data;
> > +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> > +	xfers[1].delay.value = AD4000_TQUIET2_NS;
> > +	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> > +
> > +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> > +
> > +	return devm_spi_optimize_message(st->spi, &st->msg);
> > +}
> > +
> > +/*
> > + * This executes a data sample transfer for when the device connections are
> > + * in "4-wire" mode, selected when the adi,sdi-pin device tree property is
> > + * set to "cs". In this connection mode, the controller CS pin is connected
> > to
> > + * ADC SDI pin and a GPIO is connected to ADC CNV pin.
> > + * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
> > + */
> > +static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
> > +					     const struct iio_chan_spec
> > *chan)
> > +{
> > +	unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
> > +						      : AD4000_TCONV_NS;
> > +	struct spi_transfer *xfers = st->xfers;
> > +
> > +	/*
> > +	 * Dummy transfer to cause enough delay between CNV going high and
> > SDI
> > +	 * going low.
> > +	 */
> > +	xfers[0].cs_off = 1;
> > +	xfers[0].delay.value = cnv_to_sdi_time;
> > +	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > +
> > +	xfers[1].rx_buf = &st->scan.data;
> > +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> > +
> > +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> > +
> > +	return devm_spi_optimize_message(st->spi, &st->msg);
> > +}
> 
> 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?

> 
> > +
> > +static int ad4000_convert_and_acquire(struct ad4000_state *st)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * In 4-wire mode, the CNV line is held high for the entire
> > conversion
> > +	 * and acquisition process. In other modes, the CNV GPIO is optional
> > +	 * and, if provided, replaces controller CS. If CNV GPIO is not
> > defined
> > +	 * gpiod_set_value_cansleep() has no effect.
> > +	 */
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > +	ret = spi_sync(st->spi, &st->msg);
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> > +				    const struct iio_chan_spec *chan, int
> > *val)
> > +{
> > +	struct ad4000_state *st = iio_priv(indio_dev);
> > +	u32 sample;
> > +	int ret;
> > +
> > +	ret = ad4000_convert_and_acquire(st);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (chan->scan_type.storagebits > 16)
> > +		sample = be32_to_cpu(st->scan.data.sample_buf32);
> 
> Just a minor note regarding your comment in the cover. FWIW, I prefer you leave
> it like this. Yes, with 24 bits you save some space but then you need an
> unaligned access... To me that space savings are really a micro optimization so
> I would definitely go with the simpler form.
> 
I'm no expert on this. Will go with what maintainers say.

> > +	else
> > +		sample = be16_to_cpu(st->scan.data.sample_buf16);
> > +
> > +	sample >>= chan->scan_type.shift;
> > +
> > +	if (chan->scan_type.sign == 's')
> > +		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
...
> > +static int ad4000_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int val, int
> > val2,
> > +			    long mask)
> > +{
> > +	struct ad4000_state *st = iio_priv(indio_dev);
> > +	unsigned int reg_val;
> > +	bool span_comp_en;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> 
> 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.

> 
> > +		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?

> 
> > +
> > +	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.

> 
> > +		break;
> > +	case AD4000_SDI_VIO:
> > +		indio_dev->info = &ad4000_info;
> > +		indio_dev->channels = &chip->chan_spec;
> > +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev-
> > >channels);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	case AD4000_SDI_CS:
> > +		indio_dev->info = &ad4000_info;
> > +		indio_dev->channels = &chip->chan_spec;
> > +		ret = ad4000_prepare_4wire_mode_message(st, indio_dev-
> > >channels);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	default:
> > +		return dev_err_probe(dev, -EINVAL, "Unrecognized connection
> > mode\n");
> > +	}
> > +
> > +	indio_dev->name = chip->dev_name;
> > +	indio_dev->num_channels = 1;
> > +
> > +	devm_mutex_init(dev, &st->lock);
> > +
> > +	st->gain_milli = 1000;
> > +	if (chip->has_hardware_gain) {
> > +		if (device_property_present(dev, "adi,gain-milli")) {
> > +			ret = device_property_read_u16(dev, "adi,gain-milli",
> > +						       &st->gain_milli);
> > +			if (ret)
> > +				return dev_err_probe(dev, ret,
> > +						     "Failed to read gain
> > property\n");
> > +		}
> > 
> 
> the above looks odd. Why not?
> 
> ret = device_property_read_u16(dev, "adi,gain-milli", &st->gain_milli);
> if (!ret) {
> 	...
> }

I wanted to be more protective in case anything strange comes from dt.

> 
> Note that you're also allowing any value for gain_milli when we just allow some
> of them (according to the bindings). Hence you should make sure we get supported
> values from FW.

Yes, but anything different from what is specified in the binding should make
dtbs_check fail, no?
can use device_property_match_property_string() so we assure only supported
gain-milli values in the driver as well.

> 
> - Nuno Sá

Powered by blists - more mailing lists