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
>