[<prev] [next>] [day] [month] [year] [list]
Message-ID: <6f9a6f0a4d43492604503098313c32f3.sboyd@kernel.org>
Date: Thu, 20 Jul 2023 11:21:11 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Shengjiu Wang <shengjiu.wang@...il.com>
Cc: Shengjiu Wang <shengjiu.wang@....com>, abelvesa@...nel.org,
conor+dt@...nel.org, festevam@...il.com, kernel@...gutronix.de,
krzysztof.kozlowski+dt@...aro.org, linux-imx@....com,
mturquette@...libre.com, peng.fan@....com, robh+dt@...nel.org,
s.hauer@...gutronix.de, shawnguo@...nel.org,
linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] clk: imx: imx8: add audio clock mux driver
Quoting Shengjiu Wang (2023-07-20 00:31:05)
> On Thu, Jul 20, 2023 at 4:17 AM Stephen Boyd <sboyd@...nel.org> wrote:
> 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 {
> > + struct clk_imx8_acm_sel *sels;
> > + unsigned int num_sels;
> > +};
> > +
> > +/**
> > + * struct imx8_acm_priv: private structure
>
> Compile with W=1 and see that this isn't kerneldoc. Please fix.
>
>
> I have used the W=1, but there is not error or warning from kerneldoc
>
> My command is:
> make W=1 ARCH=arm64 CROSS_COMPILE=/opt/toolchain/
> gcc-linaro-14.0.0-2023.06-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu- -j4
>
> Anything I missed?
I thought W=1 ran kerneldoc check, but maybe not by default. Look at
the docs:
https://docs.kernel.org/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
>
>
>
> > + * @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 {
> > + struct clk_imx_acm_pm_domains dev_pm;
> > + const struct imx8_acm_soc_data *soc_data;
> > + void __iomem *reg;
> > + u32 regs[IMX_ADMA_ACM_CLK_END];
> > +};
> > +
> > +static const struct clk_parent_data imx8qm_aud_clk_sels[] = {
> > + {.fw_name = "aud_rec_clk0_lpcg_clk", .name =
> "aud_rec_clk0_lpcg_clk" },
>
> 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 }.
>
>
> The clock "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.
>
> Should I remove .fw_name , only keep .name?
You should remove .name it sounds like.
Powered by blists - more mailing lists