Re: [PATCH v2 8/9] iio: dac: ad3552r: add axi platform driver

[David Lechner]

On 9/5/24 10:17 AM, Angelo Dureghello wrote:

...

> +
> +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct ad3552r_axi_state *st = iio_priv(indio_dev);
> +	int err, ch = chan->channel;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		int clk_rate;
> +
> +		err = iio_backend_read_raw(st->back, chan, &clk_rate, 0,
> +					   IIO_CHAN_INFO_FREQUENCY);

This seems odd to me. How does the backend know what frequency we want?
It would make more sense to me if this somehow indicated that we were
getting the SPI SCLK rate.

> +		if (err != IIO_VAL_INT)

Would be better to call the variable ret instead of err if it can hold
something besides an error code.

> +			return err;
> +
> +		/*
> +		 * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate).
> +		 * Samplerate has sense in DDR only.

We should also mention that this assumes QSPI in addtion to DDR enabled.

> +		 */
> +		if (st->single_channel)
> +			clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4);
> +		else
> +			clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8);
> +

Having the sample rate depend on how many channels are enabled in
the buffer seems a bit odd. Sampling frequency is not strictly
defined in IIO, so I think it would be fine to always return the
same value no matter how many channels are enabled.

We will just need to document that the sampling frequency is the
rate per sample, not per channel. So if two channels are enabled,
the effective sampling rate per channel is 1/2 of the sampling
rate reported by the sysfs attribute. 

> +		*val = clk_rate;
> +
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_RAW:

Do we need iio_device_claim_direct_scoped() here to prevent attempting
to do register access while a buffered write is in progress?

> +		err = iio_backend_bus_reg_read(st->back,
> +					       AD3552R_REG_ADDR_CH_DAC_16B(ch),
> +					       val, 2);
> +		if (err)
> +			return err;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3552r_axi_write_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +			struct ad3552r_axi_state *st = iio_priv(indio_dev);
> +			int ch = chan->channel;
> +
> +			return iio_backend_bus_reg_write(st->back,
> +				    AD3552R_REG_ADDR_CH_DAC_16B(ch), val, 2);
> +		}
> +		unreachable();
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad3552r_axi_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 = axi3552r_qspi_update_reg_bits(st->back,
> +					    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)
> +		goto exit_err;

Might want a comment or dev_warn() here. If we put the DAC in DDR
mode but can't put the backend in DDR mode, then things are probably
going to be a bit broken if we can't get the DAC back out of DDR
mode. Not likely to ever get an error here, but it will be helpful
to let readers of the code know why the unwinding isn't exactly
balanced.

> +> +	switch (*indio_dev->active_scan_mask) {
> +	case AD3552R_CH0_ACTIVE:
> +		st->single_channel = true;
> +		loop_len = AD3552R_STREAM_2BYTE_LOOP;
> +		val = AD3552R_REG_ADDR_CH_DAC_16B(0);
> +		break;
> +	case AD3552R_CH1_ACTIVE:
> +		st->single_channel = true;
> +		loop_len = AD3552R_STREAM_2BYTE_LOOP;
> +		val = AD3552R_REG_ADDR_CH_DAC_16B(1);
> +		break;
> +	case AD3552R_CH0_CH1_ACTIVE:
> +		st->single_channel = false;
> +		loop_len = AD3552R_STREAM_4BYTE_LOOP;
> +		val = AD3552R_REG_ADDR_CH_DAC_16B(1);
> +		break;
> +	default:
> +		return -EINVAL;

Move the switch statement before changing to DDR mode or
goto exit_err here.

> +	}
> +
> +	err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE,
> +					loop_len, 1);
> +	if (err)
> +		goto exit_err;
> +
> +	err = iio_backend_data_transfer_addr(st->back, val);
> +	if (err)
> +		goto exit_err;
> +
> +	/*
> +	 * The EXT_SYNC is mandatory in the CN0585 project where 2 instances
> +	 * of the IP are in the design and they need to generate the signals
> +	 * synchronized.
> +	 *
> +	 * Note: in first IP implementations CONFIG EXT_SYNC (RO) can be 0,
> +	 * but EXT_SYMC (ext synch ability) is enabled anyway.

EXT_SYNC

> +	 */
> +	if (st->synced_transfer == AD3552R_EXT_SYNC_ARM)
> +		err = iio_backend_ext_sync_enable(st->back);
> +	else
> +		err = iio_backend_ext_sync_disable(st->back);
> +	if (err)
> +		goto exit_err_sync;

		goto exit_err;

If enabling failed, no need to disable.

> +
> +	err = iio_backend_data_format_set(st->back, 0, &fmt);
> +	if (err)
> +		goto exit_err_sync;
> +
> +	err = iio_backend_buffer_enable(st->back);
> +	if (err)
> +		goto exit_err_sync;
> +
> +	return 0;
> +
> +exit_err_sync:
> +	iio_backend_ext_sync_disable(st->back);
> +
> +exit_err:
> +	axi3552r_qspi_update_reg_bits(st->back,
> +				      AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +				      AD3552R_MASK_SPI_CONFIG_DDR,
> +				      0, 1);
> +
> +	iio_backend_ddr_disable(st->back);
> +
> +	return err;
> +}
> +
> +static int ad3552r_axi_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad3552r_axi_state *st = iio_priv(indio_dev);
> +	int err;
> +
> +	err = iio_backend_buffer_disable(st->back);
> +	if (err)
> +		return err;

Looks like iio_backend_ext_sync_disable(st->back); should be called here.

> +
> +	/* Inform DAC to set in SDR mode */
> +	err = axi3552r_qspi_update_reg_bits(st->back,
> +					    AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +					    AD3552R_MASK_SPI_CONFIG_DDR,
> +					    0, 1);
> +	if (err)
> +		return err;
> +
> +	return iio_backend_ddr_disable(st->back);
> +}
> +