linux-kernel - Re: [PATCH v3 2/3] iio: adc: ad7625: add driver

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: <20240823194852.4d855eb3@jic23-huawei>
Date: Fri, 23 Aug 2024 19:48:52 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Trevor Gamblin <tgamblin@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, David
 Lechner <dlechner@...libre.com>, Uwe Kleine-Konig
 <u.kleine-koenig@...libre.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 2/3] iio: adc: ad7625: add driver

On Mon, 19 Aug 2024 10:11:44 -0400
Trevor Gamblin <tgamblin@...libre.com> wrote:

> Add a driver for the AD762x and AD796x family of ADCs. These are
> pin-compatible devices using an LVDS interface for data transfer,
> capable of sampling at rates of 6 (AD7625), 10 (AD7626), and 5
> (AD7960/AD7961) MSPS, respectively. They also feature multiple voltage
> reference options based on the configuration of the EN1/EN0 pins, which
> can be set in the devicetree.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
Hi Trevor
A few really minor things in here.  Given we are waiting on the 
PWM set anyway, I haven't just tidied them up whilst applying.
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  15 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7625.c | 688 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 705 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2361f92751dd..a90972e1c5c5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1268,6 +1268,7 @@ S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> +F:	drivers/iio/adc/ad7625.c
>  
>  ANALOG DEVICES INC AD7768-1 DRIVER
>  M:	Michael Hennerich <Michael.Hennerich@...log.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f60fe85a30d5..e25fb505f545 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -219,6 +219,21 @@ config AD7606_IFACE_SPI
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7606_spi.
>  
> +config AD7625
> +        tristate "Analog Devices AD7625/AD7626 High Speed ADC driver"
> +        select IIO_BACKEND
> +        help
> +          Say yes here to build support for Analog Devices:
> +	  * AD7625 16-Bit, 6 MSPS PulSAR Analog-to-Digital Converter
Looks like some tabs mixed in with otherwise spaces.
Should be tabs + spaces as per entry above.
> +	  * AD7626 16-Bit, 10 MSPS PulSAR Analog-to-Digital Converter
> +	  * AD7960 18-Bit, 5 MSPS PulSAR Analog-to-Digital Converter
> +	  * AD7961 16-Bit, 5 MSPS PulSAR Analog-to-Digital Converter
> +
> +          The driver requires the assistance of the AXI ADC IP core to operate.
> +
> +          To compile this driver as a module, choose M here: the module will be
> +          called ad7625.
> +
>  config AD7766
>  	tristate "Analog Devices AD7766/AD7767 ADC driver"
>  	depends on SPI_MASTER

> diff --git a/drivers/iio/adc/ad7625.c b/drivers/iio/adc/ad7625.c
> new file mode 100644
> index 000000000000..3ac3c56d43eb
> --- /dev/null
> +++ b/drivers/iio/adc/ad7625.c
> @@ -0,0 +1,688 @@

> +
> +struct ad7625_chip_info {
> +	const char *name;
> +	const unsigned int max_sample_rate_hz;
> +	const struct ad7625_timing_spec *timing_spec;
> +	const struct iio_chan_spec chan_spec;
> +	const bool has_power_down_state;
> +	const bool has_bandwidth_control;
> +	const bool has_internal_vref;
Not sure I'd bother marking these bools const. Unlikely the compiler
can do anything with that info it can't do without it. I guess it does no
real harm thowever.
> +};


> +static int ad7625_probe(struct platform_device *pdev)
> +{

...

> +	/*
> +	 * Set the initial sampling frequency to the maximum, unless the
> +	 * AD796x device is limited to narrow bandwidth by EN2 == 1, in
> +	 * which case the sampling frequency should be limited to 2MSPS
> +	 */
> +	if (!st->info->has_bandwidth_control) {
> +		default_sample_freq = st->info->max_sample_rate_hz;
> +	} else {
> +		default_sample_freq = !st->can_wide_bandwidth ?

Flip the logic.
		default_sample_freq = st->can_wide_bandwidth ?
				      st->info->max_sample_rate_hz :
				      AD7960_MAX_NBW_FREQ;

seems simpler or set a default and override it.
	default_sample_freq = st->info_max_sample_rate_hz;
	if (st->info->has_bandwidth_control &&
	    !st->can_wide_bandwidth)
		default_sampling_freq = AD7960_MAX_NBW_FREQ;
		

> +				      AD7960_MAX_NBW_FREQ :
> +				      st->info->max_sample_rate_hz;
> +	}