Re: [PATCH 4/6] clk: renesas: rzg2l: Add support for RZ/G3L DSI mux

[Geert Uytterhoeven]

Hi Biju,

On Fri, 19 Jun 2026 at 18:40, Biju <biju.das.au@xxxxxxxxx> wrote:
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Add support for RZ/G3L DSI mux that supports 2 duty cycles.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

More comments...

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -120,6 +120,11 @@
>  #define RZG3L_PLL7_FSTD_DIV_MR_MIN     (8 * MEGA)
>  #define RZG3L_PLL7_FSTD_DIV_MR_MAX     (16 * MEGA)
>
> +#define CPG_PLLDSI_SMUX_LVDS_DUTY_NUM          4
> +#define CPG_PLLDSI_SMUX_LVDS_DUTY_DEN          7
> +#define CPG_PLLDSI_SMUX_DSI_RGB_DUTY_NUM       1
> +#define CPG_PLLDSI_SMUX_DSI_RGB_DUTY_DEN       2

I am wondering if these macros are worthwhile, as IMO they make it
harder to understand the code.

> +
>  /**
>   * struct clk_hw_data - clock hardware data
>   * @hw: clock hw

> +static int rzg3l_cpg_dsi_smux_get_duty_cycle(struct clk_hw *hw,
> +                                            struct clk_duty *duty)
> +{
> +       u8 parent = clk_mux_ops.get_parent(hw);
> +
> +       /*
> +        * CDIV7_DSIx_CLK - LVDS path (div7) - duty 4/7.
> +        * CSDIV_DSIx - DSI/RGB path (csdiv) - duty 1/2.
> +        */
> +       if (parent == 0) {
> +               duty->num = CPG_PLLDSI_SMUX_LVDS_DUTY_NUM;
> +               duty->den = CPG_PLLDSI_SMUX_LVDS_DUTY_DEN;

The 4/7 duty cycle is due to the division by 7 between M2 and M2_DIV7.
Perhaps it should be set by that divider clock instead?

> +       } else {
> +               duty->num = CPG_PLLDSI_SMUX_DSI_RGB_DUTY_NUM;
> +               duty->den = CPG_PLLDSI_SMUX_DSI_RGB_DUTY_DEN;
> +       }
> +
> +       return 0;
> +}


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