| Message ID | 20250520-pwm-axi-pwmgen-add-external-clock-v1-3-6cd63cc001c8@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: axi-pwmgen: add external clock | expand |
On Tue, May 20, 2025 at 04:00:46PM -0500, David Lechner wrote: > Add support for external clock to the AXI PWM generator driver. > > In most cases, there is a separate external clock that drives the PWM > output separate from the peripheral clock. This allows enabling both > clocks. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c > index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 > --- a/drivers/pwm/pwm-axi-pwmgen.c > +++ b/drivers/pwm/pwm-axi-pwmgen.c > @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) > ddata = pwmchip_get_drvdata(chip); > ddata->regmap = regmap; > > - clk = devm_clk_get_enabled(dev, NULL); > - if (IS_ERR(clk)) > - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); > + /* When clock-names is present, there is a separate ext clock. */ > + if (device_property_present(dev, "clock-names")) { > + struct clk *axi_clk; > + > + axi_clk = devm_clk_get_enabled(dev, "axi"); > + if (IS_ERR(axi_clk)) > + return dev_err_probe(dev, PTR_ERR(axi_clk), > + "failed to get axi clock\n"); > + > + clk = devm_clk_get_enabled(dev, "ext"); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), > + "failed to get ext clock\n"); > + } else { > + /* Otherwise, a single clock does everything. */ > + clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), > + "failed to get clock\n"); > + } Can you achieve the same effect with the (IMHO slightly nicer but hand-crafted) following patch: ddata = pwmchip_get_drvdata(chip); ddata->regmap = regmap; - clk = devm_clk_get_enabled(dev, NULL); - if (IS_ERR(clk)) - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); + axi_clk = devm_clk_get_enabled(dev, "axi"); + if (IS_ERR(axi_clk)) + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); + clk = devm_clk_get_enabled_optional(dev, "ext"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); + } ret = devm_clk_rate_exclusive_get(dev, clk); if (ret) with the only side effect that for machines with a single clk we get axi_clk == clk and it's enabled twice. Another upside is that clock-names = "axi"; clocks = <...>; still works. Best regards Uwe
On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote: > Add support for external clock to the AXI PWM generator driver. > > In most cases, there is a separate external clock that drives the PWM > output separate from the peripheral clock. This allows enabling both > clocks. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c > index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 > --- a/drivers/pwm/pwm-axi-pwmgen.c > +++ b/drivers/pwm/pwm-axi-pwmgen.c > @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) > ddata = pwmchip_get_drvdata(chip); > ddata->regmap = regmap; > > - clk = devm_clk_get_enabled(dev, NULL); > - if (IS_ERR(clk)) > - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); > + /* When clock-names is present, there is a separate ext clock. */ > + if (device_property_present(dev, "clock-names")) { No. List is ordered, you do not need such dance at all. > + struct clk *axi_clk; > + > + axi_clk = devm_clk_get_enabled(dev, "axi"); > + if (IS_ERR(axi_clk)) > + return dev_err_probe(dev, PTR_ERR(axi_clk), > + "failed to get axi clock\n"); > + > + clk = devm_clk_get_enabled(dev, "ext"); So that's messing the order, which confirms my question from the binding. No. List has a strict order, you cannot change it just because you want to add optional clock. Best regards, Krzysztof
On 5/21/25 4:22 AM, Uwe Kleine-König wrote: > On Tue, May 20, 2025 at 04:00:46PM -0500, David Lechner wrote: >> Add support for external clock to the AXI PWM generator driver. >> >> In most cases, there is a separate external clock that drives the PWM >> output separate from the peripheral clock. This allows enabling both >> clocks. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c >> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 >> --- a/drivers/pwm/pwm-axi-pwmgen.c >> +++ b/drivers/pwm/pwm-axi-pwmgen.c >> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) >> ddata = pwmchip_get_drvdata(chip); >> ddata->regmap = regmap; >> >> - clk = devm_clk_get_enabled(dev, NULL); >> - if (IS_ERR(clk)) >> - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); >> + /* When clock-names is present, there is a separate ext clock. */ >> + if (device_property_present(dev, "clock-names")) { >> + struct clk *axi_clk; >> + >> + axi_clk = devm_clk_get_enabled(dev, "axi"); >> + if (IS_ERR(axi_clk)) >> + return dev_err_probe(dev, PTR_ERR(axi_clk), >> + "failed to get axi clock\n"); >> + >> + clk = devm_clk_get_enabled(dev, "ext"); >> + if (IS_ERR(clk)) >> + return dev_err_probe(dev, PTR_ERR(clk), >> + "failed to get ext clock\n"); >> + } else { >> + /* Otherwise, a single clock does everything. */ >> + clk = devm_clk_get_enabled(dev, NULL); >> + if (IS_ERR(clk)) >> + return dev_err_probe(dev, PTR_ERR(clk), >> + "failed to get clock\n"); >> + } > > Can you achieve the same effect with the (IMHO slightly nicer but > hand-crafted) following patch: > > ddata = pwmchip_get_drvdata(chip); > ddata->regmap = regmap; > > - clk = devm_clk_get_enabled(dev, NULL); > - if (IS_ERR(clk)) > - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); > + axi_clk = devm_clk_get_enabled(dev, "axi"); > + if (IS_ERR(axi_clk)) > + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); > > + clk = devm_clk_get_enabled_optional(dev, "ext"); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); > + } The trouble with this is that it would not work with existing .dtbs that don't have clock-names set. I think it would need to be more like this: axi_clk = devm_clk_get_enabled(dev, NULL); if (IS_ERR(axi_clk)) return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); clk = devm_clk_get_enabled_optional(dev, "ext"); if (IS_ERR(clk)) return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); if (!clk) clk = axi_clk > > ret = devm_clk_rate_exclusive_get(dev, clk); > if (ret) > > with the only side effect that for machines with a single clk we get > axi_clk == clk and it's enabled twice. > > Another upside is that > > clock-names = "axi"; > clocks = <...>; > > still works. > > Best regards > Uwe
On 5/21/25 5:10 AM, Krzysztof Kozlowski wrote: > On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote: >> Add support for external clock to the AXI PWM generator driver. >> >> In most cases, there is a separate external clock that drives the PWM >> output separate from the peripheral clock. This allows enabling both >> clocks. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c >> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 >> --- a/drivers/pwm/pwm-axi-pwmgen.c >> +++ b/drivers/pwm/pwm-axi-pwmgen.c >> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) >> ddata = pwmchip_get_drvdata(chip); >> ddata->regmap = regmap; >> >> - clk = devm_clk_get_enabled(dev, NULL); >> - if (IS_ERR(clk)) >> - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); >> + /* When clock-names is present, there is a separate ext clock. */ >> + if (device_property_present(dev, "clock-names")) { > > No. List is ordered, you do not need such dance at all. I should have added more to the comment here. This is also needed for backwards compatibility where only what should be the "ext" clock is given as clocks = <&spi_clk>; and the AXI clock was missing. > >> + struct clk *axi_clk; >> + >> + axi_clk = devm_clk_get_enabled(dev, "axi"); >> + if (IS_ERR(axi_clk)) >> + return dev_err_probe(dev, PTR_ERR(axi_clk), >> + "failed to get axi clock\n"); >> + >> + clk = devm_clk_get_enabled(dev, "ext"); > > So that's messing the order, which confirms my question from the > binding. > > No. List has a strict order, you cannot change it just because you want > to add optional clock. > > Best regards, > Krzysztof >
On 21/05/2025 15:23, David Lechner wrote: > On 5/21/25 5:10 AM, Krzysztof Kozlowski wrote: >> On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote: >>> Add support for external clock to the AXI PWM generator driver. >>> >>> In most cases, there is a separate external clock that drives the PWM >>> output separate from the peripheral clock. This allows enabling both >>> clocks. >>> >>> Signed-off-by: David Lechner <dlechner@baylibre.com> >>> --- >>> drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- >>> 1 file changed, 20 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c >>> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 >>> --- a/drivers/pwm/pwm-axi-pwmgen.c >>> +++ b/drivers/pwm/pwm-axi-pwmgen.c >>> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) >>> ddata = pwmchip_get_drvdata(chip); >>> ddata->regmap = regmap; >>> >>> - clk = devm_clk_get_enabled(dev, NULL); >>> - if (IS_ERR(clk)) >>> - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); >>> + /* When clock-names is present, there is a separate ext clock. */ >>> + if (device_property_present(dev, "clock-names")) { >> >> No. List is ordered, you do not need such dance at all. > > I should have added more to the comment here. This is also needed for > backwards compatibility where only what should be the "ext" clock is > given as clocks = <&spi_clk>; and the AXI clock was missing. I do not see this needed at all. That's already handled by old code. You only need to take new optional, second clock - axi clk. You take it by index. Best regards, Krzysztof
On 5/21/25 8:30 AM, Krzysztof Kozlowski wrote: > On 21/05/2025 15:23, David Lechner wrote: >> On 5/21/25 5:10 AM, Krzysztof Kozlowski wrote: >>> On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote: >>>> Add support for external clock to the AXI PWM generator driver. >>>> >>>> In most cases, there is a separate external clock that drives the PWM >>>> output separate from the peripheral clock. This allows enabling both >>>> clocks. >>>> >>>> Signed-off-by: David Lechner <dlechner@baylibre.com> >>>> --- >>>> drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- >>>> 1 file changed, 20 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c >>>> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 >>>> --- a/drivers/pwm/pwm-axi-pwmgen.c >>>> +++ b/drivers/pwm/pwm-axi-pwmgen.c >>>> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) >>>> ddata = pwmchip_get_drvdata(chip); >>>> ddata->regmap = regmap; >>>> >>>> - clk = devm_clk_get_enabled(dev, NULL); >>>> - if (IS_ERR(clk)) >>>> - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); >>>> + /* When clock-names is present, there is a separate ext clock. */ >>>> + if (device_property_present(dev, "clock-names")) { >>> >>> No. List is ordered, you do not need such dance at all. >> >> I should have added more to the comment here. This is also needed for >> backwards compatibility where only what should be the "ext" clock is >> given as clocks = <&spi_clk>; and the AXI clock was missing. > > I do not see this needed at all. That's already handled by old code. You > only need to take new optional, second clock - axi clk. You take it by > index. Except that it is the "ext" clock that is supposed to be optional and the "axi" clock is supposed to be required. > > Best regards, > Krzysztof
Hello David, On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote: > On 5/21/25 4:22 AM, Uwe Kleine-König wrote: > > Can you achieve the same effect with the (IMHO slightly nicer but > > hand-crafted) following patch: > > > > ddata = pwmchip_get_drvdata(chip); > > ddata->regmap = regmap; > > > > - clk = devm_clk_get_enabled(dev, NULL); > > - if (IS_ERR(clk)) > > - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); > > + axi_clk = devm_clk_get_enabled(dev, "axi"); > > + if (IS_ERR(axi_clk)) > > + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); > > > > + clk = devm_clk_get_enabled_optional(dev, "ext"); > > + if (IS_ERR(clk)) > > + return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); > > + } > > The trouble with this is that it would not work with existing .dtbs > that don't have clock-names set. I think it would need to be more like this: > > > axi_clk = devm_clk_get_enabled(dev, NULL); > if (IS_ERR(axi_clk)) > return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); > > clk = devm_clk_get_enabled_optional(dev, "ext"); > if (IS_ERR(clk)) > return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); > > if (!clk) > clk = axi_clk > If there are no clock-names, the parameter is ignored. (I didn't test, only quickly checked the code.) So passing "axi" instead of NULL should work and yield a more robust solution. Best regards Uwe
On 5/21/25 8:54 AM, Uwe Kleine-König wrote: > Hello David, > > On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote: >> On 5/21/25 4:22 AM, Uwe Kleine-König wrote: >>> Can you achieve the same effect with the (IMHO slightly nicer but >>> hand-crafted) following patch: >>> >>> ddata = pwmchip_get_drvdata(chip); >>> ddata->regmap = regmap; >>> >>> - clk = devm_clk_get_enabled(dev, NULL); >>> - if (IS_ERR(clk)) >>> - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); >>> + axi_clk = devm_clk_get_enabled(dev, "axi"); >>> + if (IS_ERR(axi_clk)) >>> + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); >>> >>> + clk = devm_clk_get_enabled_optional(dev, "ext"); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); >>> + } >> >> The trouble with this is that it would not work with existing .dtbs >> that don't have clock-names set. I think it would need to be more like this: >> >> >> axi_clk = devm_clk_get_enabled(dev, NULL); >> if (IS_ERR(axi_clk)) >> return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n"); >> >> clk = devm_clk_get_enabled_optional(dev, "ext"); >> if (IS_ERR(clk)) >> return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n"); >> >> if (!clk) >> clk = axi_clk >> > > If there are no clock-names, the parameter is ignored. (I didn't test, > only quickly checked the code.) So passing "axi" instead of NULL should > work and yield a more robust solution. > > Best regards > Uwe I didn't know that. So with your suggestion, I guess we would get/enable the same clock twice. I guess that doesn't hurt anything. I will try it.
On Wed, 2025-05-21 at 15:54 +0200, Uwe Kleine-König wrote: > Hello David, > > On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote: > > On 5/21/25 4:22 AM, Uwe Kleine-König wrote: > > > Can you achieve the same effect with the (IMHO slightly nicer but > > > hand-crafted) following patch: > > > > > > ddata = pwmchip_get_drvdata(chip); > > > ddata->regmap = regmap; > > > > > > - clk = devm_clk_get_enabled(dev, NULL); > > > - if (IS_ERR(clk)) > > > - return dev_err_probe(dev, PTR_ERR(clk), "failed to get > > > clock\n"); > > > + axi_clk = devm_clk_get_enabled(dev, "axi"); > > > + if (IS_ERR(axi_clk)) > > > + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to > > > get axi clock\n"); > > > > > > + clk = devm_clk_get_enabled_optional(dev, "ext"); > > > + if (IS_ERR(clk)) > > > + return dev_err_probe(dev, PTR_ERR(clk), "failed to get > > > ext clock\n"); > > > + } > > > > The trouble with this is that it would not work with existing .dtbs > > that don't have clock-names set. I think it would need to be more like this: > > > > > > axi_clk = devm_clk_get_enabled(dev, NULL); > > if (IS_ERR(axi_clk)) > > return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get > > axi clock\n"); > > > > clk = devm_clk_get_enabled_optional(dev, "ext"); > > if (IS_ERR(clk)) > > return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext > > clock\n"); > > > > if (!clk) > > clk = axi_clk > > > > If there are no clock-names, the parameter is ignored. (I didn't test, > only quickly checked the code.) So passing "axi" instead of NULL should > work and yield a more robust solution. > > Are you sure? If there are no clock-names and you pass an id, you should get an error back: https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk.c#L5198 I know it's not exactly the same we're discussing but of_property_match_string() return -EINVAL if the property is not found which leads to an index < 0 and thus of_parse_phandle_with_args() also returns an error back. I think I'm not missing anything but it's always a possibility. - Nuno Sá
On Wed, 2025-05-21 at 09:12 -0500, David Lechner wrote: > On 5/21/25 8:54 AM, Uwe Kleine-König wrote: > > Hello David, > > > > On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote: > > > On 5/21/25 4:22 AM, Uwe Kleine-König wrote: > > > > Can you achieve the same effect with the (IMHO slightly nicer but > > > > hand-crafted) following patch: > > > > > > > > ddata = pwmchip_get_drvdata(chip); > > > > ddata->regmap = regmap; > > > > > > > > - clk = devm_clk_get_enabled(dev, NULL); > > > > - if (IS_ERR(clk)) > > > > - return dev_err_probe(dev, PTR_ERR(clk), "failed to get > > > > clock\n"); > > > > + axi_clk = devm_clk_get_enabled(dev, "axi"); > > > > + if (IS_ERR(axi_clk)) > > > > + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to > > > > get axi clock\n"); > > > > > > > > + clk = devm_clk_get_enabled_optional(dev, "ext"); > > > > + if (IS_ERR(clk)) > > > > + return dev_err_probe(dev, PTR_ERR(clk), "failed to get > > > > ext clock\n"); > > > > + } > > > > > > The trouble with this is that it would not work with existing .dtbs > > > that don't have clock-names set. I think it would need to be more like > > > this: > > > > > > > > > axi_clk = devm_clk_get_enabled(dev, NULL); > > > if (IS_ERR(axi_clk)) > > > return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to > > > get axi clock\n"); > > > > > > clk = devm_clk_get_enabled_optional(dev, "ext"); > > > if (IS_ERR(clk)) > > > return dev_err_probe(dev, PTR_ERR(clk), "failed to get > > > ext clock\n"); > > > > > > if (!clk) > > > clk = axi_clk > > > > > > > If there are no clock-names, the parameter is ignored. (I didn't test, > > only quickly checked the code.) So passing "axi" instead of NULL should > > work and yield a more robust solution. > > > > Best regards > > Uwe > > > I didn't know that. So with your suggestion, I guess we would get/enable > the same clock twice. I guess that doesn't hurt anything. I will try it. So, in the axi-dac we ended up doing this if you recall: https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/iio/dac/adi-axi-dac.c#L837 But I do not think you need that here. I that what you suggested (with the first call having id as NULL) should work fine. I'm starting to think that always having clock-names just makes things easier and more extendable (though the recommendation is to not have it in case there's only one clock). Not the first time I see this (or go through this kind of stuff). I had a similar situation in the axi-clkgen IP and IIRC, me and Conor just agreed in making clock-names required and to handle backward compatibility in the driver: https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk-axi-clkgen.c#L534 I'm actually now re-testing because I think I actually have a bug in there :) - Nuno Sá
On 5/21/25 9:22 AM, Nuno Sá wrote: > On Wed, 2025-05-21 at 15:54 +0200, Uwe Kleine-König wrote: >> Hello David, >> >> On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote: >>> On 5/21/25 4:22 AM, Uwe Kleine-König wrote: >>>> Can you achieve the same effect with the (IMHO slightly nicer but >>>> hand-crafted) following patch: >>>> >>>> ddata = pwmchip_get_drvdata(chip); >>>> ddata->regmap = regmap; >>>> >>>> - clk = devm_clk_get_enabled(dev, NULL); >>>> - if (IS_ERR(clk)) >>>> - return dev_err_probe(dev, PTR_ERR(clk), "failed to get >>>> clock\n"); >>>> + axi_clk = devm_clk_get_enabled(dev, "axi"); >>>> + if (IS_ERR(axi_clk)) >>>> + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to >>>> get axi clock\n"); >>>> >>>> + clk = devm_clk_get_enabled_optional(dev, "ext"); >>>> + if (IS_ERR(clk)) >>>> + return dev_err_probe(dev, PTR_ERR(clk), "failed to get >>>> ext clock\n"); >>>> + } >>> >>> The trouble with this is that it would not work with existing .dtbs >>> that don't have clock-names set. I think it would need to be more like this: >>> >>> >>> axi_clk = devm_clk_get_enabled(dev, NULL); >>> if (IS_ERR(axi_clk)) >>> return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get >>> axi clock\n"); >>> >>> clk = devm_clk_get_enabled_optional(dev, "ext"); >>> if (IS_ERR(clk)) >>> return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext >>> clock\n"); >>> >>> if (!clk) >>> clk = axi_clk >>> >> >> If there are no clock-names, the parameter is ignored. (I didn't test, >> only quickly checked the code.) So passing "axi" instead of NULL should >> work and yield a more robust solution. >> >> > > Are you sure? If there are no clock-names and you pass an id, you should get an > error back: > > https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk.c#L5198 > > > I know it's not exactly the same we're discussing but of_property_match_string() > return -EINVAL if the property is not found which leads to an index < 0 and thus > of_parse_phandle_with_args() also returns an error back. > > I think I'm not missing anything but it's always a possibility. > > - Nuno Sá Testing agrees: Given: clocks = <&some_clock>; /delete-property/clock-names; And: axi_clk = devm_clk_get_enabled(dev, "axi"); We get: [ 1.190040] axi-pwmgen 44b00000.pwm: error -ENOENT: failed to get axi clock
On Wed, 2025-05-21 at 10:05 -0500, David Lechner wrote: > On 5/21/25 9:22 AM, Nuno Sá wrote: > > On Wed, 2025-05-21 at 15:54 +0200, Uwe Kleine-König wrote: > > > Hello David, > > > > > > On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote: > > > > On 5/21/25 4:22 AM, Uwe Kleine-König wrote: > > > > > Can you achieve the same effect with the (IMHO slightly nicer but > > > > > hand-crafted) following patch: > > > > > > > > > > ddata = pwmchip_get_drvdata(chip); > > > > > ddata->regmap = regmap; > > > > > > > > > > - clk = devm_clk_get_enabled(dev, NULL); > > > > > - if (IS_ERR(clk)) > > > > > - return dev_err_probe(dev, PTR_ERR(clk), "failed to > > > > > get > > > > > clock\n"); > > > > > + axi_clk = devm_clk_get_enabled(dev, "axi"); > > > > > + if (IS_ERR(axi_clk)) > > > > > + return dev_err_probe(dev, PTR_ERR(axi_clk), "failed > > > > > to > > > > > get axi clock\n"); > > > > > > > > > > + clk = devm_clk_get_enabled_optional(dev, "ext"); > > > > > + if (IS_ERR(clk)) > > > > > + return dev_err_probe(dev, PTR_ERR(clk), "failed to > > > > > get > > > > > ext clock\n"); > > > > > + } > > > > > > > > The trouble with this is that it would not work with existing .dtbs > > > > that don't have clock-names set. I think it would need to be more like > > > > this: > > > > > > > > > > > > axi_clk = devm_clk_get_enabled(dev, NULL); > > > > if (IS_ERR(axi_clk)) > > > > return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to > > > > get > > > > axi clock\n"); > > > > > > > > clk = devm_clk_get_enabled_optional(dev, "ext"); > > > > if (IS_ERR(clk)) > > > > return dev_err_probe(dev, PTR_ERR(clk), "failed to get > > > > ext > > > > clock\n"); > > > > > > > > if (!clk) > > > > clk = axi_clk > > > > > > > > > > If there are no clock-names, the parameter is ignored. (I didn't test, > > > only quickly checked the code.) So passing "axi" instead of NULL should > > > work and yield a more robust solution. > > > > > > > > > > Are you sure? If there are no clock-names and you pass an id, you should get > > an > > error back: > > > > https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk.c#L5198 > > > > > > I know it's not exactly the same we're discussing but > > of_property_match_string() > > return -EINVAL if the property is not found which leads to an index < 0 and > > thus > > of_parse_phandle_with_args() also returns an error back. > > > > I think I'm not missing anything but it's always a possibility. > > > > - Nuno Sá > > Testing agrees: > > Given: > > clocks = <&some_clock>; > /delete-property/clock-names; > > And: > > axi_clk = devm_clk_get_enabled(dev, "axi"); > > We get: > > [ 1.190040] axi-pwmgen 44b00000.pwm: error -ENOENT: failed to get axi clock Hmm, so it seems I have no bug (in the other link I pasted in the other reply). Looking at the code I would expect -EINVAL to be returned... I guess it's because we still try __clk_get_sys(). - Nuno Sá
diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644 --- a/drivers/pwm/pwm-axi-pwmgen.c +++ b/drivers/pwm/pwm-axi-pwmgen.c @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev) ddata = pwmchip_get_drvdata(chip); ddata->regmap = regmap; - clk = devm_clk_get_enabled(dev, NULL); - if (IS_ERR(clk)) - return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); + /* When clock-names is present, there is a separate ext clock. */ + if (device_property_present(dev, "clock-names")) { + struct clk *axi_clk; + + axi_clk = devm_clk_get_enabled(dev, "axi"); + if (IS_ERR(axi_clk)) + return dev_err_probe(dev, PTR_ERR(axi_clk), + "failed to get axi clock\n"); + + clk = devm_clk_get_enabled(dev, "ext"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "failed to get ext clock\n"); + } else { + /* Otherwise, a single clock does everything. */ + clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "failed to get clock\n"); + } ret = devm_clk_rate_exclusive_get(dev, clk); if (ret)
Add support for external clock to the AXI PWM generator driver. In most cases, there is a separate external clock that drives the PWM output separate from the peripheral clock. This allows enabling both clocks. Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)