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

[Paul Kocialkowski]

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