Hi Claudiu,
On Wed, 7 May 2025 at 14:12, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> On 05.05.2025 18:52, Geert Uytterhoeven wrote:
> > On Thu, 10 Apr 2025 at 16:06, Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> Since the sibling data is filled after the priv->clks[] array entry is
> >> populated, the first clock that is probed and has a sibling will
> >> temporarily behave as its own sibling until its actual sibling is
> >> populated. To avoid any issues, skip this clock when searching for a
> >> sibling.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/clk/renesas/rzg2l-cpg.c
> >> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> >> @@ -1324,6 +1324,9 @@ static struct mstp_clock
> >>
> >> hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> >> clk = to_mod_clock(hw);
> >> + if (clk == clock)
> >> + continue;
> >> +
> >> if (clock->off == clk->off && clock->bit == clk->bit)
> >> return clk;
> >> }
> >
> > Why not move the whole block around the call to
> > rzg2l_mod_clock_get_sibling() up instead?
> >
> > ret = devm_clk_hw_register(dev, &clock->hw);
> > if (ret) {
> > clk = ERR_PTR(ret);
> > goto fail;
> > }
> >
> > - clk = clock->hw.clk;
> > - dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> > clk_get_rate(clk));
> > - priv->clks[id] = clk;
> > -
> > if (mod->is_coupled) {
> > struct mstp_clock *sibling;
> >
> > clock->enabled = rzg2l_mod_clock_is_enabled(&clock->hw);
> > sibling = rzg2l_mod_clock_get_sibling(clock, priv);
> > if (sibling) {
> > clock->sibling = sibling;
> > sibling->sibling = clock;
> > }
> > }
> >
> > + clk = clock->hw.clk;
> > + dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> > clk_get_rate(clk));
> > + priv->clks[id] = clk;
> > +
> > return;
>
> This should work as well. I considered the proposed patch generates less
> diff. Please let me know if you prefer it addressed as you proposed.
Given you have a later patch that contains a similar check, postponing
setting priv->clks[id] looks like the better solution to me.
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
