| Message ID | 1449010541-3767-3-git-send-email-stefan.wahren@i2se.com |
|---|---|
| State | Accepted |
| Headers | show |
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>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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>
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",
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(-)