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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <CY4PR03MB3399749EAF99EB164A96CBD09B3C2@CY4PR03MB3399.namprd03.prod.outlook.com>
Date: Mon, 9 Dec 2024 13:58:57 +0000
From: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
To: David Lechner <dlechner@...libre.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>,
        "linux-pwm@...r.kernel.org"
	<linux-pwm@...r.kernel.org>
Subject: RE: [PATCH v7 5/8] iio: adc: adi-axi-adc: set data format



--
Antoniu Miclăuş

> -----Original Message-----
> From: David Lechner <dlechner@...libre.com>
> Sent: Thursday, December 5, 2024 2:46 AM
> 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; linux-
> pwm@...r.kernel.org
> 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@...log.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.

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@...libre.com/__;!!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@...libre.com/__;!!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),
> >  };