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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 5 Apr 2020 13:51:39 +0200
Gaëtan André <rvlander@gaetanandre.eu> 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é <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.  
> > 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é <rvlander@gaetanandre.eu>
> > > ---
> > >  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  
> >