Hi Geert, Thank you for the review. On Tue, May 13, 2025 at 9:03 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > On Fri, 9 May 2025 at 18:01, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > For module clocks whose parent mux may select an external source, bypass > > the normal monitor (CLK_MON) register check when the external clock is > > active. Introduce a new `ext_clk_mux_index` in `struct rzv2h_mod_clk` and > > `struct mod_clock`, and detect the current mux index in > > `rzv2h_mod_clock_is_enabled()` to disable monitoring if it matches the > > external source index. > > > > Provide the `DEF_MOD_MUX_EXTERNAL()` macro for declaring external-source > > module clocks, and populate the `ext_clk_mux_index` field in > > `rzv2h_cpg_register_mod_clk()`. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > --- > > v3->v4: > > - Dropped external_clk_mux_index and external_clk and introduced > > ext_clk_mux_index. > > - Updated DEF_MOD_*() macros to include ext_clk_mux_index. > > - Added a new helper function `rzv2h_clk_mux_to_index()` to get the > > current mux index. > > - Dropped IS_ERR() check for parent_clk in `rzv2h_clk_mux_to_index()`. > > - Updated commit description to clarify the purpose of the patch. > > Thanks for the update! > > > --- a/drivers/clk/renesas/rzv2h-cpg.c > > +++ b/drivers/clk/renesas/rzv2h-cpg.c > > @@ -563,14 +565,38 @@ static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv, > > spin_unlock_irqrestore(&priv->rmw_lock, flags); > > } > > > > +static int rzv2h_clk_mux_to_index(struct clk_hw *hw) > > Renaming to rzv2h_parent_clk_mux_to_index(), as it operates on the parent... > Agreed, it makes sense. > > +{ > > + struct clk_hw *parent_hw; > > + struct clk *parent_clk; > > + struct clk_mux *mux; > > + u32 val; > > + > > + /* This will always succeed, so no need to check for IS_ERR() */ > > + parent_clk = clk_get_parent(hw->clk); > > + > > + parent_hw = __clk_get_hw(parent_clk); > > + mux = to_clk_mux(parent_hw); > > + > > + val = readl(mux->reg) >> mux->shift; > > + val &= mux->mask; > > + return clk_mux_val_to_index(parent_hw, mux->table, 0, val); > > +} > > + > > static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw) > > { > > struct mod_clock *clock = to_mod_clock(hw); > > struct rzv2h_cpg_priv *priv = clock->priv; > > + int mon_index = clock->mon_index; > > u32 bitmask; > > u32 offset; > > > > - if (clock->mon_index >= 0) { > > + if (clock->ext_clk_mux_index >= 0) { > > + if (rzv2h_clk_mux_to_index(hw) == clock->ext_clk_mux_index) > > Collapsing into a single if-statement... > Thanks, makes sense. > > + mon_index = -1; > > + } > > + > > + if (mon_index >= 0) { > > offset = GET_CLK_MON_OFFSET(clock->mon_index); > > Dropping "clock->"... > Thanks, makes sense. Cheers, Prabhakar > > bitmask = BIT(clock->mon_bit); > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > i.e. will queue in renesas-clk for v6.17 with the above changes. > > 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 >