diff mbox series

[v1,1/3] iio: adc: ad_sigma_delta: Add CS assert function

Message ID 20241221155926.81954-2-alisa.roman@analog.com (mailing list archive)
State Accepted
Headers show
Series Add support for AD7191 | expand

Commit Message

Alisa-Dariana Roman Dec. 21, 2024, 3:56 p.m. UTC
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.

This function can be used by drivers through their set_mode callback.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 24 ++++++++++++++++++++++++
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 25 insertions(+)

Comments

Jonathan Cameron Dec. 22, 2024, 6:07 p.m. UTC | #1
On Sat, 21 Dec 2024 17:56:00 +0200
Alisa-Dariana Roman <alisadariana@gmail.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


> 
> This function can be used by drivers through their set_mode callback.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 24 ++++++++++++++++++++++++
>  include/linux/iio/adc/ad_sigma_delta.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 0f355dac7813..c0f33d4baddf 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -48,6 +48,30 @@ void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm)
>  }
>  EXPORT_SYMBOL_NS_GPL(ad_sd_set_comm, "IIO_AD_SIGMA_DELTA");
>  
> +/**
> + * ad_sd_assert_cs() - Assert chip select line
> + *
> + * @sigma_delta: The sigma delta device
> + *
> + * Returns 0 on success, an error code otherwise.
> + **/
> +int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta)
> +{
> +	struct spi_transfer t = {
> +		.len = 0,
> +		.cs_change = sigma_delta->keep_cs_asserted,
> +	};
> +	struct spi_message m;
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);
> +
> +	if (sigma_delta->bus_locked)
> +		return spi_sync_locked(sigma_delta->spi, &m);
> +	return spi_sync(sigma_delta->spi, &m);
> +}
> +EXPORT_SYMBOL_NS_GPL(ad_sd_assert_cs, IIO_AD_SIGMA_DELTA);
> +
>  /**
>   * ad_sd_write_reg() - Write a register
>   *
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 417073c52380..99ab56d04793 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -178,6 +178,7 @@ static inline int ad_sigma_delta_postprocess_sample(struct ad_sigma_delta *sd,
>  }
>  
>  void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm);
> +int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta);
>  int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
>  	unsigned int size, unsigned int val);
>  int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
Alisa-Dariana Roman Jan. 21, 2025, 9:36 a.m. UTC | #2
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@gmail.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?

Thank you!

Kind regards,
Alisa-Dariana Roman.
David Lechner Jan. 21, 2025, 10:32 p.m. UTC | #3
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@gmail.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.
Alisa-Dariana Roman Jan. 22, 2025, 12:26 p.m. UTC | #4
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@gmail.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.
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 0f355dac7813..c0f33d4baddf 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -48,6 +48,30 @@  void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm)
 }
 EXPORT_SYMBOL_NS_GPL(ad_sd_set_comm, "IIO_AD_SIGMA_DELTA");
 
+/**
+ * ad_sd_assert_cs() - Assert chip select line
+ *
+ * @sigma_delta: The sigma delta device
+ *
+ * Returns 0 on success, an error code otherwise.
+ **/
+int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta)
+{
+	struct spi_transfer t = {
+		.len = 0,
+		.cs_change = sigma_delta->keep_cs_asserted,
+	};
+	struct spi_message m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+
+	if (sigma_delta->bus_locked)
+		return spi_sync_locked(sigma_delta->spi, &m);
+	return spi_sync(sigma_delta->spi, &m);
+}
+EXPORT_SYMBOL_NS_GPL(ad_sd_assert_cs, IIO_AD_SIGMA_DELTA);
+
 /**
  * ad_sd_write_reg() - Write a register
  *
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 417073c52380..99ab56d04793 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -178,6 +178,7 @@  static inline int ad_sigma_delta_postprocess_sample(struct ad_sigma_delta *sd,
 }
 
 void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm);
+int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta);
 int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
 	unsigned int size, unsigned int val);
 int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,