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(-)

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

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