[PATCH] gpio: mvebu: use devm_clk_get_optional_enabled()

[PATCH] 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>
---
 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 329275d6518c..cdf7eb46ae5f 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 -ENODEV;
 
 	chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(*mvpwm));
 	if (IS_ERR(chip))
@@ -1194,10 +1194,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: [PATCH] gpio: mvebu: use devm_clk_get_optional_enabled()

[Bartosz Golaszewski]

On Wed, 8 Jul 2026 01:06:51 +0200, Rosen Penev <rosenp@gmail.com> said:
> 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>

Looks like someone beat you to it[1] but your patch seems to contain more.

> ---
>  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 329275d6518c..cdf7eb46ae5f 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 -ENODEV;

Why -ENODEV? Is the clock optional or not?

>
>  	chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(*mvpwm));
>  	if (IS_ERR(chip))
> @@ -1194,10 +1194,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
>
>

Bart

[1] https://lore.kernel.org/all/20260707160030.385137-1-dbgh9129@gmail.com/

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

[Andrew Lunn]

On Tue, Jul 07, 2026 at 04:06:51PM -0700, Rosen Penev wrote:
> The clock is obtained without doing any sort of cleanup on remove or
> anywhere else.

Given this is a SoC gpio controller, it is very unlikely it every gets
unloaded. There is no remove method, so is it even possible to remove
it?

How did you test this?

> -	if (IS_ERR(mvchip->clk))
> -		return PTR_ERR(mvchip->clk);
> +	if (!mvchip->clk)
> +		return -ENODEV;

You should not replace one error code with another.

This driver has been in use for over 14 years, without anybody having
problems with it. The SoCs themselves are EOL. They were used in NAS
boxes, which do tend to have a long life, but i doubt there are many
left still running a modern kernel.

Changes like this seems pointless, and just waste everybody's time.

	Andrew

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

[Rosen Penev]

On Wed, Jul 8, 2026 at 7:27 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Jul 07, 2026 at 04:06:51PM -0700, Rosen Penev wrote:
> > The clock is obtained without doing any sort of cleanup on remove or
> > anywhere else.
>
> Given this is a SoC gpio controller, it is very unlikely it every gets
> unloaded. There is no remove method, so is it even possible to remove
> it?
Not that I know of. My devices don't.
>
> How did you test this?
I have two mvebu devices currently (sold the third).
>
> > -     if (IS_ERR(mvchip->clk))
> > -             return PTR_ERR(mvchip->clk);
> > +     if (!mvchip->clk)
> > +             return -ENODEV;
>
> You should not replace one error code with another.
This section of the code is a result of the older API returning a
PTR_ERR when clock is missing. New one does not. Reading the code it
looks like it returns -ENOENT.
>
> This driver has been in use for over 14 years, without anybody having
> problems with it. The SoCs themselves are EOL. They were used in NAS
> boxes, which do tend to have a long life, but i doubt there are many
> left still running a modern kernel.
Still have one. It's unfortunate that ARM32 userspace is getting
deprecated by a bunch of tools.
>
> Changes like this seems pointless, and just waste everybody's time.
This was a sashiko tagged issue.
>
>         Andrew
>

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

[Bartosz Golaszewski]

On Wed, 8 Jul 2026 21:27:09 +0200, Rosen Penev <rosenp@gmail.com> said:
> On Wed, Jul 8, 2026 at 7:27 AM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Tue, Jul 07, 2026 at 04:06:51PM -0700, Rosen Penev wrote:
>> > The clock is obtained without doing any sort of cleanup on remove or
>> > anywhere else.
>>
>> Given this is a SoC gpio controller, it is very unlikely it every gets
>> unloaded. There is no remove method, so is it even possible to remove
>> it?
> Not that I know of. My devices don't.

You don't need the .remove() callback or even the module to be loadable to be
able to force-unbind a device.

>>
>> How did you test this?
> I have two mvebu devices currently (sold the third).
>>
>> > -     if (IS_ERR(mvchip->clk))
>> > -             return PTR_ERR(mvchip->clk);
>> > +     if (!mvchip->clk)
>> > +             return -ENODEV;
>>
>> You should not replace one error code with another.
> This section of the code is a result of the older API returning a
> PTR_ERR when clock is missing. New one does not. Reading the code it
> looks like it returns -ENOENT.
>>

So is the clock optional or is it not? The answer determines the correct
clk API to use.

>> This driver has been in use for over 14 years, without anybody having
>> problems with it. The SoCs themselves are EOL. They were used in NAS
>> boxes, which do tend to have a long life, but i doubt there are many
>> left still running a modern kernel.
> Still have one. It's unfortunate that ARM32 userspace is getting
> deprecated by a bunch of tools.
>>
>> Changes like this seems pointless, and just waste everybody's time.
> This was a sashiko tagged issue.

I'm not against refactoring of old drivers, otherwise we'll just let older
code bitrot. I'm fine with this, I just want my question above answered.

Bart

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

[Andrew Lunn]

> So is the clock optional or is it not? The answer determines the correct
> clk API to use.

The Orion5x family does not have the clock. As far as i remember, all
the other families using this driver do have the clock. And for this
line of Marvell devices, if the clock exists, you need it ticking,
otherwise the hardware just wedges if you access registers with the
clock disabled.

Also, as far as i remember, the Orion5x family does not have the PWM
extension to the GPIO controller. I don't remember the code well
enough, but it is possible the missing clock check is dual purpose, it
also detects an invalid DT when Orion5x has PWM properties when it
should not?

       Andrew

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

[Rosen Penev]

On Thu, Jul 9, 2026 at 6:28 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > So is the clock optional or is it not? The answer determines the correct
> > clk API to use.
>
> The Orion5x family does not have the clock. As far as i remember, all
> the other families using this driver do have the clock. And for this
> line of Marvell devices, if the clock exists, you need it ticking,
> otherwise the hardware just wedges if you access registers with the
> clock disabled.
I asked my LLM about this:

Compatible SoCs Has clocks in DTS?
marvell,orion-gpio Orion 5x, Dove, Kirkwood, Armada XP 98dx3236,
Armada 39x, Armada 375 Mixed - Kirkwood YES, Orion 5x/Dove NO,
98dx3236 NO, Armada 39x NO, Armada 375 NO
marvell,armada-370-g Armada 370, Armada 38x, Armada XP
(mv78230/60/460) Some have clocks, some don't

Not as simple as that looks like.
>
> Also, as far as i remember, the Orion5x family does not have the PWM
> extension to the GPIO controller. I don't remember the code well
> enough, but it is possible the missing clock check is dual purpose, it
> also detects an invalid DT when Orion5x has PWM properties when it
> should not?
>
>        Andrew