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

[Uwe Kleine-König]

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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxx>");
>  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/ |

Attachment: signature.asc
Description: PGP signature