| Message ID | 20221121170911.7cd72bfc@endymion.delvare (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rc: Drop obsolete dependencies on COMPILE_TEST | expand |
On Mon, Nov 21, 2022 at 05:09:11PM +0100, Jean Delvare wrote: > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > is possible to test-build any driver which depends on OF on any > architecture by explicitly selecting OF. Therefore depending on > COMPILE_TEST as an alternative is no longer needed. > > It is actually better to always build such drivers with OF enabled, > so that the test builds are closer to how each driver will actually be > built on its intended target. Building them without OF may not test > much as the compiler will optimize out potentially large parts of the > code. In the worst case, this could even pop false positive warnings. > Dropping COMPILE_TEST here improves the quality of our testing and > avoids wasting time on non-existent issues. > > As a minor optimization, this also lets us drop of_match_ptr(), as we > now know what it will resolve to, we might as well save cpp some work. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Cc: Sean Young <sean@mess.org> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> > --- > drivers/media/rc/Kconfig | 4 ++-- > drivers/media/rc/pwm-ir-tx.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > --- linux-6.0.orig/drivers/media/rc/Kconfig > +++ linux-6.0/drivers/media/rc/Kconfig > @@ -314,7 +314,7 @@ config IR_PWM_TX > tristate "PWM IR transmitter" > depends on LIRC > depends on PWM > - depends on OF || COMPILE_TEST > + depends on OF > help > Say Y if you want to use a PWM based IR transmitter. This is > more power efficient than the bit banging gpio driver. > @@ -361,7 +361,7 @@ config IR_SERIAL_TRANSMITTER > config IR_SPI > tristate "SPI connected IR LED" > depends on SPI && LIRC > - depends on OF || COMPILE_TEST > + depends on OF > help > Say Y if you want to use an IR LED connected through SPI bus. > > --- linux-6.0.orig/drivers/media/rc/pwm-ir-tx.c > +++ linux-6.0/drivers/media/rc/pwm-ir-tx.c > @@ -120,7 +120,7 @@ static struct platform_driver pwm_ir_dri > .probe = pwm_ir_probe, > .driver = { > .name = DRIVER_NAME, > - .of_match_table = of_match_ptr(pwm_ir_of_match), > + .of_match_table = pwm_ir_of_match, > }, > }; > module_platform_driver(pwm_ir_driver); That hunk makes sense even without the Kconfig change. ACPI makes use of .of_match_table, so .of_match_table = of_match_ptr(pwm_ir_of_match), is (almost?) always wrong. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
Hallo Uwe, On Sun, 11 Dec 2022 21:56:48 +0100, Uwe Kleine-König wrote: > On Mon, Nov 21, 2022 at 05:09:11PM +0100, Jean Delvare wrote: > > --- linux-6.0.orig/drivers/media/rc/pwm-ir-tx.c > > +++ linux-6.0/drivers/media/rc/pwm-ir-tx.c > > @@ -120,7 +120,7 @@ static struct platform_driver pwm_ir_dri > > .probe = pwm_ir_probe, > > .driver = { > > .name = DRIVER_NAME, > > - .of_match_table = of_match_ptr(pwm_ir_of_match), > > + .of_match_table = pwm_ir_of_match, > > }, > > }; > > module_platform_driver(pwm_ir_driver); > > That hunk makes sense even without the Kconfig change. ACPI makes use of > .of_match_table, so > > .of_match_table = of_match_ptr(pwm_ir_of_match), > > is (almost?) always wrong. Should we just get rid of this macro altogether then? (Somehow I have a strange feeling that we already had this discussion...)
Hello, [expanded Cc: for the acpi topic] On Sun, Dec 11, 2022 at 11:14:35PM +0100, Jean Delvare wrote: > Hallo Uwe, > > On Sun, 11 Dec 2022 21:56:48 +0100, Uwe Kleine-König wrote: > > On Mon, Nov 21, 2022 at 05:09:11PM +0100, Jean Delvare wrote: > > > --- linux-6.0.orig/drivers/media/rc/pwm-ir-tx.c > > > +++ linux-6.0/drivers/media/rc/pwm-ir-tx.c > > > @@ -120,7 +120,7 @@ static struct platform_driver pwm_ir_dri > > > .probe = pwm_ir_probe, > > > .driver = { > > > .name = DRIVER_NAME, > > > - .of_match_table = of_match_ptr(pwm_ir_of_match), > > > + .of_match_table = pwm_ir_of_match, > > > }, > > > }; > > > module_platform_driver(pwm_ir_driver); > > > > That hunk makes sense even without the Kconfig change. ACPI makes use of > > .of_match_table, so > > > > .of_match_table = of_match_ptr(pwm_ir_of_match), > > > > is (almost?) always wrong. > > Should we just get rid of this macro altogether then? > > (Somehow I have a strange feeling that we already had this > discussion...) Might be. But for me this is only second hand knowledge, too. Maybe someone of the new recipents in this thread feels competent to comment here?! Best regards Uwe
On Mon, Dec 12, 2022 at 08:59:07AM +0100, Uwe Kleine-König wrote: > On Sun, Dec 11, 2022 at 11:14:35PM +0100, Jean Delvare wrote: > > On Sun, 11 Dec 2022 21:56:48 +0100, Uwe Kleine-König wrote: > > > On Mon, Nov 21, 2022 at 05:09:11PM +0100, Jean Delvare wrote: ... > > > > - .of_match_table = of_match_ptr(pwm_ir_of_match), > > > > + .of_match_table = pwm_ir_of_match, > > > That hunk makes sense even without the Kconfig change. ACPI makes use of > > > .of_match_table, so > > > > > > .of_match_table = of_match_ptr(pwm_ir_of_match), > > > > > > is (almost?) always wrong. > > > > Should we just get rid of this macro altogether then? > > > > (Somehow I have a strange feeling that we already had this > > discussion...) > > Might be. But for me this is only second hand knowledge, too. Maybe > someone of the new recipents in this thread feels competent to comment > here?! Pros of of_match_ptr() / ACPI_PTR(): - saves a few dozens of bytes in the module ID tables - doesn't show ACPI ID for non-ACPI platform or OF ID on non-OF platforms Cons: - prevents from using OF IDs on ACPI platforms - doesn't show ACPI ID for non-ACPI platform or OF ID on non-OF platforms - makes error prone for the compiler to have the variable unused - makes code uglier (I left the second in the both because I find useful to have all supported IDs to be listed even if the system is compiled with OF/ACPI opted-out.) Personally I remove the of_match_ptr()/ACPI_PTR() from drivers that can be used on OF or ACPI platforms, which leaves us only with the drivers we are 100% sure that they won't ever be used on non-OF platforms. BUT, I do not see any sense to have of_match_ptr() that either in use, because the driver in question is 100% for OF platform, or not when it's compile tested, which means it reduces test coverage anyway. All the same for ACPI_PTR(). TL;DR: I don't see any [big] usefulness of keeping those macros.
Hello, On Mon, Nov 21, 2022 at 05:09:11PM +0100, Jean Delvare wrote: > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > is possible to test-build any driver which depends on OF on any > architecture by explicitly selecting OF. Therefore depending on > COMPILE_TEST as an alternative is no longer needed. > > It is actually better to always build such drivers with OF enabled, > so that the test builds are closer to how each driver will actually be > built on its intended target. Building them without OF may not test > much as the compiler will optimize out potentially large parts of the > code. In the worst case, this could even pop false positive warnings. > Dropping COMPILE_TEST here improves the quality of our testing and > avoids wasting time on non-existent issues. > > As a minor optimization, this also lets us drop of_match_ptr(), as we > now know what it will resolve to, we might as well save cpp some work. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Cc: Sean Young <sean@mess.org> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> FTR: I discard this patch from the PWM patchwork as "handled elsewhere". Best regards Uwe
--- linux-6.0.orig/drivers/media/rc/Kconfig +++ linux-6.0/drivers/media/rc/Kconfig @@ -314,7 +314,7 @@ config IR_PWM_TX tristate "PWM IR transmitter" depends on LIRC depends on PWM - depends on OF || COMPILE_TEST + depends on OF help Say Y if you want to use a PWM based IR transmitter. This is more power efficient than the bit banging gpio driver. @@ -361,7 +361,7 @@ config IR_SERIAL_TRANSMITTER config IR_SPI tristate "SPI connected IR LED" depends on SPI && LIRC - depends on OF || COMPILE_TEST + depends on OF help Say Y if you want to use an IR LED connected through SPI bus. --- linux-6.0.orig/drivers/media/rc/pwm-ir-tx.c +++ linux-6.0/drivers/media/rc/pwm-ir-tx.c @@ -120,7 +120,7 @@ static struct platform_driver pwm_ir_dri .probe = pwm_ir_probe, .driver = { .name = DRIVER_NAME, - .of_match_table = of_match_ptr(pwm_ir_of_match), + .of_match_table = pwm_ir_of_match, }, }; module_platform_driver(pwm_ir_driver);
Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it is possible to test-build any driver which depends on OF on any architecture by explicitly selecting OF. Therefore depending on COMPILE_TEST as an alternative is no longer needed. It is actually better to always build such drivers with OF enabled, so that the test builds are closer to how each driver will actually be built on its intended target. Building them without OF may not test much as the compiler will optimize out potentially large parts of the code. In the worst case, this could even pop false positive warnings. Dropping COMPILE_TEST here improves the quality of our testing and avoids wasting time on non-existent issues. As a minor optimization, this also lets us drop of_match_ptr(), as we now know what it will resolve to, we might as well save cpp some work. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: Sean Young <sean@mess.org> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> --- drivers/media/rc/Kconfig | 4 ++-- drivers/media/rc/pwm-ir-tx.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)