[v5,0/3] clk: imx: add audio clock mux driver

The Audio Clock Mux (ACM) is a collection of control registers
and multiplexers that are used to route the audio source clocks
to the audio peripherals on i.MX8QXP, i.MX8QM, i.MX8DXL

Shengjiu Wang (3):
  dt-bindings: clock: fsl,imx8-acm: Add audio clock mux support
  dt-bindings: clock: imx8-clock: Add audio clock mux related clock
  clk: imx: imx8: add audio clock mux driver

 .../bindings/clock/fsl,imx8-acm.yaml          | 156 ++++++
 drivers/clk/imx/Makefile                      |   3 +-
 drivers/clk/imx/clk-imx8-acm.c                | 477 ++++++++++++++++++
 include/dt-bindings/clock/imx8-clock.h        |  28 +
 4 files changed, 663 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/fsl,imx8-acm.yaml
 create mode 100644 drivers/clk/imx/clk-imx8-acm.c

On 7/14/2023 4:10 PM, Shengjiu Wang wrote:
> The Audio Clock Mux (ACM) is a collection of control registers
> and multiplexers that are used to route the audio source clocks
> to the audio peripherals.
> 
> Each audio peripheral has its dedicated audio clock mux
> (which differ based on usage) and control register.
> 
> Signed-off-by: Shengjiu Wang<shengjiu.wang@nxp.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Quoting Shengjiu Wang (2023-07-14 01:10:59)
> diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
> new file mode 100644
> index 000000000000..445a0b38281c
> --- /dev/null
> +++ b/drivers/clk/imx/clk-imx8-acm.c
> @@ -0,0 +1,477 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright 2023 NXP
> +//
> +
> +#include <dt-bindings/clock/imx8-clock.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#include "clk.h"
> +
> +/**
> + * struct clk_imx_acm_pm_domains: structure for multi power domain
> + * @pd_dev: power domain device
> + * @pd_dev_link: power domain device link
> + * @num_domains: power domain nummber
> + */
> +struct clk_imx_acm_pm_domains {
> +       struct device **pd_dev;
> +       struct device_link **pd_dev_link;
> +       int    num_domains;
> +};
> +
> +/**
> + * struct clk_imx8_acm_sel: for clock mux
> + * @name: clock name
> + * @clkid: clock id
> + * @parents: clock parents
> + * @num_parents: clock parents number
> + * @reg: register offset
> + * @shift: bit shift in register
> + * @width: bits width
> + */
> +struct clk_imx8_acm_sel {
> +       const char                      *name;
> +       int                             clkid;
> +       const struct clk_parent_data    *parents;       /* For mux */
> +       int                             num_parents;
> +       u32                             reg;
> +       u8                              shift;
> +       u8                              width;
> +};
> +
> +/**
> + * 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.

> + * @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 }.

> +       {.fw_name = "aud_rec_clk1_lpcg_clk", .name = "aud_rec_clk1_lpcg_clk" },
> +       {.fw_name = "mlb_clk", .name = "mlb_clk" },
> +       {.fw_name = "hdmi_rx_mclk", .name = "hdmi_rx_mclk" },
> +       {.fw_name = "ext_aud_mclk0", .name = "ext_aud_mclk0" },
> +       {.fw_name = "ext_aud_mclk1", .name = "ext_aud_mclk1" },
> +       {.fw_name = "esai0_rx_clk", .name = "esai0_rx_clk" },
> +       {.fw_name = "esai0_rx_hf_clk", .name = "esai0_rx_hf_clk" },
> +       {.fw_name = "esai0_tx_clk", .name = "esai0_tx_clk" },
> +       {.fw_name = "esai0_tx_hf_clk", .name = "esai0_tx_hf_clk" },
> +       {.fw_name = "esai1_rx_clk", .name = "esai1_rx_clk" },
> +       {.fw_name = "esai1_rx_hf_clk", .name = "esai1_rx_hf_clk" },
> +       {.fw_name = "esai1_tx_clk", .name = "esai1_tx_clk" },
> +       {.fw_name = "esai1_tx_hf_clk", .name = "esai1_tx_hf_clk" },
> +       {.fw_name = "spdif0_rx", .name = "spdif0_rx" },
> +       {.fw_name = "spdif1_rx", .name = "spdif1_rx" },
> +       {.fw_name = "sai0_rx_bclk", .name = "sai0_rx_bclk" },
> +       {.fw_name = "sai0_tx_bclk", .name = "sai0_tx_bclk" },
> +       {.fw_name = "sai1_rx_bclk", .name = "sai1_rx_bclk" },
> +       {.fw_name = "sai1_tx_bclk", .name = "sai1_tx_bclk" },
> +       {.fw_name = "sai2_rx_bclk", .name = "sai2_rx_bclk" },
> +       {.fw_name = "sai3_rx_bclk", .name = "sai3_rx_bclk" },
> +       {.fw_name = "sai4_rx_bclk", .name = "sai4_rx_bclk" },
> +};
[...]
> +
> +static const struct clk_parent_data imx8dxl_mclk_out_sels[] = {
> +       {.fw_name = "aud_rec_clk0_lpcg_clk", .name = "aud_rec_clk0_lpcg_clk" },
> +       {.fw_name = "aud_rec_clk1_lpcg_clk", .name = "aud_rec_clk1_lpcg_clk" },
> +       {.name = "dummy" },
> +       {.name = "dummy" },
> +       {.fw_name = "spdif0_rx", .name = "spdif0_rx" },
> +       {.name = "dummy" },
> +       {.name = "dummy" },
> +       {.name = "dummy" },

Instead of dummy can you use -1 as the index?

Quoting Shengjiu Wang (2023-07-20 00:31:05)
> On Thu, Jul 20, 2023 at 4:17 AM Stephen Boyd <sboyd@kernel.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.