[PATCH 0/2] iio: adc: ad_sigma_delta: misc fixes/improvements

[PATCH 0/2] iio: adc: ad_sigma_delta: misc fixes/improvements

[Nuno Sa]

The first patch is for ensuring proper alignment for the sigma_delta
buffers so they can be safely used in DMA context. I'm adding a fixes tag
on it but I'm not sure if there's any urgency in backporting it so up to
you Jonathan :).

The second patch is about allowing the IRQ trigger type to be specified
in firmware.

The sigma delta ADCs have a fun history around this as they initially
forced the IRQF_TRIGGER_LOW flag and then changed to IRQF_TRIGGER_FALLING
as that is what is stated in the datasheet. However, as seen in [1], in
some platforms we can verify that IRQF_TRIGGER_LOW is the one flag that
makes things work.

Apart from the above, and as seen in similar patches, respecting the
firmware configuration is what makes sense. 

[1]: https://lore.kernel.org/all/CAPJMGm4GaSjD6bdqMwCr2EVZGenWzT-nCCf3BMRaD1TSfAabpA@mail.gmail.com/

---
Nuno Sa (2):
      iio: adc: ad_sigma_delta: ensure proper DMA alignment
      iio: adc: ad_sigma_delta: allow overwriting the IRQ flags

 drivers/iio/adc/ad_sigma_delta.c       | 7 ++++++-
 include/linux/iio/adc/ad_sigma_delta.h | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)
---
base-commit: 801590b27bfbdb6721f85e2c3af70e627e52c8d5
change-id: 20240115-dev_sigma_delta_no_irq_flags-2425795e2a00
--

Thanks!
- Nuno Sá

[PATCH 1/2] iio: adc: ad_sigma_delta: ensure proper DMA alignment

[Nuno Sa]

Aligning the buffer to the L1 cache is not sufficient in some platforms
as they might have larger cacheline sizes for caches after L1 and thus,
we can't guarantee DMA safety.

That was the whole reason to introduce IIO_DMA_MINALIGN in [1]. Do the same
for the sigma_delta ADCs.

[1]: https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/
Fixes: 0fb6ee8d0b5e ("iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/adc/ad_sigma_delta.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 7852f6c9a714..719cf9cc6e1a 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -8,6 +8,8 @@
 #ifndef __AD_SIGMA_DELTA_H__
 #define __AD_SIGMA_DELTA_H__
 
+#include <linux/iio/iio.h>
+
 enum ad_sigma_delta_mode {
 	AD_SD_MODE_CONTINUOUS = 0,
 	AD_SD_MODE_SINGLE = 1,
@@ -99,7 +101,7 @@ struct ad_sigma_delta {
 	 * 'rx_buf' is up to 32 bits per sample + 64 bit timestamp,
 	 * rounded to 16 bytes to take into account padding.
 	 */
-	uint8_t				tx_buf[4] ____cacheline_aligned;
+	uint8_t				tx_buf[4] __aligned(IIO_DMA_MINALIGN);
 	uint8_t				rx_buf[16] __aligned(8);
 };
 

-- 
2.43.0

Re: [PATCH 1/2] iio: adc: ad_sigma_delta: ensure proper DMA alignment

[Jonathan Cameron]

On Wed, 17 Jan 2024 13:41:03 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Aligning the buffer to the L1 cache is not sufficient in some platforms
> as they might have larger cacheline sizes for caches after L1 and thus,
> we can't guarantee DMA safety.
> 
> That was the whole reason to introduce IIO_DMA_MINALIGN in [1]. Do the same
> for the sigma_delta ADCs.
> 
> [1]: https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/
> Fixes: 0fb6ee8d0b5e ("iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  include/linux/iio/adc/ad_sigma_delta.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 7852f6c9a714..719cf9cc6e1a 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -8,6 +8,8 @@
>  #ifndef __AD_SIGMA_DELTA_H__
>  #define __AD_SIGMA_DELTA_H__
>  
> +#include <linux/iio/iio.h>
> +
>  enum ad_sigma_delta_mode {
>  	AD_SD_MODE_CONTINUOUS = 0,
>  	AD_SD_MODE_SINGLE = 1,
> @@ -99,7 +101,7 @@ struct ad_sigma_delta {
>  	 * 'rx_buf' is up to 32 bits per sample + 64 bit timestamp,
>  	 * rounded to 16 bytes to take into account padding.
>  	 */
> -	uint8_t				tx_buf[4] ____cacheline_aligned;
> +	uint8_t				tx_buf[4] __aligned(IIO_DMA_MINALIGN);
>  	uint8_t				rx_buf[16] __aligned(8);
>  };
>  
> 

[PATCH 2/2] iio: adc: ad_sigma_delta: allow overwriting the IRQ flags

[Nuno Sa]

Make sure we can specify the IRQ trigger type from firmware and drivers
won't ignore it. In fact, this how it should be done but since someone
might be already depending on the driver to hardcode the trigger type
(and not specifying it in firmware), let's do it like this so there's
no possible breakage.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 7e2192870743..fbba3f4a1189 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -568,6 +568,7 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
 static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev)
 {
 	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+	unsigned long irq_flags = irq_get_trigger_type(sigma_delta->spi->irq);
 	int ret;
 
 	if (dev != &sigma_delta->spi->dev) {
@@ -588,9 +589,13 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
 	/* the IRQ core clears IRQ_DISABLE_UNLAZY flag when freeing an IRQ */
 	irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
 
+	/* Allow overwriting the flags from firmware */
+	if (!irq_flags)
+		irq_flags = sigma_delta->info->irq_flags;
+
 	ret = devm_request_irq(dev, sigma_delta->spi->irq,
 			       ad_sd_data_rdy_trig_poll,
-			       sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
+			       irq_flags | IRQF_NO_AUTOEN,
 			       indio_dev->name,
 			       sigma_delta);
 	if (ret)

-- 
2.43.0

Re: [PATCH 2/2] iio: adc: ad_sigma_delta: allow overwriting the IRQ flags

[Jonathan Cameron]

On Wed, 17 Jan 2024 13:41:04 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Make sure we can specify the IRQ trigger type from firmware and drivers
> won't ignore it. In fact, this how it should be done but since someone
> might be already depending on the driver to hardcode the trigger type
> (and not specifying it in firmware), let's do it like this so there's
> no possible breakage.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
I'm going to treat this as adding increased flexibility rather than a fix. Hence
Applied to the togreg branch of iio.git and pushed out as testing.
Will be rebased on rc1 once available.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad_sigma_delta.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 7e2192870743..fbba3f4a1189 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -568,6 +568,7 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
>  static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev)
>  {
>  	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +	unsigned long irq_flags = irq_get_trigger_type(sigma_delta->spi->irq);
>  	int ret;
>  
>  	if (dev != &sigma_delta->spi->dev) {
> @@ -588,9 +589,13 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
>  	/* the IRQ core clears IRQ_DISABLE_UNLAZY flag when freeing an IRQ */
>  	irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
>  
> +	/* Allow overwriting the flags from firmware */
> +	if (!irq_flags)
> +		irq_flags = sigma_delta->info->irq_flags;
> +
>  	ret = devm_request_irq(dev, sigma_delta->spi->irq,
>  			       ad_sd_data_rdy_trig_poll,
> -			       sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> +			       irq_flags | IRQF_NO_AUTOEN,
>  			       indio_dev->name,
>  			       sigma_delta);
>  	if (ret)
>