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