* [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state @ 2023-04-19 21:30 Heiner Kallweit 2023-04-21 14:57 ` Dmitry Rokosov 0 siblings, 1 reply; 5+ messages in thread From: Heiner Kallweit @ 2023-04-19 21:30 UTC (permalink / raw) To: Jerome Brunet, Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Uwe Kleine-König, thierry.reding@gmail.com Cc: linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-pwm 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(-) 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-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state 2023-04-19 21:30 [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state Heiner Kallweit @ 2023-04-21 14:57 ` Dmitry Rokosov 2023-04-21 15:33 ` Heiner Kallweit 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Rokosov @ 2023-04-21 14:57 UTC (permalink / raw) To: Heiner Kallweit Cc: Jerome Brunet, Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Uwe Kleine-König, thierry.reding@gmail.com, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-pwm, kernel 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 -- Thank you, Dmitry _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state 2023-04-21 14:57 ` Dmitry Rokosov @ 2023-04-21 15:33 ` Heiner Kallweit 2023-04-21 19:14 ` Dmitry Rokosov 0 siblings, 1 reply; 5+ messages in thread From: Heiner Kallweit @ 2023-04-21 15:33 UTC (permalink / raw) To: Dmitry Rokosov Cc: Jerome Brunet, Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Uwe Kleine-König, thierry.reding@gmail.com, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-pwm, 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 > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state 2023-04-21 15:33 ` Heiner Kallweit @ 2023-04-21 19:14 ` Dmitry Rokosov 2023-04-23 17:59 ` Martin Blumenstingl 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Rokosov @ 2023-04-21 19:14 UTC (permalink / raw) To: Heiner Kallweit Cc: Jerome Brunet, Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Uwe Kleine-König, thierry.reding@gmail.com, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-pwm, 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 > > > -- Thank you, Dmitry _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state 2023-04-21 19:14 ` Dmitry Rokosov @ 2023-04-23 17:59 ` Martin Blumenstingl 0 siblings, 0 replies; 5+ messages in thread From: Martin Blumenstingl @ 2023-04-23 17:59 UTC (permalink / raw) To: Dmitry Rokosov Cc: Heiner Kallweit, Jerome Brunet, Neil Armstrong, Kevin Hilman, Uwe Kleine-König, thierry.reding@gmail.com, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-pwm, 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). _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-23 18:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-19 21:30 [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state Heiner Kallweit 2023-04-21 14:57 ` Dmitry Rokosov 2023-04-21 15:33 ` Heiner Kallweit 2023-04-21 19:14 ` Dmitry Rokosov 2023-04-23 17:59 ` Martin Blumenstingl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox