Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v2 2/3] clk: vc3: Fix output clock mapping
>
> Hi Biju,
>
> On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > According to Table 3. ("Output Source") in the 5P35023 datasheet, the
> > output clock mapping should be 0=REF, 1=SE1, 2=SE2, 3=SE3, 4=DIFF1,
> > 5=DIFF2. But the code uses inverse. Fix this mapping issue.
> >
> > Suggested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver")
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> One suggestion for future improvement (which can be a separate patch)
> below...
Ok.
>
> > --- a/drivers/clk/clk-versaclock3.c
> > +++ b/drivers/clk/clk-versaclock3.c
> > @@ -119,20 +119,20 @@ enum vc3_div {
> > };
> >
> > enum vc3_clk_mux {
> > - VC3_DIFF2_MUX,
> > - VC3_DIFF1_MUX,
> > - VC3_SE3_MUX,
> > - VC3_SE2_MUX,
> > VC3_SE1_MUX,
> > + VC3_SE2_MUX,
> > + VC3_SE3_MUX,
> > + VC3_DIFF1_MUX,
> > + VC3_DIFF2_MUX,
> > };
> >
> > enum vc3_clk {
> > - VC3_DIFF2,
> > - VC3_DIFF1,
> > - VC3_SE3,
> > - VC3_SE2,
> > - VC3_SE1,
> > VC3_REF,
> > + VC3_SE1,
> > + VC3_SE2,
> > + VC3_SE3,
> > + VC3_DIFF1,
> > + VC3_DIFF2,
> > };
> >
> > struct vc3_clk_data {
>
> > @@ -1110,7 +1110,7 @@ static int vc3_probe(struct i2c_client *client)
> > name, 0, CLK_SET_RATE_PARENT, 1, 1);
> > else
> > clk_out[i] =
> devm_clk_hw_register_fixed_factor_parent_hw(dev,
> > - name, &clk_mux[i].hw,
> CLK_SET_RATE_PARENT, 1, 1);
> > + name, &clk_mux[i - 1].hw,
> > + CLK_SET_RATE_PARENT, 1, 1);
>
> This is (and was before) fragile, as it implies a strict relation between
> the vc3_clk_mux and vc3_clk enum values. To avoid accidental breakage, I
> think it would be wise to make this explicit, e.g.
>
> enum vc3_clk_mux {
> VC3_SE1_MUX = VC3_SE1 - 1,
> VC3_SE2_MUX = VC3_SE2 - 1,
> [...]
> };
Agreed.
Cheers,
Biju
>
> >
> > if (IS_ERR(clk_out[i]))
> > return PTR_ERR(clk_out[i]);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
> -- Linus Torvalds
