Re: [PATCHv9 05/43] CLK: TI: add DT alias clock registration mechanism

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi Matt,

On Tuesday 29 of October 2013 12:50:43 Matt Sealey wrote:
> On Fri, Oct 25, 2013 at 10:56 AM, Tero Kristo <t-kristo@ti.com> wrote:
> > Some devices require their clocks to be available with a specific
> > dev-id con-id mapping. With DT, the clocks can be found by default
> > only with their name, or alternatively through the device node of
> > the consumer. With drivers, that don't support DT fully yet, add
> > mechanism to register specific clock names.
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > Tested-by: Nishanth Menon <nm@ti.com>
> > Acked-by: Tony Lindgren <tony@atomide.com>
> 
> You guys are wonderful, by the way ;)
> 
> > +void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
> > +{
> > +       struct ti_dt_clk *c;
> > +       struct device_node *node;
> > +       struct clk *clk;
> > +       struct of_phandle_args clkspec;
> > +
> > +       for (c = oclks; c->node_name != NULL; c++) {
> > +               node = of_find_node_by_name(NULL, c->node_name);
> > +               clkspec.np = node;
> > +               clk = of_clk_get_from_provider(&clkspec);
> > +
> > +               if (!IS_ERR(clk)) {
> > +                       c->lk.clk = clk;
> > +                       clkdev_add(&c->lk);
> > +               } else {
> > +                       pr_warn("%s: failed to lookup clock node
> > %s\n",
> > +                               __func__, c->node_name);
> > +               }
> > +       }
> > +}
> 
> I have one question here, what makes this part of the patch
> TI-specific at all except the definition of the structure ti_dt_clk?
> Mapping DT clocks to generic clkdev legacy namings seems like it would
> be a necessary evil across *all* SoC platforms.

SoC platforms that are fully converted to Device Tree do not need any 
legacy clkdev namings, because they have all clkdev look-ups performed 
using data from DT (see generic clock bindings).

So this would be not "*all* SoC platforms", but instead "SoC platforms, 
which are currently undergoing conversion to DT or conversion of which is 
yet to be started".

Still this might be a good enough justification for having a generic 
solution for this problem, but I would wait with this until the real need 
on some of such platforms shows up. I don't see anything wrong in 
implementing this first as TI-specific quirk and then possibly making it 
generic if such need shows up.

By the way, we already have something like this in clk/samsung/clk.c . 
This is called aliases there and allows registering clkdev look-ups for 
particular clocks. However this is specific to our clock drivers that 
register all clocks inside a single clock privder, based on static ID 
assignment and all clocks statically listed in clock driver - see 
clk/samsung/clk-s3c64xx.c for an example.

Best regards,
Tomasz

> I would say there is a good case for this being generic and used by
> all platforms waiting for con->id/dev->id to be moved to DT-awareness,
> the structure itself is very generic within clkdev as long as the
> drivers conformed anyway (and if they didn't, none of this helps) and
> I don't think it really needs to be a TI-only thing. The next thing
> that's going to happen when another platform arrives that wants to do
> the same thing is duplication or consolidation - so it would probably
> be a good idea to make this generic to start.
> 
> While there the names could be a bit more descriptive -
> dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map,
> DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this
> is the absolute correct thing to do under all circumstances, but in
> fact this is purely for mapping the old way to the new, better way..
> in the event a brand new platform arrives, with all new drivers (or
> only compliant drivers), none of this code should be implemented for
> it and it should be presented this way.
> 
> Thanks,
> Matt Sealey <neko@bakuhatsu.net>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel