diff mbox series

[07/34] Input: stmpe-ts - mark OF related data as maybe unused

Message ID 20240403080702.3509288-8-arnd@kernel.org (mailing list archive)
State New
Headers show
Series address all -Wunused-const warnings | expand

Commit Message

Arnd Bergmann April 3, 2024, 8:06 a.m. UTC 
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

When compile tested with W=1 on x86_64 with driver as built-in:

  stmpe-ts.c:371:34: error: unused variable 'stmpe_ts_ids' [-Werror,-Wunused-const-variable]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/input/touchscreen/stmpe-ts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andy Shevchenko April 3, 2024, 9:40 a.m. UTC | #1 
On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> When compile tested with W=1 on x86_64 with driver as built-in:
> 
>   stmpe-ts.c:371:34: error: unused variable 'stmpe_ts_ids' [-Werror,-Wunused-const-variable]

...

> -static const struct of_device_id stmpe_ts_ids[] = {
> +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {

__maybe_unused? 

Why not adding it into .driver as you have done in another patch in this series?

>  	{ .compatible = "st,stmpe-ts", },
>  	{ },
>  };
Krzysztof Kozlowski April 3, 2024, 9:52 a.m. UTC | #2 
On 03/04/2024 11:40, Andy Shevchenko wrote:
> On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> When compile tested with W=1 on x86_64 with driver as built-in:
>>
>>   stmpe-ts.c:371:34: error: unused variable 'stmpe_ts_ids' [-Werror,-Wunused-const-variable]
> 
> ...
> 
>> -static const struct of_device_id stmpe_ts_ids[] = {
>> +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {
> 
> __maybe_unused? 
> 
> Why not adding it into .driver as you have done in another patch in this series?

Because there is no benefit in this. This is instantiated by MFD, so the
only thing you need is entry for module loading.

Best regards,
Krzysztof
Andy Shevchenko April 3, 2024, 10:03 a.m. UTC | #3 
On Wed, Apr 03, 2024 at 11:52:12AM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2024 11:40, Andy Shevchenko wrote:
> > On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:

...

> >> -static const struct of_device_id stmpe_ts_ids[] = {
> >> +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {
> > 
> > __maybe_unused? 
> > 
> > Why not adding it into .driver as you have done in another patch in this series?
> 
> Because there is no benefit in this. This is instantiated by MFD, so the
> only thing you need is entry for module loading.

Hmm... Seems to me rather a good candidate for MODULE_ALIAS in this case. No?
Krzysztof Kozlowski April 3, 2024, 10:10 a.m. UTC | #4 
On 03/04/2024 12:03, Andy Shevchenko wrote:
> On Wed, Apr 03, 2024 at 11:52:12AM +0200, Krzysztof Kozlowski wrote:
>> On 03/04/2024 11:40, Andy Shevchenko wrote:
>>> On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
> 
> ...
> 
>>>> -static const struct of_device_id stmpe_ts_ids[] = {
>>>> +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {
>>>
>>> __maybe_unused? 
>>>
>>> Why not adding it into .driver as you have done in another patch in this series?
>>
>> Because there is no benefit in this. This is instantiated by MFD, so the
>> only thing you need is entry for module loading.
> 
> Hmm... Seems to me rather a good candidate for MODULE_ALIAS in this case. No?

No, I do not think module alias is for that purpose. This is a valid
compatible, documented and provided by DT so it is expected to be in
of_device_id.

Best regards,
Krzysztof
Uwe Kleine-König April 3, 2024, 1:17 p.m. UTC | #5 
Hello,

On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> When compile tested with W=1 on x86_64 with driver as built-in:
> 
>   stmpe-ts.c:371:34: error: unused variable 'stmpe_ts_ids' [-Werror,-Wunused-const-variable]
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/input/touchscreen/stmpe-ts.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
> index b204fdb2d22c..022b3e94266d 100644
> --- a/drivers/input/touchscreen/stmpe-ts.c
> +++ b/drivers/input/touchscreen/stmpe-ts.c
> @@ -366,7 +366,7 @@ static struct platform_driver stmpe_ts_driver = {
>  };
>  module_platform_driver(stmpe_ts_driver);
>  
> -static const struct of_device_id stmpe_ts_ids[] = {
> +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {
>  	{ .compatible = "st,stmpe-ts", },
>  	{ },
>  };

I'd suggest the following instead:

diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
index b204fdb2d22c..e1afebc641ec 100644
--- a/drivers/input/touchscreen/stmpe-ts.c
+++ b/drivers/input/touchscreen/stmpe-ts.c
@@ -357,21 +357,22 @@ static void stmpe_ts_remove(struct platform_device *pdev)
 	stmpe_disable(ts->stmpe, STMPE_BLOCK_TOUCHSCREEN);
 }
 
-static struct platform_driver stmpe_ts_driver = {
-	.driver = {
-		.name = STMPE_TS_NAME,
-	},
-	.probe = stmpe_input_probe,
-	.remove_new = stmpe_ts_remove,
-};
-module_platform_driver(stmpe_ts_driver);
-
 static const struct of_device_id stmpe_ts_ids[] = {
 	{ .compatible = "st,stmpe-ts", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, stmpe_ts_ids);
 
+static struct platform_driver stmpe_ts_driver = {
+	.driver = {
+		.name = STMPE_TS_NAME,
+		.of_match_table = stmpe_ts_ids,
+	},
+	.probe = stmpe_input_probe,
+	.remove_new = stmpe_ts_remove,
+};
+module_platform_driver(stmpe_ts_driver);
+
 MODULE_AUTHOR("Luotao Fu <l.fu@pengutronix.de>");
 MODULE_DESCRIPTION("STMPEXXX touchscreen driver");
 MODULE_LICENSE("GPL");

I wonder if with the status quo binding via dt works at all with
stmpe_ts_driver.driver.of_match_table unset?!

Best regards
Uwe
Uwe Kleine-König April 3, 2024, 1:27 p.m. UTC | #6 
Hello again,

On Wed, Apr 03, 2024 at 03:17:32PM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 
> > When compile tested with W=1 on x86_64 with driver as built-in:
> > 
> >   stmpe-ts.c:371:34: error: unused variable 'stmpe_ts_ids' [-Werror,-Wunused-const-variable]
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/input/touchscreen/stmpe-ts.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
> > index b204fdb2d22c..022b3e94266d 100644
> > --- a/drivers/input/touchscreen/stmpe-ts.c
> > +++ b/drivers/input/touchscreen/stmpe-ts.c
> > @@ -366,7 +366,7 @@ static struct platform_driver stmpe_ts_driver = {
> >  };
> >  module_platform_driver(stmpe_ts_driver);
> >  
> > -static const struct of_device_id stmpe_ts_ids[] = {
> > +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {
> >  	{ .compatible = "st,stmpe-ts", },
> >  	{ },
> >  };
> 
> I'd suggest the following instead:
> 
> diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
> index b204fdb2d22c..e1afebc641ec 100644
> --- a/drivers/input/touchscreen/stmpe-ts.c
> +++ b/drivers/input/touchscreen/stmpe-ts.c
> @@ -357,21 +357,22 @@ static void stmpe_ts_remove(struct platform_device *pdev)
>  	stmpe_disable(ts->stmpe, STMPE_BLOCK_TOUCHSCREEN);
>  }
>  
> -static struct platform_driver stmpe_ts_driver = {
> -	.driver = {
> -		.name = STMPE_TS_NAME,
> -	},
> -	.probe = stmpe_input_probe,
> -	.remove_new = stmpe_ts_remove,
> -};
> -module_platform_driver(stmpe_ts_driver);
> -
>  static const struct of_device_id stmpe_ts_ids[] = {
>  	{ .compatible = "st,stmpe-ts", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, stmpe_ts_ids);
>  
> +static struct platform_driver stmpe_ts_driver = {
> +	.driver = {
> +		.name = STMPE_TS_NAME,
> +		.of_match_table = stmpe_ts_ids,
> +	},
> +	.probe = stmpe_input_probe,
> +	.remove_new = stmpe_ts_remove,
> +};
> +module_platform_driver(stmpe_ts_driver);
> +
>  MODULE_AUTHOR("Luotao Fu <l.fu@pengutronix.de>");
>  MODULE_DESCRIPTION("STMPEXXX touchscreen driver");
>  MODULE_LICENSE("GPL");
> 
> I wonder if with the status quo binding via dt works at all with
> stmpe_ts_driver.driver.of_match_table unset?!

I missed the discussion between Andy and Krzysztof when I wrote my mail.
I still think this should be considered and if .of_match_table should
stay unassigned (e.g. to allow dropping stmpe_ts_ids in case the driver
is built-in?) I think adding a code comment would be appropriate because
having an of_device_id array but not adding it to the driver is unusuall
and generally a bad template for new drivers.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
index b204fdb2d22c..022b3e94266d 100644
--- a/drivers/input/touchscreen/stmpe-ts.c
+++ b/drivers/input/touchscreen/stmpe-ts.c
@@ -366,7 +366,7 @@  static struct platform_driver stmpe_ts_driver = {
 };
 module_platform_driver(stmpe_ts_driver);
 
-static const struct of_device_id stmpe_ts_ids[] = {
+static const struct of_device_id stmpe_ts_ids[] __maybe_unused = {
 	{ .compatible = "st,stmpe-ts", },
 	{ },
 };