Re: [RFC][PATCH] mfd: mc13xxx-core: put mutex lock down tomc13xxx_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/