Re: [PATCH v4 2/2] clk: amlogic: Add A9 AO clock controller driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The AO PWM clocks are registered without an `ao_` or `a9_ao_` prefix, which will cause global namespace collisions with the main clock controller.
- [Low] Typographical error in the clock name string literal for the RTC dualdiv input gate: `ao_rtc_duandiv_in` instead of `ao_rtc_dualdiv_in`.
- [Low] Missing `#include <linux/module.h>` for `MODULE_*` macros.
--

commit ef5998ff087b6faed09d9eed4f59078d2b2f0a60
Author: Jian Hu <[email protected]>

clk: amlogic: Add A9 AO clock controller driver
    
Add the Always-on clock controller driver for the Amlogic A9 SoC family.

> diff --git a/drivers/clk/meson/a9-aoclk.c b/drivers/clk/meson/a9-aoclk.c
> new file mode 100644
> index 0000000000000..88aa8cf3f5d00
> --- /dev/null
> +++ b/drivers/clk/meson/a9-aoclk.c
> @@ -0,0 +1,488 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +/*
> + * Copyright (C) 2026 Amlogic, Inc. All rights reserved
> + */
> +
> +#include <dt-bindings/clock/amlogic,a9-aoclkc.h>
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>

[Severity: Low]
Should this file explicitly include <linux/module.h>?

The file relies on several module macros at the end (such as MODULE_DESCRIPTION,
MODULE_AUTHOR, and MODULE_LICENSE). Although it currently compiles successfully
due to transitive includes from <linux/platform_device.h>, standard kernel
practice is to include the header explicitly.

[ ... ]

> +static const struct clk_parent_data a9_ao_pwm_parents[] = {
> +	{ .hw = &a9_ao_xtal.hw },
> +	{ .fw_name = "fdiv5", },
> +	{ .fw_name = "fdiv4", },
> +	{ .fw_name = "fdiv3", }
> +};
> +
> +static A9_COMP_SEL(pwm_a, AO_PWM_CLK_A_CTRL, 9, 0x7, a9_ao_pwm_parents);

[Severity: Medium]
Will this cause a clock name collision with the main clock controller?

Because the A9_COMP_SEL macro uses the #_name stringification operator inside
MESON_COMP_SEL, this will generate the clock name as exactly "pwm_a_sel"
(and similarly "pwm_a_div" and "pwm_a" for the others), without any "ao_" or
"a9_ao_" prefix.

If the main clock controller conventionally registers a "pwm_a" clock, failing
to prefix the Always-On clocks will lead to global namespace collisions in the
Common Clock Framework and debugfs.

> +static A9_COMP_DIV(pwm_a, AO_PWM_CLK_A_CTRL, 0, 8);
> +static A9_COMP_GATE(pwm_a, AO_PWM_CLK_A_CTRL, 8);
> +
> +static A9_COMP_SEL(pwm_b, AO_PWM_CLK_B_CTRL, 9, 0x7, a9_ao_pwm_parents);

[ ... ]

> +static struct clk_regmap a9_ao_rtc_dualdiv_in = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = AO_RTC_BY_OSCIN_CTRL0,
> +		.bit_idx = 31,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "ao_rtc_duandiv_in",

[Severity: Low]
This isn't a bug, but there's a typo in the clock name literal. Should
"ao_rtc_duandiv_in" be "ao_rtc_dualdiv_in"?

> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&a9_ao_xtal.hw
> +		},

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic