Message-ID:
 <CY4PR03MB339972357BC25CD718D6FE039B842@CY4PR03MB3399.namprd03.prod.outlook.com>
Date: Fri, 25 Apr 2025 07:56:51 +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



--
Antoniu Miclăuş

> -----Original Message-----
> From: Nuno Sá <noname.nuno@...il.com>
> Sent: Tuesday, April 22, 2025 2:39 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@...log.com>; jic23@...nel.org;
> robh@...nel.org; conor+dt@...nel.org; linux-iio@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2 13/13] iio: adc: ad4080: add driver support
> 
> [External]
> 
> 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?
In this particular case, yes, one backend would fit for the sync process. But
taking into account that these two features are part also from the common core
of the AXI ADC, in other cases they might be used separately.
> 
> 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)?
The CNV signal is mainly used for sampling (an input pin according to the datasheet - 
conversion is initiated on the rising edge of the convert signal).
We use it only for determining the sampling frequency.
> 
> Thx!
> - Nuno Sá

Powered by blists - more mailing lists