diff mbox

[RFC,14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

Message ID 1435738921-25027-15-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON July 1, 2015, 8:22 a.m. UTC
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(-)

Comments

Heiko Stübner July 1, 2015, 11:58 a.m. UTC | #1
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
Boris BREZILLON July 1, 2015, 12:05 p.m. UTC | #2
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
Heiko Stübner July 1, 2015, 12:08 p.m. UTC | #3
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
Boris BREZILLON July 1, 2015, 12:19 p.m. UTC | #4
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
Mark Brown July 14, 2015, 10:50 a.m. UTC | #5
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.
Boris BREZILLON July 14, 2015, 11:02 a.m. UTC | #6
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
Mark Brown July 14, 2015, 11:08 a.m. UTC | #7
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?
Boris BREZILLON July 14, 2015, 11:16 a.m. UTC | #8
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 mbox

Patch

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 = {