Message-ID: <e92871489d416e4f8a350fd24fc5ed0012b3cf2b.camel@gmail.com>
Date: Wed, 05 Jun 2024 15:03:12 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: 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, marcelo.schmitt1@...il.com
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 6/6] iio: adc: Add support for AD4000

On Wed, 2024-06-05 at 08:14 -0300, Marcelo Schmitt wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---

...

> +
> +static const struct ad4000_chip_info ad4000_chips[] = {
> +	[ID_AD4000] = {
> +		.dev_name = "ad4000",
> +		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> +	},
> +	[ID_AD4001] = {
> +		.dev_name = "ad4001",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> +	},
> +	[ID_AD4002] = {
> +		.dev_name = "ad4002",
> +		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> +	},
> +	[ID_AD4003] = {
> +		.dev_name = "ad4003",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> +	},
> +	[ID_AD4004] = {
> +		.dev_name = "ad4004",
> +		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> +	},
> +	[ID_AD4005] = {
> +		.dev_name = "ad4005",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> +	},
> +	[ID_AD4006] = {
> +		.dev_name = "ad4006",
> +		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> +	},
> +	[ID_AD4007] = {
> +		.dev_name = "ad4007",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> +	},
> +	[ID_AD4008] = {
> +		.dev_name = "ad4008",
> +		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> +	},
> +	[ID_AD4010] = {
> +		.dev_name = "ad4010",
> +		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> +	},
> +	[ID_AD4011] = {
> +		.dev_name = "ad4011",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> +	},
> +	[ID_AD4020] = {
> +		.dev_name = "ad4020",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> +	},
> +	[ID_AD4021] = {
> +		.dev_name = "ad4021",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> +	},
> +	[ID_AD4022] = {
> +		.dev_name = "ad4022",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> +	},
> +	[ID_ADAQ4001] = {
> +		.dev_name = "adaq4001",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> +	},
> +	[ID_ADAQ4003] = {
> +		.dev_name = "adaq4003",
> +		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> +	},
> +};
> +

Please have the above as a different variable per device rather than the array.
Likely no need for the enum then...
 
> +struct ad4000_state {
> +	struct spi_device *spi;
> +	struct gpio_desc *cnv_gpio;
> +	struct spi_transfer xfers[2];
> +	struct spi_message msg;
> +	int vref;
> +	enum ad4000_spi_mode spi_mode;
> +	bool span_comp;
> +	bool turbo_mode;
> +	int gain_milli;
> +	int scale_tbl[2][2];
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	struct {
> +		union {
> +			__be16 sample_buf16;
> +			__be32 sample_buf32;
> +		} data;
> +		s64 timestamp __aligned(8);
> +	} scan __aligned(IIO_DMA_MINALIGN);
> +	__be16 tx_buf;
> +	__be16 rx_buf;
> +};
> +
> +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
> +				  const struct ad4000_chip_info *chip)
> +{
> +	int diff = chip->chan_spec.differential;
> +	int val, val2, tmp0, tmp1;
> +	u64 tmp2;
> +
> +	val2 = scale_bits;
> +	val = st->vref / 1000;
> +	/*
> +	 * The gain is stored as a fraction of 1000 and, as we need to
> +	 * divide vref by gain, we invert the gain/1000 fraction.
> +	 * Also multiply by an extra MILLI to avoid losing precision.
> +	 */
> +	val = mult_frac(val, MILLI * MILLI, st->gain_milli);

Why not MICRO :)?

> +	/* Would multiply by NANO here but we multiplied by extra MILLI */
> +	tmp2 = shift_right((u64)val * MICRO, val2);
> +	tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);

no cast needed...

> +	/* Store scale for when span compression is disabled */
> +	st->scale_tbl[0][0] = tmp0; /* Integer part */
> +	st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
> +	/* Store scale for when span compression is enabled */
> +	st->scale_tbl[1][0] = tmp0;
> +	/* The integer part is always zero so don't bother to divide it. */
> +	if (diff)
> +		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> +	else
> +		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> +}
> +
> +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> +{
> +	st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE |
> val);
> +	return spi_write(st->spi, &st->tx_buf, 2);

sizeof(tx_buf)?

> +}
> +
> +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 = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);
> +	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = be16_to_cpu(st->rx_buf);
> +
> +	return ret;
> +}
> +
> +static void ad4000_unoptimize_msg(void *msg)
> +{
> +	spi_unoptimize_message(msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected by setting the adi,spi-mode device tree
> property
> + * to "single". 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;
> +	int ret;
> +
> +	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);
> +
> +	ret = spi_optimize_message(st->spi, &st->msg);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
> +					&st->msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "4-wire" mode, selected when the adi,spi-mode device tree
> + * property is absent or empty. 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;
> +	int ret;
> +
> +	/*
> +	 * 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);
> +
> +	ret = spi_optimize_message(st->spi, &st->msg);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
> +					&st->msg);
> +}
> +
> +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 st->cnv_gpio is NULL and
> is
> +	 * ignored (CS is wired to CNV in those cases).
> +	 */
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);

Not sure it's a good practise to assume internal details as you're going for
GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or
not.
  
> +	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);
> 

no error check

> +	if (chan->scan_type.storagebits > 16)
> +		sample = be32_to_cpu(st->scan.data.sample_buf32);
> +	else
> +		sample = be16_to_cpu(st->scan.data.sample_buf16);
> +
> +	switch (chan->scan_type.realbits) {
> +	case 16:
> +		break;
> +	case 18:
> +		sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> +		break;
> +	case 20:
> +		sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ad4000_single_conversion(indio_dev, chan,
> val);
> +		unreachable();
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->scale_tbl[st->span_comp][0];
> +		*val2 = st->scale_tbl[st->span_comp][1];
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;
> +		if (st->span_comp)
> +			*val = mult_frac(st->vref / 1000, 1, 10);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)st->scale_tbl;
> +		*length = 2 * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan, long
> mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +}
> +
> +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:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +			ret = ad4000_read_reg(st, &reg_val);
> +			if (ret < 0)
> +				return ret;
> +
> +			span_comp_en = (val2 == st->scale_tbl[1][1]);

no () needed

> +			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)
> +				return ret;
> +
> +			st->span_comp = span_comp_en;
> +			return 0;
> +		}
> +		unreachable();
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4000_convert_and_acquire(st);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> +					   iio_get_time_ns(indio_dev));
> +
> +err_out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad4000_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (readval)
> +		ret = ad4000_read_reg(st, readval);
> +	else
> +		ret = ad4000_write_reg(st, writeval);

if (readval)
	return ad4000_read_reg();

return ad4000_write_reg();


> +
> +	return ret;
> +}
> +
> +static const struct iio_info ad4000_info = {
> +	.read_raw = &ad4000_read_raw,
> +	.read_avail = &ad4000_read_avail,
> +	.write_raw = &ad4000_write_raw,
> +	.write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> +	.debugfs_reg_access = &ad4000_reg_access,
> +
> +};
> +
> +static int ad4000_config(struct ad4000_state *st)
> +{
> +	unsigned int reg_val;
> +
> +	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> +		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
> +
> +	/*
> +	 * The ADC SDI pin might be connected to controller CS line in which
> +	 * case the write might fail. This, however, does not prevent the
> device
> +	 * from functioning even though in a configuration other than the
> +	 * requested one.
> +	 */

This raises the question if there's any way to describe that through DT (if not
doing it already)? So that, if SDI is connected to CS we don't even call this?
Other question that comes to mind is that in case SDI is connected to CS, will
all writes fail? Because if that's the case we other writes (like scale) that
won't work and we should take care of that...

> +	return ad4000_write_reg(st, reg_val);
> +}
> +
> +static void ad4000_regulator_disable(void *reg)
> +{
> +	regulator_disable(reg);
> +}
> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> +	const struct ad4000_chip_info *chip;
> +	struct regulator *vref_reg;
> +	struct iio_dev *indio_dev;
> +	struct ad4000_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->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(&spi->dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to enable VDD
> supply\n");
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "vio");
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to enable VIO
> supply\n");
> +
> +	vref_reg = devm_regulator_get(&spi->dev, "ref");
> +	if (IS_ERR(vref_reg))
> +		return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> +				     "Failed to get vref regulator\n");

Should this not be an optional one? If not, why not devm_regulator_get_enable()?
Also consider the new devm_regulator_get_enable_read_voltage() - you need to
handle -ENODEV in case this is optional. 

> +
> +	ret = regulator_enable(vref_reg);
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to enable voltage regulator\n");
> +
> +	ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable,
> vref_reg);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to add regulator disable
> action\n");
> +
> +	st->vref = regulator_get_voltage(vref_reg);
> +	if (st->vref < 0)
> +		return dev_err_probe(&spi->dev, st->vref, "Failed to get
> vref\n");
> +

I think in all places you're using this you st->vref / 1000, right? Do it here
just once...

> +	st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv",
> GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get CNV GPIO");
> +
> +	ret = device_property_match_property_string(&spi->dev, "adi,spi-
> mode",
> +						    ad4000_spi_modes,
> +						   
> ARRAY_SIZE(ad4000_spi_modes));
> +	/* Default to 4-wire mode if adi,spi-mode property is not present */
> +	if (ret == -EINVAL)
> +		st->spi_mode = AD4000_SPI_MODE_DEFAULT;
> +	else if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "getting adi,spi-mode property
> failed\n");
> +	else
> +		st->spi_mode = ret;
> +
> +	switch (st->spi_mode) {
> +	case AD4000_SPI_MODE_DEFAULT:
> +		ret = ad4000_prepare_4wire_mode_message(st, &chip-
> >chan_spec);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case AD4000_SPI_MODE_SINGLE:
> +		ret = ad4000_prepare_3wire_mode_message(st, &chip-
> >chan_spec);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * 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_MODE_0 | SPI_MOSI_IDLE_HIGH;
> +		if (spi_setup(spi))
> +			dev_warn(&st->spi->dev, "SPI controller setup
> failed\n");

Does not look like a warn to me... Also, spi_setup() should already print an
error message so this will duplicate that.

> +
> +		break;
> +	}
> +
> +	ret = ad4000_config(st);
> +	if (ret < 0)
> +		dev_dbg(&st->spi->dev, "Failed to config device\n");

Also questionable but see the my comment on ad4000_config(). In any case this
should be visible to the user so I would at least use dev_warn().

> +
> +	indio_dev->name = chip->dev_name;
> +	indio_dev->info = &ad4000_info;
> +	indio_dev->channels = &chip->chan_spec;
> +	indio_dev->num_channels = 1;
> +
> +	/* Hardware gain only applies to ADAQ devices */
> +	st->gain_milli = 1000;
> +	if (device_property_present(&spi->dev, "adi,gain-milli")) {
> +
> +		ret = device_property_read_u32(&spi->dev, "adi,gain-milli",
> +					       &st->gain_milli);

Can we have full unsigned int range for the gain? I guess not :)


- Nuno Sá

Powered by blists - more mailing lists