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

[함명주]

> Hi,
> 
> On Wed, May 02, 2012 at 02:02:55PM +0900, jonghwa3.lee@xxxxxxxxxxx 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떑꿩ěr¸›y鉉싕b˛XФ푤vŘ^–)頻{.nÇ+돴ĽŠ{ą묎çzX㎍썳變}Š옽Ć zÚ&j:+v돣žŤ묎çzZ+€Ę+zf＂ˇhš닱~넮녬i˙鎬zšŽwⅱ¸?솳鈺Ú&˘)刪f뷌^j푹y§m끷@AŤa뛴˙0띠hŽĺ’i