* Re: [PATCH v2 1/1] media: qcom: camss: Restructure camss_link_entities
2024-11-12 13:38 ` [PATCH v2 1/1] media: qcom: camss: Restructure camss_link_entities Vikram Sharma
@ 2024-11-12 23:34 ` Bryan O'Donoghue
2024-11-13 0:33 ` Vladimir Zapolskiy
1 sibling, 0 replies; 4+ messages in thread
From: Bryan O'Donoghue @ 2024-11-12 23:34 UTC (permalink / raw)
To: Vikram Sharma, rfoss, todor.too, krzk+dt
Cc: linux-media, linux-arm-msm, linux-kernel, kernel
On 12/11/2024 13: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);
So you're doing the right thing reusing camss_link_err here however
1. The commit log no-longer matches
2. I generally suggest patches should be as granular as possible
3. That means if you want to use camss_link_err in camss-vfe.c
and BTW I think that's, correct then
a) Refactor this to be two patches
b) First patch is about reducing the repitious string and introducing
the reduction in camss.c and camss-vfe.c
c) The second patch is about restructiring link_entities in camss.c
Basically this patch now does two things and instead of havin those two
things contained in the one patch, you should split those two things
into two separate patches.
---
bod
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 1/1] media: qcom: camss: Restructure camss_link_entities
2024-11-12 13:38 ` [PATCH v2 1/1] media: qcom: camss: Restructure camss_link_entities Vikram Sharma
2024-11-12 23:34 ` Bryan O'Donoghue
@ 2024-11-13 0:33 ` Vladimir Zapolskiy
1 sibling, 0 replies; 4+ messages in thread
From: Vladimir Zapolskiy @ 2024-11-13 0:33 UTC (permalink / raw)
To: Vikram Sharma, rfoss, todor.too, bryan.odonoghue, krzk+dt
Cc: linux-media, linux-arm-msm, linux-kernel, kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread