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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hello Vikram.

On 11/12/24 15: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-vfe.c |   6 +-
>   drivers/media/platform/qcom/camss/camss.c     | 196 ++++++++++++------
>   drivers/media/platform/qcom/camss/camss.h     |   4 +
>   3 files changed, 138 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 83c5a36d071f..446604cc7ef6 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -1794,9 +1794,9 @@ int msm_vfe_register_entities(struct vfe_device *vfe,
>   				&video_out->vdev.entity, 0,
>   				MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
>   		if (ret < 0) {
> -			dev_err(dev, "Failed to link %s->%s entities: %d\n",
> -				sd->entity.name, video_out->vdev.entity.name,
> -				ret);
> +			camss_link_err(vfe->camss, sd->entity.name,
> +				       video_out->vdev.entity.name,
> +				       ret);

As Bryan said, the change above is unrelated to the restructuring.

See also my next comment.

>   			goto error_link;
>   		}
>   	}
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index fabe034081ed..980cb1e337be 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1840,14 +1840,83 @@ static int camss_init_subdevices(struct camss *camss)
>   }
>   
>   /*
> - * camss_link_entities - Register subdev nodes and create links
> + * camss_link_err - print error in case link creation fails
> + * @src_name: name for source of the link
> + * @sink_name: name for sink of the link
> + */
> +inline void camss_link_err(struct camss *camss,
> +			   const char *src_name,
> +			   const char *sink_name,
> +			   int ret)
> +{
> +	if (!camss || !src_name || !sink_name)
> +		return;
> +	dev_err(camss->dev,
> +		"Failed to link %s->%s entities: %d\n",
> +		src_name,
> +		sink_name,
> +		ret);
> +}

I believe this new function is simply not needed. It is not helpful.

> +/*
> + * 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;
> +	struct media_entity *src_entity;
> +	struct media_entity *sink_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;
> +			}

So, you split one solid function, which covers csid->ispif and ispif->vfe
into two separate functions, the logic of "if (camss->ispif)" is applied
twice in two different functions, while before the change it was done just
once, then why does it enhance readability? I think it's just the opposite...

> +
> +			ret = media_create_pad_link(src_entity,
> +						    src_pad,
> +						    sink_entity,
> +						    sink_pad,
> +						    0);
> +			if (ret < 0) {
> +				camss_link_err(camss, src_entity->name,
> +					       sink_entity->name,
> +					       ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * camss_link_entities_csiphy - 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_csiphy(struct camss *camss)
> +{
> +	int i, j;
>   	int ret;
>   
>   	for (i = 0; i < camss->res->csiphy_num; i++) {
> @@ -1858,81 +1927,77 @@ static int camss_link_entities(struct camss *camss)
>   						    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);
> +				camss_link_err(camss,
> +					       camss->csiphy[i].subdev.entity.name,
> +					       camss->csid[j].subdev.entity.name,
> +					       ret);
>   				return ret;
>   			}
>   		}
>   	}
>   
> -	if (camss->ispif) {
> -		for (i = 0; i < camss->res->csid_num; i++) {
> -			for (j = 0; j < camss->ispif->line_num; j++) {
> -				ret = media_create_pad_link(&camss->csid[i].subdev.entity,
> -							    MSM_CSID_PAD_SRC,
> -							    &camss->ispif->line[j].subdev.entity,
> -							    MSM_ISPIF_PAD_SINK,
> +	return 0;
> +}
> +
> +/*
> + * camss_link_entities_ispif - 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_ispif(struct camss *camss)
> +{
> +	int i, j, k;
> +	int ret;
> +
> +	for (i = 0; i < camss->ispif->line_num; i++) {
> +		for (k = 0; k < camss->res->vfe_num; k++) {
> +			for (j = 0; j < camss->vfe[k].res->line_num; j++) {
> +				struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
> +				struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> +
> +				ret = media_create_pad_link(&ispif->entity,
> +							    MSM_ISPIF_PAD_SRC,
> +							    &vfe->entity,
> +							    MSM_VFE_PAD_SINK,
>   							    0);
>   				if (ret < 0) {
> -					dev_err(camss->dev,
> -						"Failed to link %s->%s entities: %d\n",
> -						camss->csid[i].subdev.entity.name,
> -						camss->ispif->line[j].subdev.entity.name,
> -						ret);
> +					camss_link_err(camss, ispif->entity.name,
> +						       vfe->entity.name,
> +						       ret);
>   					return ret;
>   				}
>   			}
>   		}
> -
> -		for (i = 0; i < camss->ispif->line_num; i++)
> -			for (k = 0; k < camss->res->vfe_num; k++)
> -				for (j = 0; j < camss->vfe[k].res->line_num; j++) {
> -					struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
> -					struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> -
> -					ret = media_create_pad_link(&ispif->entity,
> -								    MSM_ISPIF_PAD_SRC,
> -								    &vfe->entity,
> -								    MSM_VFE_PAD_SINK,
> -								    0);
> -					if (ret < 0) {
> -						dev_err(camss->dev,
> -							"Failed to link %s->%s entities: %d\n",
> -							ispif->entity.name,
> -							vfe->entity.name,
> -							ret);
> -						return ret;
> -					}
> -				}
> -	} else {
> -		for (i = 0; i < camss->res->csid_num; i++)
> -			for (k = 0; k < camss->res->vfe_num; k++)
> -				for (j = 0; j < camss->vfe[k].res->line_num; j++) {
> -					struct v4l2_subdev *csid = &camss->csid[i].subdev;
> -					struct v4l2_subdev *vfe = &camss->vfe[k].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;
>   }
>   
> +/*
> + * camss_link_entities - 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)
> +{
> +	int ret;
> +
> +	ret = camss_link_entities_csiphy(camss);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = camss_link_entities_csid(camss);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (camss->ispif)
> +		ret = camss_link_entities_ispif(camss);

Since there is an expectation that the change is non-functional, please
keep the logic unmodified.

	if (camss->ispif) {
		ret = camss_link_entities_ispif(camss);
	} else {
		
	}

> +	return ret;
> +}
> +
>   /*
>    * camss_register_entities - Register subdev nodes and create links
>    * @camss: CAMSS device
> @@ -2073,9 +2138,10 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
>   				input, MSM_CSIPHY_PAD_SINK,
>   				MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
>   			if (ret < 0) {
> -				dev_err(camss->dev,
> -					"Failed to link %s->%s entities: %d\n",
> -					sensor->name, input->name, ret);
> +				camss_link_err(camss,
> +					       sensor->name,
> +					       input->name,
> +					       ret);
>   				return ret;
>   			}
>   		}
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 0ce84fcbbd25..2086000ad5c1 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -160,5 +160,9 @@ void camss_pm_domain_off(struct camss *camss, int id);
>   int camss_vfe_get(struct camss *camss, int id);
>   void camss_vfe_put(struct camss *camss, int id);
>   void camss_delete(struct camss *camss);
> +void camss_link_err(struct camss *camss,
> +		    const char *src_name,
> +		    const char *sink_name,
> +		    int ret);

Please don't add this into the header file. Drop it.

>   #endif /* QC_MSM_CAMSS_H */

In fact I didn't find the change as a simplification, because now there are
more if-branches apparently. Is there a reason why the change is needed?

--
Best wishes,
Vladimir