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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YiHVHM53GUQ1jxco@aptenodytes>
Date:   Fri, 4 Mar 2022 10:00:12 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
        linux-clk@...r.kernel.org, linux-staging@...ts.linux.dev,
        Yong Deng <yong.deng@...ewell.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Helen Koike <helen.koike@...labora.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with
 port management

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)