Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] Simplify PCIe hotplug indicator control
@ 2019-08-11 13:29 Denis Efremov
  2019-08-11 13:29 ` [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Denis Efremov @ 2019-08-11 13:29 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Denis Efremov

PCIe defines two optional hotplug indicators: a Power indicator and an
Attention indicator. Both are controlled by the same register, and each
can be on, off or blinking. The current interfaces
(pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
non-uniform and require two register writes in many cases where we could
do one.

This patchset introduces the new function pciehp_set_indicators(). It
allows one to set two indicators with a single register write. All
calls to previous interfaces (pciehp_green_led_* and
pciehp_set_attention_status()) are replaced with a new one.

Denis Efremov (4):
  PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  PCI: pciehp: Switch LED indicators with a single write
  PCI: pciehp: Replace pciehp_set_attention_status()
  PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()

 drivers/pci/hotplug/pciehp.h      | 31 +++++++++--
 drivers/pci/hotplug/pciehp_ctrl.c | 14 ++---
 drivers/pci/hotplug/pciehp_hpc.c  | 87 +++++++++++++------------------
 3 files changed, 69 insertions(+), 63 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  2019-08-11 13:29 [PATCH 0/4] Simplify PCIe hotplug indicator control Denis Efremov
@ 2019-08-11 13:29 ` Denis Efremov
  2019-08-11 16:07   ` Lukas Wunner
  2019-08-11 13:29 ` [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Denis Efremov @ 2019-08-11 13:29 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Denis Efremov

This commit adds pciehp_set_indicators() to set power and attention
indicators with a single register write. enum pciehp_indicator
introduced to switch between the indicators statuses. Attention
indicator statuses are explicitly set with values in the enum to
transparently comply with pciehp_set_attention_status() from
pciehp_hpc.c and set_attention_status() from pciehp_core.c

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h     | 15 ++++++++++
 drivers/pci/hotplug/pciehp_hpc.c | 49 ++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 8c51a04b8083..92ff65132711 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -150,6 +150,18 @@ struct controller {
 #define NO_CMD_CMPL(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
 #define PSN(ctrl)		(((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)
 
+enum pciehp_indicator {
+	// Explicit values to match set_attention_status interface
+	ATTN_NONE = -1,
+	ATTN_OFF = 0,
+	ATTN_ON = 1,
+	ATTN_BLINK = 2,
+	PWR_NONE,
+	PWR_OFF,
+	PWR_ON,
+	PWR_BLINK
+};
+
 void pciehp_request(struct controller *ctrl, int action);
 void pciehp_handle_button_press(struct controller *ctrl);
 void pciehp_handle_disable_request(struct controller *ctrl);
@@ -167,6 +179,9 @@ int pciehp_power_on_slot(struct controller *ctrl);
 void pciehp_power_off_slot(struct controller *ctrl);
 void pciehp_get_power_status(struct controller *ctrl, u8 *status);
 
+void pciehp_set_indicators(struct controller *ctrl,
+			   enum pciehp_indicator pwr,
+			   enum pciehp_indicator attn);
 void pciehp_set_attention_status(struct controller *ctrl, u8 status);
 void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
 int pciehp_query_power_fault(struct controller *ctrl);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bd990e3371e3..027e3864f632 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -443,6 +443,55 @@ void pciehp_set_attention_status(struct controller *ctrl, u8 value)
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
 }
 
+void pciehp_set_indicators(struct controller *ctrl,
+			   enum pciehp_indicator pwr,
+			   enum pciehp_indicator attn)
+{
+	u16 cmd = 0;
+	bool pwr_none = (pwr == PWR_NONE);
+	bool attn_none = (attn == ATTN_NONE);
+	bool pwr_led = PWR_LED(ctrl);
+	bool attn_led = ATTN_LED(ctrl);
+
+	if ((!pwr_led && !attn_led) || (pwr_none && attn_none) ||
+	    (!attn_led && pwr_none) || (!pwr_led && attn_none))
+		return;
+
+	switch (pwr) {
+	case PWR_OFF:
+		cmd = PCI_EXP_SLTCTL_PWR_IND_OFF;
+		break;
+	case PWR_ON:
+		cmd = PCI_EXP_SLTCTL_PWR_IND_ON;
+		break;
+	case PWR_BLINK:
+		cmd = PCI_EXP_SLTCTL_PWR_IND_BLINK;
+		break;
+	default:
+		break;
+	}
+
+	switch (attn) {
+	case ATTN_OFF:
+		cmd |= PCI_EXP_SLTCTL_ATTN_IND_OFF;
+		break;
+	case ATTN_ON:
+		cmd |= PCI_EXP_SLTCTL_ATTN_IND_ON;
+		break;
+	case ATTN_BLINK:
+		cmd |= PCI_EXP_SLTCTL_ATTN_IND_BLINK;
+		break;
+	default:
+		break;
+	}
+
+	pcie_write_cmd_nowait(ctrl, cmd,
+			      PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC);
+	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
+		 cmd);
+}
+
 void pciehp_green_led_on(struct controller *ctrl)
 {
 	if (!PWR_LED(ctrl))
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write
  2019-08-11 13:29 [PATCH 0/4] Simplify PCIe hotplug indicator control Denis Efremov
  2019-08-11 13:29 ` [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
@ 2019-08-11 13:29 ` Denis Efremov
  2019-08-11 16:11   ` Lukas Wunner
  2019-08-11 13:29 ` [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
  2019-08-11 13:29 ` [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
  3 siblings, 1 reply; 11+ messages in thread
From: Denis Efremov @ 2019-08-11 13:29 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Denis Efremov

This patch replaces all consecutive switches of power and attention
indicators with pciehp_set_indicators() call. Thus, performing only
single write to a register.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 14 +++++---------
 drivers/pci/hotplug/pciehp_hpc.c  |  3 +--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 631ced0ab28a..258a4060466d 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -42,8 +42,7 @@ static void set_slot_off(struct controller *ctrl)
 		msleep(1000);
 	}
 
-	pciehp_green_led_off(ctrl);
-	pciehp_set_attention_status(ctrl, 1);
+	pciehp_set_indicators(ctrl, PWR_OFF, ATTN_ON);
 }
 
 /**
@@ -90,8 +89,7 @@ static int board_added(struct controller *ctrl)
 		}
 	}
 
-	pciehp_green_led_on(ctrl);
-	pciehp_set_attention_status(ctrl, 0);
+	pciehp_set_indicators(ctrl, PWR_ON, ATTN_OFF);
 	return 0;
 
 err_exit:
@@ -172,8 +170,7 @@ void pciehp_handle_button_press(struct controller *ctrl)
 				  slot_name(ctrl));
 		}
 		/* blink green LED and turn off amber */
-		pciehp_green_led_blink(ctrl);
-		pciehp_set_attention_status(ctrl, 0);
+		pciehp_set_indicators(ctrl, PWR_BLINK, ATTN_OFF);
 		schedule_delayed_work(&ctrl->button_work, 5 * HZ);
 		break;
 	case BLINKINGOFF_STATE:
@@ -187,12 +184,11 @@ void pciehp_handle_button_press(struct controller *ctrl)
 		cancel_delayed_work(&ctrl->button_work);
 		if (ctrl->state == BLINKINGOFF_STATE) {
 			ctrl->state = ON_STATE;
-			pciehp_green_led_on(ctrl);
+			pciehp_set_indicators(ctrl, PWR_ON, ATTN_OFF);
 		} else {
 			ctrl->state = OFF_STATE;
-			pciehp_green_led_off(ctrl);
+			pciehp_set_indicators(ctrl, PWR_OFF, ATTN_OFF);
 		}
-		pciehp_set_attention_status(ctrl, 0);
 		ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
 			  slot_name(ctrl));
 		break;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 027e3864f632..e90cb2152e8f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -687,8 +687,7 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
 		ctrl->power_fault_detected = 1;
 		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(ctrl));
-		pciehp_set_attention_status(ctrl, 1);
-		pciehp_green_led_off(ctrl);
+		pciehp_set_indicators(ctrl, PWR_OFF, ATTN_ON);
 	}
 
 	/*
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status()
  2019-08-11 13:29 [PATCH 0/4] Simplify PCIe hotplug indicator control Denis Efremov
  2019-08-11 13:29 ` [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
  2019-08-11 13:29 ` [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
@ 2019-08-11 13:29 ` Denis Efremov
  2019-08-11 16:28   ` Lukas Wunner
  2019-08-11 13:29 ` [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
  3 siblings, 1 reply; 11+ messages in thread
From: Denis Efremov @ 2019-08-11 13:29 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Denis Efremov

This patch replaces pciehp_set_attention_status() with
pciehp_set_indicators().

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h     |  4 +++-
 drivers/pci/hotplug/pciehp_hpc.c | 25 -------------------------
 2 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 92ff65132711..01ea095aa533 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -182,7 +182,6 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status);
 void pciehp_set_indicators(struct controller *ctrl,
 			   enum pciehp_indicator pwr,
 			   enum pciehp_indicator attn);
-void pciehp_set_attention_status(struct controller *ctrl, u8 status);
 void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
 int pciehp_query_power_fault(struct controller *ctrl);
 void pciehp_green_led_on(struct controller *ctrl);
@@ -201,6 +200,9 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status);
 int pciehp_set_raw_indicator_status(struct hotplug_slot *h_slot, u8 status);
 int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
 
+#define pciehp_set_attention_status(crtl, status) \
+	pciehp_set_indicators(ctrl, PWR_NONE, status)
+
 static inline const char *slot_name(struct controller *ctrl)
 {
 	return hotplug_slot_name(&ctrl->hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index e90cb2152e8f..beff1120afef 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -418,31 +418,6 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot,
 	return 0;
 }
 
-void pciehp_set_attention_status(struct controller *ctrl, u8 value)
-{
-	u16 slot_cmd;
-
-	if (!ATTN_LED(ctrl))
-		return;
-
-	switch (value) {
-	case 0:		/* turn off */
-		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_OFF;
-		break;
-	case 1:		/* turn on */
-		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_ON;
-		break;
-	case 2:		/* turn blink */
-		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_BLINK;
-		break;
-	default:
-		return;
-	}
-	pcie_write_cmd_nowait(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
-}
-
 void pciehp_set_indicators(struct controller *ctrl,
 			   enum pciehp_indicator pwr,
 			   enum pciehp_indicator attn)
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
  2019-08-11 13:29 [PATCH 0/4] Simplify PCIe hotplug indicator control Denis Efremov
                   ` (2 preceding siblings ...)
  2019-08-11 13:29 ` [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
@ 2019-08-11 13:29 ` Denis Efremov
  2019-08-11 16:29   ` Lukas Wunner
  3 siblings, 1 reply; 11+ messages in thread
From: Denis Efremov @ 2019-08-11 13:29 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Denis Efremov

This patch replaces pciehp_green_led_{on,off,blink}() with
pciehp_set_indicators().

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h     | 12 ++++++++---
 drivers/pci/hotplug/pciehp_hpc.c | 36 --------------------------------
 2 files changed, 9 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 01ea095aa533..7ae16ad1a8a7 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -184,9 +184,6 @@ void pciehp_set_indicators(struct controller *ctrl,
 			   enum pciehp_indicator attn);
 void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
 int pciehp_query_power_fault(struct controller *ctrl);
-void pciehp_green_led_on(struct controller *ctrl);
-void pciehp_green_led_off(struct controller *ctrl);
-void pciehp_green_led_blink(struct controller *ctrl);
 bool pciehp_card_present(struct controller *ctrl);
 bool pciehp_card_present_or_link_active(struct controller *ctrl);
 int pciehp_check_link_status(struct controller *ctrl);
@@ -203,6 +200,15 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
 #define pciehp_set_attention_status(crtl, status) \
 	pciehp_set_indicators(ctrl, PWR_NONE, status)
 
+#define pciehp_green_led_on(ctrl) \
+	pciehp_set_indicators(ctrl, PWR_ON, ATTN_NONE)
+
+#define pciehp_green_led_off(ctrl) \
+	pciehp_set_indicators(ctrl, PWR_OFF, ATTN_NONE)
+
+#define pciehp_green_led_blink(ctrl) \
+	pciehp_set_indicators(ctrl, PWR_BLINK, ATTN_NONE)
+
 static inline const char *slot_name(struct controller *ctrl)
 {
 	return hotplug_slot_name(&ctrl->hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index beff1120afef..13cc417493f8 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -467,42 +467,6 @@ void pciehp_set_indicators(struct controller *ctrl,
 		 cmd);
 }
 
-void pciehp_green_led_on(struct controller *ctrl)
-{
-	if (!PWR_LED(ctrl))
-		return;
-
-	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
-			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_ON);
-}
-
-void pciehp_green_led_off(struct controller *ctrl)
-{
-	if (!PWR_LED(ctrl))
-		return;
-
-	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
-			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_OFF);
-}
-
-void pciehp_green_led_blink(struct controller *ctrl)
-{
-	if (!PWR_LED(ctrl))
-		return;
-
-	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
-			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_BLINK);
-}
-
 int pciehp_power_on_slot(struct controller *ctrl)
 {
 	struct pci_dev *pdev = ctrl_dev(ctrl);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  2019-08-11 13:29 ` [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
@ 2019-08-11 16:07   ` Lukas Wunner
  2019-08-11 16:32     ` Lukas Wunner
  2019-08-11 18:26     ` Denis Efremov
  0 siblings, 2 replies; 11+ messages in thread
From: Lukas Wunner @ 2019-08-11 16:07 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Sun, Aug 11, 2019 at 04:29:42PM +0300, Denis Efremov wrote:
> This commit adds pciehp_set_indicators() to set power and attention

Nit:  "This commit ..." is superfluous, just say "Add ...".


> indicators with a single register write. enum pciehp_indicator
> introduced to switch between the indicators statuses. Attention
> indicator statuses are explicitly set with values in the enum to
> transparently comply with pciehp_set_attention_status() from
> pciehp_hpc.c and set_attention_status() from pciehp_core.c

Please document the motivation of the change (the "why").

One motivation might be to avoid waiting twice for Command Complete.

Another motivation might be to change both LEDs at the same time
in a glitch-free manner, thereby achieving a smoother user experience.


> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> +enum pciehp_indicator {
> +	// Explicit values to match set_attention_status interface

Kernel coding style is typically /* */, not //.


> +	ATTN_NONE = -1,
> +	ATTN_OFF = 0,
> +	ATTN_ON = 1,
> +	ATTN_BLINK = 2,
> +	PWR_NONE,
> +	PWR_OFF,
> +	PWR_ON,
> +	PWR_BLINK
> +};

I'd suggest using the same values that are written to the register, i.e.:

enum pciehp_indicator {
	ATTN_NONE  = -1,
	ATTN_ON    =  1,
	ATTN_BLINK =  2,
	ATTN_OFF   =  3,
	PWR_NONE   = -1,
	PWR_ON     =  1,
	PWR_BLINK  =  2,
	PWR_OFF    =  3,
};

Then you can just shift the values to the proper offset and don't need
a translation between enum pciehp_indicator and register value.


> +void pciehp_set_indicators(struct controller *ctrl,
> +			   enum pciehp_indicator pwr,
> +			   enum pciehp_indicator attn)
> +{
> +	u16 cmd = 0;
> +	bool pwr_none = (pwr == PWR_NONE);
> +	bool attn_none = (attn == ATTN_NONE);
> +	bool pwr_led = PWR_LED(ctrl);
> +	bool attn_led = ATTN_LED(ctrl);
> +
> +	if ((!pwr_led && !attn_led) || (pwr_none && attn_none) ||
> +	    (!attn_led && pwr_none) || (!pwr_led && attn_none))
> +		return;

I'd suggest the following simpler construct:

	if (!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
	    !ATTN_LED(ctrl) || attn == ATTN_NONE))
		return;


> +	switch (pwr) {
> +	case PWR_OFF:
> +		cmd = PCI_EXP_SLTCTL_PWR_IND_OFF;
> +		break;
> +	case PWR_ON:
> +		cmd = PCI_EXP_SLTCTL_PWR_IND_ON;
> +		break;
> +	case PWR_BLINK:
> +		cmd = PCI_EXP_SLTCTL_PWR_IND_BLINK;
> +		break;
> +	default:
> +		break;
> +	}

If you follow my suggestion above to use the register value for "pwr",
then you can just fold all three cases into one, i.e.

	case PWR_ON:
	case PWR_BLINK:
	case PWR_OFF:
		cmd = pwr << 8;
		mask |= PCI_EXP_SLTCTL_PIC;
		break;

Feel free to add a PCI_EXP_SLTCTL_PWR_IND_OFFSET macro for the offset 8.
Add a "u16 mask = 0" to the top of the function and pass "mask" to
pcie_write_cmd_nowait().

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write
  2019-08-11 13:29 ` [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
@ 2019-08-11 16:11   ` Lukas Wunner
  0 siblings, 0 replies; 11+ messages in thread
From: Lukas Wunner @ 2019-08-11 16:11 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Sun, Aug 11, 2019 at 04:29:43PM +0300, Denis Efremov wrote:
> This patch replaces all consecutive switches of power and attention
> indicators with pciehp_set_indicators() call. Thus, performing only
> single write to a register.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status()
  2019-08-11 13:29 ` [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
@ 2019-08-11 16:28   ` Lukas Wunner
  0 siblings, 0 replies; 11+ messages in thread
From: Lukas Wunner @ 2019-08-11 16:28 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Sun, Aug 11, 2019 at 04:29:44PM +0300, Denis Efremov wrote:
> +#define pciehp_set_attention_status(crtl, status) \
> +	pciehp_set_indicators(ctrl, PWR_NONE, status)
> +

There's a typo here, s/crtl/ctrl/.

With that addressed,

Reviewed-by: Lukas Wunner <lukas@wunner.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
  2019-08-11 13:29 ` [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
@ 2019-08-11 16:29   ` Lukas Wunner
  0 siblings, 0 replies; 11+ messages in thread
From: Lukas Wunner @ 2019-08-11 16:29 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Sun, Aug 11, 2019 at 04:29:45PM +0300, Denis Efremov wrote:
> This patch replaces pciehp_green_led_{on,off,blink}() with
> pciehp_set_indicators().
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  2019-08-11 16:07   ` Lukas Wunner
@ 2019-08-11 16:32     ` Lukas Wunner
  2019-08-11 18:26     ` Denis Efremov
  1 sibling, 0 replies; 11+ messages in thread
From: Lukas Wunner @ 2019-08-11 16:32 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Sun, Aug 11, 2019 at 06:07:55PM +0200, Lukas Wunner wrote:
> 	if (!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
> 	    !ATTN_LED(ctrl) || attn == ATTN_NONE))
> 		return;

Forgot an opening brace in two spots here, sorry.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  2019-08-11 16:07   ` Lukas Wunner
  2019-08-11 16:32     ` Lukas Wunner
@ 2019-08-11 18:26     ` Denis Efremov
  1 sibling, 0 replies; 11+ messages in thread
From: Denis Efremov @ 2019-08-11 18:26 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

Thank you for the review, I will send v2.

On 11.08.2019 19:07, Lukas Wunner wrote:
> On Sun, Aug 11, 2019 at 04:29:42PM +0300, Denis Efremov wrote:
>> This commit adds pciehp_set_indicators() to set power and attention
> 
> Nit:  "This commit ..." is superfluous, just say "Add ...".
> 
> 
>> indicators with a single register write. enum pciehp_indicator
>> introduced to switch between the indicators statuses. Attention
>> indicator statuses are explicitly set with values in the enum to
>> transparently comply with pciehp_set_attention_status() from
>> pciehp_hpc.c and set_attention_status() from pciehp_core.c
> 
> Please document the motivation of the change (the "why").
> 
> One motivation might be to avoid waiting twice for Command Complete.
> 
> Another motivation might be to change both LEDs at the same time
> in a glitch-free manner, thereby achieving a smoother user experience.
> 
> 
>> --- a/drivers/pci/hotplug/pciehp.h
>> +++ b/drivers/pci/hotplug/pciehp.h
>> +enum pciehp_indicator {
>> +	// Explicit values to match set_attention_status interface
> 
> Kernel coding style is typically /* */, not //.
> 
> 
>> +	ATTN_NONE = -1,
>> +	ATTN_OFF = 0,
>> +	ATTN_ON = 1,
>> +	ATTN_BLINK = 2,
>> +	PWR_NONE,
>> +	PWR_OFF,
>> +	PWR_ON,
>> +	PWR_BLINK
>> +};
> 
> I'd suggest using the same values that are written to the register, i.e.:
> 
> enum pciehp_indicator {
> 	ATTN_NONE  = -1,
> 	ATTN_ON    =  1,
> 	ATTN_BLINK =  2,
> 	ATTN_OFF   =  3,
> 	PWR_NONE   = -1,
> 	PWR_ON     =  1,
> 	PWR_BLINK  =  2,
> 	PWR_OFF    =  3,
> };
> 
> Then you can just shift the values to the proper offset and don't need
> a translation between enum pciehp_indicator and register value.
> 
> 
>> +void pciehp_set_indicators(struct controller *ctrl,
>> +			   enum pciehp_indicator pwr,
>> +			   enum pciehp_indicator attn)
>> +{
>> +	u16 cmd = 0;
>> +	bool pwr_none = (pwr == PWR_NONE);
>> +	bool attn_none = (attn == ATTN_NONE);
>> +	bool pwr_led = PWR_LED(ctrl);
>> +	bool attn_led = ATTN_LED(ctrl);
>> +
>> +	if ((!pwr_led && !attn_led) || (pwr_none && attn_none) ||
>> +	    (!attn_led && pwr_none) || (!pwr_led && attn_none))
>> +		return;
> 
> I'd suggest the following simpler construct:
> 
> 	if (!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
> 	    !ATTN_LED(ctrl) || attn == ATTN_NONE))
> 		return;
> 
> 
>> +	switch (pwr) {
>> +	case PWR_OFF:
>> +		cmd = PCI_EXP_SLTCTL_PWR_IND_OFF;
>> +		break;
>> +	case PWR_ON:
>> +		cmd = PCI_EXP_SLTCTL_PWR_IND_ON;
>> +		break;
>> +	case PWR_BLINK:
>> +		cmd = PCI_EXP_SLTCTL_PWR_IND_BLINK;
>> +		break;
>> +	default:
>> +		break;
>> +	}
> 
> If you follow my suggestion above to use the register value for "pwr",
> then you can just fold all three cases into one, i.e.
> 
> 	case PWR_ON:
> 	case PWR_BLINK:
> 	case PWR_OFF:
> 		cmd = pwr << 8;
> 		mask |= PCI_EXP_SLTCTL_PIC;
> 		break;
> 
> Feel free to add a PCI_EXP_SLTCTL_PWR_IND_OFFSET macro for the offset 8.
> Add a "u16 mask = 0" to the top of the function and pass "mask" to
> pcie_write_cmd_nowait().
> 
> Thanks,
> 
> Lukas
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-11 13:29 [PATCH 0/4] Simplify PCIe hotplug indicator control Denis Efremov
2019-08-11 13:29 ` [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
2019-08-11 16:07   ` Lukas Wunner
2019-08-11 16:32     ` Lukas Wunner
2019-08-11 18:26     ` Denis Efremov
2019-08-11 13:29 ` [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
2019-08-11 16:11   ` Lukas Wunner
2019-08-11 13:29 ` [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
2019-08-11 16:28   ` Lukas Wunner
2019-08-11 13:29 ` [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
2019-08-11 16:29   ` Lukas Wunner

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox