[PATCH 1/4] iio: st_sensors: simplify buffer address handling

[PATCH 1/4] iio: st_sensors: simplify buffer address handling

[Linus Walleij]

The driver goes to some length to dynamically allocate an array
to hold the channel addresses. However no ST sensor has more than
three channels (x, y, z at most). Instead of kmalloc():ing and
kfree():in the address array, just use a fixed array of three
elements.

Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c | 28 ++++++-----------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index e18bc6782256..73764961feac 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -24,19 +24,13 @@
 
 int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 {
-	u8 *addr;
+	u8 addr[3]; /* no ST sensor has more than 3 channels */
 	int i, n = 0, len;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 	unsigned int num_data_channels = sdata->num_data_channels;
 	unsigned int byte_for_channel =
 			indio_dev->channels[0].scan_type.storagebits >> 3;
 
-	addr = kmalloc(num_data_channels, GFP_KERNEL);
-	if (!addr) {
-		len = -ENOMEM;
-		goto st_sensors_get_buffer_element_error;
-	}
-
 	for (i = 0; i < num_data_channels; i++) {
 		if (test_bit(i, indio_dev->active_scan_mask)) {
 			addr[n] = indio_dev->channels[i].address;
@@ -57,10 +51,8 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 			u8 *rx_array;
 			rx_array = kmalloc(byte_for_channel * num_data_channels,
 					   GFP_KERNEL);
-			if (!rx_array) {
-				len = -ENOMEM;
-				goto st_sensors_free_memory;
-			}
+			if (!rx_array)
+				return -ENOMEM;
 
 			len = sdata->tf->read_multiple_byte(&sdata->tb,
 				sdata->dev, addr[0],
@@ -68,7 +60,7 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 				rx_array, sdata->multiread_bit);
 			if (len < 0) {
 				kfree(rx_array);
-				goto st_sensors_free_memory;
+				return len;
 			}
 
 			for (i = 0; i < n * byte_for_channel; i++) {
@@ -87,17 +79,11 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 			buf, sdata->multiread_bit);
 		break;
 	default:
-		len = -EINVAL;
-		goto st_sensors_free_memory;
-	}
-	if (len != byte_for_channel * n) {
-		len = -EIO;
-		goto st_sensors_free_memory;
+		return -EINVAL;
 	}
+	if (len != byte_for_channel * n)
+		return -EIO;
 
-st_sensors_free_memory:
-	kfree(addr);
-st_sensors_get_buffer_element_error:
 	return len;
 }
 EXPORT_SYMBOL(st_sensors_get_buffer_element);
-- 
2.4.3

[PATCH 2/4] iio: st_sensors: read each channel individually

[Linus Walleij]

The current buffer read code tries to optimize reads from the
sensor data registers by issuing a single read operation across
all the indata registers.

This doesn't work: when the LIS331DL accelerometer sensor is
configured to open drain, active low interrupt mode, this will
just clear the XDA (X-axis data available) bit in the STATUS_REG
register (0x27), while YDA, ZDA and even ZYXDA remain set to 1,
and the internal logic of the sensor holds the DRDY (INT1) line
asserted (the value of the status register is 0xee).

If we instead issue one read operation per enabled channel
(X, Y, Z) things start working and we can use open drain and
active low interrupts.

Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c | 63 +++++------------------
 1 file changed, 13 insertions(+), 50 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index 73764961feac..2ce0d2a3f855 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -24,67 +24,30 @@
 
 int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 {
-	u8 addr[3]; /* no ST sensor has more than 3 channels */
-	int i, n = 0, len;
+	int i, len;
+	int total = 0;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 	unsigned int num_data_channels = sdata->num_data_channels;
-	unsigned int byte_for_channel =
-			indio_dev->channels[0].scan_type.storagebits >> 3;
 
 	for (i = 0; i < num_data_channels; i++) {
+		unsigned int bytes_to_read;
+
 		if (test_bit(i, indio_dev->active_scan_mask)) {
-			addr[n] = indio_dev->channels[i].address;
-			n++;
-		}
-	}
-	switch (n) {
-	case 1:
-		len = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
-			addr[0], byte_for_channel, buf, sdata->multiread_bit);
-		break;
-	case 2:
-		if ((addr[1] - addr[0]) == byte_for_channel) {
+			bytes_to_read = indio_dev->channels[i].scan_type.storagebits >> 3;
 			len = sdata->tf->read_multiple_byte(&sdata->tb,
-				sdata->dev, addr[0], byte_for_channel * n,
-				buf, sdata->multiread_bit);
-		} else {
-			u8 *rx_array;
-			rx_array = kmalloc(byte_for_channel * num_data_channels,
-					   GFP_KERNEL);
-			if (!rx_array)
-				return -ENOMEM;
+				sdata->dev, indio_dev->channels[i].address,
+				bytes_to_read,
+				buf + total, sdata->multiread_bit);
 
-			len = sdata->tf->read_multiple_byte(&sdata->tb,
-				sdata->dev, addr[0],
-				byte_for_channel * num_data_channels,
-				rx_array, sdata->multiread_bit);
-			if (len < 0) {
-				kfree(rx_array);
-				return len;
-			}
+			if (len < bytes_to_read)
+				return -EIO;
 
-			for (i = 0; i < n * byte_for_channel; i++) {
-				if (i < n)
-					buf[i] = rx_array[i];
-				else
-					buf[i] = rx_array[n + i];
-			}
-			kfree(rx_array);
-			len = byte_for_channel * n;
+			/* Advance the buffer pointer */
+			total += len;
 		}
-		break;
-	case 3:
-		len = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
-			addr[0], byte_for_channel * num_data_channels,
-			buf, sdata->multiread_bit);
-		break;
-	default:
-		return -EINVAL;
 	}
-	if (len != byte_for_channel * n)
-		return -EIO;
 
-	return len;
+	return total;
 }
 EXPORT_SYMBOL(st_sensors_get_buffer_element);
 
-- 
2.4.3

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Jonathan Cameron]

On 24/03/16 13:18, Linus Walleij wrote:
> The current buffer read code tries to optimize reads from the
> sensor data registers by issuing a single read operation across
> all the indata registers.
> 
> This doesn't work: when the LIS331DL accelerometer sensor is
> configured to open drain, active low interrupt mode, this will
> just clear the XDA (X-axis data available) bit in the STATUS_REG
> register (0x27), while YDA, ZDA and even ZYXDA remain set to 1,
> and the internal logic of the sensor holds the DRDY (INT1) line
> asserted (the value of the status register is 0xee).
> 
> If we instead issue one read operation per enabled channel
> (X, Y, Z) things start working and we can use open drain and
> active low interrupts.
Hi Linus,

No problem with the patch itself, but I'd like to get a better
understanding of the issue.  Are we talking a weird hardware 'bug' that
occurs only in these particular circumstances?  Can we pin down which of
the above conditions are necessary to make it not work?

I'd just like to know if this is a fix that needs to go upstream faster
than the open drain support or not.

Thanks,

Jonathan
> 
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 63 +++++------------------
>  1 file changed, 13 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index 73764961feac..2ce0d2a3f855 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -24,67 +24,30 @@
>  
>  int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  {
> -	u8 addr[3]; /* no ST sensor has more than 3 channels */
> -	int i, n = 0, len;
> +	int i, len;
> +	int total = 0;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  	unsigned int num_data_channels = sdata->num_data_channels;
> -	unsigned int byte_for_channel =
> -			indio_dev->channels[0].scan_type.storagebits >> 3;
>  
>  	for (i = 0; i < num_data_channels; i++) {
> +		unsigned int bytes_to_read;
> +
>  		if (test_bit(i, indio_dev->active_scan_mask)) {
> -			addr[n] = indio_dev->channels[i].address;
> -			n++;
> -		}
> -	}
> -	switch (n) {
> -	case 1:
> -		len = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
> -			addr[0], byte_for_channel, buf, sdata->multiread_bit);
> -		break;
> -	case 2:
> -		if ((addr[1] - addr[0]) == byte_for_channel) {
> +			bytes_to_read = indio_dev->channels[i].scan_type.storagebits >> 3;
>  			len = sdata->tf->read_multiple_byte(&sdata->tb,
> -				sdata->dev, addr[0], byte_for_channel * n,
> -				buf, sdata->multiread_bit);
> -		} else {
> -			u8 *rx_array;
> -			rx_array = kmalloc(byte_for_channel * num_data_channels,
> -					   GFP_KERNEL);
> -			if (!rx_array)
> -				return -ENOMEM;
> +				sdata->dev, indio_dev->channels[i].address,
> +				bytes_to_read,
> +				buf + total, sdata->multiread_bit);
>  
> -			len = sdata->tf->read_multiple_byte(&sdata->tb,
> -				sdata->dev, addr[0],
> -				byte_for_channel * num_data_channels,
> -				rx_array, sdata->multiread_bit);
> -			if (len < 0) {
> -				kfree(rx_array);
> -				return len;
> -			}
> +			if (len < bytes_to_read)
> +				return -EIO;
>  
> -			for (i = 0; i < n * byte_for_channel; i++) {
> -				if (i < n)
> -					buf[i] = rx_array[i];
> -				else
> -					buf[i] = rx_array[n + i];
> -			}
> -			kfree(rx_array);
> -			len = byte_for_channel * n;
> +			/* Advance the buffer pointer */
> +			total += len;
>  		}
> -		break;
> -	case 3:
> -		len = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
> -			addr[0], byte_for_channel * num_data_channels,
> -			buf, sdata->multiread_bit);
> -		break;
> -	default:
> -		return -EINVAL;
>  	}
> -	if (len != byte_for_channel * n)
> -		return -EIO;
>  
> -	return len;
> +	return total;
>  }
>  EXPORT_SYMBOL(st_sensors_get_buffer_element);
>  
> 

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Linus Walleij]

On Mon, Mar 28, 2016 at 10:03 AM, Jonathan Cameron <jic23@kernel.org> wrote:

> No problem with the patch itself, but I'd like to get a better
> understanding of the issue.  Are we talking a weird hardware 'bug' that
> occurs only in these particular circumstances?  Can we pin down which of
> the above conditions are necessary to make it not work?

I have. I have the following patch in my tree
(Giuseppe, tell me if I should send this as patch if you
want to try it on your designs.):

>From 24fe4792c3bc92f587f44a5c54435be5745e424c Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Thu, 24 Mar 2016 09:21:01 +0100
Subject: [PATCH] iio: st_sensors: detect residue in channels

Hack.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c
b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index c55898543a47..71c079b50548 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -83,6 +83,26 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
     iio_push_to_buffers_with_timestamp(indio_dev, sdata->buffer_data,
         pf->timestamp);

+    if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
+        u8 status;
+
+        len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
+               sdata->sensor_settings->drdy_irq.addr_stat_drdy,
+               &status);
+        if (len < 0)
+            dev_err(sdata->dev, "could not read channel status\n");
+
+        /*
+         * If this was not caused by any channels on this sensor,
+         * return IRQ_NONE
+         */
+        if (status & (u8)indio_dev->active_scan_mask[0])
+            dev_err(sdata->dev,
+                "ERROR residue data status = %02x, scan_mask = %02x\n",
+                status,
+                (u8)indio_dev->active_scan_mask[0]);
+    }
+
 st_sensors_get_buffer_element_error:
     iio_trigger_notify_done(indio_dev->trig);

-- 
2.4.3

This triggers when I use active-low IRQs and open drain setting.
The register contains 0x07 or 0xee indicating that the X axis status
bit was cleared but not Y, Z.

> I'd just like to know if this is a fix that needs to go upstream faster
> than the open drain support or not.

I cannot test active-high IRQs on the LIS331DL since it is wired
up as it is (requireing active-low+open drain). I don't think it's an
issue with that sensor in general but rather with the active-low+OD
mode in general but admittedly it is a rough guess.

It *might* be that LIS331DL needs this to even work properly :/

Yours,
Linus Walleij

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Jonathan Cameron]

On 28/03/16 10:20, Linus Walleij wrote:
> On Mon, Mar 28, 2016 at 10:03 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> No problem with the patch itself, but I'd like to get a better
>> understanding of the issue.  Are we talking a weird hardware 'bug' that
>> occurs only in these particular circumstances?  Can we pin down which of
>> the above conditions are necessary to make it not work?
> 
> I have. I have the following patch in my tree
> (Giuseppe, tell me if I should send this as patch if you
> want to try it on your designs.):
> 
> From 24fe4792c3bc92f587f44a5c54435be5745e424c Mon Sep 17 00:00:00 2001
> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Thu, 24 Mar 2016 09:21:01 +0100
> Subject: [PATCH] iio: st_sensors: detect residue in channels
> 
> Hack.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index c55898543a47..71c079b50548 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -83,6 +83,26 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
>      iio_push_to_buffers_with_timestamp(indio_dev, sdata->buffer_data,
>          pf->timestamp);
> 
> +    if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
> +        u8 status;
> +
> +        len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
> +               sdata->sensor_settings->drdy_irq.addr_stat_drdy,
> +               &status);
> +        if (len < 0)
> +            dev_err(sdata->dev, "could not read channel status\n");
> +
> +        /*
> +         * If this was not caused by any channels on this sensor,
> +         * return IRQ_NONE
> +         */
> +        if (status & (u8)indio_dev->active_scan_mask[0])
> +            dev_err(sdata->dev,
> +                "ERROR residue data status = %02x, scan_mask = %02x\n",
> +                status,
> +                (u8)indio_dev->active_scan_mask[0]);
> +    }
> +
>  st_sensors_get_buffer_element_error:
>      iio_trigger_notify_done(indio_dev->trig);
> 
> This triggers when I use active-low IRQs and open drain setting.
> The register contains 0x07 or 0xee indicating that the X axis status
> bit was cleared but not Y, Z.
> 
>> I'd just like to know if this is a fix that needs to go upstream faster
>> than the open drain support or not.
> 
> I cannot test active-high IRQs on the LIS331DL since it is wired
> up as it is (requireing active-low+open drain). I don't think it's an
> issue with that sensor in general but rather with the active-low+OD
> mode in general but admittedly it is a rough guess.
> 
> It *might* be that LIS331DL needs this to even work properly :/
That was precisely what I was wondering!

Let's see if we can pin this down a bit further.  More info would certainly
help anyone trying to use this with an older kernel.

Jonathan
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Linus Walleij]

On Mon, Mar 28, 2016 at 11:20 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Mon, Mar 28, 2016 at 10:03 AM, Jonathan Cameron <jic23@kernel.org> wrote:

>> I'd just like to know if this is a fix that needs to go upstream faster
>> than the open drain support or not.
(...)
>
> It *might* be that LIS331DL needs this to even work properly :/

I tested on a second board today, which has *only* the LIS331DL
mounted, on an active-high line.

It has the same problem, and this commit fixes it.

So there are sensors out there that doesn't work well
with the bulk read optimization.

I think this patch should be applied for fixes, tell me if
you need it rebased as the first patch (though I think it
will just apply).

Giuseppe/Denis? Agree?

Yours,
Linus Walleij

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Jonathan Cameron]

On 29/03/16 09:15, Linus Walleij wrote:
> On Mon, Mar 28, 2016 at 11:20 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Mon, Mar 28, 2016 at 10:03 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
>>> I'd just like to know if this is a fix that needs to go upstream faster
>>> than the open drain support or not.
> (...)
>>
>> It *might* be that LIS331DL needs this to even work properly :/
> 
> I tested on a second board today, which has *only* the LIS331DL
> mounted, on an active-high line.
> 
> It has the same problem, and this commit fixes it.
> 
> So there are sensors out there that doesn't work well
> with the bulk read optimization.
> 
> I think this patch should be applied for fixes, tell me if
> you need it rebased as the first patch (though I think it
> will just apply).
> 
> Giuseppe/Denis? Agree?
Anyone want to comment on this?

J
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Linus Walleij]

On Sun, Apr 10, 2016 at 4:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 29/03/16 09:15, Linus Walleij wrote:
>> On Mon, Mar 28, 2016 at 11:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> So there are sensors out there that doesn't work well
>> with the bulk read optimization.
>>
>> I think this patch should be applied for fixes, tell me if
>> you need it rebased as the first patch (though I think it
>> will just apply).
>>
>> Giuseppe/Denis? Agree?
>
> Anyone want to comment on this?

I'd say apply it.

If bulk mode should be supported it needs to be opt-in
since the LIS331DL obviously cannot handle it.

Yours,
Linus Walleij

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Jonathan Cameron]

On 11/04/16 07:50, Linus Walleij wrote:
> On Sun, Apr 10, 2016 at 4:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 29/03/16 09:15, Linus Walleij wrote:
>>> On Mon, Mar 28, 2016 at 11:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
>>> So there are sensors out there that doesn't work well
>>> with the bulk read optimization.
>>>
>>> I think this patch should be applied for fixes, tell me if
>>> you need it rebased as the first patch (though I think it
>>> will just apply).
>>>
>>> Giuseppe/Denis? Agree?
>>
>> Anyone want to comment on this?
> 
> I'd say apply it.
> 
> If bulk mode should be supported it needs to be opt-in
> since the LIS331DL obviously cannot handle it.
> 
Unfortunately the context has changed a lot due to your earlier patch:

iio: st_sensors: simplify buffer address handling
which is in the togreg branch this is based on, but hasn't yet hit
mainline.  Looking at dates it must have 'just missed' the last merge
window.

Given how different they are I think we are going to have to do different
patches for current tree and a backported one.

So I'll pick this up in the togreg branch so we can move forward with
that but can you roll one against current mainline as well.

Thanks,

Jonathan

Re: [PATCH 2/4] iio: st_sensors: read each channel individually

[Linus Walleij]

On Sun, Apr 17, 2016 at 1:22 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 11/04/16 07:50, Linus Walleij wrote:

>> If bulk mode should be supported it needs to be opt-in
>> since the LIS331DL obviously cannot handle it.
>>
> Unfortunately the context has changed a lot due to your earlier patch:
>
> iio: st_sensors: simplify buffer address handling
> which is in the togreg branch this is based on, but hasn't yet hit
> mainline.  Looking at dates it must have 'just missed' the last merge
> window.
>
> Given how different they are I think we are going to have to do different
> patches for current tree and a backported one.

OK

> So I'll pick this up in the togreg branch so we can move forward with
> that but can you roll one against current mainline as well.

It's no problem for me as I use the latest mainline all the time.

I'm more worried about anyone else out there trying to use
current -stable kernels with the LIS331DL.

I can't help but suspecting that there are lots of custom trees
out there with something like this fix (or another driver stack
altogether).

If anyone wants me to backport this, tell me.

Yours,
Linus Walleij

[PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Linus Walleij]

This makes all ST sensor drivers check that they actually have
new data available for the requested channel(s) before claiming
an IRQ, by reading the status register (which is conveniently
the same for all ST sensors) and check that the channel has new
data before proceeding to read it and fill the buffer.

This way sensors can share an interrupt line: it can be flaged
as shared and then the sensor that did not fire will return
NO_IRQ, and the sensor that fired will handle the IRQ and
return IRQ_HANDLED.

Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/accel/st_accel_core.c                 |  5 +++++
 drivers/iio/common/st_sensors/st_sensors_buffer.c | 18 ++++++++++++++++++
 drivers/iio/gyro/st_gyro_core.c                   |  3 +++
 drivers/iio/magnetometer/st_magn_core.c           |  1 +
 drivers/iio/pressure/st_pressure_core.c           |  2 ++
 include/linux/iio/common/st_sensors.h             |  3 +++
 6 files changed, 32 insertions(+)

diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index a03a1417dd63..4df4c278af23 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -302,6 +302,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 			.mask_int2 = ST_ACCEL_1_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_ACCEL_1_IHL_IRQ_ADDR,
 			.mask_ihl = ST_ACCEL_1_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_ACCEL_1_MULTIREAD_BIT,
 		.bootime = 2,
@@ -367,6 +368,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 			.mask_int2 = ST_ACCEL_2_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_ACCEL_2_IHL_IRQ_ADDR,
 			.mask_ihl = ST_ACCEL_2_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_ACCEL_2_MULTIREAD_BIT,
 		.bootime = 2,
@@ -444,6 +446,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 			.mask_int2 = ST_ACCEL_3_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_ACCEL_3_IHL_IRQ_ADDR,
 			.mask_ihl = ST_ACCEL_3_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 			.ig1 = {
 				.en_addr = ST_ACCEL_3_IG1_EN_ADDR,
 				.en_mask = ST_ACCEL_3_IG1_EN_MASK,
@@ -502,6 +505,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 		.drdy_irq = {
 			.addr = ST_ACCEL_4_DRDY_IRQ_ADDR,
 			.mask_int1 = ST_ACCEL_4_DRDY_IRQ_INT1_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_ACCEL_4_MULTIREAD_BIT,
 		.bootime = 2, /* guess */
@@ -553,6 +557,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 			.mask_int2 = ST_ACCEL_5_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_ACCEL_5_IHL_IRQ_ADDR,
 			.mask_ihl = ST_ACCEL_5_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_ACCEL_5_MULTIREAD_BIT,
 		.bootime = 2, /* guess */
diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index 2ce0d2a3f855..c55898543a47 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -58,6 +58,24 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
+	/* If we have a status register, check if this IRQ came from us */
+	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
+		u8 status;
+
+		len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
+			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
+			   &status);
+		if (len < 0)
+			dev_err(sdata->dev, "could not read channel status\n");
+
+		/*
+		 * If this was not caused by any channels on this sensor,
+		 * return IRQ_NONE
+		 */
+		if (!(status & (u8)indio_dev->active_scan_mask[0]))
+			return IRQ_NONE;
+	}
+
 	len = st_sensors_get_buffer_element(indio_dev, sdata->buffer_data);
 	if (len < 0)
 		goto st_sensors_get_buffer_element_error;
diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index 110f95b6e52f..be9057e89dc3 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -190,6 +190,7 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
 			 * drain settings, but only for INT1 and not
 			 * for the DRDY line on INT2.
 			 */
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_GYRO_1_MULTIREAD_BIT,
 		.bootime = 2,
@@ -258,6 +259,7 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
 			 * drain settings, but only for INT1 and not
 			 * for the DRDY line on INT2.
 			 */
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_GYRO_2_MULTIREAD_BIT,
 		.bootime = 2,
@@ -322,6 +324,7 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
 			 * drain settings, but only for INT1 and not
 			 * for the DRDY line on INT2.
 			 */
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_GYRO_3_MULTIREAD_BIT,
 		.bootime = 2,
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 501f858df413..62036d2a9956 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -484,6 +484,7 @@ static const struct st_sensor_settings st_magn_sensors_settings[] = {
 			.mask_int1 = ST_MAGN_3_DRDY_INT_MASK,
 			.addr_ihl = ST_MAGN_3_IHL_IRQ_ADDR,
 			.mask_ihl = ST_MAGN_3_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_MAGN_3_MULTIREAD_BIT,
 		.bootime = 2,
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 172393ad34af..1cd37eaa4a57 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -226,6 +226,7 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
 			.mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_PRESS_LPS331AP_IHL_IRQ_ADDR,
 			.mask_ihl = ST_PRESS_LPS331AP_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
 		.bootime = 2,
@@ -312,6 +313,7 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
 			.mask_int2 = ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_PRESS_LPS25H_IHL_IRQ_ADDR,
 			.mask_ihl = ST_PRESS_LPS25H_IHL_IRQ_MASK,
+			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_PRESS_LPS25H_MULTIREAD_BIT,
 		.bootime = 2,
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 6670c3d25c58..d8da075bfda0 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -37,6 +37,7 @@
 #define ST_SENSORS_DEFAULT_AXIS_ADDR		0x20
 #define ST_SENSORS_DEFAULT_AXIS_MASK		0x07
 #define ST_SENSORS_DEFAULT_AXIS_N_BIT		3
+#define ST_SENSORS_DEFAULT_STAT_ADDR		0x27
 
 #define ST_SENSORS_MAX_NAME			17
 #define ST_SENSORS_MAX_4WAI			7
@@ -121,6 +122,7 @@ struct st_sensor_bdu {
  * @mask_int2: mask to enable/disable IRQ on INT2 pin.
  * @addr_ihl: address to enable/disable active low on the INT lines.
  * @mask_ihl: mask to enable/disable active low on the INT lines.
+ * @addr_stat_drdy: address to read status of DRDY (data ready) interrupt
  * struct ig1 - represents the Interrupt Generator 1 of sensors.
  * @en_addr: address of the enable ig1 register.
  * @en_mask: mask to write the on/off value for enable.
@@ -131,6 +133,7 @@ struct st_sensor_data_ready_irq {
 	u8 mask_int2;
 	u8 addr_ihl;
 	u8 mask_ihl;
+	u8 addr_stat_drdy;
 	struct {
 		u8 en_addr;
 		u8 en_mask;
-- 
2.4.3

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Jonathan Cameron]

On 24/03/16 13:18, Linus Walleij wrote:
> This makes all ST sensor drivers check that they actually have
> new data available for the requested channel(s) before claiming
> an IRQ, by reading the status register (which is conveniently
> the same for all ST sensors) and check that the channel has new
> data before proceeding to read it and fill the buffer.
> 
> This way sensors can share an interrupt line: it can be flaged
> as shared and then the sensor that did not fire will return
> NO_IRQ, and the sensor that fired will handle the IRQ and
> return IRQ_HANDLED.
> 
Looks good and even matches on the archaic lis3l02dq I keep meaning
to add to the driver :) (had that datasheet lying around)

One day we'll figure out how to report 'overruns' sensibly at
which point we can use the other bits in that register as well.

Anyhow, will let this sit just a little longer as would like Denis
and/or Giuseppe to have a look at it as well.

Jonathan
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/accel/st_accel_core.c                 |  5 +++++
>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 18 ++++++++++++++++++
>  drivers/iio/gyro/st_gyro_core.c                   |  3 +++
>  drivers/iio/magnetometer/st_magn_core.c           |  1 +
>  drivers/iio/pressure/st_pressure_core.c           |  2 ++
>  include/linux/iio/common/st_sensors.h             |  3 +++
>  6 files changed, 32 insertions(+)
> 
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index a03a1417dd63..4df4c278af23 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -302,6 +302,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_1_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_1_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_1_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_1_MULTIREAD_BIT,
>  		.bootime = 2,
> @@ -367,6 +368,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_2_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_2_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_2_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_2_MULTIREAD_BIT,
>  		.bootime = 2,
> @@ -444,6 +446,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_3_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_3_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_3_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  			.ig1 = {
>  				.en_addr = ST_ACCEL_3_IG1_EN_ADDR,
>  				.en_mask = ST_ACCEL_3_IG1_EN_MASK,
> @@ -502,6 +505,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  		.drdy_irq = {
>  			.addr = ST_ACCEL_4_DRDY_IRQ_ADDR,
>  			.mask_int1 = ST_ACCEL_4_DRDY_IRQ_INT1_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_4_MULTIREAD_BIT,
>  		.bootime = 2, /* guess */
> @@ -553,6 +557,7 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_5_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_5_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_5_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_5_MULTIREAD_BIT,
>  		.bootime = 2, /* guess */
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index 2ce0d2a3f855..c55898543a47 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -58,6 +58,24 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  
> +	/* If we have a status register, check if this IRQ came from us */
> +	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
> +		u8 status;
> +
> +		len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
> +			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
> +			   &status);
> +		if (len < 0)
> +			dev_err(sdata->dev, "could not read channel status\n");
> +
> +		/*
> +		 * If this was not caused by any channels on this sensor,
> +		 * return IRQ_NONE
> +		 */
> +		if (!(status & (u8)indio_dev->active_scan_mask[0]))
> +			return IRQ_NONE;
> +	}
> +
>  	len = st_sensors_get_buffer_element(indio_dev, sdata->buffer_data);
>  	if (len < 0)
>  		goto st_sensors_get_buffer_element_error;
> diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
> index 110f95b6e52f..be9057e89dc3 100644
> --- a/drivers/iio/gyro/st_gyro_core.c
> +++ b/drivers/iio/gyro/st_gyro_core.c
> @@ -190,6 +190,7 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
>  			 * drain settings, but only for INT1 and not
>  			 * for the DRDY line on INT2.
>  			 */
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_GYRO_1_MULTIREAD_BIT,
>  		.bootime = 2,
> @@ -258,6 +259,7 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
>  			 * drain settings, but only for INT1 and not
>  			 * for the DRDY line on INT2.
>  			 */
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_GYRO_2_MULTIREAD_BIT,
>  		.bootime = 2,
> @@ -322,6 +324,7 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
>  			 * drain settings, but only for INT1 and not
>  			 * for the DRDY line on INT2.
>  			 */
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_GYRO_3_MULTIREAD_BIT,
>  		.bootime = 2,
> diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
> index 501f858df413..62036d2a9956 100644
> --- a/drivers/iio/magnetometer/st_magn_core.c
> +++ b/drivers/iio/magnetometer/st_magn_core.c
> @@ -484,6 +484,7 @@ static const struct st_sensor_settings st_magn_sensors_settings[] = {
>  			.mask_int1 = ST_MAGN_3_DRDY_INT_MASK,
>  			.addr_ihl = ST_MAGN_3_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_MAGN_3_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_MAGN_3_MULTIREAD_BIT,
>  		.bootime = 2,
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 172393ad34af..1cd37eaa4a57 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -226,6 +226,7 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
>  			.mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_PRESS_LPS331AP_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_PRESS_LPS331AP_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
>  		.bootime = 2,
> @@ -312,6 +313,7 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
>  			.mask_int2 = ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_PRESS_LPS25H_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_PRESS_LPS25H_IHL_IRQ_MASK,
> +			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_PRESS_LPS25H_MULTIREAD_BIT,
>  		.bootime = 2,
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 6670c3d25c58..d8da075bfda0 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -37,6 +37,7 @@
>  #define ST_SENSORS_DEFAULT_AXIS_ADDR		0x20
>  #define ST_SENSORS_DEFAULT_AXIS_MASK		0x07
>  #define ST_SENSORS_DEFAULT_AXIS_N_BIT		3
> +#define ST_SENSORS_DEFAULT_STAT_ADDR		0x27
>  
>  #define ST_SENSORS_MAX_NAME			17
>  #define ST_SENSORS_MAX_4WAI			7
> @@ -121,6 +122,7 @@ struct st_sensor_bdu {
>   * @mask_int2: mask to enable/disable IRQ on INT2 pin.
>   * @addr_ihl: address to enable/disable active low on the INT lines.
>   * @mask_ihl: mask to enable/disable active low on the INT lines.
> + * @addr_stat_drdy: address to read status of DRDY (data ready) interrupt
>   * struct ig1 - represents the Interrupt Generator 1 of sensors.
>   * @en_addr: address of the enable ig1 register.
>   * @en_mask: mask to write the on/off value for enable.
> @@ -131,6 +133,7 @@ struct st_sensor_data_ready_irq {
>  	u8 mask_int2;
>  	u8 addr_ihl;
>  	u8 mask_ihl;
> +	u8 addr_stat_drdy;
>  	struct {
>  		u8 en_addr;
>  		u8 en_mask;
> 

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Linus Walleij]

On Mon, Mar 28, 2016 at 10:09 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 24/03/16 13:18, Linus Walleij wrote:
>> This makes all ST sensor drivers check that they actually have
>> new data available for the requested channel(s) before claiming
>> an IRQ, by reading the status register (which is conveniently
>> the same for all ST sensors) and check that the channel has new
>> data before proceeding to read it and fill the buffer.
>>
>> This way sensors can share an interrupt line: it can be flaged
>> as shared and then the sensor that did not fire will return
>> NO_IRQ, and the sensor that fired will handle the IRQ and
>> return IRQ_HANDLED.
>>
> Looks good and even matches on the archaic lis3l02dq I keep meaning
> to add to the driver :) (had that datasheet lying around)
>
> One day we'll figure out how to report 'overruns' sensibly at
> which point we can use the other bits in that register as well.
>
> Anyhow, will let this sit just a little longer as would like Denis
> and/or Giuseppe to have a look at it as well.

If no further comments I guess this could be applied?

Yours,
Linus Walleij

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Jonathan Cameron]

On 12/04/16 13:34, Linus Walleij wrote:
> On Mon, Mar 28, 2016 at 10:09 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 24/03/16 13:18, Linus Walleij wrote:
>>> This makes all ST sensor drivers check that they actually have
>>> new data available for the requested channel(s) before claiming
>>> an IRQ, by reading the status register (which is conveniently
>>> the same for all ST sensors) and check that the channel has new
>>> data before proceeding to read it and fill the buffer.
>>>
>>> This way sensors can share an interrupt line: it can be flaged
>>> as shared and then the sensor that did not fire will return
>>> NO_IRQ, and the sensor that fired will handle the IRQ and
>>> return IRQ_HANDLED.
>>>
>> Looks good and even matches on the archaic lis3l02dq I keep meaning
>> to add to the driver :) (had that datasheet lying around)
>>
>> One day we'll figure out how to report 'overruns' sensibly at
>> which point we can use the other bits in that register as well.
>>
>> Anyhow, will let this sit just a little longer as would like Denis
>> and/or Giuseppe to have a look at it as well.
> 
> If no further comments I guess this could be applied?
> 
> Yours,
> Linus Walleij
Agreed and applied to the togreg branch of iio.git - initially pushed
out as testing for the autobuilders to play with it.

Thanks,

Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Crestez Dan Leonard]

On 03/24/2016 03:18 PM, Linus Walleij wrote:
> This makes all ST sensor drivers check that they actually have
> new data available for the requested channel(s) before claiming
> an IRQ, by reading the status register (which is conveniently
> the same for all ST sensors) and check that the channel has new
> data before proceeding to read it and fill the buffer.
> 
> This way sensors can share an interrupt line: it can be flaged
> as shared and then the sensor that did not fire will return
> NO_IRQ, and the sensor that fired will handle the IRQ and
> return IRQ_HANDLED.
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index 2ce0d2a3f855..c55898543a47 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -58,6 +58,24 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  
> +	/* If we have a status register, check if this IRQ came from us */
> +	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
> +		u8 status;
> +
> +		len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
> +			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
> +			   &status);
> +		if (len < 0)
> +			dev_err(sdata->dev, "could not read channel status\n");
> +
> +		/*
> +		 * If this was not caused by any channels on this sensor,
> +		 * return IRQ_NONE
> +		 */
> +		if (!(status & (u8)indio_dev->active_scan_mask[0]))
> +			return IRQ_NONE;
> +	}
>
This seems to break software trigger mode when the timer frequency is
higher that the device polling frequency. In that case it is possible to
poll multiple times between updates and the first time this happens
further updates hang.

Even with hardware triggers: are you sure this is correct? Shouldn't
iio_trigger_notify_done still get called even when there is nothing to do?

-- 
Regards,
Leonard

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Linus Walleij]

On Tue, May 3, 2016 at 7:58 PM, Crestez Dan Leonard <cdleonard@gmail.com> wrote:
> On 03/24/2016 03:18 PM, Linus Walleij wrote:
>> This makes all ST sensor drivers check that they actually have
>> new data available for the requested channel(s) before claiming
>> an IRQ, by reading the status register (which is conveniently
>> the same for all ST sensors) and check that the channel has new
>> data before proceeding to read it and fill the buffer.
>>
>> This way sensors can share an interrupt line: it can be flaged
>> as shared and then the sensor that did not fire will return
>> NO_IRQ, and the sensor that fired will handle the IRQ and
>> return IRQ_HANDLED.
>>
(...)
>> +     /* If we have a status register, check if this IRQ came from us */
>> +     if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
>> +             u8 status;
>> +
>> +             len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
>> +                        sdata->sensor_settings->drdy_irq.addr_stat_drdy,
>> +                        &status);
>> +             if (len < 0)
>> +                     dev_err(sdata->dev, "could not read channel status\n");
>> +
>> +             /*
>> +              * If this was not caused by any channels on this sensor,
>> +              * return IRQ_NONE
>> +              */
>> +             if (!(status & (u8)indio_dev->active_scan_mask[0]))
>> +                     return IRQ_NONE;
>> +     }
>>
> This seems to break software trigger mode when the timer frequency is
> higher that the device polling frequency. In that case it is possible to
> poll multiple times between updates and the first time this happens
> further updates hang.

OK I understand This is a hard IRQ handler for DRDY.

We need to avoid checking the status register with software triggers.
I guess it's possible to see in struct iio_dev whether we get called
from a software trigger?

Can you give me instructions on how to reproduce the error so I
can verify that I fix it? I never managed to get software triggers
to work :(

> Even with hardware triggers: are you sure this is correct? Shouldn't
> iio_trigger_notify_done still get called even when there is nothing to do?

No then the (hardware) IRQ can have come in from some other
device sharing the line with the peripheral and we need to return
IRQ_NONE. It's not our interrupt.

Yours,
Linus Walleij

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Jonathan Cameron]

On 03/05/16 21:10, Linus Walleij wrote:
> On Tue, May 3, 2016 at 7:58 PM, Crestez Dan Leonard <cdleonard@gmail.com> wrote:
>> On 03/24/2016 03:18 PM, Linus Walleij wrote:
>>> This makes all ST sensor drivers check that they actually have
>>> new data available for the requested channel(s) before claiming
>>> an IRQ, by reading the status register (which is conveniently
>>> the same for all ST sensors) and check that the channel has new
>>> data before proceeding to read it and fill the buffer.
>>>
>>> This way sensors can share an interrupt line: it can be flaged
>>> as shared and then the sensor that did not fire will return
>>> NO_IRQ, and the sensor that fired will handle the IRQ and
>>> return IRQ_HANDLED.
>>>
> (...)
>>> +     /* If we have a status register, check if this IRQ came from us */
>>> +     if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
>>> +             u8 status;
>>> +
>>> +             len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
>>> +                        sdata->sensor_settings->drdy_irq.addr_stat_drdy,
>>> +                        &status);
>>> +             if (len < 0)
>>> +                     dev_err(sdata->dev, "could not read channel status\n");
>>> +
>>> +             /*
>>> +              * If this was not caused by any channels on this sensor,
>>> +              * return IRQ_NONE
>>> +              */
>>> +             if (!(status & (u8)indio_dev->active_scan_mask[0]))
>>> +                     return IRQ_NONE;
>>> +     }
>>>
>> This seems to break software trigger mode when the timer frequency is
>> higher that the device polling frequency. In that case it is possible to
>> poll multiple times between updates and the first time this happens
>> further updates hang.
> 
> OK I understand This is a hard IRQ handler for DRDY.
> 
> We need to avoid checking the status register with software triggers.
> I guess it's possible to see in struct iio_dev whether we get called
> from a software trigger?
> 
> Can you give me instructions on how to reproduce the error so I
> can verify that I fix it? I never managed to get software triggers
> to work :(
> 
>> Even with hardware triggers: are you sure this is correct? Shouldn't
>> iio_trigger_notify_done still get called even when there is nothing to do?
> 
> No then the (hardware) IRQ can have come in from some other
> device sharing the line with the peripheral and we need to return
> IRQ_NONE. It's not our interrupt.
Gah, I fouled up reviewing this.  We really need to know if it is our
trigger 'before' we fire off the trigger logic - any number of other
devices can in theory be hanging off a given trigger - none of them will
be able to know if it was a real trigger or not.

So the check should have been in the top level irq handler.  Right
now that is iio_trigger_generic_data_rdy_poll set in st_sensors_trigger.c
but clearly needs to be more specific.

Also, we are clearly going to have to switch to a threaded handler
which will limit what we can hang off this trigger. Maybe for now
we just validate that only st_sensors can be driven by this trigger..
I doubt anyone will notice the additional restriction... If they
do we can probably work around it.

What we need to do is work out how to elegantly fall back to 
not running top halves of triggers if we are coming from threaded
context.

Can think of one 'dubious' way of doing this that might work.
Add a flag to struct iio_poll_func which is 'I've already run'.
Set it in the top half handler (we don't actually have that many
of those) then we can query it in the bottom half if necessary and
then optionally fun the top half in threaded context (where it is
just grabbing a timestamp for example and is safe).

I'm on a long train journey today - depending on whether my battery
holds, I may have a look at this later and see how bad it would be
in terms of churn.

It might finally let us relax some of the restrictions on which
trigger can be used for which device and in a minimally invasive
way.

Jonathan

p.s. We'll need to get this fixed up in some more temporary fashion
for the current cycle though.


> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Linus Walleij]

On Wed, May 4, 2016 at 9:35 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 03/05/16 21:10, Linus Walleij wrote:

>>> Even with hardware triggers: are you sure this is correct? Shouldn't
>>> iio_trigger_notify_done still get called even when there is nothing to do?
>>
>> No then the (hardware) IRQ can have come in from some other
>> device sharing the line with the peripheral and we need to return
>> IRQ_NONE. It's not our interrupt.
>
> Gah, I fouled up reviewing this.  We really need to know if it is our
> trigger 'before' we fire off the trigger logic - any number of other
> devices can in theory be hanging off a given trigger - none of them will
> be able to know if it was a real trigger or not.

Ooops sorry, but the good thing is: we get to properly fix it.

> What we need to do is work out how to elegantly fall back to
> not running top halves of triggers if we are coming from threaded
> context.

I'm looking into this.

Pretty complex, but certainly doable. As we're not even in the
merge window yet I bet we can cook up a proper solution instead
of dirty hacks.

Yours,
Linus Walleij

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Crestez Dan Leonard]

On 05/04/2016 05:34 PM, Linus Walleij wrote:
> On Wed, May 4, 2016 at 9:35 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 03/05/16 21:10, Linus Walleij wrote:
> 
>>>> Even with hardware triggers: are you sure this is correct? Shouldn't
>>>> iio_trigger_notify_done still get called even when there is nothing to do?
>>>
>>> No then the (hardware) IRQ can have come in from some other
>>> device sharing the line with the peripheral and we need to return
>>> IRQ_NONE. It's not our interrupt.
>>
>> Gah, I fouled up reviewing this.  We really need to know if it is our
>> trigger 'before' we fire off the trigger logic - any number of other
>> devices can in theory be hanging off a given trigger - none of them will
>> be able to know if it was a real trigger or not.
> 
> Ooops sorry, but the good thing is: we get to properly fix it.
> 
I've been testing with some st_sensors devices I have around and it
seems to me that triggered buffers with hardware interrupts don't
actually work properly at high sampling frequencies. It is possible for
another sample to come in before the buffer handler completes and
further interrupts will be lost.

This happens on at least LIS3DH.

It's not obvious how to handle this. Presumably the fix would be to
check STATUS_REG after reading one sample and if new values are
available schedule a new read?

This is easy to reproduce for me because I use an usb-to-i2c adapter
with high latencies. It helps if you increase the sampling_frequency but
it might still be very hard to reproduce on a direct i2c bus.

-- 
Regards,
Leonard

Re: [PATCH 3/4] iio: st_sensors: verify interrupt event to status

[Linus Walleij]

On Wed, May 4, 2016 at 8:14 PM, Crestez Dan Leonard <cdleonard@gmail.com> wrote:
> On 05/04/2016 05:34 PM, Linus Walleij wrote:
>> On Wed, May 4, 2016 at 9:35 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 03/05/16 21:10, Linus Walleij wrote:
>>
>>>>> Even with hardware triggers: are you sure this is correct? Shouldn't
>>>>> iio_trigger_notify_done still get called even when there is nothing to do?
>>>>
>>>> No then the (hardware) IRQ can have come in from some other
>>>> device sharing the line with the peripheral and we need to return
>>>> IRQ_NONE. It's not our interrupt.
>>>
>>> Gah, I fouled up reviewing this.  We really need to know if it is our
>>> trigger 'before' we fire off the trigger logic - any number of other
>>> devices can in theory be hanging off a given trigger - none of them will
>>> be able to know if it was a real trigger or not.
>>
>> Ooops sorry, but the good thing is: we get to properly fix it.

I have sent out a fix (first v1 then v2... because I missed the timestamps)
for this: turns out it was simpler than I thought as there was already a
properly threaded accelerometer driver in the kernel, I just didn't see
it before.

> I've been testing with some st_sensors devices I have around and it
> seems to me that triggered buffers with hardware interrupts don't
> actually work properly at high sampling frequencies. It is possible for
> another sample to come in before the buffer handler completes and
> further interrupts will be lost.

Yes the threaded interrupt handler (if specifying IRQF_ONESHOT)
will mask the DRDY interrupt line while the sensor is being polled for
values.

As these interrupts are usually edge triggered, the transient interrupt
will be lost :(

> This happens on at least LIS3DH.
>
> It's not obvious how to handle this. Presumably the fix would be to
> check STATUS_REG after reading one sample and if new values are
> available schedule a new read?

I actually have a patch similar to that sitting around: I was using it
to detect residues in the sensor when I was debugging LIS331DL.
Let me look it over and see if I can post something on top of the
just sent threading patch.

Still I would consider it something of a hack: if the overall sample
speed goes above the latencies that the hardware and software can
really handle, the the system as a whole is underdimensioned,
and polling from a HRTimer may be more reliable as they at least
don't loose interrupts.

I might be able to get rid of the lock-ups, but that doesn't mean
the timestamps on the readings won't be tilted: they will.

> This is easy to reproduce for me because I use an usb-to-i2c adapter
> with high latencies. It helps if you increase the sampling_frequency but
> it might still be very hard to reproduce on a direct i2c bus.

Yeah you see where the latencies come from.

And I actually could reproduce this with just I2C on the ux500
by setting the sampling frequency on the LIS331DL to 400Hz.

An I2C bus is usually 100kHz or 400kHz if you're lucky. So 1/100k s
per bit transferred. This still would be way quicker than the ST sensors
I've seen (though I hear that things like the Oculus Rift need complete
sensor data at speeds close to the I2C bus bitrate!)

Yours,
Linus Walleij

[PATCH 4/4] iio: st_sensors: support open drain mode

[Linus Walleij]

Some types of ST Sensors can be connected to the same IRQ line
as other peripherals using open drain. Add a device tree binding
and a sensor data property to flip the right bit in the interrupt
control register to enable open drain mode on the INT line.

If the line is set to be open drain, also tag on IRQF_SHARED
to the IRQ flags when requesting the interrupt, as the whole
point of using open drain interrupt lines is to share them with
more than one peripheral (wire-or).

Cc: devicetree@vger.kernel.org
Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Rebase on top of the patches fixing the other issues (handling
  IRQ status check and channel reading bug).
ChangeLog v1->v2:
- Rebased to fit the new patch order.
---
 Documentation/devicetree/bindings/iio/st-sensors.txt |  3 +++
 drivers/iio/accel/st_accel_core.c                    |  8 ++++++++
 drivers/iio/common/st_sensors/st_sensors_core.c      | 20 ++++++++++++++++++++
 drivers/iio/common/st_sensors/st_sensors_trigger.c   | 13 +++++++++++++
 drivers/iio/pressure/st_pressure_core.c              |  8 ++++++++
 include/linux/iio/common/st_sensors.h                |  6 ++++++
 include/linux/platform_data/st_sensors_pdata.h       |  2 ++
 7 files changed, 60 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/st-sensors.txt b/Documentation/devicetree/bindings/iio/st-sensors.txt
index d4b87cc1e446..accb5681fbbb 100644
--- a/Documentation/devicetree/bindings/iio/st-sensors.txt
+++ b/Documentation/devicetree/bindings/iio/st-sensors.txt
@@ -16,6 +16,9 @@ Optional properties:
 - st,drdy-int-pin: the pin on the package that will be used to signal
   "data ready" (valid values: 1 or 2). This property is not configurable
   on all sensors.
+- st,int-pin-open-drain: the interrupt/data ready line will be configured
+  as open drain, which is useful if several sensors share the same
+  interrupt line.
 
 Sensors may also have applicable pin control settings, those use the
 standard bindings from pinctrl/pinctrl-bindings.txt.
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 4df4c278af23..f06f4329db5b 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -96,6 +96,8 @@
 #define ST_ACCEL_2_DRDY_IRQ_INT2_MASK		0x10
 #define ST_ACCEL_2_IHL_IRQ_ADDR			0x22
 #define ST_ACCEL_2_IHL_IRQ_MASK			0x80
+#define ST_ACCEL_2_OD_IRQ_ADDR			0x22
+#define ST_ACCEL_2_OD_IRQ_MASK			0x40
 #define ST_ACCEL_2_MULTIREAD_BIT		true
 
 /* CUSTOM VALUES FOR SENSOR 3 */
@@ -177,6 +179,8 @@
 #define ST_ACCEL_5_DRDY_IRQ_INT2_MASK		0x20
 #define ST_ACCEL_5_IHL_IRQ_ADDR			0x22
 #define ST_ACCEL_5_IHL_IRQ_MASK			0x80
+#define ST_ACCEL_5_OD_IRQ_ADDR			0x22
+#define ST_ACCEL_5_OD_IRQ_MASK			0x40
 #define ST_ACCEL_5_IG1_EN_ADDR			0x21
 #define ST_ACCEL_5_IG1_EN_MASK			0x08
 #define ST_ACCEL_5_MULTIREAD_BIT		false
@@ -368,6 +372,8 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 			.mask_int2 = ST_ACCEL_2_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_ACCEL_2_IHL_IRQ_ADDR,
 			.mask_ihl = ST_ACCEL_2_IHL_IRQ_MASK,
+			.addr_od = ST_ACCEL_2_OD_IRQ_ADDR,
+			.mask_od = ST_ACCEL_2_OD_IRQ_MASK,
 			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_ACCEL_2_MULTIREAD_BIT,
@@ -557,6 +563,8 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
 			.mask_int2 = ST_ACCEL_5_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_ACCEL_5_IHL_IRQ_ADDR,
 			.mask_ihl = ST_ACCEL_5_IHL_IRQ_MASK,
+			.addr_od = ST_ACCEL_5_OD_IRQ_ADDR,
+			.mask_od = ST_ACCEL_5_OD_IRQ_MASK,
 			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_ACCEL_5_MULTIREAD_BIT,
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index f5a2d445d0c0..2e03c1220d1f 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -301,6 +301,14 @@ static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
 		return -EINVAL;
 	}
 
+	if (pdata->open_drain) {
+		if (!sdata->sensor_settings->drdy_irq.addr_od)
+			dev_err(&indio_dev->dev,
+				"open drain requested but unsupported.\n");
+		else
+			sdata->int_pin_open_drain = true;
+	}
+
 	return 0;
 }
 
@@ -321,6 +329,8 @@ static struct st_sensors_platform_data *st_sensors_of_probe(struct device *dev,
 	else
 		pdata->drdy_int_pin = defdata ? defdata->drdy_int_pin : 0;
 
+	pdata->open_drain = of_property_read_bool(np, "st,int-pin-open-drain");
+
 	return pdata;
 }
 #else
@@ -374,6 +384,16 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
 			return err;
 	}
 
+	if (sdata->int_pin_open_drain) {
+		dev_info(&indio_dev->dev,
+			 "set interrupt line to open drain mode\n");
+		err = st_sensors_write_data_with_mask(indio_dev,
+				sdata->sensor_settings->drdy_irq.addr_od,
+				sdata->sensor_settings->drdy_irq.mask_od, 1);
+		if (err < 0)
+			return err;
+	}
+
 	err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
 
 	return err;
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 6a8c98327945..da72279fcf99 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -64,6 +64,19 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 			"rising edge\n", irq_trig);
 		irq_trig = IRQF_TRIGGER_RISING;
 	}
+
+	/*
+	 * If the interrupt pin is Open Drain, by definition this
+	 * means that the interrupt line may be shared with other
+	 * peripherals. But to do this we also need to have a status
+	 * register and mask to figure out if this sensor was firing
+	 * the IRQ or not, so we can tell the interrupt handle that
+	 * it was "our" interrupt.
+	 */
+	if (sdata->int_pin_open_drain &&
+	    sdata->sensor_settings->drdy_irq.addr_stat_drdy)
+		irq_trig |= IRQF_SHARED;
+
 	err = request_threaded_irq(irq,
 			iio_trigger_generic_data_rdy_poll,
 			NULL,
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 1cd37eaa4a57..9e9b72a8f18f 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -64,6 +64,8 @@
 #define ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK	0x20
 #define ST_PRESS_LPS331AP_IHL_IRQ_ADDR		0x22
 #define ST_PRESS_LPS331AP_IHL_IRQ_MASK		0x80
+#define ST_PRESS_LPS331AP_OD_IRQ_ADDR		0x22
+#define ST_PRESS_LPS331AP_OD_IRQ_MASK		0x40
 #define ST_PRESS_LPS331AP_MULTIREAD_BIT		true
 #define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
 
@@ -104,6 +106,8 @@
 #define ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK	0x10
 #define ST_PRESS_LPS25H_IHL_IRQ_ADDR		0x22
 #define ST_PRESS_LPS25H_IHL_IRQ_MASK		0x80
+#define ST_PRESS_LPS25H_OD_IRQ_ADDR		0x22
+#define ST_PRESS_LPS25H_OD_IRQ_MASK		0x40
 #define ST_PRESS_LPS25H_MULTIREAD_BIT		true
 #define ST_PRESS_LPS25H_TEMP_OFFSET		42500
 #define ST_PRESS_LPS25H_OUT_XL_ADDR		0x28
@@ -226,6 +230,8 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
 			.mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_PRESS_LPS331AP_IHL_IRQ_ADDR,
 			.mask_ihl = ST_PRESS_LPS331AP_IHL_IRQ_MASK,
+			.addr_od = ST_PRESS_LPS331AP_OD_IRQ_ADDR,
+			.mask_od = ST_PRESS_LPS331AP_OD_IRQ_MASK,
 			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
@@ -313,6 +319,8 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
 			.mask_int2 = ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK,
 			.addr_ihl = ST_PRESS_LPS25H_IHL_IRQ_ADDR,
 			.mask_ihl = ST_PRESS_LPS25H_IHL_IRQ_MASK,
+			.addr_od = ST_PRESS_LPS25H_OD_IRQ_ADDR,
+			.mask_od = ST_PRESS_LPS25H_OD_IRQ_MASK,
 			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
 		},
 		.multi_read_bit = ST_PRESS_LPS25H_MULTIREAD_BIT,
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index d8da075bfda0..d029ffac0d69 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -122,6 +122,8 @@ struct st_sensor_bdu {
  * @mask_int2: mask to enable/disable IRQ on INT2 pin.
  * @addr_ihl: address to enable/disable active low on the INT lines.
  * @mask_ihl: mask to enable/disable active low on the INT lines.
+ * @addr_od: address to enable/disable Open Drain on the INT lines.
+ * @mask_od: mask to enable/disable Open Drain on the INT lines.
  * @addr_stat_drdy: address to read status of DRDY (data ready) interrupt
  * struct ig1 - represents the Interrupt Generator 1 of sensors.
  * @en_addr: address of the enable ig1 register.
@@ -133,6 +135,8 @@ struct st_sensor_data_ready_irq {
 	u8 mask_int2;
 	u8 addr_ihl;
 	u8 mask_ihl;
+	u8 addr_od;
+	u8 mask_od;
 	u8 addr_stat_drdy;
 	struct {
 		u8 en_addr;
@@ -215,6 +219,7 @@ struct st_sensor_settings {
  * @odr: Output data rate of the sensor [Hz].
  * num_data_channels: Number of data channels used in buffer.
  * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
+ * @int_pin_open_drain: Set the interrupt/DRDY to open drain.
  * @get_irq_data_ready: Function to get the IRQ used for data ready signal.
  * @tf: Transfer function structure used by I/O operations.
  * @tb: Transfer buffers and mutex used by I/O operations.
@@ -236,6 +241,7 @@ struct st_sensor_data {
 	unsigned int num_data_channels;
 
 	u8 drdy_int_pin;
+	bool int_pin_open_drain;
 
 	unsigned int (*get_irq_data_ready) (struct iio_dev *indio_dev);
 
diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
index 753839187ba0..79b0e4cdb814 100644
--- a/include/linux/platform_data/st_sensors_pdata.h
+++ b/include/linux/platform_data/st_sensors_pdata.h
@@ -16,9 +16,11 @@
  * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
  *	Available only for accelerometer and pressure sensors.
  *	Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
+ * @open_drain: set the interrupt line to be open drain if possible.
  */
 struct st_sensors_platform_data {
 	u8 drdy_int_pin;
+	bool open_drain;
 };
 
 #endif /* ST_SENSORS_PDATA_H */
-- 
2.4.3

Re: [PATCH 4/4] iio: st_sensors: support open drain mode

[Jonathan Cameron]

On 24/03/16 13:18, Linus Walleij wrote:
> Some types of ST Sensors can be connected to the same IRQ line
> as other peripherals using open drain. Add a device tree binding
> and a sensor data property to flip the right bit in the interrupt
> control register to enable open drain mode on the INT line.
> 
> If the line is set to be open drain, also tag on IRQF_SHARED
> to the IRQ flags when requesting the interrupt, as the whole
> point of using open drain interrupt lines is to share them with
> more than one peripheral (wire-or).
> 
> Cc: devicetree@vger.kernel.org
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Few comments inline but looks fine to me.  Ideally again an
ack from Denis would be good. Mostly this one is stalled behind the earlier
ones.

Jonathan
> ---
> ChangeLog v2->v3:
> - Rebase on top of the patches fixing the other issues (handling
>   IRQ status check and channel reading bug).
> ChangeLog v1->v2:
> - Rebased to fit the new patch order.
> ---
>  Documentation/devicetree/bindings/iio/st-sensors.txt |  3 +++
>  drivers/iio/accel/st_accel_core.c                    |  8 ++++++++
>  drivers/iio/common/st_sensors/st_sensors_core.c      | 20 ++++++++++++++++++++
>  drivers/iio/common/st_sensors/st_sensors_trigger.c   | 13 +++++++++++++
>  drivers/iio/pressure/st_pressure_core.c              |  8 ++++++++
>  include/linux/iio/common/st_sensors.h                |  6 ++++++
>  include/linux/platform_data/st_sensors_pdata.h       |  2 ++
>  7 files changed, 60 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/st-sensors.txt b/Documentation/devicetree/bindings/iio/st-sensors.txt
> index d4b87cc1e446..accb5681fbbb 100644
> --- a/Documentation/devicetree/bindings/iio/st-sensors.txt
> +++ b/Documentation/devicetree/bindings/iio/st-sensors.txt
> @@ -16,6 +16,9 @@ Optional properties:
>  - st,drdy-int-pin: the pin on the package that will be used to signal
>    "data ready" (valid values: 1 or 2). This property is not configurable
>    on all sensors.
> +- st,int-pin-open-drain: the interrupt/data ready line will be configured
> +  as open drain, which is useful if several sensors share the same
> +  interrupt line.
Really feels like this one ought to be more generic.. Mind you so does
drdy-int-pin!

Hmm. Looks like most open drain controls are chip specific - other than
your pinctrl ones and that doesn't really map to here I guess.


>  
>  Sensors may also have applicable pin control settings, those use the
>  standard bindings from pinctrl/pinctrl-bindings.txt.
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index 4df4c278af23..f06f4329db5b 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -96,6 +96,8 @@
>  #define ST_ACCEL_2_DRDY_IRQ_INT2_MASK		0x10
>  #define ST_ACCEL_2_IHL_IRQ_ADDR			0x22
>  #define ST_ACCEL_2_IHL_IRQ_MASK			0x80
> +#define ST_ACCEL_2_OD_IRQ_ADDR			0x22
> +#define ST_ACCEL_2_OD_IRQ_MASK			0x40
>  #define ST_ACCEL_2_MULTIREAD_BIT		true
>  
>  /* CUSTOM VALUES FOR SENSOR 3 */
> @@ -177,6 +179,8 @@
>  #define ST_ACCEL_5_DRDY_IRQ_INT2_MASK		0x20
>  #define ST_ACCEL_5_IHL_IRQ_ADDR			0x22
>  #define ST_ACCEL_5_IHL_IRQ_MASK			0x80
> +#define ST_ACCEL_5_OD_IRQ_ADDR			0x22
> +#define ST_ACCEL_5_OD_IRQ_MASK			0x40
>  #define ST_ACCEL_5_IG1_EN_ADDR			0x21
>  #define ST_ACCEL_5_IG1_EN_MASK			0x08
>  #define ST_ACCEL_5_MULTIREAD_BIT		false
> @@ -368,6 +372,8 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_2_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_2_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_2_IHL_IRQ_MASK,
> +			.addr_od = ST_ACCEL_2_OD_IRQ_ADDR,
> +			.mask_od = ST_ACCEL_2_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_2_MULTIREAD_BIT,
> @@ -557,6 +563,8 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_5_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_5_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_5_IHL_IRQ_MASK,
> +			.addr_od = ST_ACCEL_5_OD_IRQ_ADDR,
> +			.mask_od = ST_ACCEL_5_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_5_MULTIREAD_BIT,
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index f5a2d445d0c0..2e03c1220d1f 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -301,6 +301,14 @@ static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  	}
>  
> +	if (pdata->open_drain) {
> +		if (!sdata->sensor_settings->drdy_irq.addr_od)
> +			dev_err(&indio_dev->dev,
> +				"open drain requested but unsupported.\n");
> +		else
> +			sdata->int_pin_open_drain = true;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -321,6 +329,8 @@ static struct st_sensors_platform_data *st_sensors_of_probe(struct device *dev,
>  	else
>  		pdata->drdy_int_pin = defdata ? defdata->drdy_int_pin : 0;
>  
> +	pdata->open_drain = of_property_read_bool(np, "st,int-pin-open-drain");
> +
>  	return pdata;
>  }
>  #else
> @@ -374,6 +384,16 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  			return err;
>  	}
>  
> +	if (sdata->int_pin_open_drain) {
> +		dev_info(&indio_dev->dev,
> +			 "set interrupt line to open drain mode\n");
Hmm. Curriously I have a board with an lis3l02dq on it where the interrupt
is shared, but doesn't explicitly support open drain. In that case it
is done with some external circuitry.

Anyhow - I guess when I actually add support for that I'll just add a sanity
check in here that addr_od and mask_od are specified :)


> +		err = st_sensors_write_data_with_mask(indio_dev,
> +				sdata->sensor_settings->drdy_irq.addr_od,
> +				sdata->sensor_settings->drdy_irq.mask_od, 1);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
>  
>  	return err;
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 6a8c98327945..da72279fcf99 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -64,6 +64,19 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  			"rising edge\n", irq_trig);
>  		irq_trig = IRQF_TRIGGER_RISING;
>  	}
> +
> +	/*
> +	 * If the interrupt pin is Open Drain, by definition this
> +	 * means that the interrupt line may be shared with other
> +	 * peripherals. But to do this we also need to have a status
> +	 * register and mask to figure out if this sensor was firing
> +	 * the IRQ or not, so we can tell the interrupt handle that
> +	 * it was "our" interrupt.
> +	 */
> +	if (sdata->int_pin_open_drain &&
> +	    sdata->sensor_settings->drdy_irq.addr_stat_drdy)
> +		irq_trig |= IRQF_SHARED;
> +
>  	err = request_threaded_irq(irq,
>  			iio_trigger_generic_data_rdy_poll,
>  			NULL,
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 1cd37eaa4a57..9e9b72a8f18f 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -64,6 +64,8 @@
>  #define ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK	0x20
>  #define ST_PRESS_LPS331AP_IHL_IRQ_ADDR		0x22
>  #define ST_PRESS_LPS331AP_IHL_IRQ_MASK		0x80
> +#define ST_PRESS_LPS331AP_OD_IRQ_ADDR		0x22
> +#define ST_PRESS_LPS331AP_OD_IRQ_MASK		0x40
>  #define ST_PRESS_LPS331AP_MULTIREAD_BIT		true
>  #define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
>  
> @@ -104,6 +106,8 @@
>  #define ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK	0x10
>  #define ST_PRESS_LPS25H_IHL_IRQ_ADDR		0x22
>  #define ST_PRESS_LPS25H_IHL_IRQ_MASK		0x80
> +#define ST_PRESS_LPS25H_OD_IRQ_ADDR		0x22
> +#define ST_PRESS_LPS25H_OD_IRQ_MASK		0x40
>  #define ST_PRESS_LPS25H_MULTIREAD_BIT		true
>  #define ST_PRESS_LPS25H_TEMP_OFFSET		42500
>  #define ST_PRESS_LPS25H_OUT_XL_ADDR		0x28
> @@ -226,6 +230,8 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
>  			.mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_PRESS_LPS331AP_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_PRESS_LPS331AP_IHL_IRQ_MASK,
> +			.addr_od = ST_PRESS_LPS331AP_OD_IRQ_ADDR,
> +			.mask_od = ST_PRESS_LPS331AP_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
> @@ -313,6 +319,8 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
>  			.mask_int2 = ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_PRESS_LPS25H_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_PRESS_LPS25H_IHL_IRQ_MASK,
> +			.addr_od = ST_PRESS_LPS25H_OD_IRQ_ADDR,
> +			.mask_od = ST_PRESS_LPS25H_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_PRESS_LPS25H_MULTIREAD_BIT,
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index d8da075bfda0..d029ffac0d69 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -122,6 +122,8 @@ struct st_sensor_bdu {
>   * @mask_int2: mask to enable/disable IRQ on INT2 pin.
>   * @addr_ihl: address to enable/disable active low on the INT lines.
>   * @mask_ihl: mask to enable/disable active low on the INT lines.
> + * @addr_od: address to enable/disable Open Drain on the INT lines.
> + * @mask_od: mask to enable/disable Open Drain on the INT lines.
>   * @addr_stat_drdy: address to read status of DRDY (data ready) interrupt
>   * struct ig1 - represents the Interrupt Generator 1 of sensors.
>   * @en_addr: address of the enable ig1 register.
> @@ -133,6 +135,8 @@ struct st_sensor_data_ready_irq {
>  	u8 mask_int2;
>  	u8 addr_ihl;
>  	u8 mask_ihl;
> +	u8 addr_od;
> +	u8 mask_od;
>  	u8 addr_stat_drdy;
>  	struct {
>  		u8 en_addr;
> @@ -215,6 +219,7 @@ struct st_sensor_settings {
>   * @odr: Output data rate of the sensor [Hz].
>   * num_data_channels: Number of data channels used in buffer.
>   * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
> + * @int_pin_open_drain: Set the interrupt/DRDY to open drain.
>   * @get_irq_data_ready: Function to get the IRQ used for data ready signal.
>   * @tf: Transfer function structure used by I/O operations.
>   * @tb: Transfer buffers and mutex used by I/O operations.
> @@ -236,6 +241,7 @@ struct st_sensor_data {
>  	unsigned int num_data_channels;
>  
>  	u8 drdy_int_pin;
> +	bool int_pin_open_drain;
>  
>  	unsigned int (*get_irq_data_ready) (struct iio_dev *indio_dev);
>  
> diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
> index 753839187ba0..79b0e4cdb814 100644
> --- a/include/linux/platform_data/st_sensors_pdata.h
> +++ b/include/linux/platform_data/st_sensors_pdata.h
> @@ -16,9 +16,11 @@
>   * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
>   *	Available only for accelerometer and pressure sensors.
>   *	Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
> + * @open_drain: set the interrupt line to be open drain if possible.
>   */
>  struct st_sensors_platform_data {
>  	u8 drdy_int_pin;
> +	bool open_drain;
>  };
>  
>  #endif /* ST_SENSORS_PDATA_H */
> 

Re: [PATCH 4/4] iio: st_sensors: support open drain mode

[Linus Walleij]

On Mon, Mar 28, 2016 at 11:12 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 24/03/16 13:18, Linus Walleij wrote:

>> +- st,int-pin-open-drain: the interrupt/data ready line will be configured
>> +  as open drain, which is useful if several sensors share the same
>> +  interrupt line.
>
> Really feels like this one ought to be more generic.. Mind you so does
> drdy-int-pin!

Sorry about drdy-int-pin, it wasn't me ;)

That one is a mux kind of thing: interrupt goes out on IRQ line
INT1 or INT2, select one...

> Hmm. Looks like most open drain controls are chip specific - other than
> your pinctrl ones and that doesn't really map to here I guess.

Both the muxing and the OD setting can be done with pin control,
it's just that it is like swatting a fly with a skyscraper - vast overkill
for two interrupt lines.

I came to the conclusion that with small chips or IP blocks just
randomly doing some muxing or biasing of a few pins isn't necessarily
worth all the trouble of implementing pin control, the latter was
conceived for real big arrays of pins really. There are already a
few of these all over the kernel, e.g. the ST Multi-Purpose expander
drivers/mfd/stmpe.c - could be pin control muxed but it'd be overkill.

So as pin control maintainer when this kind of hardware comes up
I usually just "think it over" and then "OK then".

>> +     if (sdata->int_pin_open_drain) {
>> +             dev_info(&indio_dev->dev,
>> +                      "set interrupt line to open drain mode\n");
>
> Hmm. Curriously I have a board with an lis3l02dq on it where the interrupt
> is shared, but doesn't explicitly support open drain. In that case it
> is done with some external circuitry.

That should run fine with just the status read patch from earlier
in the series I guess?

> Anyhow - I guess when I actually add support for that I'll just add a sanity
> check in here that addr_od and mask_od are specified :)

Hm, yeah. Otherwise just patch out my enforcement of the
OD flag for a shared line and add a comment in.

Yours,
Linus Walleij

Re: [PATCH 4/4] iio: st_sensors: support open drain mode

[Jonathan Cameron]

On 31/03/16 09:15, Linus Walleij wrote:
> On Mon, Mar 28, 2016 at 11:12 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 24/03/16 13:18, Linus Walleij wrote:
> 
>>> +- st,int-pin-open-drain: the interrupt/data ready line will be configured
>>> +  as open drain, which is useful if several sensors share the same
>>> +  interrupt line.
>>
>> Really feels like this one ought to be more generic.. Mind you so does
>> drdy-int-pin!
> 
> Sorry about drdy-int-pin, it wasn't me ;)
> 
> That one is a mux kind of thing: interrupt goes out on IRQ line
> INT1 or INT2, select one...
> 
>> Hmm. Looks like most open drain controls are chip specific - other than
>> your pinctrl ones and that doesn't really map to here I guess.
> 
> Both the muxing and the OD setting can be done with pin control,
> it's just that it is like swatting a fly with a skyscraper - vast overkill
> for two interrupt lines.
> 
> I came to the conclusion that with small chips or IP blocks just
> randomly doing some muxing or biasing of a few pins isn't necessarily
> worth all the trouble of implementing pin control, the latter was
> conceived for real big arrays of pins really. There are already a
> few of these all over the kernel, e.g. the ST Multi-Purpose expander
> drivers/mfd/stmpe.c - could be pin control muxed but it'd be overkill.
> 
> So as pin control maintainer when this kind of hardware comes up
> I usually just "think it over" and then "OK then".
I can indeed see the advantages of that flexibility, but from an IIO point
of view, we are going to have a number of very similar bindings.  I can't
see a reason not to generalize the form of the binding away from a single
driver.

So lets drop the vendor prefix from this one and have
int-pin-open-drain (and drdy-int-pin ideally).
> 
>>> +     if (sdata->int_pin_open_drain) {
>>> +             dev_info(&indio_dev->dev,
>>> +                      "set interrupt line to open drain mode\n");
>>
>> Hmm. Curriously I have a board with an lis3l02dq on it where the interrupt
>> is shared, but doesn't explicitly support open drain. In that case it
>> is done with some external circuitry.
> 
> That should run fine with just the status read patch from earlier
> in the series I guess?
I'll go with a 'probably'.  Getting that part to run at all is 'interesting'
as it's minimum rate is 440Hz and my board can't keep up.
> 
>> Anyhow - I guess when I actually add support for that I'll just add a sanity
>> check in here that addr_od and mask_od are specified :)
> 
> Hm, yeah. Otherwise just patch out my enforcement of the
> OD flag for a shared line and add a comment in.
Will figure it out when / if I ever actually do those patches...

J
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/4] iio: st_sensors: simplify buffer address handling

[Denis Ciocca]

Hi Linus,

it makes sense to me. Thanks

Acked-by: Denis Ciocca <denis.ciocca@st.com>


On |24 Mar 16 @ 14:18|, Linus Walleij wrote:
> The driver goes to some length to dynamically allocate an array
> to hold the channel addresses. However no ST sensor has more than
> three channels (x, y, z at most). Instead of kmalloc():ing and
> kfree():in the address array, just use a fixed array of three
> elements.
> 
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 28 ++++++-----------------
>  1 file changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index e18bc6782256..73764961feac 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -24,19 +24,13 @@
>  
>  int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  {
> -	u8 *addr;
> +	u8 addr[3]; /* no ST sensor has more than 3 channels */
>  	int i, n = 0, len;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  	unsigned int num_data_channels = sdata->num_data_channels;
>  	unsigned int byte_for_channel =
>  			indio_dev->channels[0].scan_type.storagebits >> 3;
>  
> -	addr = kmalloc(num_data_channels, GFP_KERNEL);
> -	if (!addr) {
> -		len = -ENOMEM;
> -		goto st_sensors_get_buffer_element_error;
> -	}
> -
>  	for (i = 0; i < num_data_channels; i++) {
>  		if (test_bit(i, indio_dev->active_scan_mask)) {
>  			addr[n] = indio_dev->channels[i].address;
> @@ -57,10 +51,8 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  			u8 *rx_array;
>  			rx_array = kmalloc(byte_for_channel * num_data_channels,
>  					   GFP_KERNEL);
> -			if (!rx_array) {
> -				len = -ENOMEM;
> -				goto st_sensors_free_memory;
> -			}
> +			if (!rx_array)
> +				return -ENOMEM;
>  
>  			len = sdata->tf->read_multiple_byte(&sdata->tb,
>  				sdata->dev, addr[0],
> @@ -68,7 +60,7 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  				rx_array, sdata->multiread_bit);
>  			if (len < 0) {
>  				kfree(rx_array);
> -				goto st_sensors_free_memory;
> +				return len;
>  			}
>  
>  			for (i = 0; i < n * byte_for_channel; i++) {
> @@ -87,17 +79,11 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  			buf, sdata->multiread_bit);
>  		break;
>  	default:
> -		len = -EINVAL;
> -		goto st_sensors_free_memory;
> -	}
> -	if (len != byte_for_channel * n) {
> -		len = -EIO;
> -		goto st_sensors_free_memory;
> +		return -EINVAL;
>  	}
> +	if (len != byte_for_channel * n)
> +		return -EIO;
>  
> -st_sensors_free_memory:
> -	kfree(addr);
> -st_sensors_get_buffer_element_error:
>  	return len;
>  }
>  EXPORT_SYMBOL(st_sensors_get_buffer_element);
> -- 
> 2.4.3
> 

Re: [PATCH 1/4] iio: st_sensors: simplify buffer address handling

[Jonathan Cameron]

On 28/03/16 04:42, Denis Ciocca wrote:
> Hi Linus,
> 
> it makes sense to me. Thanks
> 
> Acked-by: Denis Ciocca <denis.ciocca@st.com>
Applied to the togreg branch of iio.git.

Denis, any comments on the rest of the series?
> 
> 
> On |24 Mar 16 @ 14:18|, Linus Walleij wrote:
>> The driver goes to some length to dynamically allocate an array
>> to hold the channel addresses. However no ST sensor has more than
>> three channels (x, y, z at most). Instead of kmalloc():ing and
>> kfree():in the address array, just use a fixed array of three
>> elements.
>>
>> Cc: Giuseppe Barba <giuseppe.barba@st.com>
>> Cc: Denis Ciocca <denis.ciocca@st.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 28 ++++++-----------------
>>  1 file changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> index e18bc6782256..73764961feac 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> @@ -24,19 +24,13 @@
>>  
>>  int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>  {
>> -	u8 *addr;
>> +	u8 addr[3]; /* no ST sensor has more than 3 channels */
>>  	int i, n = 0, len;
>>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>>  	unsigned int num_data_channels = sdata->num_data_channels;
>>  	unsigned int byte_for_channel =
>>  			indio_dev->channels[0].scan_type.storagebits >> 3;
>>  
>> -	addr = kmalloc(num_data_channels, GFP_KERNEL);
>> -	if (!addr) {
>> -		len = -ENOMEM;
>> -		goto st_sensors_get_buffer_element_error;
>> -	}
>> -
>>  	for (i = 0; i < num_data_channels; i++) {
>>  		if (test_bit(i, indio_dev->active_scan_mask)) {
>>  			addr[n] = indio_dev->channels[i].address;
>> @@ -57,10 +51,8 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>  			u8 *rx_array;
>>  			rx_array = kmalloc(byte_for_channel * num_data_channels,
>>  					   GFP_KERNEL);
>> -			if (!rx_array) {
>> -				len = -ENOMEM;
>> -				goto st_sensors_free_memory;
>> -			}
>> +			if (!rx_array)
>> +				return -ENOMEM;
>>  
>>  			len = sdata->tf->read_multiple_byte(&sdata->tb,
>>  				sdata->dev, addr[0],
>> @@ -68,7 +60,7 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>  				rx_array, sdata->multiread_bit);
>>  			if (len < 0) {
>>  				kfree(rx_array);
>> -				goto st_sensors_free_memory;
>> +				return len;
>>  			}
>>  
>>  			for (i = 0; i < n * byte_for_channel; i++) {
>> @@ -87,17 +79,11 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>  			buf, sdata->multiread_bit);
>>  		break;
>>  	default:
>> -		len = -EINVAL;
>> -		goto st_sensors_free_memory;
>> -	}
>> -	if (len != byte_for_channel * n) {
>> -		len = -EIO;
>> -		goto st_sensors_free_memory;
>> +		return -EINVAL;
>>  	}
>> +	if (len != byte_for_channel * n)
>> +		return -EIO;
>>  
>> -st_sensors_free_memory:
>> -	kfree(addr);
>> -st_sensors_get_buffer_element_error:
>>  	return len;
>>  }
>>  EXPORT_SYMBOL(st_sensors_get_buffer_element);
>> -- 
>> 2.4.3
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/4] iio: st_sensors: simplify buffer address handling

[Denis Ciocca]

Hi Jonathan,

I'm checking rest of series, I would like to check internally with our designers about what Linus has reported.
Since in Europe today is still holiday, I will be back to you in a couple of days.

Thanks,
Denis


On |28 Mar 16 @ 09:52|, Jonathan Cameron wrote:
> On 28/03/16 04:42, Denis Ciocca wrote:
> > Hi Linus,
> > 
> > it makes sense to me. Thanks
> > 
> > Acked-by: Denis Ciocca <denis.ciocca@st.com>
> Applied to the togreg branch of iio.git.
> 
> Denis, any comments on the rest of the series?
> > 
> > 
> > On |24 Mar 16 @ 14:18|, Linus Walleij wrote:
> >> The driver goes to some length to dynamically allocate an array
> >> to hold the channel addresses. However no ST sensor has more than
> >> three channels (x, y, z at most). Instead of kmalloc():ing and
> >> kfree():in the address array, just use a fixed array of three
> >> elements.
> >>
> >> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> >> Cc: Denis Ciocca <denis.ciocca@st.com>
> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >> ---
> >>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 28 ++++++-----------------
> >>  1 file changed, 7 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >> index e18bc6782256..73764961feac 100644
> >> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >> @@ -24,19 +24,13 @@
> >>  
> >>  int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> >>  {
> >> -	u8 *addr;
> >> +	u8 addr[3]; /* no ST sensor has more than 3 channels */
> >>  	int i, n = 0, len;
> >>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
> >>  	unsigned int num_data_channels = sdata->num_data_channels;
> >>  	unsigned int byte_for_channel =
> >>  			indio_dev->channels[0].scan_type.storagebits >> 3;
> >>  
> >> -	addr = kmalloc(num_data_channels, GFP_KERNEL);
> >> -	if (!addr) {
> >> -		len = -ENOMEM;
> >> -		goto st_sensors_get_buffer_element_error;
> >> -	}
> >> -
> >>  	for (i = 0; i < num_data_channels; i++) {
> >>  		if (test_bit(i, indio_dev->active_scan_mask)) {
> >>  			addr[n] = indio_dev->channels[i].address;
> >> @@ -57,10 +51,8 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> >>  			u8 *rx_array;
> >>  			rx_array = kmalloc(byte_for_channel * num_data_channels,
> >>  					   GFP_KERNEL);
> >> -			if (!rx_array) {
> >> -				len = -ENOMEM;
> >> -				goto st_sensors_free_memory;
> >> -			}
> >> +			if (!rx_array)
> >> +				return -ENOMEM;
> >>  
> >>  			len = sdata->tf->read_multiple_byte(&sdata->tb,
> >>  				sdata->dev, addr[0],
> >> @@ -68,7 +60,7 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> >>  				rx_array, sdata->multiread_bit);
> >>  			if (len < 0) {
> >>  				kfree(rx_array);
> >> -				goto st_sensors_free_memory;
> >> +				return len;
> >>  			}
> >>  
> >>  			for (i = 0; i < n * byte_for_channel; i++) {
> >> @@ -87,17 +79,11 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> >>  			buf, sdata->multiread_bit);
> >>  		break;
> >>  	default:
> >> -		len = -EINVAL;
> >> -		goto st_sensors_free_memory;
> >> -	}
> >> -	if (len != byte_for_channel * n) {
> >> -		len = -EIO;
> >> -		goto st_sensors_free_memory;
> >> +		return -EINVAL;
> >>  	}
> >> +	if (len != byte_for_channel * n)
> >> +		return -EIO;
> >>  
> >> -st_sensors_free_memory:
> >> -	kfree(addr);
> >> -st_sensors_get_buffer_element_error:
> >>  	return len;
> >>  }
> >>  EXPORT_SYMBOL(st_sensors_get_buffer_element);
> >> -- 
> >> 2.4.3
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH 1/4] iio: st_sensors: simplify buffer address handling

[Jonathan Cameron]

On 28/03/16 09:16, Denis Ciocca wrote:
> Hi Jonathan,
> 
> I'm checking rest of series, I would like to check internally with our designers about what Linus has reported.
> Since in Europe today is still holiday, I will be back to you in a couple of days.
> 
Cool - no rush at this time in the cycle!

Jonathan
> Thanks,
> Denis
> 
> 
> On |28 Mar 16 @ 09:52|, Jonathan Cameron wrote:
>> On 28/03/16 04:42, Denis Ciocca wrote:
>>> Hi Linus,
>>>
>>> it makes sense to me. Thanks
>>>
>>> Acked-by: Denis Ciocca <denis.ciocca@st.com>
>> Applied to the togreg branch of iio.git.
>>
>> Denis, any comments on the rest of the series?
>>>
>>>
>>> On |24 Mar 16 @ 14:18|, Linus Walleij wrote:
>>>> The driver goes to some length to dynamically allocate an array
>>>> to hold the channel addresses. However no ST sensor has more than
>>>> three channels (x, y, z at most). Instead of kmalloc():ing and
>>>> kfree():in the address array, just use a fixed array of three
>>>> elements.
>>>>
>>>> Cc: Giuseppe Barba <giuseppe.barba@st.com>
>>>> Cc: Denis Ciocca <denis.ciocca@st.com>
>>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>>> ---
>>>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 28 ++++++-----------------
>>>>  1 file changed, 7 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>> index e18bc6782256..73764961feac 100644
>>>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>> @@ -24,19 +24,13 @@
>>>>  
>>>>  int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>>>  {
>>>> -	u8 *addr;
>>>> +	u8 addr[3]; /* no ST sensor has more than 3 channels */
>>>>  	int i, n = 0, len;
>>>>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>>>>  	unsigned int num_data_channels = sdata->num_data_channels;
>>>>  	unsigned int byte_for_channel =
>>>>  			indio_dev->channels[0].scan_type.storagebits >> 3;
>>>>  
>>>> -	addr = kmalloc(num_data_channels, GFP_KERNEL);
>>>> -	if (!addr) {
>>>> -		len = -ENOMEM;
>>>> -		goto st_sensors_get_buffer_element_error;
>>>> -	}
>>>> -
>>>>  	for (i = 0; i < num_data_channels; i++) {
>>>>  		if (test_bit(i, indio_dev->active_scan_mask)) {
>>>>  			addr[n] = indio_dev->channels[i].address;
>>>> @@ -57,10 +51,8 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>>>  			u8 *rx_array;
>>>>  			rx_array = kmalloc(byte_for_channel * num_data_channels,
>>>>  					   GFP_KERNEL);
>>>> -			if (!rx_array) {
>>>> -				len = -ENOMEM;
>>>> -				goto st_sensors_free_memory;
>>>> -			}
>>>> +			if (!rx_array)
>>>> +				return -ENOMEM;
>>>>  
>>>>  			len = sdata->tf->read_multiple_byte(&sdata->tb,
>>>>  				sdata->dev, addr[0],
>>>> @@ -68,7 +60,7 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>>>  				rx_array, sdata->multiread_bit);
>>>>  			if (len < 0) {
>>>>  				kfree(rx_array);
>>>> -				goto st_sensors_free_memory;
>>>> +				return len;
>>>>  			}
>>>>  
>>>>  			for (i = 0; i < n * byte_for_channel; i++) {
>>>> @@ -87,17 +79,11 @@ int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>>>  			buf, sdata->multiread_bit);
>>>>  		break;
>>>>  	default:
>>>> -		len = -EINVAL;
>>>> -		goto st_sensors_free_memory;
>>>> -	}
>>>> -	if (len != byte_for_channel * n) {
>>>> -		len = -EIO;
>>>> -		goto st_sensors_free_memory;
>>>> +		return -EINVAL;
>>>>  	}
>>>> +	if (len != byte_for_channel * n)
>>>> +		return -EIO;
>>>>  
>>>> -st_sensors_free_memory:
>>>> -	kfree(addr);
>>>> -st_sensors_get_buffer_element_error:
>>>>  	return len;
>>>>  }
>>>>  EXPORT_SYMBOL(st_sensors_get_buffer_element);
>>>> -- 
>>>> 2.4.3
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>