Hi Niklas,
Thank you for the patch.
On Fri, Aug 07, 2020 at 01:16:19PM +0200, Niklas Söderlund wrote:
> The V4L2 async framework helpers now populates the async notifier with
> endpoint matching information and there is no need to do this manually
> in the R-Car CSI-2 driver, switch to using the provided helper.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 48 +++++----------------
> 1 file changed, 11 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index c6cc4f473a077899..f0067ff21d5d9d33 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -362,7 +362,6 @@ struct rcar_csi2 {
> struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
>
> struct v4l2_async_notifier notifier;
> - struct v4l2_async_subdev asd;
> struct v4l2_subdev *remote;
>
> struct v4l2_mbus_framefmt mf;
> @@ -774,9 +773,11 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
> .unbind = rcsi2_notify_unbind,
> };
>
> -static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> - struct v4l2_fwnode_endpoint *vep)
> +static int rcar_csi2_parse_v4l2(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
> {
> + struct rcar_csi2 *priv = dev_get_drvdata(dev);
> unsigned int i;
>
> /* Only port 0 endpoint 0 is valid. */
> @@ -806,53 +807,26 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> }
> }
>
> + dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(asd->match.fwnode));
> +
> return 0;
> }
>
> static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> {
> - struct device_node *ep;
> - struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
> int ret;
>
> - ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> - if (!ep) {
> - dev_err(priv->dev, "Not connected to subdevice\n");
> - return -EINVAL;
> - }
> -
> - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> - if (ret) {
> - dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> - of_node_put(ep);
> - return -EINVAL;
> - }
> -
> - ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
> - if (ret) {
> - of_node_put(ep);
> - return ret;
> - }
> -
> - priv->asd.match.fwnode =
> - fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> - priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -
> - of_node_put(ep);
> -
> v4l2_async_notifier_init(&priv->notifier);
>
> - ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
> - if (ret) {
> - fwnode_handle_put(priv->asd.match.fwnode);
> + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> + priv->dev, &priv->notifier,
> + sizeof(struct v4l2_async_subdev), 0,
> + rcar_csi2_parse_v4l2);
It seems a bit expensive to go through a complex logic of iterating over
all endpoints when rcar_csi2_parse_v4l2() will only accept port 0,
endpoint 0, but there's a clear benefit from not having to maintain this
code in the driver.
With Jacopo's comments addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
If anyone has time to spend on it, I think
__v4l2_async_notifier_parse_fwnode_ep() should be duplicated in
v4l2_async_notifier_parse_fwnode_endpoints() and
v4l2_async_notifier_parse_fwnode_endpoints_by_port(), with the latter
only iterating over the endpoints of the requested port.
I also wonder if the !is_available check needs to be inside the loop:
fwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) {
struct fwnode_handle *dev_fwnode;
bool is_available;
dev_fwnode = fwnode_graph_get_port_parent(fwnode);
is_available = fwnode_device_is_available(dev_fwnode);
fwnode_handle_put(dev_fwnode);
if (!is_available)
continue;
...
}
Isn't dev_fwnode == dev_fwnode(dev) and thus constant ?
> + if (ret)
> return ret;
> - }
>
> priv->notifier.ops = &rcar_csi2_notify_ops;
>
> - dev_dbg(priv->dev, "Found '%pOF'\n",
> - to_of_node(priv->asd.match.fwnode));
> -
> ret = v4l2_async_subdev_notifier_register(&priv->subdev,
> &priv->notifier);
> if (ret)
--
Regards,
Laurent Pinchart
