[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state
Dmitry Rokosov
ddrokosov at sberdevices.ru
Fri Apr 21 12:14:16 PDT 2023
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
More information about the linux-amlogic
mailing list