Re: [RFC 1/2] IIO : core: Enhance read_raw capability

[Jonathan Cameron <jic23@kernel.org>]

On 02/01/14 10:32, Peter Meerwald wrote:
> Hello,
>
> one option would be to not support quaternions via read_raw() and ask
> to use buffered mode;
There will be slow devices outputing orientation as a quaternion, so I'd
rather allow for this compound output in read raw.
>
> I see some inconsistency/redundancy problem:
> for example R/G/B light values could be handled either as separate
> channels via modifiers or as the new repeated type -- both ways have some
> merit
True enough. If they are only scaled wrt each other RGB as one attribute makes
sense.  If they have any meaning indivdually - such as the raw adc reading off
a light sensor behind a red filter, then they should be separate using modifiers.


>
> some minor comments below
>
> regards, p.
>
>> On 24/12/13 16:19, Srinivas Pandruvada wrote:
>>> Sampling Issue:
>>> In some data types, unless all of its component present, data has no
>>> meaning.
>>> A quaternion type falls under this category.
>>> A quaternion is usually represented as a vector, [w, x, y, z]. They are
>>> related by equation
>>> sq(i) = sq(j) = sq(k) = ijk = -1
>>>
>>> Idea is to ensure some mechanism where multiple elements can be read in one
>>> call not just current val and val2.
>>>
>>> Jonathan suggested the following for adding quaternion type values to IIO
>>> raw_reads (mail dated : 7th Dec, 2013)
>>>
>>> 1) Introduced an IIO_VAL_QUATERNION
>>>
>>> 2) Introduced a replacement for read_raw
>>> 	int (*read_raw)(struct iio_dev *indio_dev,
>>> 			struct iio_chan_spec const *chan,
>>> 			int val_len,
>>> 			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
>>> <done>
>>>
>>> 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.
>>>
>>> <A QUATERNION type will be formated by separating each component by ":"
>>> x:y:z:w>
>>>
>>> 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?
>>> <
>>> I have a used a regular expression format.
>>> For example
>>> 	scan_elements/in_quat_rot_quaternion_type:le:(s32/32){4}>>0
>>> 	Here {sign real/storage bits} repeated four times for four components.
>>> This is done by adding additional "repeat" field in scan_type structure.
>>> This
>>> format will be only used if repeat field is set to more than 1. So for the
>>> ABI
>>> exposed by current drivers, there will be no change.
>>>>
>>>
>>> Drawback of adding this change will require changes in read_raw callbacks in
>>> every IIO driver.
>>
>> I rather like the approach of using a repeat value to specify the channel
>> type.
>> I'm sure we'll get some horendous packing in a driver at somepoint, but this
>> is nice and simple for the common case.  Perhaps we enforce sensible packing
>> for these attributes and make drivers reorganise the data if needed to meet
>> the requirements.
>>
>> The only change I will request here is to make it simple to implement the move
>> to the new read function across the tree.  Please introduce a new 'read'
>> callback rather than changing the current read_raw parameters.  There will be
>> a few extra lines of code in the core for a
>> while but that's not a problem.  We just pushed a similar scale of change
>> through for the event description interfaces without any problems.
>>
>> That way we can do one driver at a time, rather than having to move the lot
>> over in one go.  If we are lucky, Lars-Peter will fire up his semantic
>> patching skills and send us a 5 line coccinelle script to do the lot for us ;)
>>
>> These sorts of treewide changes are a little tedious from the point of
>> view of merge clashes but I think this one is well worth doing (just perhaps
>> not all in one kernel cycle).
>>
>> I've cc'd a few extra people to draw their attention to this patch and see if
>> they have any opinions/suggestions.
>>
>> Jonathan
>>
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>>    drivers/iio/iio_core.h            |  2 +-
>>>    drivers/iio/industrialio-buffer.c | 41
>>> +++++++++++++++++++++++++++++++------
>>>    drivers/iio/industrialio-core.c   | 43
>>> ++++++++++++++++++++-------------------
>>>    drivers/iio/industrialio-event.c  |  6 ++++--
>>>    drivers/iio/inkern.c              | 10 +++++++--
>>>    include/linux/iio/iio.h           | 13 +++++++++---
>>>    6 files changed, 80 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
>>> index f6db6af..4172f22 100644
>>> --- a/drivers/iio/iio_core.h
>>> +++ b/drivers/iio/iio_core.h
>>> @@ -35,7 +35,7 @@ int __iio_add_chan_devattr(const char *postfix,
>>>    			   struct list_head *attr_list);
>>>    void iio_free_chan_devattr_list(struct list_head *attr_list);
>>>
>>> -ssize_t iio_format_value(char *buf, unsigned int type, int val, int val2);
>>> +ssize_t iio_format_value(char *buf, unsigned int type, int *vals);
>>>
>>>    /* Event interface flags */
>>>    #define IIO_BUSY_BIT_POS 1
>>> diff --git a/drivers/iio/industrialio-buffer.c
>>> b/drivers/iio/industrialio-buffer.c
>>> index 7f9152c..ee57d94 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -121,7 +121,16 @@ static ssize_t iio_show_fixed_type(struct device *dev,
>>>    		type = IIO_BE;
>>>    #endif
>>>    	}
>>> -	return sprintf(buf, "%s:%c%d/%d>>%u\n",
>>> +	if (this_attr->c->scan_type.repeat > 1)
>>> +		return sprintf(buf, "%s:(%c%d/%d){%d}>>%u\n",
>>> +		       iio_endian_prefix[type],
>>> +		       this_attr->c->scan_type.sign,
>>> +		       this_attr->c->scan_type.realbits,
>>> +		       this_attr->c->scan_type.storagebits,
>>> +		       this_attr->c->scan_type.repeat,
>>> +		       this_attr->c->scan_type.shift);
>>> +		else
>>> +			return sprintf(buf, "%s:%c%d/%d>>%u\n",
>>>    		       iio_endian_prefix[type],
>>>    		       this_attr->c->scan_type.sign,
>>>    		       this_attr->c->scan_type.realbits,
>>> @@ -446,14 +455,22 @@ static int iio_compute_scan_bytes(struct iio_dev
>>> *indio_dev,
>>>    	for_each_set_bit(i, mask,
>>>    			 indio_dev->masklength) {
>>>    		ch = iio_find_channel_from_si(indio_dev, i);
>>> -		length = ch->scan_type.storagebits / 8;
>>> +		if (ch->scan_type.repeat > 1)
>>> +			length = ch->scan_type.storagebits / 8 *
>>> +				ch->scan_type.repeat;
>
> this should be
> length = (ch->scan_type.storagebits * ch->scan_type.repeat + 7) / 8;
> unless storagebits is guaranteed to be a multiple of 8
So far storagebits is guaranteed to be a multiple of 8. I suppose in theory
we could relax this for these channels as long as the length of all the parts
is a multiple of 8.  That will make life much more uggly for userspace though
so I don't think we should allow non byte aligned storage.
>
>>> +		else
>>> +			length = ch->scan_type.storagebits / 8;
>>>    		bytes = ALIGN(bytes, length);
>>>    		bytes += length;
>>>    	}
>>>    	if (timestamp) {
>>>    		ch = iio_find_channel_from_si(indio_dev,
>>>    					      indio_dev->scan_index_timestamp);
>>> -		length = ch->scan_type.storagebits / 8;
>>> +		if (ch->scan_type.repeat > 1)
>>> +			length = ch->scan_type.storagebits / 8 *
>>> +				ch->scan_type.repeat;
>>> +		else
>>> +			length = ch->scan_type.storagebits / 8;
>>>    		bytes = ALIGN(bytes, length);
>>>    		bytes += length;
>>>    	}
>>> @@ -932,7 +949,11 @@ static int iio_buffer_update_demux(struct iio_dev
>>> *indio_dev,
>>>    					       indio_dev->masklength,
>>>    					       in_ind + 1);
>>>    			ch = iio_find_channel_from_si(indio_dev, in_ind);
>>> -			length = ch->scan_type.storagebits/8;
>>> +			if (ch->scan_type.repeat > 1)
>>> +				length = ch->scan_type.storagebits / 8 *
>>> +					ch->scan_type.repeat;
>>> +			else
>>> +				length = ch->scan_type.storagebits / 8;
>>>    			/* Make sure we are aligned */
>>>    			in_loc += length;
>>>    			if (in_loc % length)
>>> @@ -944,7 +965,11 @@ static int iio_buffer_update_demux(struct iio_dev
>>> *indio_dev,
>>>    			goto error_clear_mux_table;
>>>    		}
>>>    		ch = iio_find_channel_from_si(indio_dev, in_ind);
>>> -		length = ch->scan_type.storagebits/8;
>>> +		if (ch->scan_type.repeat > 1)
>>> +			length = ch->scan_type.storagebits / 8 *
>>> +				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)
>>> @@ -965,7 +990,11 @@ static int iio_buffer_update_demux(struct iio_dev
>>> *indio_dev,
>>>    		}
>>>    		ch = iio_find_channel_from_si(indio_dev,
>>>    			indio_dev->scan_index_timestamp);
>>> -		length = ch->scan_type.storagebits/8;
>>> +		if (ch->scan_type.repeat > 1)
>>> +			length = ch->scan_type.storagebits / 8 *
>>> +				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)
>>> diff --git a/drivers/iio/industrialio-core.c
>>> b/drivers/iio/industrialio-core.c
>>> index 18f72e3..f0f2a1a 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -367,41 +367,42 @@ EXPORT_SYMBOL_GPL(iio_enum_write);
>>>     * @buf: The buffer to which the formated value gets written
>>>     * @type: One of the IIO_VAL_... constants. This decides how the val and
>>> val2
>>>     *        parameters are formatted.
>>> - * @val: First part of the value, exact meaning depends on the type
>>> parameter.
>>> - * @val2: Second part of the value, exact meaning depends on the type
>>> parameter.
>>> + * @val: pointer to the values, exact meaning depends on the the type
>>> parameter.
>
> the the
>
>>>     */
>>> -ssize_t iio_format_value(char *buf, unsigned int type, int val, int val2)
>>> +ssize_t iio_format_value(char *buf, unsigned int type, int *vals)
>>>    {
>>>    	unsigned long long tmp;
>>>    	bool scale_db = false;
>>>
>>>    	switch (type) {
>>>    	case IIO_VAL_INT:
>>> -		return sprintf(buf, "%d\n", val);
>>> +		return sprintf(buf, "%d\n", vals[0]);
>>>    	case IIO_VAL_INT_PLUS_MICRO_DB:
>>>    		scale_db = true;
>>>    	case IIO_VAL_INT_PLUS_MICRO:
>>> -		if (val2 < 0)
>>> -			return sprintf(buf, "-%ld.%06u%s\n", abs(val), -val2,
>>> +		if (vals[1] < 0)
>>> +			return sprintf(buf, "-%ld.%06u%s\n", abs(vals[0]),
>>> +					-vals[1],
>>>    				scale_db ? " dB" : "");
>>>    		else
>>> -			return sprintf(buf, "%d.%06u%s\n", val, val2,
>>> +			return sprintf(buf, "%d.%06u%s\n", vals[0], vals[1],
>>>    				scale_db ? " dB" : "");
>>>    	case IIO_VAL_INT_PLUS_NANO:
>>> -		if (val2 < 0)
>>> -			return sprintf(buf, "-%ld.%09u\n", abs(val), -val2);
>>> +		if (vals[1] < 0)
>>> +			return sprintf(buf, "-%ld.%09u\n", abs(vals[0]),
>>> +					-vals[1]);
>>>    		else
>>> -			return sprintf(buf, "%d.%09u\n", val, val2);
>>> +			return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>>>    	case IIO_VAL_FRACTIONAL:
>>> -		tmp = div_s64((s64)val * 1000000000LL, val2);
>>> -		val2 = do_div(tmp, 1000000000LL);
>>> -		val = tmp;
>>> -		return sprintf(buf, "%d.%09u\n", val, val2);
>>> +		tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
>>> +		vals[1] = do_div(tmp, 1000000000LL);
>>> +		vals[0] = tmp;
>>> +		return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>>>    	case IIO_VAL_FRACTIONAL_LOG2:
>>> -		tmp = (s64)val * 1000000000LL >> val2;
>>> -		val2 = do_div(tmp, 1000000000LL);
>>> -		val = tmp;
>>> -		return sprintf(buf, "%d.%09u\n", val, val2);
>>> +		tmp = (s64)vals[0] * 1000000000LL >> vals[1];
>>> +		vals[1] = do_div(tmp, 1000000000LL);
>>> +		vals[0] = tmp;
>>> +		return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>>>    	default:
>>>    		return 0;
>>>    	}
>>> @@ -413,14 +414,14 @@ static ssize_t iio_read_channel_info(struct device
>>> *dev,
>>>    {
>>>    	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>    	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> -	int val, val2;
>>> +	int vals[INDIO_MAX_RAW_ELEMENTS];
>>>    	int ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
>>> -					    &val, &val2, this_attr->address);
>>> +			    INDIO_MAX_RAW_ELEMENTS, vals, this_attr->address);
>>>
>>>    	if (ret < 0)
>>>    		return ret;
>>>
>>> -	return iio_format_value(buf, ret, val, val2);
>>> +	return iio_format_value(buf, ret, vals);
>>>    }
>>>
>>>    /**
>>> diff --git a/drivers/iio/industrialio-event.c
>>> b/drivers/iio/industrialio-event.c
>>> index c10eab6..b249b48 100644
>>> --- a/drivers/iio/industrialio-event.c
>>> +++ b/drivers/iio/industrialio-event.c
>>> @@ -274,7 +274,7 @@ static ssize_t iio_ev_value_show(struct device *dev,
>>>    {
>>>    	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>    	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> -	int val, val2;
>>> +	int val, val2, val_arr[2];
>>>    	int ret;
>>>
>>>    	if (indio_dev->info->read_event_value) {
>>> @@ -290,7 +290,9 @@ static ssize_t iio_ev_value_show(struct device *dev,
>>>    			&val, &val2);
>>>    		if (ret < 0)
>>>    			return ret;
>>> -		return iio_format_value(buf, ret, val, val2);
>>> +		val_arr[0] = val;
>>> +		val_arr[1] = val2;
>>> +		return iio_format_value(buf, ret, val_arr);
>>>    	}
>>>    }
>>>
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index 0cf5f8e..76d8b26 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -417,12 +417,18 @@ static int iio_channel_read(struct iio_channel *chan,
>>> int *val, int *val2,
>>>    	enum iio_chan_info_enum info)
>>>    {
>>>    	int unused;
>>> +	int vals[INDIO_MAX_RAW_ELEMENTS];
>>> +	int ret;
>>>
>>>    	if (val2 == NULL)
>>>    		val2 = &unused;
>>>
>>> -	return chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
>>> -						val, val2, info);
>>> +	ret = chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
>>> +					INDIO_MAX_RAW_ELEMENTS, vals, info);
>>> +	*val = vals[0];
>>> +	*val2 = vals[1];
>>> +
>>> +	return ret;
>>>    }
>>>
>>>    int iio_read_channel_raw(struct iio_channel *chan, int *val)
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 256a90a..0ba16b6 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -176,6 +176,8 @@ struct iio_event_spec {
>>>     *			storage_bits:	Realbits + padding
>>>     *			shift:		Shift right by this before masking out
>>>     *					realbits.
>>> + *			repeat:		No. of times real/storage bits repeats
>>> + *					.Default: 1, unless set to other
>>> value.
>
> 0 could be the default for 'repeat' and mean 1 (the default)
Excellent point - we don't want to have to add a repeat element to normal
channel types.  Just make it work with 0 and add a note that if set to
0 it will be treated as 1.
>
>>>     *			endianness:	little or big endian
>>>     * @info_mask_separate: What information is to be exported that is
>>> specific to
>>>     *			this channel.
>>> @@ -220,6 +222,7 @@ struct iio_chan_spec {
>>>    		u8	realbits;
>>>    		u8	storagebits;
>>>    		u8	shift;
>>> +		u8	repeat;
>>>    		enum iio_endian endianness;
>
> repeat should be added to the end of the struct for compatibility reasons?
Generally a good idea, but don't think it can cause any grief here given this is
never exposed to userspace and drivers should always be compiled against the
correct kernel version.
>
>>>    	} scan_type;
>>>    	long			info_mask_separate;
>>> @@ -286,6 +289,8 @@ static inline s64 iio_get_time_ns(void)
>>>    #define INDIO_ALL_BUFFER_MODES					\
>>>    	(INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE)
>>>
>>> +#define INDIO_MAX_RAW_ELEMENTS		4
>>> +
>>>    struct iio_trigger; /* forward declaration */
>>>    struct iio_dev;
>>>
>>> @@ -298,8 +303,10 @@ struct iio_dev;
>>>     * @read_raw:		function to request a value from the device.
>>>     *			mask specifies which value. Note 0 means a reading of
>>>     *			the channel in question.  Return value will specify
>>> the
>>> - *			type of value returned by the device. val and val2
>>> will
>>> + *			type of value returned by the device. val pointer
>>>     *			contain the elements making up the returned value.
>>> + *			val_len specifies maximum number of elements
>>> + *			val pointer can contain.
>>>     * @write_raw:		function to write a value to the device.
>>>     *			Parameters are the same as for read_raw.
>>>     * @write_raw_get_fmt:	callback function to query the expected
>>> @@ -330,8 +337,8 @@ struct iio_info {
>>>
>>>    	int (*read_raw)(struct iio_dev *indio_dev,
>>>    			struct iio_chan_spec const *chan,
>>> -			int *val,
>>> -			int *val2,
>>> +			int val_len,
>>> +			int *vals,
>>>    			long mask);
>>>
>>>    	int (*write_raw)(struct iio_dev *indio_dev,
>>>
>> --
>> 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
>>
>