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

On jeu. 18 juin 2026 at 10:03, [email protected] wrote:

> 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.

Yes please fix this

>
> [ ... ]
>
>> +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.

I think AI got it wrong and missed the prefix addition. Unless I missed
something my self, I think you are fine here

>
> 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"?

Yes

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

-- 
Jerome

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