clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
rate changes using clk_rate_exclusive_get().
Signed-off-by: Sean Young <[email protected]>
---
drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 9777babd5b95..52748194a3da 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -28,6 +28,7 @@ struct bcm2835_pwm {
struct device *dev;
void __iomem *base;
struct clk *clk;
+ unsigned long rate;
};
static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
@@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
- unsigned long rate = clk_get_rate(pc->clk);
unsigned long long period_cycles;
u64 max_period;
u32 val;
- if (!rate) {
- dev_err(pc->dev, "failed to get clock rate\n");
- return -EINVAL;
- }
-
/*
* period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
* must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
@@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
* <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1
*/
- max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
+ max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;
if (state->period > max_period)
return -EINVAL;
/* set period */
- period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC);
+ period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * pc->rate, NSEC_PER_SEC);
/* don't accept a period that is too small */
if (period_cycles < PERIOD_MIN)
@@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
/* set duty cycle */
- val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
+ val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, NSEC_PER_SEC);
writel(val, pc->base + DUTY(pwm->hwpwm));
/* set polarity */
@@ -151,14 +146,29 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk),
"clock not found\n");
+ ret = clk_rate_exclusive_get(pc->clk);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "fail to get exclusive rate\n");
+
+ pc->rate = clk_get_rate(pc->clk);
+ if (!pc->rate) {
+ clk_rate_exclusive_put(pc->clk);
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "failed to get clock rate\n");
+ }
+
pc->chip.dev = &pdev->dev;
pc->chip.ops = &bcm2835_pwm_ops;
+ pc->chip.atomic = true;
pc->chip.npwm = 2;
ret = devm_pwmchip_add(&pdev->dev, &pc->chip);
- if (ret < 0)
+ if (ret < 0) {
+ clk_rate_exclusive_put(pc->clk);
return dev_err_probe(&pdev->dev, ret,
"failed to add pwmchip\n");
+ }
return 0;
}
@@ -167,6 +177,7 @@ static int bcm2835_pwm_suspend(struct device *dev)
{
struct bcm2835_pwm *pc = dev_get_drvdata(dev);
+ clk_rate_exclusive_put(pc->clk);
clk_disable_unprepare(pc->clk);
return 0;
--
2.43.0
On 11/29/23 01:13, Sean Young wrote:
> clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
> rate changes using clk_rate_exclusive_get().
>
> Signed-off-by: Sean Young <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Wed, Nov 29, 2023 at 09:13:36AM +0000, Sean Young wrote:
> clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
> rate changes using clk_rate_exclusive_get().
>
> Signed-off-by: Sean Young <[email protected]>
> ---
> drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
s/pwm/PWM/ in the subject. Although, I guess you could just drop the
"PWM" altogether because the subject prefix implies that this is for
PWM. Also, please capitalize after the subject prefix.
Thierry
On Fri, Dec 08, 2023 at 05:22:52PM +0100, Thierry Reding wrote:
> On Wed, Nov 29, 2023 at 09:13:36AM +0000, Sean Young wrote:
> > clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
> > rate changes using clk_rate_exclusive_get().
> >
> > Signed-off-by: Sean Young <[email protected]>
> > ---
> > drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++----------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
>
> s/pwm/PWM/ in the subject. Although, I guess you could just drop the
> "PWM" altogether because the subject prefix implies that this is for
> PWM.
$ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c
1197 pwm
1 PWM
...
The vast majority of the commits use pwm: as a prefix, only one uses PWM:.
In fact if you look across the tree almost everywhere lower case is used
for the prefix.
I'm just trying to follow convention.
Having said that, I think the prefix is totally redundant, it is clear from
the commit files what they are affecting. I am not sure what it really adds.
> Also, please capitalize after the subject prefix.
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [A-Z]' | wc -l
217
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [a-z]' | wc -l
1069
Although not as clear, convention seems to be lower case for commits. The
first line of a commit is not really a sentence, there is no trailing
period.
I am happy to oblige, just wanted to point this out. Sorry if this starts
a bikeshed discussion.
Thanks,
Sean
On Fri, Dec 08, 2023 at 05:01:26PM +0000, Sean Young wrote:
> On Fri, Dec 08, 2023 at 05:22:52PM +0100, Thierry Reding wrote:
> > On Wed, Nov 29, 2023 at 09:13:36AM +0000, Sean Young wrote:
> > > clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
> > > rate changes using clk_rate_exclusive_get().
> > >
> > > Signed-off-by: Sean Young <[email protected]>
> > > ---
> > > drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++----------
> > > 1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > s/pwm/PWM/ in the subject. Although, I guess you could just drop the
> > "PWM" altogether because the subject prefix implies that this is for
> > PWM.
>
> $ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c
> 1197 pwm
> 1 PWM
> ...
>
> The vast majority of the commits use pwm: as a prefix, only one uses PWM:.
> In fact if you look across the tree almost everywhere lower case is used
> for the prefix.
Thierry doesn't want you to change the subject prefix, but only the
second "pwm" to make it read:
pwm: bcm2835: allow PWM driver to be used in atomic context
While I understand Thierry here, I'm fine with a lowercase pwm here,
too. In my book a PWM in all uppercase is the type of hardware and pwm
in all lowercase is the framework's name. If you use "PWM driver" or
"pwm driver" then doesn't matter much.
>
> I'm just trying to follow convention.
>
> Having said that, I think the prefix is totally redundant, it is clear from
> the commit files what they are affecting. I am not sure what it really adds.
>
> > Also, please capitalize after the subject prefix.
>
> $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [A-Z]' | wc -l
> 217
> $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [a-z]' | wc -l
> 1069
Your matching things like:
pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
with the second command. These are perfectly fine as pwm-tiehrpwm is the
driver name and so shouldn't be capitalized. With a bit more care here,
we get:
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [a-z][^:]*$' | wc -l
114
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [A-Z][^:]*$' | wc -l
1167
And the newest of the 114 with a small letter is from 2013.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Dec 08, 2023 at 06:20:40PM +0100, Uwe Kleine-K?nig wrote:
> On Fri, Dec 08, 2023 at 05:01:26PM +0000, Sean Young wrote:
> > On Fri, Dec 08, 2023 at 05:22:52PM +0100, Thierry Reding wrote:
> > > On Wed, Nov 29, 2023 at 09:13:36AM +0000, Sean Young wrote:
> > > > clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
> > > > rate changes using clk_rate_exclusive_get().
> > > >
> > > > Signed-off-by: Sean Young <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++----------
> > > > 1 file changed, 21 insertions(+), 10 deletions(-)
> > >
> > > s/pwm/PWM/ in the subject. Although, I guess you could just drop the
> > > "PWM" altogether because the subject prefix implies that this is for
> > > PWM.
> >
> > $ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c
> > 1197 pwm
> > 1 PWM
> > ...
> >
> > The vast majority of the commits use pwm: as a prefix, only one uses PWM:.
> > In fact if you look across the tree almost everywhere lower case is used
> > for the prefix.
>
> Thierry doesn't want you to change the subject prefix, but only the
> second "pwm" to make it read:
>
> pwm: bcm2835: allow PWM driver to be used in atomic context
Ah of course, my bad.
> While I understand Thierry here, I'm fine with a lowercase pwm here,
> too. In my book a PWM in all uppercase is the type of hardware and pwm
> in all lowercase is the framework's name. If you use "PWM driver" or
> "pwm driver" then doesn't matter much.
>
>
> >
> > I'm just trying to follow convention.
> >
> > Having said that, I think the prefix is totally redundant, it is clear from
> > the commit files what they are affecting. I am not sure what it really adds.
> >
> > > Also, please capitalize after the subject prefix.
> >
> > $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [A-Z]' | wc -l
> > 217
> > $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [a-z]' | wc -l
> > 1069
>
> Your matching things like:
>
> pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
>
> with the second command. These are perfectly fine as pwm-tiehrpwm is the
> driver name and so shouldn't be capitalized. With a bit more care here,
> we get:
>
> $ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [a-z][^:]*$' | wc -l
> 114
> $ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [A-Z][^:]*$' | wc -l
> 1167
>
> And the newest of the 114 with a small letter is from 2013.
Again, I stand corrected.
Thanks for pointing it out, I will fix in the next version.
Sean
On Fri, Dec 08, 2023 at 06:20:40PM +0100, Uwe Kleine-König wrote:
> On Fri, Dec 08, 2023 at 05:01:26PM +0000, Sean Young wrote:
> > On Fri, Dec 08, 2023 at 05:22:52PM +0100, Thierry Reding wrote:
> > > On Wed, Nov 29, 2023 at 09:13:36AM +0000, Sean Young wrote:
> > > > clk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent
> > > > rate changes using clk_rate_exclusive_get().
> > > >
> > > > Signed-off-by: Sean Young <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++----------
> > > > 1 file changed, 21 insertions(+), 10 deletions(-)
> > >
> > > s/pwm/PWM/ in the subject. Although, I guess you could just drop the
> > > "PWM" altogether because the subject prefix implies that this is for
> > > PWM.
> >
> > $ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c
> > 1197 pwm
> > 1 PWM
> > ...
> >
> > The vast majority of the commits use pwm: as a prefix, only one uses PWM:.
> > In fact if you look across the tree almost everywhere lower case is used
> > for the prefix.
>
> Thierry doesn't want you to change the subject prefix, but only the
> second "pwm" to make it read:
>
> pwm: bcm2835: allow PWM driver to be used in atomic context
>
> While I understand Thierry here, I'm fine with a lowercase pwm here,
> too. In my book a PWM in all uppercase is the type of hardware and pwm
> in all lowercase is the framework's name. If you use "PWM driver" or
> "pwm driver" then doesn't matter much.
I'm not fine with a lowercase "pwm" in what is clearly text. Text should
be grammatically correct and PWM being an abbreviation it should be all
caps. The framework name is also PWM, not pwm. We use the lowercase pwm
as prefix because it represents the directory where the subsystem lives
and we usually don't use capitalization in file and directory names.
Thierry