[PATCH 0/3] iio: imu: adis: misc fixes/improvements

[PATCH 0/3] iio: imu: adis: 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 the deal is the same as in the sigma_delta series.

The following patches are just about code simplification (no functional
change intended).
 

---
Nuno Sa (3):
      iio: imu: adis: ensure proper DMA alignment
      iio: imu: adis16475: make use of irq_get_trigger_type()
      iio: imu: adis16480: make use of irq_get_trigger_type()

 drivers/iio/imu/adis16475.c  | 8 +-------
 drivers/iio/imu/adis16480.c  | 9 +--------
 include/linux/iio/imu/adis.h | 3 ++-
 3 files changed, 4 insertions(+), 16 deletions(-)
---
base-commit: 801590b27bfbdb6721f85e2c3af70e627e52c8d5
change-id: 20240117-adis-improv-94f928683aba
--

Thanks!
- Nuno Sá

[PATCH 1/3] iio: imu: adis: 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: ccd2b52f4ac6 ("staging:iio: Add common ADIS library")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/imu/adis.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index dc9ea299e088..8898966bc0f0 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -11,6 +11,7 @@
 
 #include <linux/spi/spi.h>
 #include <linux/interrupt.h>
+#include <linux/iio/iio.h>
 #include <linux/iio/types.h>
 
 #define ADIS_WRITE_REG(reg) ((0x80 | (reg)))
@@ -131,7 +132,7 @@ struct adis {
 	unsigned long		irq_flag;
 	void			*buffer;
 
-	u8			tx[10] ____cacheline_aligned;
+	u8			tx[10] __aligned(IIO_DMA_MINALIGN);
 	u8			rx[4];
 };
 

-- 
2.43.0

Re: [PATCH 1/3] iio: imu: adis: ensure proper DMA alignment

[Jonathan Cameron]

On Wed, 17 Jan 2024 14:10:49 +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: ccd2b52f4ac6 ("staging:iio: Add common ADIS library")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Guess I didn't look in the main headers :(

Anyhow good to clean this straggler up.  I'll apply it to the fixes-togreg
branch of iio.git and mark it for stable.
I 'think' the definition of IIO_DMA_MINALIGN long got picked up by stable.

Jonathan

> ---
>  include/linux/iio/imu/adis.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index dc9ea299e088..8898966bc0f0 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -11,6 +11,7 @@
>  
>  #include <linux/spi/spi.h>
>  #include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
>  #include <linux/iio/types.h>
>  
>  #define ADIS_WRITE_REG(reg) ((0x80 | (reg)))
> @@ -131,7 +132,7 @@ struct adis {
>  	unsigned long		irq_flag;
>  	void			*buffer;
>  
> -	u8			tx[10] ____cacheline_aligned;
> +	u8			tx[10] __aligned(IIO_DMA_MINALIGN);
>  	u8			rx[4];
>  };
>  
> 

Re: [PATCH 1/3] iio: imu: adis: ensure proper DMA alignment

[Nuno Sá]

On Sun, 2024-01-21 at 16:12 +0000, Jonathan Cameron wrote:
> On Wed, 17 Jan 2024 14:10:49 +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: ccd2b52f4ac6 ("staging:iio: Add common ADIS library")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> Guess I didn't look in the main headers :(
> 

Not many users anyways. Doing a git grep shows:

git grep "____cacheline_aligned" include/linux/iio/

include/linux/iio/adc/ad_sigma_delta.h:102:     uint8_t                         tx_buf[4] ____cacheline_aligned;
include/linux/iio/common/st_sensors.h:261:      char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] ____cacheline_aligned;
include/linux/iio/imu/adis.h:134:       u8                      tx[10]____cacheline_aligned;

So we are only missing the st header. I can send patch for it later today.

- Nuno Sá

[PATCH 2/3] iio: imu: adis16475: make use of irq_get_trigger_type()

[Nuno Sa]

There's no need to call both irq_get_irq_data() and
irqd_get_trigger_type() as we already have an helper for that. This
allows for code simplification.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index b7cbe1565aee..7c5270f70b7b 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -1363,22 +1363,16 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
 static int adis16475_config_irq_pin(struct adis16475 *st)
 {
 	int ret;
-	struct irq_data *desc;
 	u32 irq_type;
 	u16 val = 0;
 	u8 polarity;
 	struct spi_device *spi = st->adis.spi;
 
-	desc = irq_get_irq_data(spi->irq);
-	if (!desc) {
-		dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq);
-		return -EINVAL;
-	}
 	/*
 	 * It is possible to configure the data ready polarity. Furthermore, we
 	 * need to update the adis struct if we want data ready as active low.
 	 */
-	irq_type = irqd_get_trigger_type(desc);
+	irq_type = irq_get_trigger_type(spi->irq);
 	if (irq_type == IRQ_TYPE_EDGE_RISING) {
 		polarity = 1;
 		st->adis.irq_flag = IRQF_TRIGGER_RISING;

-- 
2.43.0

[PATCH 3/3] iio: imu: adis16480: make use of irq_get_trigger_type()

[Nuno Sa]

There's no need to call both irq_get_irq_data() and
irqd_get_trigger_type() as we already have an helper for that. This
allows for code simplification.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16480.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index fe520194a837..b40a55bba30c 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -1246,18 +1246,11 @@ static int adis16480_config_irq_pin(struct adis16480 *st)
 {
 	struct device *dev = &st->adis.spi->dev;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	struct irq_data *desc;
 	enum adis16480_int_pin pin;
 	unsigned int irq_type;
 	uint16_t val;
 	int i, irq = 0;
 
-	desc = irq_get_irq_data(st->adis.spi->irq);
-	if (!desc) {
-		dev_err(dev, "Could not find IRQ %d\n", irq);
-		return -EINVAL;
-	}
-
 	/* Disable data ready since the default after reset is on */
 	val = ADIS16480_DRDY_EN(0);
 
@@ -1285,7 +1278,7 @@ static int adis16480_config_irq_pin(struct adis16480 *st)
 	 * configured as positive or negative, corresponding to
 	 * IRQ_TYPE_EDGE_RISING or IRQ_TYPE_EDGE_FALLING respectively.
 	 */
-	irq_type = irqd_get_trigger_type(desc);
+	irq_type = irq_get_trigger_type(st->adis.spi->irq);
 	if (irq_type == IRQ_TYPE_EDGE_RISING) { /* Default */
 		val |= ADIS16480_DRDY_POL(1);
 	} else if (irq_type == IRQ_TYPE_EDGE_FALLING) {

-- 
2.43.0

Re: [PATCH 3/3] iio: imu: adis16480: make use of irq_get_trigger_type()

[Jonathan Cameron]

On Wed, 17 Jan 2024 14:10:51 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> There's no need to call both irq_get_irq_data() and
> irqd_get_trigger_type() as we already have an helper for that. This
> allows for code simplification.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Applied 2 and 3 to the togreg branch of iio.git and pushed out as testing until
I can rebase it on rc1.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index fe520194a837..b40a55bba30c 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -1246,18 +1246,11 @@ static int adis16480_config_irq_pin(struct adis16480 *st)
>  {
>  	struct device *dev = &st->adis.spi->dev;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> -	struct irq_data *desc;
>  	enum adis16480_int_pin pin;
>  	unsigned int irq_type;
>  	uint16_t val;
>  	int i, irq = 0;
>  
> -	desc = irq_get_irq_data(st->adis.spi->irq);
> -	if (!desc) {
> -		dev_err(dev, "Could not find IRQ %d\n", irq);
> -		return -EINVAL;
> -	}
> -
>  	/* Disable data ready since the default after reset is on */
>  	val = ADIS16480_DRDY_EN(0);
>  
> @@ -1285,7 +1278,7 @@ static int adis16480_config_irq_pin(struct adis16480 *st)
>  	 * configured as positive or negative, corresponding to
>  	 * IRQ_TYPE_EDGE_RISING or IRQ_TYPE_EDGE_FALLING respectively.
>  	 */
> -	irq_type = irqd_get_trigger_type(desc);
> +	irq_type = irq_get_trigger_type(st->adis.spi->irq);
>  	if (irq_type == IRQ_TYPE_EDGE_RISING) { /* Default */
>  		val |= ADIS16480_DRDY_POL(1);
>  	} else if (irq_type == IRQ_TYPE_EDGE_FALLING) {
>