Messages in this thread | | | Date | Fri, 22 Jul 2022 10:48:34 +0200 | Subject | Re: [PATCH v5 7/7] clk: mediatek: Add MediaTek Helio X10 MT6795 clock drivers | From | AngeloGioacchino Del Regno <> |
| |
Il 21/07/22 19:00, Christophe JAILLET ha scritto: > Le 29/06/2022 à 13:02, AngeloGioacchino Del Regno a écrit : >> Add the clock drivers for the entire clock tree of MediaTek Helio X10 >> MT6795, including system clocks (apmixedsys, infracfg, pericfg, topckgen) >> and multimedia clocks (mmsys, mfg, vdecsys, vencsys). >> >> Signed-off-by: AngeloGioacchino Del Regno >> <angelogioacchino.delregno-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> >> Reviewed-by: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> --- >> drivers/clk/mediatek/Kconfig | 37 ++ >> drivers/clk/mediatek/Makefile | 6 + >> drivers/clk/mediatek/clk-mt6795-apmixedsys.c | 157 +++++ >> drivers/clk/mediatek/clk-mt6795-infracfg.c | 148 +++++ >> drivers/clk/mediatek/clk-mt6795-mfg.c | 50 ++ >> drivers/clk/mediatek/clk-mt6795-mm.c | 106 ++++ >> drivers/clk/mediatek/clk-mt6795-pericfg.c | 160 +++++ >> drivers/clk/mediatek/clk-mt6795-topckgen.c | 610 +++++++++++++++++++ >> drivers/clk/mediatek/clk-mt6795-vdecsys.c | 55 ++ >> drivers/clk/mediatek/clk-mt6795-vencsys.c | 50 ++ >> 10 files changed, 1379 insertions(+) >> create mode 100644 drivers/clk/mediatek/clk-mt6795-apmixedsys.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-infracfg.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-mfg.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-mm.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-pericfg.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-topckgen.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-vdecsys.c >> create mode 100644 drivers/clk/mediatek/clk-mt6795-vencsys.c >> > > > [...] > >> diff --git a/drivers/clk/mediatek/clk-mt6795-apmixedsys.c >> b/drivers/clk/mediatek/clk-mt6795-apmixedsys.c >> new file mode 100644 >> index 000000000000..e87db76799af >> --- /dev/null >> +++ b/drivers/clk/mediatek/clk-mt6795-apmixedsys.c > > [...] > >> +static int clk_mt6795_apmixed_probe(struct platform_device *pdev) >> +{ >> + struct clk_hw_onecell_data *clk_data; >> + struct device *dev = &pdev->dev; >> + struct device_node *node = dev->of_node; >> + void __iomem *base; >> + struct clk_hw *hw; >> + int ret; >> + >> + base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); >> + if (!clk_data) >> + return -ENOMEM; >> + >> + ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data); >> + if (ret) >> + goto free_clk_data; >> + >> + hw = mtk_clk_register_ref2usb_tx("ref2usb_tx", "clk26m", base + REG_REF2USB); > > This calls kzalloc() and clk_hw_register() but... > >> + if (IS_ERR(hw)) { >> + ret = PTR_ERR(hw); >> + dev_err(dev, "Failed to register ref2usb_tx: %d\n", ret); >> + goto unregister_plls; >> + } >> + clk_data->hws[CLK_APMIXED_REF2USB_TX] = hw; >> + >> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); >> + if (ret) { >> + dev_err(dev, "Cannot register clock provider: %d\n", ret); >> + goto unregister_ref2usb; >> + } >> + >> + /* Setup MD1 to avoid random crashes */ >> + dev_dbg(dev, "Performing initial setup for MD1\n"); >> + clk_mt6795_apmixed_setup_md1(base); >> + >> + return 0; >> + >> +unregister_ref2usb: >> + clk_hw_unregister(clk_data->hws[CLK_APMIXED_REF2USB_TX]); > > ... only clk_hw_register() is undone here. Should a mtk_clk_unregister_ref2usb_tx() > be useful? > > Or is it already handled somewhere else? > >> +unregister_plls: >> + mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data); >> +free_clk_data: >> + mtk_free_clk_data(clk_data); >> + return ret; >> +} >> + >> +static int clk_mt6795_apmixed_remove(struct platform_device *pdev) >> +{ >> + struct device_node *node = pdev->dev.of_node; >> + struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev); >> + >> + of_clk_del_provider(node); >> + clk_hw_unregister(clk_data->hws[CLK_APMIXED_REF2USB_TX]); > > Same here. > >> + mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data); >> + mtk_free_clk_data(clk_data); >> + >> + return 0; >> +} >> + >> +static struct platform_driver clk_mt6795_apmixed_drv = { >> + .probe = clk_mt6795_apmixed_probe, >> + .remove = clk_mt6795_apmixed_remove, >> + .driver = { >> + .name = "clk-mt6795-apmixed", >> + .of_match_table = of_match_clk_mt6795_apmixed, >> + }, >> +}; >> +module_platform_driver(clk_mt6795_apmixed_drv); >> + >> +MODULE_DESCRIPTION("MediaTek MT6795 apmixed clocks driver"); >> +MODULE_LICENSE("GPL v2"); > > [...] > >> diff --git a/drivers/clk/mediatek/clk-mt6795-mm.c >> b/drivers/clk/mediatek/clk-mt6795-mm.c >> new file mode 100644 >> index 000000000000..27a8859ff5b5 >> --- /dev/null >> +++ b/drivers/clk/mediatek/clk-mt6795-mm.c > > [...] > >> +static int clk_mt6795_mm_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *node = dev->parent->of_node; >> + struct clk_hw_onecell_data *clk_data; >> + int ret; >> + >> + clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK); >> + if (!clk_data) >> + return -ENOMEM; >> + >> + ret = mtk_clk_register_gates(node, mm_gates, ARRAY_SIZE(mm_gates), clk_data); >> + if (ret) > > Add an error handling path and call mtk_free_clk_data(clk_data); if this fails? > >> + return ret; >> + >> + return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); > > Same here, + mtk_clk_unregister_gates()? > >> +} >> + >> +static struct platform_driver clk_mt6795_mm_drv = { >> + .driver = { >> + .name = "clk-mt6795-mm", >> + }, >> + .probe = clk_mt6795_mm_probe, > > .remove function that mimics the (non existent) error handling path of the probe? > >> +}; >> +module_platform_driver(clk_mt6795_mm_drv); >> + >> +MODULE_DESCRIPTION("MediaTek MT6795 multimedia clocks driver"); >> +MODULE_LICENSE("GPL v2"); > > [...] >
Hello Cristophe, thanks for the review!
I agree with all of the above, v6 will contain your proposed fixes.
Regards, Angelo
| |