[RFC][PATCH] mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function

[RFC][PATCH] mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function

[Axel Lin]

The mc13xxx_reg_rmw function is doing read/modify/write bitmask operations,
thus add the lock to protect it.
Then we can remove the lock/unlock from the caller.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
Note I don't have the hardware handy for testing.
I appreciate if someone who has the device can test this patch.
Regards,
Axel

 drivers/mfd/mc13xxx-core.c                 |    8 ++++++--
 drivers/regulator/mc13892-regulator.c      |   16 +++++++---------
 drivers/regulator/mc13xxx-regulator-core.c |    6 ------
 3 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 7e4d44b..5fb0fcc 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -249,15 +249,19 @@ int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
 	int ret;
 	u32 valread;
 
+	mc13xxx_lock(mc13xxx);
 	BUG_ON(val & ~mask);
 
 	ret = mc13xxx_reg_read(mc13xxx, offset, &valread);
 	if (ret)
-		return ret;
+		goto out;
 
 	valread = (valread & ~mask) | val;
 
-	return mc13xxx_reg_write(mc13xxx, offset, valread);
+	ret = mc13xxx_reg_write(mc13xxx, offset, valread);
+out:
+	mc13xxx_unlock(mc13xxx);
+	return ret;
 }
 EXPORT_SYMBOL(mc13xxx_reg_rmw);
 
diff --git a/drivers/regulator/mc13892-regulator.c b/drivers/regulator/mc13892-regulator.c
index 1b8f739..679b315 100644
--- a/drivers/regulator/mc13892-regulator.c
+++ b/drivers/regulator/mc13892-regulator.c
@@ -449,7 +449,8 @@ static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev,
 	ret = mc13xxx_reg_read(priv->mc13xxx,
 		mc13892_regulators[id].vsel_reg, &val);
 	if (ret)
-		goto err;
+		mc13xxx_unlock(priv->mc13xxx);
+		return ret;
 
 	hi  = val & MC13892_SWITCHERS0_SWxHI;
 	if (value > 1375)
@@ -464,11 +465,10 @@ static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev,
 		value = (value - 600000) / 25000;
 
 	mask = mc13892_regulators[id].vsel_mask | MC13892_SWITCHERS0_SWxHI;
-	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].vsel_reg,
-			mask, value << mc13892_regulators[id].vsel_shift);
-err:
 	mc13xxx_unlock(priv->mc13xxx);
 
+	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].vsel_reg,
+			mask, value << mc13892_regulators[id].vsel_shift);
 	return ret;
 }
 
@@ -488,10 +488,8 @@ static int mc13892_vcam_set_mode(struct regulator_dev *rdev, unsigned int mode)
 	if (mode == REGULATOR_MODE_FAST)
 		en_val = MC13892_REGULATORMODE1_VCAMCONFIGEN;
 
-	mc13xxx_lock(priv->mc13xxx);
 	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].reg,
 		MC13892_REGULATORMODE1_VCAMCONFIGEN, en_val);
-	mc13xxx_unlock(priv->mc13xxx);
 
 	return ret;
 }
@@ -537,8 +535,10 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 
 	mc13xxx_lock(mc13892);
 	ret = mc13xxx_reg_read(mc13892, MC13892_REVISION, &val);
-	if (ret)
+	if (ret) {
+		mc13xxx_unlock(mc13892);
 		goto err_free;
+	}
 
 	/* enable switch auto mode */
 	if ((val & 0x0000FFFF) == 0x45d0) {
@@ -558,7 +558,6 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 		if (ret)
 			goto err_free;
 	}
-	mc13xxx_unlock(mc13892);
 
 	mc13892_regulators[MC13892_VCAM].desc.ops->set_mode
 		= mc13892_vcam_set_mode;
@@ -586,7 +585,6 @@ err:
 		regulator_unregister(priv->regulators[i]);
 
 err_free:
-	mc13xxx_unlock(mc13892);
 	kfree(priv);
 
 	return ret;
diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
index 2bb5de1..efe77d5 100644
--- a/drivers/regulator/mc13xxx-regulator-core.c
+++ b/drivers/regulator/mc13xxx-regulator-core.c
@@ -34,11 +34,9 @@ static int mc13xxx_regulator_enable(struct regulator_dev *rdev)
 
 	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
 
-	mc13xxx_lock(priv->mc13xxx);
 	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13xxx_regulators[id].reg,
 			mc13xxx_regulators[id].enable_bit,
 			mc13xxx_regulators[id].enable_bit);
-	mc13xxx_unlock(priv->mc13xxx);
 
 	return ret;
 }
@@ -52,10 +50,8 @@ static int mc13xxx_regulator_disable(struct regulator_dev *rdev)
 
 	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
 
-	mc13xxx_lock(priv->mc13xxx);
 	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13xxx_regulators[id].reg,
 			mc13xxx_regulators[id].enable_bit, 0);
-	mc13xxx_unlock(priv->mc13xxx);
 
 	return ret;
 }
@@ -143,11 +139,9 @@ static int mc13xxx_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
 	if (value < 0)
 		return value;
 
-	mc13xxx_lock(priv->mc13xxx);
 	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13xxx_regulators[id].vsel_reg,
 			mc13xxx_regulators[id].vsel_mask,
 			value << mc13xxx_regulators[id].vsel_shift);
-	mc13xxx_unlock(priv->mc13xxx);
 
 	return ret;
 }
-- 
1.7.1

Re: [RFC][PATCH] mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function

[Uwe Kleine-König]

Hello Axel,

On Wed, May 04, 2011 at 12:27:59AM +0800, Axel Lin wrote:
> The mc13xxx_reg_rmw function is doing read/modify/write bitmask operations,
> thus add the lock to protect it.
> Then we can remove the lock/unlock from the caller.
> 
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
> Note I don't have the hardware handy for testing.
> I appreciate if someone who has the device can test this patch.
> Regards,
> Axel
> 
>  drivers/mfd/mc13xxx-core.c                 |    8 ++++++--
>  drivers/regulator/mc13892-regulator.c      |   16 +++++++---------
>  drivers/regulator/mc13xxx-regulator-core.c |    6 ------
>  3 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index 7e4d44b..5fb0fcc 100644
[snip]
> diff --git a/drivers/regulator/mc13892-regulator.c b/drivers/regulator/mc13892-regulator.c
> index 1b8f739..679b315 100644
> --- a/drivers/regulator/mc13892-regulator.c
> +++ b/drivers/regulator/mc13892-regulator.c
> @@ -449,7 +449,8 @@ static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev,
>  	ret = mc13xxx_reg_read(priv->mc13xxx,
>  		mc13892_regulators[id].vsel_reg, &val);
>  	if (ret)
> -		goto err;
> +		mc13xxx_unlock(priv->mc13xxx);
> +		return ret;
>  
>  	hi  = val & MC13892_SWITCHERS0_SWxHI;
>  	if (value > 1375)
> @@ -464,11 +465,10 @@ static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev,
>  		value = (value - 600000) / 25000;
>  
>  	mask = mc13892_regulators[id].vsel_mask | MC13892_SWITCHERS0_SWxHI;
> -	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].vsel_reg,
> -			mask, value << mc13892_regulators[id].vsel_shift);
> -err:
>  	mc13xxx_unlock(priv->mc13xxx);
>  
> +	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].vsel_reg,
> +			mask, value << mc13892_regulators[id].vsel_shift);
I havn't looked deeply, but I guess this can have unwanted side effects
here. Before you had:

	lock()
	do(something)
	do(something, else, that, needs, rmw)
	unlock()

and you introduced an unlock()/lock() between these two do()s.

I'm not convinced this change is good, though I agree that

	lock()
	rmw(...)
	unlock()

looks ugly, but imho this can better be fixed by adding a wrapper for
that sequence if you really want.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC][PATCH] mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function

[Mark Brown]

On Tue, May 03, 2011 at 09:15:25PM +0200, Uwe Kleine-König wrote:
> On Wed, May 04, 2011 at 12:27:59AM +0800, Axel Lin wrote:

> > +	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].vsel_reg,
> > +			mask, value << mc13892_regulators[id].vsel_shift);

> I havn't looked deeply, but I guess this can have unwanted side effects
> here. Before you had:

> 	lock()
> 	do(something)
> 	do(something, else, that, needs, rmw)
> 	unlock()

> and you introduced an unlock()/lock() between these two do()s.

Glancing at the code I wasn't 100% convinced that the original read was
really needed, though I didn't look closely.

> I'm not convinced this change is good, though I agree that

> 	lock()
> 	rmw(...)
> 	unlock()

> looks ugly, but imho this can better be fixed by adding a wrapper for
> that sequence if you really want.

You could also make the rmw store the value somewhere if it's important.
Having to open code the locks everywhere is certainly annoying and error
prone.

Re: [RFC][PATCH] mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function

[Axel Lin]

> I havn't looked deeply, but I guess this can have unwanted side effects
> here. Before you had:
>
>        lock()
>        do(something)
>        do(something, else, that, needs, rmw)
>        unlock()
>
> and you introduced an unlock()/lock() between these two do()s.
>
Indeed, you are right. I cannot change it this way.

> I'm not convinced this change is good, though I agree that
>
>        lock()
>        rmw(...)
>        unlock()
>
> looks ugly, but imho this can better be fixed by adding a wrapper for
> that sequence if you really want.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>