| Message ID | 20240403080702.3509288-8-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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", }, > { }, > };
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
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?
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 <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
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 --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", }, { }, };