Re: [PATCH v2 13/13] iio: adc: ad4080: add driver support

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

On Tue, 2025-04-22 at 08:12 +0000, Miclaus, Antoniu wrote:
> > > +	int ret;
> > > +
> > > +	guard(mutex)(&st->lock);
> > > +	if (st->num_lanes == 1)
> > > +		ret = regmap_write(st->regmap,
> > > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > > +				   AD4080_RESERVED_CONFIG_A_MSK |
> > > +				   AD4080_INTF_CHK_EN_MSK);
> > > +	else
> > > +		ret = regmap_write(st->regmap,
> > > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > > +				   AD4080_RESERVED_CONFIG_A_MSK |
> > > +				   AD4080_INTF_CHK_EN_MSK |
> > > +				   AD4080_SPI_LVDS_LANES_MSK);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = iio_backend_data_alignment_enable(st->back);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	do {
> > > +		ret = iio_backend_sync_status_get(st->back, &sync_en);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		if (!sync_en)
> > > +			dev_dbg(&st->spi->dev, "Not Locked: Running Bit
> > > Slip\n");
> > > +	} while (--timeout && !sync_en);
> > > +
> > > +	if (timeout) {
> > > +		dev_info(&st->spi->dev, "Success: Pattern correct and
> > > Locked!\n");
> > > +		if (st->num_lanes == 1)
> > > +			return regmap_write(st->regmap,
> > > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > > +
> > 	    AD4080_RESERVED_CONFIG_A_MSK);
> > > +		else
> > > +			return regmap_write(st->regmap,
> > > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > > +
> > 	    AD4080_RESERVED_CONFIG_A_MSK |
> > > +					    AD4080_SPI_LVDS_LANES_MSK);
> > > +	} else {
> > > +		dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> > > +		if (st->num_lanes == 1) {
> > > +			ret = regmap_write(st->regmap,
> > > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > > +
> > 	   AD4080_RESERVED_CONFIG_A_MSK);
> > > +			if (ret)
> > > +				return ret;
> > > +		} else {
> > > +			ret = regmap_write(st->regmap,
> > > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > > +
> > 	   AD4080_RESERVED_CONFIG_A_MSK |
> > > +					   AD4080_SPI_LVDS_LANES_MSK);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +
> > > +		return -ETIMEDOUT;
> > 
> > So, first the things that I don't really get (also the hdl docs need to be
> > improved FWIW):
> > 
> > * It seems that we can have data alignment or data capture synchronization
> > through an internal AMD FPGA technique called bit-slip or an external
> > signal,
> > right? In here, it seems that we only use bit-slip and never disable it. Is
> > that
> > really the goal?
> > 
> > * This whole process seems to be some kind of calibration at the interface
> > level, right?
> > 
> > * What's the point of the conv clock? Is it only used to get the sample
> > rate? If
> > so, the hdl docs are misleading as it seems that it can be used instead of
> > bit-
> > slip for data capture alignment?
> > 
> > Now, speaking about the backend API's, it looks like that
> > iio_backend_self_sync_enable() and iio_backend_data_alignment_enable()
> > could be
> > merged into one API. From what I can tell,
> > iio_backend_data_alignment_enable()
> > just enables the bit-slip process but that likely does nothing unless the
> > SELF_SYNC bit is also set to bit-slip, right? In fact, we could think about
> > a
> > more generic (less flexible though) API like:
> > 
> > * iio_backend_intf_data_align(back, timeout_us), or
> > * iio_backend_intf_calib(back, timeout_us), or
> > * iio_backend_intf_data_capture_sync(back, timeout_us)
> > 
> > On the backend side, you could then enable bit-slip and do the status read
> > (with
> > timeout) and return success or an error code.
> > 
> > The above seems to be pretty much what you're doing but in just one API that
> > makes sense to me.
> > 
> > Of course that the following questions still come to mind:
> > 
> > * Do we need to disable bit-slip after we're done (still fits into the one
> > API
> > model)?
> > * Do we need a meaningful API to change between the syncing/alignment
> > methods?
> > External signal vs bit-slip?
> > 
> > The above is key to better think of an API. Right now it feels that you're
> > just
> > adding an API for every bit you want to control in this process...
> > 
> > If we end up needing more flexibility for this, we can also consider the
> > existing iio_backend_data_sample_trigger() API. I know is abusing a bit and
> > a
> > stretch but in the end of the day the whole thing is related with aligning,
> > syncing, calibrating the interface for properly sampling data. Even bit-slip
> > (while not a traditional external trigger) looks like some kind of self-
> > adjusting, data-driven trigger mechanism to establish the correct starting
> > point
> > for capturing data. So having two new enums like:
> > 
> > IIO_BACKEND_SAMPLE_TRIGGER_EXT_SIGNAL,
> > IIO_BACKEND_SAMPLE_TRIGGER_BIT_SLIP // or maybe a more generic name
> > like
> > s/BIT_SLIP/INTERNAL
> > 
> > I do not think the above is that horrible :P... But I would really like to
> > get
> > more opinions about this.
> 
> There've been some update on the HDL side changing a bit the behavior:
> - bitslip_enable is removed, instead the `sync` bit is used which is already
> part
>   of the adc common core.
>  SYNC:
>        This bit enables capture synchronization. When activated, it initiates
>        an HDL process that aligns the sample's most significant bit (MSB)
> based
>        solely on the captured data, without considering the AD4080's CNV
> signal.
>        This bit is self-clearing and should be toggled whenever
> synchronization
>        is needed (e.g., at boot or after updating the sampling rate).
> 
> The SELF_SYNC bit was removed.
> 
> SYNC_STATUS bit (which is also part of the common core) has the following
> behavior:
>       This bit indicates whether the sample's MSB alignment has been correctly
>       performed and the capture is synchronized. If successful, this bit will
>       be set to 1.

So this looks like it would fir in first approach of just one new backend API
right?

What about the CNV signal? Is that something we can fully control on the
frontend driver (though it also seems that signal is an output of the backend
device)?

Thx!
- Nuno Sá