[PATCH v2] staging: rts5208: Clean up coding style in rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into one

[PATCH v2] staging: rts5208: Clean up coding style in rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into one

[Giedrius Statkevicius]

Fix 10 occurences of coding style errors where the line exceeds 80 characters by breaking them into more lines.
Checkpatch.pl reports these errors as: 'WARNING: line over 80 characters'
Also, removes unnecessary returns in two void functions by removing the return statements.
Checkpatch.pl reports these errors as: 'WARNING: void function return statements are not generally useful'
Lastly it combines two if statements into one in rtsx_reset_chip()

Patch is against Greg KH's staging-next tree.

Signed-off-by: Giedrius Statkevicius <giedrius.statkevicius@gmail.com>
---
Changes in v2:
  - Combines two if statements into one in rtsx_reset_chip() (thanks Dan Carpenter)
  - Aligns the second line of a if statement to the opening parenthesis in rtsx_polling_func() (thanks Dan Carpenter)

 drivers/staging/rts5208/rtsx_chip.c | 47 ++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx_chip.c b/drivers/staging/rts5208/rtsx_chip.c
index 1411471..20e0712 100644
--- a/drivers/staging/rts5208/rtsx_chip.c
+++ b/drivers/staging/rts5208/rtsx_chip.c
@@ -289,12 +289,11 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
 	/* Enable ASPM */
 	if (chip->aspm_l0s_l1_en) {
 		if (chip->dynamic_aspm) {
-			if (CHK_SDIO_EXIST(chip)) {
-				if (CHECK_PID(chip, 0x5288)) {
-					retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
-					if (retval != STATUS_SUCCESS)
-						TRACE_RET(chip, STATUS_FAIL);
-				}
+			if (CHK_SDIO_EXIST(chip) && CHECK_PID(chip, 0x5288)) {
+				retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF,
+						chip->aspm_l0s_l1_en);
+				if (retval != STATUS_SUCCESS)
+					TRACE_RET(chip, STATUS_FAIL);
 			}
 		} else {
 			if (CHECK_PID(chip, 0x5208))
@@ -310,9 +309,13 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
 			if (CHK_SDIO_EXIST(chip)) {
 				chip->aspm_level[1] = chip->aspm_l0s_l1_en;
 				if (CHECK_PID(chip, 0x5288))
-					retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
+					retval = rtsx_write_cfg_dw(chip, 2,
+							0xC0, 0xFF,
+							chip->aspm_l0s_l1_en);
 				else
-					retval = rtsx_write_cfg_dw(chip, 1, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
+					retval = rtsx_write_cfg_dw(chip, 1,
+							0xC0, 0xFF,
+							chip->aspm_l0s_l1_en);
 
 				if (retval != STATUS_SUCCESS)
 					TRACE_RET(chip, STATUS_FAIL);
@@ -966,7 +969,8 @@ void rtsx_polling_func(struct rtsx_chip *chip)
 					dev_dbg(rtsx_dev(chip), "SDIO enter ASPM!\n");
 					rtsx_write_register(chip,
 						ASPM_FORCE_CTL, 0xFC,
-						0x30 | (chip->aspm_level[1] << 2));
+						0x30 |
+						(chip->aspm_level[1] << 2));
 					chip->sdio_aspm = 1;
 				}
 			}
@@ -988,8 +992,10 @@ void rtsx_polling_func(struct rtsx_chip *chip)
 
 			turn_off_led(chip, LED_GPIO);
 
-			if (chip->auto_power_down && !chip->card_ready && !chip->sd_io)
-				rtsx_force_power_down(chip, SSC_PDCTL | OC_PDCTL);
+			if (chip->auto_power_down && !chip->card_ready &&
+			    !chip->sd_io)
+				rtsx_force_power_down(chip,
+						SSC_PDCTL | OC_PDCTL);
 
 		}
 	}
@@ -1081,7 +1087,9 @@ Delink_Stage:
 					dev_dbg(rtsx_dev(chip), "False card inserted, do force delink\n");
 
 					if (enter_L1)
-						rtsx_write_register(chip, HOST_SLEEP_STATE, 0x03, 1);
+						rtsx_write_register(chip,
+							      HOST_SLEEP_STATE,
+							      0x03, 1);
 
 					rtsx_write_register(chip,
 							CHANGE_LINK_STATE, 0x0A,
@@ -1090,14 +1098,19 @@ Delink_Stage:
 					if (enter_L1)
 						rtsx_enter_L1(chip);
 
-					chip->auto_delink_cnt = delink_stage3_cnt + 1;
+					chip->auto_delink_cnt =
+						delink_stage3_cnt + 1;
 				} else {
 					dev_dbg(rtsx_dev(chip), "No card inserted, do delink\n");
 
 					if (enter_L1)
-						rtsx_write_register(chip, HOST_SLEEP_STATE, 0x03, 1);
+						rtsx_write_register(chip,
+							      HOST_SLEEP_STATE,
+							      0x03, 1);
 
-					rtsx_write_register(chip, CHANGE_LINK_STATE, 0x02, 0x02);
+					rtsx_write_register(chip,
+							CHANGE_LINK_STATE, 0x02,
+							0x02);
 
 					if (enter_L1)
 						rtsx_enter_L1(chip);
@@ -1823,8 +1836,6 @@ void rtsx_enable_aspm(struct rtsx_chip *chip)
 			}
 		}
 	}
-
-	return;
 }
 
 void rtsx_disable_aspm(struct rtsx_chip *chip)
@@ -1848,8 +1859,6 @@ void rtsx_disable_aspm(struct rtsx_chip *chip)
 			wait_timeout(1);
 		}
 	}
-
-	return;
 }
 
 int rtsx_read_ppbuf(struct rtsx_chip *chip, u8 *buf, int buf_len)
-- 
2.1.2

Re: [PATCH v2] staging: rts5208: Clean up coding style in rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into one

[Joe Perches]

On Thu, 2014-10-02 at 14:13 +0300, Giedrius Statkevicius wrote:
> Fix 10 occurences of coding style errors where the line exceeds 80 characters by breaking them into more lines.
> Checkpatch.pl reports these errors as: 'WARNING: line over 80 characters'
> Also, removes unnecessary returns in two void functions by removing the return statements.
> Checkpatch.pl reports these errors as: 'WARNING: void function return statements are not generally useful'
> Lastly it combines two if statements into one in rtsx_reset_chip()

Another way to reduce indentation is to write another
utility function like the suggested patch at the end
of this email.

Also, changing:

	if (foo) {
		[short code block...]
	} else
		[long code block...]
	}

	return bar;
to:
	if (foo) {
		[short code block...]
		return bar;
	}

	[long code block...]
	return bar;

reduces indentation.

Al Viro once gave good advice about indentation here:
https://lkml.org/lkml/2013/3/20/345

Using ternaries can also reduce line count and
indentation:
	if (something)
		foo(bar, 1, ...);
	else
		foo(bar, 2, ...);
can be written
	foo(bar, something ? 1 : 2, ...)

Here's a suggestion:
---
 drivers/staging/rts5208/rtsx_chip.c | 71 ++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx_chip.c b/drivers/staging/rts5208/rtsx_chip.c
index 1411471..85d670d 100644
--- a/drivers/staging/rts5208/rtsx_chip.c
+++ b/drivers/staging/rts5208/rtsx_chip.c
@@ -226,6 +226,43 @@ static int rtsx_pre_handle_sdio_new(struct rtsx_chip *chip)
 }
 #endif
 
+static int rtsx_reset_aspm(struct rtsx_chip *chip)
+{
+	int rtn = STATUS_SUCCESS;
+
+	if (chip->dynamic_aspm) {
+		if (CHK_SDIO_EXIST(chip) && CHECK_PID(chip, 0x5288)) {
+			rtn = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF,
+						chip->aspm_l0s_l1_en);
+			if (rtn != STATUS_SUCCESS)
+				TRACE_RET(chip, STATUS_FAIL);
+		}
+		return rtn;
+	}
+
+	if (CHECK_PID(chip, 0x5208))
+		RTSX_WRITE_REG(chip, ASPM_FORCE_CTL, 0xFF, 0x3F);
+
+	rtn = rtsx_write_config_byte(chip, LCTLR, chip->aspm_l0s_l1_en);
+	if (rtn != STATUS_SUCCESS)
+		TRACE_RET(chip, STATUS_FAIL);
+
+	chip->aspm_level[0] = chip->aspm_l0s_l1_en;
+
+	if (CHK_SDIO_EXIST(chip)) {
+		chip->aspm_level[1] = chip->aspm_l0s_l1_en;
+		rtn = rtsx_write_cfg_dw(chip,
+					CHECK_PID(chip, 0x5288) ? 2 : 1,
+					0xC0, 0xFF, chip->aspm_l0s_l1_en);
+		if (rtn != STATUS_SUCCESS)
+			TRACE_RET(chip, STATUS_FAIL);
+	}
+
+	chip->aspm_enabled = 1;
+
+	return rtn;
+}
+
 int rtsx_reset_chip(struct rtsx_chip *chip)
 {
 	int retval;
@@ -288,39 +325,7 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
 
 	/* Enable ASPM */
 	if (chip->aspm_l0s_l1_en) {
-		if (chip->dynamic_aspm) {
-			if (CHK_SDIO_EXIST(chip)) {
-				if (CHECK_PID(chip, 0x5288)) {
-					retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
-					if (retval != STATUS_SUCCESS)
-						TRACE_RET(chip, STATUS_FAIL);
-				}
-			}
-		} else {
-			if (CHECK_PID(chip, 0x5208))
-				RTSX_WRITE_REG(chip, ASPM_FORCE_CTL,
-					0xFF, 0x3F);
-
-			retval = rtsx_write_config_byte(chip, LCTLR,
-							chip->aspm_l0s_l1_en);
-			if (retval != STATUS_SUCCESS)
-				TRACE_RET(chip, STATUS_FAIL);
-
-			chip->aspm_level[0] = chip->aspm_l0s_l1_en;
-			if (CHK_SDIO_EXIST(chip)) {
-				chip->aspm_level[1] = chip->aspm_l0s_l1_en;
-				if (CHECK_PID(chip, 0x5288))
-					retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
-				else
-					retval = rtsx_write_cfg_dw(chip, 1, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
-
-				if (retval != STATUS_SUCCESS)
-					TRACE_RET(chip, STATUS_FAIL);
-
-			}
-
-			chip->aspm_enabled = 1;
-		}
+		retval = rtsx_reset_aspm(chip);
 	} else {
 		if (chip->asic_code && CHECK_PID(chip, 0x5208)) {
 			retval = rtsx_write_phy_register(chip, 0x07, 0x0129);

Re: [PATCH v2] staging: rts5208: Clean up coding style in rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into one

[Giedrius Statkevicius]

On 2014.10.02 14:45, Joe Perches wrote:
> On Thu, 2014-10-02 at 14:13 +0300, Giedrius Statkevicius wrote:
>> Fix 10 occurences of coding style errors where the line exceeds 80 characters by breaking them into more lines.
>> Checkpatch.pl reports these errors as: 'WARNING: line over 80 characters'
>> Also, removes unnecessary returns in two void functions by removing the return statements.
>> Checkpatch.pl reports these errors as: 'WARNING: void function return statements are not generally useful'
>> Lastly it combines two if statements into one in rtsx_reset_chip()
> 
> Another way to reduce indentation is to write another
> utility function like the suggested patch at the end
> of this email.
> 
> Also, changing:
> 
> 	if (foo) {
> 		[short code block...]
> 	} else
> 		[long code block...]
> 	}
> 
> 	return bar;
> to:
> 	if (foo) {
> 		[short code block...]
> 		return bar;
> 	}
> 
> 	[long code block...]
> 	return bar;
> 
> reduces indentation.
> 
> Al Viro once gave good advice about indentation here:
> https://lkml.org/lkml/2013/3/20/345
> 
> Using ternaries can also reduce line count and
> indentation:
> 	if (something)
> 		foo(bar, 1, ...);
> 	else
> 		foo(bar, 2, ...);
> can be written
> 	foo(bar, something ? 1 : 2, ...)
First of all, the purpose of this trivial patch is to clean the simple, obvious coding style mistakes reported by checkpatch.pl to get my feet wet as I am a kernel newbie.
Obviously, if we take a look at the code more closely there are many problems including the ones you've mentioned.
For example, in rts5208_init() in atleast in 6 places we can use ternaries. This is one of them:

	if (val & 0x01)
		chip->hw_bypass_sd = 1;
	else
		chip->hw_bypass_sd = 0; 

A lot of places where we can combine if's. In rtsx_disable_aspm():

	if (chip->aspm_l0s_l1_en && chip->dynamic_aspm) {
		if (chip->aspm_enabled) {

So I guess I should make a way bigger patch that cleans more of the coding style mistakes if no one wants to take this patch?
To be honest, I would like these to be two seperate patches as they do different things and not just a big one that does both things as I may not be able to do that sucessfully.

thanks,
Giedrius 
> 
> Here's a suggestion:
> ---
>  drivers/staging/rts5208/rtsx_chip.c | 71 ++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/staging/rts5208/rtsx_chip.c b/drivers/staging/rts5208/rtsx_chip.c
> index 1411471..85d670d 100644
> --- a/drivers/staging/rts5208/rtsx_chip.c
> +++ b/drivers/staging/rts5208/rtsx_chip.c
> @@ -226,6 +226,43 @@ static int rtsx_pre_handle_sdio_new(struct rtsx_chip *chip)
>  }
>  #endif
>  
> +static int rtsx_reset_aspm(struct rtsx_chip *chip)
> +{
> +	int rtn = STATUS_SUCCESS;
> +
> +	if (chip->dynamic_aspm) {
> +		if (CHK_SDIO_EXIST(chip) && CHECK_PID(chip, 0x5288)) {
> +			rtn = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF,
> +						chip->aspm_l0s_l1_en);
> +			if (rtn != STATUS_SUCCESS)
> +				TRACE_RET(chip, STATUS_FAIL);
> +		}
> +		return rtn;
> +	}
> +
> +	if (CHECK_PID(chip, 0x5208))
> +		RTSX_WRITE_REG(chip, ASPM_FORCE_CTL, 0xFF, 0x3F);
> +
> +	rtn = rtsx_write_config_byte(chip, LCTLR, chip->aspm_l0s_l1_en);
> +	if (rtn != STATUS_SUCCESS)
> +		TRACE_RET(chip, STATUS_FAIL);
> +
> +	chip->aspm_level[0] = chip->aspm_l0s_l1_en;
> +
> +	if (CHK_SDIO_EXIST(chip)) {
> +		chip->aspm_level[1] = chip->aspm_l0s_l1_en;
> +		rtn = rtsx_write_cfg_dw(chip,
> +					CHECK_PID(chip, 0x5288) ? 2 : 1,
> +					0xC0, 0xFF, chip->aspm_l0s_l1_en);
> +		if (rtn != STATUS_SUCCESS)
> +			TRACE_RET(chip, STATUS_FAIL);
> +	}
> +
> +	chip->aspm_enabled = 1;
> +
> +	return rtn;
> +}
> +
>  int rtsx_reset_chip(struct rtsx_chip *chip)
>  {
>  	int retval;
> @@ -288,39 +325,7 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
>  
>  	/* Enable ASPM */
>  	if (chip->aspm_l0s_l1_en) {
> -		if (chip->dynamic_aspm) {
> -			if (CHK_SDIO_EXIST(chip)) {
> -				if (CHECK_PID(chip, 0x5288)) {
> -					retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
> -					if (retval != STATUS_SUCCESS)
> -						TRACE_RET(chip, STATUS_FAIL);
> -				}
> -			}
> -		} else {
> -			if (CHECK_PID(chip, 0x5208))
> -				RTSX_WRITE_REG(chip, ASPM_FORCE_CTL,
> -					0xFF, 0x3F);
> -
> -			retval = rtsx_write_config_byte(chip, LCTLR,
> -							chip->aspm_l0s_l1_en);
> -			if (retval != STATUS_SUCCESS)
> -				TRACE_RET(chip, STATUS_FAIL);
> -
> -			chip->aspm_level[0] = chip->aspm_l0s_l1_en;
> -			if (CHK_SDIO_EXIST(chip)) {
> -				chip->aspm_level[1] = chip->aspm_l0s_l1_en;
> -				if (CHECK_PID(chip, 0x5288))
> -					retval = rtsx_write_cfg_dw(chip, 2, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
> -				else
> -					retval = rtsx_write_cfg_dw(chip, 1, 0xC0, 0xFF, chip->aspm_l0s_l1_en);
> -
> -				if (retval != STATUS_SUCCESS)
> -					TRACE_RET(chip, STATUS_FAIL);
> -
> -			}
> -
> -			chip->aspm_enabled = 1;
> -		}
> +		retval = rtsx_reset_aspm(chip);
>  	} else {
>  		if (chip->asic_code && CHECK_PID(chip, 0x5208)) {
>  			retval = rtsx_write_phy_register(chip, 0x07, 0x0129);
> 
> 

Re: [PATCH v2] staging: rts5208: Clean up coding style in rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into one

[Joe Perches]

On Thu, 2014-10-02 at 18:25 +0300, Giedrius Statkevicius wrote:
> On 2014.10.02 14:45, Joe Perches wrote:
> > On Thu, 2014-10-02 at 14:13 +0300, Giedrius Statkevicius wrote:
> >> Fix 10 occurences of coding style errors where the line exceeds 80 characters by breaking them into more lines.
> >> Checkpatch.pl reports these errors as: 'WARNING: line over 80 characters'
> >> Also, removes unnecessary returns in two void functions by removing the return statements.
> >> Checkpatch.pl reports these errors as: 'WARNING: void function return statements are not generally useful'
> >> Lastly it combines two if statements into one in rtsx_reset_chip()
> > 
> > Another way to reduce indentation is to write another
> > utility function like the suggested patch at the end
> > of this email.
[]
> > Using ternaries can also reduce line count and
> > indentation:
> > 	if (something)
> > 		foo(bar, 1, ...);
> > 	else
> > 		foo(bar, 2, ...);
> > can be written
> > 	foo(bar, something ? 1 : 2, ...)
> First of all, the purpose of this trivial patch is to clean the
> simple, obvious coding style mistakes reported by checkpatch.pl to get
> my feet wet as I am a kernel newbie.

Hello Giedrius.

By itself, that's fine.  A mechanical change as a first
contribution as a means to do larger changes is very
welcome, as I hope you are welcomed here.

While mechanical changes have some value, the purpose
of a tool like checkpatch isn't any significant
transformational change, it's a trivial oversight minder.

Fixing checkpatch bleats won't make code "better".

checkpatch really only helps code style conformity.
It tries to help make code a bit more like other code
already in the kernel.
 
People play a larger role in looking at code and
making it cleaner, neater, more intelligible, better
structured, smaller, faster, maintainable, etc.

> Obviously, if we take a look at the code more closely there are many
> problems including the ones you've mentioned.
> For example, in rts5208_init() in atleast in 6 places we can use
> ternaries. This is one of them:
> 
> 	if (val & 0x01)
> 		chip->hw_bypass_sd = 1;
> 	else
> 		chip->hw_bypass_sd = 0; 
> 
> A lot of places where we can combine if's. In rtsx_disable_aspm():
> 
> 	if (chip->aspm_l0s_l1_en && chip->dynamic_aspm) {
> 		if (chip->aspm_enabled) {
> 
> So I guess I should make a way bigger patch that cleans more of the
> coding style mistakes if no one wants to take this patch?
> To be honest, I would like these to be two seperate patches as they do
> different things and not just a big one that does both things as I may
> not be able to do that sucessfully.

Have at it.

Start small, learn a bit, do more, repeat ad libitum...

A series of patches that do single things is much
easier for others to validate, accept and apply.

As long as patches are done in sets of single things,
don't fear making larger transformational changes too.

cheers, Joe