| Message ID | 1435738921-25027-15-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon: > Implement the ->enable(), ->disable() and ->is_enabled methods and remove > the PWM call in ->set_voltage_sel(). > This is particularly important for critical regulators tagged as always-on, > because not claiming the PWM (and its dependencies) might lead to > unpredictable behavior (like a system hang because the PWM clk is only > claimed when the PWM device is enabled). > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/regulator/pwm-regulator.c > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644 > --- a/drivers/regulator/pwm-regulator.c > +++ b/drivers/regulator/pwm-regulator.c > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct > regulator_dev *rdev, > > drvdata->state = selector; > > - ret = pwm_enable(drvdata->pwm); > - if (ret) { > - dev_err(&rdev->dev, "Failed to enable PWM\n"); > - return ret; > - } > - > return 0; > } > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV; > } > > +static int pwm_regulator_enable(struct regulator_dev *dev) > +{ > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > + > + return pwm_enable(drvdata->pwm); > +} > + > +static int pwm_regulator_disable(struct regulator_dev *dev) > +{ > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > + > + pwm_disable(drvdata->pwm); > + > + return 0; > +} > + > +static int pwm_regulator_is_enabled(struct regulator_dev *dev) > +{ > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > + > + return pwm_is_enabled(drvdata->pwm); > +} nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Heiko, On Wed, 01 Jul 2015 13:58:09 +0200 Heiko Stübner <heiko@sntech.de> wrote: > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon: > > Implement the ->enable(), ->disable() and ->is_enabled methods and remove > > the PWM call in ->set_voltage_sel(). > > This is particularly important for critical regulators tagged as always-on, > > because not claiming the PWM (and its dependencies) might lead to > > unpredictable behavior (like a system hang because the PWM clk is only > > claimed when the PWM device is enabled). > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------ > > 1 file changed, 26 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/regulator/pwm-regulator.c > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644 > > --- a/drivers/regulator/pwm-regulator.c > > +++ b/drivers/regulator/pwm-regulator.c > > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct > > regulator_dev *rdev, > > > > drvdata->state = selector; > > > > - ret = pwm_enable(drvdata->pwm); > > - if (ret) { > > - dev_err(&rdev->dev, "Failed to enable PWM\n"); > > - return ret; > > - } > > - > > return 0; > > } > > > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct > > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV; > > } > > > > +static int pwm_regulator_enable(struct regulator_dev *dev) > > +{ > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > + > > + return pwm_enable(drvdata->pwm); > > +} > > + > > +static int pwm_regulator_disable(struct regulator_dev *dev) > > +{ > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > + > > + pwm_disable(drvdata->pwm); > > + > > + return 0; > > +} > > + > > +static int pwm_regulator_is_enabled(struct regulator_dev *dev) > > +{ > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > + > > + return pwm_is_enabled(drvdata->pwm); > > +} > > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs) Yep, I noticed checkpatch warnings/errors before sending the patch, but since this is just an RFC I decided to fix them for the next version ;-) Best Regards, Boris
Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon: > Hi Heiko, > > On Wed, 01 Jul 2015 13:58:09 +0200 > > Heiko Stübner <heiko@sntech.de> wrote: > > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon: > > > Implement the ->enable(), ->disable() and ->is_enabled methods and > > > remove > > > the PWM call in ->set_voltage_sel(). > > > This is particularly important for critical regulators tagged as > > > always-on, > > > because not claiming the PWM (and its dependencies) might lead to > > > unpredictable behavior (like a system hang because the PWM clk is only > > > claimed when the PWM device is enabled). > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > --- > > > > > > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------ > > > 1 file changed, 26 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/regulator/pwm-regulator.c > > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644 > > > --- a/drivers/regulator/pwm-regulator.c > > > +++ b/drivers/regulator/pwm-regulator.c > > > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct > > > regulator_dev *rdev, > > > > > > drvdata->state = selector; > > > > > > - ret = pwm_enable(drvdata->pwm); > > > - if (ret) { > > > - dev_err(&rdev->dev, "Failed to enable PWM\n"); > > > - return ret; > > > - } > > > - > > > > > > return 0; > > > > > > } > > > > > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct > > > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV; > > > > > > } > > > > > > +static int pwm_regulator_enable(struct regulator_dev *dev) > > > +{ > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > > + > > > + return pwm_enable(drvdata->pwm); > > > +} > > > + > > > +static int pwm_regulator_disable(struct regulator_dev *dev) > > > +{ > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > > + > > > + pwm_disable(drvdata->pwm); > > > + > > > + return 0; > > > +} > > > + > > > +static int pwm_regulator_is_enabled(struct regulator_dev *dev) > > > +{ > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > > + > > > + return pwm_is_enabled(drvdata->pwm); > > > +} > > > > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of > > tabs) > Yep, I noticed checkpatch warnings/errors before sending the patch, but > since this is just an RFC I decided to fix them for the next version ;-) ok, so I'll just skip over any more style issues for now. Making my way through your series and trying it on my veyron right now :-) Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 01 Jul 2015 14:08:18 +0200 Heiko Stübner <heiko@sntech.de> wrote: > Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon: > > Hi Heiko, > > > > On Wed, 01 Jul 2015 13:58:09 +0200 > > > > Heiko Stübner <heiko@sntech.de> wrote: > > > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon: > > > > Implement the ->enable(), ->disable() and ->is_enabled methods and > > > > remove > > > > the PWM call in ->set_voltage_sel(). > > > > This is particularly important for critical regulators tagged as > > > > always-on, > > > > because not claiming the PWM (and its dependencies) might lead to > > > > unpredictable behavior (like a system hang because the PWM clk is only > > > > claimed when the PWM device is enabled). > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > --- > > > > > > > > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------ > > > > 1 file changed, 26 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/regulator/pwm-regulator.c > > > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644 > > > > --- a/drivers/regulator/pwm-regulator.c > > > > +++ b/drivers/regulator/pwm-regulator.c > > > > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct > > > > regulator_dev *rdev, > > > > > > > > drvdata->state = selector; > > > > > > > > - ret = pwm_enable(drvdata->pwm); > > > > - if (ret) { > > > > - dev_err(&rdev->dev, "Failed to enable PWM\n"); > > > > - return ret; > > > > - } > > > > - > > > > > > > > return 0; > > > > > > > > } > > > > > > > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct > > > > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV; > > > > > > > > } > > > > > > > > +static int pwm_regulator_enable(struct regulator_dev *dev) > > > > +{ > > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > > > + > > > > + return pwm_enable(drvdata->pwm); > > > > +} > > > > + > > > > +static int pwm_regulator_disable(struct regulator_dev *dev) > > > > +{ > > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > > > + > > > > + pwm_disable(drvdata->pwm); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int pwm_regulator_is_enabled(struct regulator_dev *dev) > > > > +{ > > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > > > + > > > > + return pwm_is_enabled(drvdata->pwm); > > > > +} > > > > > > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of > > > tabs) > > Yep, I noticed checkpatch warnings/errors before sending the patch, but > > since this is just an RFC I decided to fix them for the next version ;-) > > ok, so I'll just skip over any more style issues for now. Making my way > through your series and trying it on my veyron right now :-) Also note that I haven't tested the series on a real board (just compile tested) because I don't have the board with me right now, but I wanted to post the RFC early so that we can discuss the concepts. Anyway, any feedback on the implementation (including bug reports) is welcome. This is the version I actually tested on the veyron board: https://github.com/bbrezillon/linux-rk/tree/rk-3.14 Best Regards, Boris
On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote: > Implement the ->enable(), ->disable() and ->is_enabled methods and remove > the PWM call in ->set_voltage_sel(). This doesn't apply, please check and resend.
Hi Mark, On Tue, 14 Jul 2015 11:50:19 +0100 Mark Brown <broonie@kernel.org> wrote: > On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote: > > Implement the ->enable(), ->disable() and ->is_enabled methods and remove > > the PWM call in ->set_voltage_sel(). > > This doesn't apply, please check and resend. This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14 and 15 were not meant to be applied without Thierry's approval (they depend on other changes in the PWM framework). I can rebase them on top of linux-next (or just on top of Linus' 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them on your regulator's for-next branch is a good idea. Thierry, Mark, let me know what you prefer. Thanks, Boris
On Tue, Jul 14, 2015 at 01:02:22PM +0200, Boris Brezillon wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote: > > > Implement the ->enable(), ->disable() and ->is_enabled methods and remove > > > the PWM call in ->set_voltage_sel(). > > This doesn't apply, please check and resend. > This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14 > and 15 were not meant to be applied without Thierry's approval (they > depend on other changes in the PWM framework). > I can rebase them on top of linux-next (or just on top of Linus' > 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them > on your regulator's for-next branch is a good idea. What is the dependency for the enable patch?
On Tue, 14 Jul 2015 12:08:19 +0100 Mark Brown <broonie@kernel.org> wrote: > On Tue, Jul 14, 2015 at 01:02:22PM +0200, Boris Brezillon wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > > On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote: > > > > Implement the ->enable(), ->disable() and ->is_enabled methods and remove > > > > the PWM call in ->set_voltage_sel(). > > > > This doesn't apply, please check and resend. > > > This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14 > > and 15 were not meant to be applied without Thierry's approval (they > > depend on other changes in the PWM framework). > > I can rebase them on top of linux-next (or just on top of Linus' > > 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them > > on your regulator's for-next branch is a good idea. > > What is the dependency for the enable patch? Oh, indeed. This patch has no dependency on the PWM changes. I'll send it on its own then. Thanks, Boris
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev, drvdata->state = selector; - ret = pwm_enable(drvdata->pwm); - if (ret) { - dev_err(&rdev->dev, "Failed to enable PWM\n"); - return ret; - } - return 0; } @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV; } +static int pwm_regulator_enable(struct regulator_dev *dev) +{ + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); + + return pwm_enable(drvdata->pwm); +} + +static int pwm_regulator_disable(struct regulator_dev *dev) +{ + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); + + pwm_disable(drvdata->pwm); + + return 0; +} + +static int pwm_regulator_is_enabled(struct regulator_dev *dev) +{ + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev); + + return pwm_is_enabled(drvdata->pwm); +} + static struct regulator_ops pwm_regulator_voltage_ops = { .set_voltage_sel = pwm_regulator_set_voltage_sel, .get_voltage_sel = pwm_regulator_get_voltage_sel, .list_voltage = pwm_regulator_list_voltage, .map_voltage = regulator_map_voltage_iterate, + .enable = pwm_regulator_enable, + .disable = pwm_regulator_disable, + .is_enabled = pwm_regulator_is_enabled, }; static struct regulator_desc pwm_regulator_desc = {
Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel(). This is particularly important for critical regulators tagged as always-on, because not claiming the PWM (and its dependencies) might lead to unpredictable behavior (like a system hang because the PWM clk is only claimed when the PWM device is enabled). Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)