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

[David Lechner <dlechner@baylibre.com>]

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@analog.com>
> ---
> 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.

> +		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://lore.kernel.org/linux-iio/20241121-ad7606_add_iio_backend_software_mode-v1-2-8a693a5e3fa9@baylibre.com/
[2]: https://lore.kernel.org/linux-iio/20241121-ad7606_add_iio_backend_software_mode-v1-6-8a693a5e3fa9@baylibre.com/

>  	.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),
>  };