Re: [PATCH v13 07/25] rcar-vin: Use generic parser for parsing fwnode endpoints

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

 




Hi Laurent,

On Tue, Sep 19, 2017 at 02:53:16PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:06 EEST Sakari Ailus wrote:
> > Instead of using driver implementation, use
> 
> Same comment as for patch 06/25.

Will fix.

> 
> > v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> > of the device.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 112 ++++++++-----------------
> >  drivers/media/platform/rcar-vin/rcar-dma.c  |  10 +--
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  14 ++--
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |   4 +-
> >  4 files changed, 48 insertions(+), 92 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c index
> > 142de447aaaa..62b4a94f9a39 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> 
> [snip]
> 
> > @@ -120,117 +121,70 @@ static int rvin_digital_notify_bound(struct
> 
> [snip]
> 
> > -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
> > -				    struct device_node *ep,
> > -				    struct v4l2_mbus_config *mbus_cfg)
> > +static int rvin_digital_parse_v4l2(struct device *dev,
> > +				   struct v4l2_fwnode_endpoint *vep,
> > +				   struct v4l2_async_subdev *asd)
> >  {
> > -	struct v4l2_fwnode_endpoint v4l2_ep;
> > -	int ret;
> > +	struct rvin_dev *vin = dev_get_drvdata(dev);
> 
> Doesn't this show that we miss a context argument to the callback function ? 
> Storing the context in device driver data is probably OK if the driver parsing 
> the endpoints controls the struct device, but is that always the case ?

How does a driver know the hardware other than, uh, the device?

I guess we could add a private pointer when the async notifier is
registered if there's a real need for it. The notifier could be an
alternative but it wouldn't be applicable to sub-devices.

> 
> > +	struct rvin_graph_entity *rvge =
> > +		container_of(asd, struct rvin_graph_entity, asd);
> > 
> > -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> > -	if (ret) {
> > -		vin_err(vin, "Could not parse v4l2 endpoint\n");
> > -		return -EINVAL;
> > -	}
> > +	if (vep->base.port || vep->base.id)
> > +		return -ENOTCONN;
> > 
> > -	mbus_cfg->type = v4l2_ep.bus_type;
> > +	rvge->mbus_cfg.type = vep->bus_type;
> > 
> > -	switch (mbus_cfg->type) {
> > +	switch (rvge->mbus_cfg.type) {
> >  	case V4L2_MBUS_PARALLEL:
> >  		vin_dbg(vin, "Found PARALLEL media bus\n");
> > -		mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
> > +		rvge->mbus_cfg.flags = vep->bus.parallel.flags;
> >  		break;
> >  	case V4L2_MBUS_BT656:
> >  		vin_dbg(vin, "Found BT656 media bus\n");
> > -		mbus_cfg->flags = 0;
> > +		rvge->mbus_cfg.flags = 0;
> >  		break;
> >  	default:
> >  		vin_err(vin, "Unknown media bus type\n");
> >  		return -EINVAL;
> >  	}
> > 
> > -	return 0;
> > -}
> > -
> > -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> > -{
> > -	struct device_node *ep, *np;
> > -	int ret;
> > -
> > -	vin->digital.asd.match.fwnode.fwnode = NULL;
> > -	vin->digital.subdev = NULL;
> > -
> > -	/*
> > -	 * Port 0 id 0 is local digital input, try to get it.
> > -	 * Not all instances can or will have this, that is OK
> > -	 */
> > -	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> > -	if (!ep)
> > -		return 0;
> > -
> > -	np = of_graph_get_remote_port_parent(ep);
> > -	if (!np) {
> > -		vin_err(vin, "No remote parent for digital input\n");
> > -		of_node_put(ep);
> > -		return -EINVAL;
> > -	}
> > -	of_node_put(np);
> > -
> > -	ret = rvin_digitial_parse_v4l2(vin, ep, &vin->digital.mbus_cfg);
> > -	of_node_put(ep);
> > -	if (ret)
> > -		return ret;
> > -
> > -	vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);
> > -	vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +	vin->digital = rvge;
> > 
> >  	return 0;
> >  }
> > 
> >  static int rvin_digital_graph_init(struct rvin_dev *vin)
> >  {
> > -	struct v4l2_async_subdev **subdevs = NULL;
> >  	int ret;
> > 
> > -	ret = rvin_digital_graph_parse(vin);
> > +	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > +		vin->dev, &vin->notifier,
> > +		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> >  	if (ret)
> >  		return ret;
> > 
> > -	if (!vin->digital.asd.match.fwnode.fwnode) {
> > -		vin_dbg(vin, "No digital subdevice found\n");
> > -		return -ENODEV;
> > -	}
> > -
> > -	/* Register the subdevices notifier. */
> > -	subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);
> > -	if (subdevs == NULL)
> > -		return -ENOMEM;
> > -
> > -	subdevs[0] = &vin->digital.asd;
> > -
> > -	vin_dbg(vin, "Found digital subdevice %pOF\n",
> > -		to_of_node(subdevs[0]->match.fwnode.fwnode));
> > +	if (vin->digital)
> > +		vin_dbg(vin, "Found digital subdevice %pOF\n",
> > +			to_of_node(
> > +				vin->digital->asd.match.fwnode.fwnode));
> 
> Isn't this is a change in behaviour ? The driver currently returns -ENODEV 
> when no digital subdev is found.

Seems so, I'll address that in v14.

> 
> > -	vin->notifier.num_subdevs = 1;
> > -	vin->notifier.subdevs = subdevs;
> >  	vin->notifier.bound = rvin_digital_notify_bound;
> >  	vin->notifier.unbind = rvin_digital_notify_unbind;
> >  	vin->notifier.complete = rvin_digital_notify_complete;
> > -
> >  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> >  	if (ret < 0) {
> >  		vin_err(vin, "Notifier registration failed\n");
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux