| Message ID | 20230420121615.967487-1-d-gole@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 25f0617109496e1aff49594fbae5644286447a0f |
| Headers | show |
| Series | spi: bcm63xx: remove PM_SLEEP based conditional compilation | expand |
On Thu, 20 Apr 2023 17:46:15 +0530, Dhruva Gole wrote: > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > it may introduce build issues with certain configs where it maybe disabled > This is because if above config is not enabled the suspend-resume > functions are never part of the code but the bcm63xx_spi_pm_ops struct > still inits them to non-existent suspend-resume functions. > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: bcm63xx: remove PM_SLEEP based conditional compilation commit: 25f0617109496e1aff49594fbae5644286447a0f All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On 4/20/23 05:16, Dhruva Gole wrote: > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > it may introduce build issues with certain configs where it maybe disabled > This is because if above config is not enabled the suspend-resume > functions are never part of the code but the bcm63xx_spi_pm_ops struct > still inits them to non-existent suspend-resume functions. > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > --- > drivers/spi/spi-bcm63xx.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c > index 96633a0051b1..99395932074c 100644 > --- a/drivers/spi/spi-bcm63xx.c > +++ b/drivers/spi/spi-bcm63xx.c > @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) > clk_disable_unprepare(bs->clk); > } > > -#ifdef CONFIG_PM_SLEEP > static int bcm63xx_spi_suspend(struct device *dev) Don't we need a __maybe_unused here?
On Fri, 21 Apr 2023 at 19:17, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 4/20/23 05:16, Dhruva Gole wrote: > > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > > it may introduce build issues with certain configs where it maybe disabled > > This is because if above config is not enabled the suspend-resume > > functions are never part of the code but the bcm63xx_spi_pm_ops struct > > still inits them to non-existent suspend-resume functions. > > > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > --- > > drivers/spi/spi-bcm63xx.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c > > index 96633a0051b1..99395932074c 100644 > > --- a/drivers/spi/spi-bcm63xx.c > > +++ b/drivers/spi/spi-bcm63xx.c > > @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) > > clk_disable_unprepare(bs->clk); > > } > > > > -#ifdef CONFIG_PM_SLEEP > > static int bcm63xx_spi_suspend(struct device *dev) > > Don't we need a __maybe_unused here? Actually the premise of this patch is wrong, and should rather be reverted. The bcm63xx_spi_pm_ops struct is initialized with SET_SYSTEM_SLEEP_PM_OPS(), which is defined as #ifdef CONFIG_PM_SLEEP #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) #else #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) #endif so for !CONFIG_PM_SLEEP it won't initialize the struct at all (or reference non-existing functions), and therefore there will be no build issues. Regards, Jonas
Hi Jonas, On 24/04/23 13:50, Jonas Gorski wrote: > On Fri, 21 Apr 2023 at 19:17, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> On 4/20/23 05:16, Dhruva Gole wrote: >>> Get rid of conditional compilation based on CONFIG_PM_SLEEP because >>> it may introduce build issues with certain configs where it maybe disabled >>> This is because if above config is not enabled the suspend-resume >>> functions are never part of the code but the bcm63xx_spi_pm_ops struct >>> still inits them to non-existent suspend-resume functions. >>> >>> Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") >>> >>> Signed-off-by: Dhruva Gole <d-gole@ti.com> >>> --- >>> drivers/spi/spi-bcm63xx.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c >>> index 96633a0051b1..99395932074c 100644 >>> --- a/drivers/spi/spi-bcm63xx.c >>> +++ b/drivers/spi/spi-bcm63xx.c >>> @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) >>> clk_disable_unprepare(bs->clk); >>> } >>> >>> -#ifdef CONFIG_PM_SLEEP >>> static int bcm63xx_spi_suspend(struct device *dev) >> >> Don't we need a __maybe_unused here? > > Actually the premise of this patch is wrong, and should rather be reverted. > > The bcm63xx_spi_pm_ops struct is initialized with > SET_SYSTEM_SLEEP_PM_OPS(), which is defined as > > #ifdef CONFIG_PM_SLEEP > #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) > #else > #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) > #endif > Thanks for pointing this out, I must have missed this. Anyway, I have sent another patch to migrate to using DEFINE_SIMPLE_DEV_PM_OPS as per Mark's suggestion [0]. There I think it would be necessary to remove the CONFIG_PM_SLEEP checks in the driver. So no need to revert this patch. > so for !CONFIG_PM_SLEEP it won't initialize the struct at all (or > reference non-existing functions), and therefore there will be no > build issues. > > Regards, > Jonas [0] https://lore.kernel.org/all/e65683c1-9f1b-4cfb-8e14-087ef7d69595@sirena.org.uk/
On Thu, Apr 20, 2023 at 05:46:15PM +0530, Dhruva Gole wrote: > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > it may introduce build issues with certain configs where it maybe disabled > This is because if above config is not enabled the suspend-resume > functions are never part of the code but the bcm63xx_spi_pm_ops struct > still inits them to non-existent suspend-resume functions. > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > Signed-off-by: Dhruva Gole <d-gole@ti.com> This patch results in: drivers/spi/spi-bcm63xx.c:632:12: error: 'bcm63xx_spi_resume' defined but not used [-Werror=unused-function] 632 | static int bcm63xx_spi_resume(struct device *dev) | ^~~~~~~~~~~~~~~~~~ drivers/spi/spi-bcm63xx.c:620:12: error: 'bcm63xx_spi_suspend' defined but not used [-Werror=unused-function] 620 | static int bcm63xx_spi_suspend(struct device *dev) on architectures with no PM support (alpha, csky, m68k, openrisc, parisc, riscv, s390). Guenter
Hi, On 4/25/2023 8:38 PM, Guenter Roeck wrote: > On Thu, Apr 20, 2023 at 05:46:15PM +0530, Dhruva Gole wrote: >> Get rid of conditional compilation based on CONFIG_PM_SLEEP because >> it may introduce build issues with certain configs where it maybe disabled >> This is because if above config is not enabled the suspend-resume >> functions are never part of the code but the bcm63xx_spi_pm_ops struct >> still inits them to non-existent suspend-resume functions. >> >> Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") >> >> Signed-off-by: Dhruva Gole <d-gole@ti.com> > This patch results in: > > drivers/spi/spi-bcm63xx.c:632:12: error: 'bcm63xx_spi_resume' defined but not used [-Werror=unused-function] > 632 | static int bcm63xx_spi_resume(struct device *dev) > | ^~~~~~~~~~~~~~~~~~ > drivers/spi/spi-bcm63xx.c:620:12: error: 'bcm63xx_spi_suspend' defined but not used [-Werror=unused-function] > 620 | static int bcm63xx_spi_suspend(struct device *dev) > > on architectures with no PM support (alpha, csky, m68k, openrisc, parisc, > riscv, s390). Thanks for pointing this out. I could send a patch to address this as pointed here [0] by adding a __maybe_unused. However, do you think my other patch [1] could address this issue without the need for above? I think it would because DEFINE_SIMPLE_DEV_PM_OPS doesn't seem to be under any conditional CONFIG_PM. However, I may have missed something, please do let me know what's the best way to fix. [0] https://lore.kernel.org/all/24ec3728-9720-ae6a-9ff5-3e2e13a96f76@gmail.com/ [1] https://lore.kernel.org/all/20230424102546.1604484-1-d-gole@ti.com/ > > Guenter
On 4/25/23 10:18, Gole, Dhruva wrote: > Hi, > > On 4/25/2023 8:38 PM, Guenter Roeck wrote: >> On Thu, Apr 20, 2023 at 05:46:15PM +0530, Dhruva Gole wrote: >>> Get rid of conditional compilation based on CONFIG_PM_SLEEP because >>> it may introduce build issues with certain configs where it maybe disabled >>> This is because if above config is not enabled the suspend-resume >>> functions are never part of the code but the bcm63xx_spi_pm_ops struct >>> still inits them to non-existent suspend-resume functions. >>> >>> Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") >>> >>> Signed-off-by: Dhruva Gole <d-gole@ti.com> >> This patch results in: >> >> drivers/spi/spi-bcm63xx.c:632:12: error: 'bcm63xx_spi_resume' defined but not used [-Werror=unused-function] >> 632 | static int bcm63xx_spi_resume(struct device *dev) >> | ^~~~~~~~~~~~~~~~~~ >> drivers/spi/spi-bcm63xx.c:620:12: error: 'bcm63xx_spi_suspend' defined but not used [-Werror=unused-function] >> 620 | static int bcm63xx_spi_suspend(struct device *dev) >> >> on architectures with no PM support (alpha, csky, m68k, openrisc, parisc, >> riscv, s390). > > Thanks for pointing this out. > > I could send a patch to address this as pointed here [0] > > by adding a __maybe_unused. > > However, do you think my other patch [1] could address this issue without the need for above? > Personally I would go for [0] as the least invasive solution, but I really have no idea, sorry. I just hope that your (broken) patch doesn't make it as-is into the upstream kernel. Guenter > I think it would because DEFINE_SIMPLE_DEV_PM_OPS doesn't seem to be under any conditional CONFIG_PM. > > However, I may have missed something, please do let me know what's the best way to fix. > > [0] https://lore.kernel.org/all/24ec3728-9720-ae6a-9ff5-3e2e13a96f76@gmail.com/ > > [1] https://lore.kernel.org/all/20230424102546.1604484-1-d-gole@ti.com/ > >> >> Guenter >
On Tue, Apr 25, 2023 at 10:37:24AM -0700, Guenter Roeck wrote: > Personally I would go for [0] as the least invasive solution, but I really > have no idea, sorry. I just hope that your (broken) patch doesn't make it > as-is into the upstream kernel. I've applied the SIMPLE_DEV_PM_OPS patch which seems to fix the issue for riscv.
diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c index 96633a0051b1..99395932074c 100644 --- a/drivers/spi/spi-bcm63xx.c +++ b/drivers/spi/spi-bcm63xx.c @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) clk_disable_unprepare(bs->clk); } -#ifdef CONFIG_PM_SLEEP static int bcm63xx_spi_suspend(struct device *dev) { struct spi_master *master = dev_get_drvdata(dev); @@ -644,7 +643,6 @@ static int bcm63xx_spi_resume(struct device *dev) return 0; } -#endif static const struct dev_pm_ops bcm63xx_spi_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(bcm63xx_spi_suspend, bcm63xx_spi_resume)
Get rid of conditional compilation based on CONFIG_PM_SLEEP because it may introduce build issues with certain configs where it maybe disabled This is because if above config is not enabled the suspend-resume functions are never part of the code but the bcm63xx_spi_pm_ops struct still inits them to non-existent suspend-resume functions. Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") Signed-off-by: Dhruva Gole <d-gole@ti.com> --- drivers/spi/spi-bcm63xx.c | 2 -- 1 file changed, 2 deletions(-)