linux-kernel - Re: [PATCH v2 08/16] iio: adc: ad7768-1: Add reset gpio

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: <Z6DI0Wo0sEY-yqvh@debian-BULLSEYE-live-builder-AMD64>
Date: Mon, 3 Feb 2025 10:46:57 -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,
	Sergiu Cuciurean <sergiu.cuciurean@...log.com>, lars@...afoo.de,
	Michael.Hennerich@...log.com, marcelo.schmitt@...log.com,
	jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, jonath4nns@...il.com
Subject: Re: [PATCH v2 08/16] iio: adc: ad7768-1: Add reset gpio

On 01/27, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> 
> Depending on the controller, the default state of a gpio can vary. This
> change excludes the probability that the dafult state of the ADC reset
> gpio will be HIGH if it will be passed as reference in the devicetree.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> ---
> v2 Changes:
> * Replaced usleep_range() for fsleep() and gpiod_direction_output() for 
>   gpiod_set_value_cansleep().
> * Reset via SPI register is performed if the Reset GPIO is not defined. 
> ---
LGTM.
Aside from including your the SoB, one minor thing about reset timings.
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>

>  drivers/iio/adc/ad7768-1.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index fb8d6fae5f8a..17a49bf74637 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -163,6 +163,7 @@ struct ad7768_state {
>  	struct completion completion;
>  	struct iio_trigger *trig;
>  	struct gpio_desc *gpio_sync_in;
> +	struct gpio_desc *gpio_reset;
>  	const char *labels[ARRAY_SIZE(ad7768_channels)];
>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the
> @@ -453,19 +454,30 @@ static int ad7768_setup(struct ad7768_state *st)
>  {
>  	int ret;
>  
> -	/*
> -	 * Two writes to the SPI_RESET[1:0] bits are required to initiate
> -	 * a software reset. The bits must first be set to 11, and then
> -	 * to 10. When the sequence is detected, the reset occurs.
> -	 * See the datasheet, page 70.
> -	 */
> -	ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x3);
> -	if (ret)
> -		return ret;
> +	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
> +						 GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->gpio_reset))
> +		return PTR_ERR(st->gpio_reset);
>  
> -	ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x2);
> -	if (ret)
> -		return ret;
> +	if (st->gpio_reset) {
> +		fsleep(10);
> +		gpiod_set_value_cansleep(st->gpio_reset, 0);
> +		fsleep(10);
Is 10 us enough time here? AD7768-1 datasheet page 58 sais
"The time taken from RESET to an SPI write must be at least 200 μs."

> +	} else {
> +		/*
> +		 * Two writes to the SPI_RESET[1:0] bits are required to initiate
> +		 * a software reset. The bits must first be set to 11, and then
> +		 * to 10. When the sequence is detected, the reset occurs.
> +		 * See the datasheet, page 70.
> +		 */
> +		ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x3);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x2);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
>  					  GPIOD_OUT_LOW);
> -- 
> 2.34.1
>