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]

Date: Sat, 29 Jun 2024 20:20:03 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
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 Mon, 24 Jun 2024 17:01:54 -0500
David Lechner <dlechner@...libre.com> wrote:

> This is a new driver for Analog Devices Inc. AD4695 and similar ADCs.
> The initial driver supports initializing the chip including configuring
> all possible LDO and reference voltage sources as well as any possible
> voltage input channel wiring configuration.
> 
> Only the 4-wire SPI wiring mode where the CNV pin is tied to the CS pin
> is supported at this time. And reading sample data from the ADC can only
> be done in direct mode for now.
> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Signed-off-by: David Lechner <dlechner@...libre.com>

Some minor bits an pieces from me.

> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> new file mode 100644
> index 000000000000..72ab5b997f6e
> --- /dev/null
> +++ b/drivers/iio/adc/ad4695.c
> @@ -0,0 +1,730 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SPI ADC driver for Analog Devices Inc. AD4695 and similar chips
> + *
> + * https://www.analog.com/en/products/ad4695.html
> + * https://www.analog.com/en/products/ad4696.html
> + * https://www.analog.com/en/products/ad4697.html
> + * https://www.analog.com/en/products/ad4698.html
> + *
> + * Copyright 2024 Analog Devices Inc.
> + * Copyright 2024 BayLibre, SAS
> + */
> +
> +#include <dt-bindings/iio/adi,ad4695.h>

Stick driver specific includes after linux ones.
Probably a blank line in between as well.

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/compiler.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +

> +
> +/**
> + * 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.

> +
> +	/*
> +	 * 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);
> +}