linux-kernel - Re: [PATCH v3 2/3] iio: adc: ad4695: Add driver for AD4695 and similar ADCs

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: <14695107-a119-4f68-b55a-509cbcf8a64a@baylibre.com>
Date: Thu, 11 Jul 2024 11:11:33 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, Ramona Gradinariu <ramona.gradinariu@...log.com>
Subject: Re: [PATCH v3 2/3] iio: adc: ad4695: Add driver for AD4695 and
 similar ADCs

On 6/29/24 2:20 PM, Jonathan Cameron wrote:
> On Mon, 24 Jun 2024 17:01:54 -0500
> David Lechner <dlechner@...libre.com> wrote:
> 

...

>> +
>> +/**
>> + * ad4695_read_one_sample - Read a single sample using single-cycle mode
>> + * @st: The AD4695 state
>> + * @address: The address of the channel to read
>> + *
>> + * Upon return, the sample will be stored in the raw_data field of @st.
>> + *
>> + * Context: can sleep, must be called with iio_device_claim_direct held
>> + * Return: 0 on success, a negative error code on failure
>> + */
>> +static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
>> +{
>> +	struct spi_transfer xfer[2] = { };
>> +	int ret;
>> +
>> +	ret = ad4695_set_single_cycle_mode(st, address);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Setting the first channel to the temperature channel isn't supported
>> +	 * in single-cycle mode, so we have to do an extra xfer to read the
>> +	 * temperature.
>> +	 */
>> +	if (address == AD4695_CMD_TEMP_CHAN) {
>> +		/* We aren't reading, so we can make this a short xfer. */
> I'd be tempted to let the compiler figure out it can combine storage for xfer and
> do something like
> 		struct spi_transfer xfer[2] = {
> 			{
> 				.bits_per_word = 8,
> 				.tx_buf = ...
> 
> 			}, {
> 			},
> 		};
> 
> 		st->cnv_cmd2 = ...
> etc
> 
> Advantage is that it is clearly structured data.   Up to you though to
> decide if this is worth doing. I don't care that much!
> 
> 
>> +		st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3;
>> +		xfer[0].bits_per_word = 8;
>> +		xfer[0].tx_buf = &st->cnv_cmd2;
>> +		xfer[0].len = 1;
>> +		xfer[0].cs_change = 1;
>> +		xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS;
>> +		xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +
>> +		/* Then read the result and exit conversion mode. */
>> +		st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
>> +		xfer[1].bits_per_word = 16;
>> +		xfer[1].tx_buf = &st->cnv_cmd;
>> +		xfer[1].rx_buf = &st->raw_data;
>> +		xfer[1].len = 2;
>> +
>> +		return spi_sync_transfer(st->spi, xfer, 2);
>> +	}
> 
> then an else here to reduce the scope of another xfer structure.

Tempting, but then I risk the complaint of else after return. :-)

I also realized that the second xfer above is the same as the one
below, so could skip the return here and avoid some duplicated code
(just need to add an index variable instead of hard-coding xfer[0]).

> 
>> +
>> +	/*
>> +	 * The conversion has already been done and we just have to read the
>> +	 * result and exit conversion mode.
>> +	 */
>> +	st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
>> +	xfer[0].bits_per_word = 16;
>> +	xfer[0].tx_buf = &st->cnv_cmd;
>> +	xfer[0].rx_buf = &st->raw_data;
>> +	xfer[0].len = 2;
>> +
>> +	return spi_sync_transfer(st->spi, xfer, 1);
>> +}