On 06.09.2019 00:01, Bjorn Helgaas wrote:
> On Tue, Sep 03, 2019 at 02:10:17PM +0300, Denis Efremov wrote:
>> 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. Thus,
>> the amount of duplicated code for setting indicators is reduced.
>>
>> Changes in v4:
>> - Changed the inputs validation in pciehp_set_indicators()
>> - Moved PCI_EXP_SLTCTL_ATTN_IND_NONE, PCI_EXP_SLTCTL_PWR_IND_NONE
>> to drivers/pci/hotplug/pciehp.h and set to -1 for not interfering
>> with reserved values in the PCIe Base spec
>> - Added set_power_indicator define
>>
>> Changes in v3:
>> - Changed pciehp_set_indicators() to work with existing
>> PCI_EXP_SLTCTL_* macros
>> - Reworked the inputs validation in pciehp_set_indicators()
>> - Removed pciehp_set_attention_status() and pciehp_green_led_*()
>> completely
>>
>> 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: Remove pciehp_set_attention_status()
>> PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
>>
>> drivers/pci/hotplug/pciehp.h | 12 ++++--
>> drivers/pci/hotplug/pciehp_core.c | 7 ++-
>> drivers/pci/hotplug/pciehp_ctrl.c | 26 +++++------
>> drivers/pci/hotplug/pciehp_hpc.c | 72 +++++++------------------------
>> include/uapi/linux/pci_regs.h | 1 +
>> 5 files changed, 45 insertions(+), 73 deletions(-)
>
> Thanks, Denis, I applied these to pci/pciehp for v5.4. I think this
> is a great improvement.
>
> I tweaked a few things:
>
> - Updated comments to refer to "Power" intead of "green",
> "Attention" instead of "amber", and "Indicator" instead of "LED".
>
> - Replaced PCI_EXP_SLTCTL_ATTN_IND_NONE and
> PCI_EXP_SLTCTL_PWR_IND_NONE with INDICATOR_NOOP because I didn't
> want them to look like definitions from the spec.
>
> - Dropped set_power_indicator(). It does make things locally easier
> to read, but I think the overall benefit of having fewer
> interfaces outweighs that.
>
> The interdiff from your v4 is below. Let me know if I broke anything.
Thank you for the improvements. Looks good to me.
Regards,
Denis
