Re: [PATCH v3 8/9] iio: dac: ad3552r-hs: add ad3541/2r support

[David Lechner <dlechner@baylibre.com>]

On 1/10/25 4:24 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> A new FPGA HDL has been developed from ADI to support ad354xr
> devices.
> 
> Add support for ad3541r and ad3542r with following additions:
> 
> - use common device_info structures for hs and non hs drivers,
> - DMA buffering, use DSPI mode for ad354xr and QSPI for ad355xr,
> - change sample rate to respect number of lanes.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/ad3552r-common.c |   4 +
>  drivers/iio/dac/ad3552r-hs.c     | 225 ++++++++++++++++++++++++++++++++-------
>  drivers/iio/dac/ad3552r.h        |   3 +
>  3 files changed, 195 insertions(+), 37 deletions(-)
> 

...

> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index bfb6228c9b9b..4600a9e84dfc 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c

...

> @@ -104,6 +136,42 @@ static int ad3552r_hs_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int ad3552r_hs_set_bus_io_mode_hs(struct ad3552r_hs_state *st)
> +{
> +	int bus_mode;
> +
> +	if (st->model_data->num_spi_data_lanes == 4)
> +		bus_mode = AD3552R_IO_MODE_QSPI;
> +	else
> +		bus_mode = AD3552R_IO_MODE_DSPI;
> +
> +	return st->data->bus_set_io_mode(st->back, bus_mode);
> +}
> +
> +static int ad3552r_hs_set_target_io_mode_hs(struct ad3552r_hs_state *st)
> +{
> +	int mode_target;

u32 would be more natural for register value. Or make an enum for this.

> +
> +	/*
> +	 * Best access for secondary reg area, QSPI where possible,
> +	 * else as DSPI.
> +	 */
> +	if (st->model_data->num_spi_data_lanes == 4)
> +		mode_target = AD3552R_QUAD_SPI;
> +	else
> +		mode_target = AD3552R_DUAL_SPI;
> +
> +	/*
> +	 * Better to not use update here, since generally it is already
> +	 * set as DDR mode, and it's not possible to read in DDR mode.
> +	 */
> +	return st->data->bus_reg_write(st->back,
> +				AD3552R_REG_ADDR_TRANSFER_REGISTER,
> +				FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE,
> +					   mode_target) |
> +				AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1);
> +}
> +
>  static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev)
>  {
>  	struct ad3552r_hs_state *st = iio_priv(indio_dev);
> @@ -132,6 +200,11 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * With ad3541/2r support, QSPI pin is held low at reset from HDL,
> +	 * streaming start sequence must respect strictly the order below.
> +	 */
> +
>  	/* Primary region access, set streaming mode (now in SPI + SDR). */
>  	ret = ad3552r_qspi_update_reg_bits(st,
>  					   AD3552R_REG_ADDR_INTERFACE_CONFIG_B,
> @@ -139,48 +212,106 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE,
> +	/*
> +	 * Set target loop len, 0x2c 0r 0x2a, descending loop, and keeping loop

Not sure what 0x2c and 0x2a mean, so comment could be improved. And 0r looks
like a typo.

> +	 * len value so it's not cleared hereafter when enabling streaming mode
> +	 * (cleared by CS_ up).
> +	 */
> +	ret = ad3552r_qspi_update_reg_bits(st,
> +		AD3552R_REG_ADDR_TRANSFER_REGISTER,
> +		AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE,
> +		AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1);
> +	if (ret)
> +		goto exit_err_streaming;
> +
> +	ret = st->data->bus_reg_write(st->back,
> +				      AD3552R_REG_ADDR_STREAM_MODE,
>  				      loop_len, 1);
>  	if (ret)
> -		return ret;
> +		goto exit_err_streaming;
>  
> -	/* Inform DAC chip to switch into DDR mode */
> -	ret = ad3552r_qspi_update_reg_bits(st,
> -					   AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> -					   AD3552R_MASK_SPI_CONFIG_DDR,
> -					   AD3552R_MASK_SPI_CONFIG_DDR, 1);
> +	/* Setting DDR now, caching current config_d. */
> +	ret = st->data->bus_reg_read(st->back,
> +				     AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +				     &st->config_d, 1);
>  	if (ret)
> -		return ret;
> +		goto exit_err_streaming;
> +
> +	st->config_d |= AD3552R_MASK_SPI_CONFIG_DDR;
> +	ret = st->data->bus_reg_write(st->back,
> +				      AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +				      st->config_d, 1);
> +	if (ret)
> +		goto exit_err_streaming;
>  
> -	/* Inform DAC IP to go for DDR mode from now on */
>  	ret = iio_backend_ddr_enable(st->back);
> -	if (ret) {
> -		dev_err(st->dev, "could not set DDR mode, not streaming");
> -		goto exit_err;
> -	}
> +	if (ret)
> +		goto exit_err_ddr_mode_target;
> +
> +	/*
> +	 * From here onward mode is DDR, so reading any register is not possible
> +	 * anymore, including calling "ad3552r_qspi_update_reg_bits" function.
> +	 */
> +
> +	/* Set target to best high speed mode (D or QSPI). */
> +	ret = ad3552r_hs_set_target_io_mode_hs(st);
> +	if (ret)
> +		goto exit_err_ddr_mode;
> +
> +	/* Set bus to best high speed mode (D or QSPI). */
> +	ret = ad3552r_hs_set_bus_io_mode_hs(st);
> +	if (ret)
> +		goto exit_err_bus_mode_target;
>  
> +	/*
> +	 * Backend setup must be done now only, or related register values will
> +	 * be disrupted by previous bus accesses.
> +	 */
>  	ret = iio_backend_data_transfer_addr(st->back, val);
>  	if (ret)
> -		goto exit_err;
> +		goto exit_err_bus_mode_target;
>  
>  	ret = iio_backend_data_format_set(st->back, 0, &fmt);
>  	if (ret)
> -		goto exit_err;
> +		goto exit_err_bus_mode_target;
>  
>  	ret = iio_backend_data_stream_enable(st->back);
>  	if (ret)
> -		goto exit_err;
> +		goto exit_err_bus_mode_target;
>  
>  	return 0;
>  
> -exit_err:
> -	ad3552r_qspi_update_reg_bits(st,
> -				     AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> -				     AD3552R_MASK_SPI_CONFIG_DDR,
> -				     0, 1);
> +exit_err_bus_mode_target:
> +	/* Back to simple SPI, not using update to avoid read. */
> +	st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_TRANSFER_REGISTER,
> +				FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE,
> +					   AD3552R_SPI) |
> +				AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1);
> +
> +	/*
> +	 * Back bus to simple SPI, this must be executed together with above
> +	 * target mode unwind, and can be done only after it.
> +	 */
> +	st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI);
>  
> +exit_err_ddr_mode:
>  	iio_backend_ddr_disable(st->back);
>  
> +exit_err_ddr_mode_target:
> +	/*
> +	 * Back to SDR. In DDR we cannot read, whatever the mode is, so not
> +	 * using update.
> +	 */
> +	st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> +				FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, 1),
> +				1);

Should be using st->config_d & ~AD3552R_MASK_SPI_CONFIG_DDR here instead of
hard-coding FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, 1).

> +
> +exit_err_streaming:
> +	/* Back to single instruction mode, disabling loop. */
> +	st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_INTERFACE_CONFIG_B,
> +				AD3552R_MASK_SINGLE_INST |
> +				AD3552R_MASK_SHORT_INSTRUCTION, 1);
> +
>  	return ret;
>  }
>