[PATCH v5 0/2] Use *switch*es instead of *if*s in the Artop PATA driver

[PATCH v5 0/2] Use *switch*es instead of *if*s in the Artop PATA driver

[Sergey Shtylyov]

Here are 3 patches against the 'for-next' branch of Damien Le Moal's
'libata.git' repo. The driver abuses the strings of the *if* statements
where a *switch* statements would fit better...


Sergey Shtylyov (2):
  ata: pata_artop: use *switch* in artop_init_one()
  ata: pata_artop: use *switch* in atp8xx_fixup()

 drivers/ata/pata_artop.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

-- 
2.26.3

[PATCH v5 1/2] ata: pata_artop: use *switch* in artop_init_one()

[Sergey Shtylyov]

This driver uses a string of the *if* statements in artop_init_one()
where the *switch* statement would fit better.  While fixing this,
refactor the 6280 code to e.g. avoid a compound statement inside
the *case* section...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 5:
- merged in the former patch #1;
- fixed up #define DRV_VERSION;
- added *else* branch to the *if* statement;
- updated and reformatted the patch description;
- added "ata: " prefix to the patch subject.

Changes in version 4:
- fixed up #define DRV_VERSION;
- expanded the patch description.

Changes in version 3:
- fixed up the patch subject.

Changes in version 2:
- updated #define DRV_VERSION.

 drivers/ata/pata_artop.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
index ad3c5808aaad..08e88403d0ab 100644
--- a/drivers/ata/pata_artop.c
+++ b/drivers/ata/pata_artop.c
@@ -28,7 +28,7 @@
 #include <linux/ata.h>
 
 #define DRV_NAME	"pata_artop"
-#define DRV_VERSION	"0.4.6"
+#define DRV_VERSION	"0.4.7"
 
 /*
  *	The ARTOP has 33 Mhz and "over clocked" timing tables. Until we
@@ -394,16 +394,19 @@ static int artop_init_one (struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	if (id->driver_data == 0)	/* 6210 variant */
+	switch (id->driver_data) {
+	case 0:		/* 6210 variant */
 		ppi[0] = &info_6210;
-	else if (id->driver_data == 1)	/* 6260 */
+		break;
+	case 1:		/* 6260 */
 		ppi[0] = &info_626x;
-	else if (id->driver_data == 2)	{ /* 6280 or 6280 + fast */
-		unsigned long io = pci_resource_start(pdev, 4);
-
-		ppi[0] = &info_628x;
-		if (inb(io) & 0x10)
+		break;
+	case 2:		/* 6280 or 6280 + fast */
+		if (inb(pci_resource_start(pdev, 4)) & 0x10)
 			ppi[0] = &info_628x_fast;
+		else
+			ppi[0] = &info_628x;
+		break;
 	}
 
 	BUG_ON(ppi[0] == NULL);
-- 
2.26.3

[PATCH v5 2/2] ata: pata_artop: use *switch* in atp8xx_fixup()

[Sergey Shtylyov]

This driver uses a string of the *if* statements in atp8xx_fixup() where
a *switch* statement would fit better...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---

Changes in version 5:
- fixed up #define DRV_VERSION;
- added "ata: " prefix to the patch subject.

Changes in version 4:
- new patch.

 drivers/ata/pata_artop.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
index 08e88403d0ab..20a8f31a3f57 100644
--- a/drivers/ata/pata_artop.c
+++ b/drivers/ata/pata_artop.c
@@ -28,7 +28,7 @@
 #include <linux/ata.h>
 
 #define DRV_NAME	"pata_artop"
-#define DRV_VERSION	"0.4.7"
+#define DRV_VERSION	"0.4.8"
 
 /*
  *	The ARTOP has 33 Mhz and "over clocked" timing tables. Until we
@@ -315,12 +315,15 @@ static struct ata_port_operations artop6260_ops = {
 
 static void atp8xx_fixup(struct pci_dev *pdev)
 {
-	if (pdev->device == 0x0005)
+	u8 reg;
+
+	switch (pdev->device) {
+	case 0x0005:
 		/* BIOS may have left us in UDMA, clear it before libata probe */
 		pci_write_config_byte(pdev, 0x54, 0);
-	else if (pdev->device == 0x0008 || pdev->device == 0x0009) {
-		u8 reg;
-
+		break;
+	case 0x0008:
+	case 0x0009:
 		/* Mac systems come up with some registers not set as we
 		   will need them */
 
@@ -338,6 +341,7 @@ static void atp8xx_fixup(struct pci_dev *pdev)
 		/* Enable IRQ output and burst mode */
 		pci_read_config_byte(pdev, 0x4a, &reg);
 		pci_write_config_byte(pdev, 0x4a, (reg & ~0x01) | 0x80);
+		break;
 	}
 }
 
-- 
2.26.3

Re: [PATCH v5 2/2] ata: pata_artop: use *switch* in atp8xx_fixup()

[Damien Le Moal]

On 2/10/22 05:25, Sergey Shtylyov wrote:
> This driver uses a string of the *if* statements in atp8xx_fixup() where
> a *switch* statement would fit better...
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 
> Changes in version 5:
> - fixed up #define DRV_VERSION;
> - added "ata: " prefix to the patch subject.
> 
> Changes in version 4:
> - new patch.
> 
>  drivers/ata/pata_artop.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
> index 08e88403d0ab..20a8f31a3f57 100644
> --- a/drivers/ata/pata_artop.c
> +++ b/drivers/ata/pata_artop.c
> @@ -28,7 +28,7 @@
>  #include <linux/ata.h>
>  
>  #define DRV_NAME	"pata_artop"
> -#define DRV_VERSION	"0.4.7"
> +#define DRV_VERSION	"0.4.8"
>  
>  /*
>   *	The ARTOP has 33 Mhz and "over clocked" timing tables. Until we
> @@ -315,12 +315,15 @@ static struct ata_port_operations artop6260_ops = {
>  
>  static void atp8xx_fixup(struct pci_dev *pdev)
>  {
> -	if (pdev->device == 0x0005)
> +	u8 reg;
> +
> +	switch (pdev->device) {
> +	case 0x0005:
>  		/* BIOS may have left us in UDMA, clear it before libata probe */
>  		pci_write_config_byte(pdev, 0x54, 0);
> -	else if (pdev->device == 0x0008 || pdev->device == 0x0009) {
> -		u8 reg;
> -
> +		break;
> +	case 0x0008:
> +	case 0x0009:
>  		/* Mac systems come up with some registers not set as we
>  		   will need them */
>  
> @@ -338,6 +341,7 @@ static void atp8xx_fixup(struct pci_dev *pdev)
>  		/* Enable IRQ output and burst mode */
>  		pci_read_config_byte(pdev, 0x4a, &reg);
>  		pci_write_config_byte(pdev, 0x4a, (reg & ~0x01) | 0x80);
> +		break;
>  	}

without a default case, isn't there a warning with make W=1 or make C=1
(sparse) ?


>  }
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 2/2] ata: pata_artop: use *switch* in atp8xx_fixup()

[Sergey Shtylyov]

On 2/10/22 2:29 AM, Damien Le Moal wrote:

>> This driver uses a string of the *if* statements in atp8xx_fixup() where
>> a *switch* statement would fit better...
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>>
>> Changes in version 5:
>> - fixed up #define DRV_VERSION;
>> - added "ata: " prefix to the patch subject.
>>
>> Changes in version 4:
>> - new patch.
>>
>>  drivers/ata/pata_artop.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
>> index 08e88403d0ab..20a8f31a3f57 100644
>> --- a/drivers/ata/pata_artop.c
>> +++ b/drivers/ata/pata_artop.c
>> @@ -28,7 +28,7 @@
>>  #include <linux/ata.h>
>>  
>>  #define DRV_NAME	"pata_artop"
>> -#define DRV_VERSION	"0.4.7"
>> +#define DRV_VERSION	"0.4.8"
>>  
>>  /*
>>   *	The ARTOP has 33 Mhz and "over clocked" timing tables. Until we
>> @@ -315,12 +315,15 @@ static struct ata_port_operations artop6260_ops = {
>>  
>>  static void atp8xx_fixup(struct pci_dev *pdev)
>>  {
>> -	if (pdev->device == 0x0005)
>> +	u8 reg;
>> +
>> +	switch (pdev->device) {
>> +	case 0x0005:
>>  		/* BIOS may have left us in UDMA, clear it before libata probe */
>>  		pci_write_config_byte(pdev, 0x54, 0);
>> -	else if (pdev->device == 0x0008 || pdev->device == 0x0009) {
>> -		u8 reg;
>> -
>> +		break;
>> +	case 0x0008:
>> +	case 0x0009:
>>  		/* Mac systems come up with some registers not set as we
>>  		   will need them */
>>  
>> @@ -338,6 +341,7 @@ static void atp8xx_fixup(struct pci_dev *pdev)
>>  		/* Enable IRQ output and burst mode */
>>  		pci_read_config_byte(pdev, 0x4a, &reg);
>>  		pci_write_config_byte(pdev, 0x4a, (reg & ~0x01) | 0x80);
>> +		break;
>>  	}
> 
> without a default case, isn't there a warning with make W=1

   No. I thought *default* makes sense only for *enum*s...

> or make C=1
> (sparse) ?

   Still no. :-P

[...]

MNR.Sergey

Re: [PATCH v5 0/2] Use *switch*es instead of *if*s in the Artop PATA driver

[Damien Le Moal]

On 2/10/22 05:25, Sergey Shtylyov wrote:
> Here are 3 patches against the 'for-next' branch of Damien Le Moal's
> 'libata.git' repo. The driver abuses the strings of the *if* statements
> where a *switch* statements would fit better...
> 
> 
> Sergey Shtylyov (2):
>   ata: pata_artop: use *switch* in artop_init_one()
>   ata: pata_artop: use *switch* in atp8xx_fixup()
> 
>  drivers/ata/pata_artop.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 

Applied to for-5.18. Thanks !


-- 
Damien Le Moal
Western Digital Research