Message-ID: <Z5Dj-jiLuuBp8KhN@spiri>
Date: Wed, 22 Jan 2025 14:26:34 +0200
From: Alisa-Dariana Roman <alisadariana@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	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 Tue, Jan 21, 2025 at 04:32:56PM -0600, David Lechner wrote:
> 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.
> 

Hello, David! I really appreciate your suggestions! Things look a lot
clearer.

Regarding point 2) I looked a bit further into the read function and the
ad_sd_read_reg_raw() function seems to handle no register components as
you suggested:

...
			.cs_change = sigma_delta->bus_locked,
...
	if (sigma_delta->info->has_registers) {
		data[0] = reg << sigma_delta->info->addr_shift;
		data[0] |= sigma_delta->info->read_mask;
		data[0] |= sigma_delta->comm;
		spi_message_add_tail(&t[0], &m);
	}
	spi_message_add_tail(&t[1], &m);
...

So I will handle the AD_SD_MODE_SINGLE and AD_SD_MODE_IDLE cases in the
callback function by doing empty SPI xfers as you said. The bus seems to
be already locked before all the ad_sigma_delta_set_mode() and unlocked
after, so I think I can skip this part.

I will follow with the second version as soon as possible if this setup
looks alright!

---

I initially used a GPIO for the powerdown pin, but the first interrupt
was somehow always getting lost.

Kind regards,
Alisa-Dariana Roman.

Powered by blists - more mailing lists