mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function
RFC PATCH 1/1 mfd: mc13xxx-core: put mutex lock down to mc13xxx_reg_rmw function
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.
---
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(-)
drivers/mfd/mc13xxx-core.c +6 -2
@@ -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);
drivers/regulator/mc13892-regulator.c +7 -9
@@ -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;
drivers/regulator/mc13xxx-regulator-core.c +0 -6
@@ -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; }
Signature · 1 line
1.7.1
Hello Axel,
Quoted 18 lines · Axel Lin
The
mc13xxx_reg_rmwfunction 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 <[email protected]>---
Note I don't have the hardware handy for testing.
I appreciate if someone who has the device can test this patch. Regards,
Axeldrivers/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(-)drivers/mfd/mc13xxx-core.c +0
-0
[snip]
Quoted 15 lines
drivers/regulator/mc13892-regulator.c +2
-1@@ -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 )
drivers/regulator/mc13892-regulator.c static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev, · line 471
Quoted 10 lines
drivers/regulator/mc13892-regulator.c +2
-3@@ -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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
Quoted 8 lines · Uwe Kleine-König
drivers/regulator/mc13892-regulator.c static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev, · line 471
Quoted 2 lines · Axel Lin
drivers/regulator/mc13892-regulator.c +2
-0+ 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:
Quoted 6 lines
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.
Quoted 8 lines
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.
Quoted 10 lines
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.
Quoted 16 lines
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/ |