| Message ID | 20191206075209.18068-1-hslester96@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: cs42l42: add missed regulator_bulk_disable in remove and fix probe failure | expand |
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.
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
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.
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.
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.
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, ®); 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; }
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(-)