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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 08 Jan 2025 18:29:22 +0100
Angelo Dureghello <adureghello@baylibre.com> 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 samplerate to respect number of lanes.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

One question inline.  I also wonder if you can easily add checks
that mean any spurious (bug) read in DDR mode would get an error
print to tell who ever triggered it what went wrong.

Jonathan

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

> +
> +static int ad3552r_hs_set_target_io_mode_hs(struct ad3552r_hs_state *st)
> +{
> +	int mode_target;
> +
> +	/*
> +	 * 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);
> +}

> @@ -319,6 +479,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  	if (ret)
>  		return ret;
>  
> +	/* HDL starts with DDR enabled, disabling it. */
>  	ret = iio_backend_ddr_disable(st->back);
>  	if (ret)
>  		return ret;
> @@ -352,6 +513,8 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  			 "Chip ID mismatch, detected 0x%x but expected 0x%x\n",
>  			 id, st->model_data->chip_id);
>  
> +	dev_info(st->dev, "chip id %s detected", st->model_data->model_name);
> +
>  	/* Clear reset error flag, see ad3552r manual, rev B table 38. */
>  	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS,
>  				      AD3552R_MASK_RESET_STATUS, 1);
> @@ -364,14 +527,6 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  	if (ret)
>  		return ret;
>  
> -	ret = st->data->bus_reg_write(st->back,
> -				AD3552R_REG_ADDR_TRANSFER_REGISTER,
> -				FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE,
> -					   AD3552R_QUAD_SPI) |
> -				AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1);
> -	if (ret)
> -		return ret;
> -
This is the call you just added to ensure we end up in instruction mode.
I'm not seeing another place it is now called so is this an accidental revert?
If it is intentional then break out a patch that deals with that change
before the addition of new parts.

I was expecting to see a call to _set_target_io_mode_hs.



>  	ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL);
>  	if (ret)
>  		return ret;
> @@ -528,6 +683,9 @@ static int ad3552r_hs_probe(struct platform_device *pdev)
>  }