Re: [UPDATED v3][PATCH 4/7] regulator: framework core

[Liam Girdwood <lg@opensource.wolfsonmicro.com>]

On Fri, 2008-03-07 at 00:39 -0800, Andrew Morton wrote:
> On Thu, 06 Mar 2008 18:11:54 +0000 Liam Girdwood <lg@opensource.wolfsonmicro.com> wrote:
> 
> > This patch provides the regulator framework core. The core also provides a
> > sysfs interface for userspace information.
> > 
> > +/* gets the regulator for a given consumer device */
> > +static struct regulator *get_device_regulator(struct device *dev)
> > +{
> > +	struct regulator *regulator = NULL;
> > +	struct regulator_cdev *rcdev;
> > +
> > +	mutex_lock(&regulator_list_mutex);
> > +	list_for_each_entry(rcdev, &regulator_list, list) {
> > +		mutex_lock(&rcdev->mutex);
> > +		list_for_each_entry(regulator, &rcdev->consumer_list, list) {
> > +			if (regulator->dev == dev) {
> > +				mutex_unlock(&rcdev->mutex);
> > +				return regulator;
> > +			}
> > +		}
> > +		mutex_unlock(&rcdev->mutex);
> > +	}
> > +	mutex_unlock(&regulator_list_mutex);
> > +	return NULL;
> > +}
> 
> Is this really supposed to return with the list lock held?  If so, some
> comment describing the overall locking design here would be better than
> having to reverse-engineer it.
> 
> > +static inline int get_lowest_stable_voltage(struct regulator_cdev *rcdev)
> > +{
> > +	struct regulator *regulator;
> > +	int highest_uV = 0;
> > +
> > +	/* lock is held by caller */
> > +	list_for_each_entry(regulator, &rcdev->consumer_list, list) {
> > +		if (regulator->enabled && regulator->uV_required > highest_uV)
> > +			highest_uV = regulator->uV_required;
> > +	}
> > +	return highest_uV;
> > +}
> 
> Too large to inline - the compiler will do this for you if it is a benefit
> to do so.
> 
> > +/* Calculate the new optimum regulator operating mode based on the new total
> > + * consumer load. All locks held by caller */
> > +static void drms_uA_update(struct regulator_cdev *rcdev)
> > +{
> > +	struct regulator *sibling;
> > +	int current_uA = 0, output_uV, input_uV, err;
> > +	unsigned int mode;
> > +
> > +	err = regulator_check_drms(rcdev);
> > +	if (err < 0 || !rcdev->desc->ops->get_optimum_mode ||
> > +	    !rcdev->desc->ops->get_voltage || !rcdev->desc->ops->set_mode);
> > +	return;
> > +
> > +	/* get output voltage */
> > +	output_uV = rcdev->desc->ops->get_voltage(rcdev);
> > +
> > +	/* get input voltage */
> > +	if (rcdev->parent && rcdev->parent->desc->ops->get_voltage)
> > +		input_uV = rcdev->parent->desc->ops->get_voltage(rcdev->parent);
> > +	else
> > +		input_uV = rcdev->constraints->input_uV;
> > +
> > +	/* calc total requested load */
> > +	list_for_each_entry(sibling, &rcdev->consumer_list, list)
> > +	    current_uA += sibling->uA_load;
> 
> Should be indented with a tab, not spacespacespacespace

This patch releases the lock in get_device_regulator(), makes
get_lowest_stable_voltage() non inline and fixes a minor indentation
issue.

Signed-off-by: Liam Girdwood <lg@opensource.wolfsonmicro.com>
---
 drivers/regulator/reg-core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/reg-core.c b/drivers/regulator/reg-core.c
index 56367ab..e29584f 100644
--- a/drivers/regulator/reg-core.c
+++ b/drivers/regulator/reg-core.c
@@ -85,6 +85,7 @@ static struct regulator *get_device_regulator(struct device *dev)
 		list_for_each_entry(regulator, &rcdev->consumer_list, list) {
 			if (regulator->dev == dev) {
 				mutex_unlock(&rcdev->mutex);
+				mutex_unlock(&regulator_list_mutex);
 				return regulator;
 			}
 		}
@@ -336,7 +337,7 @@ struct class regulator_class = {
 };
 
 /* find the lowest stable voltage that all enabled clients can operate at */
-static inline int get_lowest_stable_voltage(struct regulator_cdev *rcdev)
+static int get_lowest_stable_voltage(struct regulator_cdev *rcdev)
 {
 	struct regulator *regulator;
 	int highest_uV = 0;
@@ -387,7 +388,7 @@ static void drms_uA_update(struct regulator_cdev *rcdev)
 
 	/* calc total requested load */
 	list_for_each_entry(sibling, &rcdev->consumer_list, list)
-	    current_uA += sibling->uA_load;
+			   current_uA += sibling->uA_load;
 
 	/* now get the optimum mode for our new total regulator load */
 	mode = rcdev->desc->ops->get_optimum_mode(rcdev, input_uV,
-- 
1.5.4.3