| Message ID | 5a5920db-4c32-25e8-d1e3-bd2f724dd242@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: meson: simplify calculation in meson_pwm_get_state | expand |
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@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? 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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@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()") > 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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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@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@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > >
On Fri, Apr 21, 2023 at 9:14 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote: [...] > > > 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. To be honest: I don't recall why I did it like that. So please go with Dmitry's suggestion (to update the patch description that this "optimization" is now gone).
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;
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@gmail.com> --- drivers/pwm/pwm-meson.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)