Re: sysfs mount_matrix for st_lsm6dsx gyro

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]

> 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.
> 
> Best regards
> 
> Philippe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]