[PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[Jean Delvare]

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);


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[Jean Delvare]

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

Re: [PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[Andy Shevchenko]

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

Re: [PATCH] media: rc: Drop obsolete dependencies on COMPILE_TEST

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]