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
>
