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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7io6a2nrxpedkd75ovdlcb6miqwwfrnxx3mhrzv7l65vgf2qz7@h2rkoacsxtx2>
Date: Tue, 14 Jan 2025 10:18:54 +0100
From: Angelo Dureghello <adureghello@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
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>, 
	Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 8/9] iio: dac: ad3552r-hs: add ad3541/2r support

On 12.01.2025 14:36, Jonathan Cameron wrote:
> On Wed, 08 Jan 2025 18:29:22 +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.
> > 
> > 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@...libre.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.
> 
>
The above was only related the IO mode, it was set for QSPI. Now the mode
is kept as simple SPI for all the configurations, moving to DSPI or QSPI
later for streaming.
 
> 
> >  	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)
> >  }