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.
> 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 ?
> + 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.
> - 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");
[snip]
--
Regards,
Laurent Pinchart
