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

[Jonathan Cameron]

On Mon, 3 Feb 2025 11:02:58 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@xxxxxxxxxx> wrote:

>  
> > On 1/27/25 4:57 AM, Antoniu Miclaus wrote:  
> > > Add support for selecting the data format within the AXI ADC ip.
> > >
> > > Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > > ---
> > > no changes in v11.
> > >  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),  
> > >  };  
> > 
> > Why was [1] not addressed?
> > 
> > [1]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> > iio/9c262f599fb9b42feac99cfb541723a0a6f50e6b.camel@xxxxxxxxx/__;!!A
> > 3Ni8CS0y2Y!6uVytAwWUCsEazOUTACecMQkbMuHBF95sbla50CbTUFkZkyxS
> > -S7jMOCczpoyKCjtAKvMOyrt0ukYwcXC_l5q60$  
> 
> Indeed it was not addressed. I remained with the impression that adding part prefix
> in the macro definitions was enough. I will add the compatible string support.
> Although I have a question in order to minimize the number of versions to be sent
> In the future. Should I add a separate patch for the compatible support (which
> will not add value independently) or should I include it in this patch which adds
> custom function for data format for the AD485x IP core?

Binding docs update needs to be a separate patch.

Also, we should probably only set axi_adc_data_size_set in iio_backend_ops for
that ID.  So you'll need to pick from two copies of adi_axi_adc_ops
which probably means two iio_backend_info structures.
That data_size_set callback should not be set for cases that don't use it
(so the generic IP if I understand this correctly).

Similar to that part of:
https://lore.kernel.org/all/20250129-wip-bl-ad7606_add_backend_sw_mode-v3-7-c3aec77c0ab7@xxxxxxxxxxxx/

Hmm. This is looking like a messy merge.

Angelo, Antoniu,

Please figure out between you an order to the series so who is going to have
to rebase.  If this one goes first, may be worth pulling part of
patch 6 from Angelo's set to introduce struct axi_adc_info with what
this patch needs (just the backend_info pointer and maybe version?)

Thanks,

Jonathan