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

[PATCH v4 0/3] 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 (3):
  pata_artop: eliminate local variable in artop_init_one()
  pata_artop: use *switch* in artop_init_one()
  pata_artop: use *switch* in atp8xx_fixup()

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

-- 
2.26.3

[PATCH v4 1/3] pata_artop: eliminate local variable in artop_init_one()

[Sergey Shtylyov]

The 'io' local variable isn't actually necessary in the inner block of
artop_init_one() -- removing it will allow to simplify a follow-up patch...

Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 4:
- new patch.

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

diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
index ad3c5808aaad..b734cafb8783 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
@@ -398,11 +398,9 @@ static int artop_init_one (struct pci_dev *pdev, const struct pci_device_id *id)
 		ppi[0] = &info_6210;
 	else if (id->driver_data == 1)	/* 6260 */
 		ppi[0] = &info_626x;
-	else if (id->driver_data == 2)	{ /* 6280 or 6280 + fast */
-		unsigned long io = pci_resource_start(pdev, 4);
-
+	else if (id->driver_data == 2) { /* 6280 or 6280 + fast */
 		ppi[0] = &info_628x;
-		if (inb(io) & 0x10)
+		if (inb(pci_resource_start(pdev, 4)) & 0x10)
 			ppi[0] = &info_628x_fast;
 	}
 
-- 
2.26.3

[PATCH v4 2/3] 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...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
index b734cafb8783..d8c388da0c70 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
@@ -394,14 +394,18 @@ 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 */
+		break;
+	case 2:		/* 6280 or 6280 + fast */
 		ppi[0] = &info_628x;
 		if (inb(pci_resource_start(pdev, 4)) & 0x10)
 			ppi[0] = &info_628x_fast;
+		break;
 	}
 
 	BUG_ON(ppi[0] == NULL);
-- 
2.26.3

[PATCH v4 3/3] 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 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 d8c388da0c70..0aef9941b5b1 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.8"
+#define DRV_VERSION	"0.4.9"
 
 /*
  *	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 v4 2/3] pata_artop: use *switch* in artop_init_one()

[Damien Le Moal]

On 2/6/22 05:34, Sergey Shtylyov wrote:
> This driver uses a string of the *if* statements in artop_init_one() where
> the *switch* statement would fit better...
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 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 | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
> index b734cafb8783..d8c388da0c70 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
> @@ -394,14 +394,18 @@ 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 */
> +		break;
> +	case 2:		/* 6280 or 6280 + fast */
>  		ppi[0] = &info_628x;
>  		if (inb(pci_resource_start(pdev, 4)) & 0x10)
>  			ppi[0] = &info_628x_fast;

Why not use "if () else" here ? And I do not see the point of patch 1.
Since this patch is rewriting this hunk anyway, let's squash patch 1
into this one.

> +		break;
>  	}
>  
>  	BUG_ON(ppi[0] == NULL);


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2/6/22 05:34, 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 (3):
>   pata_artop: eliminate local variable in artop_init_one()
>   pata_artop: use *switch* in artop_init_one()
>   pata_artop: use *switch* in atp8xx_fixup()

Please change the patch title prefix to "ata: <drv>: xxx".
So "ata: pata_artop: xxx" in this case.

> 
>  drivers/ata/pata_artop.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 2/3] pata_artop: use *switch* in artop_init_one()

[Sergey Shtylyov]

On 2/6/22 4:55 AM, Damien Le Moal wrote:

>> This driver uses a string of the *if* statements in artop_init_one() where
>> the *switch* statement would fit better...
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> 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 | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
>> index b734cafb8783..d8c388da0c70 100644
>> --- a/drivers/ata/pata_artop.c
>> +++ b/drivers/ata/pata_artop.c
[...]
>> @@ -394,14 +394,18 @@ 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 */
>> +		break;
>> +	case 2:		/* 6280 or 6280 + fast */
>>  		ppi[0] = &info_628x;
>>  		if (inb(pci_resource_start(pdev, 4)) & 0x10)
>>  			ppi[0] = &info_628x_fast;
> 
> Why not use "if () else" here ?

    Because I'm following the basic rule: one thing per patch. :-)

> And I do not see the point of patch 1.

   Again, one thing per patch. It was a preparatory patch.

> Since this patch is rewriting this hunk anyway, let's squash patch 1
> into this one.

   I'd really prefer not doing thos...

[...]

MBR, Sergey

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

[Sergey Shtylyov]

On 2/6/22 5:01 AM, Damien Le Moal 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 (3):
>>   pata_artop: eliminate local variable in artop_init_one()
>>   pata_artop: use *switch* in artop_init_one()
>>   pata_artop: use *switch* in atp8xx_fixup()
> 
> Please change the patch title prefix to "ata: <drv>: xxx".
> So "ata: pata_artop: xxx" in this case.

   As you wish, of/c.
   But I do think "pata_" speaks for itself and saves the valuable patch subject space...

[...]

MBR, Sergey

Re: [PATCH v4 2/3] pata_artop: use *switch* in artop_init_one()

[Damien Le Moal]

On 2/6/22 19:24, Sergey Shtylyov wrote:
> On 2/6/22 4:55 AM, Damien Le Moal wrote:
> 
>>> This driver uses a string of the *if* statements in artop_init_one() where
>>> the *switch* statement would fit better...
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> 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 | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
>>> index b734cafb8783..d8c388da0c70 100644
>>> --- a/drivers/ata/pata_artop.c
>>> +++ b/drivers/ata/pata_artop.c
> [...]
>>> @@ -394,14 +394,18 @@ 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 */
>>> +		break;
>>> +	case 2:		/* 6280 or 6280 + fast */
>>>  		ppi[0] = &info_628x;
>>>  		if (inb(pci_resource_start(pdev, 4)) & 0x10)
>>>  			ppi[0] = &info_628x_fast;
>>
>> Why not use "if () else" here ?
> 
>     Because I'm following the basic rule: one thing per patch. :-)
> 
>> And I do not see the point of patch 1.
> 
>    Again, one thing per patch. It was a preparatory patch.
> 
>> Since this patch is rewriting this hunk anyway, let's squash patch 1
>> into this one.
> 
>    I'd really prefer not doing thos...

I agree in general. But in this case, you are touching that code hunk to
clean it up using a switch/case. You may as well rewrite the case code
too in one go. There are no functional changes, so all good to me. The
patch is for that code hunk, one thing :)

> 
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research