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.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.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@kernel.org>
+L:	linux-pci@vger.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@arm.com>
 M:	Lorenzo Pieralisi <lpieralisi@kernel.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
+	help
+	  Say Y here to enable the power sequencing driver for PCIe M.2
+	  connectors. This driver handles the power sequencing for the M.2
+	  connectors exposing multiple interfaces like PCIe, SATA, UART, etc...
+
 endif
diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
index 96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1..0911d461829897c5018e26dbe475b28f6fb6914c 100644
--- a/drivers/power/sequencing/Makefile
+++ b/drivers/power/sequencing/Makefile
@@ -5,3 +5,4 @@ pwrseq-core-y				:= core.o
 
 obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)	+= pwrseq-qcom-wcn.o
 obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
+obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2)	+= pwrseq-pcie-m2.o
diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
new file mode 100644
index 0000000000000000000000000000000000000000..b9f68ee9c5a377ce900a88de86a3e269f9c99e51
--- /dev/null
+++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
+ */
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct pwrseq_pcie_m2_pdata {
+	const struct pwrseq_target_data **targets;
+};
+
+struct pwrseq_pcie_m2_ctx {
+	struct pwrseq_device *pwrseq;
+	const struct pwrseq_pcie_m2_pdata *pdata;
+	struct regulator_bulk_data *regs;
+	size_t num_vregs;
+	struct notifier_block nb;
+};
+
+static int pwrseq_pcie_m2_m_vregs_enable(struct pwrseq_device *pwrseq)
+{
+	struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+	return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
+}
+
+static int pwrseq_pcie_m2_m_vregs_disable(struct pwrseq_device *pwrseq)
+{
+	struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+	return regulator_bulk_disable(ctx->num_vregs, ctx->regs);
+}
+
+static const struct pwrseq_unit_data pwrseq_pcie_m2_vregs_unit_data = {
+	.name = "regulators-enable",
+	.enable = pwrseq_pcie_m2_m_vregs_enable,
+	.disable = pwrseq_pcie_m2_m_vregs_disable,
+};
+
+static const struct pwrseq_unit_data *pwrseq_pcie_m2_m_unit_deps[] = {
+	&pwrseq_pcie_m2_vregs_unit_data,
+	NULL
+};
+
+static const struct pwrseq_unit_data pwrseq_pcie_m2_m_pcie_unit_data = {
+	.name = "pcie-enable",
+	.deps = pwrseq_pcie_m2_m_unit_deps,
+};
+
+static const struct pwrseq_target_data pwrseq_pcie_m2_m_pcie_target_data = {
+	.name = "pcie",
+	.unit = &pwrseq_pcie_m2_m_pcie_unit_data,
+};
+
+static const struct pwrseq_target_data *pwrseq_pcie_m2_m_targets[] = {
+	&pwrseq_pcie_m2_m_pcie_target_data,
+	NULL
+};
+
+static const struct pwrseq_pcie_m2_pdata pwrseq_pcie_m2_m_of_data = {
+	.targets = pwrseq_pcie_m2_m_targets,
+};
+
+static int pwrseq_pcie_m2_match(struct pwrseq_device *pwrseq,
+				 struct device *dev)
+{
+	return PWRSEQ_MATCH_OK;
+}
+
+static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwrseq_pcie_m2_ctx *ctx;
+	struct pwrseq_config config;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->pdata = of_device_get_match_data(dev);
+	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);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to get all regulators\n");
+
+	ctx->num_vregs = ret;
+
+	memset(&config, 0, sizeof(config));
+
+	config.parent = dev;
+	config.owner = THIS_MODULE;
+	config.drvdata = ctx;
+	config.match = pwrseq_pcie_m2_match;
+	config.targets = ctx->pdata->targets;
+
+	ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+	if (IS_ERR(ctx->pwrseq))
+		return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+				     "Failed to register the power sequencer\n");
+
+	return 0;
+}
+
+static const struct of_device_id pwrseq_pcie_m2_of_match[] = {
+	{
+		.compatible = "pcie-m2-m-connector",
+		.data = &pwrseq_pcie_m2_m_of_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwrseq_pcie_m2_of_match);
+
+static struct platform_driver pwrseq_pcie_m2_driver = {
+	.driver = {
+		.name = "pwrseq-pcie-m2",
+		.of_match_table = pwrseq_pcie_m2_of_match,
+	},
+	.probe = pwrseq_pcie_m2_probe,
+};
+module_platform_driver(pwrseq_pcie_m2_driver);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>");
+MODULE_DESCRIPTION("Power Sequencing driver for PCIe M.2 connector");
+MODULE_LICENSE("GPL");

-- 
2.48.1

On Wed, Nov 05, 2025 at 02:45:52PM +0530, Manivannan Sadhasivam 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.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.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@kernel.org>
> +L:	linux-pci@vger.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@arm.com>
>  M:	Lorenzo Pieralisi <lpieralisi@kernel.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
> +	help
> +	  Say Y here to enable the power sequencing driver for PCIe M.2
> +	  connectors. This driver handles the power sequencing for the M.2
> +	  connectors exposing multiple interfaces like PCIe, SATA, UART, etc...
> +
>  endif
> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> index 96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1..0911d461829897c5018e26dbe475b28f6fb6914c 100644
> --- a/drivers/power/sequencing/Makefile
> +++ b/drivers/power/sequencing/Makefile
> @@ -5,3 +5,4 @@ pwrseq-core-y				:= core.o
>
>  obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)	+= pwrseq-qcom-wcn.o
>  obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
> +obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2)	+= pwrseq-pcie-m2.o
> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b9f68ee9c5a377ce900a88de86a3e269f9c99e51
> --- /dev/null
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
...
> +
> +static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pwrseq_pcie_m2_ctx *ctx;
> +	struct pwrseq_config config;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->pdata = of_device_get_match_data(dev);
> +	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);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get all regulators\n");
> +
> +	ctx->num_vregs = ret;
> +
> +	memset(&config, 0, sizeof(config));
> +
> +	config.parent = dev;
> +	config.owner = THIS_MODULE;
> +	config.drvdata = ctx;
> +	config.match = pwrseq_pcie_m2_match;
> +	config.targets = ctx->pdata->targets;
> +
> +	ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> +	if (IS_ERR(ctx->pwrseq))
> +		return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> +				     "Failed to register the power sequencer\n");

Does need free resources allocated by of_regulator_bulk_get_all() here?

Frank

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pwrseq_pcie_m2_of_match[] = {
> +	{
> +		.compatible = "pcie-m2-m-connector",
> +		.data = &pwrseq_pcie_m2_m_of_data,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pwrseq_pcie_m2_of_match);
> +
> +static struct platform_driver pwrseq_pcie_m2_driver = {
> +	.driver = {
> +		.name = "pwrseq-pcie-m2",
> +		.of_match_table = pwrseq_pcie_m2_of_match,
> +	},
> +	.probe = pwrseq_pcie_m2_probe,
> +};
> +module_platform_driver(pwrseq_pcie_m2_driver);
> +
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>");
> +MODULE_DESCRIPTION("Power Sequencing driver for PCIe M.2 connector");
> +MODULE_LICENSE("GPL");
>
> --
> 2.48.1
>

On Wed, Nov 05, 2025 at 12:23:22PM -0500, Frank Li wrote:
> On Wed, Nov 05, 2025 at 02:45:52PM +0530, Manivannan Sadhasivam 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.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.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@kernel.org>
> > +L:	linux-pci@vger.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@arm.com>
> >  M:	Lorenzo Pieralisi <lpieralisi@kernel.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
> > +	help
> > +	  Say Y here to enable the power sequencing driver for PCIe M.2
> > +	  connectors. This driver handles the power sequencing for the M.2
> > +	  connectors exposing multiple interfaces like PCIe, SATA, UART, etc...
> > +
> >  endif
> > diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> > index 96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1..0911d461829897c5018e26dbe475b28f6fb6914c 100644
> > --- a/drivers/power/sequencing/Makefile
> > +++ b/drivers/power/sequencing/Makefile
> > @@ -5,3 +5,4 @@ pwrseq-core-y				:= core.o
> >
> >  obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)	+= pwrseq-qcom-wcn.o
> >  obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
> > +obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2)	+= pwrseq-pcie-m2.o
> > diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b9f68ee9c5a377ce900a88de86a3e269f9c99e51
> > --- /dev/null
> > +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> ...
> > +
> > +static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct pwrseq_pcie_m2_ctx *ctx;
> > +	struct pwrseq_config config;
> > +	int ret;
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->pdata = of_device_get_match_data(dev);
> > +	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);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to get all regulators\n");
> > +
> > +	ctx->num_vregs = ret;
> > +
> > +	memset(&config, 0, sizeof(config));
> > +
> > +	config.parent = dev;
> > +	config.owner = THIS_MODULE;
> > +	config.drvdata = ctx;
> > +	config.match = pwrseq_pcie_m2_match;
> > +	config.targets = ctx->pdata->targets;
> > +
> > +	ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> > +	if (IS_ERR(ctx->pwrseq))
> > +		return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> > +				     "Failed to register the power sequencer\n");
> 
> Does need free resources allocated by of_regulator_bulk_get_all() here?
> 

Yes, missed it during cleanup. Will fix.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Wed, Nov 5, 2025 at 10:17 AM Manivannan Sadhasivam
<manivannan.sadhasivam@oss.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@oss.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@kernel.org>
> +L:     linux-pci@vger.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@arm.com>
>  M:     Lorenzo Pieralisi <lpieralisi@kernel.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.

> +       help
> +         Say Y here to enable the power sequencing driver for PCIe M.2
> +         connectors. This driver handles the power sequencing for the M.2
> +         connectors exposing multiple interfaces like PCIe, SATA, UART, etc...
> +
>  endif
> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> index 96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1..0911d461829897c5018e26dbe475b28f6fb6914c 100644
> --- a/drivers/power/sequencing/Makefile
> +++ b/drivers/power/sequencing/Makefile
> @@ -5,3 +5,4 @@ pwrseq-core-y                           := core.o
>
>  obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)        += pwrseq-qcom-wcn.o
>  obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
> +obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2) += pwrseq-pcie-m2.o
> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b9f68ee9c5a377ce900a88de86a3e269f9c99e51
> --- /dev/null
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwrseq/provider.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +struct pwrseq_pcie_m2_pdata {
> +       const struct pwrseq_target_data **targets;
> +};
> +
> +struct pwrseq_pcie_m2_ctx {
> +       struct pwrseq_device *pwrseq;
> +       const struct pwrseq_pcie_m2_pdata *pdata;
> +       struct regulator_bulk_data *regs;
> +       size_t num_vregs;
> +       struct notifier_block nb;
> +};
> +
> +static int pwrseq_pcie_m2_m_vregs_enable(struct pwrseq_device *pwrseq)
> +{
> +       struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> +       return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
> +}
> +
> +static int pwrseq_pcie_m2_m_vregs_disable(struct pwrseq_device *pwrseq)
> +{
> +       struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> +       return regulator_bulk_disable(ctx->num_vregs, ctx->regs);
> +}
> +
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_vregs_unit_data = {
> +       .name = "regulators-enable",
> +       .enable = pwrseq_pcie_m2_m_vregs_enable,
> +       .disable = pwrseq_pcie_m2_m_vregs_disable,
> +};
> +
> +static const struct pwrseq_unit_data *pwrseq_pcie_m2_m_unit_deps[] = {
> +       &pwrseq_pcie_m2_vregs_unit_data,
> +       NULL
> +};
> +
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_m_pcie_unit_data = {
> +       .name = "pcie-enable",
> +       .deps = pwrseq_pcie_m2_m_unit_deps,
> +};
> +
> +static const struct pwrseq_target_data pwrseq_pcie_m2_m_pcie_target_data = {
> +       .name = "pcie",
> +       .unit = &pwrseq_pcie_m2_m_pcie_unit_data,
> +};
> +
> +static const struct pwrseq_target_data *pwrseq_pcie_m2_m_targets[] = {
> +       &pwrseq_pcie_m2_m_pcie_target_data,
> +       NULL
> +};
> +
> +static const struct pwrseq_pcie_m2_pdata pwrseq_pcie_m2_m_of_data = {
> +       .targets = pwrseq_pcie_m2_m_targets,
> +};
> +
> +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.

> +}
> +
> +static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct pwrseq_pcie_m2_ctx *ctx;
> +       struct pwrseq_config config;
> +       int ret;
> +
> +       ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx->pdata = of_device_get_match_data(dev);

I should probably address it in the WCN driver - you don't need to use
the OF variant, use device_get_match_data().

> +       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()

> +       if (ret < 0)
> +               return dev_err_probe(dev, ret,
> +                                    "Failed to get all regulators\n");
> +
> +       ctx->num_vregs = ret;
> +
> +       memset(&config, 0, sizeof(config));

Just do config = { }; above?


> +
> +       config.parent = dev;
> +       config.owner = THIS_MODULE;
> +       config.drvdata = ctx;
> +       config.match = pwrseq_pcie_m2_match;
> +       config.targets = ctx->pdata->targets;
> +
> +       ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> +       if (IS_ERR(ctx->pwrseq))
> +               return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> +                                    "Failed to register the power sequencer\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id pwrseq_pcie_m2_of_match[] = {
> +       {
> +               .compatible = "pcie-m2-m-connector",
> +               .data = &pwrseq_pcie_m2_m_of_data,
> +       },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, pwrseq_pcie_m2_of_match);
> +
> +static struct platform_driver pwrseq_pcie_m2_driver = {
> +       .driver = {
> +               .name = "pwrseq-pcie-m2",
> +               .of_match_table = pwrseq_pcie_m2_of_match,
> +       },
> +       .probe = pwrseq_pcie_m2_probe,
> +};
> +module_platform_driver(pwrseq_pcie_m2_driver);
> +
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>");
> +MODULE_DESCRIPTION("Power Sequencing driver for PCIe M.2 connector");
> +MODULE_LICENSE("GPL");
>
> --
> 2.48.1
>

Bartosz

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@oss.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@oss.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@kernel.org>
> > +L:     linux-pci@vger.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@arm.com>
> >  M:     Lorenzo Pieralisi <lpieralisi@kernel.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.

> > +       help
> > +         Say Y here to enable the power sequencing driver for PCIe M.2
> > +         connectors. This driver handles the power sequencing for the M.2
> > +         connectors exposing multiple interfaces like PCIe, SATA, UART, etc...
> > +
> >  endif
> > diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> > index 96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1..0911d461829897c5018e26dbe475b28f6fb6914c 100644
> > --- a/drivers/power/sequencing/Makefile
> > +++ b/drivers/power/sequencing/Makefile
> > @@ -5,3 +5,4 @@ pwrseq-core-y                           := core.o
> >
> >  obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)        += pwrseq-qcom-wcn.o
> >  obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
> > +obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2) += pwrseq-pcie-m2.o
> > diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b9f68ee9c5a377ce900a88de86a3e269f9c99e51
> > --- /dev/null
> > +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> > @@ -0,0 +1,138 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwrseq/provider.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +
> > +struct pwrseq_pcie_m2_pdata {
> > +       const struct pwrseq_target_data **targets;
> > +};
> > +
> > +struct pwrseq_pcie_m2_ctx {
> > +       struct pwrseq_device *pwrseq;
> > +       const struct pwrseq_pcie_m2_pdata *pdata;
> > +       struct regulator_bulk_data *regs;
> > +       size_t num_vregs;
> > +       struct notifier_block nb;
> > +};
> > +
> > +static int pwrseq_pcie_m2_m_vregs_enable(struct pwrseq_device *pwrseq)
> > +{
> > +       struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> > +
> > +       return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
> > +}
> > +
> > +static int pwrseq_pcie_m2_m_vregs_disable(struct pwrseq_device *pwrseq)
> > +{
> > +       struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> > +
> > +       return regulator_bulk_disable(ctx->num_vregs, ctx->regs);
> > +}
> > +
> > +static const struct pwrseq_unit_data pwrseq_pcie_m2_vregs_unit_data = {
> > +       .name = "regulators-enable",
> > +       .enable = pwrseq_pcie_m2_m_vregs_enable,
> > +       .disable = pwrseq_pcie_m2_m_vregs_disable,
> > +};
> > +
> > +static const struct pwrseq_unit_data *pwrseq_pcie_m2_m_unit_deps[] = {
> > +       &pwrseq_pcie_m2_vregs_unit_data,
> > +       NULL
> > +};
> > +
> > +static const struct pwrseq_unit_data pwrseq_pcie_m2_m_pcie_unit_data = {
> > +       .name = "pcie-enable",
> > +       .deps = pwrseq_pcie_m2_m_unit_deps,
> > +};
> > +
> > +static const struct pwrseq_target_data pwrseq_pcie_m2_m_pcie_target_data = {
> > +       .name = "pcie",
> > +       .unit = &pwrseq_pcie_m2_m_pcie_unit_data,
> > +};
> > +
> > +static const struct pwrseq_target_data *pwrseq_pcie_m2_m_targets[] = {
> > +       &pwrseq_pcie_m2_m_pcie_target_data,
> > +       NULL
> > +};
> > +
> > +static const struct pwrseq_pcie_m2_pdata pwrseq_pcie_m2_m_of_data = {
> > +       .targets = pwrseq_pcie_m2_m_targets,
> > +};
> > +
> > +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?

> > +}
> > +
> > +static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct pwrseq_pcie_m2_ctx *ctx;
> > +       struct pwrseq_config config;
> > +       int ret;
> > +
> > +       ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +       if (!ctx)
> > +               return -ENOMEM;
> > +
> > +       ctx->pdata = of_device_get_match_data(dev);
> 
> I should probably address it in the WCN driver - you don't need to use
> the OF variant, use device_get_match_data().
> 

Ok.

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

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Wed, Nov 5, 2025 at 6:46 PM Manivannan Sadhasivam <mani@kernel.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@oss.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@oss.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@kernel.org>
> > > +L:     linux-pci@vger.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@arm.com>
> > >  M:     Lorenzo Pieralisi <lpieralisi@kernel.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

On Thu, Nov 06, 2025 at 10:53:05AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 5, 2025 at 6:46 PM Manivannan Sadhasivam <mani@kernel.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@oss.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@oss.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@kernel.org>
> > > > +L:     linux-pci@vger.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@arm.com>
> > > >  M:     Lorenzo Pieralisi <lpieralisi@kernel.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?
> 

The 'dev' belongs to the PCIe Root Port node where the graph port is defined:

&pcie6_port0 {
	...
	port {
		pcie6a_port0_ep: endpoint {
			remote-endpoint = <&m2_pcie_ep>;
		};
	};
};

So I have to do remote-endpoint lookup from the pwrseq and compare the of_node
of the parent with 'dev->of_node', I believe. If so, this looks like a common
pattern.

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

Mark doesn't want the drivers to trust DT for regulators. It was an IRC
discussion and the conclusion was the drivers had to specify the supplies
manually and not trust DT to provide required regulators. But then we already
have this API that does the same.

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

Sure.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Thu, Nov 6, 2025 at 3:32 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> >
> > 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?
> >
>
> The 'dev' belongs to the PCIe Root Port node where the graph port is defined:
>
> &pcie6_port0 {
>         ...
>         port {
>                 pcie6a_port0_ep: endpoint {
>                         remote-endpoint = <&m2_pcie_ep>;
>                 };
>         };
> };
>
> So I have to do remote-endpoint lookup from the pwrseq and compare the of_node
> of the parent with 'dev->of_node', I believe. If so, this looks like a common
> pattern.
>

Sounds right. I would still keep it in this driver until we have at
least a second user that wants to do the same thing.

Bartosz