Re: clk: mt8189: Porting driver for clk

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/06/2025 10:25, irving.ch.lin wrote:
> From: Irving lin <irving-ch.lin@mediatek.corp-partner.google.com>
> 
> 1. Add mt8189 clk driver
> 2. Fix mux failed

I don't understand this.


> 3. Add apll12_div_tdmout_b
> 4. Add disable-unused configs

Neither this

> 
> BUG=b:387252012
> TEST=emerge-skywalker chromeos-kernel-6_6

Drop these

> 
> Signed-off-by: Irving lin <irving-ch.lin@mediatek.corp-partner.google.com>
> ---

Missing bindings / undocumented ABI

There are also several other trivial issues, so this looks like you sent
us old, straight downstream code. This won't work, you need to clean it
up seriously or better start from scratch from recent drivers. Plus read
carefully submitting patches. The way you organized it and sent makes
review very difficult.

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1 for gcc and clang. Most of these
commands (checks or W=1 build) can build specific targets, like some
directory, to narrow the scope to only your code. The code here looks
like it needs a fix. Feel free to get in touch if the warning is not clear.


...

> +static int clk_dbg_mt8189_probe(struct platform_device *pdev)
> +{
> +	set_clkdbg_ops(&clkdbg_mt8189_ops);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver clk_dbg_mt8189_drv = {
> +	.probe = clk_dbg_mt8189_probe,
> +	.driver = {
> +		.name = "clk-dbg-mt8189",
> +		.owner = THIS_MODULE,

10 year old downstream code. Use the tools please or better start from
recent drivers, don't send us 10 year old vendor code.


> +	},
> +};
> +
> +/*
> + * init functions
> + */
> +
> +static int __init clkdbg_mt8189_init(void)
> +{
> +	scpsys_base = ioremap(0x1C001000, PAGE_SIZE);

So on module load on Qcom platform you will map it? You just broke
absolutely EVERY possible user, platform, machine,

This is really, really terrible code.


Best regards,
Krzysztof