Received-SPF: pass (google.com: domain of linux-kernel+bounces-99990-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <8ee551edeff9c4c959a4dbda53d1a2a26a9bb62c.camel@gmail.com>
Subject: Re: [PATCH] iio: adc: ad7944: Add support for "3-wire mode"
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>, Jonathan Cameron
 <jic23@kernel.org>
Cc: Michael Hennerich <michael.hennerich@analog.com>, Nuno
 =?ISO-8859-1?Q?S=E1?= <nuno.sa@analog.com>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
Date: Tue, 12 Mar 2024 10:08:35 +0100
In-Reply-To: <20240311-mainline-ad7944-3-wire-mode-v1-1-8e8199efa1f7@baylibre.com>
References: 
	<20240311-mainline-ad7944-3-wire-mode-v1-1-8e8199efa1f7@baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-03-11 at 16:26 -0500, David Lechner wrote:
> This adds support for AD7944 ADCs wired in "3-wire mode". (NOTE: 3-wire
> is the datasheet name for this wiring configuration and has nothing to
> do with SPI_3WIRE.)
>=20
> In the 3-wire mode, the SPI controller CS line can be wired to the CNV
> line on the ADC and used to trigger conversions rather that using a
> separate GPIO line.
>=20
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

Just some minor stuff from me... With that addressed (at least the 'else if=
' stuff):

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

> =C2=A0drivers/iio/adc/ad7944.c | 157 ++++++++++++++++++++++++++++++++++++=
+++++------
> =C2=A01 file changed, 139 insertions(+), 18 deletions(-)
>=20
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index adb007cdd287..8f466eb66bc4 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -32,8 +32,25 @@ struct ad7944_timing_spec {
> =C2=A0	unsigned int turbo_conv_ns;
> =C2=A0};
> =C2=A0
> +enum ad7944_spi_mode {
> +	/* datasheet calls this "4-wire mode" */
> +	AD7944_SPI_MODE_DEFAULT,
> +	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> +	AD7944_SPI_MODE_SINGLE,
> +	/* datasheet calls this "chain mode" */
> +	AD7944_SPI_MODE_CHAIN,
> +};
> +
> +/* maps adi,spi-mode property value to enum */
> +static const char * const ad7944_spi_modes[] =3D {
> +	[AD7944_SPI_MODE_DEFAULT] =3D "",
> +	[AD7944_SPI_MODE_SINGLE] =3D "single",
> +	[AD7944_SPI_MODE_CHAIN] =3D "chain",
> +};
> +
> =C2=A0struct ad7944_adc {
> =C2=A0	struct spi_device *spi;
> +	enum ad7944_spi_mode spi_mode;
> =C2=A0	/* Chip-specific timing specifications. */
> =C2=A0	const struct ad7944_timing_spec *timing_spec;
> =C2=A0	/* GPIO connected to CNV pin. */
> @@ -58,6 +75,9 @@ struct ad7944_adc {
> =C2=A0	 } sample __aligned(IIO_DMA_MINALIGN);
> =C2=A0};
> =C2=A0
> +/* quite time before CNV rising edge */
> +#define T_QUIET_NS	20
> +
> =C2=A0static const struct ad7944_timing_spec ad7944_timing_spec =3D {
> =C2=A0	.conv_ns =3D 420,
> =C2=A0	.turbo_conv_ns =3D 320,
> @@ -110,6 +130,65 @@ AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 0);
> =C2=A0/* fully differential */
> =C2=A0AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 1);
> =C2=A0
> +/*
> + * ad7944_3wire_cs_mode_conversion - Perform a 3-wire CS mode conversion=
 and
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 acquisition
> + * @adc: The ADC device structure
> + * @chan: The channel specification
> + * Return: 0 on success, a negative error code on failure
> + *
> + * This performs a conversion and reads data when the chip is wired in 3=
-wire
> + * mode with the CNV line on the ADC tied to the CS line on the SPI cont=
roller.
> + *
> + * Upon successful return adc->sample.raw will contain the conversion re=
sult.
> + */
> +static int ad7944_3wire_cs_mode_conversion(struct ad7944_adc *adc,
> +					=C2=A0=C2=A0 const struct iio_chan_spec *chan)
> +{
> +	unsigned int t_conv_ns =3D adc->always_turbo ? adc->timing_spec-
> >turbo_conv_ns
> +						=C2=A0=C2=A0 : adc->timing_spec->conv_ns;
> +	struct spi_transfer xfers[] =3D {
> +		{
> +			/*
> +			 * NB: can get better performance from some SPI
> +			 * controllers if we use the same bits_per_word
> +			 * in every transfer.
> +			 */
> +			.bits_per_word =3D chan->scan_type.realbits,
> +			/*
> +			 * CS is tied to CNV and we need a low to high
> +			 * transition to start the conversion, so place CNV
> +			 * low for t_QUIET to prepare for this.
> +			 */
> +			.delay =3D {
> +				.value =3D T_QUIET_NS,
> +				.unit =3D SPI_DELAY_UNIT_NSECS,
> +			},
> +
> +		},
> +		{
> +			.bits_per_word =3D chan->scan_type.realbits,
> +			/*
> +			 * CS has to be high for full conversion time to avoid
> +			 * triggering the busy indication.
> +			 */
> +			.cs_off =3D 1,
> +			.delay =3D {
> +				.value =3D t_conv_ns,
> +				.unit =3D SPI_DELAY_UNIT_NSECS,
> +			},
> +		},
> +		{
> +			/* Then we can read the data during the acquisition phase
> */
> +			.rx_buf =3D &adc->sample.raw,
> +			.len =3D BITS_TO_BYTES(chan->scan_type.storagebits),
> +			.bits_per_word =3D chan->scan_type.realbits,
> +		},
> +	};
> +
> +	return spi_sync_transfer(adc->spi, xfers, ARRAY_SIZE(xfers));
> +}
> +
> =C2=A0/*
> =C2=A0 * ad7944_4wire_mode_conversion - Perform a 4-wire mode conversion =
and acquisition
> =C2=A0 * @adc: The ADC device structure
> @@ -167,9 +246,22 @@ static int ad7944_single_conversion(struct ad7944_ad=
c *adc,
> =C2=A0{
> =C2=A0	int ret;
> =C2=A0
> -	ret =3D ad7944_4wire_mode_conversion(adc, chan);
> -	if (ret)
> -		return ret;
> +	switch (adc->spi_mode) {
> +	case AD7944_SPI_MODE_DEFAULT:
> +		ret =3D ad7944_4wire_mode_conversion(adc, chan);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case AD7944_SPI_MODE_SINGLE:
> +		ret =3D ad7944_3wire_cs_mode_conversion(adc, chan);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case AD7944_SPI_MODE_CHAIN:
> +		return -EOPNOTSUPP;

This mode is not really supported for now and in theory we can't really hav=
e
adc->spi_mode =3D AD7944_SPI_MODE_CHAIN, right? So, I would just make this =
the
'default' branch and not care about chain mode (implementing it when adding=
 it).

> +	}
> =C2=A0
> =C2=A0	if (chan->scan_type.storagebits > 16)
> =C2=A0		*val =3D adc->sample.raw.u32;
> @@ -230,9 +322,23 @@ static irqreturn_t ad7944_trigger_handler(int irq, v=
oid *p)
> =C2=A0	struct ad7944_adc *adc =3D iio_priv(indio_dev);
> =C2=A0	int ret;
> =C2=A0
> -	ret =3D ad7944_4wire_mode_conversion(adc, &indio_dev->channels[0]);
> -	if (ret)
> +	switch (adc->spi_mode) {
> +	case AD7944_SPI_MODE_DEFAULT:
> +		ret =3D ad7944_4wire_mode_conversion(adc, &indio_dev->channels[0]);
> +		if (ret)
> +			goto out;
> +
> +		break;
> +	case AD7944_SPI_MODE_SINGLE:
> +		ret =3D ad7944_3wire_cs_mode_conversion(adc, &indio_dev-
> >channels[0]);
> +		if (ret)
> +			goto out;
> +
> +		break;
> +	case AD7944_SPI_MODE_CHAIN:
> +		/* not supported */
> =C2=A0		goto out;
> +	}
> =C2=A0
> =C2=A0	iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
> =C2=A0					=C2=A0=C2=A0 pf->timestamp);
> @@ -260,16 +366,9 @@ static int ad7944_probe(struct spi_device *spi)
> =C2=A0	struct ad7944_adc *adc;
> =C2=A0	bool have_refin =3D false;
> =C2=A0	struct regulator *ref;
> +	const char *str_val;
> =C2=A0	int ret;
> =C2=A0
> -	/*
> -	 * driver currently only supports the conventional "4-wire" mode and
> -	 * not other special wiring configurations.
> -	 */
> -	if (device_property_present(dev, "adi,spi-mode"))
> -		return dev_err_probe(dev, -EINVAL,
> -				=C2=A0=C2=A0=C2=A0=C2=A0 "adi,spi-mode is not currently supported\n"=
);
> -
> =C2=A0	indio_dev =3D devm_iio_device_alloc(dev, sizeof(*adc));
> =C2=A0	if (!indio_dev)
> =C2=A0		return -ENOMEM;
> @@ -283,6 +382,22 @@ static int ad7944_probe(struct spi_device *spi)
> =C2=A0
> =C2=A0	adc->timing_spec =3D chip_info->timing_spec;
> =C2=A0
> +	if (device_property_read_string(dev, "adi,spi-mode", &str_val) =3D=3D 0=
) {
> +		ret =3D sysfs_match_string(ad7944_spi_modes, str_val);
> +		if (ret < 0)
> +			return dev_err_probe(dev, -EINVAL,
> +					=C2=A0=C2=A0=C2=A0=C2=A0 "unsupported adi,spi-mode\n");
> +
> +		adc->spi_mode =3D ret;
> +	} else {
> +		/* absence of adi,spi-mode property means default mode */
> +		adc->spi_mode =3D AD7944_SPI_MODE_DEFAULT;
> +	}
> +
> +	if (adc->spi_mode =3D=3D AD7944_SPI_MODE_CHAIN)
> +		return dev_err_probe(dev, -EINVAL,
> +				=C2=A0=C2=A0=C2=A0=C2=A0 "chain mode is not implemented\n");
> +
> =C2=A0	/*
> =C2=A0	 * Some chips use unusual word sizes, so check now instead of wait=
ing
> =C2=A0	 * for the first xfer.
> @@ -349,15 +464,17 @@ static int ad7944_probe(struct spi_device *spi)
> =C2=A0		adc->ref_mv =3D AD7944_INTERNAL_REF_MV;
> =C2=A0	}
> =C2=A0
> -	/*
> -	 * CNV gpio is required in 4-wire mode which is the only currently
> -	 * supported mode.
> -	 */
> -	adc->cnv =3D devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> +	adc->cnv =3D devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> =C2=A0	if (IS_ERR(adc->cnv))
> =C2=A0		return dev_err_probe(dev, PTR_ERR(adc->cnv),
> =C2=A0				=C2=A0=C2=A0=C2=A0=C2=A0 "failed to get CNV GPIO\n");
> =C2=A0
> +	if (!adc->cnv && adc->spi_mode =3D=3D AD7944_SPI_MODE_DEFAULT)
> +		return dev_err_probe(&spi->dev, -EINVAL, "CNV GPIO is
> required\n");
> +	else if (adc->cnv && adc->spi_mode !=3D AD7944_SPI_MODE_DEFAULT)
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				=C2=A0=C2=A0=C2=A0=C2=A0 "CNV GPIO in single and chain mode is not
> currently supported\n");
> +

Redundant else...

> =C2=A0	adc->turbo =3D devm_gpiod_get_optional(dev, "turbo", GPIOD_OUT_LOW=
);
> =C2=A0	if (IS_ERR(adc->turbo))
> =C2=A0		return dev_err_probe(dev, PTR_ERR(adc->turbo),
> @@ -369,6 +486,10 @@ static int ad7944_probe(struct spi_device *spi)
> =C2=A0		return dev_err_probe(dev, -EINVAL,
> =C2=A0			"cannot have both turbo-gpios and adi,always-turbo\n");
> =C2=A0
> +	if (adc->spi_mode =3D=3D AD7944_SPI_MODE_CHAIN && adc->always_turbo)
> +		return dev_err_probe(dev, -EINVAL,
> +			"cannot have both chain mode and always turbo\n");
> +


I'm fine in having this now but shouldn't we only have the above when we do=
 support
chain mode? A bit odd having it when we don't even allow chain mode.

- Nuno S=C3=A1