Re: [PATCH v3 7/8] iio: Add quaternion channel

[Jonathan Cameron <jic23@kernel.org>]

On 12/06/13 22:44, Srinivas Pandruvada wrote:
> Hi Jonathan,
> 
> On 11/05/2013 11:58 PM, Jonathan Cameron wrote:
>>
>> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
>>> On 11/05/2013 03:10 PM, Jonathan Cameron wrote:
>>>> On 10/30/13 22:48, Srinivas Pandruvada wrote:
>>>>> A quaternion channel type is added. Here channel information is
>>>>> composed of four components: a vector with x, y, z coordinates and
>>>>> a w rotation. Reusing x, y, z channel modifiers, but added "w"
>>>>> component in the modifier list.
>>>>>
>>>>> Signed-off-by: Srinivas Pandruvada
>>> <srinivas.pandruvada@linux.intel.com>
>>>> In brief I am against this for the same reason I didn't like this
>>> before.
>>>> A quaternion has no meaning if it isn't all present.  Hence we need
>>> to
>>>> ensure that it is always presented to userspace with all four
>>> components
>>>> present.
>>>>
>>>> I'll hopefully have a few mins at the weekend to to bash out some
>>> example
>>>> code for how I would suggest we handle this.
>>>>
>>>> If you could repost the patches before this one with everything that
>>> should
>>>> be in them then hopefully we can take those whilst still 'discussing'
>>>> how to handle the last 2!
> If you get chance, please send me some info on how you want to handle this.
> 
Sorry for the delay on this - I might get time tomorrow to go into this in more
detail but maybe not so I'll give a very brief outline here.

Constraints:

1) Quaternions may have 4 elements but they all interact in a way that makes
   them effectively a 'single value'.  Thus they need to be handled as one.

Solutions

1) Introduce a IIO_VAL_*  - probably IIO_VAL_QUATERNION
2) Introduced a replacement for read_raw that has the following form
	int (*read)(struct iio_dev *indio_dev,
			struct iio_chan_spec const *chan,
			int *vals,
			long mask);
- for previous cases vals[0] <- val and vals[1] <- val2
- for quaternion vals[0] <- i component, vals[1] <- j component etc

3) Add a case to iio_read_channel_info in industrialio-core.c to handle
the new type and pretty print it appropriately - possibly expand
iio_format_value to handle the new parameters.

4) Buffered support is only trickier in that the buffer element needs
describing and there is a lot of possible flexibility in there...

current format is :

 [be|le]:[s|u]bits/storagebits[>>shift].

Reasonable to assume that whole thing is either be or le and that elements are byte aligned
plus all are signed (but need to state this)

Hence perhaps either

[be|le]:[s|u]bitsI/storagebitsI,bitsJ/storagebitsJ,bitsK/storagebitsK,bitsA/storagebitsA

or we assume that the elements are effectively independent but have the same placement and
indicated perhaps as

be:s12/16x4 or similar?

Either way this would need some extensions to the struct iio_chan_spec substructure
scan_type.

> Thanks,
> Srinivas
> 
>>> Sorry about the issues with previous patches. I was trying to order new
>>>
>>> driver at the end and missed dependencies.
>> Not to worry. We all make that mistake occasionally!
>>> I have applied all patches which I am sending after this email safely
>>> apply to fixes-togreg branch.
>>> For the last two, I will wait for your suggestion.
>>>
>>> Thanks,
>>> Srinivas
>>>> Jonathan
>>>>> ---
>>>>>    drivers/iio/industrialio-core.c | 2 ++
>>>>>    include/linux/iio/types.h       | 2 ++
>>>>>    2 files changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iio/industrialio-core.c
>>> b/drivers/iio/industrialio-core.c
>>>>> index f95c697..b754f50 100644
>>>>> --- a/drivers/iio/industrialio-core.c
>>>>> +++ b/drivers/iio/industrialio-core.c
>>>>> @@ -66,6 +66,7 @@ static const char * const
>>> iio_chan_type_name_spec[] = {
>>>>>        [IIO_ALTVOLTAGE] = "altvoltage",
>>>>>        [IIO_CCT] = "cct",
>>>>>        [IIO_PRESSURE] = "pressure",
>>>>> +    [IIO_QUAT_ROT] = "quat_rot",
>>>>>    };
>>>>>       static const char * const iio_modifier_names[] = {
>>>>> @@ -80,6 +81,7 @@ static const char * const iio_modifier_names[] = {
>>>>>        [IIO_MOD_LIGHT_RED] = "red",
>>>>>        [IIO_MOD_LIGHT_GREEN] = "green",
>>>>>        [IIO_MOD_LIGHT_BLUE] = "blue",
>>>>> +    [IIO_MOD_W] = "w",
>>>>>    };
>>>>>       /* relies on pairs of these shared then separate */
>>>>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>>>>> index 88bf0f0..4565f5c 100644
>>>>> --- a/include/linux/iio/types.h
>>>>> +++ b/include/linux/iio/types.h
>>>>> @@ -29,6 +29,7 @@ enum iio_chan_type {
>>>>>        IIO_ALTVOLTAGE,
>>>>>        IIO_CCT,
>>>>>        IIO_PRESSURE,
>>>>> +    IIO_QUAT_ROT,
>>>>>    };
>>>>>       enum iio_modifier {
>>>>> @@ -52,6 +53,7 @@ enum iio_modifier {
>>>>>        IIO_MOD_LIGHT_RED,
>>>>>        IIO_MOD_LIGHT_GREEN,
>>>>>        IIO_MOD_LIGHT_BLUE,
>>>>> +    IIO_MOD_W,
>>>>>    };
>>>>>       #define IIO_VAL_INT 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
>