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

Hi Jerome,


Thanks for your review.

On 6/19/2026 3:34 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> 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


Ok, I will fix it in the next version.

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


Thanks for confirming. That was my understanding as well,
so I'll keep the current implementation unchanged.

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


Ok, I will fix it in the next version.

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


--

Jian


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