Hi Sakari,
On Fri 04 Mar 22, 00:43, Sakari Ailus wrote:
> Hi Paul,
>
> On Wed, Mar 02, 2022 at 03:59:50PM +0100, Paul Kocialkowski wrote:
> > > > +static int
> > > > +sun6i_csi_bridge_notifier_bound(struct v4l2_async_notifier *notifier,
> > > > + struct v4l2_subdev *remote_subdev,
> > > > + struct v4l2_async_subdev *async_subdev)
> > > > +{
> > > > + struct sun6i_csi_device *csi_dev =
> > > > + container_of(notifier, struct sun6i_csi_device,
> > > > + bridge.notifier);
> > > > + struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> > > > + struct sun6i_csi_bridge_source *source = NULL;
> > > > + struct fwnode_handle *fwnode = dev_fwnode(csi_dev->dev);
> > > > + struct fwnode_handle *handle = NULL;
> > > > + bool enabled;
> > > > + int ret;
> > > > +
> > > > + while ((handle = fwnode_graph_get_next_endpoint(fwnode, handle))) {
> > >
> > > I'd instead store the information you need here in struct sun6i_csi_bridge.
> > > You could remove the loop here.
> >
> > Is there a different method for matching a remote subdev to a local port?
> > The rationale here is that I need the handle for fwnode_graph_parse_endpoint
> > but cannot get that handle from the remote subdev's fwnode pointer directly.
>
> You generally shouldn't try to match fwnodes here as the V4L2 async
> framework has already done that job. This information can be found behind
> the async_subdev pointer.
>
> See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2-main.c for an example.
Thanks for the feedback, I'll look into that.
> >
> > > > + struct fwnode_endpoint endpoint = { 0 };
> > > > + struct fwnode_handle *remote_fwnode;
> > > > +
> > > > + remote_fwnode = fwnode_graph_get_remote_port_parent(handle);
> > > > + if (!remote_fwnode)
> > > > + continue;
> > > > +
> > > > + if (remote_fwnode != remote_subdev->fwnode)
> > > > + goto next;
> > > > +
> > > > + ret = fwnode_graph_parse_endpoint(handle, &endpoint);
> > > > + if (ret < 0)
> > > > + goto next;
> > > > +
> > > > + switch (endpoint.port) {
> > > > + case SUN6I_CSI_PORT_PARALLEL:
> > > > + source = &bridge->source_parallel;
> > > > + enabled = true;
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > +next:
> > > > + fwnode_handle_put(remote_fwnode);
> > > > + }
> > > > +
> > > > + if (!source)
> > > > + return -EINVAL;
> > > > +
> > > > + source->subdev = remote_subdev;
> > > > +
> > > > + return sun6i_csi_bridge_link(csi_dev, SUN6I_CSI_BRIDGE_PAD_SINK,
> > > > + remote_subdev, enabled);
> > > > +}
> > > > +
> > > > +static int
> > > > +sun6i_csi_bridge_notifier_complete(struct v4l2_async_notifier *notifier)
> > > > +{
> > > > + struct sun6i_csi_device *csi_dev =
> > > > + container_of(notifier, struct sun6i_csi_device,
> > > > + bridge.notifier);
> > > > +
> > > > + return sun6i_csi_v4l2_complete(csi_dev);
> > >
> > > You could call v4l2_device_register_subdev_nodes() here.
> >
> > That's definitely what sun6i_csi_v4l2_complete does (the diff is probably not
> > very clear). Note that the wrapper is extended later on to register the capture
> > video device for the no-isp path.
>
> I could be missing something... Do you need to call
> sun6i_csi_v4l2_complete() in multiple places or not? If not, then I think
> it'd be probably better to just move the code here.
No this is only called here so I guess we can avoid it entirely.
Thanks,
Paul
> >
> > Maybe the capture registration could be kept in sun6i_csi_probe for the non-isp
> > path and then the wrapper wouldn't be needed. I don't mind either way.
>
> --
> Kind regards,
>
> Sakari Ailus
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
