lkml.org 
[lkml]   [2013]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] clk: add LP8788 clock driver
Date
> Just FYI most folks are simply using a macro to do the above.  E.g:
>
> #define to_lp8788_clk(_hw) container_of(_hw, struct lp8788_clk, hw)

OK, thanks! Will be fixed in the 2nd patch.

> > +
> > +static int lp8788_clk_is_enabled(struct clk_hw *hw)
> > +{
> > + struct lp8788_clk *pclk = to_lp8788_clk(hw);
> > +
> > + /*
> > + * Do not use the LP8788 register access here, unsafe locking
> problem
> > + * may occur during loading the driver. So stored varible is
> prefered.
> > + */
>
> What unsafe locking problem?

If I try to get LP8788 clock status via the remap-i2c here, then locking problem
occurs.
The 'enable_lock' is already locked and the MUTEX of REGMAP is used on reading
a register.

[ 2.692443] -------------------------------------------------------
[ 2.699066] swapper/0/1 is trying to acquire lock:
[ 2.704132] (&map->mutex){+.+...}, at: [<c0374ecc>] regmap_read+0x34/0x5c
[ 2.711486]
[ 2.711486] but task is already holding lock:
[ 2.717681] (enable_lock){......}, at: [<c047a6c8>] clk_disable_unused_subtr
ee+0x3c/0xb4
[ 2.726379]
[ 2.726379] which lock already depends on the new lock.

That's why lp8788_get_clk_status() is used on _probe().
- Read a register and store the value into local status variable.
Then local variable is used in .is_enabled().

> Also have you looked into using the new .is_prepare callback? See:
> https://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdif
> f;h=3d6ee287a3e341c88eafd0b4620b12d640b3736b;hp=30ee400614385ac49f4c9b4
> bc03d77ff8f07a61e

Not checked yet, but it looks no caller for this callback at this moment.
Will this be added inside the clk_prepare()?

> > +
> > + return pclk->enabled;
>
> Why provide a .is_enabled callback when there are no .enable
> or .disable
> callbacks?

LP8788 Clock is always enabled, so it needs to show the clock status.
However, I think no effect on working the clock operation without is_enabled().
Can I remove it?

Thanks,
Milo

\
 
 \ /
  Last update: 2013-04-02 05:01    [W:0.172 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site