Re: [PATCH] ata: Consistently define pci_device_ids using named initializers

[Damien Le Moal]

On 2026/05/20 11:12, Uwe Kleine-König (The Capable Hub) wrote:
> On Mon, May 04, 2026 at 10:09:53AM +0200, Damien Le Moal wrote:
>> On 4/30/26 19:06, Uwe Kleine-König (The Capable Hub) wrote:
>>> ... and PCI device helpers.
>>>
>>> The .driver_data member in the various struct pci_device_id arrays were
>>> initialized mostly by list expressions. This isn't easily readable if
>>> you're not into PCI. Using named initializers is more explicit and thus
>>> easier to parse.
>>>
>>> Also use PCI_DEVICE to conveniently assign .vendor, .device, .subvendor
>>> and .subdevice where appropriate.
>>>
>>> The secret plan is to make struct pci_device_id::driver_data an
>>> anonymous union (similar to
>>> https://lore.kernel.org/all/cover.1776579304.git.u.kleine-koenig@xxxxxxxxxxxx/)
>>> and that requires named initializers. But it's also a nice cleanup on
>>> its own.
>>>
>>> This change doesn't introduce changes to the compiled pci_device_id
>>> arrays. Tested on x86 and arm64.
>>>
>>> Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@xxxxxxxxxxxx>
>>
>> This looks like a nice cleanup to me. A couple of nits below.
>> With that, feel free to add:
>>
>> Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
>>
>> [...]
>>
>>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>>> index 495fa096dd65..fac1266f7fa6 100644
>>> --- a/drivers/ata/ata_piix.c
>>> +++ b/drivers/ata/ata_piix.c
>>> @@ -154,184 +154,186 @@ static unsigned int in_module_init = 1;
>>>  
>>>  static const struct pci_device_id piix_pci_tbl[] = {
>>>  	/* Intel PIIX3 for the 430HX etc */
>>> -	{ 0x8086, 0x7010, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_mwdma },
>>> +	{ PCI_DEVICE(0x8086, 0x7010), .driver_data = piix_pata_mwdma },
>>
>> Please split the line before the .driver_data like for all the other changes.
>> That will make things consistent...
> 
> I would go for
> 
> 	{
> 		PCI_DEVICE(0x8086, 0x7010),
> 		.driver_data = piix_pata_mwdma,
> 	}, {
> 		...

Yes, I think this is nice.

> 
> then. Or would you prefer something more compact like:
> 
> 	{ PCI_DEVICE(0x8086, 0x7010),
> 	  .driver_data = piix_pata_mwdma },
> 
> ?
> 
> Best regards
> Uwe


-- 
Damien Le Moal
Western Digital Research