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

[Bjorn Helgaas]

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

> > +++ 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.

tc9563 implements power control functions:

  tc9563_pwrctrl_bring_up(struct tc9563_pwrctrl_ctx *ctx)
  tc9563_pwrctrl_power_off(struct pci_pwrctrl_tc9563 *tc9563)

and this patch updates these to make the signature generic so they can
be used as callbacks:

  tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
  tc9563_pwrctrl_power_off(struct pci_pwrctrl *pwrctrl)

This part of the patch is super straightforward -- make the signature
generic and extract the per-driver struct using the generic pointer.

I was thinking that if a preparatory patch factored out the slot power
on/off, e.g.:

  pci_pwrctrl_slot_power_on(struct pci_pwrctrl_slot_data *slot)

Then the slot.c part of this patch wouldn't move any code around, so
the structure would be identical to the tc9563 part.

pwrseq doesn't currently have the power-on/off functions factored out
either because they're so trivial, but I might even consider factoring
those out first, e.g.:

  pci_pwrctrl_pwrseq_power_on(struct pci_pwrctrl_pwrseq *pwrseq)

If we did that, this patch would be strictly conversion from
driver-specific pointer to "struct pci_pwrctrl *pwrctrl" followed by
"<driver-specific pointer = container_of(...)", so all three driver
changes would be identical and trivial to describe and review:

  - pci_pwrctrl_pwrseq_power_on(struct pci_pwrctrl_pwrseq *pwrseq)
  + pci_pwrctrl_pwrseq_power_on(struct pci_pwrctrl *pwrctrl)
  +   struct pci_pwrctrl_pwrseq *pwrseq = container_of(...);

  - tc9563_pwrctrl_bring_up(struct pci_pwrctrl_tc9563 *tc9563)
  + tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
  +   struct pci_pwrctrl_tc9563 *tc9563 = container_of(...);

  - pci_pwrctrl_slot_power_on(struct pci_pwrctrl_slot *slot)
  + pci_pwrctrl_slot_power_on(struct pci_pwrctrl *pwrctrl)
  +   struct pci_pwrctrl_slot *slot = container_of(...);