| Message ID | 20190819160643.27998-1-efremov@linux.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Simplify PCIe hotplug indicator control | expand |
On 8/19/19 7:06 PM, 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 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}() Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by". The changes in the last 2 patches were significant. > > drivers/pci/hotplug/pciehp.h | 5 +- > drivers/pci/hotplug/pciehp_core.c | 7 ++- > drivers/pci/hotplug/pciehp_ctrl.c | 31 +++++++----- > drivers/pci/hotplug/pciehp_hpc.c | 82 ++++++++++--------------------- > include/uapi/linux/pci_regs.h | 3 ++ > 5 files changed, 54 insertions(+), 74 deletions(-) >
On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote: > On 8/19/19 7:06 PM, 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 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}() > > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by". > The changes in the last 2 patches were significant. Anybody want to review these?
On Tue, Aug 27, 2019 at 05:32:54PM -0500, Bjorn Helgaas wrote: > On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote: > > On 8/19/19 7:06 PM, 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 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}() > > > > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by". > > The changes in the last 2 patches were significant. > > Anybody want to review these? Except for one nitpick in [PATCH v3 1/4], rest seems fine to me.
On Tue, Aug 27, 2019 at 05:32:54PM -0500, Bjorn Helgaas wrote: > On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote: > > On 8/19/19 7:06 PM, 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 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}() > > > > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by". > > The changes in the last 2 patches were significant. > > Anybody want to review these? Unrelated, but if anybody is looking at pciehp, is there value in having pciehp split across five files? drivers/pci/hotplug/pciehp.h drivers/pci/hotplug/pciehp_core.c drivers/pci/hotplug/pciehp_ctrl.c drivers/pci/hotplug/pciehp_hpc.c drivers/pci/hotplug/pciehp_pci.c To me, it just makes things harder because when I'm browsing for something in pciehp and I don't know the exact name of it, I have to guess which file it's in, and I'm invariably wrong. It seems like it would be much simpler if everything were in a single pciehp.c file. Then we could also get rid of the header and make several more things static. Bjorn
On Tue, Aug 27, 2019 at 05:53:19PM -0500, Bjorn Helgaas wrote: > Unrelated, but if anybody is looking at pciehp, is there value in > having pciehp split across five files? > > drivers/pci/hotplug/pciehp.h > drivers/pci/hotplug/pciehp_core.c > drivers/pci/hotplug/pciehp_ctrl.c > drivers/pci/hotplug/pciehp_hpc.c > drivers/pci/hotplug/pciehp_pci.c > > To me, it just makes things harder because when I'm browsing for > something in pciehp and I don't know the exact name of it, I have to > guess which file it's in, and I'm invariably wrong. > > It seems like it would be much simpler if everything were in a single > pciehp.c file. Then we could also get rid of the header and make > several more things static. I wouldn't go so far as to say there's *value* in the split. It's a historic artefact, it's been like that since pciehp was introduced back in 2004: https://git.kernel.org/tglx/history/c/c16b4b14d980 I was hesitant to change it so far, in particular because it makes it harder to backport stable patches. That said, I did think about rearranging things to reduce the number of files and non-static declarations or to give the files better names. I wasn't thinking of squashing everything into one file, it would probably be quite large and not make things simpler. The general logic is that everything touching the registers is in pciehp_hpc.c. You won't (or shouldn't) find register access in any of the other files. pciehp_ctrl.c is the control logic, reaction to events, etc. Though portions of the control logic are also in pciehp_hpc.c, in particular the interrupt servicing. pciehp_core.c contains the interface to the PCI hotplug core and the probe/remove/PM routines. pciehp_pci.c contains bringup/teardown of the slot. One thing that I think deserves changing is this comment at the top of each file: "Send feedback to <greg@kroah.com>, <kristen.c.accardi@intel.com>" We now use MAINTAINERS to do this. Kristen took over maintainership in 2005 with commit 8cf4c19523b7 but by 2009 she had stepped down per commit be3652b8a253. So providing her address is no longer accurate. And I imagine Greg is no longer as familiar with the driver as it has changed considerably since 2004. He'd probably have to defer to others who have done work on it recently (such as me). Thanks, Lukas
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 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 | 5 +- drivers/pci/hotplug/pciehp_core.c | 7 ++- drivers/pci/hotplug/pciehp_ctrl.c | 31 +++++++----- drivers/pci/hotplug/pciehp_hpc.c | 82 ++++++++++--------------------- include/uapi/linux/pci_regs.h | 3 ++ 5 files changed, 54 insertions(+), 74 deletions(-)