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

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

Hi Sakari,

On Wed, Apr 15, 2020 at 02:20:36AM +0300, 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.
> 
> > 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).

Shouldn't the smiapp driver get a .get_fwnode_pad() implementation then
? It could be argued that the smiapp use case is also quite common, and
would deserve handling in the core, as a second fallback. It makes me
wonder if we should really try to have a core fallback in the first
place then, we could instead create helpers for the common cases that
would be used by drivers as their .get_fwnode_pad() implementation,
forcing each driver to think about which version matches its needs best.

-- 
Regards,

Laurent Pinchart