Re: [PATCH] serial: jsm: Use named initializers for struct pci_device_id

[Jiri Slaby <jirislaby@kernel.org>]

On 05. 05. 26, 9:30, Uwe Kleine-König (The Capable Hub) wrote:
> Initializing structures using list initializers is harder to read than
> using named initializers. Seeing the member name is more ideomatic and
> easier to understand.
> 
> Use named initializers for the driver's pci_device_id array. While at it
> also drop an explicit zero in the terminating array entry.
> 
> There are no changes to the compiled result of the array; verified with
> builds for x86 and arm64.
> 
> Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
> ---
> Hello,
> 
> this is a preparing change for making struct pci_device_id::driver_data an
> anonymous union (similar to
> https://lore.kernel.org/all/cover.1776579304.git.u.kleine-koenig@baylibre.com/).
> This requires named initializers for .driver_data. But even without that
> this is a nice cleanup making the array better readable.
> 
> Having said that I didn't find where .driver_data is used, so unless I
> missed something the assignments can just go away???
> 
> For earlier patches of this type against other drivers I got the
> feedback to use PCI_DEVICE_DATA() but I don't like that as it mixes
> hardware properties (.vendor and .device) with driver specific software
> properties (.driver_data) and thus I prefer the explicit listing
> (involving more repetition) over the shorter variant (being more opaque
> and harder to read).
> 
> Best regards
> Uwe
> 
>   drivers/tty/serial/jsm/jsm_driver.c | 75 +++++++++++++++++++++--------
>   1 file changed, 56 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/serial/jsm/jsm_driver.c b/drivers/tty/serial/jsm/jsm_driver.c
> index 4b73e51f83fb..422cac8d8d71 100644
> --- a/drivers/tty/serial/jsm/jsm_driver.c
> +++ b/drivers/tty/serial/jsm/jsm_driver.c
> @@ -296,25 +296,62 @@ static void jsm_remove_one(struct pci_dev *pdev)
>   }
>   
>   static const struct pci_device_id jsm_pci_tbl[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_2DB9), 0, 0, 0 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_2DB9PRI), 0, 0, 1 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_2RJ45), 0, 0, 2 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_2RJ45PRI), 0, 0, 3 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_4_IBM), 0, 0, 4 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_DIGI_NEO_8), 0, 0, 5 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_4), 0, 0, 6 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_1_422), 0, 0, 7 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_1_422_485), 0, 0, 8 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_2_422_485), 0, 0, 9 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_8), 0, 0, 10 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_4), 0, 0, 11 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_4RJ45), 0, 0, 12 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_8RJ45), 0, 0, 13 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_CLASSIC_4), 0, 0, 14 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_CLASSIC_4_422), 0, 0, 15 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_CLASSIC_8), 0, 0, 16 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_CLASSIC_8_422), 0, 0, 17 },
> -	{ 0, }
> +	{
> +		PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_ID_NEO_2DB9),
> +		.driver_data = 0,
> +	}, {

Sorry, but this is definitely NOT "making the array better readable". 
Esp. due to the split to multiple lines. PCI_DEVICE_DATA() would be far 
better IMO. Or PCI_VDEVICE() + .driver_data on a single line...

Anyway, you're right, pci_device_id is not currently used at all. 
Instead, there are big switches in the probe doing the casing. This 
should be rewritten to actually use driver_data and drop the whole 
switch-case stuff from probe.

So you can likely drop driver_data completely for now. It won't/can't be 
used in the current form anyway.

thanks,
-- 
js
suse labs