Re: [PATCH v10 5/8] iio: adc: adi-axi-adc: set data format

[Nuno Sá]

On Fri, 2025-01-17 at 11:50 -0600, David Lechner wrote:
> On 1/17/25 10:20 AM, Nuno Sá wrote:
> > On Fri, 2025-01-17 at 15:06 +0200, Antoniu Miclaus wrote:
> > > Add support for selecting the data format within the AXI ADC ip.
> > > 
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > > ---
> > 
> > Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > 
> > > no changes in v10.
> > >  drivers/iio/adc/adi-axi-adc.c | 46 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 46 insertions(+)
> > > 
> > > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> > > index d2e1dc63775c..3c213ca5ff8e 100644
> > > --- a/drivers/iio/adc/adi-axi-adc.c
> > > +++ b/drivers/iio/adc/adi-axi-adc.c
> > > @@ -45,6 +45,12 @@
> > >  #define ADI_AXI_ADC_REG_CTRL			0x0044
> > >  #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
> > >  
> > > +#define ADI_AXI_ADC_REG_CNTRL_3			0x004c
> > > +#define   AD485X_CNTRL_3_PACKET_FORMAT_MSK	GENMASK(1, 0)
> > > +#define   AD485X_PACKET_FORMAT_20BIT		0x0
> > > +#define   AD485X_PACKET_FORMAT_24BIT		0x1
> > > +#define   AD485X_PACKET_FORMAT_32BIT		0x2
> > > +
> > >  #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
> > >  #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
> > >  
> > > @@ -312,6 +318,45 @@ static int axi_adc_interface_type_get(struct iio_backend
> > > *back,
> > >  	return 0;
> > >  }
> > >  
> > > +static int axi_adc_data_size_set(struct iio_backend *back, unsigned int size)
> > > +{
> > > +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> > > +	unsigned int val;
> > > +
> > > +	switch (size) {
> > > +	/*
> > > +	 * There are two different variants of the AXI AD485X IP block, a 16-
> > > bit
> > > +	 * and a 20-bit variant.
> > > +	 * The 0x0 value (AD485X_PACKET_FORMAT_20BIT) is corresponding also
> > > to
> > > +	 * the 16-bit variant of the IP block.
> > > +	 */
> > > +	case 16:
> > > +	case 20:
> > > +		val = AD485X_PACKET_FORMAT_20BIT;
> > > +		break;
> > > +	case 24:
> > > +		val = AD485X_PACKET_FORMAT_24BIT;
> > > +		break;
> > > +	/*
> > > +	 * The 0x2 (AD485X_PACKET_FORMAT_32BIT) corresponds only to the 20-
> > > bit
> > > +	 * variant of the IP block. Setting this value properly is ensured by
> > > +	 * the upper layers of the drivers calling the axi-adc functions.
> > > +	 * Also, for 16-bit IP block, the 0x2 (AD485X_PACKET_FORMAT_32BIT)
> > > +	 * value is handled as maximum size available which is 24-bit for
> > > this
> > > +	 * configuration.
> > > +	 */
> > > +	case 32:
> > > +		val = AD485X_PACKET_FORMAT_32BIT;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
> > > +				  AD485X_CNTRL_3_PACKET_FORMAT_MSK,
> > > +				 
> > > FIELD_PREP(AD485X_CNTRL_3_PACKET_FORMAT_MSK, val));
> > > +}
> > > +
> > >  static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
> > >  						 struct iio_dev *indio_dev)
> > >  {
> > > @@ -360,6 +405,7 @@ static const struct iio_backend_ops adi_axi_adc_ops = {
> > >  	.test_pattern_set = axi_adc_test_pattern_set,
> > >  	.chan_status = axi_adc_chan_status,
> > >  	.interface_type_get = axi_adc_interface_type_get,
> > > +	.data_size_set = axi_adc_data_size_set,
> > >  	.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
> > >  	.debugfs_print_chan_status =
> > > iio_backend_debugfs_ptr(axi_adc_debugfs_print_chan_status),
> > >  };
> > 
> > 
> 
> Since these register values are specific to the AD485X variant of the AXI ADC,
> I still feel like it would be better if we added a new compatible string like
> we did for AD355X on the AXI DAC.
> 

Yeah, my first intent was to avoid new compatibles on the backend side (as it can
grow pretty wildly). But obviously that if you have two different IPs with different
meanings for the same registers, there's nothing we can do. At some point, I thought
about havng the backend provide a set of custom registers and then each frontend
would now what to do with them. But different compatibles has the clear advantage to
make differences between IPs clear and to actually control what a frontend can or
cannot do. Anyways, I hope that for simple enough cases we can "live" with the
generic compatible or asking HDL for more registers identifying/exposing capabilities
of the core.
> These functions accessing the CNTRL_3 register aren't applicable to the generic
> AXI ADC IP block, but only to the AXI AD485X IP core [1]. The AXI AD7606X IP
> core [2] that we are working on also uses this same register for other purposes,
> so we will on have a conflict. We are planning on adding a new AXI ADC compatible
> string for AD7606X [3], so I think we should do the same here.

Given that we'll have another device messing with the same registers, I think it's
clear we also need a new compatible here (so we are consistent at the very least).


- Nuno Sá