Message-ID:
 <CY4PR03MB3399B23673D9C3210C8CE9B99BBB2@CY4PR03MB3399.namprd03.prod.outlook.com>
Date: Tue, 22 Apr 2025 08:12:25 +0000
From: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
To: Nuno Sá <noname.nuno@...il.com>,
        "jic23@...nel.org"
	<jic23@...nel.org>,
        "robh@...nel.org" <robh@...nel.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "linux-iio@...r.kernel.org"
	<linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 13/13] iio: adc: ad4080: add driver support

> > +	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.

Powered by blists - more mailing lists