[PATCH 1/6] include: linux: pci.h: introduce pci_get_device_by_id()

[PATCH 1/6] include: linux: pci.h: introduce pci_get_device_by_id()

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

Introduce a little helper for getting pci device via struct pci_device_id,
helpful for reducing a little bit of boilerplate.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 include/linux/pci.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index eb9f371aa77c..f6650b13724a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1050,6 +1050,21 @@ struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
 
 struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device,
 			       struct pci_dev *from);
+
+/**
+ * pci_get_device_by_id() - get pci dev by struct pci_device_id
+ * @id: pci_device_id struct
+ *
+ * Retrieve PCI device by struct pci_device_id struct
+ *
+ * @return: pointer to struct pci_dev if sucessful, NULL otherwise
+ */
+static inline struct pci_dev *pci_get_device_by_id(
+	const struct pci_device_id *id)
+{
+	return pci_get_device(id->vendor, id->device, NULL);
+}
+
 struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
 			       unsigned int ss_vendor, unsigned int ss_device,
 			       struct pci_dev *from);
-- 
2.11.0

[PATCH 2/6] mtd: maps: esb2rom: use pci_get_device_by_id()

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

Use the new pci_get_device_by_id() helper in order to reduce
a bit of boilerplate.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/mtd/maps/esb2rom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/maps/esb2rom.c b/drivers/mtd/maps/esb2rom.c
index 5c27c6994896..0c638ac6ad82 100644
--- a/drivers/mtd/maps/esb2rom.c
+++ b/drivers/mtd/maps/esb2rom.c
@@ -421,7 +421,7 @@ static int __init init_esb2rom(void)
 	pdev = NULL;
 	for (id = esb2rom_pci_tbl; id->vendor; id++) {
 		printk(KERN_DEBUG "device id = %x\n", id->device);
-		pdev = pci_get_device(id->vendor, id->device, NULL);
+		pdev = pci_get_device_by_id(id);
 		if (pdev) {
 			printk(KERN_DEBUG "matched device = %x\n", id->device);
 			break;
-- 
2.11.0

[PATCH 3/6] mtd: maps: amd76xrom: use pci_get_device_by_id()

[Enrico Weigelt, metux IT consult]

Use the new pci_get_device_by_id() helper in order to reduce
a bit of boilerplate.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/mtd/maps/amd76xrom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/maps/amd76xrom.c b/drivers/mtd/maps/amd76xrom.c
index 462fadb56bdb..07a402dbdf44 100644
--- a/drivers/mtd/maps/amd76xrom.c
+++ b/drivers/mtd/maps/amd76xrom.c
@@ -323,7 +323,7 @@ static int __init init_amd76xrom(void)
 	const struct pci_device_id *id;
 	pdev = NULL;
 	for(id = amd76xrom_pci_tbl; id->vendor; id++) {
-		pdev = pci_get_device(id->vendor, id->device, NULL);
+		pdev = pci_get_device_by_id(id);
 		if (pdev) {
 			break;
 		}
-- 
2.11.0

[PATCH 4/6] edac: i82443bxgx_edac: use pci_get_device_by_id()

[Enrico Weigelt, metux IT consult]

Use the new pci_get_device_by_id() helper in order to reduce
a bit of boilerplate.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/edac/i82443bxgx_edac.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/i82443bxgx_edac.c b/drivers/edac/i82443bxgx_edac.c
index a2ca929e2168..c01f51a4848c 100644
--- a/drivers/edac/i82443bxgx_edac.c
+++ b/drivers/edac/i82443bxgx_edac.c
@@ -407,15 +407,13 @@ static int __init i82443bxgx_edacmc_init(void)
 		goto fail0;
 
 	if (mci_pdev == NULL) {
-		const struct pci_device_id *id = &i82443bxgx_pci_tbl[0];
 		int i = 0;
 		i82443bxgx_registered = 0;
 
-		while (mci_pdev == NULL && id->vendor != 0) {
-			mci_pdev = pci_get_device(id->vendor,
-					id->device, NULL);
+		while (mci_pdev == NULL && i82443bxgx_pci_tbl[i].vendor) {
+			mci_pdev = pci_get_device_by_id(
+				&i82443bxgx_pci_tbl[i]);
 			i++;
-			id = &i82443bxgx_pci_tbl[i];
 		}
 		if (!mci_pdev) {
 			edac_dbg(0, "i82443bxgx pci_get_device fail\n");
-- 
2.11.0

[PATCH 5/6] char: hw_random: intel-rng: use pci_get_device_by_id()

[Enrico Weigelt, metux IT consult]

Use the new pci_get_device_by_id() helper in order to reduce
a bit of boilerplate.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/char/hw_random/intel-rng.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/intel-rng.c b/drivers/char/hw_random/intel-rng.c
index 290c880266bf..6b8d37a81166 100644
--- a/drivers/char/hw_random/intel-rng.c
+++ b/drivers/char/hw_random/intel-rng.c
@@ -335,8 +335,7 @@ static int __init mod_init(void)
 	struct intel_rng_hw *intel_rng_hw;
 
 	for (i = 0; !dev && pci_tbl[i].vendor; ++i)
-		dev = pci_get_device(pci_tbl[i].vendor, pci_tbl[i].device,
-				     NULL);
+		dev = pci_get_device_by_id(&pci_tbl[i]);
 
 	if (!dev)
 		goto out; /* Device not found. */
-- 
2.11.0

[PATCH 6/6] (v3) drivers: hwmon: i5k_amb: simplify probing / device identification

[Enrico Weigelt, metux IT consult]

Simpilify the probing by putting all chip-specific data directly
into the pci match table, removing the redundant chipset_ids table.

Changes v3:
    * use pci_get_device_by_id() introduces by a previous patch
      of this queue

Changes v2:
    * use PCI_DEVICE_DATA() macro in the pci match table
    * directly pass the pci device id to i5k_channel_probe(),
      instead of computing it internally by extra offset parameter

Submitted: 2019-06-06
Signed-off-by: Enrico Weigelt <info@metux.net>
---
 drivers/hwmon/i5k_amb.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/i5k_amb.c b/drivers/hwmon/i5k_amb.c
index b09c39abd3a8..cb85607d104f 100644
--- a/drivers/hwmon/i5k_amb.c
+++ b/drivers/hwmon/i5k_amb.c
@@ -414,16 +414,14 @@ static int i5k_amb_add(void)
 }
 
 static int i5k_find_amb_registers(struct i5k_amb_data *data,
-					    unsigned long devid)
+				  const struct pci_device_id *devid)
 {
 	struct pci_dev *pcidev;
 	u32 val32;
 	int res = -ENODEV;
 
 	/* Find AMB register memory space */
-	pcidev = pci_get_device(PCI_VENDOR_ID_INTEL,
-				devid,
-				NULL);
+	pcidev = pci_get_device_by_id(devid);
 	if (!pcidev)
 		return -ENODEV;
 
@@ -447,14 +445,15 @@ static int i5k_find_amb_registers(struct i5k_amb_data *data,
 	return res;
 }
 
-static int i5k_channel_probe(u16 *amb_present, unsigned long dev_id)
+static int i5k_channel_probe(u16 *amb_present, unsigned int vendor,
+			     unsigned int device)
 {
 	struct pci_dev *pcidev;
 	u16 val16;
 	int res = -ENODEV;
 
 	/* Copy the DIMM presence map for these two channels */
-	pcidev = pci_get_device(PCI_VENDOR_ID_INTEL, dev_id, NULL);
+	pcidev = pci_get_device(vendor, device, NULL);
 	if (!pcidev)
 		return -ENODEV;
 
@@ -473,23 +472,12 @@ static int i5k_channel_probe(u16 *amb_present, unsigned long dev_id)
 	return res;
 }
 
-static struct {
-	unsigned long err;
-	unsigned long fbd0;
-} chipset_ids[]  = {
-	{ PCI_DEVICE_ID_INTEL_5000_ERR, PCI_DEVICE_ID_INTEL_5000_FBD0 },
-	{ PCI_DEVICE_ID_INTEL_5400_ERR, PCI_DEVICE_ID_INTEL_5400_FBD0 },
-	{ 0, 0 }
-};
-
-#ifdef MODULE
 static const struct pci_device_id i5k_amb_ids[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5000_ERR) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5400_ERR) },
+	{ PCI_DEVICE_DATA(INTEL, 5000_ERR, PCI_DEVICE_ID_INTEL_5000_FBD0) },
+	{ PCI_DEVICE_DATA(INTEL, 5400_ERR, PCI_DEVICE_ID_INTEL_5400_FBD0) },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, i5k_amb_ids);
-#endif
 
 static int i5k_amb_probe(struct platform_device *pdev)
 {
@@ -504,22 +492,26 @@ static int i5k_amb_probe(struct platform_device *pdev)
 	/* Figure out where the AMB registers live */
 	i = 0;
 	do {
-		res = i5k_find_amb_registers(data, chipset_ids[i].err);
+		res = i5k_find_amb_registers(data, &i5k_amb_ids[i]);
 		if (res == 0)
 			break;
 		i++;
-	} while (chipset_ids[i].err);
+	} while (i5k_amb_ids[i].device);
 
 	if (res)
 		goto err;
 
 	/* Copy the DIMM presence map for the first two channels */
-	res = i5k_channel_probe(&data->amb_present[0], chipset_ids[i].fbd0);
+	res = i5k_channel_probe(&data->amb_present[0],
+				i5k_amb_ids[i].vendor,
+				i5k_amb_ids[i].driver_data);
 	if (res)
 		goto err;
 
 	/* Copy the DIMM presence map for the optional second two channels */
-	i5k_channel_probe(&data->amb_present[2], chipset_ids[i].fbd0 + 1);
+	i5k_channel_probe(&data->amb_present[2],
+			  i5k_amb_ids[i].vendor,
+			  i5k_amb_ids[i].driver_data+1);
 
 	/* Set up resource regions */
 	reso = request_mem_region(data->amb_base, data->amb_len, DRVNAME);
-- 
2.11.0

Re: [PATCH 4/6] edac: i82443bxgx_edac: use pci_get_device_by_id()

[Robert Richter]

On 28.11.19 13:54:04, Enrico Weigelt, metux IT consult wrote:
> Use the new pci_get_device_by_id() helper in order to reduce
> a bit of boilerplate.

> -		while (mci_pdev == NULL && id->vendor != 0) {
> -			mci_pdev = pci_get_device(id->vendor,
> -					id->device, NULL);
> +		while (mci_pdev == NULL && i82443bxgx_pci_tbl[i].vendor) {
> +			mci_pdev = pci_get_device_by_id(
> +				&i82443bxgx_pci_tbl[i]);

I don't see how this is less boilerplate. Since this differs from the
typical pattern for this it is less readable now.

-Robert

Re: [PATCH 6/6] (v3) drivers: hwmon: i5k_amb: simplify probing / device identification

[Guenter Roeck]

On 11/28/19 4:54 AM, Enrico Weigelt, metux IT consult wrote:
> Simpilify the probing by putting all chip-specific data directly
> into the pci match table, removing the redundant chipset_ids table.
> 
> Changes v3:
>      * use pci_get_device_by_id() introduces by a previous patch
>        of this queue
> 
> Changes v2:
>      * use PCI_DEVICE_DATA() macro in the pci match table
>      * directly pass the pci device id to i5k_channel_probe(),
>        instead of computing it internally by extra offset parameter
> 
> Submitted: 2019-06-06
> Signed-off-by: Enrico Weigelt <info@metux.net>

I don't immediately see how this is better. I am not even sure if it is correct.

Guenter

> ---
>   drivers/hwmon/i5k_amb.c | 38 +++++++++++++++-----------------------
>   1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwmon/i5k_amb.c b/drivers/hwmon/i5k_amb.c
> index b09c39abd3a8..cb85607d104f 100644
> --- a/drivers/hwmon/i5k_amb.c
> +++ b/drivers/hwmon/i5k_amb.c
> @@ -414,16 +414,14 @@ static int i5k_amb_add(void)
>   }
>   
>   static int i5k_find_amb_registers(struct i5k_amb_data *data,
> -					    unsigned long devid)
> +				  const struct pci_device_id *devid)
>   {
>   	struct pci_dev *pcidev;
>   	u32 val32;
>   	int res = -ENODEV;
>   
>   	/* Find AMB register memory space */
> -	pcidev = pci_get_device(PCI_VENDOR_ID_INTEL,
> -				devid,
> -				NULL);
> +	pcidev = pci_get_device_by_id(devid);
>   	if (!pcidev)
>   		return -ENODEV;
>   
> @@ -447,14 +445,15 @@ static int i5k_find_amb_registers(struct i5k_amb_data *data,
>   	return res;
>   }
>   
> -static int i5k_channel_probe(u16 *amb_present, unsigned long dev_id)
> +static int i5k_channel_probe(u16 *amb_present, unsigned int vendor,
> +			     unsigned int device)
>   {
>   	struct pci_dev *pcidev;
>   	u16 val16;
>   	int res = -ENODEV;
>   
>   	/* Copy the DIMM presence map for these two channels */
> -	pcidev = pci_get_device(PCI_VENDOR_ID_INTEL, dev_id, NULL);
> +	pcidev = pci_get_device(vendor, device, NULL);
>   	if (!pcidev)
>   		return -ENODEV;
>   
> @@ -473,23 +472,12 @@ static int i5k_channel_probe(u16 *amb_present, unsigned long dev_id)
>   	return res;
>   }
>   
> -static struct {
> -	unsigned long err;
> -	unsigned long fbd0;
> -} chipset_ids[]  = {
> -	{ PCI_DEVICE_ID_INTEL_5000_ERR, PCI_DEVICE_ID_INTEL_5000_FBD0 },
> -	{ PCI_DEVICE_ID_INTEL_5400_ERR, PCI_DEVICE_ID_INTEL_5400_FBD0 },
> -	{ 0, 0 }
> -};
> -
> -#ifdef MODULE
>   static const struct pci_device_id i5k_amb_ids[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5000_ERR) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5400_ERR) },
> +	{ PCI_DEVICE_DATA(INTEL, 5000_ERR, PCI_DEVICE_ID_INTEL_5000_FBD0) },
> +	{ PCI_DEVICE_DATA(INTEL, 5400_ERR, PCI_DEVICE_ID_INTEL_5400_FBD0) },
>   	{ 0, }
>   };
>   MODULE_DEVICE_TABLE(pci, i5k_amb_ids);
> -#endif
>   
>   static int i5k_amb_probe(struct platform_device *pdev)
>   {
> @@ -504,22 +492,26 @@ static int i5k_amb_probe(struct platform_device *pdev)
>   	/* Figure out where the AMB registers live */
>   	i = 0;
>   	do {
> -		res = i5k_find_amb_registers(data, chipset_ids[i].err);
> +		res = i5k_find_amb_registers(data, &i5k_amb_ids[i]);
>   		if (res == 0)
>   			break;
>   		i++;
> -	} while (chipset_ids[i].err);
> +	} while (i5k_amb_ids[i].device);
>   
>   	if (res)
>   		goto err;
>   
>   	/* Copy the DIMM presence map for the first two channels */
> -	res = i5k_channel_probe(&data->amb_present[0], chipset_ids[i].fbd0);
> +	res = i5k_channel_probe(&data->amb_present[0],
> +				i5k_amb_ids[i].vendor,
> +				i5k_amb_ids[i].driver_data);
>   	if (res)
>   		goto err;
>   
>   	/* Copy the DIMM presence map for the optional second two channels */
> -	i5k_channel_probe(&data->amb_present[2], chipset_ids[i].fbd0 + 1);
> +	i5k_channel_probe(&data->amb_present[2],
> +			  i5k_amb_ids[i].vendor,
> +			  i5k_amb_ids[i].driver_data+1);
>   
>   	/* Set up resource regions */
>   	reso = request_mem_region(data->amb_base, data->amb_len, DRVNAME);
>