[PATCH] pinctrl: sunxi: skip unused per-bank supply on controllers without IO bias

[PATCH] pinctrl: sunxi: skip unused per-bank supply on controllers without IO bias

[hsyysy]

From: Yang Siyuan <hsyysy@163.com>

sunxi_pmx_request() always requests the "vcc-pX" regulator of the pin's
bank, but that supply is only consumed by
sunxi_pinctrl_set_io_bias_cfg(). Controllers without io_bias_cfg_variant
never use it, so the request only makes the regulator core warn:

  sun50i-h616-r-pinctrl 7022000.pinctrl: supply vcc-pl not found, using
  dummy regulator

The H616 R_PIO is such a controller, and its PL0/PL1 pins drive the
R_I2C bus, so every H616 board hits the warning during boot.

Return early when the controller cannot configure the IO bias voltage,
and teach the free path that no supply was requested for that bank.

Signed-off-by: Yang Siyuan <hsyysy@163.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 70099052d..a5222cfbd 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -924,6 +924,15 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
 		return 0;
 	}
 
+	/*
+	 * The per-bank supply is only used to configure the IO bias voltage.
+	 * Controllers without bias configuration (e.g. the H616 R_PIO) have no
+	 * supply to request, and asking for one only makes the regulator core
+	 * warn about the missing regulator for every bank in use.
+	 */
+	if (!pctl->desc->io_bias_cfg_variant)
+		return 0;
+
 	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
 	reg = regulator_get(pctl->dev, supply);
 	if (IS_ERR(reg))
@@ -959,6 +968,10 @@ static int sunxi_pmx_free(struct pinctrl_dev *pctldev, unsigned offset)
 					    PINS_PER_BANK;
 	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
 
+	/* No supply was requested for this bank (no IO bias configuration). */
+	if (!s_reg->regulator)
+		return 0;
+
 	if (!refcount_dec_and_test(&s_reg->refcount))
 		return 0;
 
base-commit: 091b06388b117fefdcb63336ac09956a4914b961
-- 
2.56.0

Re: [PATCH] pinctrl: sunxi: skip unused per-bank supply on controllers without IO bias

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: sunxi: sunxi_pmx_request() skips enabling per-bank power supplies
--

commit 9182c2f5356e50617749f1a17d156851c7683cd0
Author: Yang Siyuan <hsyysy@163.com>

pinctrl: sunxi: skip unused per-bank supply on controllers without IO bias

This commit skips requesting the per-bank "vcc-pX" regulator in
sunxi_pmx_request() and freeing it in sunxi_pmx_free() when
pctl->desc->io_bias_cfg_variant is not set, in order to silence missing
supply warnings on controllers without IO bias configuration such as the
H616 R_PIO.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 31cd142ce0f79..4b877ec1e1836 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -924,6 +924,15 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
>  		return 0;
>  	}
>  
> +	/*
> +	 * The per-bank supply is only used to configure the IO bias voltage.
> +	 * Controllers without bias configuration (e.g. the H616 R_PIO) have no
> +	 * supply to request, and asking for one only makes the regulator core
> +	 * warn about the missing regulator for every bank in use.
> +	 */
> +	if (!pctl->desc->io_bias_cfg_variant)
> +		return 0;
> +
>  	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
>  	reg = regulator_get(pctl->dev, supply);
>  	if (IS_ERR(reg))

[Severity: High]
Does returning early in sunxi_pmx_request() when
!pctl->desc->io_bias_cfg_variant break per-bank power supply enabling on
SoCs without software-configurable IO bias registers?

In sunxi_pmx_request(), the per-bank "vcc-pX" supply is not only passed to
sunxi_pinctrl_set_io_bias_cfg(), but is also enabled via regulator_enable()
to power the physical pin bank while its pins are requested (originally
added in commit 9a2a566adb00 ("pinctrl: sunxi: Deal with per-bank
regulators") before IO bias configuration support in commit 402bfb3c1352):

drivers/pinctrl/sunxi/pinctrl-sunxi.c:sunxi_pmx_request() {
	...
	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
	reg = regulator_get(pctl->dev, supply);
	if (IS_ERR(reg))
		return dev_err_probe(pctl->dev, PTR_ERR(reg),
				     "Couldn't get bank P%c regulator\n",
				     'A' + bank);

	ret = regulator_enable(reg);
	if (ret) {
		dev_err(pctl->dev,
			"Couldn't enable bank P%c regulator\n", 'A' + bank);
		goto out;
	}

	sunxi_pinctrl_set_io_bias_cfg(pctl, offset, reg);
	...
}

Most Allwinner pinctrl descriptors leave io_bias_cfg_variant as 0
(BIAS_VOLTAGE_NONE), including sun4i_a10_pinctrl_data in
drivers/pinctrl/sunxi/pinctrl-sun4i-a10.c (used for A10, A20, R40, V40),
a64_pinctrl_data in drivers/pinctrl/sunxi/pinctrl-sun50i-a64.c, and
sun8i_v3s_pinctrl_data in drivers/pinctrl/sunxi/pinctrl-sun8i-v3s.c:

drivers/pinctrl/sunxi/pinctrl-sun50i-a64.c:
static const struct sunxi_pinctrl_desc a64_pinctrl_data = {
	.pins = a64_pins,
	.npins = ARRAY_SIZE(a64_pins),
	.irq_banks = 3,
};

However, boards using those SoCs wire physical per-bank VCC-Px rails to
switchable, non-always-on PMIC regulators via vcc-pX-supply in Device Tree,
such as arch/arm/boot/dts/allwinner/sun8i-r40-bananapi-m2-ultra.dts:

&pio {
	pinctrl-names = "default";
	pinctrl-0 = <&clk_out_a_pin>;
	vcc-pa-supply = <&reg_aldo2>;
	vcc-pc-supply = <&reg_dcdc1>;
	vcc-pd-supply = <&reg_dcdc1>;
	vcc-pe-supply = <&reg_eldo1>;
	vcc-pf-supply = <&reg_dcdc1>;
	vcc-pg-supply = <&reg_dldo1>;
};

&reg_aldo2 {
	regulator-min-microvolt = <2500000>;
	regulator-max-microvolt = <2500000>;
	regulator-name = "vcc-pa";
};

Similar non-always-on per-bank supplies are used on A64 boards (such as
sun50i-a64-pinephone.dtsi, sun50i-a64-pinebook.dts,
sun50i-a64-olinuxino.dts, and sun50i-a64-teres-i.dts for vcc-pd-supply,
vcc-pe-supply, and vcc-pg-supply), A20 boards
(sun7i-a20-olinuxino-lime2.dts), and S3 boards (sun8i-s3-pinecube.dts).

If sunxi_pmx_request() skips regulator_get() and regulator_enable() when
!pctl->desc->io_bias_cfg_variant, won't these per-bank regulators remain
unenabled (or be disabled as unused at late_initcall by
regulator_init_complete()), cutting power to active pin banks and attached
peripherals?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026144751.1392715-1-hsyysy@163.com?part=1

[PATCH v2] pinctrl: sunxi: request the per-bank supply optionally

[Yang Siyuan]

The per-bank "vcc-pX" supplies are optional in the device tree: the
binding documents them, but notes that many boards do not describe them.
sunxi_pmx_request() uses regulator_get() anyway, so the regulator core
falls back to a dummy regulator and warns for every bank that has no
supply:

  sun50i-h616-r-pinctrl 7022000.pinctrl: supply vcc-pl not found, using
  dummy regulator

The H616 R_PIO is such a controller, and its PL0/PL1 pins drive the
R_I2C bus, so every H616 board hits the warning during boot.

Use regulator_get_optional() instead.  A bank supply that is described
in the device tree is still acquired and enabled, as it powers the
physical VCC-Px rail, but one that is absent is skipped without a
warning.

Signed-off-by: Yang Siyuan <hsyysy@163.com>
---

Changes in v2:
- Do not skip the supply on controllers without IO bias configuration:
  the per-bank regulator also powers the physical VCC-Px rail, so a
  supply described in the device tree must still be requested and
  enabled.  Use regulator_get_optional() so only banks that have no
  supply are skipped, which silences the warning without changing the
  power management behaviour.
- Link to v1: https://lore.kernel.org/linux-gpio/20261026144751.1392715-1-hsyysy@163.com/
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 70099052d..23c1fbd46 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -925,11 +925,19 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
 	}
 
 	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
-	reg = regulator_get(pctl->dev, supply);
-	if (IS_ERR(reg))
+	reg = regulator_get_optional(pctl->dev, supply);
+	if (IS_ERR(reg)) {
+		/*
+		 * The per-bank supply is optional: boards that do not
+		 * describe it simply have no regulator to manage.
+		 */
+		if (PTR_ERR(reg) == -ENODEV)
+			return 0;
+
 		return dev_err_probe(pctl->dev, PTR_ERR(reg),
 				     "Couldn't get bank P%c regulator\n",
 				     'A' + bank);
+	}
 
 	ret = regulator_enable(reg);
 	if (ret) {
@@ -959,6 +967,10 @@ static int sunxi_pmx_free(struct pinctrl_dev *pctldev, unsigned offset)
 					    PINS_PER_BANK;
 	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
 
+	/* The per-bank supply is optional, so this bank may not have one. */
+	if (!s_reg->regulator)
+		return 0;
+
 	if (!refcount_dec_and_test(&s_reg->refcount))
 		return 0;
 
base-commit: 091b06388b117fefdcb63336ac09956a4914b961
-- 
2.56.0

Re: [PATCH v2] pinctrl: sunxi: request the per-bank supply optionally

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: sunxi: unbalanced bank regulator refcount in sunxi_pmx_request()
--

commit 387a0217944495d51d6191636af797bf5c7e5da5
Author: Yang Siyuan <hsyysy@163.com>

pinctrl: sunxi: request the per-bank supply optionally

This commit switches sunxi_pmx_request() from regulator_get() to
regulator_get_optional() so that banks without a per-bank supply in the
device tree do not trigger a dummy regulator warning. When
regulator_get_optional() returns -ENODEV, sunxi_pmx_request() returns 0
with s_reg->regulator left as NULL, and sunxi_pmx_free() skips cleanup when
s_reg->regulator is NULL.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 31cd142ce0f79..f6c48a287a0e4 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -925,11 +925,19 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
>  	}
>  
>  	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
> -	reg = regulator_get(pctl->dev, supply);
> -	if (IS_ERR(reg))
> +	reg = regulator_get_optional(pctl->dev, supply);
> +	if (IS_ERR(reg)) {
> +		/*
> +		 * The per-bank supply is optional: boards that do not
> +		 * describe it simply have no regulator to manage.
> +		 */
> +		if (PTR_ERR(reg) == -ENODEV)
> +			return 0;
> +
>  		return dev_err_probe(pctl->dev, PTR_ERR(reg),
>  				     "Couldn't get bank P%c regulator\n",
>  				     'A' + bank);
> +	}
>  
>  	ret = regulator_enable(reg);
>  	if (ret) {
> @@ -959,6 +967,10 @@ static int sunxi_pmx_free(struct pinctrl_dev *pctldev, unsigned offset)
>  					    PINS_PER_BANK;
>  	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
>  
> +	/* The per-bank supply is optional, so this bank may not have one. */
> +	if (!s_reg->regulator)
> +		return 0;

[Severity: High]
Can leaving s_reg->regulator as NULL on -ENODEV cause s_reg->refcount to
become unbalanced and disable the bank regulator while other pins in the
same bank are still active?

In sunxi_pmx_request(), returning 0 on -ENODEV without recording that the
bank has no supply or incrementing s_reg->refcount means every subsequent
pin request in the same bank will call regulator_get_optional() again.

Inside regulator_get_optional() -> _regulator_get() ->
regulator_dev_lookup(), when the pinctrl device tree node does not have a
vcc-pX-supply property, regulator_dt_lookup() returns NULL and
regulator_dev_lookup() falls back to a global name lookup:

drivers/regulator/core.c:regulator_dev_lookup() {
	/* first do a dt based lookup */
	r = regulator_dt_lookup(dev, supply);
	if (r)
		return r;
...
	r = regulator_lookup_by_name(supply);
	if (r)
		return r;

	return ERR_PTR(-ENODEV);
}

On several Allwinner boards (for example,
arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi and
sun50i-a64-pine64.dts), vcc-pl-supply is omitted from r_pio to avoid a
circular dependency with the PMIC bus on PL0/PL1, while the PMIC regulator
node defines regulator-name = "vcc-pl":

	&r_pio {
		/*
		 * FIXME: We can't add that supply for now since it would
		 * create a circular dependency between pinctrl, the regulator
		 * and the RSB Bus.
		 *
		 * vcc-pl-supply = <&reg_aldo2>;
		 */
	};
	...
	&reg_aldo2 {
		regulator-always-on;
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
		regulator-name = "vcc-pl";
	};

When the PMIC bus driver probes before the PMIC and requests PL0/PL1,
regulator_get_optional(pctl->dev, "vcc-pl") returns -ENODEV, leaving
s_reg->regulator as NULL and s_reg->refcount at 0 for those active pins.

Once the PMIC registers the regulator named "vcc-pl", a later request for
another pin in bank PL sees s_reg->regulator == NULL, calls
regulator_get_optional(pctl->dev, "vcc-pl") again, matches the regulator via
regulator_lookup_by_name("vcc-pl"), and initializes s_reg->refcount to 1:

drivers/pinctrl/sunxi/pinctrl-sunxi.c:sunxi_pmx_request() {
	...
	s_reg->regulator = reg;
	refcount_set(&s_reg->refcount, 1);
}

If that later pin (or one of the earlier pins requested before the PMIC
probed) is subsequently freed in sunxi_pmx_free(), s_reg->regulator is now
non-NULL, so refcount_dec_and_test(&s_reg->refcount) drops the refcount to 0
and calls regulator_disable() and regulator_put() while other pins in the
bank remain requested.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026152027.1417178-1-hsyysy@163.com?part=1

[PATCH v3] pinctrl: sunxi: request the per-bank supply optionally

[Yang Siyuan]

The per-bank "vcc-pX" supplies are optional in the device tree: the
binding documents them, but notes that many boards do not describe them.
sunxi_pmx_request() uses regulator_get() anyway, so the regulator core
falls back to a dummy regulator and warns for every bank that has no
supply:

  sun50i-h616-r-pinctrl 7022000.pinctrl: supply vcc-pl not found, using
  dummy regulator

The H616 R_PIO is such a controller, and its PL0/PL1 pins drive the
R_I2C bus, so every H616 board hits the warning during boot.

Use of_regulator_get_optional() instead.  A bank supply that is described
in the device tree is still acquired and enabled, as it powers the
physical VCC-Px rail, but one that is absent is skipped without a
warning.

The OF-only lookup matters here: regulator_get_optional() also falls back
to a global regulator-name lookup.  If a regulator with the same name as
an absent bank supply is registered later, a subsequent pin request in
that bank could acquire it and start the per-bank refcount at one, after
earlier pins in the bank had already been requested without a regulator.
Freeing any of those pins would then disable the supply while other pins
in the bank are still active.  Restricting the lookup to the pinctrl node
makes an absent supply permanent; record that state so later pin requests
do not retry the lookup.  The refcount therefore stays balanced.

Signed-off-by: Yang Siyuan <hsyysy@163.com>
---

Changes in v3:
- Use of_regulator_get_optional() instead of regulator_get_optional().
  The latter still performs a global regulator-name lookup, which can
  bind an unrelated regulator that appears after some pins were already
  requested and unbalance the per-bank refcount.
- Remember that a bank has no supply so later pin requests skip the
  lookup, and keep the existing no-regulator check in sunxi_pmx_free().
- Link to v2: https://lore.kernel.org/linux-gpio/20261026152027.1417178-1-hsyysy@163.com/
- Link to v1: https://lore.kernel.org/linux-gpio/20261026144751.1392715-1-hsyysy@163.com/
---
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 29 +++++++++++++++++++++++++--
 drivers/pinctrl/sunxi/pinctrl-sunxi.h |  1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 70099052d..d2345a85a 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -913,23 +913,44 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
 					    PINS_PER_BANK;
 	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
 	struct regulator *reg = s_reg->regulator;
+	struct device_node *np = dev_of_node(pctl->dev);
 	char supply[16];
 	int ret;
 
 	if (WARN_ON_ONCE(bank_offset >= ARRAY_SIZE(pctl->regulators)))
 		return -EINVAL;
 
+	if (s_reg->supply_absent)
+		return 0;
+
 	if (reg) {
 		refcount_inc(&s_reg->refcount);
 		return 0;
 	}
 
+	if (!np) {
+		s_reg->supply_absent = true;
+		return 0;
+	}
+
 	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
-	reg = regulator_get(pctl->dev, supply);
-	if (IS_ERR(reg))
+	/*
+	 * Use the OF-only lookup: regulator_get_optional() would also match
+	 * a same-named regulator from another device.  Such a regulator can
+	 * appear after some pins in this bank have already been requested,
+	 * which would unbalance the per-bank refcount.
+	 */
+	reg = of_regulator_get_optional(pctl->dev, np, supply);
+	if (IS_ERR(reg)) {
+		if (PTR_ERR(reg) == -ENODEV) {
+			s_reg->supply_absent = true;
+			return 0;
+		}
+
 		return dev_err_probe(pctl->dev, PTR_ERR(reg),
 				     "Couldn't get bank P%c regulator\n",
 				     'A' + bank);
+	}
 
 	ret = regulator_enable(reg);
 	if (ret) {
@@ -959,6 +980,10 @@ static int sunxi_pmx_free(struct pinctrl_dev *pctldev, unsigned offset)
 					    PINS_PER_BANK;
 	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
 
+	/* The per-bank supply is optional, so this bank may not have one. */
+	if (!s_reg->regulator)
+		return 0;
+
 	if (!refcount_dec_and_test(&s_reg->refcount))
 		return 0;
 
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 8bd00c6ff..651249b23 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -162,6 +162,7 @@ struct sunxi_pinctrl_group {
 struct sunxi_pinctrl_regulator {
 	struct regulator	*regulator;
 	refcount_t		refcount;
+	bool			supply_absent;
 };
 
 struct sunxi_pinctrl {
base-commit: 091b06388b117fefdcb63336ac09956a4914b961
-- 
2.56.0