Message ID | 20241221155926.81954-2-alisa.roman@analog.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | Add support for AD7191 | expand |
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,
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.
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.
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 --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,
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(+)