linux-kernel - Re: [PATCH v6 7/8] iio: dac: ad3552r: add high-speed platform driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8642bdb546c6046e8fe1d20ef4c93e70c95c6f71.camel@gmail.com>
Date: Tue, 15 Oct 2024 08:37:59 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Angelo Dureghello
 <adureghello@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>,  Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>,  Jonathan Cameron
 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Olivier Moysan
 <olivier.moysan@...s.st.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v6 7/8] iio: dac: ad3552r: add high-speed platform driver

On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote:
> On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@...libre.com>
> > 
> > Add High Speed ad3552r platform driver.
> > 
> 
> ...
> 
> > +static int ad3552r_hs_read_raw(struct iio_dev *indio_dev,
> > +			       struct iio_chan_spec const *chan,
> > +			       int *val, int *val2, long mask)
> > +{
> > +	struct ad3552r_hs_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ: {
> > +		int sclk;
> > +
> > +		ret = iio_backend_read_raw(st->back, chan, &sclk, 0,
> > +					   IIO_CHAN_INFO_FREQUENCY);
> 
> FWIW, this still seems like an odd way to get the stream mode SCLK
> rate from the backend to me. How does the backend know that we want
> the stream mode clock rate and not some other frequency value? 

In this case the backend has a dedicated compatible so sky is the limit :). But yeah,
I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you have in
mind? Using the sampling frequency INFO or a dedicated OP?

> 
> > +		if (ret != IIO_VAL_INT)
> > +			return -EINVAL;
> > +
> > +		/* Using 4 lanes (QSPI) */
> > +		*val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode),
> 
> Since DDR is always enabled for buffered reads, I think we should
> always be multiplying by 2 here instead of (1 + st->ddr_mode).
> 
> Otherwise the sampling frequency attribute will return the wrong
> value if it is read when a buffered read is not currently in
> progress.
> 
> > +					 chan->scan_type.storagebits);
> 
> It would probably be more correct to use realbits here instead of
> storagebits. Usually realbits is the bits per word being sent over
> the SPI bus while storagebits can be larger.

It can go both ways I guess. For channels with eg: status bits in the sample,
realbits won't be the exact word on the bus. OTOH, yes, we do have cases for DMA
buffering where storage bits is bigger that the actual word. So, yeah, no strong
feeling about it.

- Nuno Sá