Re: [PATCH v6 7/8] iio: dac: ad3552r: add high-speed platform driver

[David Lechner]

On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> 
> Add High Speed ad3552r platform driver.
> 

...

> +static int ad3552r_hs_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int *val, int *val2, long mask)
> +{
> +	struct ad3552r_hs_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		int sclk;
> +
> +		ret = iio_backend_read_raw(st->back, chan, &sclk, 0,
> +					   IIO_CHAN_INFO_FREQUENCY);

FWIW, this still seems like an odd way to get the stream mode SCLK
rate from the backend to me. How does the backend know that we want
the stream mode clock rate and not some other frequency value? 

> +		if (ret != IIO_VAL_INT)
> +			return -EINVAL;
> +
> +		/* Using 4 lanes (QSPI) */
> +		*val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode),

Since DDR is always enabled for buffered reads, I think we should
always be multiplying by 2 here instead of (1 + st->ddr_mode).

Otherwise the sampling frequency attribute will return the wrong
value if it is read when a buffered read is not currently in
progress.

> +					 chan->scan_type.storagebits);

It would probably be more correct to use realbits here instead of
storagebits. Usually realbits is the bits per word being sent over
the SPI bus while storagebits can be larger.

> +
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_RAW:
> +		ret = st->data->bus_reg_read(st->back,
> +				AD3552R_REG_ADDR_CH_DAC_16B(chan->channel),
> +				val, 2);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...

> +static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad3552r_hs_state *st = iio_priv(indio_dev);
> +	struct iio_backend_data_fmt fmt = {
> +		.type = IIO_BACKEND_DATA_UNSIGNED
> +	};
> +	int loop_len, val, err;
> +
> +	/* Inform DAC chip to switch into DDR mode */
> +	err = ad3552r_qspi_update_reg_bits(st,
> +					   AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +					   AD3552R_MASK_SPI_CONFIG_DDR,
> +					   AD3552R_MASK_SPI_CONFIG_DDR, 1);
> +	if (err)
> +		return err;
> +
> +	/* Inform DAC IP to go for DDR mode from now on */
> +	err = iio_backend_ddr_enable(st->back);
> +	if (err) {
> +		dev_warn(st->dev, "could not set DDR mode, not streaming");
> +		goto exit_err;
> +	}
> +
> +	st->ddr_mode = true;
> +
> +	switch (*indio_dev->active_scan_mask) {
> +	case AD3552R_CH0_ACTIVE:
> +		st->single_channel = true;
> +		loop_len = 2;
> +		val = AD3552R_REG_ADDR_CH_DAC_16B(0);
> +		break;
> +	case AD3552R_CH1_ACTIVE:
> +		st->single_channel = true;
> +		loop_len = 2;
> +		val = AD3552R_REG_ADDR_CH_DAC_16B(1);
> +		break;
> +	case AD3552R_CH0_CH1_ACTIVE:
> +		st->single_channel = false;
> +		loop_len = 4;
> +		val = AD3552R_REG_ADDR_CH_DAC_16B(1);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		goto exit_err_ddr;
> +	}
> +
> +	err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE,
> +				      loop_len, 1);

It would be more logical to set this before switching to DDR mode.
No need to do a register write with DDR enabled. 

(And would be necessary for Jonathan's hypothetical 2-SPI-controller
backend.) ;-)

> +	if (err)
> +		goto exit_err_ddr;
> +
> +	err = iio_backend_data_transfer_addr(st->back, val);
> +	if (err)
> +		goto exit_err_ddr;
> +
> +	err = iio_backend_data_format_set(st->back, 0, &fmt);
> +	if (err)
> +		goto exit_err_ddr;
> +
> +	err = iio_backend_data_stream_enable(st->back);
> +	if (err)
> +		goto exit_err_ddr;
> +
> +	return 0;
> +
> +exit_err_ddr:
> +	iio_backend_ddr_disable(st->back);

Does it actually work in this order? In ad3552r_hs_buffer_predisable()
we call ad3552r_qspi_update_reg_bits() first and then iio_backend_ddr_disable().
If DDR affects register writes, then it seems like disabling DDR in the
backend first would cause the register write to disable DDR on the DAC
to fail. So the order in ad3552r_hs_buffer_predisable() seems correct.

Probably can just drop this part and change all goto exit_err_ddr;
to goto exit_err;.

> +
> +exit_err:
> +	ad3552r_qspi_update_reg_bits(st,
> +				     AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +				     AD3552R_MASK_SPI_CONFIG_DDR,
> +				     0, 1);
> +
> +	iio_backend_ddr_disable(st->back);
> +
> +	st->ddr_mode = false;
> +
> +	return err;
> +}
> +
> +static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad3552r_hs_state *st = iio_priv(indio_dev);
> +	int err;
> +
> +	err = iio_backend_data_stream_disable(st->back);
> +	if (err)
> +		return err;
> +
> +	/* Inform DAC to set in SDR mode */
> +	err = ad3552r_qspi_update_reg_bits(st,
> +					   AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +					   AD3552R_MASK_SPI_CONFIG_DDR,
> +					   0, 1);
> +	if (err)
> +		return err;
> +
> +	err = iio_backend_ddr_disable(st->back);
> +	if (err)
> +		return err;
> +
> +	st->ddr_mode = false;
> +
> +	return 0;
> +}
> +

...

> +
> +#define AD3552R_CHANNEL(ch) { \
> +	.type = IIO_VOLTAGE, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \

As described in [1], the sampling freq attr should be info_mask_separate
since it is the sample rate per individual sample, not the sample rate
per scan.

[1]: https://lore.kernel.org/linux-iio/20240908164940.7c4ffb8a@jic23-huawei/


> +	.output = 1, \
> +	.indexed = 1, \
> +	.channel = (ch), \
> +	.scan_index = (ch), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_BE, \
> +	} \
> +}
> +