| Message ID | dbb4010a-e466-d7f5-e926-72577a96a22d@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] pata_artop: use *switch* in artop_init_one() | expand |
On 2/5/22 04:09, Sergey Shtylyov wrote: > This driver uses a string of the *if* statements where a *switch* statement > fits better... > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git' > repo. > > Changes in version 3: > - fixed up the patch subject. > > Changes in version 2: > - updated #define DRV_VERSION. > > drivers/ata/pata_artop.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > Index: libata/drivers/ata/pata_artop.c > =================================================================== > --- libata.orig/drivers/ata/pata_artop.c > +++ libata/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,22 @@ static int artop_init_one (struct pci_de > 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) > - ppi[0] = &info_628x_fast; > + break; > + case 2: /* 6280 or 6280 + fast */ > + { > + unsigned long io = pci_resource_start(pdev, 4); > + > + ppi[0] = &info_628x; > + if (inb(io) & 0x10) > + ppi[0] = &info_628x_fast; > + } Do you really need the local variable ? I would make this: if (pci_resource_start(pdev, 4) & 0x10) ppi[0] = &info_628x_fast; else ppi[0] = &info_628x; simpler :) > + break; > } > > BUG_ON(ppi[0] == NULL);
On 2/5/22 2:39 AM, Damien Le Moal wrote: >> This driver uses a string of the *if* statements where a *switch* statement >> fits better... >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> --- >> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git' >> repo. >> >> Changes in version 3: >> - fixed up the patch subject. >> >> Changes in version 2: >> - updated #define DRV_VERSION. >> >> drivers/ata/pata_artop.c | 24 +++++++++++++++--------- >> 1 file changed, 15 insertions(+), 9 deletions(-) >> >> Index: libata/drivers/ata/pata_artop.c >> =================================================================== >> --- libata.orig/drivers/ata/pata_artop.c >> +++ libata/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,22 @@ static int artop_init_one (struct pci_de >> 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) >> - ppi[0] = &info_628x_fast; >> + break; >> + case 2: /* 6280 or 6280 + fast */ >> + { >> + unsigned long io = pci_resource_start(pdev, 4); >> + >> + ppi[0] = &info_628x; >> + if (inb(io) & 0x10) >> + ppi[0] = &info_628x_fast; >> + } > > > Do you really need the local variable ? > I would make this: > > if (pci_resource_start(pdev, 4) & 0x10) > ppi[0] = &info_628x_fast; > else > ppi[0] = &info_628x; > > simpler :) Yeah! :-) But a matter of another patch, I think... [...] MBR, Sergey
Index: libata/drivers/ata/pata_artop.c =================================================================== --- libata.orig/drivers/ata/pata_artop.c +++ libata/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,22 @@ static int artop_init_one (struct pci_de 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) - ppi[0] = &info_628x_fast; + break; + case 2: /* 6280 or 6280 + fast */ + { + unsigned long io = pci_resource_start(pdev, 4); + + ppi[0] = &info_628x; + if (inb(io) & 0x10) + ppi[0] = &info_628x_fast; + } + break; } BUG_ON(ppi[0] == NULL);
This driver uses a string of the *if* statements where a *switch* statement fits better... Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git' repo. Changes in version 3: - fixed up the patch subject. Changes in version 2: - updated #define DRV_VERSION. drivers/ata/pata_artop.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)