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

[David Lechner <dlechner@baylibre.com>]

On 6/29/24 2:20 PM, Jonathan Cameron wrote:
> On Mon, 24 Jun 2024 17:01:54 -0500
> David Lechner <dlechner@baylibre.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);
>> +}