[PATCH v1 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Mark Brown]

On Fri, Nov 27, 2020 at 03:26:53PM -0500, Jim Quinlan wrote:
> On Thu, Nov 26, 2020 at 6:49 AM Mark Brown <broonie at kernel.org> wrote:

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

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

The other APIs that cloned the regulator API don't have the dummy
support that the regulator has and unfortunately changed the sense a bit
there.

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

No, you should never do that.  If the supplies can be physically absent
then you should use regulator_get_optional() which allows you to do
whatever needs doing to configure the hardware for the missing supply.
If it's just that the supply may not be described in the DT but has to
be there for the device to operate then the code should use the normal
regualtor APIs - a dummy regulator will be provided if there's no supply
described.

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

That is just plain bad code, the binding may well be fine but I can't
see any excuse for that driver to be using _optional() there.

Another subsystem I'm going to have to keep an eye on :(
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20201130/c372521d/attachment.sig>