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

[Jonathan Cameron <jic23@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> ---
>   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;
> +		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.
>    */
> -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.
>    *			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;
>   	} 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html