[PATCH 0/4] Add valid sample channel

[PATCH 0/4] Add valid sample channel

[Vlad Dogaru]

This is an attempt to address the problem of buffering in devices which have
different scan frequencies for different channels.

The first patch is a minor cleanup of the dummy code, the second one a
safeguard against duplicate scan_index channels.

The final two patches add a new channel and are RFC.

Vlad Dogaru (4):
  iio: dummy: set scan_index for unbuffered channels
  iio: ensure scan index is unique at buffer register
  iio: add valid sample channel
  iio: illustrate the use of a valid sample channel

 drivers/iio/industrialio-buffer.c             | 28 ++++++++++++++++++++++++
 drivers/iio/industrialio-core.c               |  1 +
 drivers/staging/iio/iio_simple_dummy.c        | 21 +++++++++++++++---
 drivers/staging/iio/iio_simple_dummy_buffer.c | 31 +++++++++++++++++++++++++--
 include/linux/iio/types.h                     |  1 +
 5 files changed, 77 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH 1/4] iio: dummy: set scan_index for unbuffered channels

[Vlad Dogaru]

If we do not set scan_index explicitly it will default to 0, causing
more than one channel to have the same scan_index.  This does not make
sense, as scan_index is used to order values in a buffer sample.

Set the activity and voltage output channels to unbuffered
(scan_index = -1) to fix this.

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/staging/iio/iio_simple_dummy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
index 10a9e08..2948900 100644
--- a/drivers/staging/iio/iio_simple_dummy.c
+++ b/drivers/staging/iio/iio_simple_dummy.c
@@ -242,6 +242,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
 		.output = 1,
 		.indexed = 1,
 		.channel = 0,
+		.scan_index = -1,
 	},
 	{
 		.type = IIO_STEPS,
@@ -259,6 +260,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
 		.modified = 1,
 		.channel2 = IIO_MOD_RUNNING,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = -1,
 #ifdef CONFIG_IIO_SIMPLE_DUMMY_EVENTS
 		.event_spec = &iio_running_event,
 		.num_event_specs = 1,
@@ -269,6 +271,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
 		.modified = 1,
 		.channel2 = IIO_MOD_WALKING,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = -1,
 #ifdef CONFIG_IIO_SIMPLE_DUMMY_EVENTS
 		.event_spec = &iio_walking_event,
 		.num_event_specs = 1,
-- 
1.9.1

[PATCH 2/4] iio: ensure scan index is unique at buffer register

[Vlad Dogaru]

Having two or more channels with the same positive scan_index field
makes no sense.  Prevent this situation by failing to register a buffer
if two scan indexes are identical.

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index f971f79..422b29e 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -309,6 +309,30 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int iio_check_unique_scan_index(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *channels,
+				       int num_channels)
+{
+	int i, j;
+
+	for (i = 0; i < num_channels - 1; i++) {
+		if (channels[i].scan_index < 0)
+			continue;
+		for (j = i + 1; j < num_channels; j++) {
+			if (channels[j].scan_index < 0)
+				continue;
+			if (channels[i].scan_index == channels[j].scan_index) {
+				dev_err(&indio_dev->dev,
+					"duplicate scan index %d\n",
+					channels[i].scan_index);
+				return -EINVAL;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static const char * const iio_scan_elements_group_name = "scan_elements";
 
 int iio_buffer_register(struct iio_dev *indio_dev,
@@ -320,6 +344,10 @@ int iio_buffer_register(struct iio_dev *indio_dev,
 	struct iio_buffer *buffer = indio_dev->buffer;
 	int ret, i, attrn, attrcount, attrcount_orig = 0;
 
+	ret = iio_check_unique_scan_index(indio_dev, channels, num_channels);
+	if (ret < 0)
+		return ret;
+
 	if (buffer->attrs)
 		indio_dev->groups[indio_dev->groupcounter++] = buffer->attrs;
 
-- 
1.9.1

[RFC PATCH 3/4] iio: add valid sample channel

[Vlad Dogaru]

When a sensor has channels which are sampled at different rates, the
situation can arise when a new buffer sample was triggered, but some of
the channels are not yet ready to be read.

To avoid serving bogus values, we can use a "valid sample" channel.
This value is a bitmap where bit N signifies that the Nth value in the
current buffer sample is a valid reading.

This new channel only makes sense for buffering, so it should have no
other files associated, save for the ones in the scan_elements
directory.  It is also entirely optional to define the channel.  If it
is not defined, all the values in the current sample should be
interpreted as valid, thus preserving semantics for existing drivers.

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/industrialio-core.c | 1 +
 include/linux/iio/types.h       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 45bb3a4..b6bb1b3 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -72,6 +72,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_HUMIDITYRELATIVE] = "humidityrelative",
 	[IIO_ACTIVITY] = "activity",
 	[IIO_STEPS] = "steps",
+	[IIO_VALIDSAMPLE] = "validsample",
 };
 
 static const char * const iio_modifier_names[] = {
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 904dcbb..c75b724 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -32,6 +32,7 @@ enum iio_chan_type {
 	IIO_HUMIDITYRELATIVE,
 	IIO_ACTIVITY,
 	IIO_STEPS,
+	IIO_VALIDSAMPLE,
 };
 
 enum iio_modifier {
-- 
1.9.1

[RFC PATCH 4/4] iio: illustrate the use of a valid sample channel

[Vlad Dogaru]

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/staging/iio/iio_simple_dummy.c        | 18 +++++++++++++---
 drivers/staging/iio/iio_simple_dummy_buffer.c | 31 +++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
index 2948900..0e2ea40 100644
--- a/drivers/staging/iio/iio_simple_dummy.c
+++ b/drivers/staging/iio/iio_simple_dummy.c
@@ -230,11 +230,22 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
 			.shift = 0, /* zero shift */
 		},
 	},
+	{
+		.type = IIO_VALIDSAMPLE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = 4,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.shift = 0,
+		},
+	},
 	/*
-	 * Convenience macro for timestamps. 4 is the index in
+	 * Convenience macro for timestamps. 5 is the index in
 	 * the buffer.
 	 */
-	IIO_CHAN_SOFT_TIMESTAMP(4),
+	IIO_CHAN_SOFT_TIMESTAMP(5),
 	/* DAC channel out_voltage0_raw */
 	{
 		.type = IIO_VOLTAGE,
@@ -647,7 +658,8 @@ static int iio_dummy_probe(int index)
 	 * number of channels by 1.
 	 */
 	ret = iio_simple_dummy_configure_buffer(indio_dev,
-						iio_dummy_channels, 5);
+						iio_dummy_channels,
+						indio_dev->num_channels);
 	if (ret < 0)
 		goto error_unregister_events;
 
diff --git a/drivers/staging/iio/iio_simple_dummy_buffer.c b/drivers/staging/iio/iio_simple_dummy_buffer.c
index fd74f91..89ed87c 100644
--- a/drivers/staging/iio/iio_simple_dummy_buffer.c
+++ b/drivers/staging/iio/iio_simple_dummy_buffer.c
@@ -48,6 +48,8 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	int len = 0;
 	u16 *data;
+	int validsample_idx = -1;
+	u16 validsample = 0;
 
 	data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
 	if (data == NULL)
@@ -78,12 +80,37 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p)
 		     i++, j++) {
 			j = find_next_bit(indio_dev->active_scan_mask,
 					  indio_dev->masklength, j);
-			/* random access read from the 'device' */
-			data[i] = fakedata[j];
+			if (indio_dev->channels[j].type == IIO_VALIDSAMPLE)
+				validsample_idx = i;
+			else
+				/* random access read from the 'device' */
+				data[i] = fakedata[j];
+			/*
+			 * Let's assume that the acceleration channel is
+			 * sampled twice as slow as the other ones.
+			 * Normally, you would check your device to
+			 * learn if data is available.
+			 */
+			if (indio_dev->channels[j].type == IIO_ACCEL) {
+				static bool accel_valid;
+				if (accel_valid) {
+					validsample |= 1 << i;
+					data[i] = fakedata[j];
+				} else {
+					data[i] = 0;
+				}
+				accel_valid = !accel_valid;
+			} else {
+				validsample |= 1 << i;
+				data[i] = fakedata[j];
+			}
 			len += 2;
 		}
 	}
 
+	if (validsample_idx != -1)
+		memcpy(data + validsample_idx, &validsample, 2);
+
 	iio_push_to_buffers_with_timestamp(indio_dev, data, iio_get_time_ns());
 
 	kfree(data);
-- 
1.9.1

Re: [PATCH 1/4] iio: dummy: set scan_index for unbuffered channels

[Daniel Baluta]

On Tue, Nov 25, 2014 at 2:48 PM, Vlad Dogaru <vlad.dogaru@intel.com> wrote:
> If we do not set scan_index explicitly it will default to 0, causing
> more than one channel to have the same scan_index.  This does not make
> sense, as scan_index is used to order values in a buffer sample.
>
> Set the activity and voltage output channels to unbuffered
> (scan_index = -1) to fix this.
>
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>

Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>

> ---
>  drivers/staging/iio/iio_simple_dummy.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> index 10a9e08..2948900 100644
> --- a/drivers/staging/iio/iio_simple_dummy.c
> +++ b/drivers/staging/iio/iio_simple_dummy.c
> @@ -242,6 +242,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
>                 .output = 1,
>                 .indexed = 1,
>                 .channel = 0,
> +               .scan_index = -1,
>         },
>         {
>                 .type = IIO_STEPS,
> @@ -259,6 +260,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
>                 .modified = 1,
>                 .channel2 = IIO_MOD_RUNNING,
>                 .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +               .scan_index = -1,
>  #ifdef CONFIG_IIO_SIMPLE_DUMMY_EVENTS
>                 .event_spec = &iio_running_event,
>                 .num_event_specs = 1,
> @@ -269,6 +271,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
>                 .modified = 1,
>                 .channel2 = IIO_MOD_WALKING,
>                 .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +               .scan_index = -1,
>  #ifdef CONFIG_IIO_SIMPLE_DUMMY_EVENTS
>                 .event_spec = &iio_walking_event,
>                 .num_event_specs = 1,
> --
> 1.9.1
>
> --
> 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 0/4] Add valid sample channel

[Lars-Peter Clausen]

On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
> This is an attempt to address the problem of buffering in devices which have
> different scan frequencies for different channels.

Will these frequencies still be related or completely arbitrary?

Re: [PATCH 0/4] Add valid sample channel

[Daniel Baluta]

On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>
>> This is an attempt to address the problem of buffering in devices which
>> have
>> different scan frequencies for different channels.
>
>
> Will these frequencies still be related or completely arbitrary?

For Kionix KMX61 we have the same set of frequencies for
accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
but for example at some point accel can be configured with 100 Hz
and magnetometer with 12.5Hz.

Does this matter?

Daniel.

Re: [PATCH 0/4] Add valid sample channel

[Lars-Peter Clausen]

On 11/25/2014 05:55 PM, Daniel Baluta wrote:
> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>
>>> This is an attempt to address the problem of buffering in devices which
>>> have
>>> different scan frequencies for different channels.
>>
>>
>> Will these frequencies still be related or completely arbitrary?
>
> For Kionix KMX61 we have the same set of frequencies for
> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
> but for example at some point accel can be configured with 100 Hz
> and magnetometer with 12.5Hz.
>
> Does this matter?

I'm wondering if it makes more sense to register multiple buffers or devices 
if the frequencies are completely unrelated. E.g. in the KMX61 case it looks 
as if its simply two logical devices in the same physical package.

- Lars

Re: [PATCH 0/4] Add valid sample channel

[Jonathan Cameron]

On November 25, 2014 5:00:11 PM GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 11/25/2014 05:55 PM, Daniel Baluta wrote:
>> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de>
>wrote:
>>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>>
>>>> This is an attempt to address the problem of buffering in devices
>which
>>>> have
>>>> different scan frequencies for different channels.
>>>
>>>
>>> Will these frequencies still be related or completely arbitrary?
>>
>> For Kionix KMX61 we have the same set of frequencies for
>> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
>> but for example at some point accel can be configured with 100 Hz
>> and magnetometer with 12.5Hz.
>>
>> Does this matter?
>
>I'm wondering if it makes more sense to register multiple buffers or
>devices 
>if the frequencies are completely unrelated. E.g. in the KMX61 case it
>looks 
>as if its simply two logical devices in the same physical package.
>
>- Lars
That is how we have handled this so far.
Sometimes we have separate iio_devices for
the logical functions e.g. hid-sensors and sometimes just multiple buffers.. I am
 struggling to find it now but one of the ADC drivers had a insanely complex nest
 sampling arrangement requiring 8 buffers. Final driver might have been simplified though...  We might still need a few tweaks
 for multiple buffers not to having naming clashes....  Moving house so only have phone to hand!

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

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 0/4] Add valid sample channel

[Octavian Purdila]

On Tue, Nov 25, 2014 at 7:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>
>
> On November 25, 2014 5:00:11 PM GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>On 11/25/2014 05:55 PM, Daniel Baluta wrote:
>>> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de>
>>wrote:
>>>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>>>
>>>>> This is an attempt to address the problem of buffering in devices
>>which
>>>>> have
>>>>> different scan frequencies for different channels.
>>>>
>>>>
>>>> Will these frequencies still be related or completely arbitrary?
>>>
>>> For Kionix KMX61 we have the same set of frequencies for
>>> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
>>> but for example at some point accel can be configured with 100 Hz
>>> and magnetometer with 12.5Hz.
>>>
>>> Does this matter?
>>
>>I'm wondering if it makes more sense to register multiple buffers or
>>devices
>>if the frequencies are completely unrelated. E.g. in the KMX61 case it
>>looks
>>as if its simply two logical devices in the same physical package.
>>
>>- Lars
> That is how we have handled this so far.
> Sometimes we have separate iio_devices for
> the logical functions e.g. hid-sensors and sometimes just multiple buffers.. I am
>  struggling to find it now but one of the ADC drivers had a insanely complex nest
>  sampling arrangement requiring 8 buffers. Final driver might have been simplified though...  We might still need a few tweaks
>  for multiple buffers not to having naming clashes....  Moving house so only have phone to hand!
>

The main usage I see for a single device / buffer and multiple
channels is to reduce the user/kernel context switch overhead in a
sensor hub environment with lots of sensors (i.e. do a single read
instead of one for each sensor). The improvements are going to be even
bigger if the sensor hub supports a hardware buffer.

Some people are quite vocal about that, although I didn't see data
that can be used to draw a relevant conclusion yet. Time to do some
experiments, I guess :)

Re: [PATCH 0/4] Add valid sample channel

[Lars-Peter Clausen]

On 11/25/2014 07:27 PM, Octavian Purdila wrote:
> On Tue, Nov 25, 2014 at 7:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>>
>> On November 25, 2014 5:00:11 PM GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 11/25/2014 05:55 PM, Daniel Baluta wrote:
>>>> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de>
>>> wrote:
>>>>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>>>>
>>>>>> This is an attempt to address the problem of buffering in devices
>>> which
>>>>>> have
>>>>>> different scan frequencies for different channels.
>>>>>
>>>>>
>>>>> Will these frequencies still be related or completely arbitrary?
>>>>
>>>> For Kionix KMX61 we have the same set of frequencies for
>>>> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
>>>> but for example at some point accel can be configured with 100 Hz
>>>> and magnetometer with 12.5Hz.
>>>>
>>>> Does this matter?
>>>
>>> I'm wondering if it makes more sense to register multiple buffers or
>>> devices
>>> if the frequencies are completely unrelated. E.g. in the KMX61 case it
>>> looks
>>> as if its simply two logical devices in the same physical package.
>>>
>>> - Lars
>> That is how we have handled this so far.
>> Sometimes we have separate iio_devices for
>> the logical functions e.g. hid-sensors and sometimes just multiple buffers.. I am
>>   struggling to find it now but one of the ADC drivers had a insanely complex nest
>>   sampling arrangement requiring 8 buffers. Final driver might have been simplified though...  We might still need a few tweaks
>>   for multiple buffers not to having naming clashes....  Moving house so only have phone to hand!
>>
>
> The main usage I see for a single device / buffer and multiple
> channels is to reduce the user/kernel context switch overhead in a
> sensor hub environment with lots of sensors (i.e. do a single read
> instead of one for each sensor). The improvements are going to be even
> bigger if the sensor hub supports a hardware buffer.

If those sensors run asynchronously to each other you'll end up with a lot 
of padding data which is quite likely going to negate any context switch 
overhead reduction.

>
> Some people are quite vocal about that, although I didn't see data
> that can be used to draw a relevant conclusion yet. Time to do some
> experiments, I guess :)

Yes...

Re: [PATCH 0/4] Add valid sample channel

[Daniel Baluta]

On Tue, Nov 25, 2014 at 7:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>
>
> On November 25, 2014 5:00:11 PM GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>On 11/25/2014 05:55 PM, Daniel Baluta wrote:
>>> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de>
>>wrote:
>>>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>>>
>>>>> This is an attempt to address the problem of buffering in devices
>>which
>>>>> have
>>>>> different scan frequencies for different channels.
>>>>
>>>>
>>>> Will these frequencies still be related or completely arbitrary?
>>>
>>> For Kionix KMX61 we have the same set of frequencies for
>>> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
>>> but for example at some point accel can be configured with 100 Hz
>>> and magnetometer with 12.5Hz.
>>>
>>> Does this matter?
>>
>>I'm wondering if it makes more sense to register multiple buffers or
>>devices
>>if the frequencies are completely unrelated. E.g. in the KMX61 case it
>>looks
>>as if its simply two logical devices in the same physical package.

I think this makes a lot of sense for kmx61 and offers a cleaner
interface to user space.

>>
>>- Lars
> That is how we have handled this so far.
> Sometimes we have separate iio_devices for
> the logical functions e.g. hid-sensors and sometimes just multiple buffers.. I am
>  struggling to find it now but one of the ADC drivers had a insanely complex nest
>  sampling arrangement requiring 8 buffers. Final driver might have been simplified though...  We might still need a few tweaks
>  for multiple buffers not to having naming clashes....  Moving house so only have phone to hand!

I looked into iio/adc/ and iio/imu/ but couldn't find any driver which
has one IIO device  and multiple buffers. I don't see how this can
work :).

User space application will read /dev/iio:deviceX to get data
samples. How would they know the source of the data samples?

Let me know where is the source code for the 8 buffers ADC driver,
I am very curious about the implementation :).

Jonathan, please drop the KMX61 initial patch from togreg
branch.

https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=d7d787d29148cde12958c2e3765ad3a55dc55eaf

I will send a new version having two IIO devices, one per each sensor.

thanks,
Daniel.

Re: [PATCH 0/4] Add valid sample channel

[Lars-Peter Clausen]

On 11/26/2014 01:58 PM, Daniel Baluta wrote:
> On Tue, Nov 25, 2014 at 7:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>>
>> On November 25, 2014 5:00:11 PM GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 11/25/2014 05:55 PM, Daniel Baluta wrote:
>>>> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de>
>>> wrote:
>>>>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>>>>
>>>>>> This is an attempt to address the problem of buffering in devices
>>> which
>>>>>> have
>>>>>> different scan frequencies for different channels.
>>>>>
>>>>>
>>>>> Will these frequencies still be related or completely arbitrary?
>>>>
>>>> For Kionix KMX61 we have the same set of frequencies for
>>>> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
>>>> but for example at some point accel can be configured with 100 Hz
>>>> and magnetometer with 12.5Hz.
>>>>
>>>> Does this matter?
>>>
>>> I'm wondering if it makes more sense to register multiple buffers or
>>> devices
>>> if the frequencies are completely unrelated. E.g. in the KMX61 case it
>>> looks
>>> as if its simply two logical devices in the same physical package.
>
> I think this makes a lot of sense for kmx61 and offers a cleaner
> interface to user space.
>
>>>
>>> - Lars
>> That is how we have handled this so far.
>> Sometimes we have separate iio_devices for
>> the logical functions e.g. hid-sensors and sometimes just multiple buffers.. I am
>>   struggling to find it now but one of the ADC drivers had a insanely complex nest
>>   sampling arrangement requiring 8 buffers. Final driver might have been simplified though...  We might still need a few tweaks
>>   for multiple buffers not to having naming clashes....  Moving house so only have phone to hand!
>
> I looked into iio/adc/ and iio/imu/ but couldn't find any driver which
> has one IIO device  and multiple buffers. I don't see how this can
> work :).
>
> User space application will read /dev/iio:deviceX to get data
> samples. How would they know the source of the data samples?

What exactly do you mean by source?

>
> Let me know where is the source code for the 8 buffers ADC driver,
> I am very curious about the implementation :).

I think that one never made it into upstream or got reworked before.

- Lars

Re: [PATCH 0/4] Add valid sample channel

[Daniel Baluta]

On Mon, Dec 1, 2014 at 10:06 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/26/2014 01:58 PM, Daniel Baluta wrote:
>>
>> On Tue, Nov 25, 2014 at 7:29 PM, Jonathan Cameron <jic23@kernel.org>
>> wrote:
>>>
>>>
>>>
>>> On November 25, 2014 5:00:11 PM GMT+00:00, Lars-Peter Clausen
>>> <lars@metafoo.de> wrote:
>>>>
>>>> On 11/25/2014 05:55 PM, Daniel Baluta wrote:
>>>>>
>>>>> On Tue, Nov 25, 2014 at 6:36 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>>
>>>> wrote:
>>>>>>
>>>>>> On 11/25/2014 01:48 PM, Vlad Dogaru wrote:
>>>>>>>
>>>>>>>
>>>>>>> This is an attempt to address the problem of buffering in devices
>>>>
>>>> which
>>>>>>>
>>>>>>> have
>>>>>>> different scan frequencies for different channels.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Will these frequencies still be related or completely arbitrary?
>>>>>
>>>>>
>>>>> For Kionix KMX61 we have the same set of frequencies for
>>>>> accelerometer and magnetometer (12.5, 25, 50, 100 Hz, etc)
>>>>> but for example at some point accel can be configured with 100 Hz
>>>>> and magnetometer with 12.5Hz.
>>>>>
>>>>> Does this matter?
>>>>
>>>>
>>>> I'm wondering if it makes more sense to register multiple buffers or
>>>> devices
>>>> if the frequencies are completely unrelated. E.g. in the KMX61 case it
>>>> looks
>>>> as if its simply two logical devices in the same physical package.
>>
>>
>> I think this makes a lot of sense for kmx61 and offers a cleaner
>> interface to user space.
>>
>>>>
>>>> - Lars
>>>
>>> That is how we have handled this so far.
>>> Sometimes we have separate iio_devices for
>>> the logical functions e.g. hid-sensors and sometimes just multiple
>>> buffers.. I am
>>>   struggling to find it now but one of the ADC drivers had a insanely
>>> complex nest
>>>   sampling arrangement requiring 8 buffers. Final driver might have been
>>> simplified though...  We might still need a few tweaks
>>>   for multiple buffers not to having naming clashes....  Moving house so
>>> only have phone to hand!
>>
>>
>> I looked into iio/adc/ and iio/imu/ but couldn't find any driver which
>> has one IIO device  and multiple buffers. I don't see how this can
>> work :).
>>
>> User space application will read /dev/iio:deviceX to get data
>> samples. How would they know the source of the data samples?
>
>
> What exactly do you mean by source?

By source I mean accelerometer or magnetometer in the case of KMX61 sensor.

>
>>
>> Let me know where is the source code for the 8 buffers ADC driver,
>> I am very curious about the implementation :).
>
>
> I think that one never made it into upstream or got reworked before.

Ok :)

Re: [PATCH 0/4] Add valid sample channel

[Lars-Peter Clausen]

On 12/01/2014 09:46 PM, Daniel Baluta wrote:
[...[
>>> User space application will read /dev/iio:deviceX to get data
>>> samples. How would they know the source of the data samples?
>>
>>
>> What exactly do you mean by source?
>
> By source I mean accelerometer or magnetometer in the case of KMX61 sensor.

The channel type of the iio device should make it pretty clear which one it 
is ;) You can also add a suffix to the IIO device name.

Re: [PATCH 0/4] Add valid sample channel

[Daniel Baluta]

On Mon, Dec 1, 2014 at 10:51 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 12/01/2014 09:46 PM, Daniel Baluta wrote:
> [...[
>>>>
>>>> User space application will read /dev/iio:deviceX to get data
>>>> samples. How would they know the source of the data samples?
>>>
>>>
>>>
>>> What exactly do you mean by source?
>>
>>
>> By source I mean accelerometer or magnetometer in the case of KMX61
>> sensor.
>
>
> The channel type of the iio device should make it pretty clear which one it
> is ;) You can also add a suffix to the IIO device name.

Well, this question was asked in the context of using a single IIO device
for both accel / magnetometer.

The user will open /dev/iio:deviceX and read accel/magnetometer samples.
In the case were sampling frequency is the same for both devices everything
is easy. The interesting case is the one when having a single IIO device
with 2 channels (accel/magn) with different sampling rates.

Anyhow, my implementation know uses 2 IIO devices and everything is clear.

Daniel.

Re: [PATCH 2/4] iio: ensure scan index is unique at buffer register

[Hartmut Knaack]

Vlad Dogaru wrote on 25.11.2014 13:48:
> Having two or more channels with the same positive scan_index field
> makes no sense.  Prevent this situation by failing to register a buffer
> if two scan indexes are identical.
> 
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> ---
>  drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index f971f79..422b29e 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -309,6 +309,30 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int iio_check_unique_scan_index(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *channels,
> +				       int num_channels)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < num_channels - 1; i++) {
> +		if (channels[i].scan_index < 0)
> +			continue;
> +		for (j = i + 1; j < num_channels; j++) {
> +			if (channels[j].scan_index < 0)
> +				continue;
This check up here is unnecessary, since channels[i].scan_index is already equal or
greater zero, so the check below results in a false for channels[j].scan_index smaller
than zero and cause a new loop iteration anyway.

> +			if (channels[i].scan_index == channels[j].scan_index) {
> +				dev_err(&indio_dev->dev,
> +					"duplicate scan index %d\n",
> +					channels[i].scan_index);
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static const char * const iio_scan_elements_group_name = "scan_elements";
>  
>  int iio_buffer_register(struct iio_dev *indio_dev,
> @@ -320,6 +344,10 @@ int iio_buffer_register(struct iio_dev *indio_dev,
>  	struct iio_buffer *buffer = indio_dev->buffer;
>  	int ret, i, attrn, attrcount, attrcount_orig = 0;
>  
> +	ret = iio_check_unique_scan_index(indio_dev, channels, num_channels);
> +	if (ret < 0)
> +		return ret;
> +
>  	if (buffer->attrs)
>  		indio_dev->groups[indio_dev->groupcounter++] = buffer->attrs;
>  
> 

Re: [PATCH 1/4] iio: dummy: set scan_index for unbuffered channels

[Jonathan Cameron]

On 25/11/14 16:11, Daniel Baluta wrote:
> On Tue, Nov 25, 2014 at 2:48 PM, Vlad Dogaru <vlad.dogaru@intel.com> wrote:
>> If we do not set scan_index explicitly it will default to 0, causing
>> more than one channel to have the same scan_index.  This does not make
>> sense, as scan_index is used to order values in a buffer sample.
>>
>> Set the activity and voltage output channels to unbuffered
>> (scan_index = -1) to fix this.
>>
>> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> 
> Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
An equivalent change was in Lars' buffer cleanup patches.

You did there first, but his was doing lots of other things as well
so I've applied Lars' version.

Thanks

Jonathan
> 
>> ---
>>  drivers/staging/iio/iio_simple_dummy.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
>> index 10a9e08..2948900 100644
>> --- a/drivers/staging/iio/iio_simple_dummy.c
>> +++ b/drivers/staging/iio/iio_simple_dummy.c
>> @@ -242,6 +242,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
>>                 .output = 1,
>>                 .indexed = 1,
>>                 .channel = 0,
>> +               .scan_index = -1,
>>         },
>>         {
>>                 .type = IIO_STEPS,
>> @@ -259,6 +260,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
>>                 .modified = 1,
>>                 .channel2 = IIO_MOD_RUNNING,
>>                 .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +               .scan_index = -1,
>>  #ifdef CONFIG_IIO_SIMPLE_DUMMY_EVENTS
>>                 .event_spec = &iio_running_event,
>>                 .num_event_specs = 1,
>> @@ -269,6 +271,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
>>                 .modified = 1,
>>                 .channel2 = IIO_MOD_WALKING,
>>                 .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +               .scan_index = -1,
>>  #ifdef CONFIG_IIO_SIMPLE_DUMMY_EVENTS
>>                 .event_spec = &iio_walking_event,
>>                 .num_event_specs = 1,
>> --
>> 1.9.1
>>
>> --
>> 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
> 

Re: [PATCH 2/4] iio: ensure scan index is unique at buffer register

[Jonathan Cameron]

On 06/12/14 20:19, Hartmut Knaack wrote:
> Vlad Dogaru wrote on 25.11.2014 13:48:
>> Having two or more channels with the same positive scan_index field
>> makes no sense.  Prevent this situation by failing to register a buffer
>> if two scan indexes are identical.
>>
>> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
A sensible little sanity check that might avoid a few bugs.

I'm guessing will need a rebase (along with Harmut's point)
given the buffer set from Lars I applied earlier today.

Jonathan
>> ---
>>  drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index f971f79..422b29e 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -309,6 +309,30 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>>  	return ret;
>>  }
>>  
>> +static int iio_check_unique_scan_index(struct iio_dev *indio_dev,
>> +				       const struct iio_chan_spec *channels,
>> +				       int num_channels)
>> +{
>> +	int i, j;
>> +
>> +	for (i = 0; i < num_channels - 1; i++) {
>> +		if (channels[i].scan_index < 0)
>> +			continue;
>> +		for (j = i + 1; j < num_channels; j++) {
>> +			if (channels[j].scan_index < 0)
>> +				continue;
> This check up here is unnecessary, since channels[i].scan_index is already equal or
> greater zero, so the check below results in a false for channels[j].scan_index smaller
> than zero and cause a new loop iteration anyway.
> 
>> +			if (channels[i].scan_index == channels[j].scan_index) {
>> +				dev_err(&indio_dev->dev,
>> +					"duplicate scan index %d\n",
>> +					channels[i].scan_index);
>> +				return -EINVAL;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static const char * const iio_scan_elements_group_name = "scan_elements";
>>  
>>  int iio_buffer_register(struct iio_dev *indio_dev,
>> @@ -320,6 +344,10 @@ int iio_buffer_register(struct iio_dev *indio_dev,
>>  	struct iio_buffer *buffer = indio_dev->buffer;
>>  	int ret, i, attrn, attrcount, attrcount_orig = 0;
>>  
>> +	ret = iio_check_unique_scan_index(indio_dev, channels, num_channels);
>> +	if (ret < 0)
>> +		return ret;
>> +
>>  	if (buffer->attrs)
>>  		indio_dev->groups[indio_dev->groupcounter++] = buffer->attrs;
>>  
>>
> 
> --
> 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
>