[PATCH 0/3] pwm: bcm2835: improve clock handling

[PATCH 0/3] pwm: bcm2835: improve clock handling

[Stefan Wahren]

With the upcoming PWM clock support the pwm-bcm2835 should take
more care about the clock handling. But this patch series doesn't
depend on these clock driver changes.

Stefan Wahren (3):
  pwm: bcm2835: calculate scaler in pwm_config
  pwm: bcm2835: prevent division by zero
  pwm: bcm2835: fix email address specifiction

 drivers/pwm/pwm-bcm2835.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] pwm: bcm2835: calculate scaler in pwm_config

[Stefan Wahren]

Currently pwm-bcm2835 assumes a fixed clock rate and stores the
resulting scaler in the driver structure. But with the upcoming
PWM clock support for clk-bcm2835 the rate could change.
So calculate the scaler in pwm_config.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/pwm/pwm-bcm2835.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index b4c7f95..174cca9 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -29,7 +29,6 @@
 struct bcm2835_pwm {
 	struct pwm_chip chip;
 	struct device *dev;
-	unsigned long scaler;
 	void __iomem *base;
 	struct clk *clk;
 };
@@ -66,6 +65,7 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			      int duty_ns, int period_ns)
 {
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
+	unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
 
 	if (period_ns <= MIN_PERIOD) {
 		dev_err(pc->dev, "period %d not supported, minimum %d\n",
@@ -73,8 +73,8 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 	}
 
-	writel(duty_ns / pc->scaler, pc->base + DUTY(pwm->hwpwm));
-	writel(period_ns / pc->scaler, pc->base + PERIOD(pwm->hwpwm));
+	writel(duty_ns / scaler, pc->base + DUTY(pwm->hwpwm));
+	writel(period_ns / scaler, pc->base + PERIOD(pwm->hwpwm));
 
 	return 0;
 }
@@ -156,8 +156,6 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	pc->scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
-
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &bcm2835_pwm_ops;
 	pc->chip.npwm = 2;
-- 
1.7.9.5

[PATCH 2/3] pwm: bcm2835: prevent division by zero

[Stefan Wahren]

It's possible that the pwm clock become an orphan. So better
check the result of clk_get_rate in order to prevent a division
by zero.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/pwm/pwm-bcm2835.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 174cca9..31a6992 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -65,7 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			      int duty_ns, int period_ns)
 {
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
+	unsigned long rate = clk_get_rate(pc->clk);
+	unsigned long scaler;
+
+	if (!rate) {
+		dev_err(pc->dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
+	scaler = NSEC_PER_SEC / rate;
 
 	if (period_ns <= MIN_PERIOD) {
 		dev_err(pc->dev, "period %d not supported, minimum %d\n",
-- 
1.7.9.5

[PATCH 3/3] pwm: bcm2835: fix email address specifiction

[Stefan Wahren]

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/pwm/pwm-bcm2835.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 31a6992..c5dbf16 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -206,6 +206,6 @@ static struct platform_driver bcm2835_pwm_driver = {
 };
 module_platform_driver(bcm2835_pwm_driver);
 
-MODULE_AUTHOR("Bart Tanghe <bart.tanghe@thomasmore.be");
+MODULE_AUTHOR("Bart Tanghe <bart.tanghe@thomasmore.be>");
 MODULE_DESCRIPTION("Broadcom BCM2835 PWM driver");
 MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH 2/3] pwm: bcm2835: prevent division by zero

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

> It's possible that the pwm clock become an orphan. So better
> check the result of clk_get_rate in order to prevent a division
> by zero.

How would we lose our clock when we're keeping the clock enabled from
driver probe until remove?

Patches 1 and 3 are:

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] pwm: bcm2835: prevent division by zero

[Mathieu Poirier]

On 1 December 2015 at 15:55, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> It's possible that the pwm clock become an orphan. So better
> check the result of clk_get_rate in order to prevent a division
> by zero.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/pwm/pwm-bcm2835.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> index 174cca9..31a6992 100644
> --- a/drivers/pwm/pwm-bcm2835.c
> +++ b/drivers/pwm/pwm-bcm2835.c
> @@ -65,7 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                               int duty_ns, int period_ns)
>  {
>         struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> -       unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
> +       unsigned long rate = clk_get_rate(pc->clk);
> +       unsigned long scaler;
> +
> +       if (!rate) {
> +               dev_err(pc->dev, "failed to get clock rate\n");
> +               return -EINVAL;
> +       }
> +
> +       scaler = NSEC_PER_SEC / rate;

Stefan,

Please merge this code into patch 1/3.  That way it is done the right
way the first time around.

Thanks,
Mathieu

>
>         if (period_ns <= MIN_PERIOD) {
>                 dev_err(pc->dev, "period %d not supported, minimum %d\n",
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] pwm: bcm2835: prevent division by zero

[Stefan Wahren]

Am 02.12.2015 um 00:16 schrieb Eric Anholt:
> Stefan Wahren <stefan.wahren@i2se.com> writes:
>
>> It's possible that the pwm clock become an orphan. So better
>> check the result of clk_get_rate in order to prevent a division
>> by zero.
> How would we lose our clock when we're keeping the clock enabled from
> driver probe until remove?

It's not the problem that we lose the clock inside this driver. The pwm
clock could be initial assigned to a unregistered parent clock like "GND".

I'm refering to this discussion:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-November/002603.html

>
> Patches 1 and 3 are:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>

Re: [PATCH 2/3] pwm: bcm2835: prevent division by zero

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 1 December 2015 at 15:55, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> It's possible that the pwm clock become an orphan. So better
>> check the result of clk_get_rate in order to prevent a division
>> by zero.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  drivers/pwm/pwm-bcm2835.c |   10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
>> index 174cca9..31a6992 100644
>> --- a/drivers/pwm/pwm-bcm2835.c
>> +++ b/drivers/pwm/pwm-bcm2835.c
>> @@ -65,7 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>                               int duty_ns, int period_ns)
>>  {
>>         struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
>> -       unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
>> +       unsigned long rate = clk_get_rate(pc->clk);
>> +       unsigned long scaler;
>> +
>> +       if (!rate) {
>> +               dev_err(pc->dev, "failed to get clock rate\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       scaler = NSEC_PER_SEC / rate;
>
> Stefan,
>
> Please merge this code into patch 1/3.  That way it is done the right
> way the first time around.

They're separate changes and are good as separate patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] pwm: bcm2835: prevent division by zero

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

> Am 02.12.2015 um 00:16 schrieb Eric Anholt:
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>>
>>> It's possible that the pwm clock become an orphan. So better
>>> check the result of clk_get_rate in order to prevent a division
>>> by zero.
>> How would we lose our clock when we're keeping the clock enabled from
>> driver probe until remove?
>
> It's not the problem that we lose the clock inside this driver. The pwm
> clock could be initial assigned to a unregistered parent clock like "GND".
>
> I'm refering to this discussion:
>
> http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-November/002603.html

I actually would have expected that the prepare would error out on a
clock with no rate set.  Interesting.  This seems like a good safety
measure, so this one is also:

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 0/3] pwm: bcm2835: improve clock handling

[Stefan Wahren]

> Stefan Wahren <stefan.wahren@i2se.com> hat am 1. Dezember 2015 um 23:55
> geschrieben:
>
>
> With the upcoming PWM clock support the pwm-bcm2835 should take
> more care about the clock handling. But this patch series doesn't
> depend on these clock driver changes.
>
> Stefan Wahren (3):
> pwm: bcm2835: calculate scaler in pwm_config
> pwm: bcm2835: prevent division by zero
> pwm: bcm2835: fix email address specifiction
>
> drivers/pwm/pwm-bcm2835.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>

gently ping

Re: [PATCH 0/3] pwm: bcm2835: improve clock handling

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

>> Stefan Wahren <stefan.wahren@i2se.com> hat am 1. Dezember 2015 um 23:55
>> geschrieben:
>>
>>
>> With the upcoming PWM clock support the pwm-bcm2835 should take
>> more care about the clock handling. But this patch series doesn't
>> depend on these clock driver changes.
>>
>> Stefan Wahren (3):
>> pwm: bcm2835: calculate scaler in pwm_config
>> pwm: bcm2835: prevent division by zero
>> pwm: bcm2835: fix email address specifiction
>>
>> drivers/pwm/pwm-bcm2835.c | 18 ++++++++++++------
>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>
>
> gently ping

Thierry, do you want to fold in my r-bs and apply, or should Stefan
resend with them?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 0/3] pwm: bcm2835: improve clock handling

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Tue, Dec 01, 2015 at 10:55:38PM +0000, Stefan Wahren wrote:
> With the upcoming PWM clock support the pwm-bcm2835 should take
> more care about the clock handling. But this patch series doesn't
> depend on these clock driver changes.
> 
> Stefan Wahren (3):
>   pwm: bcm2835: calculate scaler in pwm_config
>   pwm: bcm2835: prevent division by zero
>   pwm: bcm2835: fix email address specifiction
> 
>  drivers/pwm/pwm-bcm2835.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

All three patches applied with Eric's Reviewed-by, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]