Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

From: Rob Herring <robh@kernel.org>
To: Jim Quinlan <james.quinlan@broadcom.com>
Cc: "Mark Brown" <broonie@kernel.org>,
	"Jim Quinlan" <jim2101024@gmail.com>,
	"open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS"
	<linux-pci@vger.kernel.org>,
	"Nicolas Saenz Julienne" <nsaenz@kernel.org>,
	"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
	<bcm-kernel-feedback-list@broadcom.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
	<linux-rpi-kernel@lists.infradead.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
	<linux-arm-kernel@lists.infradead.org>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators
Date: Mon, 25 Oct 2021 17:16:34 -0500	[thread overview]
Message-ID: <YXcswoP6fLiMs7G5@robh.at.kernel.org> (raw)
In-Reply-To: <CA+-6iNzmkB5sUL6aqA6229BhxBhF3RKvGsLh0JCYQwP_2wSGaQ@mail.gmail.com>

On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
> >
> > > +static const char * const supplies[] = {
> > > +     "vpcie3v3-supply",
> > > +     "vpcie3v3aux-supply",
> > > +     "brcm-ep-a-supply",
> > > +     "brcm-ep-b-supply",
> > > +};
> >
> > Why are you including "-supply" in the names here?  That will lead to
> > a double -supply when we look in the DT which probably isn't what you're
> > looking for.
> I'm not sure how this got past testing; will fix.
> 
> >
> > Also are you *sure* that the device has supplies with names like
> > "brcm-ep-a"?  That seems rather unidiomatic for electrical engineering,
> > the names here are supposed to correspond to the names used in the
> > datasheet for the part.
> I try to explain this in the commit message of"PCI: allow for callback
> to prepare nascent subdev".  Wrt to the names,
> 
>        "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."
> 
> Each different SOC./board we deal with may present different ways of
> making the EP device power on.  We are using
> an abstraction name "brcm-ep-a"  to represent some required regulator
> to make the EP  work for a specific board.  The RC
> driver cannot hard code a descriptive name as it must work for all
> boards designed by us, others, and third parties.
> The EP driver also doesn't know  or care about the regulator name, and
> this driver is often closed source and often immutable.  The EP
> device itself may come from Brcm, a third party,  or sometimes a competitor.
> 
> Basically, we find using a generic name such as "brcm-ep-a-supply"
> quite handy and many of our customers embrace this feature.
> I know that Rob was initially against such a generic name, but I
> vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> Or my memory is shot, which could very well be the case.

I don't recall being in favor of this. If you've got standard rails 
(3.3V and 12V), then I'm fine with standard properties for them with or 
without a slot.

Rob