| Message ID | 20190919091728.24756-1-linux@rasmusvillemoes.dk |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: rockchip: simplify rockchip_pwm_get_state() | expand |
On Thu, Sep 19, 2019 at 11:17:27AM +0200, Rasmus Villemoes wrote: > The way state->enabled is computed is rather convoluted and hard to > read - both branches of the if() actually do the exact same thing. So > remove the if(), and further simplify "<boolean condition> ? true : > false" to "<boolean condition>". > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > I stumbled on this while trying to understand how the pwm subsystem > works. This patch is a semantic no-op, but it's also possible that, > say, the first branch simply contains a "double negative" so either > the != should be == or the "false : true" should be "true : false". The change looks obviously right, it's a noop. I share your doubts however. The construct was introduced in commit 831b2790507b ("pwm: rockchip: Use same PWM ops for each IP") by David Wu. Before there were rockchip_pwm_get_state_v1 for the supports_polarity = false case and rockchip_pwm_get_state_v2 for supports_polarity = true. In both state->enabled was assigned true if ((val & enable_conf) == enable_conf). So I assume everything is fine. A confirmation by David would be great though. As a side note: Is there publicly available documentation for this IP? If a link were added to the driver's header we could check easily ourselves. Best regards Uwe
Hello David, On Thu, Sep 19, 2019 at 01:11:15PM +0200, oUwe Kleine-König wrote: > On Thu, Sep 19, 2019 at 11:17:27AM +0200, Rasmus Villemoes wrote: > > The way state->enabled is computed is rather convoluted and hard to > > read - both branches of the if() actually do the exact same thing. So > > remove the if(), and further simplify "<boolean condition> ? true : > > false" to "<boolean condition>". > > > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > --- > > I stumbled on this while trying to understand how the pwm subsystem > > works. This patch is a semantic no-op, but it's also possible that, > > say, the first branch simply contains a "double negative" so either > > the != should be == or the "false : true" should be "true : false". > > The change looks obviously right, it's a noop. > > I share your doubts however. The construct was introduced in commit > 831b2790507b ("pwm: rockchip: Use same PWM ops for each IP") by David > Wu. > > Before there were rockchip_pwm_get_state_v1 for the supports_polarity = > false case and rockchip_pwm_get_state_v2 for supports_polarity = true. > > In both state->enabled was assigned true if ((val & enable_conf) == > enable_conf). So I assume everything is fine. > > A confirmation by David would be great though. This is still open. Can you please have a look at https://patchwork.ozlabs.org/project/linux-pwm/patch/20190919091728.24756-1-linux@rasmusvillemoes.dk/ and verify it's correct? Best regards Uwe
On Thu, Sep 19, 2019 at 11:17:27AM +0200, Rasmus Villemoes wrote: > The way state->enabled is computed is rather convoluted and hard to > read - both branches of the if() actually do the exact same thing. So > remove the if(), and further simplify "<boolean condition> ? true : > false" to "<boolean condition>". > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > I stumbled on this while trying to understand how the pwm subsystem > works. This patch is a semantic no-op, but it's also possible that, > say, the first branch simply contains a "double negative" so either > the != should be == or the "false : true" should be "true : false". > > drivers/pwm/pwm-rockchip.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) I've applied this. Irrespective of any feedback David would have this is correct and a nice simplification. Thierry
This change is very good, thank you. The code continues from the original code(get_state_v1 and get_state_v2), didn’t make any changes at that time, and sorry I have not seen linux-rockchip@lists.infradead.org mail recently. 在 2020/6/2 下午8:39, Thierry Reding 写道: > On Thu, Sep 19, 2019 at 11:17:27AM +0200, Rasmus Villemoes wrote: >> The way state->enabled is computed is rather convoluted and hard to >> read - both branches of the if() actually do the exact same thing. So >> remove the if(), and further simplify "<boolean condition> ? true : >> false" to "<boolean condition>". >> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> --- >> I stumbled on this while trying to understand how the pwm subsystem >> works. This patch is a semantic no-op, but it's also possible that, >> say, the first branch simply contains a "double negative" so either >> the != should be == or the "false : true" should be "true : false". >> >> drivers/pwm/pwm-rockchip.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) > > I've applied this. Irrespective of any feedback David would have this is > correct and a nice simplification. > > Thierry >
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index 51b96cb7dd25..54c6399e3f00 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -83,12 +83,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip, state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate); val = readl_relaxed(pc->base + pc->data->regs.ctrl); - if (pc->data->supports_polarity) - state->enabled = ((val & enable_conf) != enable_conf) ? - false : true; - else - state->enabled = ((val & enable_conf) == enable_conf) ? - true : false; + state->enabled = ((val & enable_conf) == enable_conf); if (pc->data->supports_polarity) { if (!(val & PWM_DUTY_POSITIVE))
The way state->enabled is computed is rather convoluted and hard to read - both branches of the if() actually do the exact same thing. So remove the if(), and further simplify "<boolean condition> ? true : false" to "<boolean condition>". Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- I stumbled on this while trying to understand how the pwm subsystem works. This patch is a semantic no-op, but it's also possible that, say, the first branch simply contains a "double negative" so either the != should be == or the "false : true" should be "true : false". drivers/pwm/pwm-rockchip.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)