v5 [NOTE: It has been a while since v4. Sorry]
-- See "PCI: allow for callback to prepare nascent subdev"
commit message for the cornerstone of this patchset
and the reasons behind it. This is a new commit.
-- The RC driver now looks into its DT children and
turns on regulators for a sub-device, and this occurs
prior to PCIe link as it must.
-- Dropped commits not related to the focus of this patchset.
v4 [NOTE: I'm not sure this fixes RobH and MarkB constraints but I'd
like to use this pullreq as a basis for future discussion.]
[Commit: Add bindings for ...]
-- Fix syntax error in YAML bindings example (RobH)
-- {vpcie12v,vpcie3v3}-supply props are back in root complex DT node
(I believe RobH said this was okay)
[Commit: Add control of ..]
-- Do not do global search for regulator; now we look specifically
for the property {vpcie12v,vpcie3v3}-supply in the root complex
DT node and then call devm_regulator_bulk_get() (MarkB)
-- Use devm_regulator_bulk_get() (Bjorn)
-- s/EP/slot0 device/ (Bjorn)
-- Spelling, capitalization (Bjorn)
-- Have brcm_phy_stop() return a void (Bjorn)
[Commit: Do not turn off ...]
-- Capitalization (Bjorn)
[Commit: Check return value ...]
-- Commit message content (Bjorn)
-- Move 6/6 hunk to 2/6 where it belongs (Bjorn)
-- Move the rest of 6/6 before all other commits (Bjorn)
v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
If present, these regulators have the property names
"vpcie12v-supply" and "vpcie3v3-supply". The existence of these
regulators in the EP subnode are currently pending as a pullreq
in pci-bus.yaml at
https://github.com/devicetree-org/dt-schema/pull/54
(MarkB, RobH).
-- Check return of brcm_set_regulators() (Florian)
-- Specify one regulator string per line for easier update (Florian)
-- Author/Committer/Signoff email changed from that of V2 from
'[email protected]' to '[email protected]'.
v2 -- Use regulator bulk API rather than multiple calls (MarkB).
v1 -- Bindings are added for fixed regulators that may power the EP device.
-- The brcmstb RC driver is modified to control these regulators
during probe, suspend, and resume.
-- 7216 type SOCs have additional error reporting HW and a
panic handler is added to dump its info.
-- A missing return value check is added.
Jim Quinlan (6):
dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
PCI: allow for callback to prepare nascent subdev
PCI: brcmstb: split brcm_pcie_setup() into two funcs
PCI: brcmstb: Add control of subdevice voltage regulators
PCI: brcmstb: Do not turn off regulators if EP can wake up
PCI: brcmstb: change brcm_phy_stop() to return void
.../bindings/pci/brcm,stb-pcie.yaml | 23 ++
drivers/base/core.c | 5 +
drivers/pci/controller/pcie-brcmstb.c | 231 ++++++++++++++++--
drivers/pci/probe.c | 47 +++-
include/linux/device.h | 3 +
include/linux/pci.h | 3 +
6 files changed, 286 insertions(+), 26 deletions(-)
--
2.17.1
We do not use the result of this function so make it void.
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 505f74bd1a51..d3e6d9df67b5 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1190,9 +1190,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie)
return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
}
-static inline int brcm_phy_stop(struct brcm_pcie *pcie)
+static inline void brcm_phy_stop(struct brcm_pcie *pcie)
{
- return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+ if (pcie->rescal)
+ brcm_phy_cntl(pcie, 0);
}
static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1271,10 +1272,9 @@ static int brcm_pcie_get_regulators(struct brcm_pcie *pcie, struct pci_dev *dev)
static int brcm_pcie_suspend(struct device *dev)
{
struct brcm_pcie *pcie = dev_get_drvdata(dev);
- int ret;
brcm_pcie_turn_off(pcie);
- ret = brcm_phy_stop(pcie);
+ brcm_phy_stop(pcie);
reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
--
2.17.1
If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 59 +++++++++++++++++++++++----
1 file changed, 52 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 35924af1c3aa..505f74bd1a51 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -192,6 +192,7 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
static const char * const supplies[] = {
"vpcie3v3-supply",
@@ -305,22 +306,65 @@ struct brcm_pcie {
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)];
unsigned int num_supplies;
+ bool ep_wakeup_capable;
};
-static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
{
+ bool *ret = data;
+
+ if (device_may_wakeup(&dev->dev)) {
+ *ret = true;
+ dev_dbg(&dev->dev, "disable cancelled for wake-up device\n");
+ }
+ return (int) *ret;
+}
+
+enum {
+ TURN_OFF, /* Turn regulators off, unless an EP is wakeup-capable */
+ TURN_OFF_ALWAYS, /* Turn regulators off, no exceptions */
+ TURN_ON, /* Turn regulators on, unless pcie->ep_wakeup_capable */
+};
+
+static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+ struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
struct device *dev = pcie->dev;
int ret;
if (!pcie->num_supplies)
return 0;
- if (on)
+ if (how == TURN_ON) {
+ if (pcie->ep_wakeup_capable) {
+ /*
+ * We are resuming from a suspend. In the
+ * previous suspend we did not disable the power
+ * supplies, so there is no need to enable them
+ * (and falsely increase their usage count).
+ */
+ pcie->ep_wakeup_capable = false;
+ return 0;
+ }
+ } else if (how == TURN_OFF) {
+ /*
+ * If at least one device on this bus is enabled as a
+ * wake-up source, do not turn off regulators.
+ */
+ pcie->ep_wakeup_capable = false;
+ if (bridge->bus && brcm_pcie_link_up(pcie)) {
+ pci_walk_bus(bridge->bus, pci_dev_may_wakeup, &pcie->ep_wakeup_capable);
+ if (pcie->ep_wakeup_capable)
+ return 0;
+ }
+ }
+
+ if (how == TURN_ON)
ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
else
ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
if (ret)
dev_err(dev, "failed to %s EP regulators\n",
- on ? "enable" : "disable");
+ how == TURN_ON ? "enable" : "disable");
return ret;
}
@@ -1234,7 +1278,7 @@ static int brcm_pcie_suspend(struct device *dev)
reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
- return brcm_set_regulators(pcie, false);
+ return brcm_set_regulators(pcie, TURN_OFF);
}
static int brcm_pcie_resume(struct device *dev)
@@ -1250,7 +1294,8 @@ static int brcm_pcie_resume(struct device *dev)
ret = reset_control_reset(pcie->rescal);
if (ret)
goto err_disable_clk;
- ret = brcm_set_regulators(pcie, true);
+
+ ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
goto err_reset;
@@ -1297,7 +1342,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
if (pcie->num_supplies) {
- (void)brcm_set_regulators(pcie, false);
+ (void)brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
regulator_bulk_free(pcie->num_supplies, pcie->supplies);
}
}
@@ -1348,7 +1393,7 @@ static int brcm_pcie_pci_subdev_prepare(bool query, struct pci_bus *bus, int dev
return ret;
}
- ret = brcm_set_regulators(pcie, true);
+ ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
goto err_out0;
--
2.17.1
We would like the Broadcom STB PCIe root complex driver to be able to turn
off/on regulators[1] that provide power to endpoint[2] devices. Typically,
the drivers of these endpoint devices are stock Linux drivers that are not
aware that these regulator(s) exist and must be turned on for the driver to
be probed. The simple solution of course is to turn these regulators on at
boot and keep them on. However, this solution does not satisfy at least
three of our usage modes:
1. For example, one customer uses multiple PCIe controllers, but wants the
ability to, by script, turn any or all of them by and their subdevices off
to save power, e.g. when in battery mode.
2. Another example is when a watchdog script discovers that an endpoint
device is in an unresponsive state and would like to unbind, power toggle,
and re-bind just the PCIe endpoint and controller.
3. Of course we also want power turned off during suspend mode. However,
some endpoint devices may be able to "wake" during suspend and we need to
recognise this case and veto the nominal act of turning off its regulator.
Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
end-point device needs to be kept powered on in order to receive network
packets and wake-up the system.
In all of these cases it is advantageous for the PCIe controller to govern
the turning off/on the regulators needed by the endpoint device. The first
two cases can be done by simply unbinding and binding the PCIe controller,
if the controller has control of these regulators.
This commit solves the "chicken-and-egg" problem by providing a callback to
the RC driver when a downstream device is "discovered" by inspecting its
DT, and allowing said driver to allocate the device object prematurely in
order to get the regulator(s) and turn them on before the device's ID is
read.
[1] These regulators typically govern the actual power supply to the
endpoint chip. Sometimes they may be a the official PCIe socket
power -- such as 3.3v or aux-3.3v. Sometimes they are truly
the regulator(s) that supply power to the EP chip.
[2] The 99% configuration of our boards is a single endpoint device
attached to the PCIe controller. I use the term endpoint but it could
possible mean a switch as well.
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/base/core.c | 5 +++++
drivers/pci/probe.c | 47 ++++++++++++++++++++++++++++++++----------
include/linux/device.h | 3 +++
include/linux/pci.h | 3 +++
4 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 249da496581a..62d9ac123ae5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
*/
void device_initialize(struct device *dev)
{
+ /* Return if this has been called already. */
+ if (dev->device_initialized)
+ return;
+
dev->kobj.kset = devices_kset;
kobject_init(&dev->kobj, &device_ktype);
INIT_LIST_HEAD(&dev->dma_pools);
@@ -2892,6 +2896,7 @@ void device_initialize(struct device *dev)
#ifdef CONFIG_SWIOTLB
dev->dma_io_tlb_mem = &io_tlb_default_mem;
#endif
+ dev->device_initialized = true;
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d9fc02a71baa..12947e972b7b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2372,27 +2372,52 @@ EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
*/
static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
{
- struct pci_dev *dev;
+ struct pci_host_bridge *hb = pci_find_host_bridge(bus);
+ struct pci_dev *dev = NULL;
u32 l;
- if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
- return NULL;
+ /*
+ * If the host bridge has a pci_subdev_prepare() function, first
+ * call it with true as the first argument to see if it "cares"
+ * about this device. A non-zero return value indicates it cares,
+ * so in that case partially allocate some of the device and call
+ * pci_subdev_prepare() again, with false as the first argument.
+ * This tells it to allow the host bridge driver to pre-allocate
+ * some resources such as voltage regulators.
+ */
+ if (hb->pci_subdev_prepare
+ && hb->pci_subdev_prepare(true, bus, devfn, NULL, NULL)) {
+ dev = pci_alloc_dev(bus);
+ if (!dev)
+ return NULL;
- dev = pci_alloc_dev(bus);
- if (!dev)
- return NULL;
+ dev->devfn = devfn;
+ device_initialize(&dev->dev);
+ /* Call again, this time for actual prep work */
+ if (hb->pci_subdev_prepare(false, bus, devfn, hb, dev)
+ || !pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
+ goto err_out;
+ } else {
+ if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
+ return NULL;
+ dev = pci_alloc_dev(bus);
+ if (!dev)
+ return NULL;
+ }
dev->devfn = devfn;
dev->vendor = l & 0xffff;
dev->device = (l >> 16) & 0xffff;
- if (pci_setup_device(dev)) {
- pci_bus_put(dev->bus);
- kfree(dev);
- return NULL;
- }
+ if (pci_setup_device(dev))
+ goto err_out;
return dev;
+
+err_out:
+ pci_bus_put(dev->bus);
+ kfree(dev);
+ return NULL;
}
void pcie_report_downtraining(struct pci_dev *dev)
diff --git a/include/linux/device.h b/include/linux/device.h
index e270cb740b9e..cf175684a270 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -461,6 +461,8 @@ struct dev_links_info {
* and optionall (if the coherent mask is large enough) also
* for dma allocations. This flag is managed by the dma ops
* instance from ->dma_supported.
+ * @device_initialized: true if device_initialize(dev) has already been
+ * invoked, false otherwise.
*
* At the lowest level, every device in a Linux system is represented by an
* instance of struct device. The device structure contains the information
@@ -575,6 +577,7 @@ struct device {
#ifdef CONFIG_DMA_OPS_BYPASS
bool dma_ops_bypass : 1;
#endif
+ bool device_initialized : 1;
};
/**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cd8aa6fce204..7a72b3af1e33 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -552,6 +552,9 @@ struct pci_host_bridge {
u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
int (*map_irq)(const struct pci_dev *, u8, u8);
void (*release_fn)(struct pci_host_bridge *);
+ int (*pci_subdev_prepare)(bool query, struct pci_bus *bus, int devfn,
+ struct pci_host_bridge *hb,
+ struct pci_dev *pdev);
void *release_data;
unsigned int ignore_reset_delay:1; /* For entire hierarchy */
unsigned int no_ext_tags:1; /* No Extended Tags */
--
2.17.1
On Fri, Oct 22, 2021 at 10:06:55AM -0400, Jim Quinlan wrote:
> We would like the Broadcom STB PCIe root complex driver to be able to turn
> off/on regulators[1] that provide power to endpoint[2] devices. Typically,
> the drivers of these endpoint devices are stock Linux drivers that are not
> aware that these regulator(s) exist and must be turned on for the driver to
> be probed. The simple solution of course is to turn these regulators on at
> boot and keep them on. However, this solution does not satisfy at least
> three of our usage modes:
>
> 1. For example, one customer uses multiple PCIe controllers, but wants the
> ability to, by script, turn any or all of them by and their subdevices off
> to save power, e.g. when in battery mode.
>
> 2. Another example is when a watchdog script discovers that an endpoint
> device is in an unresponsive state and would like to unbind, power toggle,
> and re-bind just the PCIe endpoint and controller.
>
> 3. Of course we also want power turned off during suspend mode. However,
> some endpoint devices may be able to "wake" during suspend and we need to
> recognise this case and veto the nominal act of turning off its regulator.
> Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
> end-point device needs to be kept powered on in order to receive network
> packets and wake-up the system.
>
> In all of these cases it is advantageous for the PCIe controller to govern
> the turning off/on the regulators needed by the endpoint device. The first
> two cases can be done by simply unbinding and binding the PCIe controller,
> if the controller has control of these regulators.
>
> This commit solves the "chicken-and-egg" problem by providing a callback to
> the RC driver when a downstream device is "discovered" by inspecting its
> DT, and allowing said driver to allocate the device object prematurely in
> order to get the regulator(s) and turn them on before the device's ID is
> read.
>
> [1] These regulators typically govern the actual power supply to the
> endpoint chip. Sometimes they may be a the official PCIe socket
> power -- such as 3.3v or aux-3.3v. Sometimes they are truly
> the regulator(s) that supply power to the EP chip.
>
> [2] The 99% configuration of our boards is a single endpoint device
> attached to the PCIe controller. I use the term endpoint but it could
> possible mean a switch as well.
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/base/core.c | 5 +++++
> drivers/pci/probe.c | 47 ++++++++++++++++++++++++++++++++----------
> include/linux/device.h | 3 +++
> include/linux/pci.h | 3 +++
> 4 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 249da496581a..62d9ac123ae5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
> */
> void device_initialize(struct device *dev)
> {
> + /* Return if this has been called already. */
> + if (dev->device_initialized)
> + return;
> +
Ick, no! Who would ever be calling this function more than once? That
"should" be impossible.
This function should only be called by a bus when it first creates the
structure and before anything is done with it. As the bus itself
controls the creation, it already "knows" when the structure was
initialzed so it should not have to be called again.
Please fix the bus logic that requires this, it is broken.
Consider this a NACK for this patch, sorry.
greg k-h
On Fri, Oct 22, 2021 at 10:06:58AM -0400, Jim Quinlan wrote:
> +enum {
> + TURN_OFF, /* Turn regulators off, unless an EP is wakeup-capable */
> + TURN_OFF_ALWAYS, /* Turn regulators off, no exceptions */
> + TURN_ON, /* Turn regulators on, unless pcie->ep_wakeup_capable */
> +};
> +
> +static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
> +{
> + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
I can't help but think this would be easier to follow as multiple
functions, there is very little code sharing between the different
paths especially the on and off paths.
> if (pcie->num_supplies) {
> - (void)brcm_set_regulators(pcie, false);
> + (void)brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
I should've mentioned this on the earlier path but it's not normal Linux
style to cast return values to void and looks worrying.
On Fri, Oct 22, 2021 at 10:34 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:55AM -0400, Jim Quinlan wrote:
> > We would like the Broadcom STB PCIe root complex driver to be able to turn
> > off/on regulators[1] that provide power to endpoint[2] devices. Typically,
> > the drivers of these endpoint devices are stock Linux drivers that are not
> > aware that these regulator(s) exist and must be turned on for the driver to
> > be probed. The simple solution of course is to turn these regulators on at
> > boot and keep them on. However, this solution does not satisfy at least
> > three of our usage modes:
> >
> > 1. For example, one customer uses multiple PCIe controllers, but wants the
> > ability to, by script, turn any or all of them by and their subdevices off
> > to save power, e.g. when in battery mode.
> >
> > 2. Another example is when a watchdog script discovers that an endpoint
> > device is in an unresponsive state and would like to unbind, power toggle,
> > and re-bind just the PCIe endpoint and controller.
> >
> > 3. Of course we also want power turned off during suspend mode. However,
> > some endpoint devices may be able to "wake" during suspend and we need to
> > recognise this case and veto the nominal act of turning off its regulator.
> > Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
> > end-point device needs to be kept powered on in order to receive network
> > packets and wake-up the system.
> >
> > In all of these cases it is advantageous for the PCIe controller to govern
> > the turning off/on the regulators needed by the endpoint device. The first
> > two cases can be done by simply unbinding and binding the PCIe controller,
> > if the controller has control of these regulators.
> >
> > This commit solves the "chicken-and-egg" problem by providing a callback to
> > the RC driver when a downstream device is "discovered" by inspecting its
> > DT, and allowing said driver to allocate the device object prematurely in
> > order to get the regulator(s) and turn them on before the device's ID is
> > read.
> >
> > [1] These regulators typically govern the actual power supply to the
> > endpoint chip. Sometimes they may be a the official PCIe socket
> > power -- such as 3.3v or aux-3.3v. Sometimes they are truly
> > the regulator(s) that supply power to the EP chip.
> >
> > [2] The 99% configuration of our boards is a single endpoint device
> > attached to the PCIe controller. I use the term endpoint but it could
> > possible mean a switch as well.
> >
> > Signed-off-by: Jim Quinlan <[email protected]>
> > ---
> > drivers/base/core.c | 5 +++++
> > drivers/pci/probe.c | 47 ++++++++++++++++++++++++++++++++----------
> > include/linux/device.h | 3 +++
> > include/linux/pci.h | 3 +++
> > 4 files changed, 47 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 249da496581a..62d9ac123ae5 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
> > */
> > void device_initialize(struct device *dev)
> > {
> > + /* Return if this has been called already. */
> > + if (dev->device_initialized)
> > + return;
> > +
>
> Ick, no! Who would ever be calling this function more than once? That
> "should" be impossible.
>
> This function should only be called by a bus when it first creates the
> structure and before anything is done with it. As the bus itself
> controls the creation, it already "knows" when the structure was
> initialzed so it should not have to be called again.
>
> Please fix the bus logic that requires this, it is broken.
Got it, thanks for the prompt reply.
JimQ
>
> Consider this a NACK for this patch, sorry.
>
> greg k-h
On Fri, Oct 22, 2021 at 10:39 AM Mark Brown <[email protected]> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:58AM -0400, Jim Quinlan wrote:
>
> > +enum {
> > + TURN_OFF, /* Turn regulators off, unless an EP is wakeup-capable */
> > + TURN_OFF_ALWAYS, /* Turn regulators off, no exceptions */
> > + TURN_ON, /* Turn regulators on, unless pcie->ep_wakeup_capable */
> > +};
> > +
> > +static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
> > +{
> > + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
>
> I can't help but think this would be easier to follow as multiple
> functions, there is very little code sharing between the different
> paths especially the on and off paths.
Agree; I just wanted to make less changes to struct pci_host_bridge. Will fix.
>
> > if (pcie->num_supplies) {
> > - (void)brcm_set_regulators(pcie, false);
> > + (void)brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
>
> I should've mentioned this on the earlier path but it's not normal Linux
> style to cast return values to void and looks worrying.
Got it.
Thanks,
JimQ