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

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Jan 05, 2026 at 07:25:42PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> 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@oss.qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  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"

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

Bjorn