From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755732AbcBIUwG (ORCPT ); Tue, 9 Feb 2016 15:52:06 -0500 Received: from gagarine.paulk.fr ([109.190.93.129]:57427 "EHLO gagarine.paulk.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754134AbcBIUwE (ORCPT ); Tue, 9 Feb 2016 15:52:04 -0500 Message-ID: <1455051116.2757.15.camel@collins> Subject: Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support From: Paul Kocialkowski To: Mark Brown Cc: Liam Girdwood , linux-kernel@vger.kernel.org, "Rafael J. Wysocki" Date: Tue, 09 Feb 2016 21:51:56 +0100 In-Reply-To: <1453407851-8039-2-git-send-email-broonie@kernel.org> References: <1453407851-8039-1-git-send-email-broonie@kernel.org> <1453407851-8039-2-git-send-email-broonie@kernel.org> Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-40n6VmE6sApwQO6pBlv7" X-Mailer: Evolution 3.10.4-0ubuntu2+7.0trisquel1 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-40n6VmE6sApwQO6pBlv7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Le jeudi 21 janvier 2016 =C3=A0 20:24 +0000, Mark Brown a =C3=A9crit : > Provide a flag auto_runtime_pm in the regulator_desc which causes the > regulator core to take a runtime PM reference to a regulator while it > is enabled. This helps integration with chip wide power management for > auxiliary PMICs, they may be able to implement chip wide power savings > if nothing on the PMIC is in use. >=20 > Signed-off-by: Mark Brown > --- >=20 > Not tested at all yet, pushing out for testing by others who have > devices that could benefit from this. Thanks for pushing this, I didn't think it would be that easy to implement. I have tried using those patches on top of my Optimus Black support series (as of v2), on top of 4.5.0-rc2. As-is, it fails at run-time, due to badly-ordered calls to runtime pm functions. Here is the relevant part of the kernel log: [ 1.943359] regulator disable, pm autosuspend [ 1.949615] regulator enable, pm sync [ 1.953491] lp872x_runtime_resume() 37 [ 1.957519] lp872x_runtime_suspend() 37 Despite having the regulator disabled and enabled in that precise order, the runtime functions are called in the opposite order, which causes the enable pin (and thus the whole regulator chip) to be disabled. Is that behavior intended? I suppose this is not an issue at the regulator core level. The diff I used to produce this is as follows. Perhaps I'm doing something wrong? diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index e39c75d..416701d 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2061,6 +2061,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev) } =20 if (rdev->desc->auto_runtime_pm) { + printk(KERN_ERR "regulator enable, pm sync\n"); ret =3D pm_runtime_get_sync(rdev->dev.parent); if (ret < 0) goto err; @@ -2190,8 +2191,10 @@ static int _regulator_do_disable(struct regulator_dev *rdev) return ret; } =20 - if (rdev->desc->auto_runtime_pm) + if (rdev->desc->auto_runtime_pm) { + printk(KERN_ERR "regulator disable, pm autosuspend\n"); pm_runtime_put_autosuspend(rdev->dev.parent); + } =20 /* cares about last_off_jiffy only if off_on_delay is required by * device. diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c index 3e74861..5b99a34 100644 --- a/drivers/regulator/lp872x.c +++ b/drivers/regulator/lp872x.c @@ -15,6 +15,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -522,6 +525,7 @@ static struct regulator_desc lp8720_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo1"), .id =3D LP8720_ID_LDO1, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -536,6 +540,7 @@ static struct regulator_desc lp8720_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo2"), .id =3D LP8720_ID_LDO2, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -550,6 +555,7 @@ static struct regulator_desc lp8720_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo3"), .id =3D LP8720_ID_LDO3, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -564,6 +570,7 @@ static struct regulator_desc lp8720_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo4"), .id =3D LP8720_ID_LDO4, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp8720_ldo4_vtbl), .volt_table =3D lp8720_ldo4_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -578,6 +585,7 @@ static struct regulator_desc lp8720_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo5"), .id =3D LP8720_ID_LDO5, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -592,6 +600,7 @@ static struct regulator_desc lp8720_regulator_desc[] =3D { .of_match =3D of_match_ptr("buck"), .id =3D LP8720_ID_BUCK, .ops =3D &lp8720_buck_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp8720_buck_vtbl), .volt_table =3D lp8720_buck_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -607,6 +616,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo1"), .id =3D LP8725_ID_LDO1, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -621,6 +631,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo2"), .id =3D LP8725_ID_LDO2, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -635,6 +646,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo3"), .id =3D LP8725_ID_LDO3, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -649,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo4"), .id =3D LP8725_ID_LDO4, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -663,6 +676,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("ldo5"), .id =3D LP8725_ID_LDO5, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), .volt_table =3D lp872x_ldo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -677,6 +691,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("lilo1"), .id =3D LP8725_ID_LILO1, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp8725_lilo_vtbl), .volt_table =3D lp8725_lilo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -691,6 +706,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("lilo2"), .id =3D LP8725_ID_LILO2, .ops =3D &lp872x_ldo_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp8725_lilo_vtbl), .volt_table =3D lp8725_lilo_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -705,6 +721,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("buck1"), .id =3D LP8725_ID_BUCK1, .ops =3D &lp8725_buck_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp8725_buck_vtbl), .volt_table =3D lp8725_buck_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -717,6 +734,7 @@ static struct regulator_desc lp8725_regulator_desc[] =3D { .of_match =3D of_match_ptr("buck2"), .id =3D LP8725_ID_BUCK2, .ops =3D &lp8725_buck_ops, + .auto_runtime_pm =3D 1, .n_voltages =3D ARRAY_SIZE(lp8725_buck_vtbl), .volt_table =3D lp8725_buck_vtbl, .type =3D REGULATOR_VOLTAGE, @@ -985,9 +1003,63 @@ static int lp872x_probe(struct i2c_client *cl, const struct i2c_device_id *id) if (ret) return ret; =20 - return lp872x_regulator_register(lp); + pm_runtime_enable(&cl->dev); + + ret =3D lp872x_regulator_register(lp); + if (ret) { + pm_runtime_disable(&cl->dev); + return ret; + } + + return 0; +} + +static int lp872x_runtime_suspend(struct device *dev) +{ + struct lp872x *lp =3D i2c_get_clientdata(to_i2c_client(dev)); + struct gpio_desc *gpiod; + int gpio; + + printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio); + + gpio =3D lp->pdata->enable_gpio; + if (!gpio_is_valid(gpio)) + return 0; + + gpiod =3D gpio_to_desc(gpio); + gpiod_set_value(gpiod, 0); + + return 0; +} + +static int lp872x_runtime_resume(struct device *dev) +{ + struct lp872x *lp =3D i2c_get_clientdata(to_i2c_client(dev)); + struct gpio_desc *gpiod; + int gpio; + + printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio); + + gpio =3D lp->pdata->enable_gpio; + if (!gpio_is_valid(gpio)) + return 0; + + gpiod =3D gpio_to_desc(gpio); + gpiod_set_value(gpiod, 0); + + /* Each chip has a different enable delay. */ + if (lp->chipid =3D=3D LP8720) + usleep_range(LP8720_ENABLE_DELAY, 1.5 * LP8720_ENABLE_DELAY); + else + usleep_range(LP8725_ENABLE_DELAY, 1.5 * LP8725_ENABLE_DELAY); + + return 0; } =20 +static const struct dev_pm_ops lp872x_pm_ops =3D { + SET_RUNTIME_PM_OPS(lp872x_runtime_suspend, lp872x_runtime_resume, NULL) +}; + static const struct of_device_id lp872x_dt_ids[] =3D { { .compatible =3D "ti,lp8720", }, { .compatible =3D "ti,lp8725", }, @@ -1006,6 +1078,7 @@ static struct i2c_driver lp872x_driver =3D { .driver =3D { .name =3D "lp872x", .of_match_table =3D of_match_ptr(lp872x_dt_ids), + .pm =3D &lp872x_pm_ops, }, .probe =3D lp872x_probe, .id_table =3D lp872x_ids, --=-40n6VmE6sApwQO6pBlv7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAABAgAGBQJWulFsAAoJEIT9weqP7pUMWGIP/3yJ7xxpsKPmAnSdZ34Ng2rO odzu21eGc5xoC6h35V3LgOui2BFJ3BSVAohORFhz1CP/FslthnhHS/jzR0NXvPXO /zkJQBYpzTDMCoUoQ0tOjRS3SKx/s0K1VktR/oeuajopSIH4zu0mZDlF7fjFePiq /P+gKPz9om8de6obUaU0O2AjdzDx47XpL9qM12gkbHXW9W93szURaOxIMKl+8PFu 3zfT5COEHVCtS/6BWq2SuVhXSiG+nAGqqYVUDGpeAUmKRhrrf+3aTO+mGYIaA7JP JYnZEk5ah9P9OKbCh6iA44+2jpAZQ4mS8+rCxT1xYd6DpHf6y29TLKVpIGmsbiuU 0XEHMAmZBLcJlOwu1/JHuCjSb04D36HqjyH2cld6lZEaluIR+wpLyPovGILx8EsY fL7Cd7xwADY0g+/r0Ma/S+KolP4ppvL2nUQg5qF1tbRNJ5C4w2olZV0GRNqFvxQZ UR7OkLUrWsWrLhE8ttLoe6H3430kUaFBydGIjt03FGoLWnyUB4yc+5imtLPWBy+Y bey0LWBEHGWAWepdAma3TQXONaBSb60Yn1fgHQuZ/p+ZqmVx8kLi/cozNcQLla8W 5w7XnL9/7wjJdoQA5WgEVxYg5y1QaBSY8MGPIiti/7OHvVKUan8ElPIOzHoy2rvL Ux5RTL511JkVk+fs15yP =z0Xu -----END PGP SIGNATURE----- --=-40n6VmE6sApwQO6pBlv7--