From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: MIME-Version: 1.0 From: Kever Yang Date: Fri, 25 Dec 2020 15:15:12 +0800 Message-ID: Content-Type: multipart/alternative; boundary="00000000000016b26905b744b0ea" Subject: Re: [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate() To: Simon South Cc: tpiepho@gmail.com, thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de, Robin Murphy , lee.jones@linaro.org, Heiko Stuebner , bbrezillon@kernel.org, linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, David Wu , steven.liu@rock-chips.com List-ID: --00000000000016b26905b744b0ea Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable + David and Steven, Hi Steven, please help to review this patch set. Thanks - Kever Simon South =E4=BA=8E2020=E5=B9=B412=E6=9C=8824=E6= =97=A5=E5=91=A8=E5=9B=9B =E4=B8=8A=E5=8D=8812:01=E5=86=99=E9=81=93=EF=BC=9A > The documentation for clk_get_rate() in include/linux/clk.h states the > function's result is valid only for a clock source that has been > enabled. However, the Rockchip PWM driver uses this function in two place= s > to query the rate of a clock without first ensuring it is enabled. > > Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() s= o > they enable a device's PWM clock before querying its rate (in the latter > case, the querying is actually done in rockchip_pwm_config()) and disable > the clock again before returning. > > Reported-by: Uwe Kleine-K=C3=B6nig > Signed-off-by: Simon South > --- > drivers/pwm/pwm-rockchip.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index 02da7370db70..44425eeb4e81 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -72,6 +72,10 @@ static void rockchip_pwm_get_state(struct pwm_chip > *chip, > if (ret) > return; > > + ret =3D clk_enable(pc->clk); > + if (ret) > + return; > + > clk_rate =3D clk_get_rate(pc->clk); > > tmp =3D readl_relaxed(pc->base + pc->data->regs.period); > @@ -90,6 +94,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chi= p, > else > state->polarity =3D PWM_POLARITY_NORMAL; > > + clk_disable(pc->clk); > clk_disable(pc->pclk); > } > > @@ -189,6 +194,10 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, > struct pwm_device *pwm, > if (ret) > return ret; > > + ret =3D clk_enable(pc->clk); > + if (ret) > + return ret; > + > pwm_get_state(pwm, &curstate); > enabled =3D curstate.enabled; > > @@ -208,6 +217,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, > struct pwm_device *pwm, > } > > out: > + clk_disable(pc->clk); > clk_disable(pc->pclk); > > return ret; > -- > 2.29.2 > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > --00000000000016b26905b744b0ea Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
+ David and Steven,=

Hi Steven,
=C2=A0 =C2=A0 please hel= p to review this patch set.

Thanks
- Kever

Sim= on South <simon@simonsouth.net> =E4=BA=8E2020=E5=B9=B412=E6=9C=8824=E6=97=A5=E5=91=A8=E5=9B=9B =E4= =B8=8A=E5=8D=8812:01=E5=86=99=E9=81=93=EF=BC=9A
The documentation for clk_get_rate() in in= clude/linux/clk.h states the
function's result is valid only for a clock source that has been
enabled. However, the Rockchip PWM driver uses this function in two places<= br> to query the rate of a clock without first ensuring it is enabled.

Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() so<= br> they enable a device's PWM clock before querying its rate (in the latte= r
case, the querying is actually done in rockchip_pwm_config()) and disable the clock again before returning.

Reported-by: Uwe Kleine-K=C3=B6nig <u.kleine-koenig@pengutronix.de>
Signed-off-by: Simon South <simon@simonsouth.net>
---
=C2=A0drivers/pwm/pwm-rockchip.c | 10 ++++++++++
=C2=A01 file changed, 10 insertions(+)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 02da7370db70..44425eeb4e81 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -72,6 +72,10 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip= ,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (ret)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;

+=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D clk_enable(pc->clk);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
+
=C2=A0 =C2=A0 =C2=A0 =C2=A0 clk_rate =3D clk_get_rate(pc->clk);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 tmp =3D readl_relaxed(pc->base + pc->data= ->regs.period);
@@ -90,6 +94,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 else
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 state->polarity = =3D PWM_POLARITY_NORMAL;

+=C2=A0 =C2=A0 =C2=A0 =C2=A0clk_disable(pc->clk);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 clk_disable(pc->pclk);
=C2=A0}

@@ -189,6 +194,10 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, s= truct pwm_device *pwm,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (ret)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;

+=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D clk_enable(pc->clk);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
+
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pwm_get_state(pwm, &curstate);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 enabled =3D curstate.enabled;

@@ -208,6 +217,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, st= ruct pwm_device *pwm,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0out:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0clk_disable(pc->clk);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 clk_disable(pc->pclk);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
--
2.29.2


_______________________________________________
Linux-rockchip mailing list
Lin= ux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listin= fo/linux-rockchip
--00000000000016b26905b744b0ea--