Re: [PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[Heiner Kallweit <hkallweit1@gmail.com>]

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