| Message ID | 20241212110233.787581-1-trintaeoitogc@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RESEND] PCI: remove already resolved TODO | expand |
On Thu, Dec 12, 2024 at 08:02:33AM -0300, guilherme giacomo simoes wrote: > The get_power and set_power fields is used, and only hardware_test is > really not used. So, after commit > 5b036cada481a7a3bf30d333298f6d83dfb19bed ("PCI: cpcihp: Remove unused > struct cpci_hp_controller_ops.hardware_test") this TODO is completed. The entire 40-char SHA-1 is overkill; use 12: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v6.12#n204 I see a test and call for .get_power() and .set_power(), but no actual implementations, so I think they can be removed as well, can't they? If so, I'll wait for that removal before applying this patch. > Signed-off-by: guilherme giacomo simoes <trintaeoitogc@gmail.com> In https://lore.kernel.org/r/20241014131917.324667-1-trintaeoitogc@gmail.com, you capitalized your names. What's your preference? I'd like to use your name correctly and consistently. > --- > drivers/pci/hotplug/TODO | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO > index 92e6e20e8595..7397374af171 100644 > --- a/drivers/pci/hotplug/TODO > +++ b/drivers/pci/hotplug/TODO > @@ -2,10 +2,6 @@ Contributions are solicited in particular to remedy the following issues: > > cpcihp: > > -* There are no implementations of the ->hardware_test, ->get_power and > - ->set_power callbacks in struct cpci_hp_controller_ops. Why were they > - introduced? Can they be removed from the struct? > - > * Returned code from pci_hp_add_bridge() is not checked. > > cpqphp: > -- > 2.34.1 >
Bjorn Helgaas <helgaas@kernel.org> wrote: > I see a test and call for .get_power() and .set_power(), but no actual > implementations, so I think they can be removed as well, can't they? > If so, I'll wait for that removal before applying this patch. You are right. Both only have a check if exist the {g|s}et_power(), then this is called. But, as you already said, seems that really don't have a implementations for both. So, I can work on remove this fields an tests this. In the cpci_hotplug.h we can crate a `flags` field in `cpci_hp_controller_ops` struct, in addition of remove the {g|s}et_power(). In the cpci_hotplug_core.c that the cpci_hp_controller_ops struct is in use, maybe we can create a #define SLOT_ENABLED 0x00000001, and we can do `ops->flags |= ENABLED_SLOT` when we need enable the slot in the enable_slot() function and `ops->flags &= ~ENABLE_SLOT` in the disable_slot() function. In the get_power() function we only need return `ops->flags & SLOT_ENABLED`. what do you think? > In > https://lore.kernel.org/r/20241014131917.324667-1-trintaeoitogc@gmail.com, > you capitalized your names. What's your preference? I'd like to use > your name correctly and consistently. I make mistake, sorry for this. In the next commit I will send with my name capitalized.
On Fri, Dec 13, 2024 at 12:17:10PM -0300, Guilherme Giacomo Simoes wrote: > Bjorn Helgaas <helgaas@kernel.org> wrote: > > I see a test and call for .get_power() and .set_power(), but no actual > > implementations, so I think they can be removed as well, can't they? > > If so, I'll wait for that removal before applying this patch. > > You are right. Both only have a check if exist the {g|s}et_power(), > then this is called. > > But, as you already said, seems that really don't have a > implementations for both. So, I can work on remove this fields an > tests this. > > In the cpci_hotplug.h we can create a `flags` field in > `cpci_hp_controller_ops` struct, in addition of remove the > {g|s}et_power(). In the cpci_hotplug_core.c that the > cpci_hp_controller_ops struct is in use, maybe we can create a > #define SLOT_ENABLED 0x00000001, and we can do `ops->flags |= > ENABLED_SLOT` when we need enable the slot in the enable_slot() > function and `ops->flags &= ~ENABLE_SLOT` in the disable_slot() > function. In the get_power() function we only need return > `ops->flags & SLOT_ENABLED`. what do you think? I don't quite see what you have in mind; a patch would make it clear. But the cpci hotplug driver is basically dead. I don't think it's worth doing anything more than the most trivial cleanups to it. Bjorn
Bjorn Helgaas <helgaas@kernel.org> wrote: > I don't quite see what you have in mind; a patch would make it clear. > > But the cpci hotplug driver is basically dead. I don't think it's > worth doing anything more than the most trivial cleanups to it. I will make a little change, test a lot and I send a patch for this. And you can see that this patch is good for kernel or no. Thanks, Guilherme
diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO index 92e6e20e8595..7397374af171 100644 --- a/drivers/pci/hotplug/TODO +++ b/drivers/pci/hotplug/TODO @@ -2,10 +2,6 @@ Contributions are solicited in particular to remedy the following issues: cpcihp: -* There are no implementations of the ->hardware_test, ->get_power and - ->set_power callbacks in struct cpci_hp_controller_ops. Why were they - introduced? Can they be removed from the struct? - * Returned code from pci_hp_add_bridge() is not checked. cpqphp:
The get_power and set_power fields is used, and only hardware_test is really not used. So, after commit 5b036cada481a7a3bf30d333298f6d83dfb19bed ("PCI: cpcihp: Remove unused struct cpci_hp_controller_ops.hardware_test") this TODO is completed. Signed-off-by: guilherme giacomo simoes <trintaeoitogc@gmail.com> --- drivers/pci/hotplug/TODO | 4 ---- 1 file changed, 4 deletions(-)