Re: [PATCH v1 1/1] media: qcom: camss: Restructure camss_link_entities

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

On 11/11/2024 17:38, Vikram Sharma wrote:
> Refactor the camss_link_entities function by breaking it down into
> three distinct functions. Each function will handle the linking of
> a specific entity separately, enhancing readability.
> 
> 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 | 159 ++++++++++++++--------
>   1 file changed, 105 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index fabe034081ed..1052c01b45f3 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1840,14 +1840,66 @@ static int camss_init_subdevices(struct camss *camss)
>   }
>   
>   /*
> - * camss_link_entities - Register subdev nodes and create links
> + * camss_link_entities_csid - 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(struct camss *camss)
> +static int camss_link_entities_csid(struct camss *camss)
>   {
> -	int i, j, k;
> +	int i, j;
> +	int ret, line_num;
> +	u16 src_pad;
> +	u16 sink_pad;
> +	struct media_entity *src_entity;
> +	struct media_entity *sink_entity;

Vikram.

Thanks for the patch.

Please reverse Christmas tree this declaration

struct media_entity *sink_entity;
struct media_entity *src_entity;
int ret, line_num;
u16 sink_pad;
u16 src_pad;
int i, j;

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

We repeat this pattern over and over again.

I realise that's how it has evolved in this code but since we are going 
in with the knife we may as well fix this too.

Please functionally decompose the "Failed to link" message down into a 
function.

Once both of those are done:

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

---
bod