[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[Heiner Kallweit]

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

[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[Heiner Kallweit]

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

[PATCH] pwm: meson: simplify calculation in meson_pwm_get_state

[Heiner Kallweit]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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

Re: [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@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

Re: [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@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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Heiner Kallweit]

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
> 

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

[Heiner Kallweit]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Heiner Kallweit]

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

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

[Dmitry Rokosov]

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

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

[Dmitry Rokosov]

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

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

[Dmitry Rokosov]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Martin Blumenstingl]

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).

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

[Martin Blumenstingl]

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

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

[Martin Blumenstingl]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel