Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote:
> 
> 
> On 05/08/2018 03:28 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > Again, sorry about the delay. This thread got buried in my inbox. :-(
> > Please see my reply below.
> > 
> > On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
> > > 
> > > On 04/23/2018 12:14 AM, Sakari Ailus wrote:
> > > > Hi Steve,
> > > > 
> > > > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
> > > > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
> > > > > for parsing a sub-device's fwnode port endpoints for connected remote
> > > > > sub-devices, registering a sub-device notifier, and then registering
> > > > > the sub-device itself.
> > > > > 
> > > > > Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>
> > > > > ---
> > > > > Changes since v2:
> > > > > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
> > > > >     to put device.
> > > > > Changes since v1:
> > > > > - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
> > > > >     'struct v4l2_subdev' declaration.
> > > > > ---
> > > > >    drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
> > > > >    include/media/v4l2-fwnode.h           |  43 +++++++++++++++
> > > > >    2 files changed, 144 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > index 99198b9..d42024d 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
> > > > > +int v4l2_async_register_fwnode_subdev(
> > > > > +	struct v4l2_subdev *sd, size_t asd_struct_size,
> > > > > +	unsigned int *ports, unsigned int num_ports,
> > > > > +	int (*parse_endpoint)(struct device *dev,
> > > > > +			      struct v4l2_fwnode_endpoint *vep,
> > > > > +			      struct v4l2_async_subdev *asd))
> > > > > +{
> > > > > +	struct v4l2_async_notifier *notifier;
> > > > > +	struct device *dev = sd->dev;
> > > > > +	struct fwnode_handle *fwnode;
> > > > > +	unsigned int subdev_port;
> > > > > +	bool is_port;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (WARN_ON(!dev))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	fwnode = dev_fwnode(dev);
> > > > > +	if (!fwnode_device_is_available(fwnode))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	is_port = (is_of_node(fwnode) &&
> > > > > +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
> > > > What's the intent of this and the code below? You may not parse the graph
> > > > data structure here, it should be done in the actual firmware
> > > > implementation instead.
> > > The i.MX6 CSI sub-device registers itself from a port fwnode, so
> > > the intent of the is_port code below is to support the i.MX6 CSI.
> > > 
> > > I can remove the is_port checks, but it means
> > > v4l2_async_register_fwnode_subdev() won't be usable by the CSI
> > > sub-device.
> > This won't scale.
> 
> The vast majority of sub-devices register themselves as
> port parent nodes. So for now at least, I think
> v4l2_async_register_fwnode_subdev() could be useful to many
> platforms.
> 
> >   Instead, I think we'd need to separate registering
> > sub-devices (through async sub-devices) and binding them with the driver
> > that registered the notifier. Or at least change how that process works: a
> > single sub-device can well be bound to multiple notifiers,
> 
> Ok, that is certainly not the case now, a sub-device can only
> be bound to a single notifier.
> 
> >   or multiple
> > times to the same notifier while it may be registered only once.
> 
> Anyway, this is a future generalization if I understand you
> correctly. Not something to deal with here.
> 
> > 
> > > > Also, sub-devices generally do not match ports.
> > > Yes that's generally true, sub-devices generally match to port parent
> > > nodes. But I do know of one other sub-device that buck that trend.
> > > The ADV748x CSI-2 output sub-devices match against endpoint nodes.
> > Endpoints, yes, but not ports.
> 
> Well, the imx CSI registers from a port node.

I looked at the imx driver and it appears to be parsing DT without much
help from the frameworks; graph or V4L2 fwnode. Is there a reason to do so,
other than it's a staging driver? :-)

Do you happen to have any DT source (or bindings) for this? It'd help to
understand what is the aim here.

> 
> > 
> > > >    How sub-devices generally
> > > > correspond to fwnodes is up to the device.
> > > What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
> > > and a v4l2_async_register_endpoint_fwnode_subdev() to support such
> > > sub-devices?
> > The endpoint is more specific than a port, so why the port and not the
> > endpoint?
> 
> Do you mean there should be a
> v4l2_async_register_endpoint_fwnode_subdev() but not
> v4l2_async_register_endpoint_port_subdev()?
> 
> Steve
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux