Re: sysfs mount_matrix for st_lsm6dsx gyro

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 12 Jan 2023 12:32:11 +0100
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > Hello Lorenzo,
> > 
> > On Thu, Jan 12, 2023 at 10:51:03AM +0100, Lorenzo Bianconi wrote:  
> > > Date: Thu, 12 Jan 2023 10:51:03 +0100
> > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Philippe De Muyter <phdm@macq.eu>, linux-iio@vger.kernel.org
> > > Subject: Re: sysfs mount_matrix for st_lsm6dsx gyro
> > >   
> > > > On Wed, 11 Jan 2023 13:09:40 +0100
> > > > Philippe De Muyter <phdm@macq.eu> wrote:
> > > >   
> > > > > Hello Lorenzo and list,
> > > > > 
> > > > > I do not find a "*mount_matrix" entry in sysfs for a 'ism330dlc_gyro'
> > > > > iio device.
> > > > > Is that normal ?
> > > > > Is a fix available ?  
> > > > 
> > > > Looks like the channel definition for the gyro does not include an
> > > > appropriate ext_info entry unlike the accelerometer channels which
> > > > have one with mount_matrix support.
> > > > 
> > > > From a quick glance looks like a simple fix. Add that entry.  
> > > 
> > > I am not sure if accel and gyro can be mounted with a different orientation.
> > > Do you think we should have a per-sensor mount_matrix?  
> > 
> > My chip is a 'ism330dlc'.  It's one chip containing an accel and a gyro,
> > so the mount_matrix should be the same for the accel and the gyro.
> > Unfortunately the accel and the gyro are presented in /sysfs as two
> > separate devices, and only the accel one has a 'mount_matrix' entry.
> > So a user looking for any gyro will find the gyro entry, but without
> > 'mount_matrix'.
> > 
> > I have followed Jonathan's proposal and added this simple patch in
> > the definition of the ST_LSM6DSX_CHANNEL macro :
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > index d80ba2e688ed..9d18145d5041 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > @@ -96,6 +96,7 @@ enum st_lsm6dsx_hw_id {
> >  		.storagebits = 16,                                      \
> >  		.endianness = IIO_LE,                                   \
> > 	},                                                              \
> > +	.ext_info = st_lsm6dsx_accel_ext_info,                          \
> >  }  
> 
> I am fine with this approach, probably having a per-sensor mount_matrix is
> unnecessary. Can you please just rename st_lsm6dsx_accel_ext_info in
> st_lsm6dsx_ext_info?
> 
> Regards,
> Lorenzo
> 
> > 
> >  struct st_lsm6dsx_reg {
> > 
> > Another fix would be to create only one chip entry in sysfs, but that's
> > above my knowledge of this driver.

We can't make that change. It would be ABI breakage - userspace code would
stop working.  

Jonathan

> > 
> > Best regards
> > 
> > Philippe  
>