[PATCH v2 0/4] pinctrl: qcom: spmi-gpio: Add bidirectional level-shifter function support

[PATCH v2 0/4] pinctrl: qcom: spmi-gpio: Add bidirectional level-shifter function support

[Fenglin Wu]

The PMH0101 PMIC introduces BIDIR_LVL_SHIFTER modules that provide
bidirectional voltage translation between 1.2 V and 1.8 V power
domains, targeting open-drain signal buses such as I2C. Each level
shifter shares its two physical pins with a corresponding pair of GPIO
modules, and its enable state is centrally managed by AOP firmware as
a shared RPMh "XOB" resource.

This series adds kernel support for controlling these level shifters
through the pinctrl subsystem. Patches are ordered from infrastructure
to driver to bindings:

Patch 1 (soc: qcom: rpmh) extends the RPMh driver to accept write
commands from devices that are not children of the RPMh controller.
The existing write path assumes the caller is a child of the RSC device;
however, the SPMI GPIO driver sits under the SPMI controller bus. Two
new APIs are introduced — rpmh_get_ctrlr_dev() for resolving the
controller from a "qcom,rpmh" DT phandle, and rpmh_write_ctrlr() /
rpmh_write_async_ctrlr() which accept the controller device pointer
directly rather than deriving it through the device parent chain.

Patch 2 (dt-bindings) documents the new "level-shifter" function, the
qcom,1p2v-1p8v-ls-en property, the qcom,rpmh phandle, and the
qcom,pmic-id string required on pmh0101 nodes. The pmh0101 conditional
block is split out from pmih0108 and given its own required properties.

Patch 3 (pinctrl: qcom: spmi-gpio, rearchitect) refactors the driver's
group and function registration to use the generic pinctrl group and
function APIs (pinctrl_generic_add_group, pinmux_generic_add_function).
The previous design treated every pin as its own group with access to
all functions. The new design allows multi-pin groups with restricted
function sets, which is a prerequisite for exposing the level-shifter
function that is tied to specific GPIO pairs only.

Patch 4 (pinctrl: qcom: spmi-gpio, level-shifter) builds on the above
to introduce the "level-shifter" function. When selected, both GPIO
pads in the pair are disabled (high-impedance), and the RPMh XOB vote
for the level shifter is controlled separately via the new
qcom,1p2v-1p8v-ls-en pinconf parameter, allowing enable and disable
to be represented as distinct pinctrl states with the same function and
group.

With all these changes, the BIDIR_LVL_SHIFTER in PMH0101 could be
controlled with following settings:

    &pmh0101_gpios {
    	qcom,rpmh = <&apps_rsc>;
    	qcom,pmic-id = "B_E0";

    	pmh0101-ls1-en {
    	        groups = "gpio11, gpio12";
    	        function = "level-shifter";
    	        qcom,1p2v-1p8v-ls-en = <1>;
    	};

    	pmh0101-ls1-dis {
    	        groups = "gpio11, gpio12";
    	        function = "level-shifter";
    	        qcom,1p2v-1p8v-ls-en = <0>;
    	};
    };

Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
---
Changes in v2:
Add following change to address review comments from Sashiko AI:

- PATCH 1, in drivers/soc/qcom/rpmh.c
  1. Add rpmh_put_device() wrapper to avoid casting put_device directly to
     void (*)(void *) in devm_add_action_or_reset(), which would trigger a
     CFI kernel panic on indirect call type mismatch.
  2. Add device_link_add() with DL_FLAG_AUTOREMOVE_CONSUMER | DL_FLAG_PM_RUNTIME
     in rpmh_get_ctrlr_dev() to enforce probe/remove ordering and runtime PM
     suspend/resume ordering between the RPMH controller and non-child consumer
     devices, preventing use-after-free and hardware state machine violations on unbind.

- PATCH 2, in Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
  1. Make qcom,rpmh and qcom,pmic-id optional for qcom,pmh0101-gpio by removing
     them from the required list in the if/then conditional block. Update property
     descriptions to clarify they are only needed when the level-shifter function is used.

- PATCH 3, in drivers/pinctrl/qcom/Kconfig
  1. Add select GENERIC_PINCTRL_GROUPS and select GENERIC_PINMUX_FUNCTIONS to
     PINCTRL_QCOM_SPMI_PMIC to declare explicit dependencies on the generic pinctrl
     APIs (pinctrl_generic_add_group(), pinmux_generic_add_function(), etc.) used by the driver.

- PATCH 3, in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
  1. Switch from devm_pinctrl_register() to devm_pinctrl_register_and_init() + pinctrl_enable()
     to defer hog application until after all pin groups, functions, are fully registered,
     fixing a probe sequence bug where hogs would be applied against an incomplete pinctrl state.

- PATCH 4, in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
  1. Guard pad->is_enabled = true in pmic_gpio_pinconf_pin_set() to skip the assignment
     when pad->function == PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER, preventing pinconf
     application from re-enabling GPIO pads that were explicitly disabled by the
     level-shifter mux path.
  2. Call cmd_db_ready() before cmd_db_read_addr() in pmic_gpio_register_level_shifters()
     to propagate -EPROBE_DEFER when the command DB is not yet initialized, instead of
     incorrectly returning -ENODEV which causes a permanent probe failure.
  3. Add qcom,rpmh / qcom,pmic-id DT property check before calling pmic_gpio_register_level_shifters()
     add make the level shifter function registration optional to match with the DT binding change.

- Link to v1: https://patch.msgid.link/20260527-pinctrl-level-shifter-v1-0-1965461d0a7c@oss.qualcomm.com

---
Fenglin Wu (4):
      soc: qcom: rpmh: Allow non-child devices to issue write commands
      dt-bindings: pinctrl: qcom,pmic-gpio: Add level-shifter function
      pinctrl: qcom: spmi-gpio: Rearchitect for flexible group support
      pinctrl: qcom: spmi-gpio: Add level-shifter function support

 .../bindings/pinctrl/qcom,pmic-gpio.yaml           |  66 +-
 drivers/pinctrl/qcom/Kconfig                       |   2 +
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c           | 796 ++++++++++++++++-----
 drivers/soc/qcom/rpmh.c                            | 173 ++++-
 include/dt-bindings/pinctrl/qcom,pmic-gpio.h       |   1 +
 include/soc/qcom/rpmh.h                            |  21 +
 6 files changed, 845 insertions(+), 214 deletions(-)
---
base-commit: b8f192cec7dcb2e4f04ee57ab78d51777b0a5729
change-id: 20260527-pinctrl-level-shifter-929b286f583d

Best regards,
--  
Fenglin Wu <fenglin.wu@oss.qualcomm.com>

[PATCH v2 1/4] soc: qcom: rpmh: Allow non-child devices to issue write commands

[Fenglin Wu]

Currently, the RPMH driver only allows child devices of the RPMH
controller to issue commands, as it assumes dev->parent points to the
RSC device.

There is a possibility that certain devices which are not children of
the RPMH controller want to send commands for special control at the
RPMH side. For example, in PMH0101 PMICs, there are bidirectional
level shifter (LS) peripherals, and each LS works with a pair of PMIC
GPIOs. The control of the LS, which is combined with the GPIO
configuration, is handled by RPMH firmware for sharing the resource
between different subsystems. From a hardware point of view, the LS
functionality is tied to a pair of PMIC GPIOs, so its control is more
suitable to be added in the pinctrl-spmi-gpio driver by adding the
level-shifter function. However, the pinctrl-spmi-gpio device is a
child device of the SPMI controller, not the RPMH controller.

This patch extends the RPMH driver to support write commands from any
device that has a pointer to the RPMH controller device:

1. Add rpmh_get_ctrlr_dev() to lookup controller via device tree
   phandle "qcom,rpmh"
2. Add new APIs: rpmh_write_async_ctrlr() and rpmh_write_ctrlr()
   that accept controller device pointer directly

With this change, the pinctrl-spmi-gpio driver is able to issue write
commands to the RPMH controller by using the controller device pointer,
and vote for enabling the level-shifter function.

Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
---
 drivers/soc/qcom/rpmh.c | 173 +++++++++++++++++++++++++++++++++++++++++++-----
 include/soc/qcom/rpmh.h |  21 ++++++
 2 files changed, 179 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index ca37da3dc2b1..9dbc42b775d9 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -12,6 +12,7 @@
 #include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -76,6 +77,21 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
 	return &drv->client;
 }
 
+static struct rpmh_ctrlr *get_rpmh_ctrlr_from_dev(const struct device *ctrl_dev)
+{
+	struct rsc_drv *drv;
+
+	if (!ctrl_dev)
+		return ERR_PTR(-EINVAL);
+
+	drv = dev_get_drvdata(ctrl_dev);
+
+	if (!drv)
+		return ERR_PTR(-ENODEV);
+
+	return &drv->client;
+}
+
 void rpmh_tx_done(const struct tcs_request *msg)
 {
 	struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
@@ -156,23 +172,11 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
 	return req;
 }
 
-/**
- * __rpmh_write: Cache and send the RPMH request
- *
- * @dev: The device making the request
- * @state: Active/Sleep request type
- * @rpm_msg: The data that needs to be sent (cmds).
- *
- * Cache the RPMH request and send if the state is ACTIVE_ONLY.
- * SLEEP/WAKE_ONLY requests are not sent to the controller at
- * this time. Use rpmh_flush() to send them to the controller.
- */
-static int __rpmh_write(const struct device *dev, enum rpmh_state state,
-			struct rpmh_request *rpm_msg)
+static int __rpmh_write_direct(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
+			       struct rpmh_request *rpm_msg)
 {
-	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
-	int ret = -EINVAL;
 	struct cache_req *req;
+	int ret = -EINVAL;
 	int i;
 
 	/* Cache the request in our store and link the payload */
@@ -193,6 +197,25 @@ static int __rpmh_write(const struct device *dev, enum rpmh_state state,
 	return ret;
 }
 
+/**
+ * __rpmh_write: Cache and send the RPMH request
+ *
+ * @dev: The device making the request
+ * @state: Active/Sleep request type
+ * @rpm_msg: The data that needs to be sent (cmds).
+ *
+ * Cache the RPMH request and send if the state is ACTIVE_ONLY.
+ * SLEEP/WAKE_ONLY requests are not sent to the controller at
+ * this time. Use rpmh_flush() to send them to the controller.
+ */
+static int __rpmh_write(const struct device *dev, enum rpmh_state state,
+			struct rpmh_request *rpm_msg)
+{
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+
+	return __rpmh_write_direct(ctrlr, state, rpm_msg);
+}
+
 static int __fill_rpmh_msg(struct rpmh_request *req, enum rpmh_state state,
 		const struct tcs_cmd *cmd, u32 n)
 {
@@ -271,6 +294,126 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
 }
 EXPORT_SYMBOL_GPL(rpmh_write);
 
+static void rpmh_put_device(void *data)
+{
+	put_device(data);
+}
+
+/**
+ * rpmh_get_ctrlr_dev: Get RPMH controller device from device tree
+ *
+ * @dev: Device with "qcom,rpmh" phandle property
+ *
+ * Returns: Pointer to RPMH controller device, with a devm action registered
+ * on @dev to release the reference when @dev is unbound.
+ */
+struct device *rpmh_get_ctrlr_dev(struct device *dev)
+{
+	struct device_node *rpmh_np;
+	struct platform_device *pdev;
+	struct device_link *link;
+	int ret;
+
+	rpmh_np = of_parse_phandle(dev->of_node, "qcom,rpmh", 0);
+	if (!rpmh_np)
+		return ERR_PTR(-ENODEV);
+
+	pdev = of_find_device_by_node(rpmh_np);
+	of_node_put(rpmh_np);
+
+	if (!pdev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	link = device_link_add(dev, &pdev->dev,
+			       DL_FLAG_AUTOREMOVE_CONSUMER | DL_FLAG_PM_RUNTIME);
+	if (!link) {
+		put_device(&pdev->dev);
+		return ERR_PTR(-EINVAL);
+	}
+
+	ret = devm_add_action_or_reset(dev, rpmh_put_device, &pdev->dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &pdev->dev;
+}
+EXPORT_SYMBOL_GPL(rpmh_get_ctrlr_dev);
+
+/**
+ * rpmh_write_async_ctrlr: Write RPMH commands with the controller device pointer
+ *
+ * @ctrl_dev: The RPMH controller device
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The number of elements in payload
+ *
+ * Write a set of RPMH commands, the order of commands is maintained
+ * and will be sent as a single shot.
+ */
+int rpmh_write_async_ctrlr(const struct device *ctrl_dev, enum rpmh_state state,
+			   const struct tcs_cmd *cmd, u32 n)
+{
+	struct rpmh_request *rpm_msg;
+	struct rpmh_ctrlr *ctrlr;
+	int ret;
+
+	ctrlr = get_rpmh_ctrlr_from_dev(ctrl_dev);
+	if (IS_ERR(ctrlr))
+		return PTR_ERR(ctrlr);
+
+	rpm_msg = kzalloc_obj(*rpm_msg, GFP_ATOMIC);
+	if (!rpm_msg)
+		return -ENOMEM;
+	rpm_msg->needs_free = true;
+
+	ret = __fill_rpmh_msg(rpm_msg, state, cmd, n);
+	if (ret) {
+		kfree(rpm_msg);
+		return ret;
+	}
+
+	return __rpmh_write_direct(ctrlr, state, rpm_msg);
+}
+EXPORT_SYMBOL_GPL(rpmh_write_async_ctrlr);
+
+/**
+ * rpmh_write_ctrlr: Write RPMH commands and block until response,
+ * with the controller device pointer
+ *
+ * @ctrlr_dev: The RPMH controller device
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The number of elements in @cmd
+ *
+ * May sleep. Do not call from atomic contexts.
+ */
+int rpmh_write_ctrlr(const struct device *ctrlr_dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 n)
+{
+	DECLARE_COMPLETION_ONSTACK(compl);
+	/* dev is unused in the synchronous non-batch path; pass NULL */
+	DEFINE_RPMH_MSG_ONSTACK(NULL, state, &compl, rpm_msg);
+	struct rpmh_ctrlr *ctrlr;
+	int ret;
+
+	ctrlr = get_rpmh_ctrlr_from_dev(ctrlr_dev);
+	if (IS_ERR(ctrlr))
+		return PTR_ERR(ctrlr);
+
+	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n);
+	if (ret)
+		return ret;
+
+	ret = __rpmh_write_direct(ctrlr, state, &rpm_msg);
+	if (ret)
+		return ret;
+
+	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
+	WARN_ON(!ret);
+	return (ret > 0) ? 0 : -ETIMEDOUT;
+}
+EXPORT_SYMBOL_GPL(rpmh_write_ctrlr);
+
 static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
 {
 	unsigned long flags;
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index bdbee1a97d36..90ddcd7ca2fe 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -22,6 +22,14 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 
 void rpmh_invalidate(const struct device *dev);
 
+struct device *rpmh_get_ctrlr_dev(struct device *dev);
+
+int rpmh_write_async_ctrlr(const struct device *ctrl_dev, enum rpmh_state state,
+			   const struct tcs_cmd *cmd, u32 n);
+
+int rpmh_write_ctrlr(const struct device *ctrlr_dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 n);
+
 #else
 
 static inline int rpmh_write(const struct device *dev, enum rpmh_state state,
@@ -42,6 +50,19 @@ static inline void rpmh_invalidate(const struct device *dev)
 {
 }
 
+static inline struct device *rpmh_get_ctrlr_dev(struct device *dev)
+{ return ERR_PTR(-ENODEV); }
+
+static inline int rpmh_write_async_ctrlr(const struct device *ctrl_dev,
+					  enum rpmh_state state,
+					  const struct tcs_cmd *cmd, u32 n)
+{ return -ENODEV; }
+
+static inline int rpmh_write_ctrlr(const struct device *ctrlr_dev,
+				    enum rpmh_state state,
+				    const struct tcs_cmd *cmd, u32 n)
+{ return -ENODEV; }
+
 #endif /* CONFIG_QCOM_RPMH */
 
 #endif /* __SOC_QCOM_RPMH_H__ */

-- 
2.43.0

Re: [PATCH v2 1/4] soc: qcom: rpmh: Allow non-child devices to issue write commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Returning `-ENODEV` instead of `-EPROBE_DEFER` when the RPMH controller hasn't fully probed causes premature and permanent probe failures for consumers.
- [High] Missing validation of the device tree phandle target can lead to type confusion and memory corruption.

Pre-existing issues:
- [Critical] Stack variables accessed asynchronously after stack frame is destroyed on timeout, leading to Use-After-Free.
--

commit 4f9cc5ff213e0a26acf5f3c77494a0af70926c9e
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

soc: qcom: rpmh: Allow non-child devices to issue write commands

This patch extends the RPMH driver to allow any device with a pointer to
the RPMH controller device to issue write commands. It introduces
rpmh_get_ctrlr_dev() to look up the controller via a device tree phandle,
and new APIs to accept the controller device pointer directly.

> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index ca37da3dc2b14..9dbc42b775d9f 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
[ ... ]
> @@ -76,6 +77,21 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>  	return &drv->client;
>  }
>  
> +static struct rpmh_ctrlr *get_rpmh_ctrlr_from_dev(const struct device *ctrl_dev)
> +{
> +	struct rsc_drv *drv;
> +
> +	if (!ctrl_dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	drv = dev_get_drvdata(ctrl_dev);
> +
> +	if (!drv)
> +		return ERR_PTR(-ENODEV);

[Severity: High]
If the RPMH controller device is found by of_find_device_by_node() but hasn't
fully probed yet, dev_get_drvdata() will return NULL. Does returning -ENODEV
here cause a premature and permanent probe failure for consumers instead of
allowing them to gracefully defer by returning -EPROBE_DEFER?

> +
> +	return &drv->client;
> +}
[ ... ]
> +/**
> + * rpmh_get_ctrlr_dev: Get RPMH controller device from device tree
> + *
> + * @dev: Device with "qcom,rpmh" phandle property
> + *
> + * Returns: Pointer to RPMH controller device, with a devm action registered
> + * on @dev to release the reference when @dev is unbound.
> + */
> +struct device *rpmh_get_ctrlr_dev(struct device *dev)
> +{
> +	struct device_node *rpmh_np;
> +	struct platform_device *pdev;
> +	struct device_link *link;
> +	int ret;
> +
> +	rpmh_np = of_parse_phandle(dev->of_node, "qcom,rpmh", 0);
> +	if (!rpmh_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	pdev = of_find_device_by_node(rpmh_np);

[Severity: High]
If a device tree maliciously or accidentally points the "qcom,rpmh" phandle
to a non-RPMH device, is it possible for drvdata to point to a different
driver's structure when get_rpmh_ctrlr_from_dev() evaluates it?

Would this cause type confusion and memory corruption when the consumer
subsequently casts it to struct rsc_drv? Should there be a validation
check to ensure the resolved device is actually an RPMH controller?

> +	of_node_put(rpmh_np);
> +
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
[ ... ]
> +/**
> + * rpmh_write_ctrlr: Write RPMH commands and block until response,
> + * with the controller device pointer
> + *
> + * @ctrlr_dev: The RPMH controller device
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The number of elements in @cmd
> + *
> + * May sleep. Do not call from atomic contexts.
> + */
> +int rpmh_write_ctrlr(const struct device *ctrlr_dev, enum rpmh_state state,
> +		     const struct tcs_cmd *cmd, u32 n)
> +{
> +	DECLARE_COMPLETION_ONSTACK(compl);
> +	/* dev is unused in the synchronous non-batch path; pass NULL */
> +	DEFINE_RPMH_MSG_ONSTACK(NULL, state, &compl, rpm_msg);
> +	struct rpmh_ctrlr *ctrlr;
> +	int ret;
> +
> +	ctrlr = get_rpmh_ctrlr_from_dev(ctrlr_dev);
> +	if (IS_ERR(ctrlr))
> +		return PTR_ERR(ctrlr);
> +
> +	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n);
> +	if (ret)
> +		return ret;
> +
> +	ret = __rpmh_write_direct(ctrlr, state, &rpm_msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
> +	WARN_ON(!ret);
> +	return (ret > 0) ? 0 : -ETIMEDOUT;
> +}

[Severity: Critical]
This is a pre-existing issue, but can this lead to a stack use-after-free
if the timeout expires?

If wait_for_completion_timeout() times out, the function returns -ETIMEDOUT
and the local stack frame is destroyed. However, rpm_msg and compl are
allocated on the stack and the hardware transaction isn't explicitly
canceled.

When the hardware eventually completes the request, the interrupt handler
would call rpmh_tx_done() which accesses the stale rpm_msg->completion
pointer and calls complete(), writing to unmapped or reused stack memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-pinctrl-level-shifter-v2-0-3a6a025392bf@oss.qualcomm.com?part=1

[PATCH v2 2/4] dt-bindings: pinctrl: qcom,pmic-gpio: Add level-shifter function

[Fenglin Wu]

Add the "level-shifter" function and add the required DT properties to
allow RPMh firmware to control the level-shifter. Introduce a custom
pinconf parameter "qcom,1p2v-1p8v-ls-en" for enabling or disabling the
level-shifter function.

Additionally, add the "groups" property with the allowed group names
that can be used to control the level-shifter function on pmh0101.

Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
---
 .../bindings/pinctrl/qcom,pmic-gpio.yaml           | 66 +++++++++++++++++++++-
 include/dt-bindings/pinctrl/qcom,pmic-gpio.h       |  1 +
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
index b8109e6c2a10..19dc61ddff2d 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
@@ -119,6 +119,21 @@ properties:
       The first cell will be used to define gpio number and the
       second denotes the flags for this gpio
 
+  qcom,rpmh:
+    description:
+      Phandle to the RPMh controller device. Required for PMICs when the
+      bidirectional level shifters is used (e.g., pmh0101), to enable
+      communication with RPMh firmware for level shifter control.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  qcom,pmic-id:
+    description:
+      The ID of the PMIC which supports bidirectional level shifter function.
+      It is used as the RPMh resource name suffix to request control of the
+      level shifter to the RPMh firmware.
+    $ref: /schemas/types.yaml#/definitions/string
+    pattern: "^[A-N]_E[0-3]+$"
+
 additionalProperties: false
 
 required:
@@ -330,6 +345,22 @@ allOf:
           contains:
             enum:
               - qcom,pmh0101-gpio
+    then:
+      properties:
+        gpio-line-names:
+          minItems: 18
+          maxItems: 18
+        gpio-reserved-ranges:
+          minItems: 1
+          maxItems: 9
+        qcom,rpmh: true
+        qcom,pmic-id: true
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,pmih0108-gpio
     then:
       properties:
@@ -523,6 +554,19 @@ $defs:
         items:
           pattern: '^gpio([0-9]+)$'
 
+      groups:
+        $ref: /schemas/types.yaml#/definitions/string-array
+        description:
+          List of GPIO groups to apply properties to. Only valid for
+          function "level-shifter" on pmh0101. Valid groups are
+          gpio11, gpio12; gpio13, gpio14; gpio15, gpio16; gpio17, gpio18.
+        items:
+          enum:
+            - gpio11, gpio12
+            - gpio13, gpio14
+            - gpio15, gpio16
+            - gpio17, gpio18
+
       function:
         items:
           - enum:
@@ -536,6 +580,7 @@ $defs:
               - dtest4
               - func3  # supported by LV/MV GPIO subtypes
               - func4  # supported by LV/MV GPIO subtypes
+              - level-shifter  # supported only by pmh0101
 
       bias-disable: true
       bias-pull-down: true
@@ -592,9 +637,24 @@ $defs:
           configured as digital input.
         enum: [1, 2, 3, 4]
 
-    required:
-      - pins
-      - function
+      qcom,1p2v-1p8v-ls-en:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          Enable or disable the bidirectional 1.2V/1.8V level shifter
+          associated with the specified GPIO group. When set to 1, an RPMh
+          vote is sent to AOP to enable the level shifter. When set to 0,
+          the vote is withdrawn. Only valid when function is "level-shifter"
+          and groups is a level-shifter GPIO pair (e.g., "gpio11, gpio12"
+          on pmh0101).
+        enum: [0, 1]
+
+    oneOf:
+      - required:
+          - pins
+          - function
+      - required:
+          - groups
+          - function
 
     additionalProperties: false
 
diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
index e5df5ce45a0f..b0824d5eb056 100644
--- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
@@ -105,6 +105,7 @@
 #define PMIC_GPIO_FUNC_DTEST2		"dtest2"
 #define PMIC_GPIO_FUNC_DTEST3		"dtest3"
 #define PMIC_GPIO_FUNC_DTEST4		"dtest4"
+#define PMIC_GPIO_FUNC_LEVEL_SHIFTER	"level-shifter"
 
 #define PM8038_GPIO1_2_LPG_DRV		PMIC_GPIO_FUNC_FUNC1
 #define PM8038_GPIO3_5V_BOOST_EN	PMIC_GPIO_FUNC_FUNC1

-- 
2.43.0

Re: [PATCH v2 2/4] dt-bindings: pinctrl: qcom,pmic-gpio: Add level-shifter function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `qcom,rpmh` and `qcom,pmic-id` properties are globally permitted for all PMIC compatibles instead of being restricted to variants that support level-shifters.
--

commit 146a2c85c7b535972af82ea257bf85404fa5bec0
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

dt-bindings: pinctrl: qcom,pmic-gpio: Add level-shifter function

This commit adds a new level-shifter function to the pmic-gpio binding.
It introduces qcom,rpmh and qcom,pmic-id properties for the pmh0101 variant
to enable communication with the RPMh firmware, along with a custom pinconf
parameter.

> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
> @@ -119,6 +119,21 @@ properties:
>        The first cell will be used to define gpio number and the
>        second denotes the flags for this gpio
>  
> +  qcom,rpmh:
> +    description:
> +      Phandle to the RPMh controller device. Required for PMICs when the
> +      bidirectional level shifters is used (e.g., pmh0101), to enable
> +      communication with RPMh firmware for level shifter control.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  qcom,pmic-id:
> +    description:
> +      The ID of the PMIC which supports bidirectional level shifter function.
> +      It is used as the RPMh resource name suffix to request control of the
> +      level shifter to the RPMh firmware.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    pattern: "^[A-N]_E[0-3]+$"

[ ... ]

> @@ -330,6 +345,22 @@ allOf:
>            contains:
>              enum:
>                - qcom,pmh0101-gpio
> +    then:
> +      properties:
> +        gpio-line-names:
> +          minItems: 18
> +          maxItems: 18
> +        gpio-reserved-ranges:
> +          minItems: 1
> +          maxItems: 9
> +        qcom,rpmh: true
> +        qcom,pmic-id: true

[Severity: Medium]
Does this schema actually restrict qcom,rpmh and qcom,pmic-id to the pmh0101
variant as intended?

Because these properties are defined in the top-level properties block, they
are globally permitted for all PMIC compatibles. Setting them to true in this
then block is a no-op in JSON schema, as affirming a property is allowed
does not forbid it from appearing in unmatched conditionals.

Should the schema explicitly forbid these properties for unsupported variants
so they are not implicitly valid on all PMICs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-pinctrl-level-shifter-v2-0-3a6a025392bf@oss.qualcomm.com?part=2

[PATCH v2 3/4] pinctrl: qcom: spmi-gpio: Rearchitect for flexible group support

[Fenglin Wu]

Currently, the driver treats each pin as a group, and every pin or
group can function correctly in all existing functions. However,
this approach is no longer valid since some PMIC pins only operate
together in specific functions, which are limited to certain GPIO
groups. For example, in pmh0101, the level-shifter function is
supported only between GPIO pairs like GPIO11/12, GPIO13/14,
GPIO15/16, and GPIO17/18.

To better accommodate these new functions and restrict GPIOs to
those that support them, rearchitect the driver to enable flexible
pinctrl group configurations by utilizing the generic pinctrl group
and function APIs.

Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
---
 drivers/pinctrl/qcom/Kconfig             |   2 +
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 377 +++++++++++++++++++++----------
 2 files changed, 258 insertions(+), 121 deletions(-)

diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
index a09e840a01c6..80bcec33b9b8 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -25,6 +25,8 @@ config PINCTRL_QCOM_SPMI_PMIC
 	select PINMUX
 	select PINCONF
 	select GENERIC_PINCONF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
 	select GPIOLIB
 	select GPIOLIB_IRQCHIP
 	select IRQ_DOMAIN_HIERARCHY
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index cdd61dae74cf..268cfae706a8 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -24,6 +24,7 @@
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
 
 #include "../core.h"
+#include "../pinmux.h"
 #include "../pinctrl-utils.h"
 
 #define PMIC_GPIO_ADDRESS_RANGE			0x100
@@ -253,139 +254,124 @@ static int pmic_gpio_write(struct pmic_gpio_state *state,
 	return ret;
 }
 
-static int pmic_gpio_get_groups_count(struct pinctrl_dev *pctldev)
-{
-	/* Every PIN is a group */
-	return pctldev->desc->npins;
-}
-
-static const char *pmic_gpio_get_group_name(struct pinctrl_dev *pctldev,
-					    unsigned pin)
-{
-	return pctldev->desc->pins[pin].name;
-}
-
-static int pmic_gpio_get_group_pins(struct pinctrl_dev *pctldev, unsigned pin,
-				    const unsigned **pins, unsigned *num_pins)
-{
-	*pins = &pctldev->desc->pins[pin].number;
-	*num_pins = 1;
-	return 0;
-}
-
 static const struct pinctrl_ops pmic_gpio_pinctrl_ops = {
-	.get_groups_count	= pmic_gpio_get_groups_count,
-	.get_group_name		= pmic_gpio_get_group_name,
-	.get_group_pins		= pmic_gpio_get_group_pins,
+	.get_groups_count	= pinctrl_generic_get_group_count,
+	.get_group_name		= pinctrl_generic_get_group_name,
+	.get_group_pins		= pinctrl_generic_get_group_pins,
 	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
 	.dt_free_map		= pinctrl_utils_free_map,
 };
 
-static int pmic_gpio_get_functions_count(struct pinctrl_dev *pctldev)
-{
-	return ARRAY_SIZE(pmic_gpio_functions);
-}
-
-static const char *pmic_gpio_get_function_name(struct pinctrl_dev *pctldev,
-					       unsigned function)
-{
-	return pmic_gpio_functions[function];
-}
-
-static int pmic_gpio_get_function_groups(struct pinctrl_dev *pctldev,
-					 unsigned function,
-					 const char *const **groups,
-					 unsigned *const num_qgroups)
-{
-	*groups = pmic_gpio_groups;
-	*num_qgroups = pctldev->desc->npins;
-	return 0;
-}
-
-static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned function,
-				unsigned pin)
+static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
+				unsigned int selector)
 {
 	struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev);
 	struct pmic_gpio_pad *pad;
-	unsigned int val;
-	int ret;
+	struct group_desc *group;
+	unsigned int val, pin, func;
+	int ret, i;
 
 	if (function > PMIC_GPIO_FUNC_INDEX_DTEST4) {
 		pr_err("function: %d is not defined\n", function);
 		return -EINVAL;
 	}
 
-	pad = pctldev->desc->pins[pin].drv_data;
-	/*
-	 * Non-LV/MV subtypes only support 2 special functions,
-	 * offsetting the dtestx function values by 2
-	 */
-	if (!pad->lv_mv_type) {
-		if (function == PMIC_GPIO_FUNC_INDEX_FUNC3 ||
-				function == PMIC_GPIO_FUNC_INDEX_FUNC4) {
-			pr_err("LV/MV subtype doesn't have func3/func4\n");
-			return -EINVAL;
+	group = pinctrl_generic_get_group(pctldev, selector);
+	if (!group)
+		return -EINVAL;
+
+	/* For standard functions, iterate over all pins in the group */
+	for (i = 0; i < group->grp.npins; i++) {
+		pin = group->grp.pins[i];
+		pad = pctldev->desc->pins[pin].drv_data;
+
+		func = function;
+		/*
+		 * Non-LV/MV subtypes only support 2 special functions,
+		 * offsetting the dtestx function values by 2
+		 */
+		if (!pad->lv_mv_type) {
+			if (func == PMIC_GPIO_FUNC_INDEX_FUNC3 ||
+					func == PMIC_GPIO_FUNC_INDEX_FUNC4) {
+				dev_err(state->dev,
+					"pin%d: LV/MV subtype doesn't have func3/func4\n",
+					pin);
+				return -EINVAL;
+			}
+			if (func >= PMIC_GPIO_FUNC_INDEX_DTEST1)
+				func -= (PMIC_GPIO_FUNC_INDEX_DTEST1 -
+						PMIC_GPIO_FUNC_INDEX_FUNC3);
 		}
-		if (function >= PMIC_GPIO_FUNC_INDEX_DTEST1)
-			function -= (PMIC_GPIO_FUNC_INDEX_DTEST1 -
-					PMIC_GPIO_FUNC_INDEX_FUNC3);
-	}
 
-	pad->function = function;
+		pad->function = func;
 
-	if (pad->analog_pass)
-		val = PMIC_GPIO_MODE_ANALOG_PASS_THRU;
-	else if (pad->output_enabled && pad->input_enabled)
-		val = PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT;
-	else if (pad->output_enabled)
-		val = PMIC_GPIO_MODE_DIGITAL_OUTPUT;
-	else
-		val = PMIC_GPIO_MODE_DIGITAL_INPUT;
+		if (pad->analog_pass)
+			val = PMIC_GPIO_MODE_ANALOG_PASS_THRU;
+		else if (pad->output_enabled && pad->input_enabled)
+			val = PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT;
+		else if (pad->output_enabled)
+			val = PMIC_GPIO_MODE_DIGITAL_OUTPUT;
+		else
+			val = PMIC_GPIO_MODE_DIGITAL_INPUT;
 
-	if (pad->lv_mv_type) {
-		ret = pmic_gpio_write(state, pad,
-				PMIC_GPIO_REG_MODE_CTL, val);
-		if (ret < 0)
-			return ret;
+		if (pad->lv_mv_type) {
+			ret = pmic_gpio_write(state, pad,
+					PMIC_GPIO_REG_MODE_CTL, val);
+			if (ret < 0)
+				return ret;
 
-		val = pad->atest - 1;
-		ret = pmic_gpio_write(state, pad,
-				PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL, val);
-		if (ret < 0)
-			return ret;
+			val = pad->atest - 1;
+			ret = pmic_gpio_write(state, pad,
+					PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL, val);
+			if (ret < 0)
+				return ret;
+
+			val = pad->out_value
+				<< PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT;
+			val |= pad->function
+				& PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK;
+			ret = pmic_gpio_write(state, pad,
+				PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL, val);
+			if (ret < 0)
+				return ret;
+		} else {
+			val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT;
+			val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT;
+			val |= pad->out_value & PMIC_GPIO_REG_MODE_VALUE_SHIFT;
 
-		val = pad->out_value
-			<< PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT;
-		val |= pad->function
-			& PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK;
-		ret = pmic_gpio_write(state, pad,
-			PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL, val);
-		if (ret < 0)
-			return ret;
-	} else {
-		val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT;
-		val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT;
-		val |= pad->out_value & PMIC_GPIO_REG_MODE_VALUE_SHIFT;
+			ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_MODE_CTL, val);
+			if (ret < 0)
+				return ret;
+		}
 
-		ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_MODE_CTL, val);
+		val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
+
+		ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
 		if (ret < 0)
 			return ret;
 	}
 
-	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
-
-	return pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
+	return 0;
 }
 
 static const struct pinmux_ops pmic_gpio_pinmux_ops = {
-	.get_functions_count	= pmic_gpio_get_functions_count,
-	.get_function_name	= pmic_gpio_get_function_name,
-	.get_function_groups	= pmic_gpio_get_function_groups,
+	.get_functions_count	= pinmux_generic_get_function_count,
+	.get_function_name	= pinmux_generic_get_function_name,
+	.get_function_groups	= pinmux_generic_get_function_groups,
 	.set_mux		= pmic_gpio_set_mux,
 };
 
-static int pmic_gpio_config_get(struct pinctrl_dev *pctldev,
-				unsigned int pin, unsigned long *config)
+/**
+ * pmic_gpio_pinconf_pin_get() - Get configuration for a single pin
+ * @pctldev: pinctrl device
+ * @pin: Pin number
+ * @config: Configuration parameter to get
+ *
+ * Core function to read a single pin's configuration.
+ * Used by both per-pin and per-group config get operations.
+ */
+static int pmic_gpio_pinconf_pin_get(struct pinctrl_dev *pctldev,
+				     unsigned int pin, unsigned long *config)
 {
 	unsigned param = pinconf_to_config_param(*config);
 	struct pmic_gpio_pad *pad;
@@ -476,8 +462,48 @@ static int pmic_gpio_config_get(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
-				unsigned long *configs, unsigned nconfs)
+/**
+ * pmic_gpio_pinconf_group_get() - Get configuration for a pin group
+ * @pctldev: pinctrl device
+ * @selector: Group selector
+ * @config: Configuration parameter to get
+ *
+ * For multi-pin groups, we assume all pins have the same configuration,
+ * so we read the configuration from the first pin in the group.
+ */
+static int pmic_gpio_pinconf_group_get(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       unsigned long *config)
+{
+	const struct group_desc *group;
+	unsigned int pin;
+
+	group = pinctrl_generic_get_group(pctldev, selector);
+	if (!group)
+		return -EINVAL;
+
+	/*
+	 * For multi-pin groups, we assume all pins have the same configuration,
+	 * so we read the configuration from the first pin in the group.
+	 */
+	pin = group->grp.pins[0];
+
+	return pmic_gpio_pinconf_pin_get(pctldev, pin, config);
+}
+
+/**
+ * pmic_gpio_pinconf_pin_set() - Set configuration for a single pin
+ * @pctldev: pinctrl device
+ * @pin: Pin number
+ * @configs: Array of configuration parameters
+ * @nconfs: Number of configurations
+ *
+ * Core function to configure a single pin.
+ * Used by both per-pin and per-group config set operations.
+ */
+static int pmic_gpio_pinconf_pin_set(struct pinctrl_dev *pctldev,
+				     unsigned int pin,
+				     unsigned long *configs, unsigned int nconfs)
 {
 	struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev);
 	struct pmic_gpio_pad *pad;
@@ -651,12 +677,58 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
 
 	ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	return 0;
 }
 
-static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev,
-				      struct seq_file *s, unsigned pin)
+/**
+ * pmic_gpio_pinconf_group_set() - Set configuration for a pin group
+ * @pctldev: pinctrl device
+ * @selector: Group selector
+ * @configs: Array of configuration parameters
+ * @nconfs: Number of configurations
+ *
+ * Iterates over all pins in the group and applies config to each.
+ */
+static int pmic_gpio_pinconf_group_set(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       unsigned long *configs,
+				       unsigned int nconfs)
+{
+	const struct group_desc *group;
+	unsigned int pin;
+	int i, ret;
+
+	group = pinctrl_generic_get_group(pctldev, selector);
+	if (!group)
+		return -EINVAL;
+
+	/* Iterate over all pins in the group and apply config to each */
+	for (i = 0; i < group->grp.npins; i++) {
+		pin = group->grp.pins[i];
+
+		ret = pmic_gpio_pinconf_pin_set(pctldev, pin, configs, nconfs);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * pmic_gpio_pinconf_pin_dbg_show() - Show configuration for a single pin
+ * @pctldev: pinctrl device
+ * @s: seq_file for output
+ * @pin: Pin number
+ *
+ * Core function that dumps the configuration of a single GPIO pin.
+ * Used by pinconf-pins debugfs, pinconf-groups debugfs, and gpio debugfs.
+ */
+static void pmic_gpio_pinconf_pin_dbg_show(struct pinctrl_dev *pctldev,
+					   struct seq_file *s,
+					   unsigned int pin)
 {
 	struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev);
 	struct pmic_gpio_pad *pad;
@@ -716,11 +788,46 @@ static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev,
 	}
 }
 
+/**
+ * pmic_gpio_pinconf_group_dbg_show() - Show configuration for a pin group
+ * @pctldev: pinctrl device
+ * @s: seq_file for output
+ * @selector: Group selector
+ *
+ * Shows the configuration for all pins in a group.
+ * Used by the pinconf-groups debugfs interface.
+ */
+static void pmic_gpio_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
+					     struct seq_file *s,
+					     unsigned int selector)
+{
+	const struct group_desc *group;
+	unsigned int pin;
+	int i;
+
+	group = pinctrl_generic_get_group(pctldev, selector);
+	if (!group)
+		return;
+
+	/* Iterate over all pins in the group and show status for each */
+	for (i = 0; i < group->grp.npins; i++) {
+		pin = group->grp.pins[i];
+
+		if (i > 0)
+			seq_puts(s, "\n       ");
+
+		pmic_gpio_pinconf_pin_dbg_show(pctldev, s, pin);
+	}
+}
+
 static const struct pinconf_ops pmic_gpio_pinconf_ops = {
 	.is_generic			= true,
-	.pin_config_group_get		= pmic_gpio_config_get,
-	.pin_config_group_set		= pmic_gpio_config_set,
-	.pin_config_group_dbg_show	= pmic_gpio_config_dbg_show,
+	.pin_config_get			= pmic_gpio_pinconf_pin_get,
+	.pin_config_set			= pmic_gpio_pinconf_pin_set,
+	.pin_config_dbg_show		= pmic_gpio_pinconf_pin_dbg_show,
+	.pin_config_group_get		= pmic_gpio_pinconf_group_get,
+	.pin_config_group_set		= pmic_gpio_pinconf_group_set,
+	.pin_config_group_dbg_show	= pmic_gpio_pinconf_group_dbg_show,
 };
 
 static int pmic_gpio_get_direction(struct gpio_chip *chip, unsigned pin)
@@ -745,7 +852,7 @@ static int pmic_gpio_direction_input(struct gpio_chip *chip, unsigned pin)
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
 
-	return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
+	return pmic_gpio_pinconf_pin_set(state->ctrl, pin, &config, 1);
 }
 
 static int pmic_gpio_direction_output(struct gpio_chip *chip,
@@ -756,7 +863,7 @@ static int pmic_gpio_direction_output(struct gpio_chip *chip,
 
 	config = pinconf_to_config_packed(PIN_CONFIG_LEVEL, val);
 
-	return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
+	return pmic_gpio_pinconf_pin_set(state->ctrl, pin, &config, 1);
 }
 
 static int pmic_gpio_get(struct gpio_chip *chip, unsigned pin)
@@ -788,7 +895,7 @@ static int pmic_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
 
 	config = pinconf_to_config_packed(PIN_CONFIG_LEVEL, value);
 
-	return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
+	return pmic_gpio_pinconf_pin_set(state->ctrl, pin, &config, 1);
 }
 
 static int pmic_gpio_of_xlate(struct gpio_chip *chip,
@@ -810,7 +917,7 @@ static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned i;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		pmic_gpio_config_dbg_show(state->ctrl, s, i);
+		pmic_gpio_pinconf_pin_dbg_show(state->ctrl, s, i);
 		seq_puts(s, "\n");
 	}
 }
@@ -1129,11 +1236,11 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	pctrldesc->custom_conf_items = pmic_conf_items;
 #endif
 
-	for (i = 0; i < npins; i++, pindesc++) {
+	for (i = 0; i < npins; i++) {
 		pad = &pads[i];
-		pindesc->drv_data = pad;
-		pindesc->number = i;
-		pindesc->name = pmic_gpio_groups[i];
+		pindesc[i].drv_data = pad;
+		pindesc[i].number = i;
+		pindesc[i].name = pmic_gpio_groups[i];
 
 		pad->base = reg + i * PMIC_GPIO_ADDRESS_RANGE;
 
@@ -1150,9 +1257,33 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	state->chip.of_gpio_n_cells = 2;
 	state->chip.can_sleep = false;
 
-	state->ctrl = devm_pinctrl_register(dev, pctrldesc, state);
-	if (IS_ERR(state->ctrl))
-		return PTR_ERR(state->ctrl);
+	ret = devm_pinctrl_register_and_init(dev, pctrldesc, state, &state->ctrl);
+	if (ret)
+		return ret;
+
+	/* Register pin groups - each GPIO is a group for standard functions */
+	for (i = 0; i < npins; i++) {
+		ret = pinctrl_generic_add_group(state->ctrl,
+						pmic_gpio_groups[i],
+						&pindesc[i].number, 1, NULL);
+		if (ret < 0) {
+			dev_err(dev, "failed to register group %s\n",
+				pmic_gpio_groups[i]);
+			return ret;
+		}
+	}
+
+	/* Register standard functions - all GPIOs support these */
+	for (i = 0; i < ARRAY_SIZE(pmic_gpio_functions); i++) {
+		ret = pinmux_generic_add_function(state->ctrl,
+						  pmic_gpio_functions[i],
+						  pmic_gpio_groups, npins, NULL);
+		if (ret < 0) {
+			dev_err(dev, "failed to register function %s\n",
+				pmic_gpio_functions[i]);
+			return ret;
+		}
+	}
 
 	parent_node = of_irq_find_parent(state->dev->of_node);
 	if (!parent_node)
@@ -1174,6 +1305,10 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	girq->child_offset_to_irq = pmic_gpio_child_offset_to_irq;
 	girq->child_irq_domain_ops.translate = pmic_gpio_domain_translate;
 
+	ret = pinctrl_enable(state->ctrl);
+	if (ret)
+		return ret;
+
 	ret = gpiochip_add_data(&state->chip, state);
 	if (ret) {
 		dev_err(state->dev, "can't add gpio chip\n");

-- 
2.43.0

[PATCH v2 4/4] pinctrl: qcom: spmi-gpio: Add level-shifter function support

[Fenglin Wu]

PMH0101 introduces bidirectional level shifter (BIDIR_LVL_SHIFTER)
modules that provide voltage translation between 1.2 V and 1.8 V power
domains for open-drain signals, such as connecting a camera sensor with
1.8 V I2C IO to a SoC I2C bus operating at 1.2 V IO.

Each BIDIR_LVL_SHIFTER shares its two physical pins with a corresponding
pair of GPIO modules. When used, the associated GPIOs need to keep
disabled, with no GPIO function or configuration enabled. The
BIDIR_LVL_SHIFTER then operates as a bidirectional open-drain voltage
translator, using fixed 1.2 V and 1.8 V reference voltages on each side
and relying on external board-level pull-up resistors for open-drain
drive control.

From a system perspective, a BIDIR_LVL_SHIFTER may be used on a serial
bus shared by multiple clients managed by different subsystems. As a
result, its enable control is shared and centrally managed by AOP,
with each subsystem voting for the enable state via an RPMh “XOB”
resource.

Add a new "level-shifter" function to the SPMI GPIO driver to support
BIDIR_LVL_SHIFTER operation considering that BIDIR_LVL_SHIFTER shares
physical pins with GPIOs, it must be mutually exclusive with all
existing functions and configurations. Additionally, limit the
"level-shifter" function to specific GPIO pairs that share physical
pins with the BIDIR_LVL_SHIFTER module by creating pingroups and
linking them to the "level-shifter" function.

With this change, the BIDIR_LVL_SHIFTER could be controlled with
following dtsi nodes:

    pmh0101-ls1-en {
    	groups = "gpio11, gpio12";
    	function = "level-shifter";
    	qcom,1p2v-1p8v-ls-en = <1>;
    };

    pmh0101-ls1-dis {
    	groups = "gpio11, gpio12";
    	function = "level-shifter";
    	qcom,1p2v-1p8v-ls-en = <0>;
    };

Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 441 +++++++++++++++++++++++++------
 1 file changed, 355 insertions(+), 86 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 268cfae706a8..741795ae261e 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -23,6 +23,9 @@
 
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
 
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
 #include "../core.h"
 #include "../pinmux.h"
 #include "../pinctrl-utils.h"
@@ -118,12 +121,16 @@
 /* PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL */
 #define PMIC_GPIO_LV_MV_ANA_MUX_SEL_MASK		0x3
 
+/* Level shifter register offset */
+#define REG_LS_ENABLE_OFFSET			4
+
 /* Qualcomm specific pin configurations */
 #define PMIC_GPIO_CONF_PULL_UP			(PIN_CONFIG_END + 1)
 #define PMIC_GPIO_CONF_STRENGTH			(PIN_CONFIG_END + 2)
 #define PMIC_GPIO_CONF_ATEST			(PIN_CONFIG_END + 3)
 #define PMIC_GPIO_CONF_ANALOG_PASS		(PIN_CONFIG_END + 4)
 #define PMIC_GPIO_CONF_DTEST_BUFFER		(PIN_CONFIG_END + 5)
+#define PMIC_GPIO_CONF_LS_ENABLE		(PIN_CONFIG_END + 6)
 
 /* The index of each function in pmic_gpio_functions[] array */
 enum pmic_gpio_func_index {
@@ -137,6 +144,7 @@ enum pmic_gpio_func_index {
 	PMIC_GPIO_FUNC_INDEX_DTEST2,
 	PMIC_GPIO_FUNC_INDEX_DTEST3,
 	PMIC_GPIO_FUNC_INDEX_DTEST4,
+	PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER,
 };
 
 /**
@@ -183,25 +191,86 @@ struct pmic_gpio_state {
 	struct regmap	*map;
 	struct pinctrl_dev *ctrl;
 	struct gpio_chip chip;
+	struct device	*rpmh_dev;
+	const char	*pmic_id;
 	u8 usid;
 	u8 pid_base;
 };
 
+/**
+ * struct ls_config - Level shifter configuration
+ * @name: Pinctrl group name (e.g., "gpio11, gpio12")
+ * @rpmh_prefix: RPMh resource name prefix (e.g., "LS1")
+ * @pins: Array of pin numbers for this level shifter pair
+ */
+struct ls_config {
+	const char *name;
+	const char *rpmh_prefix;
+	const unsigned int *pins;
+};
+
+/**
+ * struct ls_pingroup_data - Level shifter pin group data
+ * @config: Pointer to level shifter configuration
+ * @level_shifter_addr: RPMh address for level shifter control
+ * @ls_enabled: Shadowed enable state, updated on each successful RPMh write
+ */
+struct ls_pingroup_data {
+	struct ls_config *config;
+	u32 level_shifter_addr;
+	bool ls_enabled;
+};
+
+/**
+ * struct pmic_gpio_hw_data - Hardware-specific data
+ * @npins: Number of GPIO pins
+ * @ls_config: Array of level shifter configurations
+ * @num_ls: Number of level shifters
+ */
+struct pmic_gpio_hw_data {
+	u32 npins;
+	struct ls_config *ls_config;
+	u32 num_ls;
+};
+
+/* Macro to create hardware data inline */
+#define PMIC_GPIO_HW_DATA(n, ls, num) \
+	(&(const struct pmic_gpio_hw_data) { \
+		.npins = n, \
+		.ls_config = ls, \
+		.num_ls = num, \
+	})
+
+/* Level shifter pin groups for PMH0101 */
+static const unsigned int ls1_pins[] = { 10, 11 }; /* GPIO11, GPIO12 */
+static const unsigned int ls2_pins[] = { 12, 13 }; /* GPIO13, GPIO14 */
+static const unsigned int ls3_pins[] = { 14, 15 }; /* GPIO15, GPIO16 */
+static const unsigned int ls4_pins[] = { 16, 17 }; /* GPIO17, GPIO18 */
+
+static struct ls_config pmh0101_ls_configs[] = {
+	{ "gpio11, gpio12", "LS1", ls1_pins },
+	{ "gpio13, gpio14", "LS2", ls2_pins },
+	{ "gpio15, gpio16", "LS3", ls3_pins },
+	{ "gpio17, gpio18", "LS4", ls4_pins },
+};
+
 static const struct pinconf_generic_params pmic_gpio_bindings[] = {
 	{"qcom,pull-up-strength",	PMIC_GPIO_CONF_PULL_UP,		0},
 	{"qcom,drive-strength",		PMIC_GPIO_CONF_STRENGTH,	0},
 	{"qcom,atest",			PMIC_GPIO_CONF_ATEST,		0},
 	{"qcom,analog-pass",		PMIC_GPIO_CONF_ANALOG_PASS,	0},
-	{"qcom,dtest-buffer",           PMIC_GPIO_CONF_DTEST_BUFFER,    0},
+	{"qcom,dtest-buffer",		PMIC_GPIO_CONF_DTEST_BUFFER,	0},
+	{"qcom,1p2v-1p8v-ls-en",	PMIC_GPIO_CONF_LS_ENABLE,	0},
 };
 
 #ifdef CONFIG_DEBUG_FS
 static const struct pin_config_item pmic_conf_items[ARRAY_SIZE(pmic_gpio_bindings)] = {
-	PCONFDUMP(PMIC_GPIO_CONF_PULL_UP,  "pull up strength", NULL, true),
-	PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true),
-	PCONFDUMP(PMIC_GPIO_CONF_ATEST, "atest", NULL, true),
-	PCONFDUMP(PMIC_GPIO_CONF_ANALOG_PASS, "analog-pass", NULL, true),
-	PCONFDUMP(PMIC_GPIO_CONF_DTEST_BUFFER, "dtest-buffer", NULL, true),
+	PCONFDUMP(PMIC_GPIO_CONF_PULL_UP,      "pull up strength", NULL, true),
+	PCONFDUMP(PMIC_GPIO_CONF_STRENGTH,     "drive-strength",   NULL, true),
+	PCONFDUMP(PMIC_GPIO_CONF_ATEST,        "atest",            NULL, true),
+	PCONFDUMP(PMIC_GPIO_CONF_ANALOG_PASS,  "analog-pass",      NULL, true),
+	PCONFDUMP(PMIC_GPIO_CONF_DTEST_BUFFER, "dtest-buffer",     NULL, true),
+	PCONFDUMP(PMIC_GPIO_CONF_LS_ENABLE,    "ls-enable",        NULL, true),
 };
 #endif
 
@@ -214,16 +283,16 @@ static const char *const pmic_gpio_groups[] = {
 };
 
 static const char *const pmic_gpio_functions[] = {
-	[PMIC_GPIO_FUNC_INDEX_NORMAL]	= PMIC_GPIO_FUNC_NORMAL,
-	[PMIC_GPIO_FUNC_INDEX_PAIRED]	= PMIC_GPIO_FUNC_PAIRED,
-	[PMIC_GPIO_FUNC_INDEX_FUNC1]	= PMIC_GPIO_FUNC_FUNC1,
-	[PMIC_GPIO_FUNC_INDEX_FUNC2]	= PMIC_GPIO_FUNC_FUNC2,
-	[PMIC_GPIO_FUNC_INDEX_FUNC3]	= PMIC_GPIO_FUNC_FUNC3,
-	[PMIC_GPIO_FUNC_INDEX_FUNC4]	= PMIC_GPIO_FUNC_FUNC4,
-	[PMIC_GPIO_FUNC_INDEX_DTEST1]	= PMIC_GPIO_FUNC_DTEST1,
-	[PMIC_GPIO_FUNC_INDEX_DTEST2]	= PMIC_GPIO_FUNC_DTEST2,
-	[PMIC_GPIO_FUNC_INDEX_DTEST3]	= PMIC_GPIO_FUNC_DTEST3,
-	[PMIC_GPIO_FUNC_INDEX_DTEST4]	= PMIC_GPIO_FUNC_DTEST4,
+	[PMIC_GPIO_FUNC_INDEX_NORMAL]		= PMIC_GPIO_FUNC_NORMAL,
+	[PMIC_GPIO_FUNC_INDEX_PAIRED]		= PMIC_GPIO_FUNC_PAIRED,
+	[PMIC_GPIO_FUNC_INDEX_FUNC1]		= PMIC_GPIO_FUNC_FUNC1,
+	[PMIC_GPIO_FUNC_INDEX_FUNC2]		= PMIC_GPIO_FUNC_FUNC2,
+	[PMIC_GPIO_FUNC_INDEX_FUNC3]		= PMIC_GPIO_FUNC_FUNC3,
+	[PMIC_GPIO_FUNC_INDEX_FUNC4]		= PMIC_GPIO_FUNC_FUNC4,
+	[PMIC_GPIO_FUNC_INDEX_DTEST1]		= PMIC_GPIO_FUNC_DTEST1,
+	[PMIC_GPIO_FUNC_INDEX_DTEST2]		= PMIC_GPIO_FUNC_DTEST2,
+	[PMIC_GPIO_FUNC_INDEX_DTEST3]		= PMIC_GPIO_FUNC_DTEST3,
+	[PMIC_GPIO_FUNC_INDEX_DTEST4]		= PMIC_GPIO_FUNC_DTEST4,
 };
 
 static int pmic_gpio_read(struct pmic_gpio_state *state,
@@ -262,6 +331,41 @@ static const struct pinctrl_ops pmic_gpio_pinctrl_ops = {
 	.dt_free_map		= pinctrl_utils_free_map,
 };
 
+/**
+ * pmic_gpio_set_ls_rpmh() - Send RPMh XOB vote for a level shifter
+ * @state: PMIC GPIO state
+ * @ls_data: Level shifter pin group data
+ * @enable: true to enable, false to disable
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int pmic_gpio_set_ls_rpmh(struct pmic_gpio_state *state,
+				  struct ls_pingroup_data *ls_data,
+				  bool enable)
+{
+	struct tcs_cmd cmd = { };
+	int ret;
+
+	if (!ls_data->level_shifter_addr) {
+		dev_err(state->dev, "Level shifter address not configured\n");
+		return -EINVAL;
+	}
+
+	if (!state->rpmh_dev) {
+		dev_err(state->dev, "RPMh device not available\n");
+		return -ENODEV;
+	}
+
+	cmd.addr = ls_data->level_shifter_addr + REG_LS_ENABLE_OFFSET;
+	cmd.data = enable ? 1 : 0;
+
+	ret = rpmh_write_ctrlr(state->rpmh_dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1);
+	if (!ret)
+		ls_data->ls_enabled = enable;
+
+	return ret;
+}
+
 static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
 				unsigned int selector)
 {
@@ -271,7 +375,7 @@ static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
 	unsigned int val, pin, func;
 	int ret, i;
 
-	if (function > PMIC_GPIO_FUNC_INDEX_DTEST4) {
+	if (function > PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER) {
 		pr_err("function: %d is not defined\n", function);
 		return -EINVAL;
 	}
@@ -280,6 +384,28 @@ static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
 	if (!group)
 		return -EINVAL;
 
+	if (function == PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER) {
+		if (!group->data)
+			return -EINVAL;
+
+		/*
+		 * Disable both GPIO pads in the pair. The RPMh XOB vote is
+		 * sent separately via the qcom,1p2v-1p8v-ls-en pinconf parameter.
+		 */
+		for (i = 0; i < group->grp.npins; i++) {
+			pin = group->grp.pins[i];
+			pad = pctldev->desc->pins[pin].drv_data;
+
+			ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, 0);
+			if (ret < 0)
+				return ret;
+
+			pad->is_enabled = false;
+			pad->function = PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER;
+		}
+		return 0;
+	}
+
 	/* For standard functions, iterate over all pins in the group */
 	for (i = 0; i < group->grp.npins; i++) {
 		pin = group->grp.pins[i];
@@ -513,7 +639,8 @@ static int pmic_gpio_pinconf_pin_set(struct pinctrl_dev *pctldev,
 
 	pad = pctldev->desc->pins[pin].drv_data;
 
-	pad->is_enabled = true;
+	if (pad->function != PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER)
+		pad->is_enabled = true;
 	for (i = 0; i < nconfs; i++) {
 		param = pinconf_to_config_param(configs[i]);
 		arg = pinconf_to_config_argument(configs[i]);
@@ -597,6 +724,9 @@ static int pmic_gpio_pinconf_pin_set(struct pinctrl_dev *pctldev,
 				return -EINVAL;
 			pad->dtest_buffer = arg;
 			break;
+		case PMIC_GPIO_CONF_LS_ENABLE:
+			/* Group-level only; handled in pmic_gpio_pinconf_group_set */
+			break;
 		default:
 			return -EINVAL;
 		}
@@ -690,13 +820,14 @@ static int pmic_gpio_pinconf_pin_set(struct pinctrl_dev *pctldev,
  * @configs: Array of configuration parameters
  * @nconfs: Number of configurations
  *
- * Iterates over all pins in the group and applies config to each.
+ * Handles group-level parameters (e.g., LS enable) before iterating per-pin.
  */
 static int pmic_gpio_pinconf_group_set(struct pinctrl_dev *pctldev,
 				       unsigned int selector,
 				       unsigned long *configs,
 				       unsigned int nconfs)
 {
+	struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev);
 	const struct group_desc *group;
 	unsigned int pin;
 	int i, ret;
@@ -705,7 +836,25 @@ static int pmic_gpio_pinconf_group_set(struct pinctrl_dev *pctldev,
 	if (!group)
 		return -EINVAL;
 
-	/* Iterate over all pins in the group and apply config to each */
+	/* Handle group-level LS_ENABLE before iterating per-pin configs */
+	for (i = 0; i < nconfs; i++) {
+		if (pinconf_to_config_param(configs[i]) != PMIC_GPIO_CONF_LS_ENABLE)
+			continue;
+
+		if (!group->data) {
+			dev_err(state->dev,
+				"qcom,1p2v-1p8v-ls-en is only valid for level-shifter groups\n");
+			return -EINVAL;
+		}
+
+		ret = pmic_gpio_set_ls_rpmh(state,
+					    (struct ls_pingroup_data *)group->data,
+					    !!pinconf_to_config_argument(configs[i]));
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Apply per-pin configs to each pin in the group */
 	for (i = 0; i < group->grp.npins; i++) {
 		pin = group->grp.pins[i];
 
@@ -794,8 +943,8 @@ static void pmic_gpio_pinconf_pin_dbg_show(struct pinctrl_dev *pctldev,
  * @s: seq_file for output
  * @selector: Group selector
  *
- * Shows the configuration for all pins in a group.
- * Used by the pinconf-groups debugfs interface.
+ * Shows the configuration for all pins in a group. For level-shifter groups,
+ * also prints the RPMh address as a preamble.
  */
 static void pmic_gpio_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 					     struct seq_file *s,
@@ -809,6 +958,14 @@ static void pmic_gpio_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 	if (!group)
 		return;
 
+	/* For level-shifter groups, print the enable status as preamble */
+	if (group->data) {
+		const struct ls_pingroup_data *ls_data = group->data;
+
+		seq_printf(s, ", level-shifter (%s)\n       ",
+			   ls_data->ls_enabled ? "enabled" : "disabled");
+	}
+
 	/* Iterate over all pins in the group and show status for each */
 	for (i = 0; i < group->grp.npins; i++) {
 		pin = group->grp.pins[i];
@@ -1177,6 +1334,102 @@ static const struct irq_chip spmi_gpio_irq_chip = {
 	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
+/**
+ * pmic_gpio_register_level_shifters() - Register level-shifter groups and function
+ * @state: PMIC GPIO state
+ * @hw_data: Hardware-specific data containing level-shifter configurations
+ *
+ * This function registers level-shifter support by:
+ * 1. Getting RPMh device reference and PMIC ID from device tree
+ * 2. Registering each level-shifter pair as a multi-pin group
+ * 3. Getting RPMh addresses from cmd_db for each level-shifter
+ * 4. Registering the level-shifter function once with all valid groups
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int pmic_gpio_register_level_shifters(struct pmic_gpio_state *state,
+					     const struct pmic_gpio_hw_data *hw_data)
+{
+	struct device *dev = state->dev;
+	const char **ls_group_names;
+	int ret, i;
+
+	/* Get RPMh device reference for level shifter control */
+	state->rpmh_dev = rpmh_get_ctrlr_dev(dev);
+	if (IS_ERR(state->rpmh_dev))
+		return dev_err_probe(dev, PTR_ERR(state->rpmh_dev),
+				     "Level shifter needs rpmh device\n");
+
+	/* Get PMIC ID from device tree for RPMh resource name composition */
+	ret = of_property_read_string(dev->of_node, "qcom,pmic-id",
+				       &state->pmic_id);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "get qcom,pmic-id failed for rpmh resource\n");
+
+	/* Allocate array to hold all level-shifter group names */
+	ls_group_names = devm_kcalloc(dev, hw_data->num_ls,
+				      sizeof(*ls_group_names), GFP_KERNEL);
+	if (!ls_group_names)
+		return -ENOMEM;
+
+	/* Register each level-shifter pair as a multi-pin group */
+	for (i = 0; i < hw_data->num_ls; i++) {
+		struct ls_config *ls = &hw_data->ls_config[i];
+		struct ls_pingroup_data *ls_group_data;
+		char rpmh_resource_name[32];
+
+		/* Allocate ls_pingroup_data for this level shifter */
+		ls_group_data = devm_kzalloc(dev, sizeof(*ls_group_data),
+					     GFP_KERNEL);
+		if (!ls_group_data)
+			return -ENOMEM;
+
+		ls_group_data->config = ls;
+
+		/* Compose RPMh resource name and get address from cmd_db */
+		if (state->pmic_id && ls->rpmh_prefix) {
+			int ret_ready = cmd_db_ready();
+
+			if (ret_ready)
+				return dev_err_probe(dev, ret_ready,
+						     "Command DB not available\n");
+
+			snprintf(rpmh_resource_name,
+				 sizeof(rpmh_resource_name),
+				 "%s%s", ls->rpmh_prefix, state->pmic_id);
+			ls_group_data->level_shifter_addr =
+				cmd_db_read_addr(rpmh_resource_name);
+			if (!ls_group_data->level_shifter_addr)
+				return dev_err_probe(dev, -ENODEV,
+						     "RPMh resource %s not found in cmd_db\n",
+						     rpmh_resource_name);
+		}
+
+		/* Store group name for function registration */
+		ls_group_names[i] = ls->name;
+
+		/* Register the multi-pin group using the level-shifter name */
+		ret = pinctrl_generic_add_group(state->ctrl, ls->name,
+						ls->pins, 2, ls_group_data);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to register level-shifter group %s\n",
+					     ls->name);
+	}
+
+	/* Register level-shifter function once with all valid groups */
+	ret = pinmux_generic_add_function(state->ctrl,
+					  PMIC_GPIO_FUNC_LEVEL_SHIFTER,
+					  ls_group_names, hw_data->num_ls,
+					  NULL);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to register level-shifter function\n");
+
+	return 0;
+}
+
 static int pmic_gpio_probe(struct platform_device *pdev)
 {
 	struct irq_domain *parent_domain;
@@ -1188,6 +1441,7 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	struct pmic_gpio_state *state;
 	struct gpio_irq_chip *girq;
 	const struct spmi_device *parent_spmi_dev;
+	const struct pmic_gpio_hw_data *hw_data;
 	int ret, npins, i;
 	u32 reg;
 
@@ -1197,7 +1451,13 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	npins = (uintptr_t) device_get_match_data(&pdev->dev);
+	hw_data = device_get_match_data(&pdev->dev);
+	if (!hw_data) {
+		dev_err(dev, "no match data found\n");
+		return -EINVAL;
+	}
+
+	npins = hw_data->npins;
 
 	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
 	if (!state)
@@ -1285,6 +1545,15 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* Register level-shifter groups and function if hardware and DT both opt in */
+	if (hw_data->ls_config && hw_data->num_ls &&
+	    of_property_present(dev->of_node, "qcom,rpmh") &&
+	    of_property_present(dev->of_node, "qcom,pmic-id")) {
+		ret = pmic_gpio_register_level_shifters(state, hw_data);
+		if (ret < 0)
+			return ret;
+	}
+
 	parent_node = of_irq_find_parent(state->dev->of_node);
 	if (!parent_node)
 		return -ENXIO;
@@ -1349,80 +1618,80 @@ static void pmic_gpio_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id pmic_gpio_of_match[] = {
-	{ .compatible = "qcom,pm2250-gpio", .data = (void *) 10 },
+	{ .compatible = "qcom,pm2250-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
 	/* pm660 has 13 GPIOs with holes on 1, 5, 6, 7, 8 and 10 */
-	{ .compatible = "qcom,pm660-gpio", .data = (void *) 13 },
+	{ .compatible = "qcom,pm660-gpio", .data = PMIC_GPIO_HW_DATA(13, NULL, 0) },
 	/* pm660l has 12 GPIOs with holes on 1, 2, 10, 11 and 12 */
-	{ .compatible = "qcom,pm660l-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm6125-gpio", .data = (void *) 9 },
-	{ .compatible = "qcom,pm6150-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pm6150l-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm6350-gpio", .data = (void *) 9 },
-	{ .compatible = "qcom,pm6450-gpio", .data = (void *) 9 },
-	{ .compatible = "qcom,pm7250b-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm7325-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pm7550-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm7550ba-gpio", .data = (void *) 8},
-	{ .compatible = "qcom,pm8005-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pm8010-gpio", .data = (void *) 2 },
-	{ .compatible = "qcom,pm8019-gpio", .data = (void *) 6 },
+	{ .compatible = "qcom,pm660l-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm6125-gpio", .data = PMIC_GPIO_HW_DATA(9, NULL, 0) },
+	{ .compatible = "qcom,pm6150-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pm6150l-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm6350-gpio", .data = PMIC_GPIO_HW_DATA(9, NULL, 0) },
+	{ .compatible = "qcom,pm6450-gpio", .data = PMIC_GPIO_HW_DATA(9, NULL, 0) },
+	{ .compatible = "qcom,pm7250b-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm7325-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pm7550-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm7550ba-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pm8005-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
+	{ .compatible = "qcom,pm8010-gpio", .data = PMIC_GPIO_HW_DATA(2, NULL, 0) },
+	{ .compatible = "qcom,pm8019-gpio", .data = PMIC_GPIO_HW_DATA(6, NULL, 0) },
 	/* pm8150 has 10 GPIOs with holes on 2, 5, 7 and 8 */
-	{ .compatible = "qcom,pm8150-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pmc8180-gpio", .data = (void *) 10 },
+	{ .compatible = "qcom,pm8150-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pmc8180-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
 	/* pm8150b has 12 GPIOs with holes on 3, r and 7 */
-	{ .compatible = "qcom,pm8150b-gpio", .data = (void *) 12 },
+	{ .compatible = "qcom,pm8150b-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
 	/* pm8150l has 12 GPIOs with holes on 7 */
-	{ .compatible = "qcom,pm8150l-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pmc8180c-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm8226-gpio", .data = (void *) 8 },
-	{ .compatible = "qcom,pm8350-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pm8350b-gpio", .data = (void *) 8 },
-	{ .compatible = "qcom,pm8350c-gpio", .data = (void *) 9 },
-	{ .compatible = "qcom,pm8450-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pm8550-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm8550b-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pm8550ve-gpio", .data = (void *) 8 },
-	{ .compatible = "qcom,pm8550vs-gpio", .data = (void *) 6 },
-	{ .compatible = "qcom,pm8916-gpio", .data = (void *) 4 },
+	{ .compatible = "qcom,pm8150l-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pmc8180c-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm8226-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pm8350-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pm8350b-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pm8350c-gpio", .data = PMIC_GPIO_HW_DATA(9, NULL, 0) },
+	{ .compatible = "qcom,pm8450-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
+	{ .compatible = "qcom,pm8550-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm8550b-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pm8550ve-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pm8550vs-gpio", .data = PMIC_GPIO_HW_DATA(6, NULL, 0) },
+	{ .compatible = "qcom,pm8916-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
 	/* pm8937 has 8 GPIOs with holes on 3, 4 and 6 */
-	{ .compatible = "qcom,pm8937-gpio", .data = (void *) 8 },
-	{ .compatible = "qcom,pm8941-gpio", .data = (void *) 36 },
+	{ .compatible = "qcom,pm8937-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pm8941-gpio", .data = PMIC_GPIO_HW_DATA(36, NULL, 0) },
 	/* pm8950 has 8 GPIOs with holes on 3 */
-	{ .compatible = "qcom,pm8950-gpio", .data = (void *) 8 },
+	{ .compatible = "qcom,pm8950-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
 	/* pm8953 has 8 GPIOs with holes on 3 and 6 */
-	{ .compatible = "qcom,pm8953-gpio", .data = (void *) 8 },
-	{ .compatible = "qcom,pm8994-gpio", .data = (void *) 22 },
-	{ .compatible = "qcom,pm8998-gpio", .data = (void *) 26 },
-	{ .compatible = "qcom,pma8084-gpio", .data = (void *) 22 },
-	{ .compatible = "qcom,pmc8380-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pmcx0102-gpio", .data = (void *)14 },
-	{ .compatible = "qcom,pmd8028-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pmh0101-gpio", .data = (void *)18 },
-	{ .compatible = "qcom,pmh0104-gpio", .data = (void *)8 },
-	{ .compatible = "qcom,pmh0110-gpio", .data = (void *)14 },
-	{ .compatible = "qcom,pmi632-gpio", .data = (void *) 8 },
-	{ .compatible = "qcom,pmi8950-gpio", .data = (void *) 2 },
-	{ .compatible = "qcom,pmi8994-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pmi8998-gpio", .data = (void *) 14 },
-	{ .compatible = "qcom,pmih0108-gpio", .data = (void *) 18 },
-	{ .compatible = "qcom,pmiv0104-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pmk8350-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pmk8550-gpio", .data = (void *) 6 },
-	{ .compatible = "qcom,pmk8850-gpio", .data = (void *)8 },
-	{ .compatible = "qcom,pmm8155au-gpio", .data = (void *) 10 },
-	{ .compatible = "qcom,pmm8654au-gpio", .data = (void *) 12 },
+	{ .compatible = "qcom,pm8953-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pm8994-gpio", .data = PMIC_GPIO_HW_DATA(22, NULL, 0) },
+	{ .compatible = "qcom,pm8998-gpio", .data = PMIC_GPIO_HW_DATA(26, NULL, 0) },
+	{ .compatible = "qcom,pma8084-gpio", .data = PMIC_GPIO_HW_DATA(22, NULL, 0) },
+	{ .compatible = "qcom,pmc8380-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pmcx0102-gpio", .data = PMIC_GPIO_HW_DATA(14, NULL, 0) },
+	{ .compatible = "qcom,pmd8028-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
+	{ .compatible = "qcom,pmh0101-gpio", .data = PMIC_GPIO_HW_DATA(18, pmh0101_ls_configs, 4) },
+	{ .compatible = "qcom,pmh0104-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pmh0110-gpio", .data = PMIC_GPIO_HW_DATA(14, NULL, 0) },
+	{ .compatible = "qcom,pmi632-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pmi8950-gpio", .data = PMIC_GPIO_HW_DATA(2, NULL, 0) },
+	{ .compatible = "qcom,pmi8994-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pmi8998-gpio", .data = PMIC_GPIO_HW_DATA(14, NULL, 0) },
+	{ .compatible = "qcom,pmih0108-gpio", .data = PMIC_GPIO_HW_DATA(18, NULL, 0) },
+	{ .compatible = "qcom,pmiv0104-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pmk8350-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
+	{ .compatible = "qcom,pmk8550-gpio", .data = PMIC_GPIO_HW_DATA(6, NULL, 0) },
+	{ .compatible = "qcom,pmk8850-gpio", .data = PMIC_GPIO_HW_DATA(8, NULL, 0) },
+	{ .compatible = "qcom,pmm8155au-gpio", .data = PMIC_GPIO_HW_DATA(10, NULL, 0) },
+	{ .compatible = "qcom,pmm8654au-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
 	/* pmp8074 has 12 GPIOs with holes on 1 and 12 */
-	{ .compatible = "qcom,pmp8074-gpio", .data = (void *) 12 },
-	{ .compatible = "qcom,pmr735a-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pmr735b-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pmr735d-gpio", .data = (void *) 2 },
+	{ .compatible = "qcom,pmp8074-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
+	{ .compatible = "qcom,pmr735a-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
+	{ .compatible = "qcom,pmr735b-gpio", .data = PMIC_GPIO_HW_DATA(4, NULL, 0) },
+	{ .compatible = "qcom,pmr735d-gpio", .data = PMIC_GPIO_HW_DATA(2, NULL, 0) },
 	/* pms405 has 12 GPIOs with holes on 1, 9, and 10 */
-	{ .compatible = "qcom,pms405-gpio", .data = (void *) 12 },
+	{ .compatible = "qcom,pms405-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
 	/* pmx55 has 11 GPIOs with holes on 3, 7, 10, 11 */
-	{ .compatible = "qcom,pmx55-gpio", .data = (void *) 11 },
-	{ .compatible = "qcom,pmx65-gpio", .data = (void *) 16 },
-	{ .compatible = "qcom,pmx75-gpio", .data = (void *) 16 },
-	{ .compatible = "qcom,pmxr2230-gpio", .data = (void *) 12 },
+	{ .compatible = "qcom,pmx55-gpio", .data = PMIC_GPIO_HW_DATA(11, NULL, 0) },
+	{ .compatible = "qcom,pmx65-gpio", .data = PMIC_GPIO_HW_DATA(16, NULL, 0) },
+	{ .compatible = "qcom,pmx75-gpio", .data = PMIC_GPIO_HW_DATA(16, NULL, 0) },
+	{ .compatible = "qcom,pmxr2230-gpio", .data = PMIC_GPIO_HW_DATA(12, NULL, 0) },
 	{ },
 };
 

-- 
2.43.0

Re: [PATCH v2 4/4] pinctrl: qcom: spmi-gpio: Add level-shifter function support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Writing the software-defined `LEVEL_SHIFTER` function index to hardware registers causes register corruption.
- [High] Out-of-bounds array access when displaying pinconf debugfs for a level-shifter pin.
- [High] Use-After-Free of level shifter resources during driver unbind due to reversed devm initialization order.
- [High] Failure to enforce mutual exclusivity between the bidirectional level shifter and physical GPIO functions.
- [Medium] Missing GET support for the newly added `qcom,1p2v-1p8v-ls-en` pin configuration.
--

commit febef2022fa1d171b552bf56874bdc04c2cb463b
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

pinctrl: qcom: spmi-gpio: Add level-shifter function support

This commit introduces bidirectional level shifter support for open-drain
signals. It adds a level-shifter function to the SPMI GPIO driver for specific
GPIO pairs managed via RPMh.

> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index 268cfae706a82..741795ae261ef 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -137,6 +144,7 @@ enum pmic_gpio_func_index {
>  	PMIC_GPIO_FUNC_INDEX_DTEST2,
>  	PMIC_GPIO_FUNC_INDEX_DTEST3,
>  	PMIC_GPIO_FUNC_INDEX_DTEST4,
> +	PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER,
>  };

[ ... ]

> @@ -183,25 +191,86 @@ struct pmic_gpio_state {
>  	{"qcom,analog-pass",		PMIC_GPIO_CONF_ANALOG_PASS,	0},
> -	{"qcom,dtest-buffer",           PMIC_GPIO_CONF_DTEST_BUFFER,    0},
> +	{"qcom,dtest-buffer",		PMIC_GPIO_CONF_DTEST_BUFFER,	0},
> +	{"qcom,1p2v-1p8v-ls-en",	PMIC_GPIO_CONF_LS_ENABLE,	0},
>  };

[Severity: Medium]
Since this commit adds the PMIC_GPIO_CONF_LS_ENABLE property and handles it
in pmic_gpio_pinconf_group_set(), does it also need corresponding read
support?

The driver uses generic pinconf dumps but lacks a pin_config_group_get
implementation, and pmic_gpio_pinconf_pin_get() lacks a case for the new
parameter. This appears to break the symmetry of the pinctrl API, which
might cause programmatic reads of this configuration to fail with -EINVAL.

[ ... ]

> @@ -214,16 +283,16 @@ static const char *const pmic_gpio_groups[] = {
>  };
>  
>  static const char *const pmic_gpio_functions[] = {
> -	[PMIC_GPIO_FUNC_INDEX_NORMAL]	= PMIC_GPIO_FUNC_NORMAL,
> -	[PMIC_GPIO_FUNC_INDEX_PAIRED]	= PMIC_GPIO_FUNC_PAIRED,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC1]	= PMIC_GPIO_FUNC_FUNC1,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC2]	= PMIC_GPIO_FUNC_FUNC2,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC3]	= PMIC_GPIO_FUNC_FUNC3,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC4]	= PMIC_GPIO_FUNC_FUNC4,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST1]	= PMIC_GPIO_FUNC_DTEST1,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST2]	= PMIC_GPIO_FUNC_DTEST2,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST3]	= PMIC_GPIO_FUNC_DTEST3,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST4]	= PMIC_GPIO_FUNC_DTEST4,
> +	[PMIC_GPIO_FUNC_INDEX_NORMAL]		= PMIC_GPIO_FUNC_NORMAL,
> +	[PMIC_GPIO_FUNC_INDEX_PAIRED]		= PMIC_GPIO_FUNC_PAIRED,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC1]		= PMIC_GPIO_FUNC_FUNC1,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC2]		= PMIC_GPIO_FUNC_FUNC2,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC3]		= PMIC_GPIO_FUNC_FUNC3,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC4]		= PMIC_GPIO_FUNC_FUNC4,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST1]		= PMIC_GPIO_FUNC_DTEST1,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST2]		= PMIC_GPIO_FUNC_DTEST2,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST3]		= PMIC_GPIO_FUNC_DTEST3,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST4]		= PMIC_GPIO_FUNC_DTEST4,
>  };

[Severity: High]
Should PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER be added to this array?

If pmic_gpio_pinconf_pin_dbg_show() formats the status of a level-shifter
pin, it dereferences pmic_gpio_functions[function]. For a level-shifter
pin, this would be index 10 (or potentially 12 for non-LV/MV pins), while
the array only has 10 elements.

Could this cause an out-of-bounds memory access that crashes the kernel when
reading pinctrl debugfs?

[ ... ]

> @@ -513,7 +639,8 @@ static int pmic_gpio_pinconf_pin_set(struct pinctrl_dev *pctldev,
>  
>  	pad = pctldev->desc->pins[pin].drv_data;
>  
> -	pad->is_enabled = true;
> +	if (pad->function != PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER)
> +		pad->is_enabled = true;

[Severity: High]
The commit message states that the level shifter must be mutually exclusive
with all existing functions and configurations. However, does this code
enforce that exclusivity?

If a device tree simultaneously assigns a normal function to a pin and
enables the level shifter on its associated group, pad->function will be
NORMAL.

Since the check above will pass, it will set pad->is_enabled = true,
potentially causing both the physical GPIO output buffer and the external
level shifter to drive the pin concurrently. Could this result in
electrical overstress?

[Severity: High]
Is there a risk of corrupting hardware registers with the LEVEL_SHIFTER
index?

When setting PMIC_GPIO_REG_MODE_CTL for non-LV/MV pins later in
pmic_gpio_pinconf_pin_set(), the driver does this:

        val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT;
        val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT;

If pad->function is PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER (10, 0b1010),
shifting it by 1 results in 20 (0x14). Since the hardware function field is
only 3 bits, this overflows and overwrites bit 4
(PMIC_GPIO_REG_MODE_DIR_SHIFT).

Since LEVEL_SHIFTER is a software-defined pseudo-function that physically
operates by keeping the pad disabled, should the driver explicitly bypass
hardware register writes for level-shifter pins?

[ ... ]

> @@ -1285,6 +1545,15 @@ static int pmic_gpio_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* Register level-shifter groups and function if hardware and DT both opt in */
> +	if (hw_data->ls_config && hw_data->num_ls &&
> +	    of_property_present(dev->of_node, "qcom,rpmh") &&
> +	    of_property_present(dev->of_node, "qcom,pmic-id")) {
> +		ret = pmic_gpio_register_level_shifters(state, hw_data);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: High]
Will this devres allocation order cause a Use-After-Free during driver
unbind?

Earlier in pmic_gpio_probe(), devm_pinctrl_register_and_init() is called to
register the pinctrl device. Then, pmic_gpio_register_level_shifters()
allocates the level shifter resources (ls_group_data, ls_group_names) using
devm_kcalloc and devm_kzalloc.

Because devres releases resources in LIFO order, the level shifter memory
will be freed before the pinctrl device is unregistered. Does this leave a
window where the pinctrl device remains active and exposed to userspace
(e.g. sysfs/debugfs) while its registered groups and functions point to
freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-pinctrl-level-shifter-v2-0-3a6a025392bf@oss.qualcomm.com?part=4

Re: [PATCH v2 0/4] pinctrl: qcom: spmi-gpio: Add bidirectional level-shifter function support

[Linus Walleij]

On Fri, May 29, 2026 at 3:05 AM Fenglin Wu <fenglin.wu@oss.qualcomm.com> wrote:

> The PMH0101 PMIC introduces BIDIR_LVL_SHIFTER modules that provide
> bidirectional voltage translation between 1.2 V and 1.8 V power
> domains, targeting open-drain signal buses such as I2C. Each level
> shifter shares its two physical pins with a corresponding pair of GPIO
> modules, and its enable state is centrally managed by AOP firmware as
> a shared RPMh "XOB" resource.
(...)

This cover letter has a *very* long text mass, something Andy
Shevchenko strikingly dubbed "Dostoyevsky commitlogs".

It adds completely obvious descriptions of what every
patch does breaking the rule of "don't comment the obvious".

This is usually a sign of LLM AI-assisted commit message.
It also contains emdashes and other obvious signs of AI.

In that case, please use the Assisted-by tag, because the
LLM can then read this comment of mine and learn from it.
https://github.com/torvalds/linux/blob/master/Documentation/process/coding-assistants.rst

Yours,
Linus Walleij