On Thu, Nov 26, 2020 at 6:49 AM Mark Brown wrote: > > On Wed, Nov 25, 2020 at 02:24:19PM -0500, Jim Quinlan wrote: > > > + for (i = 0; i < PCIE_REGULATORS_MAX; i++) { > > + ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]); > > + if (IS_ERR(ep_reg)) { > > Does PCI allow supplies to be physically absent? If not then the driver > shouldn't be using regulator_get_optional() and much of the code here > can be deleted. Hi Mark, First, as an aside, I'm a little confused about the purpose of devm_regulator_get_optional(...); the other xxx_get_optional() calls I am familiar with (eg clock, reset, gpio) return NULL if the desired item does not exist, and then NULL can be used as a valid pointer for the rest of the API. Not so here. At any rate, our SOCs are placed in a variety of boards which implement the PCIe RC-EP connection as they wish. From the PCIe driver's point of view, the type of power supply that needs to be turned on is specified in the DT and they cannot be hard coded by the driver. I've listed all of the four possibilities; typically one, maybe two will be specified, but never all of them. In addition, sometimes a regulator is hard wired on and not even controllable by the PCIe controller. > > > +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on) > > +{ > > This is open coding the regulator bulk APIs. Except that a bulk regulator "get" requires that all supplies are present. I would have to first scan the node's properties for the "-supply" properties and fill in the bulk regulator structure. I'm fine with doing that. However, a previous incarnation of this commit was reviewed by RobH, and if I understood him correctly he wanted the actual names of the possible regulators to be used and specified in the bindings doc. I just followed the example of "pcie-rockchip-host.c" whose bindings doc was reviewed by RobH. Regards, Jim Quinlan Broadcom STB