| Message ID | 20241112133846.2397017-1-quic_vikramsa@quicinc.com |
|---|---|
| Headers | show |
| Series | media: qcom: camss: Re-structure camss_link_entities | expand |
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
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
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. Changes in V2: - Declared variables in reverse christmas tree order. - Functionally decomposed link error message. - Link to v1: https://lore.kernel.org/linux-arm-msm/20241111173845.1773553-1-quic_vikramsa@quicinc.com/ To: Robert Foss <rfoss@kernel.org> To: Todor Tomov <todor.too@gmail.com> To: Bryan O'Donoghue <bryan.odonoghue@linaro.org> To: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: linux-arm-msm@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> Vikram Sharma (1): media: qcom: camss: Restructure camss_link_entities 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(-)