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

Manivannan Sadhasivam via B4 Relay posted 8 patches 1 month ago
There is a newer version of this series
[PATCH v4 2/8] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
Posted by Manivannan Sadhasivam via B4 Relay 1 month ago
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(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
index 4e664e7b8dd2..7e7093cbff3c 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
@@ -52,11 +52,27 @@ static const struct pci_pwrctrl_pwrseq_pdata pci_pwrctrl_pwrseq_qcom_wcn_pdata =
 	.validate_device = pci_pwrctrl_pwrseq_qcm_wcn_validate_device,
 };
 
+static int pci_pwrctrl_pwrseq_power_on(struct pci_pwrctrl *ctx)
+{
+	struct pci_pwrctrl_pwrseq_data *data = container_of(ctx, struct pci_pwrctrl_pwrseq_data,
+							    ctx);
+
+	return pwrseq_power_on(data->pwrseq);
+}
+
+static int pci_pwrctrl_pwrseq_power_off(struct pci_pwrctrl *ctx)
+{
+	struct pci_pwrctrl_pwrseq_data *data = container_of(ctx, struct pci_pwrctrl_pwrseq_data,
+							    ctx);
+
+	return pwrseq_power_off(data->pwrseq);
+}
+
 static void devm_pci_pwrctrl_pwrseq_power_off(void *data)
 {
-	struct pwrseq_desc *pwrseq = data;
+	struct pci_pwrctrl_pwrseq_data *pwrseq_data = data;
 
-	pwrseq_power_off(pwrseq);
+	pci_pwrctrl_pwrseq_power_off(&pwrseq_data->ctx);
 }
 
 static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
@@ -85,16 +101,19 @@ static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(data->pwrseq),
 				     "Failed to get the power sequencer\n");
 
-	ret = pwrseq_power_on(data->pwrseq);
+	ret = pci_pwrctrl_pwrseq_power_on(&data->ctx);
 	if (ret)
 		return dev_err_probe(dev, ret,
 				     "Failed to power-on the device\n");
 
 	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_pwrseq_power_off,
-				       data->pwrseq);
+				       data);
 	if (ret)
 		return ret;
 
+	data->ctx.power_on = pci_pwrctrl_pwrseq_power_on;
+	data->ctx.power_off = pci_pwrctrl_pwrseq_power_off;
+
 	pci_pwrctrl_init(&data->ctx, dev);
 
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &data->ctx);
diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 0a63add84d09..616d3caedb34 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -434,15 +434,22 @@ static int tc9563_pwrctrl_parse_device_dt(struct tc9563_pwrctrl_ctx *ctx, struct
 	return 0;
 }
 
-static void tc9563_pwrctrl_power_off(struct tc9563_pwrctrl_ctx *ctx)
+static int tc9563_pwrctrl_power_off(struct pci_pwrctrl *pwrctrl)
 {
+	struct tc9563_pwrctrl_ctx *ctx = container_of(pwrctrl,
+					struct tc9563_pwrctrl_ctx, pwrctrl);
+
 	gpiod_set_value(ctx->reset_gpio, 1);
 
 	regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+
+	return 0;
 }
 
-static int tc9563_pwrctrl_bring_up(struct tc9563_pwrctrl_ctx *ctx)
+static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
 {
+	struct tc9563_pwrctrl_ctx *ctx = container_of(pwrctrl,
+					struct tc9563_pwrctrl_ctx, pwrctrl);
 	struct tc9563_pwrctrl_cfg *cfg;
 	int ret, i;
 
@@ -502,7 +509,7 @@ static int tc9563_pwrctrl_bring_up(struct tc9563_pwrctrl_ctx *ctx)
 		return 0;
 
 power_off:
-	tc9563_pwrctrl_power_off(ctx);
+	tc9563_pwrctrl_power_off(&ctx->pwrctrl);
 	return ret;
 }
 
@@ -591,7 +598,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 			goto remove_i2c;
 	}
 
-	ret = tc9563_pwrctrl_bring_up(ctx);
+	ret = tc9563_pwrctrl_power_on(&ctx->pwrctrl);
 	if (ret)
 		goto remove_i2c;
 
@@ -601,6 +608,9 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 			goto power_off;
 	}
 
+	ctx->pwrctrl.power_on = tc9563_pwrctrl_power_on;
+	ctx->pwrctrl.power_off = tc9563_pwrctrl_power_off;
+
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &ctx->pwrctrl);
 	if (ret)
 		goto power_off;
@@ -610,7 +620,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 	return 0;
 
 power_off:
-	tc9563_pwrctrl_power_off(ctx);
+	tc9563_pwrctrl_power_off(&ctx->pwrctrl);
 remove_i2c:
 	i2c_unregister_device(ctx->client);
 	put_device(&ctx->adapter->dev);
@@ -621,7 +631,7 @@ static void tc9563_pwrctrl_remove(struct platform_device *pdev)
 {
 	struct tc9563_pwrctrl_ctx *ctx = platform_get_drvdata(pdev);
 
-	tc9563_pwrctrl_power_off(ctx);
+	tc9563_pwrctrl_power_off(&ctx->pwrctrl);
 	i2c_unregister_device(ctx->client);
 	put_device(&ctx->adapter->dev);
 }
diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
index 3320494b62d8..9f91f04b4ae0 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ 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);
+}
+
+static int pci_pwrctrl_slot_power_off(struct pci_pwrctrl *ctx)
+{
+	struct pci_pwrctrl_slot_data *slot = container_of(ctx, struct pci_pwrctrl_slot_data, ctx);
 
 	regulator_bulk_disable(slot->num_supplies, slot->supplies);
+	clk_disable_unprepare(slot->clk);
+
+	return 0;
+}
+
+static void devm_pci_pwrctrl_slot_release(void *data)
+{
+	struct pci_pwrctrl_slot_data *slot = data;
+
+	pci_pwrctrl_slot_power_off(&slot->ctx);
 	regulator_bulk_free(slot->num_supplies, slot->supplies);
 }
 
@@ -31,7 +56,6 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 {
 	struct pci_pwrctrl_slot_data *slot;
 	struct device *dev = &pdev->dev;
-	struct clk *clk;
 	int ret;
 
 	slot = devm_kzalloc(dev, sizeof(*slot), GFP_KERNEL);
@@ -46,23 +70,21 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 	}
 
 	slot->num_supplies = ret;
-	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
-	if (ret < 0) {
-		dev_err_probe(dev, ret, "Failed to enable slot regulators\n");
-		regulator_bulk_free(slot->num_supplies, slot->supplies);
-		return ret;
-	}
 
-	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_slot_power_off,
+	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_slot_release,
 				       slot);
 	if (ret)
 		return ret;
 
-	clk = devm_clk_get_optional_enabled(dev, NULL);
-	if (IS_ERR(clk)) {
-		return dev_err_probe(dev, PTR_ERR(clk),
+	slot->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(slot->clk))
+		return dev_err_probe(dev, PTR_ERR(slot->clk),
 				     "Failed to enable slot clock\n");
-	}
+
+	pci_pwrctrl_slot_power_on(&slot->ctx);
+
+	slot->ctx.power_on = pci_pwrctrl_slot_power_on;
+	slot->ctx.power_off = pci_pwrctrl_slot_power_off;
 
 	pci_pwrctrl_init(&slot->ctx, dev);
 
diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
index 4aefc7901cd1..435b822c841e 100644
--- a/include/linux/pci-pwrctrl.h
+++ b/include/linux/pci-pwrctrl.h
@@ -31,6 +31,8 @@ struct device_link;
 /**
  * struct pci_pwrctrl - PCI device power control context.
  * @dev: Address of the power controlling device.
+ * @power_on: Callback to power on the power controlling device.
+ * @power_off: Callback to power off the power controlling device.
  *
  * An object of this type must be allocated by the PCI power control device and
  * passed to the pwrctrl subsystem to trigger a bus rescan and setup a device
@@ -38,6 +40,8 @@ struct device_link;
  */
 struct pci_pwrctrl {
 	struct device *dev;
+	int (*power_on)(struct pci_pwrctrl *pwrctrl);
+	int (*power_off)(struct pci_pwrctrl *pwrctrl);
 
 	/* private: internal use only */
 	struct notifier_block nb;

-- 
2.48.1
Re: [PATCH v4 2/8] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
Posted by Bjorn Helgaas 4 weeks ago
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
Re: [PATCH v4 2/8] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
Posted by Manivannan Sadhasivam 3 weeks, 5 days ago
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@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"
> 

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

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH v4 2/8] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
Posted by Bjorn Helgaas 3 weeks, 5 days ago
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@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(-)

> > +++ 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(...);
Re: [PATCH v4 2/8] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
Posted by Bartosz Golaszewski 1 month ago
On Mon, 5 Jan 2026 14:55:42 +0100, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> said:
> 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>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>