| Message ID | 20240403080702.3509288-32-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Apr 03, 2024 at 10:06:49AM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > When building with CONFIG_OF disabled but W=1 extra warnings enabled, > a couple of driver cause a warning about an unused ID table: > > drivers/spi/spi-armada-3700.c:806:34: error: unused variable 'a3700_spi_dt_ids' [-Werror,-Wunused-const-variable] > drivers/spi/spi-orion.c:614:34: error: unused variable 'orion_spi_of_match_table' [-Werror,-Wunused-const-variable] > drivers/spi/spi-pic32-sqi.c:673:34: error: unused variable 'pic32_sqi_of_ids' [-Werror,-Wunused-const-variable] > drivers/spi/spi-pic32.c:850:34: error: unused variable 'pic32_spi_of_match' [-Werror,-Wunused-const-variable] > drivers/spi/spi-rockchip.c:1020:34: error: unused variable 'rockchip_spi_dt_match' [-Werror,-Wunused-const-variable] > drivers/spi/spi-s3c64xx.c:1642:34: error: unused variable 's3c64xx_spi_dt_match' [-Werror,-Wunused-const-variable] > drivers/spi/spi-st-ssc4.c:439:34: error: unused variable 'stm_spi_match' [-Werror,-Wunused-const-variable] I would drop these 'drivers/spi/' parts as we know they are all in the same folder. > These appear to all be copied from the same original driver, so fix them at the > same time by removing the unnecessary of_match_ptr() annotation. As far as I > can tell, all these drivers are only actually used on configurations that > have CONFIG_OF enabled. LGTM, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On 03/04/2024 10:06, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > When building with CONFIG_OF disabled but W=1 extra warnings enabled, > a couple of driver cause a warning about an unused ID table: > > drivers/spi/spi-armada-3700.c:806:34: error: unused variable 'a3700_spi_dt_ids' [-Werror,-Wunused-const-variable] > drivers/spi/spi-orion.c:614:34: error: unused variable 'orion_spi_of_match_table' [-Werror,-Wunused-const-variable] > drivers/spi/spi-pic32-sqi.c:673:34: error: unused variable 'pic32_sqi_of_ids' [-Werror,-Wunused-const-variable] > drivers/spi/spi-pic32.c:850:34: error: unused variable 'pic32_spi_of_match' [-Werror,-Wunused-const-variable] > drivers/spi/spi-rockchip.c:1020:34: error: unused variable 'rockchip_spi_dt_match' [-Werror,-Wunused-const-variable] > drivers/spi/spi-s3c64xx.c:1642:34: error: unused variable 's3c64xx_spi_dt_match' [-Werror,-Wunused-const-variable] > drivers/spi/spi-st-ssc4.c:439:34: error: unused variable 'stm_spi_match' [-Werror,-Wunused-const-variable] > > These appear to all be copied from the same original driver, so fix them at the > same time by removing the unnecessary of_match_ptr() annotation. As far as I > can tell, all these drivers are only actually used on configurations that > have CONFIG_OF enabled. I think I already tried to fix all of these, but Mark rejected my patches: https://lore.kernel.org/all/7a65d775-cf07-4393-8b10-2cef4d5266ab@sirena.org.uk/ All of the changes here look the same as my patchset, I also got there some Acks. Best regards, Krzysztof
On Wed, Apr 03, 2024 at 10:06:49AM +0200, Arnd Bergmann wrote: > These appear to all be copied from the same original driver, so fix them at the > same time by removing the unnecessary of_match_ptr() annotation. As far as I > can tell, all these drivers are only actually used on configurations that > have CONFIG_OF enabled. Why are we not fixing of_match_ptr() here, or at least adding the ifdefs in case someone does end up wanting to run without OF? Just as a general thing for something like this where the patches aren't expected to get merged together it makes life much easier to not send as a series - pulling individual patches out of a series causes issues with things like b4, especially if you have to apply them to multiple places, and there's limited benefit.
On Wed, Apr 03, 2024 at 10:56:58AM +0100, Mark Brown wrote: > On Wed, Apr 03, 2024 at 10:06:49AM +0200, Arnd Bergmann wrote: > > > These appear to all be copied from the same original driver, so fix them at the > > same time by removing the unnecessary of_match_ptr() annotation. As far as I > > can tell, all these drivers are only actually used on configurations that > > have CONFIG_OF enabled. > > Why are we not fixing of_match_ptr() here, or at least adding the ifdefs > in case someone does end up wanting to run without OF? Fixing of_match_ptr = diff --git a/include/linux/of.h b/include/linux/of.h index a0bedd038a05..d980bccffda0 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -890,7 +890,7 @@ static inline const void *of_device_get_match_data(const struct device *dev) return NULL; } -#define of_match_ptr(_ptr) NULL +#define of_match_ptr(_ptr) (0 ? (_ptr) : NULL) #define of_match_node(_matches, _node) NULL #endif /* CONFIG_OF */ ? Assuming this helps, I agree this would be the better fix. Best regards Uwe
Wed, Apr 03, 2024 at 11:05:51PM +0200, Uwe Kleine-König kirjoitti: > On Wed, Apr 03, 2024 at 10:56:58AM +0100, Mark Brown wrote: > > On Wed, Apr 03, 2024 at 10:06:49AM +0200, Arnd Bergmann wrote: > > > > > These appear to all be copied from the same original driver, so fix them at the > > > same time by removing the unnecessary of_match_ptr() annotation. As far as I > > > can tell, all these drivers are only actually used on configurations that > > > have CONFIG_OF enabled. > > > > Why are we not fixing of_match_ptr() here, or at least adding the ifdefs > > in case someone does end up wanting to run without OF? > > Fixing of_match_ptr = > > diff --git a/include/linux/of.h b/include/linux/of.h > index a0bedd038a05..d980bccffda0 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -890,7 +890,7 @@ static inline const void *of_device_get_match_data(const struct device *dev) > return NULL; > } > > -#define of_match_ptr(_ptr) NULL > +#define of_match_ptr(_ptr) (0 ? (_ptr) : NULL) FWIW, we have PTR_IF() (with a side note to split it from kernel.h in a separate header or less twisted one). > #define of_match_node(_matches, _node) NULL > #endif /* CONFIG_OF */ > > ? > > Assuming this helps, I agree this would be the better fix. Why? I mean why do we need to even have this API? It's always good to know which devices are supported by the module even if you have no need in such support or it's not compiled in. One of the reasons why is to be able to google for compatible hardware, for example.
On Wed, Apr 3, 2024, at 23:13, Andy Shevchenko wrote: > Wed, Apr 03, 2024 at 11:05:51PM +0200, Uwe Kleine-König kirjoitti: >> On Wed, Apr 03, 2024 at 10:56:58AM +0100, Mark Brown wrote: >> > On Wed, Apr 03, 2024 at 10:06:49AM +0200, Arnd Bergmann wrote: >> > >> > > These appear to all be copied from the same original driver, so fix them at the >> > > same time by removing the unnecessary of_match_ptr() annotation. As far as I >> > > can tell, all these drivers are only actually used on configurations that >> > > have CONFIG_OF enabled. >> > >> > Why are we not fixing of_match_ptr() here, or at least adding the ifdefs >> > in case someone does end up wanting to run without OF? >> >> Fixing of_match_ptr = >> >> diff --git a/include/linux/of.h b/include/linux/of.h >> index a0bedd038a05..d980bccffda0 100644 >> --- a/include/linux/of.h >> +++ b/include/linux/of.h >> @@ -890,7 +890,7 @@ static inline const void *of_device_get_match_data(const struct device *dev) >> return NULL; >> } >> >> -#define of_match_ptr(_ptr) NULL >> +#define of_match_ptr(_ptr) (0 ? (_ptr) : NULL) This would require removing several hundred "#ifdef CONFIG_OF" checks around the of_device_id tables at the same time unfortunately. Most of those are completely unnecessary, so if we wanted to remove those, we should remove the of_match_ptr() instances at the same time. >> #define of_match_node(_matches, _node) NULL >> #endif /* CONFIG_OF */ >> >> ? >> >> Assuming this helps, I agree this would be the better fix. > > Why? I mean why do we need to even have this API? It's always > good to know which devices are supported by the module even > if you have no need in such support or it's not compiled in. > One of the reasons why is to be able to google for compatible hardware, > for example. As far as I can tell, the of_match_ptr() helper was a historic mistake, it makes pretty much no sense in its current form. The version that Uwe proposes would have made sense but we can't change it now. Arnd
diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c index 3c9ed412932f..2fc2e7c0daf2 100644 --- a/drivers/spi/spi-armada-3700.c +++ b/drivers/spi/spi-armada-3700.c @@ -902,7 +902,7 @@ static int a3700_spi_probe(struct platform_device *pdev) static struct platform_driver a3700_spi_driver = { .driver = { .name = DRIVER_NAME, - .of_match_table = of_match_ptr(a3700_spi_dt_ids), + .of_match_table = a3700_spi_dt_ids, }, .probe = a3700_spi_probe, }; diff --git a/drivers/spi/spi-img-spfi.c b/drivers/spi/spi-img-spfi.c index d8360f94d3b7..0d7f89301f8d 100644 --- a/drivers/spi/spi-img-spfi.c +++ b/drivers/spi/spi-img-spfi.c @@ -753,7 +753,7 @@ static struct platform_driver img_spfi_driver = { .driver = { .name = "img-spfi", .pm = &img_spfi_pm_ops, - .of_match_table = of_match_ptr(img_spfi_of_match), + .of_match_table = img_spfi_of_match, }, .probe = img_spfi_probe, .remove_new = img_spfi_remove, diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c index fc75492e50ff..4189d4434e37 100644 --- a/drivers/spi/spi-meson-spicc.c +++ b/drivers/spi/spi-meson-spicc.c @@ -946,7 +946,7 @@ static struct platform_driver meson_spicc_driver = { .remove_new = meson_spicc_remove, .driver = { .name = "meson-spicc", - .of_match_table = of_match_ptr(meson_spicc_of_match), + .of_match_table = meson_spicc_of_match, }, }; diff --git a/drivers/spi/spi-meson-spifc.c b/drivers/spi/spi-meson-spifc.c index fd8b26dd4a79..0c39d59d0af8 100644 --- a/drivers/spi/spi-meson-spifc.c +++ b/drivers/spi/spi-meson-spifc.c @@ -432,7 +432,7 @@ static struct platform_driver meson_spifc_driver = { .remove_new = meson_spifc_remove, .driver = { .name = "meson-spifc", - .of_match_table = of_match_ptr(meson_spifc_dt_match), + .of_match_table = meson_spifc_dt_match, .pm = &meson_spifc_pm_ops, }, }; diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index eee9ff4bfa5b..a0029e8dc0be 100644 --- a/drivers/spi/spi-orion.c +++ b/drivers/spi/spi-orion.c @@ -843,7 +843,7 @@ static struct platform_driver orion_spi_driver = { .driver = { .name = DRIVER_NAME, .pm = &orion_spi_pm_ops, - .of_match_table = of_match_ptr(orion_spi_of_match_table), + .of_match_table = orion_spi_of_match_table, }, .probe = orion_spi_probe, .remove_new = orion_spi_remove, diff --git a/drivers/spi/spi-pic32-sqi.c b/drivers/spi/spi-pic32-sqi.c index 3f1e5b27776b..458c1a8d5719 100644 --- a/drivers/spi/spi-pic32-sqi.c +++ b/drivers/spi/spi-pic32-sqi.c @@ -679,7 +679,7 @@ MODULE_DEVICE_TABLE(of, pic32_sqi_of_ids); static struct platform_driver pic32_sqi_driver = { .driver = { .name = "sqi-pic32", - .of_match_table = of_match_ptr(pic32_sqi_of_ids), + .of_match_table = pic32_sqi_of_ids, }, .probe = pic32_sqi_probe, .remove_new = pic32_sqi_remove, diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c index 709edb70ad7d..e8bd32c1fc7c 100644 --- a/drivers/spi/spi-pic32.c +++ b/drivers/spi/spi-pic32.c @@ -856,7 +856,7 @@ MODULE_DEVICE_TABLE(of, pic32_spi_of_match); static struct platform_driver pic32_spi_driver = { .driver = { .name = "spi-pic32", - .of_match_table = of_match_ptr(pic32_spi_of_match), + .of_match_table = pic32_spi_of_match, }, .probe = pic32_spi_probe, .remove_new = pic32_spi_remove, diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index e1ecd96c7858..4dbb5127a9e5 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -1042,7 +1042,7 @@ static struct platform_driver rockchip_spi_driver = { .driver = { .name = DRIVER_NAME, .pm = &rockchip_spi_pm, - .of_match_table = of_match_ptr(rockchip_spi_dt_match), + .of_match_table = rockchip_spi_dt_match, }, .probe = rockchip_spi_probe, .remove_new = rockchip_spi_remove, diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index f726d8670428..dbb89f6cb3dd 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -1677,7 +1677,7 @@ static struct platform_driver s3c64xx_spi_driver = { .driver = { .name = "s3c64xx-spi", .pm = &s3c64xx_spi_pm, - .of_match_table = of_match_ptr(s3c64xx_spi_dt_match), + .of_match_table = s3c64xx_spi_dt_match, }, .probe = s3c64xx_spi_probe, .remove_new = s3c64xx_spi_remove, diff --git a/drivers/spi/spi-st-ssc4.c b/drivers/spi/spi-st-ssc4.c index e064025e2fd6..6d288497c500 100644 --- a/drivers/spi/spi-st-ssc4.c +++ b/drivers/spi/spi-st-ssc4.c @@ -446,7 +446,7 @@ static struct platform_driver spi_st_driver = { .driver = { .name = "spi-st", .pm = &spi_st_pm, - .of_match_table = of_match_ptr(stm_spi_match), + .of_match_table = stm_spi_match, }, .probe = spi_st_probe, .remove_new = spi_st_remove,