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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The clock gate for `ao_pwm_b` incorrectly writes to the `AO_PWM_CLK_A_CTRL` register instead of `AO_PWM_CLK_B_CTRL`.
- [Low] Typographical errors and naming mismatches in clock names (i2c3 instead of i3c, and duandiv instead of dualdiv).
- [Low] Missing explicit inclusion of `<linux/module.h>` for module macros.
--

commit e37e299a2bc904562b2a27fa6a1542c7920a030a
Author: Jian Hu <jian.hu@amlogic.com>

clk: amlogic: Add A9 AO clock controller driver

This commit introduces the Always-on clock controller driver for the Amlogic
A9 SoC family. It adds the clock definitions and registers the platform
driver for the A9 AO clocks.

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

This isn't a bug, but should <linux/module.h> be explicitly included here?
The driver uses MODULE_DESCRIPTION, MODULE_AUTHOR, MODULE_LICENSE,
MODULE_IMPORT_NS, and MODULE_DEVICE_TABLE macros, which rely on this header.

[ ... ]

> +static A9_AO_PCLK(i2c3,		0,	0);

This isn't a bug, but should this be named i3c instead of i2c3?
This is later mapped to the CLKID_AO_SYS_I3C enum, so the dt-bindings and
the hardware clock name appear to be mismatched.

[ ... ]

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

Could the gate for ao_pwm_b be incorrectly writing to AO_PWM_CLK_A_CTRL
instead of AO_PWM_CLK_B_CTRL?

If so, toggling the ao_pwm_b clock will modify the gate bit for ao_pwm_a,
which could cause uncoordinated toggling of the ao_pwm_a hardware state
and fail to actually control the ao_pwm_b clock.

[ ... ]

> +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",

This isn't a bug, but is there a typo in the clock name "ao_rtc_duandiv_in"?
It looks like it should be "ao_rtc_dualdiv_in".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-b4-a9_clk-v1-0-41cb4071b7c9@amlogic.com?part=10