[RFC] Regulator: Possibility of passing notifications of non alarm events.

[RFC] Regulator: Possibility of passing notifications of non alarm events.

[Jonathan Cameron]

Dear All,

I am in the process of writing an hwmon driver for the Sensiron SHT15 humidity
sensor.

In common (I assume) with lots of other devices, the formula for converting
the outputs of this chip to real units involves the supply voltage.
Unfortunately the chip doesn't itself have any means of measuring this.
Obviously this can be supplied via a board config if it is constant.

As I have it connected to a DA9030 pmic I can register it as a consumer
and hence query what the pmic thinks it is supplying.

Unfortunately this regulator is shared with a number of other chips and so
there is no guarantee that this voltage won't be changed (within the bounds
supported by the SHT15).  I'd like to avoid querying the regulator voltage
every time I do a conversion.

As the regulator framework already allows you to register to receive events
such as under voltage detections, it seems to me that it would also make sense
to allow registrations of handlers for events that occur during normal
operation such as voltage changes.

What do people think to adding this functionality?

Thanks

--
Jonathan Cameron

Re: [RFC] Regulator: Possibility of passing notifications of non alarm events.

[Liam Girdwood]

On Sat, 2009-01-03 at 21:11 +0000, Jonathan Cameron wrote:
> Dear All,
> 
> I am in the process of writing an hwmon driver for the Sensiron SHT15 humidity
> sensor.
> 
> In common (I assume) with lots of other devices, the formula for converting
> the outputs of this chip to real units involves the supply voltage.
> Unfortunately the chip doesn't itself have any means of measuring this.
> Obviously this can be supplied via a board config if it is constant.
> 
> As I have it connected to a DA9030 pmic I can register it as a consumer
> and hence query what the pmic thinks it is supplying.
> 
> Unfortunately this regulator is shared with a number of other chips and so
> there is no guarantee that this voltage won't be changed (within the bounds
> supported by the SHT15).  I'd like to avoid querying the regulator voltage
> every time I do a conversion.
> 
> As the regulator framework already allows you to register to receive events
> such as under voltage detections, it seems to me that it would also make sense
> to allow registrations of handlers for events that occur during normal
> operation such as voltage changes.
> 
> What do people think to adding this functionality?

Sounds fine. I assume you have a patch in development ;)

Thanks

Liam

[RFC] Regulator: Add a voltage changed event to notify consumers

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>
Regulator: Add a voltage changed event to notify consumers
that care when another device changes the regulator voltage.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
If people are happy with this I'll post a follow up to update the
documentation to reflect this additional event.

This is pretty simple and very similar to the way the forced
disable event is handled. 

Worth noting is that (along with other events) the notifier
blocks are called with the regulator's lock held so any
attempt by consumers to update their voltage must be done
via a scheduled update call rather than within the callback
it self. The documentation will emphasize this.

Example of usage is the sht15 driver posted to lm-sensors where
the temperature measurement is dependent on the supply voltage
and hence needs to know if this has changed.

 drivers/regulator/core.c           |   10 ++++++----
 include/linux/regulator/consumer.h |    2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f511a40..9fec166 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1231,18 +1231,20 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 	/* sanity check */
 	if (!rdev->desc->ops->set_voltage) {
 		ret = -EINVAL;
-		goto out;
+		goto out_unlock;
 	}
 
 	/* constraints check */
 	ret = regulator_check_voltage(rdev, &min_uV, &max_uV);
 	if (ret < 0)
-		goto out;
+		goto out_unlock;
 	regulator->min_uV = min_uV;
 	regulator->max_uV = max_uV;
 	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV);
-
-out:
+	mutex_unlock(&rdev->mutex);
+	_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE, NULL);
+	return 0;
+out_unlock:
 	mutex_unlock(&rdev->mutex);
 	return ret;
 }
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 801bf77..6107a70 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -88,6 +88,7 @@
  * FAIL           Regulator output has failed.
  * OVER_TEMP      Regulator over temp.
  * FORCE_DISABLE  Regulator shut down by software.
+ * VOLTAGE_CHANGE Regulator voltage changed.	
  *
  * NOTE: These events can be OR'ed together when passed into handler.
  */
@@ -98,6 +99,7 @@
 #define REGULATOR_EVENT_FAIL			0x08
 #define REGULATOR_EVENT_OVER_TEMP		0x10
 #define REGULATOR_EVENT_FORCE_DISABLE		0x20
+#define REGULATOR_EVENT_VOLTAGE_CHANGE		0x40
 
 struct regulator;
 

Re: [RFC] Regulator: Add a voltage changed event to notify consumers

[Mark Brown]

On Sun, Jan 18, 2009 at 06:47:25PM +0000, Jonathan Cameron wrote:

> -out:
> +	mutex_unlock(&rdev->mutex);
> +	_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE, NULL);
> +	return 0;
> +out_unlock:

It'd be nice if we could modify _notifier_call_chain() to push the 
locking out a bit so we don't need to drop the lock before calling the
notifier.  On the other hand, for anything that isn't memory mapped or
GPIO controlled (most regulators are in this category) the cost of the
I/O is going to make this a non-issue.

Re: [RFC] Regulator: Add a voltage changed event to notify consumers

[Jonathan Cameron]

Mark Brown wrote:
> On Sun, Jan 18, 2009 at 06:47:25PM +0000, Jonathan Cameron wrote:
> 
>> -out:
>> +	mutex_unlock(&rdev->mutex);
>> +	_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE, NULL);
>> +	return 0;
>> +out_unlock:
> 
> It'd be nice if we could modify _notifier_call_chain() to push the 
> locking out a bit so we don't need to drop the lock before calling the
> notifier.  On the other hand, for anything that isn't memory mapped or
> GPIO controlled (most regulators are in this category) the cost of the
> I/O is going to make this a non-issue.
Agreed.  On that note, isn't any call to regulator_force_disable
currently going to deadlock? (lock held in regulator_force_disable,
then re-called in _notifier_call_chain.)

I'll have a look into moving the locks out of _notifier_call_chain.

Jonathan

Re: [RFC] Regulator: Add a voltage changed event to notify consumers

[Jonathan Cameron]

Jonathan Cameron wrote:
> Mark Brown wrote:
>> On Sun, Jan 18, 2009 at 06:47:25PM +0000, Jonathan Cameron wrote:
>>
>>> -out:
>>> +	mutex_unlock(&rdev->mutex);
>>> +	_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE, NULL);
>>> +	return 0;
>>> +out_unlock:
>> It'd be nice if we could modify _notifier_call_chain() to push the 
>> locking out a bit so we don't need to drop the lock before calling the
>> notifier.  On the other hand, for anything that isn't memory mapped or
>> GPIO controlled (most regulators are in this category) the cost of the
>> I/O is going to make this a non-issue.
> Agreed.  On that note, isn't any call to regulator_force_disable
> currently going to deadlock? (lock held in regulator_force_disable,
> then re-called in _notifier_call_chain.)
> 
> I'll have a look into moving the locks out of _notifier_call_chain.
Having had a quick look at this, it comes down to a question of
whether we want to hold the lock on one regulator whilst notifying
any regulators it supplies.

I personally can't see that this would be a problem, but it has definitely
been structured to avoid doing so.

Trying to come up with scenarios that may make this a problem:

Parent notifies child of a voltage change. This change results in 
some complex problem (not covered by constraints - I'm stretching here)
that in turn causes a the child regulator to request a forced disable
from the parent and causes deadlock.

Can anyone come up with a non contrived reason not to move constraints clean
out of _notifier_call_chain and rely on caller holding the lock?
Clearly this also requires applying locks to child regulators in 
the loop at the end of _notifier_call_chain.

Next email contains a patch combing this change with the voltage
notification patch.

Cheers,

Jonathan

[RFC] Regulator: Push lock out of _notifier_call_chain + add voltage change event.

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>
Regulator: Push lock out of _notifier_call_chain and into caller functions
(side effect of fixing deadlock in regulator_force_disable)
+ Add a voltage changed event.
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>

---
Follow up to Mark Brown's suggestion of moving the regulator lock out
of the notifier code.  This avoids cases where it was previously necessary
to unlock the regulator before notification occured.

Any scenarios where this will cause trouble?

I'll do a documentation patch when we've pinned down how to do this.
+ can break the patch into one for pushing the lock out and one for
adding the event.

 drivers/regulator/core.c             |   15 ++++++++++-----
 drivers/regulator/wm8350-regulator.c |    2 ++
 drivers/regulator/wm8400-regulator.c |    2 +-
 include/linux/regulator/consumer.h   |    2 ++
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f511a40..2a8a294 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1243,6 +1243,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV);
 
 out:
+	_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE, NULL);
 	mutex_unlock(&rdev->mutex);
 	return ret;
 }
@@ -1543,20 +1544,23 @@ int regulator_unregister_notifier(struct regulator *regulator,
 }
 EXPORT_SYMBOL_GPL(regulator_unregister_notifier);
 
-/* notify regulator consumers and downstream regulator consumers */
+/* notify regulator consumers and downstream regulator consumers.
+ * Note mutex must be held by caller.
+ */
 static void _notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
 	struct regulator_dev *_rdev;
 
 	/* call rdev chain first */
-	mutex_lock(&rdev->mutex);
 	blocking_notifier_call_chain(&rdev->notifier, event, NULL);
-	mutex_unlock(&rdev->mutex);
 
 	/* now notify regulator we supply */
-	list_for_each_entry(_rdev, &rdev->supply_list, slist)
-		_notifier_call_chain(_rdev, event, data);
+	list_for_each_entry(_rdev, &rdev->supply_list, slist) {
+	  mutex_lock(&_rdev->mutex);
+	  _notifier_call_chain(_rdev, event, data);
+	  mutex_unlock(&_rdev->mutex);
+	}
 }
 
 /**
@@ -1703,6 +1707,7 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
  *
  * Called by regulator drivers to notify clients a regulator event has
  * occurred. We also notify regulator clients downstream.
+ * Note lock must be held by caller.
  */
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
diff --git a/drivers/regulator/wm8350-regulator.c b/drivers/regulator/wm8350-regulator.c
index 7aa3524..c975664 100644
--- a/drivers/regulator/wm8350-regulator.c
+++ b/drivers/regulator/wm8350-regulator.c
@@ -1293,6 +1293,7 @@ static void pmic_uv_handler(struct wm8350 *wm8350, int irq, void *data)
 {
 	struct regulator_dev *rdev = (struct regulator_dev *)data;
 
+	mutex_lock(&rdev->mutex);
 	if (irq == WM8350_IRQ_CS1 || irq == WM8350_IRQ_CS2)
 		regulator_notifier_call_chain(rdev,
 					      REGULATOR_EVENT_REGULATION_OUT,
@@ -1301,6 +1302,7 @@ static void pmic_uv_handler(struct wm8350 *wm8350, int irq, void *data)
 		regulator_notifier_call_chain(rdev,
 					      REGULATOR_EVENT_UNDER_VOLTAGE,
 					      wm8350);
+	mutex_unlock(&rdev->mutex);
 }
 
 static int wm8350_regulator_probe(struct platform_device *pdev)
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 801bf77..6107a70 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -88,6 +88,7 @@
  * FAIL           Regulator output has failed.
  * OVER_TEMP      Regulator over temp.
  * FORCE_DISABLE  Regulator shut down by software.
+ * VOLTAGE_CHANGE Regulator voltage changed.	
  *
  * NOTE: These events can be OR'ed together when passed into handler.
  */
@@ -98,6 +99,7 @@
 #define REGULATOR_EVENT_FAIL			0x08
 #define REGULATOR_EVENT_OVER_TEMP		0x10
 #define REGULATOR_EVENT_FORCE_DISABLE		0x20
+#define REGULATOR_EVENT_VOLTAGE_CHANGE		0x40
 
 struct regulator;

Re: [RFC] Regulator: Push lock out of _notifier_call_chain + add voltage change event.

[Liam Girdwood]

On Mon, 2009-01-19 at 18:20 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <jic23@cam.ac.uk>
> Regulator: Push lock out of _notifier_call_chain and into caller functions
> (side effect of fixing deadlock in regulator_force_disable)
> + Add a voltage changed event.
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> ---
> Follow up to Mark Brown's suggestion of moving the regulator lock out
> of the notifier code.  This avoids cases where it was previously necessary
> to unlock the regulator before notification occured.
> 
> Any scenarios where this will cause trouble?
> 
> I'll do a documentation patch when we've pinned down how to do this.
> + can break the patch into one for pushing the lock out and one for
> adding the event.
> 
>  drivers/regulator/core.c             |   15 ++++++++++-----
>  drivers/regulator/wm8350-regulator.c |    2 ++
>  drivers/regulator/wm8400-regulator.c |    2 +-
>  include/linux/regulator/consumer.h   |    2 ++
>  4 files changed, 15 insertions(+), 6 deletions(-)
> 

Applied and fixed trailing white space.

Thanks

Liam