Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210326191906.43567-1-jim2101024@gmail.com> <20210326191906.43567-3-jim2101024@gmail.com>
 <20210329162539.GG5166@sirena.org.uk> <CANCKTBsBNhwG8VQQAQfAfw9jaWLkT+yYJ0oG-HBhA9xiO+jLvA@mail.gmail.com>
 <20210329171613.GI5166@sirena.org.uk>
In-Reply-To: <20210329171613.GI5166@sirena.org.uk>
From:   Jim Quinlan <jim2101024@gmail.com>
Date:   Mon, 29 Mar 2021 15:48:46 -0400
Message-ID: <CANCKTBvwWdVgjgTf620KqaAyyMwPkRgO3FHOqs_Gen+bnYTJFw@mail.gmail.com>
Subject: Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators
To:     Mark Brown <broonie@kernel.org>
Cc:     linux-pci <linux-pci@vger.kernel.org>,
        Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
        Rob Herring <robh@kernel.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        Jim Quinlan <james.quinlan@broadcom.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Florian Fainelli <f.fainelli@gmail.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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

    /* Pmap_idx to avs pmap number */
    const uint8_t pmap_idx_to_avs_id[20];

On Mon, Mar 29, 2021 at 1:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>
>
> > > Here you are figuring out a device local supply name...
>
> > > > +     /*
> > > > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > > > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > > > +      * it will not find the regulators.  Instead, use NULL and the
> > > > +      * regulators are looked up by their name.
> > > > +      */
> > > > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> > > ...and here you are trying to look up that device local name in the
> > > global namespace.  That's not going to work well, the global names that
> > > supplies are labelled with may be completely different to what the chip
> > > designer called them and there could easily be naming collisions between
> > > different chips.
>
> > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> > NULL for the device and if so does this fix it?  If not, what do you
> > suggest that I do?
>
> If you use the struct device for the PCIe controller then that's going
> to first try the PCIe controller then the global namespace so you still
> have all the same problems.  You really need to use the struct device
> for the device that is being supplied not some random other struct
> device you happened to find in the system.
Hello Mark,
I'm not concerned about a namespace collision and I don't think you
should be concerned either.  First, this driver is for Broadcom STB
PCIe chips and boards, and we also deliver the DT to the customers.
We typically do not have any other regulators in the DT besides the
ones I am proposing.  For example, the 7216 SOC DT has 0 other
regulators -- no namespace collision possible.  Our DT-generating
scripts also flag namespace issues.  I admit that this driver is also
used by RPi chips, but I can easily exclude this feature from the RPI
if Nicolas has any objection.

Further, if you want, I can restrict the search to the two regulators
I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
"vpcie3v3-supply".

Is the above enough to alleviate your concerns about global namespace collision?

>
> As I said in my earlier reply I think either the driver core or PCI
> needs something like Soundwire has which lets it create struct devices
> for things that have been enumerated via software but not enumerated by
> hardware and a callback or something which lets those devices take
> whatever steps are needed to trigger probe.

Can you please elaborate this further and in detail?  This sounds like
a decent-size undertaking, and the last time I followed a review
suggestion that was similar in spirit to this, the pullreq was
ironically NAKed by the person who suggested it.  Do you really think
it is necessary to construct such a subsystem just to avoid the  very
remote possibility of a namespace collision which is our (Broadcom
STB) responsibility and consequence to avoid?

Regards,
Jim Quinlan
Broadcom STB