Hey Ezequiel,

On Mon 11 Jan 21, 15:21, Ezequiel Garcia wrote:

Salut Paul,

Just a minor comment about the v4l2 async API.

On Thu, 31 Dec 2020 at 11:30, Paul Kocialkowski
[off-list ref] wrote:

quoted

The A31 MIPI CSI-2 controller is a dedicated MIPI CSI-2 bridge
found on Allwinner SoCs such as the A31 and V3/V3s.

It is a standalone block, connected to the CSI controller on one side
and to the MIPI D-PHY block on the other. It has a dedicated address
space, interrupt line and clock.

It is represented as a V4L2 subdev to the CSI controller and takes a
MIPI CSI-2 sensor as its own subdev, all using the fwnode graph and
media controller API.

Only 8-bit and 10-bit Bayer formats are currently supported.
While up to 4 internal channels to the CSI controller exist, only one
is currently supported by this implementation.

Signed-off-by: Paul Kocialkowski <redacted>
---
 drivers/media/platform/sunxi/Kconfig          |   1 +
 drivers/media/platform/sunxi/Makefile         |   1 +
 .../platform/sunxi/sun6i-mipi-csi2/Kconfig    |  12 +
 .../platform/sunxi/sun6i-mipi-csi2/Makefile   |   4 +
 .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c   | 590 ++++++++++++++++++
 .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h   | 117 ++++
 6 files changed, 725 insertions(+)
 create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
 create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
 create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
 create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h

[..]

quoted

+static int sun6i_mipi_csi2_v4l2_setup(struct sun6i_mipi_csi2_dev *cdev)
+{
+       struct sun6i_mipi_csi2_video *video = &cdev->video;
+       struct v4l2_subdev *subdev = &video->subdev;
+       struct v4l2_async_notifier *notifier = &video->notifier;
+       struct fwnode_handle *handle;
+       struct v4l2_fwnode_endpoint *endpoint;
+       struct v4l2_async_subdev *subdev_async;
+       int ret;
+
+       /* Subdev */
+
+       v4l2_subdev_init(subdev, &sun6i_mipi_csi2_subdev_ops);
+       subdev->dev = cdev->dev;
+       subdev->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+       strscpy(subdev->name, MODULE_NAME, sizeof(subdev->name));
+       v4l2_set_subdevdata(subdev, cdev);
+
+       /* Entity */
+
+       subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+       subdev->entity.ops = &sun6i_mipi_csi2_entity_ops;
+
+       /* Pads */
+
+       video->pads[0].flags = MEDIA_PAD_FL_SINK;
+       video->pads[1].flags = MEDIA_PAD_FL_SOURCE;
+
+       ret = media_entity_pads_init(&subdev->entity, 2, video->pads);
+       if (ret)
+               return ret;
+
+       /* Endpoint */
+
+       handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(cdev->dev), 0, 0,
+                                                FWNODE_GRAPH_ENDPOINT_NEXT);
+       if (!handle) {
+               ret = -ENODEV;
+               goto error_media_entity;
+       }
+
+       endpoint = &video->endpoint;
+       endpoint->bus_type = V4L2_MBUS_CSI2_DPHY;
+
+       ret = v4l2_fwnode_endpoint_parse(handle, endpoint);
+       fwnode_handle_put(handle);

I think the _put should be...

quoted

+       if (ret)
+               goto error_media_entity;
+
+       /* Notifier */
+
+       v4l2_async_notifier_init(notifier);
+
+       subdev_async = &video->subdev_async;
+       ret = v4l2_async_notifier_add_fwnode_remote_subdev(notifier, handle,
+                                                          subdev_async);

... here. See for instance drivers/media/platform/rcar-vin/rcar-csi2.c.

(Unless I've missed something, of course).

I think you're right, the reference is obtained at
fwnode_graph_get_endpoint_by_id and should be held when passing handle to
v4l2_async_notifier_add_fwnode_remote_subdev since it will be used to get
a reference to the remote port.

Good catch and thanks for the review!

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com