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>
> Closes: https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0Y2ZpLCMNg@xxxxxxxxxxxxxx/
> 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...
> --- 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,
[...]
};
>
> 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@xxxxxxxxxxxxxx
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
