linux-kernel - Re: [PATCH v2 6/9] iio: dac: ad3552r-hs: use instruction mode for configuration

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: <80fa8faf-b5ac-4f3e-84b9-acf8ac6ab26d@baylibre.com>
Date: Wed, 8 Jan 2025 15:16:21 -0600
From: David Lechner <dlechner@...libre.com>
To: Angelo Dureghello <adureghello@...libre.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>,
 Mihail Chindris <mihail.chindris@...log.com>, Nuno Sa <nuno.sa@...log.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/9] iio: dac: ad3552r-hs: use instruction mode for
 configuration

On 1/8/25 11:29 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@...libre.com>
> 
> Use "instruction" mode over initial configuration and all other
> non-streaming operations.
> 
> DAC boots in streaming mode as default, and the driver is not
> changing this mode.
> 
> Instruction r/w is still working becouse instruction is processed

s/becouse/because/

> from the DAC after chip select is deasserted, this works until
> loop mode is 0 or greater than the instruction size.
> 
> All initial operations should be more safely done in instruction
> mode, a mode provided for this.

I'm not sure it is really "safer". The way I read the datasheet, this just
enables bulk reads of multiple registers. So unless we need to do bulk reads
it seems like this is just adding extra complexity to the driver without a
tangible benefit.

> 
> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> ---
>  drivers/iio/dac/ad3552r-hs.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 27949f207d42..991b11702273 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -132,6 +132,13 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev)
>  		return -EINVAL;
>  	}
>  
> +	/* Primary region access, set streaming mode (now in SPI + SDR). */
> +	ret = ad3552r_qspi_update_reg_bits(st,
> +					   AD3552R_REG_ADDR_INTERFACE_CONFIG_B,
> +					   AD3552R_MASK_SINGLE_INST, 0, 1);

Missing undoing this operation in the error path if a later operation in this
function fails?

> +	if (ret)
> +		return ret;
> +
>  	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE,
>  				      loop_len, 1);
>  	if (ret)