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@xxxxxx>
>>> ---
>>> 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
