On Fri, 2 Jul 2010, Magnus Damm wrote:
> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
> <g.liakhovetski@xxxxxx> wrote:
> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
> > other clocks.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>
> Hi Guennadi,
>
> Thanks for your work on this.
>
> > diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
> > index 26521a7..21cb629 100644
> > --- a/arch/arm/mach-shmobile/clock-sh7372.c
> > +++ b/arch/arm/mach-shmobile/clock-sh7372.c
> > @@ -50,6 +50,12 @@
> > #define SMSTPCR3 0xe615013c
> > #define SMSTPCR4 0xe6150140
> >
> > +/* Fixed 27 MHz video clock from DV_CLKI pin */
> > +static struct clk dv_clki_clk = {
> > + .name = "dv_clki",
> > + .rate = 27000000,
> > +};
>
> Hm, 27MHz is a board property, not a cpu property.
Well, yes, but I think somewhere something suggested, that 27MHz is what
you're actually supposed to provide there. Otherwise your board can always
just call clk_set_rate(), right? Or what's the preferred way to let
platforms set up clocks?
> Also, you don't want to use "name" in struct clk. clkdev is used for
> lookup these days.
Right, it is not needed, thanks.
> > /* PLLC2 */
> > +
> > +/* Indices are important - they are the actual src selecting values */
> > +static struct clk *pllc2_parent[] = {
> > + [0] = &extal1_div2_clk,
> > + [1] = &extal2_div2_clk,
> > + [2] = &dv_clki_div2_clk,
> > + [3] = NULL, /* extal2_div4 not implemented yet*/
> > +};
>
> Why not implemented yet?
Because it's not used yet. Should I implement it anyway?
> > + /*
> > + * TODO: If the PLL is off, mult should be == 1, but the clock must be
> > + * stopped during re-parenting, a better solution to this conflict
> > + * should be found.
> > + */
> > + mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
>
> Yes, this needs to be fixed.
You mean now or in the future? If now - I don't see a reasonable fix so
far... Do you?
> > +enum { DIV6_HDMI, DIV6_REPARENT_NR };
> > +
> > +/* Indices are important - they are the actual src selecting values */
> > +static struct clk *hdmi_parent[] = {
> > + [0] = &pllc1_div2_clk,
> > + [1] = &pllc2_clk,
> > + [2] = &dv_clki_clk,
> > + [3] = NULL, /* pllc2_div4 not implemented yet */
> > +};
> > +
> > +static struct clk div6_reparent_clks[DIV6_REPARENT_NR] = {
> > + [DIV6_HDMI] = SH_CLK_DIV6_EXT(&pllc1_div2_clk, HDMICKCR, 0,
> > + hdmi_parent, ARRAY_SIZE(hdmi_parent), 6, 2),
> > +};
>
> This part looks nice and clean IMO. Thanks!
Thanks for the review!
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
