[PATCH 0/2] PCI: shpchp: trivial code cleanup patches

[PATCH 0/2] PCI: shpchp: trivial code cleanup patches

[ngn]

This is a small and simple patchset that cleans up the old shpchp driver,
a part from fixing checkpatch warnings, this patch set removes the hpc_ops
struct, which is unnecessary. This is explained in the TODO file:
drivers/pci/hotplug/TODO.

Also this is my first patch!! So if you have the time and the enegery
please let me know if I did something wrong :)

ngn (2):
  PCI: shpchp: Remove hpc_ops
  PCI: shpchp: Follow kernel coding style

 drivers/pci/hotplug/shpchp.h       |  84 ++++++++--------
 drivers/pci/hotplug/shpchp_core.c  |  20 ++--
 drivers/pci/hotplug/shpchp_ctrl.c  | 107 ++++++++++----------
 drivers/pci/hotplug/shpchp_hpc.c   | 154 ++++++++++++-----------------
 drivers/pci/hotplug/shpchp_sysfs.c |   7 +-
 5 files changed, 167 insertions(+), 205 deletions(-)

-- 
2.45.1

[PATCH 1/2] PCI: shpchp: Remove hpc_ops

[ngn]

fix checpatch warnings and call hpc_ops functions directly, this has been
also done in pciehp with commit 82a9e79ef132

Signed-off-by: ngn <ngn@ngn.tf>
---
 drivers/pci/hotplug/shpchp.h      |  84 ++++++++++++-----------
 drivers/pci/hotplug/shpchp_core.c |  20 +++---
 drivers/pci/hotplug/shpchp_ctrl.c | 107 ++++++++++++++----------------
 drivers/pci/hotplug/shpchp_hpc.c  |  63 ++++++------------
 4 files changed, 120 insertions(+), 154 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index 3a97f455336e..6931d648b873 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -48,16 +48,15 @@ do {									\
 #define ctrl_dbg(ctrl, format, arg...)					\
 	do {								\
 		if (shpchp_debug)					\
-			pci_printk(KERN_DEBUG, ctrl->pci_dev,		\
+			pci_printk(KERN_DEBUG, (ctrl)->pci_dev,		\
 					format, ## arg);		\
 	} while (0)
 #define ctrl_err(ctrl, format, arg...)					\
-	pci_err(ctrl->pci_dev, format, ## arg)
+	pci_err((ctrl)->pci_dev, format, ## arg)
 #define ctrl_info(ctrl, format, arg...)					\
-	pci_info(ctrl->pci_dev, format, ## arg)
+	pci_info((ctrl)->pci_dev, format, ## arg)
 #define ctrl_warn(ctrl, format, arg...)					\
-	pci_warn(ctrl->pci_dev, format, ## arg)
-
+	pci_warn((ctrl)->pci_dev, format, ## arg)
 
 #define SLOT_NAME_SIZE 10
 struct slot {
@@ -72,7 +71,6 @@ struct slot {
 	u8 latch_save;
 	u8 pwr_save;
 	struct controller *ctrl;
-	const struct hpc_ops *hpc_ops;
 	struct hotplug_slot hotplug_slot;
 	struct list_head	slot_list;
 	struct delayed_work work;	/* work for button event */
@@ -94,7 +92,6 @@ struct controller {
 	int slot_num_inc;		/* 1 or -1 */
 	struct pci_dev *pci_dev;
 	struct list_head slot_list;
-	const struct hpc_ops *hpc_ops;
 	wait_queue_head_t queue;	/* sleep & wake process */
 	u8 slot_device_offset;
 	u32 pcix_misc2_reg;	/* for amd pogo errata */
@@ -175,20 +172,20 @@ static inline const char *slot_name(struct slot *slot)
 }
 
 struct ctrl_reg {
-	volatile u32 base_offset;
-	volatile u32 slot_avail1;
-	volatile u32 slot_avail2;
-	volatile u32 slot_config;
-	volatile u16 sec_bus_config;
-	volatile u8  msi_ctrl;
-	volatile u8  prog_interface;
-	volatile u16 cmd;
-	volatile u16 cmd_status;
-	volatile u32 intr_loc;
-	volatile u32 serr_loc;
-	volatile u32 serr_intr_enable;
-	volatile u32 slot1;
-} __attribute__ ((packed));
+	u32 base_offset;
+	u32 slot_avail1;
+	u32 slot_avail2;
+	u32 slot_config;
+	u16 sec_bus_config;
+	u8  msi_ctrl;
+	u8  prog_interface;
+	u16 cmd;
+	u16 cmd_status;
+	u32 intr_loc;
+	u32 serr_loc;
+	u32 serr_intr_enable;
+	u32 slot1;
+} __packed;
 
 /* offsets to the controller registers based on the above structure layout */
 enum ctrl_offsets {
@@ -252,18 +249,21 @@ static inline void amd_pogo_errata_restore_misc_reg(struct slot *p_slot)
 	u8  rse_set;
 
 	/* write-one-to-clear Bridge_Errors[ PERR_OBSERVED ] */
-	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET, &pcix_bridge_errors_reg);
+	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET,
+			      &pcix_bridge_errors_reg);
 	perr_set = pcix_bridge_errors_reg & PERR_OBSERVED_MASK;
 	if (perr_set) {
 		ctrl_dbg(p_slot->ctrl,
 			 "Bridge_Errors[ PERR_OBSERVED = %08X] (W1C)\n",
 			 perr_set);
 
-		pci_write_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET, perr_set);
+		pci_write_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET,
+				       perr_set);
 	}
 
 	/* write-one-to-clear Memory_Base_Limit[ RSE ] */
-	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MEM_BASE_LIMIT_OFFSET, &pcix_mem_base_reg);
+	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MEM_BASE_LIMIT_OFFSET,
+			      &pcix_mem_base_reg);
 	rse_set = pcix_mem_base_reg & RSE_MASK;
 	if (rse_set) {
 		ctrl_dbg(p_slot->ctrl, "Memory_Base_Limit[ RSE ] (W1C)\n");
@@ -300,24 +300,22 @@ static inline void amd_pogo_errata_restore_misc_reg(struct slot *p_slot)
 	pci_write_config_dword(p_slot->ctrl->pci_dev, PCIX_MISCII_OFFSET, pcix_misc2_temp);
 }
 
-struct hpc_ops {
-	int (*power_on_slot)(struct slot *slot);
-	int (*slot_enable)(struct slot *slot);
-	int (*slot_disable)(struct slot *slot);
-	int (*set_bus_speed_mode)(struct slot *slot, enum pci_bus_speed speed);
-	int (*get_power_status)(struct slot *slot, u8 *status);
-	int (*get_attention_status)(struct slot *slot, u8 *status);
-	int (*set_attention_status)(struct slot *slot, u8 status);
-	int (*get_latch_status)(struct slot *slot, u8 *status);
-	int (*get_adapter_status)(struct slot *slot, u8 *status);
-	int (*get_adapter_speed)(struct slot *slot, enum pci_bus_speed *speed);
-	int (*get_prog_int)(struct slot *slot, u8 *prog_int);
-	int (*query_power_fault)(struct slot *slot);
-	void (*green_led_on)(struct slot *slot);
-	void (*green_led_off)(struct slot *slot);
-	void (*green_led_blink)(struct slot *slot);
-	void (*release_ctlr)(struct controller *ctrl);
-	int (*check_cmd_status)(struct controller *ctrl);
-};
+int shpchp_power_on_slot(struct slot *slot);
+int shpchp_slot_enable(struct slot *slot);
+int shpchp_slot_disable(struct slot *slot);
+int shpchp_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed speed);
+int shpchp_get_power_status(struct slot *slot, u8 *status);
+int shpchp_get_attention_status(struct slot *slot, u8 *status);
+int shpchp_set_attention_status(struct slot *slot, u8 status);
+int shpchp_get_latch_status(struct slot *slot, u8 *status);
+int shpchp_get_adapter_status(struct slot *slot, u8 *status);
+int shpchp_get_adapter_speed(struct slot *slot, enum pci_bus_speed *speed);
+int shpchp_get_prog_int(struct slot *slot, u8 *prog_int);
+int shpchp_query_power_fault(struct slot *slot);
+void shpchp_green_led_on(struct slot *slot);
+void shpchp_green_led_off(struct slot *slot);
+void shpchp_green_led_blink(struct slot *slot);
+void shpchp_release_ctlr(struct controller *ctrl);
+int shpchp_check_cmd_status(struct controller *ctrl);
 
 #endif				/* _SHPCHP_H */
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 56c7795ed890..f9a54a509dab 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -81,7 +81,6 @@ static int init_slots(struct controller *ctrl)
 		slot->ctrl = ctrl;
 		slot->bus = ctrl->pci_dev->subordinate->number;
 		slot->device = ctrl->slot_device_offset + i;
-		slot->hpc_ops = ctrl->hpc_ops;
 		slot->number = ctrl->first_slot + (ctrl->slot_num_inc * i);
 
 		slot->wq = alloc_workqueue("shpchp-%d", 0, 0, slot->number);
@@ -102,7 +101,7 @@ static int init_slots(struct controller *ctrl)
 			 slot->bus, slot->device, slot->hp_slot, slot->number,
 			 ctrl->slot_device_offset);
 		retval = pci_hp_register(hotplug_slot,
-				ctrl->pci_dev->subordinate, slot->device, name);
+					 ctrl->pci_dev->subordinate, slot->device, name);
 		if (retval) {
 			ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
 				 retval);
@@ -150,7 +149,7 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
 		 __func__, slot_name(slot));
 
 	slot->attention_save = status;
-	slot->hpc_ops->set_attention_status(slot, status);
+	shpchp_set_attention_status(slot, status);
 
 	return 0;
 }
@@ -183,7 +182,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
 		 __func__, slot_name(slot));
 
-	retval = slot->hpc_ops->get_power_status(slot, value);
+	retval = shpchp_get_power_status(slot, value);
 	if (retval < 0)
 		*value = slot->pwr_save;
 
@@ -198,7 +197,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
 		 __func__, slot_name(slot));
 
-	retval = slot->hpc_ops->get_attention_status(slot, value);
+	retval = shpchp_get_attention_status(slot, value);
 	if (retval < 0)
 		*value = slot->attention_save;
 
@@ -213,7 +212,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
 		 __func__, slot_name(slot));
 
-	retval = slot->hpc_ops->get_latch_status(slot, value);
+	retval = shpchp_get_latch_status(slot, value);
 	if (retval < 0)
 		*value = slot->latch_save;
 
@@ -228,7 +227,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
 		 __func__, slot_name(slot));
 
-	retval = slot->hpc_ops->get_adapter_status(slot, value);
+	retval = shpchp_get_adapter_status(slot, value);
 	if (retval < 0)
 		*value = slot->presence_save;
 
@@ -264,7 +263,7 @@ static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
 	if (!ctrl)
-		goto err_out_none;
+		return -ENODEV;
 
 	INIT_LIST_HEAD(&ctrl->slot_list);
 
@@ -293,10 +292,9 @@ static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_cleanup_slots:
 	cleanup_slots(ctrl);
 err_out_release_ctlr:
-	ctrl->hpc_ops->release_ctlr(ctrl);
+	shpchp_release_ctlr(ctrl);
 err_out_free_ctrl:
 	kfree(ctrl);
-err_out_none:
 	return -ENODEV;
 }
 
@@ -306,7 +304,7 @@ static void shpc_remove(struct pci_dev *dev)
 
 	dev->shpc_managed = 0;
 	shpchp_remove_ctrl_files(ctrl);
-	ctrl->hpc_ops->release_ctlr(ctrl);
+	shpchp_release_ctlr(ctrl);
 	kfree(ctrl);
 }
 
diff --git a/drivers/pci/hotplug/shpchp_ctrl.c b/drivers/pci/hotplug/shpchp_ctrl.c
index 6a6705e0cf17..9e6437e318f5 100644
--- a/drivers/pci/hotplug/shpchp_ctrl.c
+++ b/drivers/pci/hotplug/shpchp_ctrl.c
@@ -51,7 +51,7 @@ u8 shpchp_handle_attention_button(u8 hp_slot, struct controller *ctrl)
 	ctrl_dbg(ctrl, "Attention button interrupt received\n");
 
 	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);
-	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
+	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
 
 	/*
 	 *  Button pressed - See if need to TAKE ACTION!!!
@@ -62,7 +62,6 @@ u8 shpchp_handle_attention_button(u8 hp_slot, struct controller *ctrl)
 	queue_interrupt_event(p_slot, event_type);
 
 	return 0;
-
 }
 
 u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
@@ -75,8 +74,8 @@ u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
 	ctrl_dbg(ctrl, "Switch interrupt received\n");
 
 	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);
-	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
-	p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
+	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
+	shpchp_get_latch_status(p_slot, &getstatus);
 	ctrl_dbg(ctrl, "Card present %x Power status %x\n",
 		 p_slot->presence_save, p_slot->pwr_save);
 
@@ -116,7 +115,7 @@ u8 shpchp_handle_presence_change(u8 hp_slot, struct controller *ctrl)
 	/*
 	 * Save the presence state
 	 */
-	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
+	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
 	if (p_slot->presence_save) {
 		/*
 		 * Card Present
@@ -148,7 +147,7 @@ u8 shpchp_handle_power_fault(u8 hp_slot, struct controller *ctrl)
 
 	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);
 
-	if (!(p_slot->hpc_ops->query_power_fault(p_slot))) {
+	if (!(shpchp_query_power_fault(p_slot))) {
 		/*
 		 * Power fault Cleared
 		 */
@@ -173,15 +172,15 @@ u8 shpchp_handle_power_fault(u8 hp_slot, struct controller *ctrl)
 }
 
 /* The following routines constitute the bulk of the
-   hotplug controller logic
+ * hotplug controller logic
  */
 static int change_bus_speed(struct controller *ctrl, struct slot *p_slot,
-		enum pci_bus_speed speed)
+			    enum pci_bus_speed speed)
 {
 	int rc = 0;
 
 	ctrl_dbg(ctrl, "Change speed to %d\n", speed);
-	rc = p_slot->hpc_ops->set_bus_speed_mode(p_slot, speed);
+	rc = shpchp_set_bus_speed_mode(p_slot, speed);
 	if (rc) {
 		ctrl_err(ctrl, "%s: Issue of set bus speed mode command failed\n",
 			 __func__);
@@ -191,8 +190,8 @@ static int change_bus_speed(struct controller *ctrl, struct slot *p_slot,
 }
 
 static int fix_bus_speed(struct controller *ctrl, struct slot *pslot,
-		u8 flag, enum pci_bus_speed asp, enum pci_bus_speed bsp,
-		enum pci_bus_speed msp)
+			 u8 flag, enum pci_bus_speed asp, enum pci_bus_speed bsp,
+			 enum pci_bus_speed msp)
 {
 	int rc = 0;
 
@@ -241,14 +240,14 @@ static int board_added(struct slot *p_slot)
 		 __func__, p_slot->device, ctrl->slot_device_offset, hp_slot);
 
 	/* Power on slot without connecting to bus */
-	rc = p_slot->hpc_ops->power_on_slot(p_slot);
+	rc = shpchp_power_on_slot(p_slot);
 	if (rc) {
 		ctrl_err(ctrl, "Failed to power on slot\n");
 		return -1;
 	}
 
-	if ((ctrl->pci_dev->vendor == 0x8086) && (ctrl->pci_dev->device == 0x0332)) {
-		rc = p_slot->hpc_ops->set_bus_speed_mode(p_slot, PCI_SPEED_33MHz);
+	if (ctrl->pci_dev->vendor == 0x8086 && ctrl->pci_dev->device == 0x0332) {
+		rc = shpchp_set_bus_speed_mode(p_slot, PCI_SPEED_33MHz);
 		if (rc) {
 			ctrl_err(ctrl, "%s: Issue of set bus speed mode command failed\n",
 				 __func__);
@@ -256,14 +255,14 @@ static int board_added(struct slot *p_slot)
 		}
 
 		/* turn on board, blink green LED, turn off Amber LED */
-		rc = p_slot->hpc_ops->slot_enable(p_slot);
+		rc = shpchp_slot_enable(p_slot);
 		if (rc) {
 			ctrl_err(ctrl, "Issue of Slot Enable command failed\n");
 			return rc;
 		}
 	}
 
-	rc = p_slot->hpc_ops->get_adapter_speed(p_slot, &asp);
+	rc = shpchp_get_adapter_speed(p_slot, &asp);
 	if (rc) {
 		ctrl_err(ctrl, "Can't get adapter speed or bus mode mismatch\n");
 		return WRONG_BUS_FREQUENCY;
@@ -285,7 +284,7 @@ static int board_added(struct slot *p_slot)
 		return rc;
 
 	/* turn on board, blink green LED, turn off Amber LED */
-	rc = p_slot->hpc_ops->slot_enable(p_slot);
+	rc = shpchp_slot_enable(p_slot);
 	if (rc) {
 		ctrl_err(ctrl, "Issue of Slot Enable command failed\n");
 		return rc;
@@ -313,13 +312,13 @@ static int board_added(struct slot *p_slot)
 	p_slot->is_a_board = 0x01;
 	p_slot->pwr_save = 1;
 
-	p_slot->hpc_ops->green_led_on(p_slot);
+	shpchp_green_led_on(p_slot);
 
 	return 0;
 
 err_exit:
 	/* turn off slot, turn on Amber LED, turn off Green LED */
-	rc = p_slot->hpc_ops->slot_disable(p_slot);
+	rc = shpchp_slot_disable(p_slot);
 	if (rc) {
 		ctrl_err(ctrl, "%s: Issue of Slot Disable command failed\n",
 			 __func__);
@@ -329,7 +328,6 @@ static int board_added(struct slot *p_slot)
 	return(rc);
 }
 
-
 /**
  * remove_board - Turns off slot and LEDs
  * @p_slot: target &slot
@@ -352,14 +350,14 @@ static int remove_board(struct slot *p_slot)
 		p_slot->status = 0x01;
 
 	/* turn off slot, turn on Amber LED, turn off Green LED */
-	rc = p_slot->hpc_ops->slot_disable(p_slot);
+	rc = shpchp_slot_disable(p_slot);
 	if (rc) {
 		ctrl_err(ctrl, "%s: Issue of Slot Disable command failed\n",
 			 __func__);
 		return rc;
 	}
 
-	rc = p_slot->hpc_ops->set_attention_status(p_slot, 0);
+	rc = shpchp_set_attention_status(p_slot, 0);
 	if (rc) {
 		ctrl_err(ctrl, "Issue of Set Attention command failed\n");
 		return rc;
@@ -371,7 +369,6 @@ static int remove_board(struct slot *p_slot)
 	return 0;
 }
 
-
 struct pushbutton_work_info {
 	struct slot *p_slot;
 	struct work_struct work;
@@ -401,7 +398,7 @@ static void shpchp_pushbutton_thread(struct work_struct *work)
 	case POWERON_STATE:
 		mutex_unlock(&p_slot->lock);
 		if (shpchp_enable_slot(p_slot))
-			p_slot->hpc_ops->green_led_off(p_slot);
+			shpchp_green_led_off(p_slot);
 		mutex_lock(&p_slot->lock);
 		p_slot->state = STATIC_STATE;
 		break;
@@ -446,10 +443,10 @@ void shpchp_queue_pushbutton_work(struct work_struct *work)
 
 static void update_slot_info(struct slot *slot)
 {
-	slot->hpc_ops->get_power_status(slot, &slot->pwr_save);
-	slot->hpc_ops->get_attention_status(slot, &slot->attention_save);
-	slot->hpc_ops->get_latch_status(slot, &slot->latch_save);
-	slot->hpc_ops->get_adapter_status(slot, &slot->presence_save);
+	shpchp_get_power_status(slot, &slot->pwr_save);
+	shpchp_get_attention_status(slot, &slot->attention_save);
+	shpchp_get_latch_status(slot, &slot->latch_save);
+	shpchp_get_adapter_status(slot, &slot->presence_save);
 }
 
 /*
@@ -462,7 +459,7 @@ static void handle_button_press_event(struct slot *p_slot)
 
 	switch (p_slot->state) {
 	case STATIC_STATE:
-		p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
+		shpchp_get_power_status(p_slot, &getstatus);
 		if (getstatus) {
 			p_slot->state = BLINKINGOFF_STATE;
 			ctrl_info(ctrl, "PCI slot #%s - powering off due to button press\n",
@@ -473,10 +470,10 @@ static void handle_button_press_event(struct slot *p_slot)
 				  slot_name(p_slot));
 		}
 		/* blink green LED and turn off amber */
-		p_slot->hpc_ops->green_led_blink(p_slot);
-		p_slot->hpc_ops->set_attention_status(p_slot, 0);
+		shpchp_green_led_blink(p_slot);
+		shpchp_set_attention_status(p_slot, 0);
 
-		queue_delayed_work(p_slot->wq, &p_slot->work, 5*HZ);
+		queue_delayed_work(p_slot->wq, &p_slot->work, 5 * HZ);
 		break;
 	case BLINKINGOFF_STATE:
 	case BLINKINGON_STATE:
@@ -489,10 +486,10 @@ static void handle_button_press_event(struct slot *p_slot)
 			  slot_name(p_slot));
 		cancel_delayed_work(&p_slot->work);
 		if (p_slot->state == BLINKINGOFF_STATE)
-			p_slot->hpc_ops->green_led_on(p_slot);
+			shpchp_green_led_on(p_slot);
 		else
-			p_slot->hpc_ops->green_led_off(p_slot);
-		p_slot->hpc_ops->set_attention_status(p_slot, 0);
+			shpchp_green_led_off(p_slot);
+		shpchp_set_attention_status(p_slot, 0);
 		ctrl_info(ctrl, "PCI slot #%s - action canceled due to button press\n",
 			  slot_name(p_slot));
 		p_slot->state = STATIC_STATE;
@@ -526,8 +523,8 @@ static void interrupt_event_handler(struct work_struct *work)
 		break;
 	case INT_POWER_FAULT:
 		ctrl_dbg(p_slot->ctrl, "%s: Power fault\n", __func__);
-		p_slot->hpc_ops->set_attention_status(p_slot, 1);
-		p_slot->hpc_ops->green_led_off(p_slot);
+		shpchp_set_attention_status(p_slot, 1);
+		shpchp_green_led_off(p_slot);
 		break;
 	default:
 		update_slot_info(p_slot);
@@ -538,8 +535,7 @@ static void interrupt_event_handler(struct work_struct *work)
 	kfree(info);
 }
 
-
-static int shpchp_enable_slot (struct slot *p_slot)
+static int shpchp_enable_slot(struct slot *p_slot)
 {
 	u8 getstatus = 0;
 	int rc, retval = -ENODEV;
@@ -547,17 +543,17 @@ static int shpchp_enable_slot (struct slot *p_slot)
 
 	/* Check to see if (latch closed, card present, power off) */
 	mutex_lock(&p_slot->ctrl->crit_sect);
-	rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
+	rc = shpchp_get_adapter_status(p_slot, &getstatus);
 	if (rc || !getstatus) {
 		ctrl_info(ctrl, "No adapter on slot(%s)\n", slot_name(p_slot));
 		goto out;
 	}
-	rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
+	rc = shpchp_get_latch_status(p_slot, &getstatus);
 	if (rc || getstatus) {
 		ctrl_info(ctrl, "Latch open on slot(%s)\n", slot_name(p_slot));
 		goto out;
 	}
-	rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
+	rc = shpchp_get_power_status(p_slot, &getstatus);
 	if (rc || getstatus) {
 		ctrl_info(ctrl, "Already enabled on slot(%s)\n",
 			  slot_name(p_slot));
@@ -567,26 +563,26 @@ static int shpchp_enable_slot (struct slot *p_slot)
 	p_slot->is_a_board = 1;
 
 	/* We have to save the presence info for these slots */
-	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
-	p_slot->hpc_ops->get_power_status(p_slot, &(p_slot->pwr_save));
+	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
+	shpchp_get_power_status(p_slot, &p_slot->pwr_save);
 	ctrl_dbg(ctrl, "%s: p_slot->pwr_save %x\n", __func__, p_slot->pwr_save);
-	p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
+	shpchp_get_latch_status(p_slot, &getstatus);
 
 	if ((p_slot->ctrl->pci_dev->vendor == PCI_VENDOR_ID_AMD &&
-	     p_slot->ctrl->pci_dev->device == PCI_DEVICE_ID_AMD_POGO_7458)
-	     && p_slot->ctrl->num_slots == 1) {
+	     p_slot->ctrl->pci_dev->device == PCI_DEVICE_ID_AMD_POGO_7458) &&
+	     p_slot->ctrl->num_slots == 1) {
 		/* handle AMD POGO errata; this must be done before enable  */
 		amd_pogo_errata_save_misc_reg(p_slot);
 		retval = board_added(p_slot);
 		/* handle AMD POGO errata; this must be done after enable  */
 		amd_pogo_errata_restore_misc_reg(p_slot);
-	} else
+	} else {
 		retval = board_added(p_slot);
+	}
 
 	if (retval) {
-		p_slot->hpc_ops->get_adapter_status(p_slot,
-				&(p_slot->presence_save));
-		p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
+		shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
+		shpchp_get_latch_status(p_slot, &getstatus);
 	}
 
 	update_slot_info(p_slot);
@@ -595,8 +591,7 @@ static int shpchp_enable_slot (struct slot *p_slot)
 	return retval;
 }
 
-
-static int shpchp_disable_slot (struct slot *p_slot)
+static int shpchp_disable_slot(struct slot *p_slot)
 {
 	u8 getstatus = 0;
 	int rc, retval = -ENODEV;
@@ -608,17 +603,17 @@ static int shpchp_disable_slot (struct slot *p_slot)
 	/* Check to see if (latch closed, card present, power on) */
 	mutex_lock(&p_slot->ctrl->crit_sect);
 
-	rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
+	rc = shpchp_get_adapter_status(p_slot, &getstatus);
 	if (rc || !getstatus) {
 		ctrl_info(ctrl, "No adapter on slot(%s)\n", slot_name(p_slot));
 		goto out;
 	}
-	rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
+	rc = shpchp_get_latch_status(p_slot, &getstatus);
 	if (rc || getstatus) {
 		ctrl_info(ctrl, "Latch open on slot(%s)\n", slot_name(p_slot));
 		goto out;
 	}
-	rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
+	rc = shpchp_get_power_status(p_slot, &getstatus);
 	if (rc || !getstatus) {
 		ctrl_info(ctrl, "Already disabled on slot(%s)\n",
 			  slot_name(p_slot));
diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 48e4daefc44a..012b9e3fe5b0 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c
@@ -167,7 +167,6 @@
 
 static irqreturn_t shpc_isr(int irq, void *dev_id);
 static void start_int_poll_timer(struct controller *ctrl, int sec);
-static int hpc_check_cmd_status(struct controller *ctrl);
 
 static inline u8 shpc_readb(struct controller *ctrl, int reg)
 {
@@ -317,7 +316,7 @@ static int shpc_write_cmd(struct slot *slot, u8 t_slot, u8 cmd)
 	if (retval)
 		goto out;
 
-	cmd_status = hpc_check_cmd_status(slot->ctrl);
+	cmd_status = shpchp_check_cmd_status(slot->ctrl);
 	if (cmd_status) {
 		ctrl_err(ctrl, "Failed to issued command 0x%x (error code = %d)\n",
 			 cmd, cmd_status);
@@ -328,7 +327,7 @@ static int shpc_write_cmd(struct slot *slot, u8 t_slot, u8 cmd)
 	return retval;
 }
 
-static int hpc_check_cmd_status(struct controller *ctrl)
+int shpchp_check_cmd_status(struct controller *ctrl)
 {
 	int retval = 0;
 	u16 cmd_status = shpc_readw(ctrl, CMD_STATUS) & 0x000F;
@@ -357,7 +356,7 @@ static int hpc_check_cmd_status(struct controller *ctrl)
 }
 
 
-static int hpc_get_attention_status(struct slot *slot, u8 *status)
+int shpchp_get_attention_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
 	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
@@ -381,7 +380,7 @@ static int hpc_get_attention_status(struct slot *slot, u8 *status)
 	return 0;
 }
 
-static int hpc_get_power_status(struct slot *slot, u8 *status)
+int shpchp_get_power_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
 	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
@@ -406,7 +405,7 @@ static int hpc_get_power_status(struct slot *slot, u8 *status)
 }
 
 
-static int hpc_get_latch_status(struct slot *slot, u8 *status)
+int shpchp_get_latch_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
 	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
@@ -416,7 +415,7 @@ static int hpc_get_latch_status(struct slot *slot, u8 *status)
 	return 0;
 }
 
-static int hpc_get_adapter_status(struct slot *slot, u8 *status)
+int shpchp_get_adapter_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
 	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
@@ -427,7 +426,7 @@ static int hpc_get_adapter_status(struct slot *slot, u8 *status)
 	return 0;
 }
 
-static int hpc_get_prog_int(struct slot *slot, u8 *prog_int)
+int shpchp_get_prog_int(struct slot *slot, u8 *prog_int)
 {
 	struct controller *ctrl = slot->ctrl;
 
@@ -436,7 +435,7 @@ static int hpc_get_prog_int(struct slot *slot, u8 *prog_int)
 	return 0;
 }
 
-static int hpc_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
+int shpchp_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
 {
 	int retval = 0;
 	struct controller *ctrl = slot->ctrl;
@@ -444,7 +443,7 @@ static int hpc_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
 	u8 m66_cap  = !!(slot_reg & MHZ66_CAP);
 	u8 pi, pcix_cap;
 
-	retval = hpc_get_prog_int(slot, &pi);
+	retval = shpchp_get_prog_int(slot, &pi);
 	if (retval)
 		return retval;
 
@@ -489,7 +488,7 @@ static int hpc_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
 	return retval;
 }
 
-static int hpc_query_power_fault(struct slot *slot)
+int shpchp_query_power_fault(struct slot *slot)
 {
 	struct controller *ctrl = slot->ctrl;
 	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
@@ -498,7 +497,7 @@ static int hpc_query_power_fault(struct slot *slot)
 	return !(slot_reg & POWER_FAULT);
 }
 
-static int hpc_set_attention_status(struct slot *slot, u8 value)
+int shpchp_set_attention_status(struct slot *slot, u8 value)
 {
 	u8 slot_cmd = 0;
 
@@ -520,22 +519,22 @@ static int hpc_set_attention_status(struct slot *slot, u8 value)
 }
 
 
-static void hpc_set_green_led_on(struct slot *slot)
+void shpchp_green_led_on(struct slot *slot)
 {
 	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_ON);
 }
 
-static void hpc_set_green_led_off(struct slot *slot)
+void shpchp_green_led_off(struct slot *slot)
 {
 	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_OFF);
 }
 
-static void hpc_set_green_led_blink(struct slot *slot)
+void shpchp_green_led_blink(struct slot *slot)
 {
 	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_BLINK);
 }
 
-static void hpc_release_ctlr(struct controller *ctrl)
+void shpchp_release_ctlr(struct controller *ctrl)
 {
 	int i;
 	u32 slot_reg, serr_int;
@@ -575,7 +574,7 @@ static void hpc_release_ctlr(struct controller *ctrl)
 	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
 }
 
-static int hpc_power_on_slot(struct slot *slot)
+int shpchp_power_on_slot(struct slot *slot)
 {
 	int retval;
 
@@ -586,7 +585,7 @@ static int hpc_power_on_slot(struct slot *slot)
 	return retval;
 }
 
-static int hpc_slot_enable(struct slot *slot)
+int shpchp_slot_enable(struct slot *slot)
 {
 	int retval;
 
@@ -599,7 +598,7 @@ static int hpc_slot_enable(struct slot *slot)
 	return retval;
 }
 
-static int hpc_slot_disable(struct slot *slot)
+int shpchp_slot_disable(struct slot *slot)
 {
 	int retval;
 
@@ -681,7 +680,7 @@ static int shpc_get_cur_bus_speed(struct controller *ctrl)
 }
 
 
-static int hpc_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed value)
+int shpchp_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed value)
 {
 	int retval;
 	struct controller *ctrl = slot->ctrl;
@@ -871,28 +870,6 @@ static int shpc_get_max_bus_speed(struct controller *ctrl)
 	return retval;
 }
 
-static const struct hpc_ops shpchp_hpc_ops = {
-	.power_on_slot			= hpc_power_on_slot,
-	.slot_enable			= hpc_slot_enable,
-	.slot_disable			= hpc_slot_disable,
-	.set_bus_speed_mode		= hpc_set_bus_speed_mode,
-	.set_attention_status	= hpc_set_attention_status,
-	.get_power_status		= hpc_get_power_status,
-	.get_attention_status	= hpc_get_attention_status,
-	.get_latch_status		= hpc_get_latch_status,
-	.get_adapter_status		= hpc_get_adapter_status,
-
-	.get_adapter_speed		= hpc_get_adapter_speed,
-	.get_prog_int			= hpc_get_prog_int,
-
-	.query_power_fault		= hpc_query_power_fault,
-	.green_led_on			= hpc_set_green_led_on,
-	.green_led_off			= hpc_set_green_led_off,
-	.green_led_blink		= hpc_set_green_led_blink,
-
-	.release_ctlr			= hpc_release_ctlr,
-};
-
 int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
 {
 	int rc = -1, num_slots = 0;
@@ -978,8 +955,6 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
 	/* Setup wait queue */
 	init_waitqueue_head(&ctrl->queue);
 
-	ctrl->hpc_ops = &shpchp_hpc_ops;
-
 	/* Return PCI Controller Info */
 	slot_config = shpc_readl(ctrl, SLOT_CONFIG);
 	ctrl->slot_device_offset = (slot_config & FIRST_DEV_NUM) >> 8;
-- 
2.45.1

Re: [PATCH 1/2] PCI: shpchp: Remove hpc_ops

[Bjorn Helgaas]

On Fri, Jun 21, 2024 at 09:35:00AM +0300, ngn wrote:
> fix checpatch warnings and call hpc_ops functions directly, this has been
> also done in pciehp with commit 82a9e79ef132

Trivial changes like checkpatch fixes should be in a separate patch
from substantive things.  This helps make both easier to review.

It *might* be worthwhile to remove hpc_ops, although shpchp is pretty
old and dusty by now, and it's hard to justify touching things like
that because the risk of breaking something is relatively high and the
benefit is relatively low.

The fact that shpchp is the only user of hpc_ops does make me a little
more sympathetic to the idea of removing it, though.

> Signed-off-by: ngn <ngn@ngn.tf>
> ---
>  drivers/pci/hotplug/shpchp.h      |  84 ++++++++++++-----------
>  drivers/pci/hotplug/shpchp_core.c |  20 +++---
>  drivers/pci/hotplug/shpchp_ctrl.c | 107 ++++++++++++++----------------
>  drivers/pci/hotplug/shpchp_hpc.c  |  63 ++++++------------
>  4 files changed, 120 insertions(+), 154 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
> index 3a97f455336e..6931d648b873 100644
> --- a/drivers/pci/hotplug/shpchp.h
> +++ b/drivers/pci/hotplug/shpchp.h
> @@ -48,16 +48,15 @@ do {									\
>  #define ctrl_dbg(ctrl, format, arg...)					\
>  	do {								\
>  		if (shpchp_debug)					\
> -			pci_printk(KERN_DEBUG, ctrl->pci_dev,		\
> +			pci_printk(KERN_DEBUG, (ctrl)->pci_dev,		\
>  					format, ## arg);		\
>  	} while (0)
>  #define ctrl_err(ctrl, format, arg...)					\
> -	pci_err(ctrl->pci_dev, format, ## arg)
> +	pci_err((ctrl)->pci_dev, format, ## arg)
>  #define ctrl_info(ctrl, format, arg...)					\
> -	pci_info(ctrl->pci_dev, format, ## arg)
> +	pci_info((ctrl)->pci_dev, format, ## arg)
>  #define ctrl_warn(ctrl, format, arg...)					\
> -	pci_warn(ctrl->pci_dev, format, ## arg)
> -
> +	pci_warn((ctrl)->pci_dev, format, ## arg)
>  
>  #define SLOT_NAME_SIZE 10
>  struct slot {
> @@ -72,7 +71,6 @@ struct slot {
>  	u8 latch_save;
>  	u8 pwr_save;
>  	struct controller *ctrl;
> -	const struct hpc_ops *hpc_ops;
>  	struct hotplug_slot hotplug_slot;
>  	struct list_head	slot_list;
>  	struct delayed_work work;	/* work for button event */
> @@ -94,7 +92,6 @@ struct controller {
>  	int slot_num_inc;		/* 1 or -1 */
>  	struct pci_dev *pci_dev;
>  	struct list_head slot_list;
> -	const struct hpc_ops *hpc_ops;
>  	wait_queue_head_t queue;	/* sleep & wake process */
>  	u8 slot_device_offset;
>  	u32 pcix_misc2_reg;	/* for amd pogo errata */
> @@ -175,20 +172,20 @@ static inline const char *slot_name(struct slot *slot)
>  }
>  
>  struct ctrl_reg {
> -	volatile u32 base_offset;
> -	volatile u32 slot_avail1;
> -	volatile u32 slot_avail2;
> -	volatile u32 slot_config;
> -	volatile u16 sec_bus_config;
> -	volatile u8  msi_ctrl;
> -	volatile u8  prog_interface;
> -	volatile u16 cmd;
> -	volatile u16 cmd_status;
> -	volatile u32 intr_loc;
> -	volatile u32 serr_loc;
> -	volatile u32 serr_intr_enable;
> -	volatile u32 slot1;
> -} __attribute__ ((packed));
> +	u32 base_offset;
> +	u32 slot_avail1;
> +	u32 slot_avail2;
> +	u32 slot_config;
> +	u16 sec_bus_config;
> +	u8  msi_ctrl;
> +	u8  prog_interface;
> +	u16 cmd;
> +	u16 cmd_status;
> +	u32 intr_loc;
> +	u32 serr_loc;
> +	u32 serr_intr_enable;
> +	u32 slot1;
> +} __packed;
>  
>  /* offsets to the controller registers based on the above structure layout */
>  enum ctrl_offsets {
> @@ -252,18 +249,21 @@ static inline void amd_pogo_errata_restore_misc_reg(struct slot *p_slot)
>  	u8  rse_set;
>  
>  	/* write-one-to-clear Bridge_Errors[ PERR_OBSERVED ] */
> -	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET, &pcix_bridge_errors_reg);
> +	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET,
> +			      &pcix_bridge_errors_reg);
>  	perr_set = pcix_bridge_errors_reg & PERR_OBSERVED_MASK;
>  	if (perr_set) {
>  		ctrl_dbg(p_slot->ctrl,
>  			 "Bridge_Errors[ PERR_OBSERVED = %08X] (W1C)\n",
>  			 perr_set);
>  
> -		pci_write_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET, perr_set);
> +		pci_write_config_dword(p_slot->ctrl->pci_dev, PCIX_MISC_BRIDGE_ERRORS_OFFSET,
> +				       perr_set);
>  	}
>  
>  	/* write-one-to-clear Memory_Base_Limit[ RSE ] */
> -	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MEM_BASE_LIMIT_OFFSET, &pcix_mem_base_reg);
> +	pci_read_config_dword(p_slot->ctrl->pci_dev, PCIX_MEM_BASE_LIMIT_OFFSET,
> +			      &pcix_mem_base_reg);
>  	rse_set = pcix_mem_base_reg & RSE_MASK;
>  	if (rse_set) {
>  		ctrl_dbg(p_slot->ctrl, "Memory_Base_Limit[ RSE ] (W1C)\n");
> @@ -300,24 +300,22 @@ static inline void amd_pogo_errata_restore_misc_reg(struct slot *p_slot)
>  	pci_write_config_dword(p_slot->ctrl->pci_dev, PCIX_MISCII_OFFSET, pcix_misc2_temp);
>  }
>  
> -struct hpc_ops {
> -	int (*power_on_slot)(struct slot *slot);
> -	int (*slot_enable)(struct slot *slot);
> -	int (*slot_disable)(struct slot *slot);
> -	int (*set_bus_speed_mode)(struct slot *slot, enum pci_bus_speed speed);
> -	int (*get_power_status)(struct slot *slot, u8 *status);
> -	int (*get_attention_status)(struct slot *slot, u8 *status);
> -	int (*set_attention_status)(struct slot *slot, u8 status);
> -	int (*get_latch_status)(struct slot *slot, u8 *status);
> -	int (*get_adapter_status)(struct slot *slot, u8 *status);
> -	int (*get_adapter_speed)(struct slot *slot, enum pci_bus_speed *speed);
> -	int (*get_prog_int)(struct slot *slot, u8 *prog_int);
> -	int (*query_power_fault)(struct slot *slot);
> -	void (*green_led_on)(struct slot *slot);
> -	void (*green_led_off)(struct slot *slot);
> -	void (*green_led_blink)(struct slot *slot);
> -	void (*release_ctlr)(struct controller *ctrl);
> -	int (*check_cmd_status)(struct controller *ctrl);
> -};
> +int shpchp_power_on_slot(struct slot *slot);
> +int shpchp_slot_enable(struct slot *slot);
> +int shpchp_slot_disable(struct slot *slot);
> +int shpchp_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed speed);
> +int shpchp_get_power_status(struct slot *slot, u8 *status);
> +int shpchp_get_attention_status(struct slot *slot, u8 *status);
> +int shpchp_set_attention_status(struct slot *slot, u8 status);
> +int shpchp_get_latch_status(struct slot *slot, u8 *status);
> +int shpchp_get_adapter_status(struct slot *slot, u8 *status);
> +int shpchp_get_adapter_speed(struct slot *slot, enum pci_bus_speed *speed);
> +int shpchp_get_prog_int(struct slot *slot, u8 *prog_int);
> +int shpchp_query_power_fault(struct slot *slot);
> +void shpchp_green_led_on(struct slot *slot);
> +void shpchp_green_led_off(struct slot *slot);
> +void shpchp_green_led_blink(struct slot *slot);
> +void shpchp_release_ctlr(struct controller *ctrl);
> +int shpchp_check_cmd_status(struct controller *ctrl);
>  
>  #endif				/* _SHPCHP_H */
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 56c7795ed890..f9a54a509dab 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -81,7 +81,6 @@ static int init_slots(struct controller *ctrl)
>  		slot->ctrl = ctrl;
>  		slot->bus = ctrl->pci_dev->subordinate->number;
>  		slot->device = ctrl->slot_device_offset + i;
> -		slot->hpc_ops = ctrl->hpc_ops;
>  		slot->number = ctrl->first_slot + (ctrl->slot_num_inc * i);
>  
>  		slot->wq = alloc_workqueue("shpchp-%d", 0, 0, slot->number);
> @@ -102,7 +101,7 @@ static int init_slots(struct controller *ctrl)
>  			 slot->bus, slot->device, slot->hp_slot, slot->number,
>  			 ctrl->slot_device_offset);
>  		retval = pci_hp_register(hotplug_slot,
> -				ctrl->pci_dev->subordinate, slot->device, name);
> +					 ctrl->pci_dev->subordinate, slot->device, name);
>  		if (retval) {
>  			ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
>  				 retval);
> @@ -150,7 +149,7 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
>  		 __func__, slot_name(slot));
>  
>  	slot->attention_save = status;
> -	slot->hpc_ops->set_attention_status(slot, status);
> +	shpchp_set_attention_status(slot, status);
>  
>  	return 0;
>  }
> @@ -183,7 +182,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
>  	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
>  		 __func__, slot_name(slot));
>  
> -	retval = slot->hpc_ops->get_power_status(slot, value);
> +	retval = shpchp_get_power_status(slot, value);
>  	if (retval < 0)
>  		*value = slot->pwr_save;
>  
> @@ -198,7 +197,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
>  	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
>  		 __func__, slot_name(slot));
>  
> -	retval = slot->hpc_ops->get_attention_status(slot, value);
> +	retval = shpchp_get_attention_status(slot, value);
>  	if (retval < 0)
>  		*value = slot->attention_save;
>  
> @@ -213,7 +212,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
>  	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
>  		 __func__, slot_name(slot));
>  
> -	retval = slot->hpc_ops->get_latch_status(slot, value);
> +	retval = shpchp_get_latch_status(slot, value);
>  	if (retval < 0)
>  		*value = slot->latch_save;
>  
> @@ -228,7 +227,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
>  	ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
>  		 __func__, slot_name(slot));
>  
> -	retval = slot->hpc_ops->get_adapter_status(slot, value);
> +	retval = shpchp_get_adapter_status(slot, value);
>  	if (retval < 0)
>  		*value = slot->presence_save;
>  
> @@ -264,7 +263,7 @@ static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>  	if (!ctrl)
> -		goto err_out_none;
> +		return -ENODEV;
>  
>  	INIT_LIST_HEAD(&ctrl->slot_list);
>  
> @@ -293,10 +292,9 @@ static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  err_cleanup_slots:
>  	cleanup_slots(ctrl);
>  err_out_release_ctlr:
> -	ctrl->hpc_ops->release_ctlr(ctrl);
> +	shpchp_release_ctlr(ctrl);
>  err_out_free_ctrl:
>  	kfree(ctrl);
> -err_out_none:
>  	return -ENODEV;
>  }
>  
> @@ -306,7 +304,7 @@ static void shpc_remove(struct pci_dev *dev)
>  
>  	dev->shpc_managed = 0;
>  	shpchp_remove_ctrl_files(ctrl);
> -	ctrl->hpc_ops->release_ctlr(ctrl);
> +	shpchp_release_ctlr(ctrl);
>  	kfree(ctrl);
>  }
>  
> diff --git a/drivers/pci/hotplug/shpchp_ctrl.c b/drivers/pci/hotplug/shpchp_ctrl.c
> index 6a6705e0cf17..9e6437e318f5 100644
> --- a/drivers/pci/hotplug/shpchp_ctrl.c
> +++ b/drivers/pci/hotplug/shpchp_ctrl.c
> @@ -51,7 +51,7 @@ u8 shpchp_handle_attention_button(u8 hp_slot, struct controller *ctrl)
>  	ctrl_dbg(ctrl, "Attention button interrupt received\n");
>  
>  	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);
> -	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
> +	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
>  
>  	/*
>  	 *  Button pressed - See if need to TAKE ACTION!!!
> @@ -62,7 +62,6 @@ u8 shpchp_handle_attention_button(u8 hp_slot, struct controller *ctrl)
>  	queue_interrupt_event(p_slot, event_type);
>  
>  	return 0;
> -
>  }
>  
>  u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
> @@ -75,8 +74,8 @@ u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
>  	ctrl_dbg(ctrl, "Switch interrupt received\n");
>  
>  	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);
> -	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
> -	p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
> +	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
> +	shpchp_get_latch_status(p_slot, &getstatus);
>  	ctrl_dbg(ctrl, "Card present %x Power status %x\n",
>  		 p_slot->presence_save, p_slot->pwr_save);
>  
> @@ -116,7 +115,7 @@ u8 shpchp_handle_presence_change(u8 hp_slot, struct controller *ctrl)
>  	/*
>  	 * Save the presence state
>  	 */
> -	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
> +	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
>  	if (p_slot->presence_save) {
>  		/*
>  		 * Card Present
> @@ -148,7 +147,7 @@ u8 shpchp_handle_power_fault(u8 hp_slot, struct controller *ctrl)
>  
>  	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);
>  
> -	if (!(p_slot->hpc_ops->query_power_fault(p_slot))) {
> +	if (!(shpchp_query_power_fault(p_slot))) {
>  		/*
>  		 * Power fault Cleared
>  		 */
> @@ -173,15 +172,15 @@ u8 shpchp_handle_power_fault(u8 hp_slot, struct controller *ctrl)
>  }
>  
>  /* The following routines constitute the bulk of the
> -   hotplug controller logic
> + * hotplug controller logic
>   */
>  static int change_bus_speed(struct controller *ctrl, struct slot *p_slot,
> -		enum pci_bus_speed speed)
> +			    enum pci_bus_speed speed)
>  {
>  	int rc = 0;
>  
>  	ctrl_dbg(ctrl, "Change speed to %d\n", speed);
> -	rc = p_slot->hpc_ops->set_bus_speed_mode(p_slot, speed);
> +	rc = shpchp_set_bus_speed_mode(p_slot, speed);
>  	if (rc) {
>  		ctrl_err(ctrl, "%s: Issue of set bus speed mode command failed\n",
>  			 __func__);
> @@ -191,8 +190,8 @@ static int change_bus_speed(struct controller *ctrl, struct slot *p_slot,
>  }
>  
>  static int fix_bus_speed(struct controller *ctrl, struct slot *pslot,
> -		u8 flag, enum pci_bus_speed asp, enum pci_bus_speed bsp,
> -		enum pci_bus_speed msp)
> +			 u8 flag, enum pci_bus_speed asp, enum pci_bus_speed bsp,
> +			 enum pci_bus_speed msp)
>  {
>  	int rc = 0;
>  
> @@ -241,14 +240,14 @@ static int board_added(struct slot *p_slot)
>  		 __func__, p_slot->device, ctrl->slot_device_offset, hp_slot);
>  
>  	/* Power on slot without connecting to bus */
> -	rc = p_slot->hpc_ops->power_on_slot(p_slot);
> +	rc = shpchp_power_on_slot(p_slot);
>  	if (rc) {
>  		ctrl_err(ctrl, "Failed to power on slot\n");
>  		return -1;
>  	}
>  
> -	if ((ctrl->pci_dev->vendor == 0x8086) && (ctrl->pci_dev->device == 0x0332)) {
> -		rc = p_slot->hpc_ops->set_bus_speed_mode(p_slot, PCI_SPEED_33MHz);
> +	if (ctrl->pci_dev->vendor == 0x8086 && ctrl->pci_dev->device == 0x0332) {
> +		rc = shpchp_set_bus_speed_mode(p_slot, PCI_SPEED_33MHz);
>  		if (rc) {
>  			ctrl_err(ctrl, "%s: Issue of set bus speed mode command failed\n",
>  				 __func__);
> @@ -256,14 +255,14 @@ static int board_added(struct slot *p_slot)
>  		}
>  
>  		/* turn on board, blink green LED, turn off Amber LED */
> -		rc = p_slot->hpc_ops->slot_enable(p_slot);
> +		rc = shpchp_slot_enable(p_slot);
>  		if (rc) {
>  			ctrl_err(ctrl, "Issue of Slot Enable command failed\n");
>  			return rc;
>  		}
>  	}
>  
> -	rc = p_slot->hpc_ops->get_adapter_speed(p_slot, &asp);
> +	rc = shpchp_get_adapter_speed(p_slot, &asp);
>  	if (rc) {
>  		ctrl_err(ctrl, "Can't get adapter speed or bus mode mismatch\n");
>  		return WRONG_BUS_FREQUENCY;
> @@ -285,7 +284,7 @@ static int board_added(struct slot *p_slot)
>  		return rc;
>  
>  	/* turn on board, blink green LED, turn off Amber LED */
> -	rc = p_slot->hpc_ops->slot_enable(p_slot);
> +	rc = shpchp_slot_enable(p_slot);
>  	if (rc) {
>  		ctrl_err(ctrl, "Issue of Slot Enable command failed\n");
>  		return rc;
> @@ -313,13 +312,13 @@ static int board_added(struct slot *p_slot)
>  	p_slot->is_a_board = 0x01;
>  	p_slot->pwr_save = 1;
>  
> -	p_slot->hpc_ops->green_led_on(p_slot);
> +	shpchp_green_led_on(p_slot);
>  
>  	return 0;
>  
>  err_exit:
>  	/* turn off slot, turn on Amber LED, turn off Green LED */
> -	rc = p_slot->hpc_ops->slot_disable(p_slot);
> +	rc = shpchp_slot_disable(p_slot);
>  	if (rc) {
>  		ctrl_err(ctrl, "%s: Issue of Slot Disable command failed\n",
>  			 __func__);
> @@ -329,7 +328,6 @@ static int board_added(struct slot *p_slot)
>  	return(rc);
>  }
>  
> -
>  /**
>   * remove_board - Turns off slot and LEDs
>   * @p_slot: target &slot
> @@ -352,14 +350,14 @@ static int remove_board(struct slot *p_slot)
>  		p_slot->status = 0x01;
>  
>  	/* turn off slot, turn on Amber LED, turn off Green LED */
> -	rc = p_slot->hpc_ops->slot_disable(p_slot);
> +	rc = shpchp_slot_disable(p_slot);
>  	if (rc) {
>  		ctrl_err(ctrl, "%s: Issue of Slot Disable command failed\n",
>  			 __func__);
>  		return rc;
>  	}
>  
> -	rc = p_slot->hpc_ops->set_attention_status(p_slot, 0);
> +	rc = shpchp_set_attention_status(p_slot, 0);
>  	if (rc) {
>  		ctrl_err(ctrl, "Issue of Set Attention command failed\n");
>  		return rc;
> @@ -371,7 +369,6 @@ static int remove_board(struct slot *p_slot)
>  	return 0;
>  }
>  
> -
>  struct pushbutton_work_info {
>  	struct slot *p_slot;
>  	struct work_struct work;
> @@ -401,7 +398,7 @@ static void shpchp_pushbutton_thread(struct work_struct *work)
>  	case POWERON_STATE:
>  		mutex_unlock(&p_slot->lock);
>  		if (shpchp_enable_slot(p_slot))
> -			p_slot->hpc_ops->green_led_off(p_slot);
> +			shpchp_green_led_off(p_slot);
>  		mutex_lock(&p_slot->lock);
>  		p_slot->state = STATIC_STATE;
>  		break;
> @@ -446,10 +443,10 @@ void shpchp_queue_pushbutton_work(struct work_struct *work)
>  
>  static void update_slot_info(struct slot *slot)
>  {
> -	slot->hpc_ops->get_power_status(slot, &slot->pwr_save);
> -	slot->hpc_ops->get_attention_status(slot, &slot->attention_save);
> -	slot->hpc_ops->get_latch_status(slot, &slot->latch_save);
> -	slot->hpc_ops->get_adapter_status(slot, &slot->presence_save);
> +	shpchp_get_power_status(slot, &slot->pwr_save);
> +	shpchp_get_attention_status(slot, &slot->attention_save);
> +	shpchp_get_latch_status(slot, &slot->latch_save);
> +	shpchp_get_adapter_status(slot, &slot->presence_save);
>  }
>  
>  /*
> @@ -462,7 +459,7 @@ static void handle_button_press_event(struct slot *p_slot)
>  
>  	switch (p_slot->state) {
>  	case STATIC_STATE:
> -		p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
> +		shpchp_get_power_status(p_slot, &getstatus);
>  		if (getstatus) {
>  			p_slot->state = BLINKINGOFF_STATE;
>  			ctrl_info(ctrl, "PCI slot #%s - powering off due to button press\n",
> @@ -473,10 +470,10 @@ static void handle_button_press_event(struct slot *p_slot)
>  				  slot_name(p_slot));
>  		}
>  		/* blink green LED and turn off amber */
> -		p_slot->hpc_ops->green_led_blink(p_slot);
> -		p_slot->hpc_ops->set_attention_status(p_slot, 0);
> +		shpchp_green_led_blink(p_slot);
> +		shpchp_set_attention_status(p_slot, 0);
>  
> -		queue_delayed_work(p_slot->wq, &p_slot->work, 5*HZ);
> +		queue_delayed_work(p_slot->wq, &p_slot->work, 5 * HZ);
>  		break;
>  	case BLINKINGOFF_STATE:
>  	case BLINKINGON_STATE:
> @@ -489,10 +486,10 @@ static void handle_button_press_event(struct slot *p_slot)
>  			  slot_name(p_slot));
>  		cancel_delayed_work(&p_slot->work);
>  		if (p_slot->state == BLINKINGOFF_STATE)
> -			p_slot->hpc_ops->green_led_on(p_slot);
> +			shpchp_green_led_on(p_slot);
>  		else
> -			p_slot->hpc_ops->green_led_off(p_slot);
> -		p_slot->hpc_ops->set_attention_status(p_slot, 0);
> +			shpchp_green_led_off(p_slot);
> +		shpchp_set_attention_status(p_slot, 0);
>  		ctrl_info(ctrl, "PCI slot #%s - action canceled due to button press\n",
>  			  slot_name(p_slot));
>  		p_slot->state = STATIC_STATE;
> @@ -526,8 +523,8 @@ static void interrupt_event_handler(struct work_struct *work)
>  		break;
>  	case INT_POWER_FAULT:
>  		ctrl_dbg(p_slot->ctrl, "%s: Power fault\n", __func__);
> -		p_slot->hpc_ops->set_attention_status(p_slot, 1);
> -		p_slot->hpc_ops->green_led_off(p_slot);
> +		shpchp_set_attention_status(p_slot, 1);
> +		shpchp_green_led_off(p_slot);
>  		break;
>  	default:
>  		update_slot_info(p_slot);
> @@ -538,8 +535,7 @@ static void interrupt_event_handler(struct work_struct *work)
>  	kfree(info);
>  }
>  
> -
> -static int shpchp_enable_slot (struct slot *p_slot)
> +static int shpchp_enable_slot(struct slot *p_slot)
>  {
>  	u8 getstatus = 0;
>  	int rc, retval = -ENODEV;
> @@ -547,17 +543,17 @@ static int shpchp_enable_slot (struct slot *p_slot)
>  
>  	/* Check to see if (latch closed, card present, power off) */
>  	mutex_lock(&p_slot->ctrl->crit_sect);
> -	rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
> +	rc = shpchp_get_adapter_status(p_slot, &getstatus);
>  	if (rc || !getstatus) {
>  		ctrl_info(ctrl, "No adapter on slot(%s)\n", slot_name(p_slot));
>  		goto out;
>  	}
> -	rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
> +	rc = shpchp_get_latch_status(p_slot, &getstatus);
>  	if (rc || getstatus) {
>  		ctrl_info(ctrl, "Latch open on slot(%s)\n", slot_name(p_slot));
>  		goto out;
>  	}
> -	rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
> +	rc = shpchp_get_power_status(p_slot, &getstatus);
>  	if (rc || getstatus) {
>  		ctrl_info(ctrl, "Already enabled on slot(%s)\n",
>  			  slot_name(p_slot));
> @@ -567,26 +563,26 @@ static int shpchp_enable_slot (struct slot *p_slot)
>  	p_slot->is_a_board = 1;
>  
>  	/* We have to save the presence info for these slots */
> -	p_slot->hpc_ops->get_adapter_status(p_slot, &(p_slot->presence_save));
> -	p_slot->hpc_ops->get_power_status(p_slot, &(p_slot->pwr_save));
> +	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
> +	shpchp_get_power_status(p_slot, &p_slot->pwr_save);
>  	ctrl_dbg(ctrl, "%s: p_slot->pwr_save %x\n", __func__, p_slot->pwr_save);
> -	p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
> +	shpchp_get_latch_status(p_slot, &getstatus);
>  
>  	if ((p_slot->ctrl->pci_dev->vendor == PCI_VENDOR_ID_AMD &&
> -	     p_slot->ctrl->pci_dev->device == PCI_DEVICE_ID_AMD_POGO_7458)
> -	     && p_slot->ctrl->num_slots == 1) {
> +	     p_slot->ctrl->pci_dev->device == PCI_DEVICE_ID_AMD_POGO_7458) &&
> +	     p_slot->ctrl->num_slots == 1) {
>  		/* handle AMD POGO errata; this must be done before enable  */
>  		amd_pogo_errata_save_misc_reg(p_slot);
>  		retval = board_added(p_slot);
>  		/* handle AMD POGO errata; this must be done after enable  */
>  		amd_pogo_errata_restore_misc_reg(p_slot);
> -	} else
> +	} else {
>  		retval = board_added(p_slot);
> +	}
>  
>  	if (retval) {
> -		p_slot->hpc_ops->get_adapter_status(p_slot,
> -				&(p_slot->presence_save));
> -		p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
> +		shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
> +		shpchp_get_latch_status(p_slot, &getstatus);
>  	}
>  
>  	update_slot_info(p_slot);
> @@ -595,8 +591,7 @@ static int shpchp_enable_slot (struct slot *p_slot)
>  	return retval;
>  }
>  
> -
> -static int shpchp_disable_slot (struct slot *p_slot)
> +static int shpchp_disable_slot(struct slot *p_slot)
>  {
>  	u8 getstatus = 0;
>  	int rc, retval = -ENODEV;
> @@ -608,17 +603,17 @@ static int shpchp_disable_slot (struct slot *p_slot)
>  	/* Check to see if (latch closed, card present, power on) */
>  	mutex_lock(&p_slot->ctrl->crit_sect);
>  
> -	rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
> +	rc = shpchp_get_adapter_status(p_slot, &getstatus);
>  	if (rc || !getstatus) {
>  		ctrl_info(ctrl, "No adapter on slot(%s)\n", slot_name(p_slot));
>  		goto out;
>  	}
> -	rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
> +	rc = shpchp_get_latch_status(p_slot, &getstatus);
>  	if (rc || getstatus) {
>  		ctrl_info(ctrl, "Latch open on slot(%s)\n", slot_name(p_slot));
>  		goto out;
>  	}
> -	rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
> +	rc = shpchp_get_power_status(p_slot, &getstatus);
>  	if (rc || !getstatus) {
>  		ctrl_info(ctrl, "Already disabled on slot(%s)\n",
>  			  slot_name(p_slot));
> diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
> index 48e4daefc44a..012b9e3fe5b0 100644
> --- a/drivers/pci/hotplug/shpchp_hpc.c
> +++ b/drivers/pci/hotplug/shpchp_hpc.c
> @@ -167,7 +167,6 @@
>  
>  static irqreturn_t shpc_isr(int irq, void *dev_id);
>  static void start_int_poll_timer(struct controller *ctrl, int sec);
> -static int hpc_check_cmd_status(struct controller *ctrl);
>  
>  static inline u8 shpc_readb(struct controller *ctrl, int reg)
>  {
> @@ -317,7 +316,7 @@ static int shpc_write_cmd(struct slot *slot, u8 t_slot, u8 cmd)
>  	if (retval)
>  		goto out;
>  
> -	cmd_status = hpc_check_cmd_status(slot->ctrl);
> +	cmd_status = shpchp_check_cmd_status(slot->ctrl);
>  	if (cmd_status) {
>  		ctrl_err(ctrl, "Failed to issued command 0x%x (error code = %d)\n",
>  			 cmd, cmd_status);
> @@ -328,7 +327,7 @@ static int shpc_write_cmd(struct slot *slot, u8 t_slot, u8 cmd)
>  	return retval;
>  }
>  
> -static int hpc_check_cmd_status(struct controller *ctrl)
> +int shpchp_check_cmd_status(struct controller *ctrl)
>  {
>  	int retval = 0;
>  	u16 cmd_status = shpc_readw(ctrl, CMD_STATUS) & 0x000F;
> @@ -357,7 +356,7 @@ static int hpc_check_cmd_status(struct controller *ctrl)
>  }
>  
>  
> -static int hpc_get_attention_status(struct slot *slot, u8 *status)
> +int shpchp_get_attention_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
> @@ -381,7 +380,7 @@ static int hpc_get_attention_status(struct slot *slot, u8 *status)
>  	return 0;
>  }
>  
> -static int hpc_get_power_status(struct slot *slot, u8 *status)
> +int shpchp_get_power_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
> @@ -406,7 +405,7 @@ static int hpc_get_power_status(struct slot *slot, u8 *status)
>  }
>  
>  
> -static int hpc_get_latch_status(struct slot *slot, u8 *status)
> +int shpchp_get_latch_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
> @@ -416,7 +415,7 @@ static int hpc_get_latch_status(struct slot *slot, u8 *status)
>  	return 0;
>  }
>  
> -static int hpc_get_adapter_status(struct slot *slot, u8 *status)
> +int shpchp_get_adapter_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
> @@ -427,7 +426,7 @@ static int hpc_get_adapter_status(struct slot *slot, u8 *status)
>  	return 0;
>  }
>  
> -static int hpc_get_prog_int(struct slot *slot, u8 *prog_int)
> +int shpchp_get_prog_int(struct slot *slot, u8 *prog_int)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  
> @@ -436,7 +435,7 @@ static int hpc_get_prog_int(struct slot *slot, u8 *prog_int)
>  	return 0;
>  }
>  
> -static int hpc_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
> +int shpchp_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
>  {
>  	int retval = 0;
>  	struct controller *ctrl = slot->ctrl;
> @@ -444,7 +443,7 @@ static int hpc_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
>  	u8 m66_cap  = !!(slot_reg & MHZ66_CAP);
>  	u8 pi, pcix_cap;
>  
> -	retval = hpc_get_prog_int(slot, &pi);
> +	retval = shpchp_get_prog_int(slot, &pi);
>  	if (retval)
>  		return retval;
>  
> @@ -489,7 +488,7 @@ static int hpc_get_adapter_speed(struct slot *slot, enum pci_bus_speed *value)
>  	return retval;
>  }
>  
> -static int hpc_query_power_fault(struct slot *slot)
> +int shpchp_query_power_fault(struct slot *slot)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  	u32 slot_reg = shpc_readl(ctrl, SLOT_REG(slot->hp_slot));
> @@ -498,7 +497,7 @@ static int hpc_query_power_fault(struct slot *slot)
>  	return !(slot_reg & POWER_FAULT);
>  }
>  
> -static int hpc_set_attention_status(struct slot *slot, u8 value)
> +int shpchp_set_attention_status(struct slot *slot, u8 value)
>  {
>  	u8 slot_cmd = 0;
>  
> @@ -520,22 +519,22 @@ static int hpc_set_attention_status(struct slot *slot, u8 value)
>  }
>  
>  
> -static void hpc_set_green_led_on(struct slot *slot)
> +void shpchp_green_led_on(struct slot *slot)
>  {
>  	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_ON);
>  }
>  
> -static void hpc_set_green_led_off(struct slot *slot)
> +void shpchp_green_led_off(struct slot *slot)
>  {
>  	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_OFF);
>  }
>  
> -static void hpc_set_green_led_blink(struct slot *slot)
> +void shpchp_green_led_blink(struct slot *slot)
>  {
>  	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_BLINK);
>  }
>  
> -static void hpc_release_ctlr(struct controller *ctrl)
> +void shpchp_release_ctlr(struct controller *ctrl)
>  {
>  	int i;
>  	u32 slot_reg, serr_int;
> @@ -575,7 +574,7 @@ static void hpc_release_ctlr(struct controller *ctrl)
>  	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
>  }
>  
> -static int hpc_power_on_slot(struct slot *slot)
> +int shpchp_power_on_slot(struct slot *slot)
>  {
>  	int retval;
>  
> @@ -586,7 +585,7 @@ static int hpc_power_on_slot(struct slot *slot)
>  	return retval;
>  }
>  
> -static int hpc_slot_enable(struct slot *slot)
> +int shpchp_slot_enable(struct slot *slot)
>  {
>  	int retval;
>  
> @@ -599,7 +598,7 @@ static int hpc_slot_enable(struct slot *slot)
>  	return retval;
>  }
>  
> -static int hpc_slot_disable(struct slot *slot)
> +int shpchp_slot_disable(struct slot *slot)
>  {
>  	int retval;
>  
> @@ -681,7 +680,7 @@ static int shpc_get_cur_bus_speed(struct controller *ctrl)
>  }
>  
>  
> -static int hpc_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed value)
> +int shpchp_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed value)
>  {
>  	int retval;
>  	struct controller *ctrl = slot->ctrl;
> @@ -871,28 +870,6 @@ static int shpc_get_max_bus_speed(struct controller *ctrl)
>  	return retval;
>  }
>  
> -static const struct hpc_ops shpchp_hpc_ops = {
> -	.power_on_slot			= hpc_power_on_slot,
> -	.slot_enable			= hpc_slot_enable,
> -	.slot_disable			= hpc_slot_disable,
> -	.set_bus_speed_mode		= hpc_set_bus_speed_mode,
> -	.set_attention_status	= hpc_set_attention_status,
> -	.get_power_status		= hpc_get_power_status,
> -	.get_attention_status	= hpc_get_attention_status,
> -	.get_latch_status		= hpc_get_latch_status,
> -	.get_adapter_status		= hpc_get_adapter_status,
> -
> -	.get_adapter_speed		= hpc_get_adapter_speed,
> -	.get_prog_int			= hpc_get_prog_int,
> -
> -	.query_power_fault		= hpc_query_power_fault,
> -	.green_led_on			= hpc_set_green_led_on,
> -	.green_led_off			= hpc_set_green_led_off,
> -	.green_led_blink		= hpc_set_green_led_blink,
> -
> -	.release_ctlr			= hpc_release_ctlr,
> -};
> -
>  int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
>  {
>  	int rc = -1, num_slots = 0;
> @@ -978,8 +955,6 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
>  	/* Setup wait queue */
>  	init_waitqueue_head(&ctrl->queue);
>  
> -	ctrl->hpc_ops = &shpchp_hpc_ops;
> -
>  	/* Return PCI Controller Info */
>  	slot_config = shpc_readl(ctrl, SLOT_CONFIG);
>  	ctrl->slot_device_offset = (slot_config & FIRST_DEV_NUM) >> 8;
> -- 
> 2.45.1
> 

Re: [PATCH 1/2] PCI: shpchp: Remove hpc_ops

[ngn]

On Fri, Jul 12, 2024 at 05:44:25PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 21, 2024 at 09:35:00AM +0300, ngn wrote:
> > fix checpatch warnings and call hpc_ops functions directly, this has been
> > also done in pciehp with commit 82a9e79ef132
> 
> Trivial changes like checkpatch fixes should be in a separate patch
> from substantive things.  This helps make both easier to review.
> 
> It *might* be worthwhile to remove hpc_ops, although shpchp is pretty
> old and dusty by now, and it's hard to justify touching things like
> that because the risk of breaking something is relatively high and the
> benefit is relatively low.
> 
> The fact that shpchp is the only user of hpc_ops does make me a little
> more sympathetic to the idea of removing it, though.
> 

Thanks for the reply! I'll just remove hpc_ops then, and I'll send a separate
patch.

[PATCH 2/2] PCI: shpchp: Follow kernel coding style

[ngn]

fixing checkpatch warnings

Signed-off-by: ngn <ngn@ngn.tf>
---
 drivers/pci/hotplug/shpchp_hpc.c   | 91 +++++++++++++++---------------
 drivers/pci/hotplug/shpchp_sysfs.c |  7 +--
 2 files changed, 47 insertions(+), 51 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 012b9e3fe5b0..dd30e7e611a7 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c
@@ -47,24 +47,24 @@
 /*
  * Interrupt Locator Register definitions
  */
-#define CMD_INTR_PENDING	(1 << 0)
-#define SLOT_INTR_PENDING(i)	(1 << (i + 1))
+#define CMD_INTR_PENDING	BIT(0)
+#define SLOT_INTR_PENDING(i)	BIT((i) + 1)
 
 /*
  * Controller SERR-INT Register
  */
-#define GLOBAL_INTR_MASK	(1 << 0)
-#define GLOBAL_SERR_MASK	(1 << 1)
-#define COMMAND_INTR_MASK	(1 << 2)
-#define ARBITER_SERR_MASK	(1 << 3)
-#define COMMAND_DETECTED	(1 << 16)
-#define ARBITER_DETECTED	(1 << 17)
+#define GLOBAL_INTR_MASK	BIT(0)
+#define GLOBAL_SERR_MASK	BIT(1)
+#define COMMAND_INTR_MASK	BIT(2)
+#define ARBITER_SERR_MASK	BIT(3)
+#define COMMAND_DETECTED	BIT(16)
+#define ARBITER_DETECTED	BIT(17)
 #define SERR_INTR_RSVDZ_MASK	0xfffc0000
 
 /*
  * Logical Slot Register definitions
  */
-#define SLOT_REG(i)		(SLOT1 + (4 * i))
+#define SLOT_REG(i)		(SLOT1 + (4 * (i)))
 
 #define SLOT_STATE_SHIFT	(0)
 #define SLOT_STATE_MASK		(3 << 0)
@@ -78,27 +78,27 @@
 #define ATN_LED_STATE_ON	(1)
 #define ATN_LED_STATE_BLINK	(2)
 #define ATN_LED_STATE_OFF	(3)
-#define POWER_FAULT		(1 << 6)
-#define ATN_BUTTON		(1 << 7)
-#define MRL_SENSOR		(1 << 8)
-#define MHZ66_CAP		(1 << 9)
+#define POWER_FAULT		BIT(6)
+#define ATN_BUTTON		BIT(7)
+#define MRL_SENSOR		BIT(8)
+#define MHZ66_CAP		BIT(9)
 #define PRSNT_SHIFT		(10)
 #define PRSNT_MASK		(3 << 10)
 #define PCIX_CAP_SHIFT		(12)
 #define PCIX_CAP_MASK_PI1	(3 << 12)
 #define PCIX_CAP_MASK_PI2	(7 << 12)
-#define PRSNT_CHANGE_DETECTED	(1 << 16)
-#define ISO_PFAULT_DETECTED	(1 << 17)
-#define BUTTON_PRESS_DETECTED	(1 << 18)
-#define MRL_CHANGE_DETECTED	(1 << 19)
-#define CON_PFAULT_DETECTED	(1 << 20)
-#define PRSNT_CHANGE_INTR_MASK	(1 << 24)
-#define ISO_PFAULT_INTR_MASK	(1 << 25)
-#define BUTTON_PRESS_INTR_MASK	(1 << 26)
-#define MRL_CHANGE_INTR_MASK	(1 << 27)
-#define CON_PFAULT_INTR_MASK	(1 << 28)
-#define MRL_CHANGE_SERR_MASK	(1 << 29)
-#define CON_PFAULT_SERR_MASK	(1 << 30)
+#define PRSNT_CHANGE_DETECTED	BIT(16)
+#define ISO_PFAULT_DETECTED	BIT(17)
+#define BUTTON_PRESS_DETECTED	BIT(18)
+#define MRL_CHANGE_DETECTED	BIT(19)
+#define CON_PFAULT_DETECTED	BIT(20)
+#define PRSNT_CHANGE_INTR_MASK	BIT(24)
+#define ISO_PFAULT_INTR_MASK	BIT(25)
+#define BUTTON_PRESS_INTR_MASK	BIT(26)
+#define MRL_CHANGE_INTR_MASK	BIT(27)
+#define CON_PFAULT_INTR_MASK	BIT(28)
+#define MRL_CHANGE_SERR_MASK	BIT(29)
+#define CON_PFAULT_SERR_MASK	BIT(30)
 #define SLOT_REG_RSVDZ_MASK	((1 << 15) | (7 << 21))
 
 /*
@@ -228,7 +228,7 @@ static void int_poll_timeout(struct timer_list *t)
 static void start_int_poll_timer(struct controller *ctrl, int sec)
 {
 	/* Clamp to sane value */
-	if ((sec <= 0) || (sec > 60))
+	if (sec <= 0 || sec > 60)
 		sec = 2;
 
 	ctrl->poll_timer.expires = jiffies + sec * HZ;
@@ -238,6 +238,7 @@ static void start_int_poll_timer(struct controller *ctrl, int sec)
 static inline int is_ctrl_busy(struct controller *ctrl)
 {
 	u16 cmd_status = shpc_readw(ctrl, CMD_STATUS);
+
 	return cmd_status & 0x1;
 }
 
@@ -272,7 +273,7 @@ static inline int shpc_wait_cmd(struct controller *ctrl)
 		rc = shpc_poll_ctrl_busy(ctrl);
 	else
 		rc = wait_event_interruptible_timeout(ctrl->queue,
-						!is_ctrl_busy(ctrl), timeout);
+						      !is_ctrl_busy(ctrl), timeout);
 	if (!rc && is_ctrl_busy(ctrl)) {
 		retval = -EIO;
 		ctrl_err(ctrl, "Command not completed in 1000 msec\n");
@@ -355,7 +356,6 @@ int shpchp_check_cmd_status(struct controller *ctrl)
 	return retval;
 }
 
-
 int shpchp_get_attention_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
@@ -404,7 +404,6 @@ int shpchp_get_power_status(struct slot *slot, u8 *status)
 	return 0;
 }
 
-
 int shpchp_get_latch_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
@@ -502,23 +501,22 @@ int shpchp_set_attention_status(struct slot *slot, u8 value)
 	u8 slot_cmd = 0;
 
 	switch (value) {
-		case 0:
-			slot_cmd = SET_ATTN_OFF;	/* OFF */
-			break;
-		case 1:
-			slot_cmd = SET_ATTN_ON;		/* ON */
-			break;
-		case 2:
-			slot_cmd = SET_ATTN_BLINK;	/* BLINK */
-			break;
-		default:
-			return -1;
+	case 0:
+		slot_cmd = SET_ATTN_OFF;	/* OFF */
+		break;
+	case 1:
+		slot_cmd = SET_ATTN_ON;		/* ON */
+		break;
+	case 2:
+		slot_cmd = SET_ATTN_BLINK;	/* BLINK */
+		break;
+	default:
+		return -1;
 	}
 
 	return shpc_write_cmd(slot, slot->hp_slot, slot_cmd);
 }
 
-
 void shpchp_green_led_on(struct slot *slot)
 {
 	shpc_write_cmd(slot, slot->hp_slot, SET_PWR_ON);
@@ -563,9 +561,9 @@ void shpchp_release_ctlr(struct controller *ctrl)
 	serr_int &= ~SERR_INTR_RSVDZ_MASK;
 	shpc_writel(ctrl, SERR_INTR_ENABLE, serr_int);
 
-	if (shpchp_poll_mode)
+	if (shpchp_poll_mode) {
 		del_timer(&ctrl->poll_timer);
-	else {
+	} else {
 		free_irq(ctrl->pci_dev->irq, ctrl);
 		pci_disable_msi(ctrl->pci_dev);
 	}
@@ -591,7 +589,7 @@ int shpchp_slot_enable(struct slot *slot)
 
 	/* Slot - Enable, Power Indicator - Blink, Attention Indicator - Off */
 	retval = shpc_write_cmd(slot, slot->hp_slot,
-			SET_SLOT_ENABLE | SET_PWR_BLINK | SET_ATTN_OFF);
+				SET_SLOT_ENABLE | SET_PWR_BLINK | SET_ATTN_OFF);
 	if (retval)
 		ctrl_err(slot->ctrl, "%s: Write command failed!\n", __func__);
 
@@ -620,7 +618,7 @@ static int shpc_get_cur_bus_speed(struct controller *ctrl)
 	u8 pi = shpc_readb(ctrl, PROG_INTERFACE);
 	u8 speed_mode = (pi == 2) ? (sec_bus_reg & 0xF) : (sec_bus_reg & 0x7);
 
-	if ((pi == 1) && (speed_mode > 4)) {
+	if (pi == 1 && speed_mode > 4) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -679,7 +677,6 @@ static int shpc_get_cur_bus_speed(struct controller *ctrl)
 	return retval;
 }
 
-
 int shpchp_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed value)
 {
 	int retval;
@@ -687,7 +684,7 @@ int shpchp_set_bus_speed_mode(struct slot *slot, enum pci_bus_speed value)
 	u8 pi, cmd;
 
 	pi = shpc_readb(ctrl, PROG_INTERFACE);
-	if ((pi == 1) && (value > PCI_SPEED_133MHz_PCIX))
+	if (pi == 1 && value > PCI_SPEED_133MHz_PCIX)
 		return -EINVAL;
 
 	switch (value) {
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c b/drivers/pci/hotplug/shpchp_sysfs.c
index 01d47a42da04..06c928cfbc81 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -18,7 +18,6 @@
 #include <linux/pci.h>
 #include "shpchp.h"
 
-
 /* A few routines that create sysfs entries for the hot plug controller */
 
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, char *buf)
@@ -35,7 +34,7 @@ static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, char
 	len += sysfs_emit_at(buf, len, "Free resources: memory\n");
 	pci_bus_for_each_resource(bus, res) {
 		if (res && (res->flags & IORESOURCE_MEM) &&
-				!(res->flags & IORESOURCE_PREFETCH)) {
+		    !(res->flags & IORESOURCE_PREFETCH)) {
 			len += sysfs_emit_at(buf, len,
 					     "start = %8.8llx, length = %8.8llx\n",
 					     (unsigned long long)res->start,
@@ -45,7 +44,7 @@ static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, char
 	len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
 	pci_bus_for_each_resource(bus, res) {
 		if (res && (res->flags & IORESOURCE_MEM) &&
-			       (res->flags & IORESOURCE_PREFETCH)) {
+		    (res->flags & IORESOURCE_PREFETCH)) {
 			len += sysfs_emit_at(buf, len,
 					     "start = %8.8llx, length = %8.8llx\n",
 					     (unsigned long long)res->start,
@@ -73,7 +72,7 @@ static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, char
 
 	return len;
 }
-static DEVICE_ATTR(ctrl, S_IRUGO, show_ctrl, NULL);
+static DEVICE_ATTR(ctrl, 0444, show_ctrl, NULL);
 
 int shpchp_create_ctrl_files(struct controller *ctrl)
 {
-- 
2.45.1