Re: [PATCH v3 4/7] media: qcom: camss: Match VFE lite clocks by name prefix

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] media: qcom: camss: undeclared CAMSS_8150 in camss-vfe.c breaks
  build
- [Low] media: qcom: camss: missing Fixes trailer for
  vfe_match_clock_names() bug
- [Low] media: camss: unguarded lite clock checks in
  vfe_match_clock_names()
--

commit 304a59495a7a3eb87dc5d254ba4381a9abf0957b
Author: Andrew Gigena <mail@andrewgigena.dev>

> media: qcom: camss: Match VFE lite clocks by name prefix
>
> vfe_match_clock_names() builds the expected lite clock name from
> vfe->id, but vfe->id is the index of the VFE in the resources array.
> On SoCs where the lite VFEs come after the full VFEs, the generated
> name is vfe_lite2, vfe_lite3, ... and never matches the actual
> vfe_lite0/vfe_lite1 clocks, so their rates are never programmed.
>
> Restrict the new prefix match to numbered "vfe_liteN" clocks (digit
> check) to avoid matching aux clocks, leaving the pre-existing
> vfe_lite_name and bare "vfe_lite" matches unchanged.
>
> This affects SM8150, SC7280 and SC8280XP. On a Pixel 4 (SM8150),
> streaming through VFE-lite0 left cam_cc_ife_lite_0_clk at 19.2 MHz and
> failed with "VFE reset timeout".
>
> Signed-off-by: Andrew Gigena <mail@andrewgigena.dev>

[Severity: Low]
This fixes VFE lite clock matching on existing platforms such as SC7280 and
SC8280XP.  Should this include:

  Fixes: bcd2adfef23a ("media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite")

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 2680a9914765f..29e04d0971163 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
[ ... ]
> @@ -346,6 +347,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>  	case CAMSS_6350:
>  	case CAMSS_7280:
>  	case CAMSS_8x96:
> +	case CAMSS_8150:
>  	case CAMSS_8250:
>  	case CAMSS_8280XP:
>  	case CAMSS_8300:

[Severity: High]
Does adding case CAMSS_8150 here and in vfe_bpl_align_rdi() break the build at
this commit?  CAMSS_8150 is not declared in enum camss_version in
drivers/media/platform/qcom/camss/camss.h until the following commit "media:
qcom: camss: Add support for SM8150", where these two hunks belong.

> @@ -914,6 +916,9 @@ static int vfe_match_clock_names(struct vfe_device *vfe,
>  	return (!strcmp(clock->name, vfe_name) ||
>  		!strcmp(clock->name, vfe_lite_name) ||
>  		!strcmp(clock->name, "vfe_lite") ||
> +		(vfe->res->is_lite &&
> +		 !strncmp(clock->name, "vfe_lite", 8) &&
> +		 isdigit((unsigned char)clock->name[8])) ||
>  		!strcmp(clock->name, "camnoc_axi") ||
>  		!strcmp(clock->name, "camnoc_rt_axi"));
>  }

[Severity: Low]
As reported by Bryan O'Donoghue on v2 [1], should all lite clock checks in
vfe_match_clock_names() be gated on vfe->res->is_lite?  Only lite VFE resource
entries use vfe_lite or vfe_liteN clocks, and once gated on vfe->res->is_lite,
the vfe_lite_name buffer and its snprintf() call are already covered by the
numbered "vfe_lite" prefix check and can be dropped.

[1] https://lore.kernel.org/r/ac2e970b-1f3a-4aef-9a84-034d7e460c0f@linaro.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sm8150-v7.4-rc1-camss-v3-0-0103e8a0bbcd@andrewgigena.dev?part=4