| Message ID | 20240417164616.74651-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 | expand |
On 4/17/24 6:46 PM, Hans de Goede wrote: > Hi All, > > Here is a patch series to refactor the ACPI ROTM orientation matrix > handling in kxcjk-1013 + bmc150-accel to share the code instead of > having 2 copies and then also use the shared implementation in > the mxc4005 driver since some MXC6655 ACPI firmware nodes also > include this. > > Note the mxc4005 support is untested, I will ask the report of: > https://bugzilla.kernel.org/show_bug.cgi?id=218578 to test. The ROTM support has been tested on a tablet with a MXC6655 now and has been confirmed to work. Regards, Hans > Hans de Goede (4): > iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival > iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h > iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper > iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method > > drivers/iio/accel/acpi-helpers.h | 76 +++++++++++++++++++++++++ > drivers/iio/accel/bmc150-accel-core.c | 45 +-------------- > drivers/iio/accel/kxcjk-1013.c | 82 +-------------------------- > drivers/iio/accel/mxc4005.c | 24 ++++++++ > 4 files changed, 106 insertions(+), 121 deletions(-) > create mode 100644 drivers/iio/accel/acpi-helpers.h >
Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: > Hi All, > > Here is a patch series to refactor the ACPI ROTM orientation matrix > handling in kxcjk-1013 + bmc150-accel to share the code instead of > having 2 copies and then also use the shared implementation in > the mxc4005 driver since some MXC6655 ACPI firmware nodes also > include this. You beat me up to it, as I (used to?) have it in my TODO list for a couple of years or so. Thanks for doing this! > Note the mxc4005 support is untested, I will ask the report of: > https://bugzilla.kernel.org/show_bug.cgi?id=218578 to test.
Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: > Hi All, > > Here is a patch series to refactor the ACPI ROTM orientation matrix > handling in kxcjk-1013 + bmc150-accel to share the code instead of > having 2 copies and then also use the shared implementation in > the mxc4005 driver since some MXC6655 ACPI firmware nodes also > include this. > > Note the mxc4005 support is untested, I will ask the report of: I have briefly looked into this and I like this, except the part of the big function being in the header. Why? Why can't it be in a C-file? Note, 3 users justify very well to me that shared code, should be shared in binary as well. (I.o.w. you may argue that IRL there will be no more than one of such device connected, but in case of DIY and prototyping it might still be the use case.)
Hi, On 4/22/24 9:55 AM, Andy Shevchenko wrote: > Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: >> Hi All, >> >> Here is a patch series to refactor the ACPI ROTM orientation matrix >> handling in kxcjk-1013 + bmc150-accel to share the code instead of >> having 2 copies and then also use the shared implementation in >> the mxc4005 driver since some MXC6655 ACPI firmware nodes also >> include this. >> >> Note the mxc4005 support is untested, I will ask the report of: > > I have briefly looked into this and I like this, except the part of the big > function being in the header. Why? Why can't it be in a C-file? > > Note, 3 users justify very well to me that shared code, should be shared in > binary as well. (I.o.w. you may argue that IRL there will be no more than > one of such device connected, but in case of DIY and prototyping it might > still be the use case.) It is only 1 function and it is not that big. IMHO the static inline in a header solution here is much better then making this a separate .ko file with all the associated overhead. Regards, Hans
On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote: > On 4/22/24 9:55 AM, Andy Shevchenko wrote: > > Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: ... > > I have briefly looked into this and I like this, except the part of the big > > function being in the header. Why? Why can't it be in a C-file? > > > > Note, 3 users justify very well to me that shared code, should be shared in > > binary as well. (I.o.w. you may argue that IRL there will be no more than > > one of such device connected, but in case of DIY and prototyping it might > > still be the use case.) > > It is only 1 function and it is not that big. IMHO the static inline > in a header solution here is much better then making this a separate .ko > file with all the associated overhead. Look how the i8042 RTC header became a disaster. :-) Nevertheless, this can be part of the IIO core for the ACPI enabled kernels. Which eliminates a need for a separate module.
Hi, On 4/22/24 1:33 PM, Andy Shevchenko wrote: > On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote: >> On 4/22/24 9:55 AM, Andy Shevchenko wrote: >>> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: > > ... > >>> I have briefly looked into this and I like this, except the part of the big >>> function being in the header. Why? Why can't it be in a C-file? >>> >>> Note, 3 users justify very well to me that shared code, should be shared in >>> binary as well. (I.o.w. you may argue that IRL there will be no more than >>> one of such device connected, but in case of DIY and prototyping it might >>> still be the use case.) >> >> It is only 1 function and it is not that big. IMHO the static inline >> in a header solution here is much better then making this a separate .ko >> file with all the associated overhead. > > Look how the i8042 RTC header became a disaster. :-) > Nevertheless, this can be part of the IIO core for the ACPI enabled > kernels. Which eliminates a need for a separate module. Putting this in the IIO core, with an iio-prefix, so say something like: #ifdef CONFIG_ACPI bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name); #else static inline bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name) { return false; } in include/linux/iio/iio.h ? works for me and that also avoids Jonathan's worry about using an acpi_ prefix in iio code. Jonathan how does that sound to you ? Regards, Hans
On Mon, Apr 22, 2024 at 2:45 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 4/22/24 1:33 PM, Andy Shevchenko wrote: > > On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 4/22/24 9:55 AM, Andy Shevchenko wrote: > >>> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: ... > >>> I have briefly looked into this and I like this, except the part of the big > >>> function being in the header. Why? Why can't it be in a C-file? > >>> > >>> Note, 3 users justify very well to me that shared code, should be shared in > >>> binary as well. (I.o.w. you may argue that IRL there will be no more than > >>> one of such device connected, but in case of DIY and prototyping it might > >>> still be the use case.) > >> > >> It is only 1 function and it is not that big. IMHO the static inline > >> in a header solution here is much better then making this a separate .ko > >> file with all the associated overhead. > > > > Look how the i8042 RTC header became a disaster. :-) > > Nevertheless, this can be part of the IIO core for the ACPI enabled > > kernels. Which eliminates a need for a separate module. > > Putting this in the IIO core, with an iio-prefix, so say something like: > > #ifdef CONFIG_ACPI > bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name); > #else > static inline bool > iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name) > { > return false; > } > > in include/linux/iio/iio.h ? Yes, like SPI, I²C, etc. do in similar cases. > works for me and that also avoids Jonathan's worry about using an acpi_ > prefix in iio code. > > Jonathan how does that sound to you ?
On Mon, 22 Apr 2024 15:28:48 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 22, 2024 at 2:45 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 4/22/24 1:33 PM, Andy Shevchenko wrote: > > > On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote: > > >> On 4/22/24 9:55 AM, Andy Shevchenko wrote: > > >>> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti: > > ... > > > >>> I have briefly looked into this and I like this, except the part of the big > > >>> function being in the header. Why? Why can't it be in a C-file? > > >>> > > >>> Note, 3 users justify very well to me that shared code, should be shared in > > >>> binary as well. (I.o.w. you may argue that IRL there will be no more than > > >>> one of such device connected, but in case of DIY and prototyping it might > > >>> still be the use case.) > > >> > > >> It is only 1 function and it is not that big. IMHO the static inline > > >> in a header solution here is much better then making this a separate .ko > > >> file with all the associated overhead. > > > > > > Look how the i8042 RTC header became a disaster. :-) > > > Nevertheless, this can be part of the IIO core for the ACPI enabled > > > kernels. Which eliminates a need for a separate module. > > > > Putting this in the IIO core, with an iio-prefix, so say something like: > > > > #ifdef CONFIG_ACPI > > bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name); > > #else > > static inline bool > > iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name) > > { > > return false; > > } > > > > in include/linux/iio/iio.h ? > > Yes, like SPI, I²C, etc. do in similar cases. > > > works for me and that also avoids Jonathan's worry about using an acpi_ > > prefix in iio code. > > > > Jonathan how does that sound to you ? > Good