On Mon, 16 Dec 2024 21:36:27 +0100
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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);
