Re: [PATCH v2 34/35] clk: mediatek: mt8196: Switch to common probe/remove helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: mediatek: redundant ioremap and exclusive region request for PLLs
--

commit 493ba4ac616daf31697a4dcddf3d65ea3f269cb3
Author: Akari Tsuyukusa <akkun11.open@gmail.com>

clk: mediatek: mt8196: Switch to common probe/remove helpers

This commit updates the MT8196 apmixedsys, mcusys, and mfg drivers to use
the common mtk_clk_simple_probe() and mtk_clk_simple_remove() helpers by
representing their PLL configurations with struct mtk_clk_desc.

> diff --git a/drivers/clk/mediatek/clk-mt8196-apmixedsys.c b/drivers/clk/mediatek/clk-mt8196-apmixedsys.c
> index c4ebb0170b82b..a20fa02070a88 100644
> --- a/drivers/clk/mediatek/clk-mt8196-apmixedsys.c
> +++ b/drivers/clk/mediatek/clk-mt8196-apmixedsys.c

[ ... ]

> @@ -192,8 +141,8 @@ static const struct of_device_id of_match_clk_mt8196_apmixed[] = {
>  MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_apmixed);
>  
>  static struct platform_driver clk_mt8196_apmixed_drv = {
> -	.probe = clk_mt8196_apmixed_probe,
> -	.remove = clk_mt8196_apmixed_remove,
> +	.probe = mtk_clk_simple_probe,

[Severity: Medium]
Does switching to mtk_clk_simple_probe() cause an unintended exclusive 
memory region request for the shared apmixedsys controller?

By switching to the common helper without setting .shared_io = true in 
the mtk_clk_desc structures, __mtk_clk_simple_probe() will map the IO 
region exclusively:

drivers/clk/mediatek/clk-mtk.c:__mtk_clk_simple_probe() {
	/* Composite and divider clocks needs us to pass iomem pointer */
	if (mcd->composite_clks || mcd->divider_clks || mcd->plls) {
		if (!mcd->shared_io)
			base = devm_platform_ioremap_resource(pdev, 0);
...
}

Because devm_platform_ioremap_resource() calls request_mem_region(), 
this exclusive claim could conflict with other drivers expecting to 
access this shared syscon node.

Furthermore, the exclusively mapped `base` pointer appears to be completely 
unused for PLL registration, as mtk_clk_register_plls() maps the region a 
second time via of_iomap():

drivers/clk/mediatek/clk-pll.c:mtk_clk_register_plls() {
...
	base = of_iomap(dev->of_node, 0);
...
}

Would it be safer to set .shared_io = true in apmixed_desc and 
apmixed2_desc (and similarly for other shared controllers in this patch) to 
avoid the redundant mapping and exclusive resource lock?

> +	.remove = mtk_clk_simple_remove,
>  	.driver = {
>  		.name = "clk-mt8196-apmixed",
>  		.of_match_table = of_match_clk_mt8196_apmixed,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013151734.3177690-1-akkun11.open@gmail.com?part=34