Hi Laurent, Thank you for the review. On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which > > virtual channel should be processed from the four available VCs. To > > retrieve this information from the connected subdevice, the > > .get_frame_desc() function is called. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > --- > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index bbf4674f888d..6101a070e785 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > > spin_unlock_irqrestore(&cru->qlock, flags); > > } > > > > +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru) > > +{ > > + struct v4l2_mbus_frame_desc fd = { }; > > + struct media_pad *pad; > > + int ret; > > + > > + pad = media_pad_remote_pad_unique(&cru->ip.pads[1]); > > It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding > the pad number. That would require moving rzg2l_csi2_pads to the shared > header. You can do that on top. > With the below comment we dont need to move rzg2l_csi2_pads into the shared header. > An now that I've said that, is it really the source pad you need here ? > Ouch you are right. > > + if (IS_ERR(pad)) > > + return PTR_ERR(pad); > > Can this happen, or would the pipeline fail to validate ? I think you > can set the MUST_CONNECT flag on the sink pad, then you'll have a > guarantee something will be connected. > After adding the MUST_CONNECT flag, I wouldn't need the above media_pad_remote_pad_unique()... > > + > > + ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc, > > + pad->index, &fd); ... and here I can use '0' instead or do you prefer RZG2L_CRU_IP_SINK (I say because we are calling into remote subdev of IP which is CSI so the RZG2L_CRU_IP_SINK wont make sense)? > > + if (ret < 0 && ret != -ENOIOCTLCMD) > > Printing an error message would help debugging. > OK, I will add. > > + return ret; > > + /* If remote subdev does not implement .get_frame_desc default to VC0. */ > > + if (ret == -ENOIOCTLCMD) > > + return 0; > > + > > + if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > An error message would help here too I think. > OK, I will add. > > + return -EINVAL; > > + > > + return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0; > > I think you should return an error if fd.num_entries is 0, that > shouldn't happen. > OK, I will add. > > +} > > + > > int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru) > > { > > struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru); > > unsigned long flags; > > int ret; > > > > + ret = rzg2l_cru_get_virtual_channel(cru); > > + if (ret < 0) > > + return ret; > > + cru->csi.channel = ret; > > How about passing the value to the function that needs it, instead of > storing it in cru->csi.channel ? You can do that on top and drop the > csi.channel field. > OK, let me check if this can be done. Cheers, Prabhakar