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

[Christophe JAILLET]

Le 05/09/2024 à 17:17, Angelo Dureghello a écrit :
> From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@xxxxxxxxxxxxxxxx>
>
> Add support for ad3552r-axi, where ad3552r has to be controlled
> by the custom (fpga-based) ad3552r AXI DAC IP.

...

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

I don't know if it can be an issue, but iio_backend_ddr_disable() is 
called if iio_backend_ddr_enable() fails.

> +
> +	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;
> +	}
> +
> +	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.
> +	 */
> +	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;
> +
> +	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;
> +}

...


> +#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), \
> +	.output = 1, \
> +	.indexed = 1, \
> +	.channel = (ch), \
> +	.scan_index = (ch), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_BE, \
> +	}, \
> +	.ext_info = ad3552r_axi_ext_info, \
> +}
> +
> +static struct iio_chan_spec ad3552r_axi_channels[] = {

I think (but I've not checked :)) that it could be const.

CJ

> +	AD3552R_CHANNEL(0),
> +	AD3552R_CHANNEL(1),
> +};

...