Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6f9a6f0a4d43492604503098313c32f3.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAA+D8APd3n8XvC3m5fAoPxqqLzaU-RKm+e9y_G++D6KdaJJhiQ@mail.gmail.com>
References: <1689322259-13504-1-git-send-email-shengjiu.wang@nxp.com> <1689322259-13504-4-git-send-email-shengjiu.wang@nxp.com> <ad757ed215d8f098472be370e3bbd961.sboyd@kernel.org> <CAA+D8APd3n8XvC3m5fAoPxqqLzaU-RKm+e9y_G++D6KdaJJhiQ@mail.gmail.com>
Subject: Re: [PATCH v5 3/3] clk: imx: imx8: add audio clock mux driver
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     Shengjiu Wang <shengjiu.wang@nxp.com>, abelvesa@kernel.org,
        conor+dt@kernel.org, festevam@gmail.com, kernel@pengutronix.de,
        krzysztof.kozlowski+dt@linaro.org, linux-imx@nxp.com,
        mturquette@baylibre.com, peng.fan@nxp.com, robh+dt@kernel.org,
        s.hauer@pengutronix.de, shawnguo@kernel.org,
        linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
To:     Shengjiu Wang <shengjiu.wang@gmail.com>
Date:   Thu, 20 Jul 2023 11:21:11 -0700
User-Agent: alot/0.10
Precedence: bulk

Quoting Shengjiu Wang (2023-07-20 00:31:05)
> On Thu, Jul 20, 2023 at 4:17=E2=80=AFAM Stephen Boyd <sboyd@kernel.org> w=
rote:
>     Quoting Shengjiu Wang (2023-07-14 01:10:59)
>     > +/**
>     > + * struct imx8_acm_soc_data: soc specific data
>     > + * @sels: pointer to struct clk_imx8_acm_sel
>     > + * @num_sels: numbers of items
>     > + */
>     > +struct imx8_acm_soc_data {
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct clk_imx8_acm_sel *sels;
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int num_sels;
>     > +};
>     > +
>     > +/**
>     > + * struct imx8_acm_priv: private structure
>=20
>     Compile with W=3D1 and see that this isn't kerneldoc. Please fix.
>=20
>=20
> I have used the W=3D1,=C2=A0 but there is not error or warning from kerne=
ldoc
>=20
> My command is:
> make W=3D1 ARCH=3Darm64 CROSS_COMPILE=3D/opt/toolchain/
> gcc-linaro-14.0.0-2023.06-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-=
 -j4
>=20
> Anything I missed?

I thought W=3D1 ran kerneldoc check, but maybe not by default. Look at
the docs:

https://docs.kernel.org/doc-guide/kernel-doc.html#structure-union-and-enume=
ration-documentation

>=20
>=20
>=20
>     > + * @dev_pm: multi power domain
>     > + * @soc_data: pointer to soc data
>     > + * @reg: base address of registers
>     > + * @regs: save registers for suspend
>     > + */
>     > +struct imx8_acm_priv {
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct clk_imx_acm_pm_domains dev_pm;
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct imx8_acm_soc_data *soc_dat=
a;
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0void __iomem *reg;
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 regs[IMX_ADMA_ACM_CLK_END];
>     > +};
>     > +
>     > +static const struct clk_parent_data imx8qm_aud_clk_sels[] =3D {
>     > +=C2=A0 =C2=A0 =C2=A0 =C2=A0{.fw_name =3D "aud_rec_clk0_lpcg_clk", =
.name =3D
>     "aud_rec_clk0_lpcg_clk" },
>=20
>     There should only be fw_name here, or use an index. Presumably this
>     isn't migrating old code or bindings. Also, please add space after { =
and
>     before }.
>=20
>=20
> The clock=C2=A0 "aud_rec_clk0_lpcg_clk" is not defined in this provider. =
if
> remove .name There is an issue for finding parent.

The .fw_name should be part of the DT binding. If it isn't part of the
binding as an element of clock-names then it shouldn't be in the driver
as a .fw_name. If it isn't part of the provider, then it should be
referenced by DT index or clock-names. If you use the index, you avoid a
possibly time consuming set of string comparisons. So if you can, use
the index as much as possible. If you're converting a pre-existing
binding you will have to use .name as well to fallback to the globally
unique string name for a clk.

>=20
> Should I remove .fw_name , only keep .name?

You should remove .name it sounds like.