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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `hdmitx1_pixel` clock uses the wrong parent selector.
- [Low] The Kconfig symbol `COMMON_CLK_A9_PLL` is implied by `COMMON_CLK_A9_PERIPHERALS` but is never defined anywhere in the kernel tree.
- [Low] Typographical error in clock name: `a9_encoder1` is named "encorder1".
--

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

clk: amlogic: Add A9 peripherals clock controller driver
    
Add the peripherals clock controller driver for the Amlogic A9 SoC family.

> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index cf8cf3f9e4ee8..86d2e270e1b82 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -132,6 +132,21 @@ config COMMON_CLK_A1_PERIPHERALS
>  	  device, A1 SoC Family. Say Y if you want A1 Peripherals clock
>  	  controller to work.
>  
> +config COMMON_CLK_A9_PERIPHERALS
> +	tristate "Amlogic A9 SoC peripherals clock controller support"
> +	depends on ARM64 || COMPILE_TEST
> +	default ARCH_MESON
> +	select COMMON_CLK_MESON_REGMAP
> +	select COMMON_CLK_MESON_CLKC_UTILS
> +	select COMMON_CLK_MESON_DUALDIV
> +	select COMMON_CLK_MESON_VID_PLL_DIV
> +	imply COMMON_CLK_SCMI
> +	imply COMMON_CLK_A9_PLL

[Severity: Low]
Does the symbol COMMON_CLK_A9_PLL exist in the kernel tree? It appears to be
undefined, which means implying it has no effect. Could this be a typo or
perhaps a missing patch dependency?

[ ... ]
> diff --git a/drivers/clk/meson/a9-peripherals.c b/drivers/clk/meson/a9-peripherals.c
> new file mode 100644
> index 0000000000000..8e0ae3205d772
> --- /dev/null
> +++ b/drivers/clk/meson/a9-peripherals.c
[ ... ]
> +static struct clk_regmap a9_encoder1_sel = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = VIID_CLK_DIV,
> +		.mask = 0xf,
> +		.shift = 8,
> +		.table = a9_vid_parents_val_table,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "encoder1_sel",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_hws = a9_vid_parents,
> +		.num_parents = ARRAY_SIZE(a9_vid_parents),
> +	},
> +};
> +
> +static struct clk_regmap a9_encoder1 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = VID_CLK_CTRL2,
> +		.bit_idx = 11,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "encorder1",

[Severity: Low]
This isn't a bug, but there's a typo in the clock name (encorder1 instead of
encoder1).

> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&a9_encoder1_sel.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
[ ... ]
> +static struct clk_regmap a9_hdmitx1_pixel = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = VID_CLK_CTRL2,
> +		.bit_idx = 12,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "hdmitx1_pixel",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&a9_hdmitx_pixel_sel.hw

[Severity: Medium]
Should this parent be &a9_hdmitx1_pixel_sel.hw instead of
&a9_hdmitx_pixel_sel.hw? Using a9_hdmitx_pixel_sel looks like a copy-paste error
that might route the clock incorrectly for the secondary HDMI transmitter.

> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};

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