[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[Dmitry Rokosov]

On Fri, Apr 21, 2023 at 05:33:29PM +0200, Heiner Kallweit wrote:
> On 21.04.2023 16:57, Dmitry Rokosov wrote:
> > Hello Heiner,
> > 
> > Thank you for the patch! Please find my comments below.
> > 
> > On Wed, Apr 19, 2023 at 11:30:55PM +0200, Heiner Kallweit wrote:
> >> I don't see a reason why we should treat the case lo < hi that
> >> different and return 0 as period and duty_cycle. Let's handle it as
> >> normal use case and also remove the optimization for lo == 0.
> >> I think the improved readability is worth it.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1 at gmail.com>
> > 
> > Inside this patch, in my opinion, you have not only simplified and
> > optimized but have also modified the logic. It is important to provide
> > more details on this modification. Previously, in cases where
> > (channel->lo != 0) && (channel->lo < channel->hi), period and duty_cycle
> > were not calculated. However, in your patchset, duty_cycle and polarity
> > are calculated and returned to the caller in such cases.
> > Can you please share the details of why this is the right solution?
> 
> It's the obvious solution. I see no reason to return all zero's for
> lo < hi, and also the commit that added this calculation doesn't provide
> an explanation. It just references the calculation in meson_pwm_calc(),
> however I fail to see that lo < hi is treated differently there.
> 
> c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()")
> 

Actually, I don't see any arguments to bypass the case where lo < hi,
so the current implementation of get_state() is questionable.
I think it would be better to wait Martin's opinion why meson_pwm_calc()
logic was inversed with such conditions.

> > Also, please rephrase the commit message using 'modify' instead of
> > 'simplify'.
> > 
> >> ---
> >>  drivers/pwm/pwm-meson.c | 14 ++------------
> >>  1 file changed, 2 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> >> index 5732300eb..3865538dd 100644
> >> --- a/drivers/pwm/pwm-meson.c
> >> +++ b/drivers/pwm/pwm-meson.c
> >> @@ -351,18 +351,8 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >>  	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
> >>  	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
> >>  
> >> -	if (channel->lo == 0) {
> >> -		state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> >> -		state->duty_cycle = state->period;
> >> -	} else if (channel->lo >= channel->hi) {
> >> -		state->period = meson_pwm_cnt_to_ns(chip, pwm,
> >> -						    channel->lo + channel->hi);
> >> -		state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm,
> >> -							channel->hi);
> >> -	} else {
> >> -		state->period = 0;
> >> -		state->duty_cycle = 0;
> >> -	}
> >> +	state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi);
> >> +	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> >>  
> >>  	state->polarity = PWM_POLARITY_NORMAL;
> >>  
> >> -- 
> >> 2.40.0
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 

-- 
Thank you,
Dmitry