[PATCH 0/4] Simplify PCIe hotplug indicator control

[PATCH 0/4] Simplify PCIe hotplug indicator control

[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

[PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[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

[PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write

[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

[PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status()

[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

[PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()

[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

Re: [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Lukas Wunner]

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

Re: [PATCH 2/4] PCI: pciehp: Switch LED indicators with a single write

[Lukas Wunner]

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>

Re: [PATCH 3/4] PCI: pciehp: Replace pciehp_set_attention_status()

[Lukas Wunner]

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>

Re: [PATCH 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()

[Lukas Wunner]

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>

Re: [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Lukas Wunner]

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.

Re: [PATCH 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Denis Efremov]

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
>