Message-ID: <d9828b7b-facb-4b42-aec2-427e68352a94@baylibre.com>
Date: Tue, 21 Jan 2025 16:32:56 -0600
From: David Lechner <dlechner@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>,
 Jonathan Cameron <jic23@...nel.org>
Cc: Alisa-Dariana Roman <alisa.roman@...log.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Uwe Kleine-König <ukleinek@...nel.org>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

On 1/21/25 3:36 AM, Alisa-Dariana Roman wrote:
> On Sun, Dec 22, 2024 at 06:07:13PM +0000, Jonathan Cameron wrote:
>> On Sat, 21 Dec 2024 17:56:00 +0200
>> Alisa-Dariana Roman <alisadariana@...il.com> wrote:
>>
>>> Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
>>> start conversion when CS is asserted. Add helper function to support
>>> this use case by allowing devices to assert CS without performing
>>> register operations.
>> Hi Alisa-Dariana,
>>
>> I had a look at the ad7191 datasheet. Given this description,
>> I was expecting to see it do a pre pulse of the chip select to trigger
>> the acquisition.  However, what I see is a power down line (which is more
>> or less a chip select) but it just has a specified t1 delay before the
>> DOUT will change to the state for the first bit and the host
>> can start driving the clock.
>>
>> That can be done by setting spi_device->cs_setup to whatever delay is
>> needed.  The text is spi_device docs are a little vague,
>> but I'd take it as t1 + t2 (maybe t3 to be safe).
>>
>> That is going to be more reliable than trying to hold the cs across
>> messages / spi_sync() calls, particularly if the bus might not be
>> locked (which the code below suggests).
>>
>> Jonathan
>>
>>
> 
> Hello Jonathan! I am grateful for your and everyone's feedback, as
> always!
> 
> I got a bit stuck on this part. The motivation for adding this function
> is as following:
> 
> int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
> 	const struct iio_chan_spec *chan, int *val)
> {
> 
> ...
> 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
> 
> 	ad_sd_enable_irq(sigma_delta);
> 	ret = wait_for_completion_interruptible_timeout(
> 			&sigma_delta->completion, HZ);
> ...
> }
> 
> I noticed that adc drivers need to call the ad_sd_write_reg function in
> their callback set_mode function, in order to keep the cs line pulled
> down before waiting for the interrupt (if I understand correctly). But
> since this component and AD7780 have no register I just copied the
> functionality of ad_sd_write_reg without actually writing anything.
> 
> Should I change the description/name to more accurately present the
> functionality? Or would it be a better idea to not use the single
> conversion function and write something from scratch leveraging the
> cs_setup?

If the RDY interrupt handling wasn't so tricky, I would suggest to just
make a separate function. But to avoid duplicating that tricky code I
think using the existing function would be best. I think you have mostly
the right idea here. Here is how I would try to do it...

1)

	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);

In the implementation of this callback, call spi_bus_lock(), then do
the SPI xfer with no data that has cs_change set so that CS does not
deassert (using spi_sync_locked() since we manually control the lock).

2)

This is the main part of your question, I think. In this part of the
function...

	if (sigma_delta->info->data_reg != 0)
		data_reg = sigma_delta->info->data_reg;
	else
		data_reg = AD_SD_REG_DATA;

	ret = ad_sd_read_reg(sigma_delta, data_reg,
		DIV_ROUND_UP(chan->scan_type.realbits + chan->scan_type.shift, 8),
		&raw_sample);

I would add a new flag or create a sentinel value for sigma_delta->info->data_reg
(e.g. #define AD_SD_NO_REG ~0U) that indicates that this chip doesn't have registers.

Then modify the if statement a bit so that if the chip has registers, call the
existing ad_sd_read_reg() function or if the chip doesn't have registers, call
a new function that reads one sample and has cs_change set on the last SPI xfer
so that CS still does not deassert.

This way, we don't have to mess with modifying ad_sd_read_reg() to not read
a register and avoid the naming issue. :-)

3)

	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);

In the callback for this function, do an empty SPI xfer so that CS finally
deasserts. Then call spi_bus_unlock() to release the lock that was taken
earlier.


---

Also, thinking outside the box, could we use a GPIO instead of connecting
SPI CS to the powerdown pin? The DT bindings already have a powerdown-gpios
binding for that. The could simplify things a bit.

With this, the set_mode callback would just be poking the GPIO instead of
dealing with the SPI CS line. But otherwise would be the same as above.

Powered by blists - more mailing lists