[PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[Chuhong Yuan]

The driver forgets to call regulator_bulk_disable() in remove like that
in probe failure.
Besides, some failed branches in probe do not handle failure correctly.
Add the missed call and revise wrong direct returns to fix it.

Fixes: 2c394ca79604b ("ASoC: Add support for CS42L42 codec")
Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
 sound/soc/codecs/cs42l42.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 5125bb9b37b5..96b3cff50ce9 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -1796,8 +1796,10 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 	/* Reset the Device */
 	cs42l42->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev,
 		"reset", GPIOD_OUT_LOW);
-	if (IS_ERR(cs42l42->reset_gpio))
-		return PTR_ERR(cs42l42->reset_gpio);
+	if (IS_ERR(cs42l42->reset_gpio)) {
+		ret = PTR_ERR(cs42l42->reset_gpio);
+		goto err_disable;
+	}
 
 	if (cs42l42->reset_gpio) {
 		dev_dbg(&i2c_client->dev, "Found reset GPIO\n");
@@ -1831,13 +1833,13 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 		dev_err(&i2c_client->dev,
 			"CS42L42 Device ID (%X). Expected %X\n",
 			devid, CS42L42_CHIP_ID);
-		return ret;
+		goto err_disable;
 	}
 
 	ret = regmap_read(cs42l42->regmap, CS42L42_REVID, &reg);
 	if (ret < 0) {
 		dev_err(&i2c_client->dev, "Get Revision ID failed\n");
-		return ret;
+		goto err_disable;
 	}
 
 	dev_info(&i2c_client->dev,
@@ -1863,7 +1865,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 	if (i2c_client->dev.of_node) {
 		ret = cs42l42_handle_device_data(i2c_client, cs42l42);
 		if (ret != 0)
-			return ret;
+			goto err_disable;
 	}
 
 	/* Setup headset detection */
@@ -1892,6 +1894,8 @@ static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
 	/* Hold down reset */
 	gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
 
+	regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies),
+				cs42l42->supplies);
 	return 0;
 }
 
-- 
2.24.0

Re: [PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Fri, Dec 06, 2019 at 03:52:09PM +0800, Chuhong Yuan wrote:
> The driver forgets to call regulator_bulk_disable() in remove like that
> in probe failure.
> Besides, some failed branches in probe do not handle failure correctly.
> Add the missed call and revise wrong direct returns to fix it.

Same issue with runtime PM here.  

Also please submit one patch per change, each with a clear changelog, as
covered in SubmittingPatches.  This makes it much easier to review
things since it's easier to tell if the patch does what it was intended
to do.  When splitting patches up git gui can be helpful, you can stage
and unstage individual lines by right clicking on them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[Chuhong Yuan]

On Tue, Dec 10, 2019 at 12:24 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Dec 06, 2019 at 03:52:09PM +0800, Chuhong Yuan wrote:
> > The driver forgets to call regulator_bulk_disable() in remove like that
> > in probe failure.
> > Besides, some failed branches in probe do not handle failure correctly.
> > Add the missed call and revise wrong direct returns to fix it.
>
> Same issue with runtime PM here.
>
> Also please submit one patch per change, each with a clear changelog, as
> covered in SubmittingPatches.  This makes it much easier to review
> things since it's easier to tell if the patch does what it was intended
> to do.  When splitting patches up git gui can be helpful, you can stage
> and unstage individual lines by right clicking on them.

I'm sorry that I didn't notice this problem and these patches should be merged
into a series.

I have a question that what if CONFIG_PM is not defined?
Since I have met runtime PM before in the patch
a31eda65ba21 ("net: fec: fix clock count mis-match").
I learned there that in some cases CONFIG_PM is not defined so runtime PM
cannot take effect.
Therefore, undo operations should still exist in remove functions.

Regards,
Chuhong

Re: [PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Tue, Dec 10, 2019 at 12:52:30AM +0800, Chuhong Yuan wrote:

> I have a question that what if CONFIG_PM is not defined?
> Since I have met runtime PM before in the patch
> a31eda65ba21 ("net: fec: fix clock count mis-match").
> I learned there that in some cases CONFIG_PM is not defined so runtime PM
> cannot take effect.
> Therefore, undo operations should still exist in remove functions.

There's also the case where runtime PM is there and the device is active
at suspend - it's not that there isn't a problem, it's that we can't
unconditionally do a disable because we don't know if there was a
matching enable.  It'll need to be conditional on the runtime PM state.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[Chuhong Yuan]

On Tue, Dec 10, 2019 at 1:00 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Dec 10, 2019 at 12:52:30AM +0800, Chuhong Yuan wrote:
>
> > I have a question that what if CONFIG_PM is not defined?
> > Since I have met runtime PM before in the patch
> > a31eda65ba21 ("net: fec: fix clock count mis-match").
> > I learned there that in some cases CONFIG_PM is not defined so runtime PM
> > cannot take effect.
> > Therefore, undo operations should still exist in remove functions.
>
> There's also the case where runtime PM is there and the device is active
> at suspend - it's not that there isn't a problem, it's that we can't
> unconditionally do a disable because we don't know if there was a
> matching enable.  It'll need to be conditional on the runtime PM state.

How about adding a check like #ifndef CONFIG_PM?
I use this in an old version of the mentioned patch.
However, that is not accepted since it seems not symmetric with enable
in the probe.
But I don't find an explicit runtime PM call in the probe here so the
revision pattern of
("net: fec: fix clock count mis-match") seems not applicable.
So I think adding a check is acceptable here, at least it solves the problem.

Re: [PATCH] ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Tue, Dec 10, 2019 at 09:32:12AM +0800, Chuhong Yuan wrote:
> On Tue, Dec 10, 2019 at 1:00 AM Mark Brown <broonie@kernel.org> wrote:

> > There's also the case where runtime PM is there and the device is active
> > at suspend - it's not that there isn't a problem, it's that we can't
> > unconditionally do a disable because we don't know if there was a
> > matching enable.  It'll need to be conditional on the runtime PM state.

> How about adding a check like #ifndef CONFIG_PM?
> I use this in an old version of the mentioned patch.

That won't handle the runtime PM problem, the state will vary depending
on what the system is doing at the time.

> However, that is not accepted since it seems not symmetric with enable
> in the probe.
> But I don't find an explicit runtime PM call in the probe here so the
> revision pattern of

It's got runtime PM ops though so that's clearly a bug that needs to be
fixed itself.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]