[PATCH 1/4] iio: buffer: Fix demux table creation

[PATCH 1/4] iio: buffer: Fix demux table creation

[Lars-Peter Clausen]

When creating the demux table we need to iterate over the selected scan mask for
the buffer to get the samples which should be copied to destination buffer.
Right now the code uses the mask which contains all active channels, which means
the demux table contains entries which causes it to copy all the samples from
source to destination buffer one by one without doing any demuxing.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 2952ee0..0472ee2 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -963,7 +963,7 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 
 	/* Now we have the two masks, work from least sig and build up sizes */
 	for_each_set_bit(out_ind,
-			 indio_dev->active_scan_mask,
+			 buffer->scan_mask,
 			 indio_dev->masklength) {
 		in_ind = find_next_bit(indio_dev->active_scan_mask,
 				       indio_dev->masklength,
-- 
1.8.0

[PATCH 2/4] iio: buffer: Use roundup() instead of ALIGN()

[Lars-Peter Clausen]

ALIGN() only works correctly if the alignment is a power of two. Some drivers
use 3 bytes for the storage size of the word in which case ALIGN() will cause
incorrect results. Use the more generic roundup() instead.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
This at least makes the rules for alignment predictable and consistent. But
mixing 3 bytes words with other word sizes will still result in strange layouts.
E.g. 4-byte word, 3-byte word will result in 4-byte word, 2-byte alignment gap,
3-byte word.
---
 drivers/iio/industrialio-buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 0472ee2..6da5272 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -486,7 +486,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 				ch->scan_type.repeat;
 		else
 			length = ch->scan_type.storagebits / 8;
-		bytes = ALIGN(bytes, length);
+		bytes = roundup(bytes, length);
 		bytes += length;
 	}
 	if (timestamp) {
@@ -497,7 +497,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 				ch->scan_type.repeat;
 		else
 			length = ch->scan_type.storagebits / 8;
-		bytes = ALIGN(bytes, length);
+		bytes = roundup(bytes, length);
 		bytes += length;
 	}
 	return bytes;
-- 
1.8.0

Re: [PATCH 2/4] iio: buffer: Use roundup() instead of ALIGN()

[Jonathan Cameron]

On 17/07/14 16:59, Lars-Peter Clausen wrote:
> ALIGN() only works correctly if the alignment is a power of two. Some drivers
> use 3 bytes for the storage size of the word in which case ALIGN() will cause
> incorrect results. Use the more generic roundup() instead.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Gah.  Which driver is using a non power of two storage size?  I thought I'd caught
all of those at review.  As you've noted it's a very bad idea.

 From a quick dubious grep I have:
dac/ad5791.c (not effected as such given we aren't using this stuff for output)
pressure/st_pressure_core.c

I'd be tempted to fix those up rather than change this and introduce the
fun you've pointed out below.

What do you think?
> ---
> This at least makes the rules for alignment predictable and consistent. But
> mixing 3 bytes words with other word sizes will still result in strange layouts.
> E.g. 4-byte word, 3-byte word will result in 4-byte word, 2-byte alignment gap,
> 3-byte word.

> ---
>   drivers/iio/industrialio-buffer.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 0472ee2..6da5272 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -486,7 +486,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>   				ch->scan_type.repeat;
>   		else
>   			length = ch->scan_type.storagebits / 8;
> -		bytes = ALIGN(bytes, length);
> +		bytes = roundup(bytes, length);
>   		bytes += length;
>   	}
>   	if (timestamp) {
> @@ -497,7 +497,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>   				ch->scan_type.repeat;
>   		else
>   			length = ch->scan_type.storagebits / 8;
> -		bytes = ALIGN(bytes, length);
> +		bytes = roundup(bytes, length);
>   		bytes += length;
>   	}
>   	return bytes;
>

Re: [PATCH 2/4] iio: buffer: Use roundup() instead of ALIGN()

[Lars-Peter Clausen]

On 07/20/2014 05:08 PM, Jonathan Cameron wrote:
> On 17/07/14 16:59, Lars-Peter Clausen wrote:
>> ALIGN() only works correctly if the alignment is a power of two. Some drivers
>> use 3 bytes for the storage size of the word in which case ALIGN() will cause
>> incorrect results. Use the more generic roundup() instead.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Gah.  Which driver is using a non power of two storage size?  I thought I'd
> caught
> all of those at review.  As you've noted it's a very bad idea.
>
>  From a quick dubious grep I have:
> dac/ad5791.c (not effected as such given we aren't using this stuff for output)
> pressure/st_pressure_core.c

Yes, this were the two I was seeing as well. But its noteworthy that the 
ad5791 driver doesn't have any buffer support yet, so that's not really an 
issue.

>
> I'd be tempted to fix those up rather than change this and introduce the
> fun you've pointed out below.
>
> What do you think?

I'm all for fixing this up and also adding a big return -EINVAL in case 
somebody tries to register a channel with a non power of two storage size.

But I can also see potential issues here with hardware that has 24-bit 
samples and tightly packs them when transferring them over the bus. E.g. a 2 
channel sensor with two 24-bit that are transferred in a 48-bit word. 
Especially for high-speed converters having a extra step that adds a 8-bit 
gap after/before each 24-bit word will be a undesirable performance 
overhead. But maybe to properly support this we'll need an extension that 
allows a driver to explicitly define its data layout rather than implicitly 
inferring it from  the sample sizes.

Btw. looking at drivers/iio/common/st_sensors/st_sensors_buffer.c it seems 
as if the driver doesn't support having multiple channels with different 
storage sizes for the same chip.

>> ---
>> This at least makes the rules for alignment predictable and consistent. But
>> mixing 3 bytes words with other word sizes will still result in strange
>> layouts.
>> E.g. 4-byte word, 3-byte word will result in 4-byte word, 2-byte alignment
>> gap,
>> 3-byte word.
>
>> ---
>>   drivers/iio/industrialio-buffer.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c
>> b/drivers/iio/industrialio-buffer.c
>> index 0472ee2..6da5272 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -486,7 +486,7 @@ static int iio_compute_scan_bytes(struct iio_dev
>> *indio_dev,
>>                   ch->scan_type.repeat;
>>           else
>>               length = ch->scan_type.storagebits / 8;
>> -        bytes = ALIGN(bytes, length);
>> +        bytes = roundup(bytes, length);
>>           bytes += length;
>>       }
>>       if (timestamp) {
>> @@ -497,7 +497,7 @@ static int iio_compute_scan_bytes(struct iio_dev
>> *indio_dev,
>>                   ch->scan_type.repeat;
>>           else
>>               length = ch->scan_type.storagebits / 8;
>> -        bytes = ALIGN(bytes, length);
>> +        bytes = roundup(bytes, length);
>>           bytes += length;
>>       }
>>       return bytes;
>>
>

Re: [PATCH 2/4] iio: buffer: Use roundup() instead of ALIGN()

[Jonathan Cameron]

On 21/07/14 09:46, Lars-Peter Clausen wrote:
> On 07/20/2014 05:08 PM, Jonathan Cameron wrote:
>> On 17/07/14 16:59, Lars-Peter Clausen wrote:
>>> ALIGN() only works correctly if the alignment is a power of two. Some drivers
>>> use 3 bytes for the storage size of the word in which case ALIGN() will cause
>>> incorrect results. Use the more generic roundup() instead.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> Gah.  Which driver is using a non power of two storage size?  I thought I'd
>> caught
>> all of those at review.  As you've noted it's a very bad idea.
>>
>>  From a quick dubious grep I have:
>> dac/ad5791.c (not effected as such given we aren't using this stuff for output)
>> pressure/st_pressure_core.c
>

> Yes, this were the two I was seeing as well. But its noteworthy that
> the ad5791 driver doesn't have any buffer support yet, so that's not
> really an issue.
>>
>> I'd be tempted to fix those up rather than change this and introduce the
>> fun you've pointed out below.
>>

>> What do you think?
>
> I'm all for fixing this up and also adding a big return -EINVAL in
> case somebody tries to register a channel with a non power of two
> storage size.
Sounds good to me.  That way we don't rely on our clearly less than
perfect ability to pick it up in review.
>
> But I can also see potential issues here with hardware that has
> 24-bit samples and tightly packs them when transferring them over the
> bus. E.g. a 2 channel sensor with two 24-bit that are transferred in
> a 48-bit word. Especially for high-speed converters having a extra
> step that adds a 8-bit gap after/before each 24-bit word will be a
> undesirable performance overhead.
If it's going through the demux then things aren't going to be all that
fast anyway.
> But maybe to properly support this
> we'll need an extension that allows a driver to explicitly define its
> data layout rather than implicitly inferring it from the sample
> sizes.
Possibly though then do we just prevent demux on these because it
could be very costly....
>
> Btw. looking at drivers/iio/common/st_sensors/st_sensors_buffer.c it
> seems as if the driver doesn't support having multiple channels with
> different storage sizes for the same chip.
That sounds bad.  Denis?
>
>>> ---
>>> This at least makes the rules for alignment predictable and consistent. But
>>> mixing 3 bytes words with other word sizes will still result in strange
>>> layouts.
>>> E.g. 4-byte word, 3-byte word will result in 4-byte word, 2-byte alignment
>>> gap,
>>> 3-byte word.
>>
>>> ---
>>>   drivers/iio/industrialio-buffer.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c
>>> b/drivers/iio/industrialio-buffer.c
>>> index 0472ee2..6da5272 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -486,7 +486,7 @@ static int iio_compute_scan_bytes(struct iio_dev
>>> *indio_dev,
>>>                   ch->scan_type.repeat;
>>>           else
>>>               length = ch->scan_type.storagebits / 8;
>>> -        bytes = ALIGN(bytes, length);
>>> +        bytes = roundup(bytes, length);
>>>           bytes += length;
>>>       }
>>>       if (timestamp) {
>>> @@ -497,7 +497,7 @@ static int iio_compute_scan_bytes(struct iio_dev
>>> *indio_dev,
>>>                   ch->scan_type.repeat;
>>>           else
>>>               length = ch->scan_type.storagebits / 8;
>>> -        bytes = ALIGN(bytes, length);
>>> +        bytes = roundup(bytes, length);
>>>           bytes += length;
>>>       }
>>>       return bytes;
>>>
>>
>
> --
> 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

[PATCH 3/4] iio: buffer: Use roundup() instead of open-coding it

[Lars-Peter Clausen]

Makes the code slightly shorter and a bit easier to understand.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-buffer.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 6da5272..5063703 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -979,9 +979,7 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 			else
 				length = ch->scan_type.storagebits / 8;
 			/* Make sure we are aligned */
-			in_loc += length;
-			if (in_loc % length)
-				in_loc += length - in_loc % length;
+			in_loc = roundup(in_loc, length) + length;
 		}
 		p = kmalloc(sizeof(*p), GFP_KERNEL);
 		if (p == NULL) {
@@ -994,10 +992,8 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 				ch->scan_type.repeat;
 		else
 			length = ch->scan_type.storagebits / 8;
-		if (out_loc % length)
-			out_loc += length - out_loc % length;
-		if (in_loc % length)
-			in_loc += length - in_loc % length;
+		out_loc = roundup(out_loc, length);
+		in_loc = roundup(in_loc, length);
 		p->from = in_loc;
 		p->to = out_loc;
 		p->length = length;
@@ -1019,10 +1015,8 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 				ch->scan_type.repeat;
 		else
 			length = ch->scan_type.storagebits / 8;
-		if (out_loc % length)
-			out_loc += length - out_loc % length;
-		if (in_loc % length)
-			in_loc += length - in_loc % length;
+		out_loc = roundup(out_loc, length);
+		in_loc = roundup(in_loc, length);
 		p->from = in_loc;
 		p->to = out_loc;
 		p->length = length;
-- 
1.8.0

Re: [PATCH 3/4] iio: buffer: Use roundup() instead of open-coding it

[Jonathan Cameron]

On 17/07/14 16:59, Lars-Peter Clausen wrote:
> Makes the code slightly shorter and a bit easier to understand.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Indeed, a sensible cleanup.

Applied to the togreg branch of iio.git

Thanks,

J
> ---
>   drivers/iio/industrialio-buffer.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 6da5272..5063703 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -979,9 +979,7 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   			else
>   				length = ch->scan_type.storagebits / 8;
>   			/* Make sure we are aligned */
> -			in_loc += length;
> -			if (in_loc % length)
> -				in_loc += length - in_loc % length;
> +			in_loc = roundup(in_loc, length) + length;
>   		}
>   		p = kmalloc(sizeof(*p), GFP_KERNEL);
>   		if (p == NULL) {
> @@ -994,10 +992,8 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   				ch->scan_type.repeat;
>   		else
>   			length = ch->scan_type.storagebits / 8;
> -		if (out_loc % length)
> -			out_loc += length - out_loc % length;
> -		if (in_loc % length)
> -			in_loc += length - in_loc % length;
> +		out_loc = roundup(out_loc, length);
> +		in_loc = roundup(in_loc, length);
>   		p->from = in_loc;
>   		p->to = out_loc;
>   		p->length = length;
> @@ -1019,10 +1015,8 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   				ch->scan_type.repeat;
>   		else
>   			length = ch->scan_type.storagebits / 8;
> -		if (out_loc % length)
> -			out_loc += length - out_loc % length;
> -		if (in_loc % length)
> -			in_loc += length - in_loc % length;
> +		out_loc = roundup(out_loc, length);
> +		in_loc = roundup(in_loc, length);
>   		p->from = in_loc;
>   		p->to = out_loc;
>   		p->length = length;
>

[PATCH 4/4] iio: buffer: Coalesce adjacent demux table entries

[Lars-Peter Clausen]

When copying multiple multiple samples that are adjacent in both the source as
well as the destination buffer, instead of creating a new demux table entry for
each sample just increase the length of the previous entry by the size of the
new sample. This makes the demuxing process slightly more efficient.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-buffer.c | 47 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5063703..779e5b3 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -942,13 +942,34 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
 }
 EXPORT_SYMBOL_GPL(iio_push_to_buffers);
 
+static int iio_buffer_add_demux(struct iio_buffer *buffer,
+	struct iio_demux_table **p, unsigned int in_loc, unsigned int out_loc,
+	unsigned int length)
+{
+
+	if (*p && (*p)->from + (*p)->length == in_loc &&
+		(*p)->to + (*p)->length == out_loc) {
+		(*p)->length += length;
+	} else {
+		*p = kmalloc(sizeof(*p), GFP_KERNEL);
+		if (*p == NULL)
+			return -ENOMEM;
+		(*p)->from = in_loc;
+		(*p)->to = out_loc;
+		(*p)->length = length;
+		list_add_tail(&(*p)->l, &buffer->demux_list);
+	}
+
+	return 0;
+}
+
 static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 				   struct iio_buffer *buffer)
 {
 	const struct iio_chan_spec *ch;
 	int ret, in_ind = -1, out_ind, length;
 	unsigned in_loc = 0, out_loc = 0;
-	struct iio_demux_table *p;
+	struct iio_demux_table *p = NULL;
 
 	/* Clear out any old demux */
 	iio_buffer_demux_free(buffer);
@@ -981,11 +1002,6 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 			/* Make sure we are aligned */
 			in_loc = roundup(in_loc, length) + length;
 		}
-		p = kmalloc(sizeof(*p), GFP_KERNEL);
-		if (p == NULL) {
-			ret = -ENOMEM;
-			goto error_clear_mux_table;
-		}
 		ch = iio_find_channel_from_si(indio_dev, in_ind);
 		if (ch->scan_type.repeat > 1)
 			length = ch->scan_type.storagebits / 8 *
@@ -994,20 +1010,14 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 			length = ch->scan_type.storagebits / 8;
 		out_loc = roundup(out_loc, length);
 		in_loc = roundup(in_loc, length);
-		p->from = in_loc;
-		p->to = out_loc;
-		p->length = length;
-		list_add_tail(&p->l, &buffer->demux_list);
+		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
+		if (ret)
+			goto error_clear_mux_table;
 		out_loc += length;
 		in_loc += length;
 	}
 	/* Relies on scan_timestamp being last */
 	if (buffer->scan_timestamp) {
-		p = kmalloc(sizeof(*p), GFP_KERNEL);
-		if (p == NULL) {
-			ret = -ENOMEM;
-			goto error_clear_mux_table;
-		}
 		ch = iio_find_channel_from_si(indio_dev,
 			indio_dev->scan_index_timestamp);
 		if (ch->scan_type.repeat > 1)
@@ -1017,10 +1027,9 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 			length = ch->scan_type.storagebits / 8;
 		out_loc = roundup(out_loc, length);
 		in_loc = roundup(in_loc, length);
-		p->from = in_loc;
-		p->to = out_loc;
-		p->length = length;
-		list_add_tail(&p->l, &buffer->demux_list);
+		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
+		if (ret)
+			goto error_clear_mux_table;
 		out_loc += length;
 		in_loc += length;
 	}
-- 
1.8.0

Re: [PATCH 4/4] iio: buffer: Coalesce adjacent demux table entries

[Jonathan Cameron]

On 17/07/14 16:59, Lars-Peter Clausen wrote:
> When copying multiple multiple samples that are adjacent in both the source as
> well as the destination buffer, instead of creating a new demux table entry for
> each sample just increase the length of the previous entry by the size of the
> new sample. This makes the demuxing process slightly more efficient.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Nice.  Will hold these last two until the first one is in and we have decided
what to do about the second one.

J
> ---
>   drivers/iio/industrialio-buffer.c | 47 +++++++++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5063703..779e5b3 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -942,13 +942,34 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
>   }
>   EXPORT_SYMBOL_GPL(iio_push_to_buffers);
>
> +static int iio_buffer_add_demux(struct iio_buffer *buffer,
> +	struct iio_demux_table **p, unsigned int in_loc, unsigned int out_loc,
> +	unsigned int length)
> +{
> +
> +	if (*p && (*p)->from + (*p)->length == in_loc &&
> +		(*p)->to + (*p)->length == out_loc) {
> +		(*p)->length += length;
> +	} else {
> +		*p = kmalloc(sizeof(*p), GFP_KERNEL);
> +		if (*p == NULL)
> +			return -ENOMEM;
> +		(*p)->from = in_loc;
> +		(*p)->to = out_loc;
> +		(*p)->length = length;
> +		list_add_tail(&(*p)->l, &buffer->demux_list);
> +	}
> +
> +	return 0;
> +}
> +
>   static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   				   struct iio_buffer *buffer)
>   {
>   	const struct iio_chan_spec *ch;
>   	int ret, in_ind = -1, out_ind, length;
>   	unsigned in_loc = 0, out_loc = 0;
> -	struct iio_demux_table *p;
> +	struct iio_demux_table *p = NULL;
>
>   	/* Clear out any old demux */
>   	iio_buffer_demux_free(buffer);
> @@ -981,11 +1002,6 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   			/* Make sure we are aligned */
>   			in_loc = roundup(in_loc, length) + length;
>   		}
> -		p = kmalloc(sizeof(*p), GFP_KERNEL);
> -		if (p == NULL) {
> -			ret = -ENOMEM;
> -			goto error_clear_mux_table;
> -		}
>   		ch = iio_find_channel_from_si(indio_dev, in_ind);
>   		if (ch->scan_type.repeat > 1)
>   			length = ch->scan_type.storagebits / 8 *
> @@ -994,20 +1010,14 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   			length = ch->scan_type.storagebits / 8;
>   		out_loc = roundup(out_loc, length);
>   		in_loc = roundup(in_loc, length);
> -		p->from = in_loc;
> -		p->to = out_loc;
> -		p->length = length;
> -		list_add_tail(&p->l, &buffer->demux_list);
> +		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
> +		if (ret)
> +			goto error_clear_mux_table;
>   		out_loc += length;
>   		in_loc += length;
>   	}
>   	/* Relies on scan_timestamp being last */
>   	if (buffer->scan_timestamp) {
> -		p = kmalloc(sizeof(*p), GFP_KERNEL);
> -		if (p == NULL) {
> -			ret = -ENOMEM;
> -			goto error_clear_mux_table;
> -		}
>   		ch = iio_find_channel_from_si(indio_dev,
>   			indio_dev->scan_index_timestamp);
>   		if (ch->scan_type.repeat > 1)
> @@ -1017,10 +1027,9 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>   			length = ch->scan_type.storagebits / 8;
>   		out_loc = roundup(out_loc, length);
>   		in_loc = roundup(in_loc, length);
> -		p->from = in_loc;
> -		p->to = out_loc;
> -		p->length = length;
> -		list_add_tail(&p->l, &buffer->demux_list);
> +		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
> +		if (ret)
> +			goto error_clear_mux_table;
>   		out_loc += length;
>   		in_loc += length;
>   	}
>

Re: [PATCH 4/4] iio: buffer: Coalesce adjacent demux table entries

[Jonathan Cameron]

On 20/07/14 16:14, Jonathan Cameron wrote:
> On 17/07/14 16:59, Lars-Peter Clausen wrote:
>> When copying multiple multiple samples that are adjacent in both the source as
>> well as the destination buffer, instead of creating a new demux table entry for
>> each sample just increase the length of the previous entry by the size of the
>> new sample. This makes the demuxing process slightly more efficient.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Nice.  Will hold these last two until the first one is in and we have decided
> what to do about the second one.
Applied to the togreg branch of iio.git.

Given Linus' comments on the last rc, there is unlikely to be another IIO pull
request until after the merge window.

Jonathan
>
> J
>> ---
>>   drivers/iio/industrialio-buffer.c | 47 +++++++++++++++++++++++----------------
>>   1 file changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index 5063703..779e5b3 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -942,13 +942,34 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
>>   }
>>   EXPORT_SYMBOL_GPL(iio_push_to_buffers);
>>
>> +static int iio_buffer_add_demux(struct iio_buffer *buffer,
>> +    struct iio_demux_table **p, unsigned int in_loc, unsigned int out_loc,
>> +    unsigned int length)
>> +{
>> +
>> +    if (*p && (*p)->from + (*p)->length == in_loc &&
>> +        (*p)->to + (*p)->length == out_loc) {
>> +        (*p)->length += length;
>> +    } else {
>> +        *p = kmalloc(sizeof(*p), GFP_KERNEL);
>> +        if (*p == NULL)
>> +            return -ENOMEM;
>> +        (*p)->from = in_loc;
>> +        (*p)->to = out_loc;
>> +        (*p)->length = length;
>> +        list_add_tail(&(*p)->l, &buffer->demux_list);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>>                      struct iio_buffer *buffer)
>>   {
>>       const struct iio_chan_spec *ch;
>>       int ret, in_ind = -1, out_ind, length;
>>       unsigned in_loc = 0, out_loc = 0;
>> -    struct iio_demux_table *p;
>> +    struct iio_demux_table *p = NULL;
>>
>>       /* Clear out any old demux */
>>       iio_buffer_demux_free(buffer);
>> @@ -981,11 +1002,6 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>>               /* Make sure we are aligned */
>>               in_loc = roundup(in_loc, length) + length;
>>           }
>> -        p = kmalloc(sizeof(*p), GFP_KERNEL);
>> -        if (p == NULL) {
>> -            ret = -ENOMEM;
>> -            goto error_clear_mux_table;
>> -        }
>>           ch = iio_find_channel_from_si(indio_dev, in_ind);
>>           if (ch->scan_type.repeat > 1)
>>               length = ch->scan_type.storagebits / 8 *
>> @@ -994,20 +1010,14 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>>               length = ch->scan_type.storagebits / 8;
>>           out_loc = roundup(out_loc, length);
>>           in_loc = roundup(in_loc, length);
>> -        p->from = in_loc;
>> -        p->to = out_loc;
>> -        p->length = length;
>> -        list_add_tail(&p->l, &buffer->demux_list);
>> +        ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
>> +        if (ret)
>> +            goto error_clear_mux_table;
>>           out_loc += length;
>>           in_loc += length;
>>       }
>>       /* Relies on scan_timestamp being last */
>>       if (buffer->scan_timestamp) {
>> -        p = kmalloc(sizeof(*p), GFP_KERNEL);
>> -        if (p == NULL) {
>> -            ret = -ENOMEM;
>> -            goto error_clear_mux_table;
>> -        }
>>           ch = iio_find_channel_from_si(indio_dev,
>>               indio_dev->scan_index_timestamp);
>>           if (ch->scan_type.repeat > 1)
>> @@ -1017,10 +1027,9 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>>               length = ch->scan_type.storagebits / 8;
>>           out_loc = roundup(out_loc, length);
>>           in_loc = roundup(in_loc, length);
>> -        p->from = in_loc;
>> -        p->to = out_loc;
>> -        p->length = length;
>> -        list_add_tail(&p->l, &buffer->demux_list);
>> +        ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
>> +        if (ret)
>> +            goto error_clear_mux_table;
>>           out_loc += length;
>>           in_loc += length;
>>       }
>>
>
> --
> 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: buffer: Fix demux table creation

[Jonathan Cameron]

On 17/07/14 16:59, Lars-Peter Clausen wrote:
> When creating the demux table we need to iterate over the selected scan mask for
> the buffer to get the samples which should be copied to destination buffer.
> Right now the code uses the mask which contains all active channels, which means
> the demux table contains entries which causes it to copy all the samples from
> source to destination buffer one by one without doing any demuxing.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Ouch.  Applied to the fixes-togreg branch of iio.git and marked
for stable.

How we managed to fail to see the effects of this is beyond me.
I guess the fact that the timestamp is handled separately made
it quite a bit less obvious than it otherwise would have been.

Thanks,

Jonathan
> ---
>   drivers/iio/industrialio-buffer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 2952ee0..0472ee2 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -963,7 +963,7 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
>
>   	/* Now we have the two masks, work from least sig and build up sizes */
>   	for_each_set_bit(out_ind,
> -			 indio_dev->active_scan_mask,
> +			 buffer->scan_mask,
>   			 indio_dev->masklength) {
>   		in_ind = find_next_bit(indio_dev->active_scan_mask,
>   				       indio_dev->masklength,
>