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 <[email protected]>
Cc: Sean Young <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: "Uwe Kleine-König" <[email protected]>
---
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);
--
Jean Delvare
SUSE L3 Support
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 <[email protected]>
> Cc: Sean Young <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: "Uwe Kleine-K?nig" <[email protected]>
> ---
> 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 <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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...)
--
Jean Delvare
SUSE L3 Support
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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> Cc: Sean Young <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: "Uwe Kleine-K?nig" <[email protected]>
FTR: I discard this patch from the PWM patchwork as "handled elsewhere".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |