Re: [PATCH 4/5 v2] iio: magnetometer: st_magn: Support mount matrix

From: Stephan Gerhold <stephan@gerhold.net>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Jonathan Cameron <jic23@kernel.org>,
	linux-iio@vger.kernel.org, Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Denis Ciocca <denis.ciocca@st.com>,
	Daniel Drake <drake@endlessm.com>
Subject: Re: [PATCH 4/5 v2] iio: magnetometer: st_magn: Support mount matrix
Date: Tue, 18 May 2021 11:01:37 +0200	[thread overview]
Message-ID: <YKOCcQNfJ1t7YmGB@gerhold.net> (raw)
In-Reply-To: <20210517233322.383043-4-linus.walleij@linaro.org>

Hi,

On Tue, May 18, 2021 at 01:33:21AM +0200, Linus Walleij wrote:
> Add support to read and present the mounting matrix on ST magnetometers.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Cc: Daniel Drake <drake@endlessm.com>
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch because why not.

Thanks for patching the other ST drivers as well!
That reminds me about something I was thinking about when I was making
the changes a week ago. :)

> ---
>  drivers/iio/magnetometer/st_magn_core.c | 64 ++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
> index 71faebd07feb..fa587975cb85 100644
> --- a/drivers/iio/magnetometer/st_magn_core.c
> +++ b/drivers/iio/magnetometer/st_magn_core.c
> @@ -53,51 +53,74 @@
>  #define ST_MAGN_3_OUT_Y_L_ADDR			0x6a
>  #define ST_MAGN_3_OUT_Z_L_ADDR			0x6c
>  
> +static const struct iio_mount_matrix *
> +st_magn_get_mount_matrix(const struct iio_dev *indio_dev,
> +			 const struct iio_chan_spec *chan)
> +{
> +	struct st_sensor_data *mdata = iio_priv(indio_dev);
> +
> +	return &mdata->mount_matrix;
> +}
> +
> +static const struct iio_chan_spec_ext_info st_magn_mount_matrix_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, st_magn_get_mount_matrix),
> +	{ },
> +};
> +

I'm not sure if this is worth it (or even a particularly good idea),
but we could share this function and struct in st_sensors_core.c
since it's exactly the same for st_accel/magn/gyro AFAICT.
(It just operates on the common struct st_sensor_data).

This could be done with a macro like ST_SENSORS_LSM_CHANNELS_MOUNT_MATRIX(...)
(maybe a bit long and clumsy) instead of _EXT(...) and pointing to
a struct iio_chan_spec_ext_info somewhere in st_sensors_core.c.

The disadvantage however is that st_accel/magn/gyro couldn't add other
(sensor-specific) ext_info later. Not sure if that is realistic though.

I wasn't entirely sure myself that's why I went with the _EXT(...)
instead (especially since I only wanted to patch st_accel).
Just wanted to mention it :)

Stephan