Re: [PATCH v8 16/17] PCI/pwrctl: add a PCI power control driver for power sequenced devices

[Bjorn Helgaas]

On Wed, Jun 05, 2024 at 10:47:32AM +0200, Bartosz Golaszewski wrote:
> On Wed, Jun 5, 2024 at 4:13 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Wed, Jun 05, 2024 at 02:34:52AM +0300, Dmitry Baryshkov wrote:
> > > On Wed, 5 Jun 2024 at 02:23, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > On Tue, May 28, 2024 at 09:03:24PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > > >
> > > > > Add a PCI power control driver that's capable of correctly powering up
> > > > > devices using the power sequencing subsystem. The first users of this
> > > > > driver are the ath11k module on QCA6390 and ath12k on WCN7850.
> >
> > > > > +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = {
> > > > > +     {
> > > > > +             /* ATH11K in QCA6390 package. */
> > > > > +             .compatible = "pci17cb,1101",
> > > > > +             .data = "wlan",
> > > > > +     },
> > > > > +     {
> > > > > +             /* ATH12K in WCN7850 package. */
> > > > > +             .compatible = "pci17cb,1107",
> > > > > +             .data = "wlan",
> > > > > +     },
> > > >
> > > > IIUC, "pci17cb,1101" and "pci17cb,1107" exist partly so we can check
> > > > that a DTS conforms to the schema, e.g., a "pci17cb,1101" node
> > > > contains all the required regulators.  For that use, we obviously need
> > > > a very specific "compatible" string.
> > > >
> > > > Is there any opportunity to add a more generic "compatible" string in
> > > > addition to those so this list doesn't have to be updated for every
> > > > PMU?  The .data here is "wlan" in both cases, and for this purpose, we
> > > > don't care whether it's "pci17cb,1101" or "pci17cb,1107".
> > >
> > > These two devices have different set of regulators and different
> > > requirements to power them on.
> >
> > Right, but I don't think pci_pwrctl_pwrseq_probe() knows about those
> > different sets.  It basically looks like:
> >
> >   pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
> >   {
> >     struct pci_pwrctl_pwrseq_data *data;
> >     struct device *dev = &pdev->dev;
> >
> >     data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev));
> >     pwrseq_power_on(data->pwrseq);
> >     data->ctx.dev = dev;
> >     devm_pci_pwrctl_device_set_ready(dev, &data->ctx);
> >   }
> >
> > I think of_device_get_match_data(dev) will return "wlan" for both
> > "pci17cb,1101" and "pci17cb,1107", so devm_pwrseq_get(),
> > pwrseq_power_on(), and devm_pci_pwrctl_device_set_ready() don't see
> > the distinction between them.
> 
> These are only the first two users of this generic driver. We may end
> up adding more that will use different targets or even extend the
> match data with additional fields.

If that were the only reason, I would suggest waiting to add the
specific device strings until we need the functionality, but it sounds
like there are other stronger reasons.

> > Of course, they also get "dev", so they can find the device-specifc
> > stuff that way, but I think that's on the drivers/power/sequencing/
> > side, not in this pci-pwrctl-pwrseq driver itself.
> >
> > So what if there were a more generic "compatible" string, e.g., if the
> > DT contained something like this:
> >
> >   wifi@0 {
> >     compatible = "pci17cb,1101", "wlan-pwrseq";
> 
> What even is "pwrseq" in the context of the hardware description? DT
> maintainers would like to have a word with you. :)

There are "compatible" strings like "simple-bus", "simple-mfd", and
"syscon" that allow drivers to bind and provide generic functionality
when they don't need to know the exact hardware.

> > and pci_pwrctl_pwrseq_of_match[] had this:
> >
> >   { .compatible = "wlan-pwrseq", .data = "wlan", }
> >
> > Wouldn't this pci-pwrctl-pwrseq driver work the same?  I'm not a DT
> > whiz, so likely I'm missing something, but it would be nice if we
> > didn't have to update this very generic-looking driver to add every
> > device that needs it.

Do you have any other ideas to reduce the churn in this file?  It just
seems weird to have to add an ID to this file without adding any
actual code or data related to it.

We should probably also add a pattern to MAINTAINERS so
get_maintainers.pl on this file will show you as a maintainer.

Bjorn