linux-kernel - Re: [PATCH v6 07/11] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

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] [day] [month] [year] [list]

Message-ID: <aBTXCi0HINcrvXay@debian-BULLSEYE-live-builder-AMD64>
Date: Fri, 2 May 2025 11:30:34 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	andy@...nel.org, nuno.sa@...log.com, Michael.Hennerich@...log.com,
	marcelo.schmitt@...log.com, jic23@...nel.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, linus.walleij@...aro.org,
	brgl@...ev.pl, lgirdwood@...il.com, broonie@...nel.org,
	jonath4nns@...il.com, dlechner@...libre.com,
	David Lechner <dlechner@...lire.com>
Subject: Re: [PATCH v6 07/11] iio: adc: ad7768-1: add multiple scan types to
 support 16-bits mode

Hi Jonathan,

The new adjustable sample rate / precision patch looks good to me.
My only concern is about one error handling path in the trigger handler function.
With that sorted out
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>

On 04/27, Jonathan Santos wrote:
> When the device is configured to decimation x8, only possible in the
> sinc5 filter, output data is reduced to 16-bits in order to support
> 1 MHz of sampling frequency due to clock limitation.
> 
> Use multiple scan types feature to enable the driver to switch
> scan type in runtime, making possible to support both 24-bit and
> 16-bit resolution.
> 
> Reviewed-by: David Lechner <dlechner@...lire.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
> ---
...
> +enum ad7768_scan_type {
> +	AD7768_SCAN_TYPE_NORMAL,
> +	AD7768_SCAN_TYPE_HIGH_SPEED,
> +};
> +
> +static const int ad7768_mclk_div_rates[4] = {
I think we can omit the 4 constant here
static const int ad7768_mclk_div_rates[] = {

Probably not a reason for a v7 if the other parts of the series are good.
> +	16, 8, 4, 2,
> +};
> +
...
>  
> @@ -674,9 +722,15 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
>  	int ret;
>  
> -	ret = spi_read(st->spi, &st->data.scan.chan, 3);
> +	scan_type = iio_get_current_scan_type(indio_dev, &indio_dev->channels[0]);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);

The IRQ never gets handled if we get an error from iio_get_current_scan_type()?
Maybe make it jump to out?
	if (IS_ERR(scan_type))
		goto out;

> +
> +	ret = spi_read(st->spi, &st->data.scan.chan,
> +		       BITS_TO_BYTES(scan_type->realbits));
>  	if (ret < 0)
>  		goto out;
>  

Best regards,
Marcelo