[PATCH v1] pwm: lpss: Use positive condition in pwm_lpss_prepare()

[PATCH v1] pwm: lpss: Use positive condition in pwm_lpss_prepare()

[Andy Shevchenko]

For better readability and maintenance use positive condition
in pwm_lpss_prepare(). No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 75bbfe5f3bc2..6930a1d99860 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -109,10 +109,11 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
 	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
 	ctrl |= on_time_div;
 
-	if (orig_ctrl != ctrl) {
-		pwm_lpss_write(pwm, ctrl);
-		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
-	}
+	if (orig_ctrl == ctrl)
+		return;
+
+	pwm_lpss_write(pwm, ctrl);
+	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
 }
 
 static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
-- 
2.24.1

Re: [PATCH v1] pwm: lpss: Use positive condition in pwm_lpss_prepare()

[Uwe Kleine-König]

On Wed, Jan 15, 2020 at 05:08:49PM +0200, Andy Shevchenko wrote:
> For better readability and maintenance use positive condition
> in pwm_lpss_prepare(). No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pwm/pwm-lpss.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 75bbfe5f3bc2..6930a1d99860 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -109,10 +109,11 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
>  	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
>  	ctrl |= on_time_div;
>  
> -	if (orig_ctrl != ctrl) {
> -		pwm_lpss_write(pwm, ctrl);
> -		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> -	}
> +	if (orig_ctrl == ctrl)
> +		return;
> +
> +	pwm_lpss_write(pwm, ctrl);
> +	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);

I personally don't think that readability improved much and think that
the old code is more intuitive. ("If the wanted register value doesn't
match the actual value, write the value out.")

But I agree that the patch doesn't introduce a semantic difference.

What made you create that patch? Is it really that you read through the
driver and thought "Huh, this is more complicated than necessary."?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v1] pwm: lpss: Use positive condition in pwm_lpss_prepare()

[Andy Shevchenko]

On Wed, Jan 15, 2020 at 04:54:25PM +0100, Uwe Kleine-König wrote:
> On Wed, Jan 15, 2020 at 05:08:49PM +0200, Andy Shevchenko wrote:

...

> > -	if (orig_ctrl != ctrl) {
> > -		pwm_lpss_write(pwm, ctrl);
> > -		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> > -	}
> > +	if (orig_ctrl == ctrl)
> > +		return;
> > +
> > +	pwm_lpss_write(pwm, ctrl);
> > +	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> 
> I personally don't think that readability improved much and think that
> the old code is more intuitive. ("If the wanted register value doesn't
> match the actual value, write the value out.")
> 
> But I agree that the patch doesn't introduce a semantic difference.
> 
> What made you create that patch? Is it really that you read through the
> driver and thought "Huh, this is more complicated than necessary."?

My personal preferable style. So, I can survive with the current code.
Thanks for review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] pwm: lpss: Use positive condition in pwm_lpss_prepare()

[Andy Shevchenko]

On Wed, Jan 15, 2020 at 04:54:25PM +0100, Uwe Kleine-König wrote:
> On Wed, Jan 15, 2020 at 05:08:49PM +0200, Andy Shevchenko wrote:
> > For better readability and maintenance use positive condition
> > in pwm_lpss_prepare(). No functional change intended.

...

> > @@ -109,10 +109,11 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
> >  	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
> >  	ctrl |= on_time_div;
> >  
> > -	if (orig_ctrl != ctrl) {
> > -		pwm_lpss_write(pwm, ctrl);
> > -		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> > -	}
> > +	if (orig_ctrl == ctrl)
> > +		return;
> > +
> > +	pwm_lpss_write(pwm, ctrl);
> > +	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> 
> I personally don't think that readability improved much and think that
> the old code is more intuitive. ("If the wanted register value doesn't
> match the actual value, write the value out.")
> 
> But I agree that the patch doesn't introduce a semantic difference.
> 
> What made you create that patch? Is it really that you read through the
> driver and thought "Huh, this is more complicated than necessary."?

Fun fact that d6d54bacb1dd could be slightly better with this applied, but we
have what we have :-)

-- 
With Best Regards,
Andy Shevchenko