Discussion:
[PATCH 1/1] regulator: return set_mode is same mode is requested
(too old to reply)
Liam Girdwood
2010-05-17 14:35:29 UTC
Permalink
---
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
Sundar Iyer
2010-05-17 14:09:53 UTC
Permalink
From: Sundar R Iyer <***@stericsson.com>

Cc: Liam Girdwood <***@slimlogic.co.uk>
Cc: Mark Brown <***@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <***@stericsson.com>
Signed-off-by: Sundar R 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
Mark Brown
2010-05-17 14:44:13 UTC
Permalink
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.
Sundar R Iyer
2010-05-17 14:56:54 UTC
Permalink
Post by Mark Brown
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.
Post by Mark Brown
This is going to oops if the regulator doesn't implement a get_mode()
operation.
Okay. I will add a sanity check for that.
Post by Mark Brown
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.
Post by Mark Brown
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
Sundar R Iyer
2010-05-17 15:18:03 UTC
Permalink
Post by Sundar R Iyer
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.
From edab94fefd2e9087f4c27df8d352097ddda2dac0 Mon Sep 17 00:00:00 2001
From: Sundar R 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 <***@slimlogic.co.uk>
Cc: Mark Brown <***@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <***@stericsson.com>
Signed-off-by: Sundar R 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
Sundar R Iyer
2010-05-17 15:15:45 UTC
Permalink
Post by Sundar R Iyer
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.
From edab94fefd2e9087f4c27df8d352097ddda2dac0 Mon Sep 17 00:00:00 2001
From: Sundar R 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 <***@slimlogic.co.uk>
Cc: Mark Brown <***@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <***@stericsson.com>
Signed-off-by: Sundar R 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
Mark Brown
2010-05-17 15:34:32 UTC
Permalink
Post by Sundar R Iyer
+ /* 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.
Sundar R Iyer
2010-05-17 15:54:48 UTC
Permalink
Post by Mark Brown
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.
Post by Mark Brown
From bad0d5eb51ef84be5b100e3dd0f5a590ea0529b6 Mon Sep 17 00:00:00 2001
From: Sundar R 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 <***@slimlogic.co.uk>
Cc: Mark Brown <***@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <***@stericsson.com>
Signed-off-by: Sundar R 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
Mark Brown
2010-05-17 16:08:15 UTC
Permalink
Post by Sundar R Iyer
Post by Mark Brown
From bad0d5eb51ef84be5b100e3dd0f5a590ea0529b6 Mon Sep 17 00:00:00 2001
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 <***@opensource.wolfsonmicro.com>
Liam Girdwood
2010-05-17 19:44:19 UTC
Permalink
Post by Sundar R Iyer
Post by Mark Brown
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
- check if get_mode is present to avoid oops;
- if get_mode is not present, proceed anyways for the request.
Post by Mark Brown
From bad0d5eb51ef84be5b100e3dd0f5a590ea0529b6 Mon Sep 17 00:00:00 2001
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
---
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
Continue reading on narkive:
Loading...