On Tue, 18 May 2021 11:01:37 +0200
Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx>
> > Cc: Denis Ciocca <denis.ciocca@xxxxxx>
> > Cc: Daniel Drake <drake@xxxxxxxxxxxx>
> > Cc: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> > 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.
My gut feeling is it's not worth the effort for such a small bit of
repetition, but I don't feel that strongly about it so will take
whatever comes in v3 ;)
Jonathan
>
> 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
