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@xxxxxxxxx>
> > > ---
> > > 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
