Received-SPF: pass (google.com: domain of linux-kernel+bounces-105282-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Sat, 16 Mar 2024 21:57:03 +0200
To: David Lechner <dlechner@baylibre.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	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
Subject: Re: [PATCH v2] iio: adc: ad7944: Add support for "3-wire mode"
Message-ID: <ZfX5jynjW4M9pvw1@surfacebook.localdomain>
References: <20240314-mainline-ad7944-3-wire-mode-v2-1-d469da0705d2@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240314-mainline-ad7944-3-wire-mode-v2-1-d469da0705d2@baylibre.com>

Thu, Mar 14, 2024 at 12:43:38PM -0500, David Lechner kirjoitti:
> 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.)
> 
> 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.
> 
> The turbo/chain mode compatibility check at the end of the probe
> function is technically can't be triggered right now but adding it now
> anyway so that we don't forget to add it later when support for
> daisy-chaining is added.

..

> +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,

Why not kernel doc?

> +};

..

>  struct ad7944_adc {
>  	struct spi_device *spi;
> +	enum ad7944_spi_mode spi_mode;
>  	/* Chip-specific timing specifications. */
>  	const struct ad7944_timing_spec *timing_spec;
>  	/* GPIO connected to CNV pin. */
> @@ -58,6 +75,9 @@ struct ad7944_adc {
>  	 } sample __aligned(IIO_DMA_MINALIGN);
>  };

Have you run `pahole` to see if there is a better place for a new member?

..

> +/*

The below is mimicing the kernel doc, but there is no marker for this.
Why?

> + * ad7944_3wire_cs_mode_conversion - Perform a 3-wire CS mode conversion and
> + *                                   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 controller.
> + *
> + * Upon successful return adc->sample.raw will contain the conversion result.
> + */

..

> +	struct spi_transfer xfers[] = {
> +		{
> +			/*
> +			 * NB: can get better performance from some SPI
> +			 * controllers if we use the same bits_per_word
> +			 * in every transfer.

I believe you may reformat this to reduce by 1 line.

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

This also seems narrow.

> +			 */
> +			.delay = {
> +				.value = T_QUIET_NS,
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +
> +		},
> +		{
> +			.bits_per_word = chan->scan_type.realbits,
> +			/*
> +			 * CS has to be high for full conversion time to avoid
> +			 * triggering the busy indication.
> +			 */
> +			.cs_off = 1,
> +			.delay = {
> +				.value = t_conv_ns,
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +		},
> +		{
> +			/* Then we can read the data during the acquisition phase */
> +			.rx_buf = &adc->sample.raw,
> +			.len = BITS_TO_BYTES(chan->scan_type.storagebits),
> +			.bits_per_word = chan->scan_type.realbits,
> +		},
> +	};

..

> +	case AD7944_SPI_MODE_SINGLE:
> +		ret = ad7944_3wire_cs_mode_conversion(adc, &indio_dev->channels[0]);
> +		if (ret)
> +			goto out;
> +
> +		break;
> +	default:
> +		/* not supported */

No error code set?

>  		goto out;
> +	}

..

> +	if (device_property_read_string(dev, "adi,spi-mode", &str_val) == 0) {
> +		ret = sysfs_match_string(ad7944_spi_modes, str_val);

Don't you want use new fwnode/device property API for making these two in
one go?

> +		if (ret < 0)
> +			return dev_err_probe(dev, -EINVAL,

Why shadowing the error code?

> +					     "unsupported adi,spi-mode\n");
> +
> +		adc->spi_mode = ret;
> +	} else {
> +		/* absence of adi,spi-mode property means default mode */
> +		adc->spi_mode = AD7944_SPI_MODE_DEFAULT;
> +	}

-- 
With Best Regards,
Andy Shevchenko