[PATCHv2] gpio: mvebu: use devm_clk_get_optional_enabled()

[PATCHv2] gpio: mvebu: use devm_clk_get_optional_enabled()

[Rosen Penev]

The clock is obtained without doing any sort of cleanup on remove or
anywhere else. Use the proper function to handle this. When it fails
with -EPROBE_DEFER for example, return so that it can be handled. When
the clock is not found, it's NULL and not a PTR_ERR. Handle that as
well.

Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: return -ENOENT as before.
 drivers/gpio/gpio-mvebu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 8d3acadb0d68..a863b0bf46ff 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -833,8 +833,8 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 		offset = 0;
 	}
 
-	if (IS_ERR(mvchip->clk))
-		return PTR_ERR(mvchip->clk);
+	if (!mvchip->clk)
+		return -ENOENT;
 
 	chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(*mvpwm));
 	if (IS_ERR(chip))
@@ -1182,10 +1182,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 		return id;
 	}
 
-	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
 	/* Not all SoCs require a clock.*/
-	if (!IS_ERR(mvchip->clk))
-		clk_prepare_enable(mvchip->clk);
+	mvchip->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	if (IS_ERR(mvchip->clk))
+		return PTR_ERR(mvchip->clk);
 
 	mvchip->soc_variant = soc_variant;
 	mvchip->chip.label = dev_name(&pdev->dev);
-- 
2.55.0

Re: [PATCHv2] gpio: mvebu: use devm_clk_get_optional_enabled()

[Andrew Lunn]

On Wed, Jul 08, 2026 at 03:48:22PM -0700, Rosen Penev wrote:
> The clock is obtained without doing any sort of cleanup on remove or
> anywhere else.

Didn't you say you could not unload this driver? Then why is cleanup
needed?

> Use the proper function to handle this. When it fails
> with -EPROBE_DEFER for example, return so that it can be handled.

Why would it fail with -EPROBE_DEFER? The only resource this driver
needs is optional clocks. If the SoC clock driver is missing, the SoC
is going to die very soon anyway, it is such a core part of the SoC.
If you access any register without the clock enabled, the SoC just
hard wedges. I've had to debug that situation.

> When
> the clock is not found, it's NULL and not a PTR_ERR. Handle that as
> well.

/**
 * devm_clk_get - lookup and obtain a managed reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Context: May sleep.
 *
 * Return: a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.

The documentation disagrees with you.

Please take a step back. What is really broken here? What reports are
there from users? What is bothering people?

	Andrew

Re: [PATCHv2] gpio: mvebu: use devm_clk_get_optional_enabled()

[Rosen Penev]

On Wed, Jul 8, 2026 at 4:38 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jul 08, 2026 at 03:48:22PM -0700, Rosen Penev wrote:
> > The clock is obtained without doing any sort of cleanup on remove or
> > anywhere else.
>
> Didn't you say you could not unload this driver? Then why is cleanup
> needed?
devm is used here. If probe fails, the driver relies on devm to handle
cleanup in order.
>
> > Use the proper function to handle this. When it fails
> > with -EPROBE_DEFER for example, return so that it can be handled.
>
> Why would it fail with -EPROBE_DEFER? The only resource this driver
> needs is optional clocks. If the SoC clock driver is missing, the SoC
> is going to die very soon anyway, it is such a core part of the SoC.
> If you access any register without the clock enabled, the SoC just
> hard wedges. I've had to debug that situation.
That's the only realistic error it can throw in probe, generally
speaking. It won't throw when the clock is missing.
>
> > When
> > the clock is not found, it's NULL and not a PTR_ERR. Handle that as
> > well.
>
> /**
>  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>  * @dev: device for clock "consumer"
>  * @id: clock consumer ID
>  *
>  * Context: May sleep.
>  *
>  * Return: a struct clk corresponding to the clock producer, or
>  * valid IS_ERR() condition containing errno.
>
> The documentation disagrees with you.
We're dealing with the _optional variant. The code in
drivers/clk/clk.c ultimately throws -ENOENT when a clock is missing.
mvebu_pwm_probe requires that optional clock, hence why an early
return is done.
>
> Please take a step back. What is really broken here? What reports are
> there from users? What is bothering people?
The plan is to fix sashiko complaints, followed by sending a dual PWM
patch so that sashiko doesn't complain as loud.
>
>         Andrew