[PATCH 1/1] regulator: return set_mode is same mode is requested

[PATCH 1/1] regulator: return set_mode is same mode is requested

[Sundar Iyer]

From: Sundar R Iyer <sundar.iyer@stericsson.com>

Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
 drivers/regulator/core.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 98e5d14..5054add 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1599,6 +1599,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
+	int regulator_curr_mode;
 
 	mutex_lock(&rdev->mutex);
 
@@ -1754,6 +1755,13 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 		goto out;
 	}
 
+	/* return if the same mode is requested */
+	regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
+	if (regulator_curr_mode == mode) {
+		ret = 0;
+		goto out;
+	}
+
 	/* constraints check */
 	ret = regulator_check_mode(rdev, mode);
 	if (ret < 0)
-- 
1.7.0

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Liam Girdwood]

On Mon, 2010-05-17 at 19:39 +0530, Sundar Iyer wrote:
> From: Sundar R Iyer <sundar.iyer@stericsson.com>
> 
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
> ---
>  drivers/regulator/core.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 98e5d14..5054add 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1599,6 +1599,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
>  	int ret;
> +	int regulator_curr_mode;

Shouldn't this variable be in regulator_set_mode() ?

>  
>  	mutex_lock(&rdev->mutex);
>  
> @@ -1754,6 +1755,13 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
>  		goto out;
>  	}
>  
> +	/* return if the same mode is requested */
> +	regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
> +	if (regulator_curr_mode == mode) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	/* constraints check */
>  	ret = regulator_check_mode(rdev, mode);
>  	if (ret < 0)

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Mark Brown]

On Mon, May 17, 2010 at 07:39:53PM +0530, Sundar Iyer wrote:
> From: Sundar R Iyer <sundar.iyer@stericsson.com>

The commit message reads "regulator: return set_mode is same mode is
requested".  I'm having a hard time parsing what that actually means,
you probably need a "when" in there...

> +	/* return if the same mode is requested */
> +	regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
> +	if (regulator_curr_mode == mode) {
> +		ret = 0;
> +		goto out;
> +	}
> +

This is going to oops if the regulator doesn't implement a get_mode()
operation.  

I'm also a little ambivalent on the benefit of it - if the goal is to
save I/O costs (you didn't say...) it's not clear to me that the effort
of checking the current mode is going to be a win in situations where
the mode is actually being changed a lot.

As I said in reply to your previous message the trend is away from
having any mode configration at all, with regulators being able to adapt
to their current load without any software assistance.

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Sundar R Iyer]

On Mon, 2010-05-17 at 16:44 +0200, Mark Brown wrote:
> The commit message reads "regulator: return set_mode is same mode is
> requested".  I'm having a hard time parsing what that actually means,
> you probably need a "when" in there...
> 
Oops. My bad. I messed by typoing up the commit message.

> This is going to oops if the regulator doesn't implement a get_mode()
> operation.  
Okay. I will add a sanity check for that.

> I'm also a little ambivalent on the benefit of it - if the goal is to
> save I/O costs (you didn't say...) it's not clear to me that the effort
> of checking the current mode is going to be a win in situations where
> the mode is actually being changed a lot.
Okay. I came up across this and hence the change. This is intended when
the same mode is requested! I will edit the commit message appropriately.

> 
> As I said in reply to your previous message the trend is away from
> having any mode configration at all, with regulators being able to adapt
> to their current load without any software assistance.
I posted this as this is currently supported in the tree.

Thanx for the prompt review and reply,

Regards,
Sundar

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Sundar R Iyer]

On Mon, 2010-05-17 at 16:56 +0200, Sundar R IYER wrote:
> Oops. My bad. I messed by typoing up the commit message.
> Okay. I will add a sanity check for that.
> Okay. I came up across this and hence the change. This is intended when
> the same mode is requested! I will edit the commit message appropriately.

Please find the updated patch below:

>From edab94fefd2e9087f4c27df8d352097ddda2dac0 Mon Sep 17 00:00:00 2001
From: Sundar R Iyer <sundar.iyer@stericsson.com>
Date: Fri, 14 May 2010 15:14:17 +0530
Subject: [PATCH 1/1] regulator: return set_mode when same mode is
requested

save I/O costs by returning when the same mode is
requested for the regulator

Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
 drivers/regulator/core.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 98e5d14..9cb21cd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1745,6 +1745,7 @@ int regulator_set_mode(struct regulator
*regulator, unsigned int mode)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
+	int regulator_curr_mode;
 
 	mutex_lock(&rdev->mutex);
 
@@ -1754,6 +1755,19 @@ int regulator_set_mode(struct regulator
*regulator, unsigned int mode)
 		goto out;
 	}
 
+	/* sanity check */
+	if (!rdev->desc->ops->get_mode) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* return if the same mode is requested */
+	regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
+	if (regulator_curr_mode == mode) {
+		ret = 0;
+		goto out;
+	}
+
 	/* constraints check */
 	ret = regulator_check_mode(rdev, mode);
 	if (ret < 0)
-- 
1.7.0

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Mark Brown]

On Mon, May 17, 2010 at 08:45:45PM +0530, Sundar R Iyer wrote:

> +	/* sanity check */
> +	if (!rdev->desc->ops->get_mode) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

This doesn't seem like the right error handling - if the driver has a
set_mode() you'd *expect* it to have a get_mode() but there's no need
for it to be a strict requirement.

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Sundar R Iyer]

On Mon, 2010-05-17 at 17:34 +0200, Mark Brown wrote:
> This doesn't seem like the right error handling - if the driver has a
> set_mode() you'd *expect* it to have a get_mode() but there's no need
> for it to be a strict requirement.
True. In such a case, even a valid request would be lost! So now
in the updated patch:
 - check if get_mode is present to avoid oops;
 - if get_mode is not present, proceed anyways for the request.

Here is the updated patch:

>From bad0d5eb51ef84be5b100e3dd0f5a590ea0529b6 Mon Sep 17 00:00:00 2001
From: Sundar R Iyer <sundar.iyer@stericsson.com>
Date: Fri, 14 May 2010 15:14:17 +0530
Subject: [PATCH 1/1] regulator: return set_mode when same mode is requested

save I/O costs by returning when the same mode is
requested for the regulator

Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
 drivers/regulator/core.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 98e5d14..2248087 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1745,6 +1745,7 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
+	int regulator_curr_mode;
 
 	mutex_lock(&rdev->mutex);
 
@@ -1754,6 +1755,15 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 		goto out;
 	}
 
+	/* return if the same mode is requested */
+	if (rdev->desc->ops->get_mode) {
+		regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
+		if (regulator_curr_mode == mode) {
+			ret = 0;
+			goto out;
+		}
+	}
+
 	/* constraints check */
 	ret = regulator_check_mode(rdev, mode);
 	if (ret < 0)
-- 
1.7.0

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Mark Brown]

On Mon, May 17, 2010 at 09:24:48PM +0530, Sundar R Iyer wrote:

> >From bad0d5eb51ef84be5b100e3dd0f5a590ea0529b6 Mon Sep 17 00:00:00 2001
> From: Sundar R Iyer <sundar.iyer@stericsson.com>
> Date: Fri, 14 May 2010 15:14:17 +0530
> Subject: [PATCH 1/1] regulator: return set_mode when same mode is requested
> 
> save I/O costs by returning when the same mode is
> requested for the regulator

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Liam Girdwood]

On Mon, 2010-05-17 at 21:24 +0530, Sundar R Iyer wrote:
> On Mon, 2010-05-17 at 17:34 +0200, Mark Brown wrote:
> > This doesn't seem like the right error handling - if the driver has a
> > set_mode() you'd *expect* it to have a get_mode() but there's no need
> > for it to be a strict requirement.
> True. In such a case, even a valid request would be lost! So now
> in the updated patch:
>  - check if get_mode is present to avoid oops;
>  - if get_mode is not present, proceed anyways for the request.
> 
> Here is the updated patch:
> 
> >From bad0d5eb51ef84be5b100e3dd0f5a590ea0529b6 Mon Sep 17 00:00:00 2001
> From: Sundar R Iyer <sundar.iyer@stericsson.com>
> Date: Fri, 14 May 2010 15:14:17 +0530
> Subject: [PATCH 1/1] regulator: return set_mode when same mode is requested
> 
> save I/O costs by returning when the same mode is
> requested for the regulator
> 
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
> ---
>  drivers/regulator/core.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 98e5d14..2248087 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1745,6 +1745,7 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
>  	int ret;
> +	int regulator_curr_mode;
>  
>  	mutex_lock(&rdev->mutex);
>  
> @@ -1754,6 +1755,15 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
>  		goto out;
>  	}
>  
> +	/* return if the same mode is requested */
> +	if (rdev->desc->ops->get_mode) {
> +		regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
> +		if (regulator_curr_mode == mode) {
> +			ret = 0;
> +			goto out;
> +		}
> +	}
> +
>  	/* constraints check */
>  	ret = regulator_check_mode(rdev, mode);
>  	if (ret < 0)


Applied.

Thanks

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 1/1] regulator: return set_mode is same mode is requested

[Sundar R Iyer]

On Mon, 2010-05-17 at 16:56 +0200, Sundar R IYER wrote:
> Oops. My bad. I messed by typoing up the commit message.
> Okay. I will add a sanity check for that.
> Okay. I came up across this and hence the change. This is intended when
> the same mode is requested! I will edit the commit message appropriately.

Please find the updated patch below:

>From edab94fefd2e9087f4c27df8d352097ddda2dac0 Mon Sep 17 00:00:00 2001
From: Sundar R Iyer <sundar.iyer@stericsson.com>
Date: Fri, 14 May 2010 15:14:17 +0530
Subject: [PATCH 1/1] regulator: return set_mode when same mode is
requested

save I/O costs by returning when the same mode is
requested for the regulator

Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
 drivers/regulator/core.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 98e5d14..9cb21cd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1745,6 +1745,7 @@ int regulator_set_mode(struct regulator
*regulator, unsigned int mode)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
+	int regulator_curr_mode;
 
 	mutex_lock(&rdev->mutex);
 
@@ -1754,6 +1755,19 @@ int regulator_set_mode(struct regulator
*regulator, unsigned int mode)
 		goto out;
 	}
 
+	/* sanity check */
+	if (!rdev->desc->ops->get_mode) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* return if the same mode is requested */
+	regulator_curr_mode = rdev->desc->ops->get_mode(rdev);
+	if (regulator_curr_mode == mode) {
+		ret = 0;
+		goto out;
+	}
+
 	/* constraints check */
 	ret = regulator_check_mode(rdev, mode);
 	if (ret < 0)
-- 
1.7.0