Hi Steve,
Thank you for the patch.
On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> For some subdevices, a fwnode endpoint that has no connection to a remote
> endpoint may not be an error. Let the parse_endpoint callback make that
> decision in v4l2_async_notifier_fwnode_parse_endpoint(). If the callback
> indicates that is not an error, skip adding the asd to the notifier and
> return 0.
>
> For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> unavailable remote fwnodes to maintain the previous behavior.
I'm not sure this should be a per-driver decision.
Generally speaking, if an endpoint node has no remote-endpoint property, the
endpoint node is not needed. I've always considered such an endpoint node as
invalid. The OF graphs DT bindings are however not clear on this subject. I
have either failed to notice when they got merged, or they slowly evolved over
time to contain contradictory information. In any case, I think we should
decide on whether such a situation is valid or not from an OF graph point of
view, and then always reject or always accept and ignore those endpoints.
> Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>
> ---
> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 3 +++
> drivers/media/platform/omap3isp/isp.c | 3 +++
> drivers/media/platform/rcar-vin/rcar-core.c | 3 +++
> drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++--
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 6c4444b..2323151 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1477,6 +1477,9 @@ static int cio2_fwnode_parse(struct device *dev,
> struct sensor_async_subdev *s_asd =
> container_of(asd, struct sensor_async_subdev, asd);
>
> + if (!fwnode_device_is_available(asd->match.fwnode))
> + return -EINVAL;
> +
> if (vep->bus_type != V4L2_MBUS_CSI2) {
> dev_err(dev, "Only CSI2 bus type is currently supported\n");
> return -EINVAL;
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 8eb000e..4a302f2 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2025,6 +2025,9 @@ static int isp_fwnode_parse(struct device *dev,
> dev_dbg(dev, "parsing endpoint %pOF, interface %u\n",
> to_of_node(vep->base.local_fwnode), vep->base.port);
>
> + if (!fwnode_device_is_available(asd->match.fwnode))
> + return -EINVAL;
> +
> switch (vep->base.port) {
> case ISP_OF_PHY_PARALLEL:
> buscfg->interface = ISP_INTERFACE_PARALLEL;
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index f1fc797..51bb8f1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -149,6 +149,9 @@ static int rvin_digital_parse_v4l2(struct device *dev,
> struct rvin_graph_entity *rvge =
> container_of(asd, struct rvin_graph_entity, asd);
>
> + if (!fwnode_device_is_available(asd->match.fwnode))
> + return -EINVAL;
> +
> if (vep->base.port || vep->base.id)
> return -ENOTCONN;
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index d630640..446646b 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -361,7 +361,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode =
> fwnode_graph_get_remote_port_parent(endpoint);
> - if (!asd->match.fwnode) {
> + if (!asd->match.fwnode && !parse_endpoint) {
> dev_warn(dev, "bad remote port parent\n");
> ret = -EINVAL;
> goto out_err;
> @@ -384,7 +384,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> "driver could not parse port@%u/endpoint@%u (%d)\n",
> vep->base.port, vep->base.id, ret);
> v4l2_fwnode_endpoint_free(vep);
> - if (ret < 0)
> + if (ret < 0 || !asd->match.fwnode)
> goto out_err;
>
> notifier->subdevs[notifier->num_subdevs] = asd;
--
Regards,
Laurent Pinchart
