| Message ID | 20211229161119.1006-2-pmenzel@molgen.mpg.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] PCI: Add device code for AMD FCH SATA Controller in AHCI mode | expand |
On 12/30/21 01:11, Paul Menzel wrote: > Use the defined macro from `include/linux/pci_ids.h`. > > Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> > --- > drivers/ata/ahci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 1e1167e725a4..6a2432e4adda 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { > .class_mask = 0xffffff, > board_ahci_al }, > /* AMD */ > - { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */ > + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, That breaks the style of the declarations here... And since PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE is used only in drivers/pci/quirks.c, I do not really see the point of this change. > { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */ > { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */ > /* AMD is using RAID class only for ahci controllers */
Dear Damien, Am 30.12.21 um 03:13 schrieb Damien Le Moal: > On 12/30/21 01:11, Paul Menzel wrote: >> Use the defined macro from `include/linux/pci_ids.h`. >> >> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> >> --- >> drivers/ata/ahci.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >> index 1e1167e725a4..6a2432e4adda 100644 >> --- a/drivers/ata/ahci.c >> +++ b/drivers/ata/ahci.c >> @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { >> .class_mask = 0xffffff, >> board_ahci_al }, >> /* AMD */ >> - { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */ >> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, > > That breaks the style of the declarations here... And since > PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE is used only in drivers/pci/quirks.c, > I do not really see the point of this change. 1. It makes the comment superfluous. 2. `git grep 0x7800` in the Linux source returns 1034 hits, so getting the macro name from the header `pci_ids.h` and then grepping for that is more straight forward. I agree, that it breaks the style, and, if you agree, I could try to fix up the whole file. >> { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */ >> { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */ >> /* AMD is using RAID class only for ahci controllers */ Kind regards, Paul
On 12/29/21 5:11 PM, Paul Menzel wrote: > Use the defined macro from `include/linux/pci_ids.h`. > > Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> > --- > drivers/ata/ahci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 1e1167e725a4..6a2432e4adda 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { > .class_mask = 0xffffff, > board_ahci_al }, > /* AMD */ > - { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */ > + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, > { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */ > { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */ > /* AMD is using RAID class only for ahci controllers */ > Weelll ... there are defines for AMD Hudson-2 and similar in the pci_ids file, yet these definitions are not used here. I'd vote keeping the style for all entries, ie either convert all entries here to use #defines, or stay with the numeral. But we shouldn't mix them. Cheers, Hannes
On 12/30/21 20:01, Paul Menzel wrote: > Dear Damien, > > > Am 30.12.21 um 03:13 schrieb Damien Le Moal: >> On 12/30/21 01:11, Paul Menzel wrote: >>> Use the defined macro from `include/linux/pci_ids.h`. >>> >>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> >>> --- >>> drivers/ata/ahci.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >>> index 1e1167e725a4..6a2432e4adda 100644 >>> --- a/drivers/ata/ahci.c >>> +++ b/drivers/ata/ahci.c >>> @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { >>> .class_mask = 0xffffff, >>> board_ahci_al }, >>> /* AMD */ >>> - { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */ >>> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, > >> That breaks the style of the declarations here... And since >> PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE is used only in drivers/pci/quirks.c, >> I do not really see the point of this change. > > 1. It makes the comment superfluous. > 2. `git grep 0x7800` in the Linux source returns 1034 hits, so getting > the macro name from the header `pci_ids.h` and then grepping for that is > more straight forward. > > I agree, that it breaks the style, and, if you agree, I could try to fix > up the whole file. Arg, please no ! That would need defining a ton of PCI IDs for using them only here. Let's not do that. See Sergey and Hannes comment too. Let's keep the current style. Note that with this change, AMD_HUDSON2_SATA_IDE PCI ID would be used both here and in drivers/pci/quirks.c, so 2 places. But given that the ID here is only for the table entry and used nowhere else, using the numeral is better to preserve the style. So let's drop this patch. > >>> { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */ >>> { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */ >>> /* AMD is using RAID class only for ahci controllers */ > > > Kind regards, > > Paul
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 1e1167e725a4..6a2432e4adda 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { .class_mask = 0xffffff, board_ahci_al }, /* AMD */ - { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */ + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */ { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */ /* AMD is using RAID class only for ahci controllers */
Use the defined macro from `include/linux/pci_ids.h`. Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> --- drivers/ata/ahci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)