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 <[email protected]>
---
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)
{
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)
--
2.25.1
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 <[email protected]>
> ---
> 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?
--
Florian
On Fri, 21 Apr 2023 at 19:17, Florian Fainelli <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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/[email protected]/
--
Thanks and Regards,
Dhruva Gole
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 <[email protected]>
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 <[email protected]>
> 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/[email protected]/
[1] https://lore.kernel.org/all/[email protected]/
>
> Guenter
--
Regards,
Dhruva Gole <[email protected]>
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 <[email protected]>
>> 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/[email protected]/
>
> [1] https://lore.kernel.org/all/[email protected]/
>
>>
>> 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.