From: Krzysztof Kozlowski <[email protected]>
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 <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
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", },
{ },
};
--
2.39.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 <[email protected]>
>>
>> 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
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?
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
> From: Krzysztof Kozlowski <[email protected]>
>
> 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", },
> { },
> };
--
With Best Regards,
Andy Shevchenko
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
Hello,
On Wed, Apr 03, 2024 at 10:06:25AM +0200, Arnd Bergmann wrote:
> From: Krzysztof Kozlowski <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> 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 <[email protected]>");
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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > 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 <[email protected]>");
> 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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |