Re: [PATCH] mmc: sdhci-pci: Use macros in pci_ids definition

[Adrian Hunter <adrian.hunter@intel.com>]

On 04/05/17 21:55, Matthias Kraemer wrote:
> This patch applies the PCI_DEVICE_ macros to specify the pci_ids instead
> of open-coding them within the sdhci-pci driver.
> 
> v2,v3:
> Suggested-by Adrian Hunter <adrian.hunter@intel.com>
> Instead of using the generic PCI_ marcos, introduce device specific macros
> to be able to shorten the table entries even further.
> 
> v4:
> Fix compile-time issue

Thanks, it looks better, although I still have some concerns - see below.

> 
> Signed-off-by: Matthias Kraemer <matthiasmartinsson@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci-core.c | 619 +++++---------------------------------
>  drivers/mmc/host/sdhci-pci.h      |  41 ++-
>  2 files changed, 111 insertions(+), 549 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 86560d5..d2b6115 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -992,554 +992,77 @@ static const struct sdhci_pci_fixes sdhci_amd = {
>  };
>  
>  static const struct pci_device_id pci_ids[] = {
> -	{
> -		.vendor		= PCI_VENDOR_ID_RICOH,
> -		.device		= PCI_DEVICE_ID_RICOH_R5C822,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.driver_data	= (kernel_ulong_t)&sdhci_ricoh,
> -	},
> -
> -	{
> -		.vendor         = PCI_VENDOR_ID_RICOH,
> -		.device         = 0x843,
> -		.subvendor      = PCI_ANY_ID,
> -		.subdevice      = PCI_ANY_ID,
> -		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> -	},
> -

<SNIP>

> -	{
> -		.vendor		= PCI_VENDOR_ID_AMD,
> -		.device		= PCI_ANY_ID,
> -		.class		= PCI_CLASS_SYSTEM_SDHCI << 8,
> -		.class_mask	= 0xFFFF00,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.driver_data	= (kernel_ulong_t)&sdhci_amd,
> -	},
> -	{	/* Generic SD host controller */
> -		PCI_DEVICE_CLASS((PCI_CLASS_SYSTEM_SDHCI << 8), 0xFFFF00)
> -	},
> -
> +	{SDHCI_PCI_DEVICE(RICOH, RICOH_R5C822, ricoh)},
> +	{SDHCI_PCI_DEVICE(RICOH, RICOH_0x843, ricoh_mmc)},

<SNIP>

> +	{SDHCI_PCI_DEVICE_CLASS(AMD, SYSTEM_SDHCI, PCI_CLASS_MASK, amd)},
> +	/* Generic SD host controller */
> +	{PCI_DEVICE_CLASS(SYSTEM_SDHCI, PCI_CLASS_MASK)},
>  	{ /* end: all zeroes */ },
>  };
>  
> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> index 36f7434..b3485cc 100644
> --- a/drivers/mmc/host/sdhci-pci.h
> +++ b/drivers/mmc/host/sdhci-pci.h
> @@ -2,7 +2,7 @@
>  #define __SDHCI_PCI_H
>  
>  /*
> - * PCI device IDs
> + * PCI device IDs, sub IDs
>   */
>  
>  #define PCI_DEVICE_ID_INTEL_PCH_SDIO0	0x8809
> @@ -38,6 +38,45 @@
>  #define PCI_DEVICE_ID_INTEL_GLK_EMMC	0x31cc
>  #define PCI_DEVICE_ID_INTEL_GLK_SDIO	0x31d0
>  
> +#define PCI_DEVICE_ID_RICOH_0x843	0x843
> +#define PCI_DEVICE_ID_RICOH_0xe822	0xe822
> +#define PCI_DEVICE_ID_RICOH_0xe823	0xe823
> +#define PCI_DEVICE_ID_SYSKONNECT_0x8000	0x8000
> +#define PCI_DEVICE_ID_VIA_0x95d0	0x95d0
> +#define PCI_DEVICE_ID_REALTEK_0x5250	0x5250
> +
> +#define PCI_DEVICE_ID_INTEL_SUB_0x7884	0x7884
> +
> +/*
> + * PCI device class and mask
> + */
> +
> +#define SYSTEM_SDHCI			(PCI_CLASS_SYSTEM_SDHCI << 8)
> +#define PCI_CLASS_MASK			0xFFFF00
> +
> +/*
> + * Macros for PCI device-description
> + */
> +
> +#define _PCI_VEND(vend) PCI_VENDOR_ID_##vend
> +#define _PCI_DEV(dev) PCI_DEVICE_ID_##dev

Without exception, the device is prefixed by the vendor, so this should be:

#define _PCI_DEV(vend, dev) PCI_DEVICE_ID_##vend##_##dev

> +
> +#define SDHCI_PCI_DEVICE(vend, dev, cfg) \
> +	.vendor = _PCI_VEND(vend), .device = _PCI_DEV(dev), \
> +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, \
> +	.driver_data = (kernel_ulong_t)&(sdhci_##cfg)

Still don't understand why you don't put the {} brackets into the macro?

> +
> +#define SDHCI_PCI_DEVICE_SUB(vend, dev, subvend, subdev, cfg) \
> +	.vendor = _PCI_VEND(vend), .device = _PCI_DEV(dev), \
> +	.subvendor = _PCI_VEND(subvend), .subdevice = _PCI_DEV(subdev), \
> +	.driver_data = (kernel_ulong_t)&(sdhci_##cfg)
> +

Still don't understand why you don't put the {} brackets into the macro?

> +#define SDHCI_PCI_DEVICE_CLASS(vend, cl, cl_msk, cfg) \
> +	.vendor = _PCI_VEND(vend), .device = PCI_ANY_ID, \
> +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, \
> +	.class = (cl), .class_mask = (cl_msk), \
> +	.driver_data = (kernel_ulong_t)&(sdhci_##cfg)

Still don't understand why you don't put the {} brackets into the macro?

> +
>  /*
>   * PCI registers
>   */
>