Re: [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with port management

[Sakari Ailus]

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.

> 
> > > +		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.

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