Message-ID: <20241219170807.4137f4ea@jic23-huawei>
Date: Thu, 19 Dec 2024 17:08:07 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Mihail Chindris
 <mihail.chindris@...log.com>, Nuno Sa <nuno.sa@...log.com>, David Lechner
 <dlechner@...libre.com>, Olivier Moysan <olivier.moysan@...s.st.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/8] iio: dac: ad3552r-hs: add ad3541/2r support

On Mon, 16 Dec 2024 21:36:27 +0100
Angelo Dureghello <adureghello@...libre.com> wrote:

> From: Angelo Dureghello <adureghello@...libre.com>
> 
> A new fpga HDL has been developed from ADI to support ad354xr
> devices.
> 
Hi Angelo,

A few comments inline, but mostly / I think this could do with breaking
up into several patches. Particularly as that instruction mode
one sounds like it may turn out to be a fix we need to backport at
some point if problems are seen with previous approach.

There is also a move / consolidation of structures that could be a trivial
precursor and make the final patch simpler.

> 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,
> - use DAC "instruction mode" when backend is not buffering, suggested
>   from the ADI HDL team as more proper configuration mode to be used
>   for all ad35xxr devices,
Perhaps that change should be a precursor patch? 

> - change samplerate to respect number of lanes.
> 
> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>

> ---
>  drivers/iio/dac/ad3552r-common.c |  44 +++++++
>  drivers/iio/dac/ad3552r-hs.c     | 262 ++++++++++++++++++++++++++++++++-------
>  drivers/iio/dac/ad3552r.c        |  36 ------
>  drivers/iio/dac/ad3552r.h        |   8 ++
>  4 files changed, 270 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c
> index 03e0864f5084..2a0dd18ca906 100644
> --- a/drivers/iio/dac/ad3552r-common.c
> +++ b/drivers/iio/dac/ad3552r-common.c
> @@ -47,6 +47,50 @@ u16 ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs)
>  }
>  EXPORT_SYMBOL_NS_GPL(ad3552r_calc_custom_gain, "IIO_AD3552R");
>  
> +const struct ad3552r_model_data ad3541r_model_data = {
> +	.model_name = "ad3541r",
> +	.chip_id = AD3541R_ID,
> +	.num_hw_channels = 1,
> +	.ranges_table = ad3542r_ch_ranges,
> +	.num_ranges = ARRAY_SIZE(ad3542r_ch_ranges),
> +	.requires_output_range = true,
> +	.num_spi_data_lanes = 2,
> +};
> +EXPORT_SYMBOL_NS_GPL(ad3541r_model_data, "IIO_AD3552R");
> +
> +const struct ad3552r_model_data ad3542r_model_data = {
> +	.model_name = "ad3542r",
> +	.chip_id = AD3542R_ID,
> +	.num_hw_channels = 2,
> +	.ranges_table = ad3542r_ch_ranges,
> +	.num_ranges = ARRAY_SIZE(ad3542r_ch_ranges),
> +	.requires_output_range = true,
> +	.num_spi_data_lanes = 2,
> +};
> +EXPORT_SYMBOL_NS_GPL(ad3542r_model_data, "IIO_AD3552R");
> +
> +const struct ad3552r_model_data ad3551r_model_data = {
> +	.model_name = "ad3551r",
> +	.chip_id = AD3551R_ID,
> +	.num_hw_channels = 1,
> +	.ranges_table = ad3552r_ch_ranges,
> +	.num_ranges = ARRAY_SIZE(ad3552r_ch_ranges),
> +	.requires_output_range = false,
> +	.num_spi_data_lanes = 4,
> +};
> +EXPORT_SYMBOL_NS_GPL(ad3551r_model_data, "IIO_AD3552R");
> +
> +const struct ad3552r_model_data ad3552r_model_data = {
> +	.model_name = "ad3552r",
> +	.chip_id = AD3552R_ID,
> +	.num_hw_channels = 2,
> +	.ranges_table = ad3552r_ch_ranges,
> +	.num_ranges = ARRAY_SIZE(ad3552r_ch_ranges),
> +	.requires_output_range = false,
> +	.num_spi_data_lanes = 4,
> +};
> +EXPORT_SYMBOL_NS_GPL(ad3552r_model_data, "IIO_AD3552R");

Maybe move these in a precursor patch . That will make it obvious
that you are also squashing a duplicate.

> +
>  static void ad3552r_get_custom_range(struct ad3552r_ch_data *ch_data,
>  				     s32 *v_min, s32 *v_max)
>  {
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index e613eee7fc11..58c8661f483b 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -19,6 +19,31 @@

...

> @@ -304,10 +481,18 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  	if (ret)
>  		return ret;
>  
> +	/* HDL starts with DDR enabled, disabling it. */

Seems obvious. Does the comment add anything?

>  	ret = iio_backend_ddr_disable(st->back);
>  	if (ret)
>  		return ret;
>  
> +	ret = st->data->bus_reg_write(st->back,
> +				      AD3552R_REG_ADDR_INTERFACE_CONFIG_B,
> +				      AD3552R_MASK_SINGLE_INST |
> +				      AD3552R_MASK_SHORT_INSTRUCTION, 1);
> +	if (ret)
> +		return ret;
> +
>  	ret = ad3552r_hs_scratch_pad_test(st);
>  	if (ret)
>  		return ret;
> @@ -330,6 +515,8 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  				     "chip id error, expected 0x%x, got 0x%x\n",
>  				     st->model_data->chip_id, id);
>  
> +	dev_info(st->dev, "chip id %s detected", st->model_data->model_name);

dev_dbg().  We have to much noise in the kernel log already! With dynamic debug
it is easy for people to turn this on if they want it.

> +
>  	/* 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);
> @@ -342,14 +529,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;
> -
>  	ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL);
>  	if (ret)
>  		return ret;
> @@ -505,15 +684,10 @@ static int ad3552r_hs_probe(struct platform_device *pdev)
>  	return devm_iio_device_register(&pdev->dev, indio_dev);

Powered by blists - more mailing lists