Proposal to add full scale attribute

Proposal to add full scale attribute

[Denis Ciocca]

I everyone,

my patch (not working) is a proposal to add full scale attribute to change
the current full scale for devices. The patch doesn't work because the function
for_each_set_bit in the industrialio-core.c use the type unsigned long and
the new entry is too big. However, what do you think about?

Thanks,

Denis
 

[PATCH] iio: Proposal for full scale attribute (not working)

[Denis Ciocca]

Proposal to add full scale attribute for channel and sysfs macro for full scale available. This method will substitute the change full scale by write gain attribute that is inefficient.
---
 include/linux/iio/iio.h   |    5 +++++
 include/linux/iio/sysfs.h |   16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index da8c776..e250d25 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -36,6 +36,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_PHASE,
 	IIO_CHAN_INFO_HARDWAREGAIN,
 	IIO_CHAN_INFO_HYSTERESIS,
+	IIO_CHAN_INFO_FULL_SCALE,
 };
 
 #define IIO_CHAN_INFO_SHARED_BIT(type) BIT(type*2)
@@ -107,6 +108,10 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_SEPARATE_BIT(IIO_CHAN_INFO_HYSTERESIS)
 #define IIO_CHAN_INFO_HYSTERESIS_SHARED_BIT			\
 	IIO_CHAN_INFO_SHARED_BIT(IIO_CHAN_INFO_HYSTERESIS)
+#define IIO_CHAN_INFO_FULL_SCALE_SEPARATE_BIT			\
+	IIO_CHAN_INFO_SEPARATE_BIT(IIO_CHAN_INFO_FULL_SCALE)
+#define IIO_CHAN_INFO_FULL_SCALE_SHARED_BIT			\
+	IIO_CHAN_INFO_SHARED_BIT(IIO_CHAN_INFO_FULL_SCALE)
 
 enum iio_endian {
 	IIO_CPU,
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index b7a934b..7e322a9 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -104,6 +104,22 @@ struct iio_const_attr {
  **/
 #define IIO_CONST_ATTR_SAMP_FREQ_AVAIL(_string)			\
 	IIO_CONST_ATTR(sampling_frequency_available, _string)
+/**
+ * IIO_DEV_ATTR_FULL_SCALE_AVAIL - list available full scales
+ * @_show: output method for the attribute
+ *
+ * May be mode dependent on some devices
+ **/
+#define IIO_DEV_ATTR_FULL_SCALE_AVAIL(_show)				\
+	IIO_DEVICE_ATTR(full_scale_available, S_IRUGO, _show, NULL, 0)
+/**
+ * IIO_CONST_ATTR_FULL_SCALE_AVAIL - list available full scales
+ * @_string: full scale string for the attribute
+ *
+ * Constant version
+ **/
+#define IIO_CONST_ATTR_FULL_SCALE_AVAIL(_string)			\
+	IIO_CONST_ATTR(full_scale_available, _string)
 
 #define IIO_DEV_ATTR_TEMP_RAW(_show)			\
 	IIO_DEVICE_ATTR(in_temp_raw, S_IRUGO, _show, NULL, 0)
-- 
1.7.9.5

Re: Proposal to add full scale attribute

[Jonathan Cameron]

On 02/09/2013 05:34 PM, Denis Ciocca wrote:
> I everyone,
> 
> my patch (not working) is a proposal to add full scale attribute to change
> the current full scale for devices. The patch doesn't work because the function
> for_each_set_bit in the industrialio-core.c use the type unsigned long and
> the new entry is too big. However, what do you think about?
> 
> Thanks,
> 
> Denis

Hi Denis,

What you propose has come up a number of times before (and
is present I think as 'range' in a number of drivers still in staging)

We did have a brief discussion a while ago about how to handle describing
the possible range of channels and off controls on channels.
One thing I suggested at the time (but have done nothing about since)
was that we should actually have descriptions of the possible values
of all elements of iio_info (including raw channel readings).
The discussion was deep in a thread on something else so I won't
try and dig it out now.

Up shot was something like

1) have an additional callback in struct iio_info along the lines of:
	int (*read_avail)(struct iio_dev *indio_dev,
		          struct iio_chan_spec const *chan,
			  int **vals,
			  long mask);
(return type indicates what vals is filled with).  Typically it'll be
returning a pointer to a static array to avoid leaking memory.  Otherwise
some care will be needed in drivers.

2) For all iio_info elements there will be additional attributes to effectively
access pretty printed versions of the output of this callback.

in_accel_raw_availble
in_accel_scale_available
...
(or similar).

These would be read only and describe the possible values the associated
output (e.g. in_accel_raw) can take.

To make up some syntax how about
[0...255] or {0, 13, 33} to indicate a range, or a small set of possible
values?

The in_accel_raw_available in conjunction with in_accel_scale would then
effectively give your full range but also a lot of other useful information.

As I stated earlier, we have had a range attribute proposed in the past
(and is in some staging drivers) which is I think similar to your proposed
full scale attribute.
The issue I always had with it is that we then end up with the question of
which should be controllable, scale or range?  Scale is important for
processing the data, but people like to see full scale as well.

You touch on efficiency of writing full scale in the patch. I am afraid
I don't follow what you mean by that. Could you explain?  Setting these
values is never in the fast path and in the vast majority of cases the
conversion from one to the other is trivial.

What do you think?

Thanks for pointing out that we were out of space in the mask. I've been
meaning to do something about that for a while.  One obvious option is
to split the shared and separate masks up into a pair with the fields just
defined in the appropriate one. Any other simple options occur to anyone?

Jonathan

Re: Proposal to add full scale attribute

[Denis CIOCCA]

Hi Jonathan,


> What you propose has come up a number of times before (and
> is present I think as 'range' in a number of drivers still in staging)

I saw in the light/isl29018 driver what have you tell me, is exactly=20
what I would add.
To maintain the consistency, these values should be expressed in=20
framework units [m/s^2, ...]



> We did have a brief discussion a while ago about how to handle describing
> the possible range of channels and off controls on channels.
> One thing I suggested at the time (but have done nothing about since)
> was that we should actually have descriptions of the possible values
> of all elements of iio_info (including raw channel readings).
> The discussion was deep in a thread on something else so I won't
> try and dig it out now.
>
> Up shot was something like
>
> 1) have an additional callback in struct iio_info along the lines of:
> 	int (*read_avail)(struct iio_dev *indio_dev,
> 		          struct iio_chan_spec const *chan,
> 			  int **vals,
> 			  long mask);
> (return type indicates what vals is filled with).  Typically it'll be
> returning a pointer to a static array to avoid leaking memory.  Otherwise
> some care will be needed in drivers.

Very interesting...



> 2) For all iio_info elements there will be additional attributes to effec=
tively
> access pretty printed versions of the output of this callback.
>
> in_accel_raw_availble
> in_accel_scale_available
> ...
> (or similar).
>
> These would be read only and describe the possible values the associated
> output (e.g. in_accel_raw) can take.
>
> To make up some syntax how about
> [0...255] or {0, 13, 33} to indicate a range, or a small set of possible
> values?

In this case I'd like to find in_accel_range_available with this output:
{9.8 19.6 39.2 78.4}

Attribute scale is only one value to convert the current raw data to the=20
default frameworks units.

Now that I think, I don't know if is useful the raw_available and=20
scale_available...



> The in_accel_raw_available in conjunction with in_accel_scale would then
> effectively give your full range but also a lot of other useful informati=
on.
> As I stated earlier, we have had a range attribute proposed in the past
> (and is in some staging drivers) which is I think similar to your propose=
d
> full scale attribute.
> The issue I always had with it is that we then end up with the question o=
f
> which should be controllable, scale or range?  Scale is important for
> processing the data, but people like to see full scale as well.

Ok you have right, the scale attribute is more important to processing=20
the data, but what about resolution of the sensors?
The output data are fixed to a fixed number of bits in the all possible=20
full scale available for the sensor, this implies that the resolution of=20
the ADCs is different in function of the full scale selected.

If I know that the maximum interesting values are less than 9.8 m/s^2=20
how can I maximize the resolution?
If my only data is 'scale', I have to examine the datasheet and=20
understand which value of full scale is.



> You touch on efficiency of writing full scale in the patch. I am afraid
> I don't follow what you mean by that. Could you explain?  Setting these
> values is never in the fast path and in the vast majority of cases the
> conversion from one to the other is trivial.
>
> What do you think?

When I speak about efficiency I mean what I say before, increase the=20
resolution of the output data without consult the datasheet and do=20
reverse operations...


> Thanks for pointing out that we were out of space in the mask. I've been
> meaning to do something about that for a while.  One obvious option is
> to split the shared and separate masks up into a pair with the fields jus=
t
> defined in the appropriate one. Any other simple options occur to anyone?

No, I thought the same thing... :D



Denis=