[PATCH v3 0/3] Handle shared reset GPIO for WSA883x speakers

[PATCH v3 0/3] Handle shared reset GPIO for WSA883x speakers

[Mohammad Rafi Shaik]

On some Qualcomm platforms such as QCS6490-RB3Gen2, the multiple
WSA8830/WSA8835 speakers share a common reset (shutdown) GPIO.
To handle such cases, use the reset controller framework along with the
"reset-gpio" driver.

Add devm action to safely disable regulator on device removal to
prevent potential warnings from _regulator_put() during device
removal

Tested on:
	- QCS6490-RB3Gen2

changes in [v3]:
	- Created separate patch for devm action to safely disable
	  regulator.
	- cleanup the v2-0002 patch.
	- Link to V2: https://lore.kernel.org/linux-sound/20250718104628.3732645-1-mohammad.rafi.shaik@oss.qualcomm.com/

changes in [v2]:
	- Addressed the review comments from Krzysztof, Dmitry, Philipp.
	- Used devm_reset_control_get_optional_shared_deasserted() api.
	- created deasserts/asserts functions to handle reset gpios.
	- Register devm action to safely disable the regulator on device removal.
	- Link to V1: https://lore.kernel.org/linux-sound/20250620103012.360794-1-mohammad.rafi.shaik@oss.qualcomm.com/	

Mohammad Rafi Shaik (3):
  ASoC: dt-bindings: qcom,wsa8830: Add reset-gpios for shared line
  ASoC: codecs: wsa883x: Add devm action to safely disable regulator on
    device removal
  ASoC: codecs: wsa883x: Handle shared reset GPIO for WSA883x speakers

 .../bindings/sound/qcom,wsa883x.yaml          | 11 ++-
 sound/soc/codecs/wsa883x.c                    | 93 ++++++++++++++-----
 2 files changed, 81 insertions(+), 23 deletions(-)


base-commit: d7af19298454ed155f5cf67201a70f5cf836c842
-- 
2.34.1

[PATCH v3 1/3] ASoC: dt-bindings: qcom,wsa8830: Add reset-gpios for shared line

[Mohammad Rafi Shaik]

On Qualcomm platforms such as QCS6490-RB3Gen2, the WSA883x speaker
amplifiers share the SD_N GPIO line between two speakers, thus
requires coordinated control when asserting the GPIO. Linux supports
shared GPIO handling via the "reset-gpios" property, which can be
used to specify either the powerdown or reset GPIOs.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 .../devicetree/bindings/sound/qcom,wsa883x.yaml       | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/qcom,wsa883x.yaml b/Documentation/devicetree/bindings/sound/qcom,wsa883x.yaml
index 14d312f9c345..098f1df62c8c 100644
--- a/Documentation/devicetree/bindings/sound/qcom,wsa883x.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,wsa883x.yaml
@@ -29,6 +29,10 @@ properties:
     description: GPIO spec for Powerdown/Shutdown line to use (pin SD_N)
     maxItems: 1
 
+  reset-gpios:
+    description: Powerdown/Shutdown line to use (pin SD_N)
+    maxItems: 1
+
   vdd-supply:
     description: VDD Supply for the Codec
 
@@ -50,10 +54,15 @@ required:
   - compatible
   - reg
   - vdd-supply
-  - powerdown-gpios
   - "#thermal-sensor-cells"
   - "#sound-dai-cells"
 
+oneOf:
+  - required:
+      - powerdown-gpios
+  - required:
+      - reset-gpios
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

[PATCH v3 2/3] ASoC: codecs: wsa883x: Add devm action to safely disable regulator on device removal

[Mohammad Rafi Shaik]

To prevent potential warnings from _regulator_put() during device
removal, register a devm-managed cleanup action using
devm_add_action_or_reset() to safely disable the regulator
associated with the WSA883x codec, ensuring that the regulator
is properly disabled when the device is removed, even if the
probe fails or the driver is unloaded unexpectedly.

Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/codecs/wsa883x.c | 44 ++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c
index 188363b03b93..d5bc71b28a3a 100644
--- a/sound/soc/codecs/wsa883x.c
+++ b/sound/soc/codecs/wsa883x.c
@@ -14,6 +14,7 @@
 #include <linux/printk.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
@@ -1546,6 +1547,13 @@ static const struct hwmon_chip_info wsa883x_hwmon_chip_info = {
 	.info	= wsa883x_hwmon_info,
 };
 
+static void wsa883x_regulator_disable(void *data)
+{
+	struct wsa883x_priv *wsa883x = data;
+
+	regulator_disable(wsa883x->vdd);
+}
+
 static int wsa883x_probe(struct sdw_slave *pdev,
 			 const struct sdw_device_id *id)
 {
@@ -1566,13 +1574,20 @@ static int wsa883x_probe(struct sdw_slave *pdev,
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to enable vdd regulator\n");
 
+	/*
+	 * Register devm action to safely disable the regulator on device removal.
+	 * This prevents a potential release warning from _regulator_put().
+	 */
+	ret = devm_add_action_or_reset(dev, wsa883x_regulator_disable,
+				       wsa883x);
+	if (ret)
+		return ret;
+
 	wsa883x->sd_n = devm_gpiod_get_optional(dev, "powerdown",
 						GPIOD_FLAGS_BIT_NONEXCLUSIVE | GPIOD_OUT_HIGH);
-	if (IS_ERR(wsa883x->sd_n)) {
-		ret = dev_err_probe(dev, PTR_ERR(wsa883x->sd_n),
-				    "Shutdown Control GPIO not found\n");
-		goto err;
-	}
+	if (IS_ERR(wsa883x->sd_n))
+		return dev_err_probe(dev, PTR_ERR(wsa883x->sd_n),
+				     "Shutdown Control GPIO not found\n");
 
 	dev_set_drvdata(dev, wsa883x);
 	wsa883x->slave = pdev;
@@ -1598,12 +1613,9 @@ static int wsa883x_probe(struct sdw_slave *pdev,
 	gpiod_direction_output(wsa883x->sd_n, 0);
 
 	wsa883x->regmap = devm_regmap_init_sdw(pdev, &wsa883x_regmap_config);
-	if (IS_ERR(wsa883x->regmap)) {
-		gpiod_direction_output(wsa883x->sd_n, 1);
-		ret = dev_err_probe(dev, PTR_ERR(wsa883x->regmap),
-				    "regmap_init failed\n");
-		goto err;
-	}
+	if (IS_ERR(wsa883x->regmap))
+		return dev_err_probe(dev, PTR_ERR(wsa883x->regmap),
+				     "regmap_init failed\n");
 
 	if (IS_REACHABLE(CONFIG_HWMON)) {
 		struct device *hwmon;
@@ -1623,16 +1635,10 @@ static int wsa883x_probe(struct sdw_slave *pdev,
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 
-	ret = devm_snd_soc_register_component(dev,
-					      &wsa883x_component_drv,
+	return devm_snd_soc_register_component(dev,
+					       &wsa883x_component_drv,
 					       wsa883x_dais,
 					       ARRAY_SIZE(wsa883x_dais));
-err:
-	if (ret)
-		regulator_disable(wsa883x->vdd);
-
-	return ret;
-
 }
 
 static int wsa883x_runtime_suspend(struct device *dev)
-- 
2.34.1

Re: [PATCH v3 2/3] ASoC: codecs: wsa883x: Add devm action to safely disable regulator on device removal

[Krzysztof Kozlowski]

On 27/07/2025 10:31, Mohammad Rafi Shaik wrote:
> To prevent potential warnings from _regulator_put() during device

Warning is either there or not. Either you fix real, specific issue or
not. The code looks correct at first glance, so please describe exactly
how these warnings happen or how what is the bug being fixed.

> removal, register a devm-managed cleanup action using
> devm_add_action_or_reset() to safely disable the regulator
> associated with the WSA883x codec, ensuring that the regulator
> is properly disabled when the device is removed, even if the

Device cannot be removed/unloaded, AFAIK, because of suppressed bind.
Regulator is already disabled during error paths, so that part of above
sentences is just misleading.

How can one trigger the warnings?


> probe fails or the driver is unloaded unexpectedly.

How driver can be unloaded unexpectedly?

Best regards,
Krzysztof

Re: [PATCH v3 2/3] ASoC: codecs: wsa883x: Add devm action to safely disable regulator on device removal

[Mohammad Rafi Shaik]

On 7/27/2025 3:00 PM, Krzysztof Kozlowski wrote:
> On 27/07/2025 10:31, Mohammad Rafi Shaik wrote:
>> To prevent potential warnings from _regulator_put() during device
> 
> Warning is either there or not. Either you fix real, specific issue or
> not. The code looks correct at first glance, so please describe exactly
> how these warnings happen or how what is the bug being fixed.
>

The current wsa883x codec driver manually enables and disables 
regulators during probe and remove.
In patch v3-0003, reset functionality was added using 
devm_reset_control_get_optional_shared_deasserted() for shared gpios.

However, during cleanup, this led to a warning:
"WARNING: CPU: 2 PID: 195 at drivers/regulator/core.c:2450 
_regulator_put+0x50/0x58"

This occurs because the regulator is still enabled/released when the 
devm-managed cleanup path attempts to release it.

To resolve this, remove the manual regulator disable logic and instead 
register a devm-managed cleanup action using devm_add_action_or_reset(). 
This ensures proper cleanup and avoids regulator misuse warnings.

For reference, the wsa884x codec driver already follows this approach by 
using devm actions for regulator management.

>> removal, register a devm-managed cleanup action using
>> devm_add_action_or_reset() to safely disable the regulator
>> associated with the WSA883x codec, ensuring that the regulator
>> is properly disabled when the device is removed, even if the
> 
> Device cannot be removed/unloaded, AFAIK, because of suppressed bind.
> Regulator is already disabled during error paths, so that part of above
> sentences is just misleading.
> 
> How can one trigger the warnings?
>

The warning in _regulator_put() can be triggered by applying patch 
v3-0003, which introduces reset functionality using 
devm_reset_control_get_optional_shared_deasserted().

Since the existing driver handles regulator enable/disable manually, the 
devm-managed reset cleanup path may attempt to release regulators that 
are still enabled, leading to the warning.

This issue highlights the need to replace manual regulator handling with 
devm_add_action_or_reset() to ensure proper cleanup and avoid such warnings.

> 
>> probe fails or the driver is unloaded unexpectedly.
> 
> How driver can be unloaded unexpectedly?
> 

"Unloaded" might not be the most accurate term here. What I meant is 
that the driver’s probe can fail due to an error—such as missing 
resources or improper regulator handling.

Thanks & Regards,
Rafi.

> Best regards,
> Krzysztof

Re: [PATCH v3 2/3] ASoC: codecs: wsa883x: Add devm action to safely disable regulator on device removal

[Krzysztof Kozlowski]

On 28/07/2025 14:36, Mohammad Rafi Shaik wrote:
> 
> 
> On 7/27/2025 3:00 PM, Krzysztof Kozlowski wrote:
>> On 27/07/2025 10:31, Mohammad Rafi Shaik wrote:
>>> To prevent potential warnings from _regulator_put() during device
>>
>> Warning is either there or not. Either you fix real, specific issue or
>> not. The code looks correct at first glance, so please describe exactly
>> how these warnings happen or how what is the bug being fixed.
>>
> 
> The current wsa883x codec driver manually enables and disables 
> regulators during probe and remove.
> In patch v3-0003, reset functionality was added using 
> devm_reset_control_get_optional_shared_deasserted() for shared gpios.


There is no such code at this point. Each patch is a separate commit and
must stand on its own. With its own explanation. You cannot say that you
add bugs later, so you need to fix something now.

Describe actual problem here. If there is no problem here, describe why
you are doing this.

> 
> However, during cleanup, this led to a warning:
> "WARNING: CPU: 2 PID: 195 at drivers/regulator/core.c:2450 
> _regulator_put+0x50/0x58"
> 
> This occurs because the regulator is still enabled/released when the 
> devm-managed cleanup path attempts to release it.

So that patch was broken? You just did not properly clean up there?

> 
> To resolve this, remove the manual regulator disable logic and instead 
> register a devm-managed cleanup action using devm_add_action_or_reset(). 
> This ensures proper cleanup and avoids regulator misuse warnings.
> 
> For reference, the wsa884x codec driver already follows this approach by 
> using devm actions for regulator management.
> 
>>> removal, register a devm-managed cleanup action using
>>> devm_add_action_or_reset() to safely disable the regulator
>>> associated with the WSA883x codec, ensuring that the regulator
>>> is properly disabled when the device is removed, even if the
>>
>> Device cannot be removed/unloaded, AFAIK, because of suppressed bind.
>> Regulator is already disabled during error paths, so that part of above
>> sentences is just misleading.
>>
>> How can one trigger the warnings?
>>
> 
> The warning in _regulator_put() can be triggered by applying patch 
> v3-0003, which introduces reset functionality using 
> devm_reset_control_get_optional_shared_deasserted().


There is no such code now. You say "potential warnings" are here.

> 
> Since the existing driver handles regulator enable/disable manually, the 
> devm-managed reset cleanup path may attempt to release regulators that 
> are still enabled, leading to the warning.
> 
> This issue highlights the need to replace manual regulator handling with 
> devm_add_action_or_reset() to ensure proper cleanup and avoid such warnings.
> 
>>
>>> probe fails or the driver is unloaded unexpectedly.
>>
>> How driver can be unloaded unexpectedly?
>>
> 
> "Unloaded" might not be the most accurate term here. What I meant is 
> that the driver’s probe can fail due to an error—such as missing 
> resources or improper regulator handling.


Use standard Linux terms, e.g. probe failure, probe deferral etc.

Best regards,
Krzysztof

Re: [PATCH v3 2/3] ASoC: codecs: wsa883x: Add devm action to safely disable regulator on device removal

[Mohammad Rafi Shaik]

On 7/28/2025 6:32 PM, Krzysztof Kozlowski wrote:
> On 28/07/2025 14:36, Mohammad Rafi Shaik wrote:
>>
>>
>> On 7/27/2025 3:00 PM, Krzysztof Kozlowski wrote:
>>> On 27/07/2025 10:31, Mohammad Rafi Shaik wrote:
>>>> To prevent potential warnings from _regulator_put() during device
>>>
>>> Warning is either there or not. Either you fix real, specific issue or
>>> not. The code looks correct at first glance, so please describe exactly
>>> how these warnings happen or how what is the bug being fixed.
>>>
>>
>> The current wsa883x codec driver manually enables and disables
>> regulators during probe and remove.
>> In patch v3-0003, reset functionality was added using
>> devm_reset_control_get_optional_shared_deasserted() for shared gpios.
> 
> 
> There is no such code at this point. Each patch is a separate commit and
> must stand on its own. With its own explanation. You cannot say that you
> add bugs later, so you need to fix something now.
> 
> Describe actual problem here. If there is no problem here, describe why
> you are doing this.
> 

Identified the actual root cause of the issue observed in the reset changes.

The failure condition was not properly handled in the reset patch.

I will update the patch to include error handling for failure scenarios 
and ensure regulators are disabled appropriately.

will Drop this patch for next version, only will keep the reset changes.

Thanks & Regards,
Rafi.

>>
>> However, during cleanup, this led to a warning:
>> "WARNING: CPU: 2 PID: 195 at drivers/regulator/core.c:2450
>> _regulator_put+0x50/0x58"
>>
>> This occurs because the regulator is still enabled/released when the
>> devm-managed cleanup path attempts to release it.
> 
> So that patch was broken? You just did not properly clean up there?
> 
>>
>> To resolve this, remove the manual regulator disable logic and instead
>> register a devm-managed cleanup action using devm_add_action_or_reset().
>> This ensures proper cleanup and avoids regulator misuse warnings.
>>
>> For reference, the wsa884x codec driver already follows this approach by
>> using devm actions for regulator management.
>>
>>>> removal, register a devm-managed cleanup action using
>>>> devm_add_action_or_reset() to safely disable the regulator
>>>> associated with the WSA883x codec, ensuring that the regulator
>>>> is properly disabled when the device is removed, even if the
>>>
>>> Device cannot be removed/unloaded, AFAIK, because of suppressed bind.
>>> Regulator is already disabled during error paths, so that part of above
>>> sentences is just misleading.
>>>
>>> How can one trigger the warnings?
>>>
>>
>> The warning in _regulator_put() can be triggered by applying patch
>> v3-0003, which introduces reset functionality using
>> devm_reset_control_get_optional_shared_deasserted().
> 
> 
> There is no such code now. You say "potential warnings" are here.
> 
>>
>> Since the existing driver handles regulator enable/disable manually, the
>> devm-managed reset cleanup path may attempt to release regulators that
>> are still enabled, leading to the warning.
>>
>> This issue highlights the need to replace manual regulator handling with
>> devm_add_action_or_reset() to ensure proper cleanup and avoid such warnings.
>>
>>>
>>>> probe fails or the driver is unloaded unexpectedly.
>>>
>>> How driver can be unloaded unexpectedly?
>>>
>>
>> "Unloaded" might not be the most accurate term here. What I meant is
>> that the driver’s probe can fail due to an error—such as missing
>> resources or improper regulator handling.
> 
> 
> Use standard Linux terms, e.g. probe failure, probe deferral etc.

Ack,

will ensure all upcoming changes are managed effectively.

Thanks & Regards,
Rafi.


> 
> Best regards,
> Krzysztof

[PATCH v3 3/3] ASoC: codecs: wsa883x: Handle shared reset GPIO for WSA883x speakers

[Mohammad Rafi Shaik]

On some Qualcomm platforms such as QCS6490-RB3Gen2, the multiple
WSA8830/WSA8835 speakers share a common reset (shutdown) GPIO.
To handle such cases, use the reset controller framework along
with the "reset-gpio" driver.

Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/codecs/wsa883x.c | 55 +++++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c
index d5bc71b28a3a..0acf111f9583 100644
--- a/sound/soc/codecs/wsa883x.c
+++ b/sound/soc/codecs/wsa883x.c
@@ -469,6 +469,7 @@ struct wsa883x_priv {
 	struct sdw_stream_runtime *sruntime;
 	struct sdw_port_config port_config[WSA883X_MAX_SWR_PORTS];
 	struct gpio_desc *sd_n;
+	struct reset_control *sd_reset;
 	bool port_prepared[WSA883X_MAX_SWR_PORTS];
 	bool port_enable[WSA883X_MAX_SWR_PORTS];
 	int active_ports;
@@ -1547,6 +1548,24 @@ static const struct hwmon_chip_info wsa883x_hwmon_chip_info = {
 	.info	= wsa883x_hwmon_info,
 };
 
+static void wsa883x_reset_assert(void *data)
+{
+	struct wsa883x_priv *wsa883x = data;
+
+	if (wsa883x->sd_reset)
+		reset_control_assert(wsa883x->sd_reset);
+	else
+		gpiod_direction_output(wsa883x->sd_n, 1);
+}
+
+static void wsa883x_reset_deassert(struct wsa883x_priv *wsa883x)
+{
+	if (wsa883x->sd_reset)
+		reset_control_deassert(wsa883x->sd_reset);
+	else
+		gpiod_direction_output(wsa883x->sd_n, 0);
+}
+
 static void wsa883x_regulator_disable(void *data)
 {
 	struct wsa883x_priv *wsa883x = data;
@@ -1554,6 +1573,28 @@ static void wsa883x_regulator_disable(void *data)
 	regulator_disable(wsa883x->vdd);
 }
 
+static int wsa883x_get_reset(struct device *dev, struct wsa883x_priv *wsa883x)
+{
+	wsa883x->sd_reset = devm_reset_control_get_optional_shared_deasserted(dev, NULL);
+	if (IS_ERR(wsa883x->sd_reset))
+		return dev_err_probe(dev, PTR_ERR(wsa883x->sd_reset),
+				     "Failed to get reset\n");
+	/*
+	 * if sd_reset: NULL, so use the backwards compatible way for powerdown-gpios,
+	 * which does not handle sharing GPIO properly.
+	 */
+	if (!wsa883x->sd_reset) {
+		wsa883x->sd_n = devm_gpiod_get_optional(dev, "powerdown",
+							GPIOD_FLAGS_BIT_NONEXCLUSIVE |
+							GPIOD_OUT_HIGH);
+		if (IS_ERR(wsa883x->sd_n))
+			return dev_err_probe(dev, PTR_ERR(wsa883x->sd_n),
+					     "Shutdown Control GPIO not found\n");
+	}
+
+	return 0;
+}
+
 static int wsa883x_probe(struct sdw_slave *pdev,
 			 const struct sdw_device_id *id)
 {
@@ -1583,11 +1624,9 @@ static int wsa883x_probe(struct sdw_slave *pdev,
 	if (ret)
 		return ret;
 
-	wsa883x->sd_n = devm_gpiod_get_optional(dev, "powerdown",
-						GPIOD_FLAGS_BIT_NONEXCLUSIVE | GPIOD_OUT_HIGH);
-	if (IS_ERR(wsa883x->sd_n))
-		return dev_err_probe(dev, PTR_ERR(wsa883x->sd_n),
-				     "Shutdown Control GPIO not found\n");
+	ret = wsa883x_get_reset(dev, wsa883x);
+	if (ret)
+		return ret;
 
 	dev_set_drvdata(dev, wsa883x);
 	wsa883x->slave = pdev;
@@ -1610,7 +1649,11 @@ static int wsa883x_probe(struct sdw_slave *pdev,
 	pdev->prop.simple_clk_stop_capable = true;
 	pdev->prop.sink_dpn_prop = wsa_sink_dpn_prop;
 	pdev->prop.scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
-	gpiod_direction_output(wsa883x->sd_n, 0);
+
+	wsa883x_reset_deassert(wsa883x);
+	ret = devm_add_action_or_reset(dev, wsa883x_reset_assert, wsa883x);
+	if (ret)
+		return ret;
 
 	wsa883x->regmap = devm_regmap_init_sdw(pdev, &wsa883x_regmap_config);
 	if (IS_ERR(wsa883x->regmap))
-- 
2.34.1