Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Steve,

On Tue, Apr 14, 2020 at 04:50:55PM -0700, Steve Longerbeam wrote:
> On 4/14/20 4:20 PM, Sakari Ailus wrote:
> > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote:
> >> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
> >>> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
> >>> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
> >>> maps port numbers and pad indexes 1:1.
> >>>
> >>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> >>> ---
> >>>   drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
> >>>   1 file changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> >>> index fdd763587e6c..8500207e5ea9 100644
> >>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> >>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> >>> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
> >>>   				      640, 480, 0, V4L2_FIELD_NONE, NULL);
> >>>   }
> >>>   
> >>> +static int csi2_get_fwnode_pad(struct media_entity *entity,
> >>> +			       struct fwnode_endpoint *endpoint)
> >>> +{
> >>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >>> +	struct csi2_dev *csi2 = sd_to_dev(sd);
> >>> +	struct fwnode_handle *csi2_ep;
> >>> +
> >>> +	/*
> >>> +	 * If the endpoint is one of ours, return the endpoint's port
> >>> +	 * number. This device maps port numbers and pad indexes 1:1.
> >>> +	 */
> >>> +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
> >>> +		if (endpoint->local_fwnode == csi2_ep) {
> >>> +			struct fwnode_endpoint fwep;
> >>> +			int ret;
> >>> +
> >>> +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
> >>> +
> >>> +			fwnode_handle_put(csi2_ep);
> >>> +
> >>> +			return ret ? ret : fwep.port;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return -ENXIO;
> >>> +}
> >>
> >> As the 1:1 mapping is the common case, would it make sense to modify
> >> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
> >> set ? The current behaviour is to return the first pad that matches the
> >
> > I also think this would make sense.
> 
> What do you think about https://patchwork.linuxtv.org/patch/60312/ ? I'm 
> planning to resurrect it for v5.

The approach looks good to me.

> >> requested direction, which could be preserved as a second-level fallback
> >> if the 1:1 mapping doesn't give the right direction (but I'm not sure
> >> there's a use case for that, the 1:1 mapping seems to be all we need if
> >> there's no specific .get_fwnode_pad implementation).
> >
> > I believe at least the smiapp driver breaks if you do that, so the current
> > behaviour should be retained (secondary to the 1:1 mapping).

-- 
Regards,

Laurent Pinchart