Re: [PATCH 2/2] v4l: fwnode: Support generic parsing of graph endpoints in V4L2

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

 



On 2017-06-29 12:57:55 +0300, Sakari Ailus wrote:
> On Thu, Jun 29, 2017 at 11:02:03AM +0200, Niklas Söderlund wrote:
> > Hi Sakari,
> > 
> > Thanks for your patch.
> > 
> > On 2017-06-29 10:30:10 +0300, Sakari Ailus wrote:
> > > The current practice is that drivers iterate over their endpoints and
> > > parse each endpoint separately. This is very similar in a number of
> > > drivers, implement a generic function for the job. Driver specific matters
> > > can be taken into account in the driver specific callback.
> > > 
> > > Convert the omap3isp as an example.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/media/platform/omap3isp/isp.c | 91 ++++++++++-----------------------
> > >  drivers/media/platform/omap3isp/isp.h |  3 --
> > >  drivers/media/v4l2-core/v4l2-fwnode.c | 94 +++++++++++++++++++++++++++++++++++
> > >  include/media/v4l2-async.h            |  4 +-
> > >  include/media/v4l2-fwnode.h           |  9 ++++
> > >  5 files changed, 132 insertions(+), 69 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> > > index 9df64c1..9ccf883 100644
> > > --- a/drivers/media/platform/omap3isp/isp.c
> > > +++ b/drivers/media/platform/omap3isp/isp.c
> > > @@ -2008,43 +2008,42 @@ enum isp_of_phy {
> > >  	ISP_OF_PHY_CSIPHY2,
> > >  };
> > >  
> > > -static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> > > -			    struct isp_async_subdev *isd)
> > > +static int isp_fwnode_parse(struct device *dev,
> > > +			    struct v4l2_fwnode_endpoint *vep,
> > > +			    struct v4l2_async_subdev *asd)
> > >  {
> > > +	struct isp_async_subdev *isd =
> > > +		container_of(asd, struct isp_async_subdev, asd);
> > >  	struct isp_bus_cfg *buscfg = &isd->bus;
> > > -	struct v4l2_fwnode_endpoint vep;
> > >  	unsigned int i;
> > > -	int ret;
> > > -
> > > -	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> > > -	if (ret)
> > > -		return ret;
> > >  
> > >  	dev_dbg(dev, "parsing endpoint %s, interface %u\n",
> > > -		to_of_node(fwnode)->full_name, vep.base.port);
> > > +		to_of_node(vep->base.local_fwnode)->full_name, vep->base.port);
> > >  
> > > -	switch (vep.base.port) {
> > > +	switch (vep->base.port) {
> > >  	case ISP_OF_PHY_PARALLEL:
> > >  		buscfg->interface = ISP_INTERFACE_PARALLEL;
> > >  		buscfg->bus.parallel.data_lane_shift =
> > > -			vep.bus.parallel.data_shift;
> > > +			vep->bus.parallel.data_shift;
> > >  		buscfg->bus.parallel.clk_pol =
> > > -			!!(vep.bus.parallel.flags
> > > +			!!(vep->bus.parallel.flags
> > >  			   & V4L2_MBUS_PCLK_SAMPLE_FALLING);
> > >  		buscfg->bus.parallel.hs_pol =
> > > -			!!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
> > > +			!!(vep->bus.parallel.flags &
> > > +			   V4L2_MBUS_VSYNC_ACTIVE_LOW);
> > >  		buscfg->bus.parallel.vs_pol =
> > > -			!!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > > +			!!(vep->bus.parallel.flags &
> > > +			   V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > >  		buscfg->bus.parallel.fld_pol =
> > > -			!!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> > > +			!!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> > >  		buscfg->bus.parallel.data_pol =
> > > -			!!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> > > +			!!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> > >  		break;
> > >  
> > >  	case ISP_OF_PHY_CSIPHY1:
> > >  	case ISP_OF_PHY_CSIPHY2:
> > >  		/* FIXME: always assume CSI-2 for now. */
> > > -		switch (vep.base.port) {
> > > +		switch (vep->base.port) {
> > >  		case ISP_OF_PHY_CSIPHY1:
> > >  			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> > >  			break;
> > > @@ -2052,18 +2051,19 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> > >  			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> > >  			break;
> > >  		}
> > > -		buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
> > > +		buscfg->bus.csi2.lanecfg.clk.pos =
> > > +			vep->bus.mipi_csi2.clock_lane;
> > >  		buscfg->bus.csi2.lanecfg.clk.pol =
> > > -			vep.bus.mipi_csi2.lane_polarities[0];
> > > +			vep->bus.mipi_csi2.lane_polarities[0];
> > >  		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> > >  			buscfg->bus.csi2.lanecfg.clk.pol,
> > >  			buscfg->bus.csi2.lanecfg.clk.pos);
> > >  
> > >  		for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> > >  			buscfg->bus.csi2.lanecfg.data[i].pos =
> > > -				vep.bus.mipi_csi2.data_lanes[i];
> > > +				vep->bus.mipi_csi2.data_lanes[i];
> > >  			buscfg->bus.csi2.lanecfg.data[i].pol =
> > > -				vep.bus.mipi_csi2.lane_polarities[i + 1];
> > > +				vep->bus.mipi_csi2.lane_polarities[i + 1];
> > >  			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> > >  				buscfg->bus.csi2.lanecfg.data[i].pol,
> > >  				buscfg->bus.csi2.lanecfg.data[i].pos);
> > > @@ -2079,55 +2079,14 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> > >  
> > >  	default:
> > >  		dev_warn(dev, "%s: invalid interface %u\n",
> > > -			 to_of_node(fwnode)->full_name, vep.base.port);
> > > +			 to_of_node(vep->base.local_fwnode)->full_name,
> > > +			 vep->base.port);
> > >  		break;
> > >  	}
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > -static int isp_fwnodes_parse(struct device *dev,
> > > -			     struct v4l2_async_notifier *notifier)
> > > -{
> > > -	struct fwnode_handle *fwnode = NULL;
> > > -
> > > -	notifier->subdevs = devm_kcalloc(
> > > -		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
> > > -	if (!notifier->subdevs)
> > > -		return -ENOMEM;
> > > -
> > > -	while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
> > > -	       (fwnode = fwnode_graph_get_next_endpoint(
> > > -			of_fwnode_handle(dev->of_node), fwnode))) {
> > > -		struct isp_async_subdev *isd;
> > > -
> > > -		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> > > -		if (!isd)
> > > -			goto error;
> > > -
> > > -		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> > > -
> > > -		if (isp_fwnode_parse(dev, fwnode, isd))
> > > -			goto error;
> > > -
> > > -		isd->asd.match.fwnode.fwnode =
> > > -			fwnode_graph_get_remote_port_parent(fwnode);
> > > -		if (!isd->asd.match.fwnode.fwnode) {
> > > -			dev_warn(dev, "bad remote port parent\n");
> > > -			goto error;
> > > -		}
> > > -
> > > -		isd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > -		notifier->num_subdevs++;
> > > -	}
> > > -
> > > -	return notifier->num_subdevs;
> > > -
> > > -error:
> > > -	fwnode_handle_put(fwnode);
> > > -	return -EINVAL;
> > > -}
> > > -
> > >  static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> > >  				     struct v4l2_subdev *subdev,
> > >  				     struct v4l2_async_subdev *asd)
> > > @@ -2210,7 +2169,9 @@ static int isp_probe(struct platform_device *pdev)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
> > > +	ret = v4l2_fwnode_endpoints_parse(
> > > +		&pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
> > > +		isp_fwnode_parse);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> > > index 2f2ae60..a852c11 100644
> > > --- a/drivers/media/platform/omap3isp/isp.h
> > > +++ b/drivers/media/platform/omap3isp/isp.h
> > > @@ -220,9 +220,6 @@ struct isp_device {
> > >  
> > >  	unsigned int sbl_resources;
> > >  	unsigned int subclk_resources;
> > > -
> > > -#define ISP_MAX_SUBDEVS		8
> > > -	struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
> > >  };
> > >  
> > >  struct isp_async_subdev {
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index 0ec6c14..b35d525 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/string.h>
> > >  #include <linux/types.h>
> > >  
> > > +#include <media/v4l2-async.h>
> > >  #include <media/v4l2-fwnode.h>
> > >  
> > >  static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle *fwnode,
> > > @@ -339,6 +340,99 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > >  
> > > +static int notifier_realloc(struct device *dev,
> > > +			    struct v4l2_async_notifier *notifier,
> > > +			    unsigned int max_subdevs)
> > > +{
> > > +	struct v4l2_async_subdev **subdevs;
> > > +	unsigned int i;
> > > +
> > > +	if (max_subdevs <= notifier->max_subdevs)
> > > +		return 0;
> > > +
> > > +	subdevs = devm_kcalloc(
> > > +		dev, max_subdevs, sizeof(*notifier->subdevs), GFP_KERNEL);
> > > +	if (!subdevs)
> > > +		return -ENOMEM;
> > > +
> > > +	if (notifier->subdevs) {
> > > +		for (i = 0; i < notifier->num_subdevs; i++)
> > > +			subdevs[i] = notifier->subdevs[i];
> > > +
> > > +		devm_kfree(dev, notifier->subdevs);
> > > +	}
> > > +
> > > +	notifier->subdevs = subdevs;
> > > +	notifier->max_subdevs = max_subdevs;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int v4l2_fwnode_endpoints_parse(
> > > +	struct device *dev, struct v4l2_async_notifier *notifier,
> > > +	size_t asd_struct_size,
> > > +	int (*parse_single)(struct device *dev,
> > > +			    struct v4l2_fwnode_endpoint *vep,
> > > +			    struct v4l2_async_subdev *asd))
> > > +{
> > > +	struct fwnode_handle *fwnode = NULL;
> > > +	unsigned int max_subdevs = notifier->max_subdevs;
> > > +	int ret;
> > > +
> > > +	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > > +		return -EINVAL;
> > > +
> > > +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> > > +							fwnode)))
> > > +		max_subdevs++;
> > > +
> > > +	ret = notifier_realloc(dev, notifier, max_subdevs);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > > +				     dev_fwnode(dev), fwnode)) &&
> > > +		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
> > > +		) {
> > > +		struct v4l2_fwnode_endpoint *vep;
> > > +		struct v4l2_async_subdev *asd;
> > > +
> > > +		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> > > +		if (!asd) {
> > > +			ret = -ENOMEM;
> > > +			goto error;
> > > +		}
> > > +
> > > +		notifier->subdevs[notifier->num_subdevs] = asd;
> > > +
> > > +		/* Ignore endpoints the parsing of which failed. */
> > > +		vep = v4l2_fwnode_endpoint_alloc_parse(fwnode);
> > > +		if (IS_ERR(vep))
> > > +			continue;
> > > +
> > > +		ret = parse_single(dev, vep, asd);
> > > +		v4l2_fwnode_endpoint_free(vep);
> > > +		if (ret)
> > > +			goto error;
> > 
> > First off I think this is a good step in the right direction to create 
> > core functions for this task.
> > 
> > However while reading this I got to think about a use-case I have with 
> > the Renesas R-Car VIN and CSI-2 drivers where I would not be able to use 
> > this helper. I have previously posted a patch-set to introduce 
> > incremental async, see [1]. If that gets picked-up not only the video 
> > device driver can have use of this helper but subdevices drivers.  As 
> > the subdevice driver would also need to pars its DT node to search for 
> > subdevices to add to it's own subnotifier. In this case this helper wont 
> > suffice, I think.
> > 
> > Since the subdevice DT node will contain endpoints pointing to both the 
> > subdevice which the incremental async notifier would like to find and 
> > endpoints pointing back to the root video device node it self.
> > 
> > In my use-case for Renesas R-Car Gen3 VIN and CSI-2 DT (not yet accepted 
> > upstream) I have solved this by having the CSI-2 DT node having two port 
> > nodes, port0 and port1. In port0 endpoints describing the 'upstream' 
> > video source from the CSI-2 point of view are defined (these should be 
> > added to the subnotifier). In port1 endpoints describing connections 
> > back to the VIN video devices are described. Currently the CSI-2 driver 
> > simply only looks for endpoints in the port0 node to add to its 
> > subnotifier, and I don't think this would be possible with this helper?
> 
> What we could also do is to add a variant of this that parses the endpoints
> by the port / endpoint IDs. Then you don't need to have a cumbersome way to
> skip parsing of some ports --- you would have allocated the memory and
> parsed the port for nothing by the time you call the callback anyway.
> 
> That would seem to be a better approach in general. I remember Rob was
> pushing this for DRM as it greatly simplified driver code there. But then
> the DT bindings have to be written accordingly: in current bindings the
> endpoint IDs are mostly undefined.
> 
> We may still need to take extra measures to support cases where there are
> multiple links between two sub-devices.
> 
> What do you think?

A variant of this that as you suggest parses by port I think make sens.  
I'm not sure how one would go about creating a simple interface which 
would support parsing by port+endpoint IDs, but if it's possible that 
would be also be nice.

Another option is to instead of passing a struct device pass a struct 
fwnode_handle which would act as the root search path. That way one 
could pass a fwnode_handle to a specific port node and just iterate over 
all endpoints in that port. But that way the driver side implementation 
becomes more complex since it needs to lookup the fwnode_handle so I'm 
not sure I think that is the best solution.

So yes I think your suggestion of adding a variant which takes port 
and/or port+endpoint IDs into account to be the best solution.

> 
> Cc Rob + devicetree list as well.
> 
> > 
> > Perhaps this patch can be modified so that en error code from the 
> > callback parse_single() could be taken in to account when making the 
> > decision if the parsed endpoint should be added to the notifiers 
> > list of subdevices?
> > 
> > > +
> > > +		asd->match.fwnode.fwnode =
> > > +			fwnode_graph_get_remote_port_parent(fwnode);
> > > +		if (!asd->match.fwnode.fwnode) {
> > > +			dev_warn(dev, "bad remote port parent\n");
> > > +			ret = -EINVAL;
> > > +			goto error;
> > > +		}
> > > +
> > > +		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > +		notifier->num_subdevs++;
> > > +	}
> > > +
> > > +error:
> > > +	fwnode_handle_put(fwnode);
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> > > +
> > >  MODULE_LICENSE("GPL");
> > >  MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> > >  MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
> > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > index c69d8c8..067f368 100644
> > > --- a/include/media/v4l2-async.h
> > > +++ b/include/media/v4l2-async.h
> > > @@ -78,7 +78,8 @@ struct v4l2_async_subdev {
> > >  /**
> > >   * struct v4l2_async_notifier - v4l2_device notifier data
> > >   *
> > > - * @num_subdevs: number of subdevices
> > > + * @num_subdevs: number of subdevices used in subdevs array
> > > + * @max_subdevs: number of subdevices allocated in subdevs array
> > >   * @subdevs:	array of pointers to subdevice descriptors
> > >   * @v4l2_dev:	pointer to struct v4l2_device
> > >   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
> > > @@ -90,6 +91,7 @@ struct v4l2_async_subdev {
> > >   */
> > >  struct v4l2_async_notifier {
> > >  	unsigned int num_subdevs;
> > > +	unsigned int max_subdevs;
> > >  	struct v4l2_async_subdev **subdevs;
> > >  	struct v4l2_device *v4l2_dev;
> > >  	struct list_head waiting;
> > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > > index ecc1233..ff489f7 100644
> > > --- a/include/media/v4l2-fwnode.h
> > > +++ b/include/media/v4l2-fwnode.h
> > > @@ -25,6 +25,8 @@
> > >  #include <media/v4l2-mediabus.h>
> > >  
> > >  struct fwnode_handle;
> > > +struct v4l2_async_notifier;
> > > +struct v4l2_async_subdev;
> > >  
> > >  /**
> > >   * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
> > > @@ -101,4 +103,11 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> > >  			   struct v4l2_fwnode_link *link);
> > >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> > >  
> > > +int v4l2_fwnode_endpoints_parse(
> > > +	struct device *dev, struct v4l2_async_notifier *notifier,
> > > +	size_t asd_struct_size,
> > > +	int (*parse_single)(struct device *dev,
> > > +			    struct v4l2_fwnode_endpoint *vep,
> > > +			    struct v4l2_async_subdev *asd));
> > > +
> > >  #endif /* _V4L2_FWNODE_H */
> > > -- 
> > > 2.1.4
> > > 
> > > 
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> -- 
> Sakari Ailus
> e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx

-- 
Regards,
Niklas Söderlund



[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