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@xxxxxxxxxxxxx>
>>> ---
>>> 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
--
Damien Le Moal
Western Digital Research
