[PATCH v1 0/2] misc: pch_phub: Two cleanups

[PATCH v1 0/2] misc: pch_phub: Two cleanups

[Uwe Kleine-König (The Capable Hub)]

Hello,

my objective is to convert all pci_device_id assignments to use named
initializer for a further cleanup. See
https://lore.kernel.org/all/20260507074102.2654314-2-u.kleine-koenig@baylibre.com/
for details about that.

While working on the pch_phub driver, I found a few more opportunities
for improvement and the conversion to named initializers is just a side
effect of the 2nd patch.

If a "= 1" was added to the definition of PCH_EG20T in patch #2, there would be
no differences in the generated code. Without that there is a shift in the
magic constants that doesn't change the semantics however.

Best regards
Uwe

Uwe Kleine-König (The Capable Hub) (2):
  misc: pch_phub: Drop two unused functions
  misc: pch_phub: Introduce an enum for device indentification

 drivers/misc/pch_phub.c | 151 +++++-----------------------------------
 1 file changed, 18 insertions(+), 133 deletions(-)


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.47.3

[PATCH v1 1/2] misc: pch_phub: Drop two unused functions

[Uwe Kleine-König (The Capable Hub)]

The two functions are unused since commit 34afa1d657d4
("misc/pch_phub.c: use generic power management") but the compiler
didn't warn about it because the same commit marked the functions as
__maybe_unsed.

Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
---
 drivers/misc/pch_phub.c | 123 ----------------------------------------
 1 file changed, 123 deletions(-)

diff --git a/drivers/misc/pch_phub.c b/drivers/misc/pch_phub.c
index fd147fd2800f..539912e825eb 100644
--- a/drivers/misc/pch_phub.c
+++ b/drivers/misc/pch_phub.c
@@ -149,129 +149,6 @@ static void pch_phub_read_modify_write_reg(struct pch_phub_reg *chip,
 	iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
 }
 
-/* pch_phub_save_reg_conf - saves register configuration */
-static void __maybe_unused pch_phub_save_reg_conf(struct pci_dev *pdev)
-{
-	unsigned int i;
-	struct pch_phub_reg *chip = pci_get_drvdata(pdev);
-
-	void __iomem *p = chip->pch_phub_base_address;
-
-	chip->phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
-	chip->q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
-	chip->rc_q_maxsize_reg = ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
-	chip->bri_q_maxsize_reg = ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
-	chip->comp_resp_timeout_reg =
-				ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
-	chip->bus_slave_control_reg =
-				ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
-	chip->deadlock_avoid_type_reg =
-				ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
-	chip->intpin_reg_wpermit_reg0 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
-	chip->intpin_reg_wpermit_reg1 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
-	chip->intpin_reg_wpermit_reg2 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
-	chip->intpin_reg_wpermit_reg3 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
-	dev_dbg(&pdev->dev, "%s : "
-		"chip->phub_id_reg=%x, "
-		"chip->q_pri_val_reg=%x, "
-		"chip->rc_q_maxsize_reg=%x, "
-		"chip->bri_q_maxsize_reg=%x, "
-		"chip->comp_resp_timeout_reg=%x, "
-		"chip->bus_slave_control_reg=%x, "
-		"chip->deadlock_avoid_type_reg=%x, "
-		"chip->intpin_reg_wpermit_reg0=%x, "
-		"chip->intpin_reg_wpermit_reg1=%x, "
-		"chip->intpin_reg_wpermit_reg2=%x, "
-		"chip->intpin_reg_wpermit_reg3=%x\n", __func__,
-		chip->phub_id_reg,
-		chip->q_pri_val_reg,
-		chip->rc_q_maxsize_reg,
-		chip->bri_q_maxsize_reg,
-		chip->comp_resp_timeout_reg,
-		chip->bus_slave_control_reg,
-		chip->deadlock_avoid_type_reg,
-		chip->intpin_reg_wpermit_reg0,
-		chip->intpin_reg_wpermit_reg1,
-		chip->intpin_reg_wpermit_reg2,
-		chip->intpin_reg_wpermit_reg3);
-	for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
-		chip->int_reduce_control_reg[i] =
-		    ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
-		dev_dbg(&pdev->dev, "%s : "
-			"chip->int_reduce_control_reg[%d]=%x\n",
-			__func__, i, chip->int_reduce_control_reg[i]);
-	}
-	chip->clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
-	if ((chip->ioh_type == 2) || (chip->ioh_type == 4))
-		chip->funcsel_reg = ioread32(p + FUNCSEL_REG_OFFSET);
-}
-
-/* pch_phub_restore_reg_conf - restore register configuration */
-static void __maybe_unused pch_phub_restore_reg_conf(struct pci_dev *pdev)
-{
-	unsigned int i;
-	struct pch_phub_reg *chip = pci_get_drvdata(pdev);
-	void __iomem *p;
-	p = chip->pch_phub_base_address;
-
-	iowrite32(chip->phub_id_reg, p + PCH_PHUB_ID_REG);
-	iowrite32(chip->q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
-	iowrite32(chip->rc_q_maxsize_reg, p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
-	iowrite32(chip->bri_q_maxsize_reg, p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
-	iowrite32(chip->comp_resp_timeout_reg,
-					p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
-	iowrite32(chip->bus_slave_control_reg,
-					p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
-	iowrite32(chip->deadlock_avoid_type_reg,
-					p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
-	iowrite32(chip->intpin_reg_wpermit_reg0,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
-	iowrite32(chip->intpin_reg_wpermit_reg1,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
-	iowrite32(chip->intpin_reg_wpermit_reg2,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
-	iowrite32(chip->intpin_reg_wpermit_reg3,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
-	dev_dbg(&pdev->dev, "%s : "
-		"chip->phub_id_reg=%x, "
-		"chip->q_pri_val_reg=%x, "
-		"chip->rc_q_maxsize_reg=%x, "
-		"chip->bri_q_maxsize_reg=%x, "
-		"chip->comp_resp_timeout_reg=%x, "
-		"chip->bus_slave_control_reg=%x, "
-		"chip->deadlock_avoid_type_reg=%x, "
-		"chip->intpin_reg_wpermit_reg0=%x, "
-		"chip->intpin_reg_wpermit_reg1=%x, "
-		"chip->intpin_reg_wpermit_reg2=%x, "
-		"chip->intpin_reg_wpermit_reg3=%x\n", __func__,
-		chip->phub_id_reg,
-		chip->q_pri_val_reg,
-		chip->rc_q_maxsize_reg,
-		chip->bri_q_maxsize_reg,
-		chip->comp_resp_timeout_reg,
-		chip->bus_slave_control_reg,
-		chip->deadlock_avoid_type_reg,
-		chip->intpin_reg_wpermit_reg0,
-		chip->intpin_reg_wpermit_reg1,
-		chip->intpin_reg_wpermit_reg2,
-		chip->intpin_reg_wpermit_reg3);
-	for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
-		iowrite32(chip->int_reduce_control_reg[i],
-			p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
-		dev_dbg(&pdev->dev, "%s : "
-			"chip->int_reduce_control_reg[%d]=%x\n",
-			__func__, i, chip->int_reduce_control_reg[i]);
-	}
-
-	iowrite32(chip->clkcfg_reg, p + CLKCFG_REG_OFFSET);
-	if ((chip->ioh_type == 2) || (chip->ioh_type == 4))
-		iowrite32(chip->funcsel_reg, p + FUNCSEL_REG_OFFSET);
-}
-
 /**
  * pch_phub_read_serial_rom() - Reading Serial ROM
  * @chip:		Pointer to the PHUB register structure
-- 
2.47.3

[PATCH v1 2/2] misc: pch_phub: Introduce an enum for device indentification

[Uwe Kleine-König (The Capable Hub)]

Instead of using magic constants give them names that make the code more
idiomatic. While touching the pci_device_id array, use named
initializers to assign .driver_data.

Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
---
 drivers/misc/pch_phub.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/pch_phub.c b/drivers/misc/pch_phub.c
index 539912e825eb..19c4fa017f24 100644
--- a/drivers/misc/pch_phub.c
+++ b/drivers/misc/pch_phub.c
@@ -537,6 +537,14 @@ static const struct bin_attribute pch_bin_attr = {
 	.write = pch_phub_bin_write,
 };
 
+enum {
+	PCH_EG20T,
+	PCH_ML7213,
+	PCH_ML7223M,
+	PCH_ML7223N,
+	PCH_ML7831,
+};
+
 static int pch_phub_probe(struct pci_dev *pdev,
 				    const struct pci_device_id *id)
 {
@@ -579,7 +587,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 
 	chip->pdev = pdev; /* Save pci device struct */
 
-	if (id->driver_data == 1) { /* EG20T PCH */
+	if (id->driver_data == PCH_EG20T) { /* EG20T PCH */
 		const char *board_name;
 		unsigned int prefetch = 0x000affaa;
 
@@ -627,7 +635,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 					CLKCFG_UART_MASK);
 			}
 		}
-	} else if (id->driver_data == 2) { /* ML7213 IOH */
+	} else if (id->driver_data == PCH_ML7213) { /* ML7213 IOH */
 		ret = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
 		if (ret)
 			goto err_sysfs_create;
@@ -640,7 +648,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 		iowrite32(0x000affa0, chip->pch_phub_base_address + 0x14);
 		chip->pch_opt_rom_start_address =\
 						 PCH_PHUB_ROM_START_ADDR_ML7213;
-	} else if (id->driver_data == 3) { /* ML7223 IOH Bus-m*/
+	} else if (id->driver_data == PCH_ML7223M) { /* ML7223 IOH Bus-m*/
 		/* set the prefech value
 		 * Device8(GbE)
 		 */
@@ -650,7 +658,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 		chip->pch_opt_rom_start_address =\
 						 PCH_PHUB_ROM_START_ADDR_ML7223;
 		chip->pch_mac_start_address = PCH_PHUB_MAC_START_ADDR_ML7223;
-	} else if (id->driver_data == 4) { /* ML7223 IOH Bus-n*/
+	} else if (id->driver_data == PCH_ML7223N) { /* ML7223 IOH Bus-n*/
 		ret = sysfs_create_file(&pdev->dev.kobj,
 					&dev_attr_pch_mac.attr);
 		if (ret)
@@ -667,7 +675,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 		chip->pch_opt_rom_start_address =\
 						 PCH_PHUB_ROM_START_ADDR_ML7223;
 		chip->pch_mac_start_address = PCH_PHUB_MAC_START_ADDR_ML7223;
-	} else if (id->driver_data == 5) { /* ML7831 */
+	} else if (id->driver_data == PCH_ML7831) { /* ML7831 */
 		ret = sysfs_create_file(&pdev->dev.kobj,
 					&dev_attr_pch_mac.attr);
 		if (ret)
@@ -731,11 +739,11 @@ static int __maybe_unused pch_phub_resume(struct device *dev_d)
 }
 
 static const struct pci_device_id pch_phub_pcidev_id[] = {
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PCH1_PHUB),       1,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7213_PHUB), 2,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_mPHUB), 3,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_nPHUB), 4,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7831_PHUB), 5,  },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PCH1_PHUB),        .driver_data = PCH_EG20T },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7213_PHUB),  .driver_data = PCH_ML7213 },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_mPHUB), .driver_data = PCH_ML7223M },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_nPHUB), .driver_data = PCH_ML7223N },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7831_PHUB),  .driver_data = PCH_ML7831 },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pch_phub_pcidev_id);
-- 
2.47.3

Re: [PATCH v1 0/2] misc: pch_phub: Two cleanups

[Uwe Kleine-König (The Capable Hub)]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

Hello Greg,

On Thu, May 21, 2026 at 12:46:34PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> my objective is to convert all pci_device_id assignments to use named
> initializer for a further cleanup. See
> https://lore.kernel.org/all/20260507074102.2654314-2-u.kleine-koenig@baylibre.com/
> for details about that.
> 
> While working on the pch_phub driver, I found a few more opportunities
> for improvement and the conversion to named initializers is just a side
> effect of the 2nd patch.
> 
> If a "= 1" was added to the definition of PCH_EG20T in patch #2, there would be
> no differences in the generated code. Without that there is a shift in the
> magic constants that doesn't change the semantics however.

Sashiko[1] found a few issues in this series that I'd like to address. Can
you please drop this series from your queue?

Best regards
Uwe

[1] https://sashiko.dev/#/patchset/cover.1779360001.git.u.kleine-koenig%40baylibre.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]