| Message ID | 179dc1ce85702a8b64b43c0e0df656b0c5e3ce30.1701248996.git.sean@mess.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/4] pwm: rename pwm_apply_state() to pwm_apply_might_sleep() | expand |
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 <sean@mess.org> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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 <sean@mess.org> > --- > 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 <sean@mess.org> > > --- > > 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 <sean@mess.org> > > > --- > > > 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
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 <sean@mess.org> > > > > --- > > > > 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 <sean@mess.org> > > > > --- > > > > 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
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;
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 <sean@mess.org> --- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)