--
Antoniu Miclăuş
> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Thursday, December 5, 2024 2:46 AM
> To: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>; jic23@xxxxxxxxxx;
> robh@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> pwm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 5/8] iio: adc: adi-axi-adc: set data format
>
> [External]
>
> On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> > Add support for selecting the data format within the AXI ADC ip.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > ---
> > changes in v7:
> > - add back 16-bit case
> > drivers/iio/adc/adi-axi-adc.c | 31 +++++++++++++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> > index f6475bc93796..cb3b8299a65e 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_CUSTOM_CTRL_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,30 @@ 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) {
>
> This could use some explanation that there are two different variants of the
> AXI AD485X IP block, a 16-bit and a 20-bit variant. So 0x0
> (AD485X_PACKET_FORMAT_20BIT)
> is really 16-bit on 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;
> > + case 32:
> > + val = AD485X_PACKET_FORMAT_32BIT;
>
> AFAICT, technically 0x2 (AD485X_PACKET_FORMAT_32BIT) is not valid on
> the 16-bit variant of the IP block, so we should explain why it is
> safe to allow this instead of returning error in that case.
>
> Or we could solve both issues by just create two separate functions.
I will go for the first suggestion you made: to explain the different variants inline.
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return regmap_update_bits(st->regmap,
> ADI_AXI_ADC_REG_CNTRL_3,
> > +
> AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK, val);
>
> To be consistent, would be nice to use FIELD_PREP() with val.
>
> > +}
> > +
> > static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
> > struct iio_dev *indio_dev)
> > {
> > @@ -360,6 +390,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,
>
> As mentioned before, this callback is specifically for the AXI AD485X version
> of the IP block and doesn't apply to the generic base AXI ADC IP block.
>
> [1] and [2] are adding DT compatible and lookup table to handle a different
> AXI ADC variant. So we could build on top of that to add the variants for
> AXI AD485X. We could add two compatible strings, one for the 16-bit version
> and one for the 20-bit version which would allow us to have separate callback
> functions as suggested above.
>
> [1]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20241121-ad7606_add_iio_backend_software_mode-v1-2-
> 8a693a5e3fa9@xxxxxxxxxxxx/__;!!A3Ni8CS0y2Y!40xaV2kwv9GFnh1xLPGPw
> Gd4nFw7n6dzj4Vjo9JTHX1SAGN8A-VlWmgvVyn7Bq-
> WKlJ_dyFbOEqNja_vbqwHyCc$
> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20241121-ad7606_add_iio_backend_software_mode-v1-6-
> 8a693a5e3fa9@xxxxxxxxxxxx/__;!!A3Ni8CS0y2Y!40xaV2kwv9GFnh1xLPGPw
> Gd4nFw7n6dzj4Vjo9JTHX1SAGN8A-VlWmgvVyn7Bq-
> WKlJ_dyFbOEqNja_vidP9fsQ$
>
> > .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),
> > };
