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

[Nuno Sá]

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@xxxxxxxxxxxx>
> > 
> > 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á