RE: [PATCH 3/3] clk: imx: Add clock driver support for imx8mm

[Jacky Bai <ping.bai@nxp.com>]

> Subject: Re: [PATCH 3/3] clk: imx: Add clock driver support for imx8mm
> 
> Quoting Jacky Bai (2019-01-08 01:01:04)
> > +
> > +static int imx8mm_clocks_probe(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +       void __iomem *base;
> > +       int ret = 0;
> 
> Please don't assign ret here. Just let it be assigned later on.
> 

Sure.

> > +
> > +       clks[IMX8MM_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> > +       clks[IMX8MM_CLK_24M] = of_clk_get_by_name(np, "osc_24m");
> > +       clks[IMX8MM_CLK_32K] = of_clk_get_by_name(np, "osc_32k");
> /* Check more */
> > +       clks[IMX8MM_CLK_EXT1] = of_clk_get_by_name(np, "clk_ext1");
> > +       clks[IMX8MM_CLK_EXT2] = of_clk_get_by_name(np, "clk_ext2");
> > +       clks[IMX8MM_CLK_EXT3] = of_clk_get_by_name(np, "clk_ext3");
> > +       clks[IMX8MM_CLK_EXT4] = of_clk_get_by_name(np, "clk_ext4");
> > +
> > +       np = of_find_compatible_node(NULL, NULL,
> "fsl,imx8mm-anatop");
> > +       base = of_iomap(np, 0);
> > +       if (WARN_ON(!base))
> > +               return -ENOMEM;
> 
> Why do we need to reach into some other node to get the memory region to
> map?
> 

The PLLs' config register is in a sperate memory region. As we registered all the PLLs clocks in this driver, the PLLs memory region
Need to be read out from its corresponding node. This is the method that we used on our all i.MX platform.

> > +
> > +       clks[IMX8MM_AUDIO_PLL1_REF_SEL] =
> imx_clk_mux("audio_pll1_ref_sel", base + 0x0, 0, 2, pll_ref_sels,
> ARRAY_SIZE(pll_ref_sels));
> > +       clks[IMX8MM_AUDIO_PLL2_REF_SEL] =
> imx_clk_mux("audio_pll2_ref_sel", base + 0x14, 0, 2, pll_ref_sels,
> ARRAY_SIZE(pll_ref_sels));
> > +       clks[IMX8MM_VIDEO_PLL1_REF_SEL] =
> imx_clk_mux("video_pll1_ref_sel", base + 0x28, 0, 2, pll_ref_sels,
> ARRAY_SIZE(pll_ref_sels));
> > +       clks[IMX8MM_DRAM_PLL_REF_SEL] =
> > + imx_clk_mux("dram_pll_ref_sel", base + 0x50, 0, 2, pll_ref_sels,
> > + ARRAY_SIZE(pll_ref_sels));
> [...]
> > +       clks[IMX8MM_SYS_PLL2_333M] =
> imx_clk_fixed_factor("sys_pll2_333m", "sys_pll2_out", 1, 3);
> > +       clks[IMX8MM_SYS_PLL2_500M] =
> imx_clk_fixed_factor("sys_pll2_500m", "sys_pll2_out", 1, 2);
> > +       clks[IMX8MM_SYS_PLL2_1000M] =
> > + imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1);
> > +
> > +       np = dev->of_node;
> > +       base = of_iomap(np, 0);
> > +       if (WARN_ON(!base))
> > +               return -ENOMEM;
> 
> This can surely use the platform device APIs to map and retrieve memory
> regions.
> 

Ok, will fix it

> > +
> [...]
> > +
> > +       imx_check_clocks(clks, ARRAY_SIZE(clks));
> > +
> > +       clk_data.clks = clks;
> > +       clk_data.clk_num = ARRAY_SIZE(clks);
> > +       ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to register clks for i.MX8MM\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       imx_register_uart_clocks(uart_clks);
> > +
> > +       pr_info("i.MX8MM clock driver init done\n");
> 
> Please no "I"m alive" messages.

I will remove it in V2.

BR
> 
> > +
> > +       return 0;
> > +}
> > +