Re: [PATCH 06/10] media: qcom: camss: Add camss_link_entities_v2

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 04/09/2024 12:10, Vikram Sharma wrote:
> Add camss_link_entities_v2, derived from the camss_link_entities
> function, to handle linking for targets without ISPIF.
> 
> camss_link_entities -> Targets with ispif.
> camss_link_entities_v2 -> Targets without ispif.
> 
> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 53 ++++++++++++++++++++++++++++++-
>   1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 5e7235001239..516434686a27 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2154,6 +2154,57 @@ static int camss_init_subdevices(struct camss *camss)
>   	return 0;
>   }
>   
> +/*
> + * camss_link_entities_v2 - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities_v2(struct camss *camss)
> +{
> +	int i, j;
> +	int ret;
> +
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
> +		for (j = 0; j < camss->res->csid_num; j++) {
> +			ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
> +						    MSM_CSIPHY_PAD_SRC,
> +						    &camss->csid[j].subdev.entity,
> +						    MSM_CSID_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					camss->csiphy[i].subdev.entity.name,
> +					camss->csid[j].subdev.entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	}

Please reduce the above loop down into a common call that both 
camss_link_entities_ispif()[1] and camss_link_entities() can call.

Because => functional decomposition and code reuse instead of code 
replication.

> +	for (i = 0; i < camss->res->csid_num; i++)
> +		for (j = 0; j < camss->vfe[i].res->line_num; j++) {
> +			struct v4l2_subdev *csid = &camss->csid[i].subdev;
> +			struct v4l2_subdev *vfe = &camss->vfe[i].line[j].subdev;
> +
> +			ret = media_create_pad_link(&csid->entity,
> +						    MSM_CSID_PAD_FIRST_SRC + j,
> +						    &vfe->entity,
> +						    MSM_VFE_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					csid->entity.name,
> +					vfe->entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	return 0;
> +}

Having a tidy function for every non ispif SoC seems like nice code, 
approve.

However the corollary is the ispif version of the code should then drop the

if (camss->ispif) {
     //do stuff
} else {
     // do other stuff
}

i.e. your function pointer now determines if ispif is true so you can 
drop the dead code on the else path in camss_link_entities_ispif()

> +
>   /*
>    * camss_link_entities - Register subdev nodes and create links
>    * @camss: CAMSS device
> @@ -2769,7 +2820,7 @@ static const struct camss_resources sc8280xp_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_sc8280xp),
>   	.csid_num = ARRAY_SIZE(csid_res_sc8280xp),
>   	.vfe_num = ARRAY_SIZE(vfe_res_sc8280xp),
> -	.link_entities = camss_link_entities
> +	.link_entities = camss_link_entities_v2
>   };
>   
>   static const struct camss_resources sc7280_resources = {
> 

This change isn't correct.

There are six upstream camss entires

grep camss arch/arm64/boot/dts/qcom/*.dtsi  | grep compat | wc -l
6

Only three of which have ispif

grep -m1 "ispif" arch/arm64/boot/dts/qcom/*.dtsi  | wc -l
3

=> You've missed 2/3 of the SoCs the change should apply to - 845 and 8250.

[1] I also think the legacy function should be called 
camss_link_entities_ispif() with the default being camss_link_entities();

1. Please change the name of the existing function to
    camss_link_entities_ispif() to reflect its function and dependency.
2. Functionally decompose the top part of the loop into some common
    method that both camss_link_entities_ispif() and
    camss_link_entities() can call.
    Because functionally decomposed and reused code is better code.
3. You need to make sure you hook the right set of SoCs with the
    default and legacy versions of the function
    -> msm8916/msm8996/sdm630 .link_entites = camss_link_entities_ispif()
    -> sdm845/sc7280/sc8280xp .link_entities = camss_link_entities()

---
bod