Re: Re: [PATCH] MFD : add MAX77686 mfd driver

[함명주 <myungjoo.ham@samsung.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 1949 bytes --]

> Hi,
> 
> On Wed, May 02, 2012 at 02:02:55PM +0900, jonghwa3.lee@samsung.com wrote:
> > On 2012-04-30 18:17, Andi Shyti wrote:
> > > Hi,
> > > 
> > >> + mutex_lock(&max77686->iolock);
> > >> + ret = i2c_smbus_read_i2c_block_data(i2c, reg, count, buf);
> > >> + mutex_unlock(&max77686->iolock);
> > > 
> > > Is it relly necessay to lock whenever you read/write from/to the
> > > i2c bus? Considering also that these are exported function,
> > > someone else may lock here before, so we can have a double
> > > locking on the same mutex.
> > 
> > These exported functions will be used in 77686 area only, so there is no
> > overlap locking.
> 
> OK... I think this could be a reason more to not over-use mutexes :)
> 
> When you call i2c_smbus_* functions you are not accessing to any
> private data, all the new data is allocated in a new area. The
> smbus_xfer function should take care by himself that the global
> data are locked correctly. If not, is not up to your driver to do
> it.
> If, instead, you are taking care about the concurrency in the
> bus, this should be somehow managed by the chip itself.
> In my opinion you are abusing a bit of mutex_lock/unlock.
> 
> Andi
> 
> P.S. copied and paste your reply at the bottom of my previous
> comment.

I expect MAX77686-PMIC(Regulator) driver will be using update_reg() heavily. That function requires mutexing such contexts to work correctly. You won't get correct update without a mutex as it will read a register and write to a register not atomically.

Without this mutex, updating a register (i.e., update the third bit to 1) can be disasterous with regulators.


Cheers!
MyungJoo.

> 
> 
> 


--

MyungJoo Ham (ÇÔ¸íÁÖ), PHD

System S/W Lab, S/W Platform Team, Software Center
Samsung Electronics
Cell: +82-10-6714-2858



 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥