Message-ID: <CAMRc=McDYL_B+hFtLekevtB2XpUkaMN1dsDNeefvR+ppj4whFg@mail.gmail.com>
Date: Thu, 6 Nov 2025 10:53:05 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, Stephan Gerhold <stephan.gerhold@...aro.org>, 
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Subject: Re: [PATCH 4/4] power: sequencing: Add the Power Sequencing driver
 for the PCIe M.2 connectors

On Wed, Nov 5, 2025 at 6:46 PM Manivannan Sadhasivam <mani@...nel.org> wrote:
>
> On Wed, Nov 05, 2025 at 05:21:46PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 5, 2025 at 10:17 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@....qualcomm.com> wrote:
> > >
> > > This driver is used to control the PCIe M.2 connectors of different
> > > Mechanical Keys attached to the host machines and supporting different
> > > interfaces like PCIe/SATA, USB/UART etc...
> > >
> > > Currently, this driver supports only the Mechanical Key M connectors with
> > > PCIe interface. The driver also only supports driving the mandatory 3.3v
> > > and optional 1.8v power supplies. The optional signals of the Key M
> > > connectors are not currently supported.
> > >
> >
> > I'm assuming you followed some of the examples from the existing WCN
> > power sequencing driver. Not all of them are good or matching this
> > one, please see below.
> >
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > > ---
> > >  MAINTAINERS                               |   7 ++
> > >  drivers/power/sequencing/Kconfig          |   8 ++
> > >  drivers/power/sequencing/Makefile         |   1 +
> > >  drivers/power/sequencing/pwrseq-pcie-m2.c | 138 ++++++++++++++++++++++++++++++
> > >  4 files changed, 154 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 46126ce2f968e4f9260263f1574ee29f5ff0de1c..9b3f689d1f50c62afa3772a0c6802f99a98ac2de 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -20474,6 +20474,13 @@ F:     Documentation/driver-api/pwrseq.rst
> > >  F:     drivers/power/sequencing/
> > >  F:     include/linux/pwrseq/
> > >
> > > +PCIE M.2 POWER SEQUENCING
> > > +M:     Manivannan Sadhasivam <mani@...nel.org>
> > > +L:     linux-pci@...r.kernel.org
> > > +S:     Maintained
> > > +F:     Documentation/devicetree/bindings/connector/pcie-m2-m-connector.yaml
> > > +F:     drivers/power/sequencing/pwrseq-pcie-m2.c
> > > +
> > >  POWER STATE COORDINATION INTERFACE (PSCI)
> > >  M:     Mark Rutland <mark.rutland@....com>
> > >  M:     Lorenzo Pieralisi <lpieralisi@...nel.org>
> > > diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
> > > index 280f92beb5d0ed524e67a28d1c5dd264bbd6c87e..f5fff84566ba463b55d3cd0c07db34c82f9f1e31 100644
> > > --- a/drivers/power/sequencing/Kconfig
> > > +++ b/drivers/power/sequencing/Kconfig
> > > @@ -35,4 +35,12 @@ config POWER_SEQUENCING_TH1520_GPU
> > >           GPU. This driver handles the complex clock and reset sequence
> > >           required to power on the Imagination BXM GPU on this platform.
> > >
> > > +config POWER_SEQUENCING_PCIE_M2
> > > +       tristate "PCIe M.2 connector power sequencing driver"
> > > +       depends on OF || COMPILE_TEST
> >
> > The OF dependency in the WCN driver is there because we're doing some
> > phandle parsing and inspecting the parent-child relationships of the
> > associated nodes. It doesn't look like you need it here. On the other
> > hand, if you add more logic to the match() callback, this may come
> > into play.
> >
>
> For sure the driver will build fine for !CONFIG_OF, but it is not going to work.
> And for the build coverage, COMPILE_TEST is already present. Maybe I was wrong
> to enforce functional dependency in Kconfig.
>

Given what you said below for the regulator API, let's keep it as is.

> > > +
> > > +static int pwrseq_pcie_m2_match(struct pwrseq_device *pwrseq,
> > > +                                struct device *dev)
> > > +{
> > > +       return PWRSEQ_MATCH_OK;
> >
> > Eek! That will match any device we check. I'm not sure this is what
> > you want. Looking at the binding example, I assume struct device *
> > here will be the endpoint? If so, you should resolve it and confirm
> > it's the one referenced from the connector node.
> >
>
> I was expecting this question, so returned PWRSEQ_MATCH_OK on purpose. I feel it
> is redundant to have match callback that just does link resolution and matches
> the of_node of the caller. Can't we have a default match callback that does just
> this?
>

To be clear: the above is certainly wrong. Any power sequencing
consumer would match against this device.

To answer your question: sure, there is nothing wrong with having a
default match callback but first: I'd like to see more than one user
before we generalize it, and second: it still needs some logic. What
is the relationship between the firmware nodes of dev and pwrseq here
exactly?

> > > +       if (!ctx->pdata)
> > > +               return dev_err_probe(dev, -ENODEV,
> > > +                                    "Failed to obtain platform data\n");
> > > +
> > > +       ret = of_regulator_bulk_get_all(dev, dev_of_node(dev), &ctx->regs);
> >
> > Same here, you already have the device, no need to get the regulators
> > through the OF node. Just use devm_regulator_bulk_get()
> >
>
> I used it on purpose. This is the only regulator API that just gets all
> regulators defined in the devicetree node without complaining. Here, 3.3v is
> mandatory and 1.8v is optional. There could be other supplies in the future and
> I do not want to hardcode the supply names in the driver. IMO, the driver should
> trust devicetree to supply enough supplies and it should just consume them
> instead of doing validation. I proposed to add a devm_ variant for this, but
> Mark was against that idea.
>

What was the reason for being against it? Anyway: in that case, would
you mind adding a comment containing what you wrote here so that
people don't mindlessly try convert it to the regular variant in the
future?

Bart

Powered by blists - more mailing lists