RE: [PATCH v3] clk: renesas: rz: Select EXTAL vs USB clock

From: Chris Brandt <Chris.Brandt@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
	Simon Horman <horms+renesas@verge.net.au>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@codeaurora.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v3] clk: renesas: rz: Select EXTAL vs USB clock
Date: Thu, 1 Sep 2016 19:41:22 +0000	[thread overview]
Message-ID: <SG2PR06MB1165E96781DE628E27DCF7AE8AE20@SG2PR06MB1165.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdXd8WxTtpiQ5rujKKVP04oEriF62K9d3iTh1zVakVtTog@mail.gmail.com>

Hi Geert,

On 9/1/2016, Geert Uytterhoeven wrote:
> > +#define MD_BOOT0(x) ((x >> 0) & 1)     /* P0_0 */
> > +#define MD_BOOT1(x) ((x >> 1) & 1)     /* P0_1 */
> > +#define MD_CLK(x)   ((x >> 2) & 1)     /* P0_2 */
>
> Good idea to use a macro for that!
> However, adding more (unused) macros may trigger more questions
> from reviewers, cfr. below ;-)

OK, I can pull the other ones out. I'm OK with that.

> > +#define MD_CLKS(x)  ((x >> 3) & 1)     /* P0_3 */
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Initialization
> >   */
> >
> > +u16 rz_cpg_read_mode_pins(void)
>
> static u16 __init rz_cpg_read_mode_pins(void)
>
> Or do you intend to make this a global function?

Ooops. I'll make it static.

> There's also MD_BOOT2 at P7_0. To read that, you have to map and read
> a different set of registers, though...

Good point. So...going back to your first comment, there is no reason for making macros or discussing mode pins that you have no plan on dealing with.

> >         if (strcmp(name, "pll") == 0) {
> > -               /* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet */
> > -               unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */
>
> I'd just assign "MD_CLK(rz_cpg_read_mode_pins())" to cpg_mode here...
>
> > -               const char *parent_name = of_clk_get_parent_name(np, cpg_mode);
> > +               unsigned int cpg_mode;
> > +               const char *parent_name;
> >
> > +               cpg_mode = MD_CLK(rz_cpg_read_mode_pins());
> > +               parent_name = of_clk_get_parent_name(np, cpg_mode);
>
>... and not bother splitting the lines, as they don't break the 80-chars-per-line limit.

But I get paid per line of code!!!   (just kidding)
I'll make the changes and resend.

Thanks for your review!

Chris