Re: [PATCH v2 2/2] clk: qcom: Add support for the camera clock controller (CAMCC) on Hawi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing post-divider definition and mismatched clock parent mapping for `cam_cc_pll7_out_even`.
- [Low] Redundant and identical parent map and parent data definitions.
--

commit 94962e26b9035957a94cf2180f7c86517eb17167
Author: Vivek Aknurwar <vivek.aknurwar@oss.qualcomm.com>

clk: qcom: Add support for the camera clock controller (CAMCC) on Hawi

This commit adds support for the camera clock controller (CAMCC) on the
Qualcomm Hawi SoC. It introduces the clock definitions and routing
for camera operations, reusing the MCLK driver from Kaanapali.

> diff --git a/drivers/clk/qcom/camcc-hawi.c b/drivers/clk/qcom/camcc-hawi.c
> new file mode 100644
> index 0000000000000..5b004a627fdde
> --- /dev/null
> +++ b/drivers/clk/qcom/camcc-hawi.c

[ ... ]

> +static const struct parent_map cam_cc_parent_map_1[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_CAM_CC_PLL0_OUT_MAIN, 1 },
> +	{ P_CAM_CC_PLL0_OUT_EVEN, 2 },
> +	{ P_CAM_CC_PLL0_OUT_ODD, 3 },
> +	{ P_CAM_CC_PLL6_OUT_ODD, 4 },
> +	{ P_CAM_CC_PLL6_OUT_EVEN, 5 },
> +	{ P_CAM_CC_PLL6_OUT_MAIN, 6 },
> +};
> +
> +static const struct clk_parent_data cam_cc_parent_data_1[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &cam_cc_pll0.clkr.hw },
> +	{ .hw = &cam_cc_pll0_out_even.clkr.hw },
> +	{ .hw = &cam_cc_pll0_out_odd.clkr.hw },
> +	{ .hw = &cam_cc_pll6_out_odd.clkr.hw },
> +	{ .hw = &cam_cc_pll6_out_even.clkr.hw },
> +	{ .hw = &cam_cc_pll6.clkr.hw },
> +};

[Severity: Low]
Are cam_cc_parent_map_1 and cam_cc_parent_data_1 intended to be completely
identical to the _0 variants defined just above this block?

This isn't a functional issue, but could we reuse the _0 map and data
structures to reduce code duplication and simplify the driver?

[ ... ]

> +static const struct parent_map cam_cc_parent_map_2[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_CAM_CC_PLL0_OUT_MAIN, 1 },
> +	{ P_CAM_CC_PLL0_OUT_EVEN, 2 },
> +	{ P_CAM_CC_PLL0_OUT_ODD, 3 },
> +	{ P_CAM_CC_PLL7_OUT_EVEN, 5 },
> +	{ P_CAM_CC_PLL7_OUT_MAIN, 6 },
> +};
> +
> +static const struct clk_parent_data cam_cc_parent_data_2[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &cam_cc_pll0.clkr.hw },
> +	{ .hw = &cam_cc_pll0_out_even.clkr.hw },
> +	{ .hw = &cam_cc_pll0_out_odd.clkr.hw },
> +	{ .hw = &cam_cc_pll7.clkr.hw },
> +	{ .hw = &cam_cc_pll7.clkr.hw },
> +};

[Severity: Medium]
Does this misconfigure the clock parent mapping for cam_cc_pll7_out_even?

The parent map array specifies P_CAM_CC_PLL7_OUT_EVEN at index 5, but the
corresponding parent data array maps it to the undivided main PLL
(&cam_cc_pll7.clkr.hw) instead of a post-divider hw struct.

If the bootloader leaves the hardware mux at index 5, could this cause the
framework to miscalculate the frequency as the undivided main PLL rate
instead of the divided rate, leading to incorrect downstream clock rates?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-camcc-hawi-v2-0-f892ba1d118e@oss.qualcomm.com?part=2