staging: rts5208: Two patches to improve code style

staging: rts5208: Two patches to improve code style

[Tobias Nießen]

The following two patches improve the code style in the rts5208 staging
driver. Many other style issues cannot be resolved without much more
extensive refactoring.

[PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Tobias Nießen]

This commit uses the fact that

    if (a) {
            if (b) {
                    ...
            }
    }

can instead be written as

    if (a && b) {
            ...
    }

without any change in behavior, allowing to decrease the indentation
of the contained code block and thus reducing the average line length.

Signed-off-by: Tobias Nießen <tobias.niessen@stud.uni-hannover.de>
Signed-off-by: Sabrina Gaube <sabrina-gaube@web.de>
---
 drivers/staging/rts5208/sd.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
index b3f829ed1019..a06045344301 100644
--- a/drivers/staging/rts5208/sd.c
+++ b/drivers/staging/rts5208/sd.c
@@ -4507,20 +4507,19 @@ int sd_execute_write_data(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 			sd_lock_state, sd_card->sd_lock_status);
 		if (sd_lock_state ^ (sd_card->sd_lock_status & SD_LOCKED)) {
 			sd_card->sd_lock_notify = 1;
-			if (sd_lock_state) {
-				if (sd_card->sd_lock_status & SD_LOCK_1BIT_MODE) {
-					sd_card->sd_lock_status |= (
-						SD_UNLOCK_POW_ON | SD_SDR_RST);
-					if (CHK_SD(sd_card)) {
-						retval = reset_sd(chip);
-						if (retval != STATUS_SUCCESS) {
-							sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
-							goto sd_execute_write_cmd_failed;
-						}
+			if (sd_lock_state &&
+			    (sd_card->sd_lock_status & SD_LOCK_1BIT_MODE)) {
+				sd_card->sd_lock_status |= (
+					SD_UNLOCK_POW_ON | SD_SDR_RST);
+				if (CHK_SD(sd_card)) {
+					retval = reset_sd(chip);
+					if (retval != STATUS_SUCCESS) {
+						sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
+						goto sd_execute_write_cmd_failed;
 					}
-
-					sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
 				}
+
+				sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
 			}
 		}
 	}
-- 
2.17.0

[PATCH 2/2] staging: rts5208: Simplify boolean expression to improve code style

[Tobias Nießen]

This bitwisen / boolean expression can be made more readable while
reducing the line lengths at the same time. This commit uses the
fact that

    a & (b | c) == (b | c)

evaluates to true if and only if

    (a & b) && (a & c)

is true. Since b and c are constants with relatively long names,
using the second form makes the code much more readable and shorter.

Signed-off-by: Tobias Nießen <tobias.niessen@stud.uni-hannover.de>
Signed-off-by: Sabrina Gaube <sabrina-gaube@web.de>
---
 drivers/staging/rts5208/xd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rts5208/xd.c b/drivers/staging/rts5208/xd.c
index c5ee04ecd1c9..f3dc96a4c59d 100644
--- a/drivers/staging/rts5208/xd.c
+++ b/drivers/staging/rts5208/xd.c
@@ -1155,10 +1155,10 @@ static int xd_copy_page(struct rtsx_chip *chip, u32 old_blk, u32 new_blk,
 					return STATUS_FAIL;
 				}
 
-				if (((reg & (XD_ECC1_ERROR | XD_ECC1_UNCORRECTABLE)) ==
-						(XD_ECC1_ERROR | XD_ECC1_UNCORRECTABLE)) ||
-					((reg & (XD_ECC2_ERROR | XD_ECC2_UNCORRECTABLE)) ==
-						(XD_ECC2_ERROR | XD_ECC2_UNCORRECTABLE))) {
+				if (((reg & XD_ECC1_ERROR) &&
+				     (reg & XD_ECC1_UNCORRECTABLE)) ||
+				    ((reg & XD_ECC2_ERROR) &&
+				     (reg & XD_ECC2_UNCORRECTABLE))) {
 					rtsx_write_register(chip,
 							    XD_PAGE_STATUS,
 							    0xFF,
-- 
2.17.0

Re: [PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Dan Carpenter]

Both these patches seem fine.

On Wed, Jun 26, 2019 at 04:28:56PM +0200, Tobias Nießen wrote:
> This commit uses the fact that
> 
>     if (a) {
>             if (b) {
>                     ...
>             }
>     }
> 
> can instead be written as
> 
>     if (a && b) {
>             ...
>     }
> 
> without any change in behavior, allowing to decrease the indentation
> of the contained code block and thus reducing the average line length.
> 
> Signed-off-by: Tobias Nießen <tobias.niessen@stud.uni-hannover.de>
> Signed-off-by: Sabrina Gaube <sabrina-gaube@web.de>

Signed-off-by is like signing a legal document that you didn't put any
of SCO's secret UNIXWARE source code into your patch or do other illegal
activities.  Everyone who handles a patch is supposed to Sign it.

It's weird to see Sabrina randomly signing your patches.  Probably there
is a more appropriate kind of tag to use as well or instead such as
Co-Developed-by, Reviewed-by or Suggested-by.

regards,
dan carpenter

Re: [PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Joe Perches]

On Wed, 2019-06-26 at 16:28 +0200, Tobias Nießen wrote:
> This commit uses the fact that
> 
>     if (a) {
>             if (b) {
>                     ...
>             }
>     }
> 
> can instead be written as
> 
>     if (a && b) {
>             ...
>     }
> 
> without any change in behavior, allowing to decrease the indentation
> of the contained code block and thus reducing the average line length.

unrelated and trivially:

> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
[]
> @@ -4507,20 +4507,19 @@ int sd_execute_write_data(struct scsi_cmnd *srb, struct rtsx_chip *chip)
[]
> +			if (sd_lock_state &&
> +			    (sd_card->sd_lock_status & SD_LOCK_1BIT_MODE)) {
> +				sd_card->sd_lock_status |= (
> +					SD_UNLOCK_POW_ON | SD_SDR_RST);

This could go on a single line like the &= ~() equivalent below.
It hardly matters if it's > 80 chars.

> +				if (CHK_SD(sd_card)) {
> +					retval = reset_sd(chip);
> +					if (retval != STATUS_SUCCESS) {
> +						sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
> +						goto sd_execute_write_cmd_failed;
>  					}
> -
> -					sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
>  				}
> +
> +				sd_card->sd_lock_status &= ~(SD_UNLOCK_POW_ON | SD_SDR_RST);
>  			}
>  		}
>  	}

Re: [PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Tobias Nießen]

Am 26.06.2019 um 16:56 schrieb Dan Carpenter:
> Both these patches seem fine.
> 
> On Wed, Jun 26, 2019 at 04:28:56PM +0200, Tobias Nießen wrote:
>> This commit uses the fact that
>>
>>     if (a) {
>>             if (b) {
>>                     ...
>>             }
>>     }
>>
>> can instead be written as
>>
>>     if (a && b) {
>>             ...
>>     }
>>
>> without any change in behavior, allowing to decrease the indentation
>> of the contained code block and thus reducing the average line length.
>>
>> Signed-off-by: Tobias Nießen <tobias.niessen@stud.uni-hannover.de>
>> Signed-off-by: Sabrina Gaube <sabrina-gaube@web.de>
> 
> Signed-off-by is like signing a legal document that you didn't put any
> of SCO's secret UNIXWARE source code into your patch or do other illegal
> activities.  Everyone who handles a patch is supposed to Sign it.
> 
> It's weird to see Sabrina randomly signing your patches.  Probably there
> is a more appropriate kind of tag to use as well or instead such as
> Co-Developed-by, Reviewed-by or Suggested-by.
> 
> regards,
> dan carpenter
> 

Thank you, Dan. This patch series is a mandatory part of a course Sabrina and I are taking at university. We were told to add Signed-off-by for both of us. I can add Co-Developed-by if that helps? Or should she just verify via email that she did indeed sign off?

Regards,
Tobias

Re: [PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Dan Carpenter]

On Sun, Jun 30, 2019 at 04:12:44PM +0200, Tobias Nießen wrote:
> Am 26.06.2019 um 16:56 schrieb Dan Carpenter:
> > Both these patches seem fine.
> > 
> > On Wed, Jun 26, 2019 at 04:28:56PM +0200, Tobias Nießen wrote:
> >> This commit uses the fact that
> >>
> >>     if (a) {
> >>             if (b) {
> >>                     ...
> >>             }
> >>     }
> >>
> >> can instead be written as
> >>
> >>     if (a && b) {
> >>             ...
> >>     }
> >>
> >> without any change in behavior, allowing to decrease the indentation
> >> of the contained code block and thus reducing the average line length.
> >>
> >> Signed-off-by: Tobias Nießen <tobias.niessen@stud.uni-hannover.de>
> >> Signed-off-by: Sabrina Gaube <sabrina-gaube@web.de>
> > 
> > Signed-off-by is like signing a legal document that you didn't put any
> > of SCO's secret UNIXWARE source code into your patch or do other illegal
> > activities.  Everyone who handles a patch is supposed to Sign it.
> > 
> > It's weird to see Sabrina randomly signing your patches.  Probably there
> > is a more appropriate kind of tag to use as well or instead such as
> > Co-Developed-by, Reviewed-by or Suggested-by.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Thank you, Dan. This patch series is a mandatory part of a course
> Sabrina and I are taking at university. We were told to add
> Signed-off-by for both of us. I can add Co-Developed-by if that helps?

Yes.  It does help.

regards,
dan carpenter