[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state
Heiner Kallweit
hkallweit1 at gmail.com
Fri Apr 21 08:33:29 PDT 2023
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()")
> 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
>
More information about the linux-amlogic
mailing list