Message-ID: <f6f558a0daafa454c02fe29cd904cfcd.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <0e77bf23-b359-9884-6a8c-368e31d718a4@starfivetech.com>
References: <20230120024445.244345-1-xingyu.wu@starfivetech.com> <20230120024445.244345-4-xingyu.wu@starfivetech.com> <5bb5263d26b157548d7ba39f80989c69.sboyd@kernel.org> <0e77bf23-b359-9884-6a8c-368e31d718a4@starfivetech.com>
Subject: Re: [PATCH v1 03/11] clk: starfive: Add StarFive JH7110 System-Top-Group clock driver
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>, Conor Dooley <conor@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Hal Feng <hal.feng@starfivetech.com>,
        linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org
To:     Emil Renner Berthing <kernel@esmil.dk>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Xingyu Wu <xingyu.wu@starfivetech.com>,
        devicetree@vger.kernel.org, linux-riscv@lists.infradead.org
Date:   Mon, 30 Jan 2023 16:35:59 -0800
User-Agent: alot/0.10
Precedence: bulk

Quoting Xingyu Wu (2023-01-30 00:02:28)
> On 2023/1/26 10:33, Stephen Boyd wrote:
> > Quoting Xingyu Wu (2023-01-19 18:44:37)
> >> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-stg.c b/drivers/=
clk/starfive/clk-starfive-jh7110-stg.c
> >> new file mode 100644
> >> index 000000000000..c2740f44e796
> >> --- /dev/null
> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-stg.c
[...]
> >> +                               parents[i].fw_name =3D "nocstg_bus";
> >> +                       else if (pidx =3D=3D JH7110_STGCLK_APB_BUS)
> >> +                               parents[i].fw_name =3D "apb_bus";
> >=20
> > Can this be an array lookup instead of a pile of conditions?
> >=20
> >       if (pidx < JH7110_STGCLK_END)
> >               ...
> >       else
> >               parents[i].fw_name =3D fw_table[pidx - JH7110_STGCLK_END];
> >=20
> > Or even better, don't use strings at all and just make the 'pidx' number
> > (possibly minus the end constant) be the 'clocks' property index that
> > you want.
>=20
> It seen to be a good way that there uses an array.
> Based on the another way, can I use the 'pidx' number to get the 'clock-n=
ames' property
> to be the parent clock name?

The binding is your design. It is incorrect if the binding is referencing c=
locks
provided by the same node though. If that's the case, simply use the hw
pointer directly.

>=20
> >=20
> >> +               }
> >> +
> >> +               clk->hw.init =3D &init;
> >> +               clk->idx =3D idx;
> >> +               clk->max_div =3D max & JH71X0_CLK_DIV_MASK;
> >> +
> >> +               ret =3D devm_clk_hw_register(&pdev->dev, &clk->hw);
> >> +               if (ret)
> >> +                       return ret;
> >> +       }
> >> +
> >> +       ret =3D devm_of_clk_add_hw_provider(&pdev->dev, jh7110_stgclk_=
get, priv);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       return jh7110_reset_controller_register(priv, "reset-stg", 2);
> >=20
> > Is this also devm-ified?
>=20
> No, it need to be freed actively. I will advise Hal Feng this.
>=20

Oh, that's not good.