iio: st_sensors: make scale channels also shared by type

Commit Message

Gaëtan André March 30, 2020, 2:59 p.m. UTC

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é <[e-mail]>
---
 include/linux/iio/common/st_sensors.h | 1 +
 1 file changed, 1 insertion(+)


base-commit: b723e9431b77976b83efb90178dfcada3405321c

Comments

Jonathan Cameron April 5, 2020, 10:02 a.m. UTC | #1

On Mon, 30 Mar 2020 16:59:20 +0200
Gaëtan André <[e-mail]> 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.
Hi Gaëtan,

Thanks for this.  Whilst I agree the ideal ABI would be to have just the
shared version userspace should cope with the current version anyway as
it would be the right option if for example the scale of x and y are controlled
by one register field and z by another (this used to be common for accelerometers)

Any userspace software using this will have to assign a precedence to the
two files that result and the most likely option is more specific first meaning
the shared version is unused.

Hence I'd argue we aren't broke (just non ideal) and adding the additional
interface just confuses matters.  Hence I would rather leave things how they
currently are.  Do we have some userspace that is broken by this being less
than ideal?

Thanks

Jonathan

> 
> Signed-off-by: Gaëtan André <[e-mail]>
> ---
>  include/linux/iio/common/st_sensors.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 33e939977444..f31e309f0fd1 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -52,6 +52,7 @@
>  	.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: b723e9431b77976b83efb90178dfcada3405321c

Gaëtan André April 5, 2020, 11:51 a.m. UTC | #2

On Sun, Apr 05, 2020 at 11:02:17AM +0100, Jonathan Cameron wrote:
> On Mon, 30 Mar 2020 16:59:20 +0200
> Gaëtan André <[e-mail]> 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.
> Hi Gaëtan,
> 
> Thanks for this.  Whilst I agree the ideal ABI would be to have just the
> shared version userspace should cope with the current version anyway as
> it would be the right option if for example the scale of x and y are controlled
> by one register field and z by another (this used to be common for accelerometers)
> 
> Any userspace software using this will have to assign a precedence to the
> two files that result and the most likely option is more specific first meaning
> the shared version is unused.
> 
> Hence I'd argue we aren't broke (just non ideal) and adding the additional
> interface just confuses matters.  Hence I would rather leave things how they
> currently are.  Do we have some userspace that is broken by this being less
> than ideal?
> 
Hi Jonathan,

Thanks for taking time to answer.

I don't have any point of view regarding what is better.

What I know is that iio-sensor-proxy [1] only looks for a common scale.
Hence, it won't work with ST sensors as is.

I could either do this patch or patch iio-sensor-proxy. What decided me
is that all ST sensors, if I am correct, use only one scale value for all axis.

If things are to be kept as is, then iio-sensor-proxy should be patched.

Also, note that currently in_acceleration_scale_{x, y, z} don't seem to
be documented.

Thanks,

Gaëtan

[1] iio-sensor-proxy: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/

> Thanks
> 
> Jonathan
> 
> > 
> > Signed-off-by: Gaëtan André <[e-mail]>
> > ---
> >  include/linux/iio/common/st_sensors.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> > index 33e939977444..f31e309f0fd1 100644
> > --- a/include/linux/iio/common/st_sensors.h
> > +++ b/include/linux/iio/common/st_sensors.h
> > @@ -52,6 +52,7 @@
> >  	.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: b723e9431b77976b83efb90178dfcada3405321c
>

Jonathan Cameron April 5, 2020, 12:24 p.m. UTC | #3

On Sun, 5 Apr 2020 13:51:39 +0200
Gaëtan André <[e-mail]> wrote:

> On Sun, Apr 05, 2020 at 11:02:17AM +0100, Jonathan Cameron wrote:
> > On Mon, 30 Mar 2020 16:59:20 +0200
> > Gaëtan André <[e-mail]> 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.  
> > Hi Gaëtan,
> > 
> > Thanks for this.  Whilst I agree the ideal ABI would be to have just the
> > shared version userspace should cope with the current version anyway as
> > it would be the right option if for example the scale of x and y are controlled
> > by one register field and z by another (this used to be common for accelerometers)
> > 
> > Any userspace software using this will have to assign a precedence to the
> > two files that result and the most likely option is more specific first meaning
> > the shared version is unused.
> > 
> > Hence I'd argue we aren't broke (just non ideal) and adding the additional
> > interface just confuses matters.  Hence I would rather leave things how they
> > currently are.  Do we have some userspace that is broken by this being less
> > than ideal?
> >   
> Hi Jonathan,
> 
> Thanks for taking time to answer.
> 
> I don't have any point of view regarding what is better.
> 
> What I know is that iio-sensor-proxy [1] only looks for a common scale.
> Hence, it won't work with ST sensors as is.

Ah.  Cc'd Bastien for that.

> 
> I could either do this patch or patch iio-sensor-proxy. What decided me
> is that all ST sensors, if I am correct, use only one scale value for all axis.
> 
> If things are to be kept as is, then iio-sensor-proxy should be patched.
Hmm. Even if we patched iio-sensor-proxy (and we probably should to support
devices that actually have different scales), we'd still have the normal
problem of people not upgrading their stacks terribly often.

Lets see what Bastien says, but (with the addition of a comment to say why
we are doing both in the code) I'm fine taking this patch given we have
known userspace issue without it.

> 
> Also, note that currently in_acceleration_scale_{x, y, z} don't seem to
> be documented.

Good spot.  We should tidy that up.  Seems in_magn_x_scale is there, but
not the in_accel_x_scale etc.  Likely some of the others are missing as well
but we never noticed.

Thanks

Jonathan

> 
> Thanks,
> 
> Gaëtan
> 
> [1] iio-sensor-proxy: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/
> 
> > Thanks
> > 
> > Jonathan
> >   
> > > 
> > > Signed-off-by: Gaëtan André <[e-mail]>
> > > ---
> > >  include/linux/iio/common/st_sensors.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> > > index 33e939977444..f31e309f0fd1 100644
> > > --- a/include/linux/iio/common/st_sensors.h
> > > +++ b/include/linux/iio/common/st_sensors.h
> > > @@ -52,6 +52,7 @@
> > >  	.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: b723e9431b77976b83efb90178dfcada3405321c  
> >

Patch

diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 33e939977444..f31e309f0fd1 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -52,6 +52,7 @@ 
 	.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, \