Re: [PATCH v4 2/8] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks

[Manivannan Sadhasivam]

On Sun, Jan 11, 2026 at 09:27:11PM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 05, 2026 at 07:25:42PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > 
> > To allow the pwrctrl core to control the power on/off sequences of the
> > pwrctrl drivers, add the 'struct pci_pwrctrl::power_{on/off}' callbacks and
> > populate them in the respective pwrctrl drivers.
> > 
> > The pwrctrl drivers still power on the resources on their own now. So there
> > is no functional change.
> > 
> > Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> > Tested-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c | 27 ++++++++++++++---
> >  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 22 ++++++++++----
> >  drivers/pci/pwrctrl/slot.c               | 50 +++++++++++++++++++++++---------
> >  include/linux/pci-pwrctrl.h              |  4 +++
> >  4 files changed, 79 insertions(+), 24 deletions(-)
> 
> I had a hard time reading this because of the gratuitous differences
> in names of pwrseq, tc9563, and slot structs, members, and pointers.
> These are all corresponding private structs that could be named
> similarly:
> 
>   struct pci_pwrctrl_pwrseq_data
>   struct tc9563_pwrctrl_ctx
>   struct pci_pwrctrl_slot_data
> 
> These are all corresponding members:
> 
>   struct pci_pwrctrl_pwrseq_data.ctx
>   struct tc9563_pwrctrl_ctx.pwrctrl (last in struct instead of first)
>   struct pci_pwrctrl_slot_data.ctx
>   
> And these are all corresponding pointers or parameters:
> 
>   struct pci_pwrctrl_pwrseq_data *data
>   struct tc9563_pwrctrl_ctx *ctx
>   struct pci_pwrctrl_slot_data *slot
> 
> There's no need for this confusion, so I reworked those names to make
> them a *little* more consistent:
> 
>   structs:
>     struct pci_pwrctrl_pwrseq
>     struct pci_pwrctrl_tc9563
>     struct pci_pwrctrl_slot
> 
>   member:
>     struct pci_pwrctrl pwrctrl (for all)
> 
>   pointers/parameters:
>     struct pci_pwrctrl_pwrseq *pwrseq
>     struct pci_pwrctrl_tc9563 *tc9563
>     struct pci_pwrctrl_slot *slot
> 
> The file names, function names, and driver names are still not very
> consistent, but I didn't do anything with them:
> 
>   pci-pwrctrl-pwrseq.c  pci_pwrctrl_pwrseq_probe()  "pci-pwrctrl-pwrseq"
>   pci-pwrctrl-tc9563.c  tc9563_pwrctrl_probe()      "pwrctrl-tc9563"
>   slot.c                pci_pwrctrl_slot_probe()    ""pci-pwrctrl-slot"
> 

Yeah, because all 3 were written by 3 different developers and Bartosz didn't
pay attention to the detail :) I can unify them in the upcoming patches. Thanks
for spotting the differences.

> > +++ b/drivers/pci/pwrctrl/slot.c
> > @@ -17,13 +17,38 @@ struct pci_pwrctrl_slot_data {
> >  	struct pci_pwrctrl ctx;
> >  	struct regulator_bulk_data *supplies;
> >  	int num_supplies;
> > +	struct clk *clk;
> >  };
> >  
> > -static void devm_pci_pwrctrl_slot_power_off(void *data)
> > +static int pci_pwrctrl_slot_power_on(struct pci_pwrctrl *ctx)
> >  {
> > -	struct pci_pwrctrl_slot_data *slot = data;
> > +	struct pci_pwrctrl_slot_data *slot = container_of(ctx, struct pci_pwrctrl_slot_data, ctx);
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
> > +	if (ret < 0) {
> > +		dev_err(slot->ctx.dev, "Failed to enable slot regulators\n");
> > +		return ret;
> > +	}
> > +
> > +	return clk_prepare_enable(slot->clk);
> 
> It would be nice if we could add a preparatory patch to factor out
> pci_pwrctrl_slot_power_on() before this one.  Then the slot.c patch
> would look more like the pwrseq and tc9563 ones.
> 

Agree, other two drivers atleast had a helper to do power on/off, so that made
them look nicer in diff.

- Mani

-- 
மணிவண்ணன் சதாசிவம்