| Message ID | 20220112234741.1232858-1-colin.i.king@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: pata_ali: remove redundant return statement | expand |
On 1/13/22 08:47, Colin Ian King wrote: > A return statement is unnecessarily complicated, currently value > in variable mask is bitwise-masked and the variable is being > updated and then returned. Just updating the mask is all that is > required as the following statement is a return. > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > --- > drivers/ata/pata_ali.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c > index ab28a6707b94..1b90cda27246 100644 > --- a/drivers/ata/pata_ali.c > +++ b/drivers/ata/pata_ali.c > @@ -123,7 +123,7 @@ static unsigned long ali_20_filter(struct ata_device *adev, unsigned long mask) > mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA); > ata_id_c_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num)); > if (strstr(model_num, "WDC")) > - return mask &= ~ATA_MASK_UDMA; Yeah, not to mention that is really ugly as the return should really have been: return mask & ~ATA_MASK_UDMA; > + mask &= ~ATA_MASK_UDMA; > return mask; > } > Will queue this up.
On 12/01/2022 23:52, Damien Le Moal wrote: > On 1/13/22 08:47, Colin Ian King wrote: >> A return statement is unnecessarily complicated, currently value >> in variable mask is bitwise-masked and the variable is being >> updated and then returned. Just updating the mask is all that is >> required as the following statement is a return. >> >> Signed-off-by: Colin Ian King <colin.i.king@gmail.com> >> --- >> drivers/ata/pata_ali.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c >> index ab28a6707b94..1b90cda27246 100644 >> --- a/drivers/ata/pata_ali.c >> +++ b/drivers/ata/pata_ali.c >> @@ -123,7 +123,7 @@ static unsigned long ali_20_filter(struct ata_device *adev, unsigned long mask) >> mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA); >> ata_id_c_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num)); >> if (strstr(model_num, "WDC")) >> - return mask &= ~ATA_MASK_UDMA; > > Yeah, not to mention that is really ugly as the return should really > have been: > > return mask & ~ATA_MASK_UDMA; Yep, I did think of that as the original intention, but two return statements one after the other was equally as ugly. It was a 50/50 choice of what was perceived as the better fix :-) > >> + mask &= ~ATA_MASK_UDMA; >> return mask; >> } >> > > Will queue this up. > Thank you.
diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c index ab28a6707b94..1b90cda27246 100644 --- a/drivers/ata/pata_ali.c +++ b/drivers/ata/pata_ali.c @@ -123,7 +123,7 @@ static unsigned long ali_20_filter(struct ata_device *adev, unsigned long mask) mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA); ata_id_c_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num)); if (strstr(model_num, "WDC")) - return mask &= ~ATA_MASK_UDMA; + mask &= ~ATA_MASK_UDMA; return mask; }
A return statement is unnecessarily complicated, currently value in variable mask is bitwise-masked and the variable is being updated and then returned. Just updating the mask is all that is required as the following statement is a return. Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/ata/pata_ali.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)