Hejssan!!
On Sat, Dec 02, 2017 at 03:50:21PM +0100, Niklas Söderlund wrote:
> Hej Sakari,
>
> Thanks for your feedback.
>
> On 2017-12-02 16:05:08 +0200, Sakari Ailus wrote:
> > Hejssan,
> >
> > On Sat, Dec 02, 2017 at 12:08:21PM +0100, Niklas Söderlund wrote:
> > ...
> > > > > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > > > > +{
> > > > > + struct device_node *ep;
> > > > > + struct v4l2_fwnode_endpoint v4l2_ep;
> > > > > + int ret;
> > > > > +
> > > > > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > > > > + if (!ep) {
> > > > > + dev_err(priv->dev, "Not connected to subdevice\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep),
> > > > > &v4l2_ep);
> > > > > + if (ret) {
> > > > > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> > > > > + of_node_put(ep);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + priv->remote.match.fwnode.fwnode =
> > > > > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > >
> > > > To continue the discussion from v10 --- how does this work? The V4L2
> > > > async
> > > > framework does still matching for the node of the device, not the
> > > > endpoint.
> > > >
> > > > My suggestion is to handle this in match_fwnode until all drivers have
> > > > been
> > > > converted. The V4L2 fwnode helper should be changed as well, then you
> > > > could
> > > > use it here, too.
> > >
> > > I agree that the V4L2 async framework should be changed to work with
> > > endpoints and not the node of the device. I also agree on how this
> > > change should be introduced.
> > >
> > > But I don't feel that this change of the framework needs to block this
> > > patch-set. Once the framework is updated to work with endpoints it
> > > should be a trivial patch to change rcar-csi2 to use the new helper. Do
> > > you agree whit this or do you feel that this patch-set should depend on
> > > such change of the framework?
> >
> > Without changes to the framework, I don't think this would work since the
> > async framework (or individual drivers) will assign the device's fwnode,
> > not that of the endpoint, to the fwnode against which to match the async
> > sub-device.
> >
> > Therefore you'll need these changes for this driver to work. Or if you
> > introduce a sub-device driver that uses endpoints as well, then we have two
> > non-interoperable sets of ISP (or bridge) and sub-device drivers. That'd be
> > quite undesirable.
>
> Such a driver is already upstream, the adv748x driver register its
> subdevices using endpoints rather then the node of the device itself.
>
> <snip from adv748x-csi2.c in v4.15-rc1>
> int adv748x_csi2_init(...)
> {
>
> ...
>
> /* Ensure that matching is based upon the endpoint fwnodes */
> tx->sd.fwnode = of_fwnode_handle(ep);
>
> ...
> }
> </snip>
>
> A related patch to when the adv748x driver was unstreamed where
> 'v4l2-async: Match parent devices' by Kieran to make this change in
> behavior not to cause the non-interoperable sets your mention. It seems
> however that that change have fallen thru the cracks.
Please see the other patch I just sent you (plus linux-media).
>
> >
> > Or, if you don't care whether it'd work with your use case right now, you
> > could use the helper function without changes. :-)
>
> The adv748x is the primary use-case for the rcar-csi2 driver at the
> moment. So I must either keep this workaround in the driver or make this
> patch-set depend on future framework fixes. I would prefer to be able to
> use the helper as it makes the driver less complex. At the same time I
> don't want yet another framework change as a blocker for this patch-set
> :-)
>
> Once I'm back from my short vacation I will give the framework update a
> try and if it turns out OK I will make this patch-set dependent on those
> changes and squash in my patch which converts rcar-csi2 to use the
> helper which is already done in preparation of future framework updates.
>
> If it turns out the changes needed are complex or get stuck in review I
> would prefer if it was possible to move forward with the workaround in
> the driver for now and move it to the helper once it's available. Do
> this sound like a agreeable plan to you?
Yes, but I think changing the framework should be fine.
--
Sakari Ailus
e-mail: [email protected]
