[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[Dmitry Rokosov]

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?
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