[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+V-a8ubw=1JdnH-Sr0xj222e0-+Q97KYAYqUPMVU4N3siL5jw@mail.gmail.com>
Date: Fri, 27 Jun 2025 21:33:43 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Magnus Damm <magnus.damm@...il.com>, linux-renesas-soc@...r.kernel.org,
linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 4/6] clk: renesas: rzv2h: Add fixed-factor module clocks
with status reporting
Hi Geert,
Thank you for the review.
On Thu, Jun 26, 2025 at 2:22 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 24 Jun 2025 at 19:30, Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Add support for fixed-factor module clocks that can report their enable
> > status through the module status monitor. Introduce a new clock type,
> > CLK_TYPE_FF_MOD_STATUS, and define the associated structure,
> > rzv2h_ff_mod_status_clk, to manage these clocks.
> >
> > Implement the .is_enabled callback by reading the module status register
> > using monitor index and bit definitions. Provide a helper macro,
> > DEF_FIXED_MOD_STATUS, to simplify the definition of such clocks.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Thanks for your patch!
>
> One early review comment below...
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > +static struct clk_ops rzv2h_clk_ff_mod_status_ops;
>
> This is an empty block of 200 bytes, consuming memory even when running
> on a different platform.
>
Agreed.
> > +static struct clk * __init
> > +rzv2h_cpg_fixed_mod_status_clk_register(const struct cpg_core_clk *core,
> > + struct rzv2h_cpg_priv *priv)
> > +{
> > + struct rzv2h_ff_mod_status_clk *clk_hw_data;
> > + struct clk_init_data init = { };
> > + struct clk_fixed_factor *fix;
> > + const struct clk *parent;
> > + const char *parent_name;
> > + int ret;
> > +
> > + WARN_DEBUG(core->parent >= priv->num_core_clks);
> > + parent = priv->clks[core->parent];
> > + if (IS_ERR(parent))
> > + return ERR_CAST(parent);
> > +
> > + parent_name = __clk_get_name(parent);
> > + parent = priv->clks[core->parent];
> > + if (IS_ERR(parent))
> > + return ERR_CAST(parent);
> > +
> > + clk_hw_data = devm_kzalloc(priv->dev, sizeof(*clk_hw_data), GFP_KERNEL);
> > + if (!clk_hw_data)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + clk_hw_data->priv = priv;
> > + clk_hw_data->conf = core->cfg.fixed_mod;
> > +
> > + rzv2h_clk_ff_mod_status_ops = clk_fixed_factor_ops;
>
> This overwrites rzv2h_clk_ff_mod_status_ops in every call (currently
> there is only one).
>
Good point.
> > + rzv2h_clk_ff_mod_status_ops.is_enabled = rzv2h_clk_ff_mod_status_is_enabled;
>
> If there would be multiple calls, there is a short time window where
> rzv2h_clk_ff_mod_status_ops.is_enabled is NULL, possibly affecting
> already-registered clocks of the same type.
>
Yes agreed.
> Hence I think you better store rzv2h_clk_ff_mod_status_ops inside
> rzv2h_cpg_priv (so it is allocated dynamically), and initialize it from
> rzv2h_cpg_probe (so it is initialized once).
>
Sure, I'll update as above, that is allocate only when needed (and only once).
Cheers,
Prabhakar
Powered by blists - more mailing lists