Re: [PATCH v2 8/9] iio: dac: ad3552r: add axi platform driver

["Nuno Sá" <noname.nuno@gmail.com>]

Hi all,

Some comments on top of what David already said...

On Thu, 2024-09-05 at 15:40 -0500, David Lechner wrote:
> On 9/5/24 10:17 AM, Angelo Dureghello wrote:
> 
> ...
> 
> > +
> > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev,
> > +				struct iio_chan_spec const *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct ad3552r_axi_state *st = iio_priv(indio_dev);
> > +	int err, ch = chan->channel;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ: {
> > +		int clk_rate;
> > +
> > +		err = iio_backend_read_raw(st->back, chan, &clk_rate, 0,
> > +					   IIO_CHAN_INFO_FREQUENCY);
> 
> This seems odd to me. How does the backend know what frequency we want?
> It would make more sense to me if this somehow indicated that we were
> getting the SPI SCLK rate.
> 

Yes, this sampling frequency bit seems very wrong atm. And the thing is, we're
not even getting SCLK. According to [1], the /4 and /8 is for clk_in which is
not the same as SCLK (unless I'm missing something). 

OTOH, if in the backend patch, that clk_get() is somehow getting sclk, that's
wrong because sclk is an output clk of the IP. So we need to get clk_in which
should be (typically) 133MHz.

> > +		if (err != IIO_VAL_INT)
> 
> Would be better to call the variable ret instead of err if it can hold
> something besides an error code.
> 
> > +			return err;
> > +
> > +		/*
> > +		 * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate).
> > +		 * Samplerate has sense in DDR only.
> 
> We should also mention that this assumes QSPI in addtion to DDR enabled.
> 

I understand the QSPI bit but why the DDR part? I just don't understand the
comment "Samplerate has sense in DDR only.". It needs way more explanation if
that is true...

> > +		 */
> > +		if (st->single_channel)
> > +			clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4);
> > +		else
> > +			clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8);
> > +
> 

This division also looks to be very backend dependent. So it's far from ideal
being in here...

To me, the way we need to get this done is for the backend to effectively report
back SCLK (in a correct way). Then, depending on the number of bits per clk (4
for QSPI), the word size and DDR vs SDR we get the device sample rate. With it,
we then choose one of Jonathan's suggestion (a per channel attr might be less
confusing).

All the above said, I probably need to catch up on the above. It might happen
that David and Angelo already got some more info from the hdl guys while I was
on vacation.

[1]: https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
- Nuno Sá