Re: [PATCH 1/8] sta2x11-mfd : add apb-soc regs driver and factor out common code

[Davide Ciminaghi <ciminaghi@gnudd.com>]

On Wed, Sep 26, 2012 at 05:49:33PM +0100, Mark Brown wrote:
> On Wed, Sep 26, 2012 at 06:31:45PM +0200, Davide Ciminaghi wrote:
> 
> > Oh, and there's another problem (I'm looking at the code right now, I had
> > forgotten about this): the clock framework also asks for a spinlock_t *.
> > Regmap has its own spinlock (or mutex), and I don't think it would be a
> > good idea trying to export it.
> 
> Why is this a problem?  Nested spinlocks are perfectly fine.

after some cups of coffee, I tried thinking about this subject again, and
here's what I found:

as far as I know, nested locks are fine provided that you always take them in
the same order and release them in the opposite order (lock A, lock B,
unlock B, unlock A). So my conclusion is that nested spinlocks require
potential regmap users of sta2x11 registers to take the sta2x11-mfd spinlock
first. The pattern would be (sctl registers for instance):

spinlock_t *mfd_lock;
void *addr;

/* 
   Read sta2x11-mfd spinlock address for sctl registers and put it into
   mfd_lock
*/
sta2x11_mfd_get_regs_data(pdev, sta2x11_sctl, &addr, &mfd_lock);

......

/* Take the outer lock */
spin_lock(mfd_lock);
/* 
   Take the inner lock, do whatever you need to do and release the inner lock
*/
regmap_...();
/* Release the outer lock */
spin_unlock(mfd_lock);

The other way around (taking the regmap spinlock as the "outer" one)
would imply at least exporting the regmap spinlock/mutex address or making the
regmap.lock/unlock function pointers public somehow, not
to mention the fact that, for instance, the clock framework helpers should be
modified to deal with nested locking.

If the above is correct, I'm not sure we'd gain a lot from regmap,
because we'd still need some sta2x11 extra code around regmap calls.

Maybe there's another solution: what about adding a couple of function
pointers (lock, unlock) to struct regmap_config ? If they're set to NULL,
everything works as usual. If they're not NULL, regmap uses such functions
to do locking and unlocking instead of the usual ones (and regmap_bus.fast_io
is ignored). I think this wouldn't hurt existing regmap users and allow
sta2x11-mfd to have just one spinlock for everything, no need for nested
locking this way.

Thanks and regards
Davide