On Mon, Oct 27, 2025 at 08:24:12PM +0000, Vitor Soares wrote:

Hello Mani,

Thank you for the feedback.

On Tue, 2025-10-21 at 07:36 +0530, Manivannan Sadhasivam wrote:

quoted

On Tue, Oct 14, 2025 at 12:25:49PM +0100, Vitor Soares wrote:

quoted

From: Vitor Soares <redacted>

Some boards require external regulators to power PCIe endpoints.
Add support for optional 1.5V, 3.3V, and 12V supplies, which may be
defined in the device tree as vpcie1v5-supply, vpcie3v3-supply, and
vpcie12v-supply.

Use devm_regulator_get_enable_optional() to obtain and enable each
supply, so it will be automatically disabled when the driver is
removed.

Signed-off-by: Vitor Soares <redacted>
---
 drivers/pci/controller/cadence/pci-j721e.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c

b/drivers/pci/controller/cadence/pci-j721e.c
index 5bc5ab20aa6d..f29ce2aef04e 100644

--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c

@@ -21,6 +21,7 @@

 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include "../../pci.h"
 #include "pcie-cadence.h"

@@ -467,6 +468,10 @@ static const struct of_device_id of_j721e_pcie_match[]

= {
 };
 MODULE_DEVICE_TABLE(of, of_j721e_pcie_match);
 
+static const char * const j721e_pcie_supplies[] = {
+       "vpcie12v", "vpcie3v3", "vpcie1v5"
+};

Please don't hardcode the supplies in driver. The DT binding should make sure
the relevant supplies are passed (including the optional ones). Just use
of_regulator_bulk_get_all() to acquire all the passed supplies.

- Mani

I checked the bulk regulator APIs as suggested and of_regulator_bulk_get_all()
does handle optional supplies correctly, however it is not a managed function
and doesn't enable the  regulators automatically.

To use of_regulator_bulk_get_all(), I would need to:
- Manually enable regulators with regulator_bulk_enable()
- Add cleanup/disable logic in remove path
- Handle error cleanup path manually

This would actually make the code more complex and error-prone compared to the
current approach using devm_regulator_get_enable_optional(), which provides
managed cleanup and automatic enable for optional supplies.

I also checked devm_regulator_bulk_get_enable(), it treats all supplies as
required and needs the supplies name as well.

Unless there is a devm_regulator_bulk_get_optional_enable() API I'm not aware
of, the current per-supply approach is the standard kernel pattern for this use
case. Would you still prefer the bulk approach despite these limitations?

Fine then. If you do not have a reason to manualy turn off/on the supplies
(during suspend/resume), then it might be better to use
devm_regulator_get_enable_optional() for now.

I'd love to add the managed version of of_regulator_bulk_get_all(), but I guess
Mark wouldn't want it.

- Mani

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