On Tue, Oct 14, 2008 at 11:08:48AM -0600, Paul Walmsley wrote:
> Hi Felipe,
>
> so I'll put most of my comments here.
>
> On Tue, 7 Oct 2008, Felipe Balbi wrote:
>
> > Introduce a new mechanism to omap's clk implementation to
> > associate the device with its clock during platform_device
> > registration. Also gives the clock a function name (like
> > mmc_fck, uart_fck, ehci_fck, etc) so drivers won't have to
> > care about clk names any longer.
>
> Let's make the function names shorter, like "fclk" and "iclk". That
> should make them even easier to use in situations where the device name
> itself changes, e.g., "mmc"/"hsmmc" etc. Plus the linker will be able to
> merge many them together into single constant strings. For a device with
> multiple fclks like DSS, we can use "tv_fclk" also, etc.
I didn't quite get you here. The idea of mmc_fck is so that
clk_get(dev, "mmc_fck");
works fine and returns the correct clock. If we have several fck and ick
function names, how will we clk_get() the right one ??
> > diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
> > index 7bbfba2..c090f23 100644
> > --- a/arch/arm/plat-omap/clock.c
> > +++ b/arch/arm/plat-omap/clock.c
> > @@ -43,7 +43,7 @@ static struct clk_functions *arch_clock;
> > */
> > struct clk * clk_get(struct device *dev, const char *id)
> > {
> > - struct clk *p, *clk = ERR_PTR(-ENOENT);
> > + struct clk *p, *clk;
> > int idno;
> >
> > if (dev == NULL || dev->bus != &platform_bus_type)
> > @@ -53,6 +53,15 @@ struct clk * clk_get(struct device *dev, const char *id)
> >
> > mutex_lock(&clocks_mutex);
> >
> > + list_for_each_entry(clk, &clocks, node) {
> > + if (clk->function && (dev == clk->dev) &&
> > + strcmp(id, clk->function) == 0)
> > + goto found;
>
> Please avoid the gotos and use the previously-used form for returning
> success, e.g., iterate on p; if found, then assign to clk, and break.
>
> Also, is there some reason why there are two list_for_each_entry() blocks?
> Those should be merged into one.
Will do.
>
> > +
> > + if (strcmp(id, clk->name) == 0)
> > + goto found;
>
> Doesn't the following code already handle the above case?
my bad, when merging both list_for_each_entry() this will disappear.
> > +/**
>
> First, thank you for using kerneldoc. But ...
>
> > + * omap_clk_associate - associates a user to a clock so device drivers don't
> > + * have to care about clock names
>
> ... Documentation/kernel-doc-nano-HOWTO.txt requires the short function
> description to fit on one line. If you need more room, please put the
> larger description after a blank line after the last argument
> documentation line (e.g., line beginning with '@').
Will fix.
>
> > + *
>
> This blank line needs to be removed per kerneldoc - "The @argument
> descriptions must begin on the very next line ..."
>
> > + * @id: clock id as defined in arch/arm/mach-omapX/clkxxxx.h
> > + * @dev: device pointer for the clock user
> > + * @f: a function for the clock (uart_[if]ck, musb_ick, ehci_[if]ck, etc)
> > + */
> > +void __init omap_clk_associate(const char *id, struct device *dev, const char *f)
> > +{
> > + struct clk *clk = clk_get(NULL, id);
> > +
> > + if (!dev || !clk || !IS_ERR(clk_get(dev, f)))
> > + return;
>
> Please break the clk_get() test above out into its own statement, and
> clk_put() it before returning.
ok.
> There needs to be a test before these lines to ensure that some driver has
> not already associated a function with this clock, or a device with this
> clock, and to WARN_ON(1) if it has.
sounds good.
> But there seems to be a deeper problem. What happens when multiple device
> drivers want to associate to the same clock? osc_ck is the pathological
> case. Seems like you'll need a different data structure, like a list, to
> store in the struct clk.
Yeah, have to think about that, but then again, how can several users
concurrently enable and disable the same clock ?
I mean, imagine driver A clk_enable(osc_ck) and while needing that
clock, driver B clk_disable(osc_ck). That would break driver A, right ?
How is omap clk fw handling that case right now ? I'd say we should have
one user per clk and in the case of osc_ck, that would be a clk input
for generating another clk, or something like that, so driver A can
clk_enable(osc_ck_A) and yet driver B could clk_disable(osc_ck_B) and
everything still works.
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
