[PATCH v2] iio: st_sensors: make scale channels also shared by type

[PATCH v2] iio: st_sensors: make scale channels also shared by type

[Gaëtan André]

Scale channels are available by axis. For example for accelerometers,
in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are available.

However, they should be shared by type as documented in
Documentation/ABI/testing/sysfs-bus-iio.

For each sensor (acceleros, gyros and magnetos) only one value is specified
for all the axes.

Existing, by axis, entries are preserved in order to to leave the old ABI
untouched.

Signed-off-by: Gaëtan André <rvlander@gaetanandre.eu>
---
v1->v2: add comment explaining why we are doing both.

 include/linux/iio/common/st_sensors.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 33e939977444..42663fbab085 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -46,12 +46,19 @@
 #define ST_SENSORS_MAX_NAME			17
 #define ST_SENSORS_MAX_4WAI			8
 
+/*
+ * Scale channels are configured both by type and by axis.
+ * - By axis to keep the previous ABI and flexibility.
+ * - By type because it is how some userland
+ * applications are expecting them (ex: iio-sensor-proxy).
+ */
 #define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
 					ch2, s, endian, rbits, sbits, addr) \
 { \
 	.type = device_type, \
 	.modified = mod, \
 	.info_mask_separate = mask, \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
 	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
 	.scan_index = index, \
 	.channel2 = ch2, \

base-commit: 2de8c02349f02d014e51b43f306d28fc7a23ea6e
-- 
2.26.1

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Jonathan Cameron]

On Thu, 23 Apr 2020 14:17:15 +0200
Gaëtan André <rvlander@gaetanandre.eu> wrote:

> Scale channels are available by axis. For example for accelerometers,
> in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are available.
> 
> However, they should be shared by type as documented in
> Documentation/ABI/testing/sysfs-bus-iio.
> 
> For each sensor (acceleros, gyros and magnetos) only one value is specified
> for all the axes.
> 
> Existing, by axis, entries are preserved in order to to leave the old ABI
> untouched.
As I mentioned in v1, there isn't a strict ABI rule that says that we must
do the shared form

+CC'd Bastien for comment on what userspace is assuming and whether we should
push this back to stable or not.

Thanks,

Jonathan

> 
> Signed-off-by: Gaëtan André <rvlander@gaetanandre.eu>
> ---
> v1->v2: add comment explaining why we are doing both.
> 
>  include/linux/iio/common/st_sensors.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 33e939977444..42663fbab085 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -46,12 +46,19 @@
>  #define ST_SENSORS_MAX_NAME			17
>  #define ST_SENSORS_MAX_4WAI			8
>  
> +/*
> + * Scale channels are configured both by type and by axis.
> + * - By axis to keep the previous ABI and flexibility.
> + * - By type because it is how some userland
> + * applications are expecting them (ex: iio-sensor-proxy).
> + */
>  #define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
>  					ch2, s, endian, rbits, sbits, addr) \
>  { \
>  	.type = device_type, \
>  	.modified = mod, \
>  	.info_mask_separate = mask, \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>  	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>  	.scan_index = index, \
>  	.channel2 = ch2, \
> 
> base-commit: 2de8c02349f02d014e51b43f306d28fc7a23ea6e

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Bastien Nocera]

On Sat, 2020-04-25 at 18:13 +0100, Jonathan Cameron wrote:
> On Thu, 23 Apr 2020 14:17:15 +0200
> Gaëtan André <rvlander@gaetanandre.eu> wrote:
> 
> > Scale channels are available by axis. For example for
> > accelerometers,
> > in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are
> > available.
> > 
> > However, they should be shared by type as documented in
> > Documentation/ABI/testing/sysfs-bus-iio.
> > 
> > For each sensor (acceleros, gyros and magnetos) only one value is
> > specified
> > for all the axes.
> > 
> > Existing, by axis, entries are preserved in order to to leave the
> > old ABI
> > untouched.
> As I mentioned in v1, there isn't a strict ABI rule that says that we
> must
> do the shared form
> 
> +CC'd Bastien for comment on what userspace is assuming and whether
> we should
> push this back to stable or not.

I have no idea what the effects of this would be on the ABI, and how
this would impact iio-sensor-proxy.

Code is here though, so it might be best to test it:
https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/tree/master/src

And we accept merge requests :)

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Jonathan Cameron]

On Sun, 26 Apr 2020 13:19:09 +0200
Bastien Nocera <hadess@hadess.net> wrote:

> On Sat, 2020-04-25 at 18:13 +0100, Jonathan Cameron wrote:
> > On Thu, 23 Apr 2020 14:17:15 +0200
> > Gaëtan André <rvlander@gaetanandre.eu> wrote:
> >   
> > > Scale channels are available by axis. For example for
> > > accelerometers,
> > > in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are
> > > available.
> > > 
> > > However, they should be shared by type as documented in
> > > Documentation/ABI/testing/sysfs-bus-iio.
> > > 
> > > For each sensor (acceleros, gyros and magnetos) only one value is
> > > specified
> > > for all the axes.
> > > 
> > > Existing, by axis, entries are preserved in order to to leave the
> > > old ABI
> > > untouched.  
> > As I mentioned in v1, there isn't a strict ABI rule that says that we
> > must
> > do the shared form
> > 
> > +CC'd Bastien for comment on what userspace is assuming and whether
> > we should
> > push this back to stable or not.  
> 
> I have no idea what the effects of this would be on the ABI, and how
> this would impact iio-sensor-proxy.

There goes me being lazy ;)

> 
> Code is here though, so it might be best to test it:
> https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/tree/master/src
> 
> And we accept merge requests :)

Only looks at scale and in_accel_scale

Easy enough to fix...

Note that for some older accelerometers it has to do per channel scales btw.
It used to be hard to have the same range out of the plane of the silicon
than within it, so was common to have sensors with different ranges and hence
scales in z direction from x and y.

I'll apply the kernel patch but good to fix up iio-sensor-proxy as well.

I would ideally like Denis to give this a quick sanity check though as I'd
like to give it a stable tag and don't want any unexpected breakage.

Thanks,

Jonathan

> 

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Bastien Nocera]

On Sat, 2020-05-02 at 19:07 +0100, Jonathan Cameron wrote:
> On Sun, 26 Apr 2020 13:19:09 +0200
> Bastien Nocera <hadess@hadess.net> wrote:
> 
> > On Sat, 2020-04-25 at 18:13 +0100, Jonathan Cameron wrote:
> > > On Thu, 23 Apr 2020 14:17:15 +0200
> > > Gaëtan André <rvlander@gaetanandre.eu> wrote:
> > >   
> > > > Scale channels are available by axis. For example for
> > > > accelerometers,
> > > > in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are
> > > > available.
> > > > 
> > > > However, they should be shared by type as documented in
> > > > Documentation/ABI/testing/sysfs-bus-iio.
> > > > 
> > > > For each sensor (acceleros, gyros and magnetos) only one value
> > > > is
> > > > specified
> > > > for all the axes.
> > > > 
> > > > Existing, by axis, entries are preserved in order to to leave
> > > > the
> > > > old ABI
> > > > untouched.  
> > > As I mentioned in v1, there isn't a strict ABI rule that says
> > > that we
> > > must
> > > do the shared form
> > > 
> > > +CC'd Bastien for comment on what userspace is assuming and
> > > whether
> > > we should
> > > push this back to stable or not.  
> > 
> > I have no idea what the effects of this would be on the ABI, and
> > how
> > this would impact iio-sensor-proxy.
> 
> There goes me being lazy ;)
> 
> > Code is here though, so it might be best to test it:
> > https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/tree/master/src
> > 
> > And we accept merge requests :)
> 
> Only looks at scale and in_accel_scale
> 
> Easy enough to fix...
> 
> Note that for some older accelerometers it has to do per channel
> scales btw.
> It used to be hard to have the same range out of the plane of the
> silicon
> than within it, so was common to have sensors with different ranges
> and hence
> scales in z direction from x and y.
> 
> I'll apply the kernel patch but good to fix up iio-sensor-proxy as
> well.
> 
> I would ideally like Denis to give this a quick sanity check though
> as I'd
> like to give it a stable tag and don't want any unexpected breakage.

I've made this:
https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/merge_requests/306
which should work just fine for the sensors which have a trigger (the z
scaling was the one applied to all 3 axes).

What's the name of the file that contains the x/y/z axis scale
information for devices without triggers?

Cheers

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Jonathan Cameron]

On Mon, 4 May 2020 12:02:36 +0200
Bastien Nocera <hadess@hadess.net> wrote:

> On Sat, 2020-05-02 at 19:07 +0100, Jonathan Cameron wrote:
> > On Sun, 26 Apr 2020 13:19:09 +0200
> > Bastien Nocera <hadess@hadess.net> wrote:
> >   
> > > On Sat, 2020-04-25 at 18:13 +0100, Jonathan Cameron wrote:  
> > > > On Thu, 23 Apr 2020 14:17:15 +0200
> > > > Gaëtan André <rvlander@gaetanandre.eu> wrote:
> > > >     
> > > > > Scale channels are available by axis. For example for
> > > > > accelerometers,
> > > > > in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are
> > > > > available.
> > > > > 
> > > > > However, they should be shared by type as documented in
> > > > > Documentation/ABI/testing/sysfs-bus-iio.
> > > > > 
> > > > > For each sensor (acceleros, gyros and magnetos) only one value
> > > > > is
> > > > > specified
> > > > > for all the axes.
> > > > > 
> > > > > Existing, by axis, entries are preserved in order to to leave
> > > > > the
> > > > > old ABI
> > > > > untouched.    
> > > > As I mentioned in v1, there isn't a strict ABI rule that says
> > > > that we
> > > > must
> > > > do the shared form
> > > > 
> > > > +CC'd Bastien for comment on what userspace is assuming and
> > > > whether
> > > > we should
> > > > push this back to stable or not.    
> > > 
> > > I have no idea what the effects of this would be on the ABI, and
> > > how
> > > this would impact iio-sensor-proxy.  
> > 
> > There goes me being lazy ;)
> >   
> > > Code is here though, so it might be best to test it:
> > > https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/tree/master/src
> > > 
> > > And we accept merge requests :)  
> > 
> > Only looks at scale and in_accel_scale
> > 
> > Easy enough to fix...
> > 
> > Note that for some older accelerometers it has to do per channel
> > scales btw.
> > It used to be hard to have the same range out of the plane of the
> > silicon
> > than within it, so was common to have sensors with different ranges
> > and hence
> > scales in z direction from x and y.
> > 
> > I'll apply the kernel patch but good to fix up iio-sensor-proxy as
> > well.
> > 
> > I would ideally like Denis to give this a quick sanity check though
> > as I'd
> > like to give it a stable tag and don't want any unexpected breakage.  
> 
> I've made this:
> https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/merge_requests/306
> which should work just fine for the sensors which have a trigger (the z
> scaling was the one applied to all 3 axes).
> 
> What's the name of the file that contains the x/y/z axis scale
> information for devices without triggers?

For both devices with triggers and without, if they are per axis they should be
in_accel_x_scale
in_accel_y_scale
in_accel_z_scale

hmm. It's not in our ABI docs for accel channels (but is for magnetometers).
Should fix that...

https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L340

In theory the other combinations you can get are

in_accel_scale
in_scale
scale

The in scale one tends not to make much sense for accelerometers though so I would
hope there are no instances of that in the wild (none in mainline).
Shared by direction attributes are tend to apply for things like sampling frequency,
not scale.

Thanks,

Jonathan

> 
> Cheers
> 

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Bastien Nocera]

On Mon, 2020-05-04 at 11:22 +0100, Jonathan Cameron wrote:
> On Mon, 4 May 2020 12:02:36 +0200
> Bastien Nocera <hadess@hadess.net> wrote:
> 
> > On Sat, 2020-05-02 at 19:07 +0100, Jonathan Cameron wrote:
> > > On Sun, 26 Apr 2020 13:19:09 +0200
> > > Bastien Nocera <hadess@hadess.net> wrote:
> > >   
> > > > On Sat, 2020-04-25 at 18:13 +0100, Jonathan Cameron wrote:  
> > > > > On Thu, 23 Apr 2020 14:17:15 +0200
> > > > > Gaëtan André <rvlander@gaetanandre.eu> wrote:
> > > > >     
> > > > > > Scale channels are available by axis. For example for
> > > > > > accelerometers,
> > > > > > in_accel_x_scale, in_accel_y_scale and in_accel_z_scale are
> > > > > > available.
> > > > > > 
> > > > > > However, they should be shared by type as documented in
> > > > > > Documentation/ABI/testing/sysfs-bus-iio.
> > > > > > 
> > > > > > For each sensor (acceleros, gyros and magnetos) only one
> > > > > > value
> > > > > > is
> > > > > > specified
> > > > > > for all the axes.
> > > > > > 
> > > > > > Existing, by axis, entries are preserved in order to to
> > > > > > leave
> > > > > > the
> > > > > > old ABI
> > > > > > untouched.    
> > > > > As I mentioned in v1, there isn't a strict ABI rule that says
> > > > > that we
> > > > > must
> > > > > do the shared form
> > > > > 
> > > > > +CC'd Bastien for comment on what userspace is assuming and
> > > > > whether
> > > > > we should
> > > > > push this back to stable or not.    
> > > > 
> > > > I have no idea what the effects of this would be on the ABI,
> > > > and
> > > > how
> > > > this would impact iio-sensor-proxy.  
> > > 
> > > There goes me being lazy ;)
> > >   
> > > > Code is here though, so it might be best to test it:
> > > > https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/tree/master/src
> > > > 
> > > > And we accept merge requests :)  
> > > 
> > > Only looks at scale and in_accel_scale
> > > 
> > > Easy enough to fix...
> > > 
> > > Note that for some older accelerometers it has to do per channel
> > > scales btw.
> > > It used to be hard to have the same range out of the plane of the
> > > silicon
> > > than within it, so was common to have sensors with different
> > > ranges
> > > and hence
> > > scales in z direction from x and y.
> > > 
> > > I'll apply the kernel patch but good to fix up iio-sensor-proxy
> > > as
> > > well.
> > > 
> > > I would ideally like Denis to give this a quick sanity check
> > > though
> > > as I'd
> > > like to give it a stable tag and don't want any unexpected
> > > breakage.  
> > 
> > I've made this:
> > https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/merge_requests/306
> > which should work just fine for the sensors which have a trigger
> > (the z
> > scaling was the one applied to all 3 axes).
> > 
> > What's the name of the file that contains the x/y/z axis scale
> > information for devices without triggers?
> 
> For both devices with triggers and without, if they are per axis they
> should be
> in_accel_x_scale
> in_accel_y_scale
> in_accel_z_scale
> 
> hmm. It's not in our ABI docs for accel channels (but is for
> magnetometers).
> Should fix that...
> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L340
> 
> In theory the other combinations you can get are
> 
> in_accel_scale
> in_scale
> scale
> 
> The in scale one tends not to make much sense for accelerometers
> though so I would
> hope there are no instances of that in the wild (none in mainline).
> Shared by direction attributes are tend to apply for things like
> sampling frequency,
> not scale.

I've updated the merge request. Would be good if somebody could test it
before I merge it.

Re: [PATCH v2] iio: st_sensors: make scale channels also shared by type

[Bastien Nocera]

On Mon, 2020-05-04 at 11:22 +0100, Jonathan Cameron wrote:
> 
<snip>
> For both devices with triggers and without, if they are per axis they
> should be
> in_accel_x_scale
> in_accel_y_scale
> in_accel_z_scale
> 
> hmm. It's not in our ABI docs for accel channels (but is for
> magnetometers).
> Should fix that...
> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L340
> 
> In theory the other combinations you can get are
> 
> in_accel_scale
> in_scale
> scale
> 
> The in scale one tends not to make much sense for accelerometers
> though so I would
> hope there are no instances of that in the wild (none in mainline).
> Shared by direction attributes are tend to apply for things like
> sampling frequency,
> not scale.

FYI, I merged:
https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/merge_requests/306

Let me know if I got it wrong.