Re: [linux-sunxi] [PATCH v4 09/15] media: sunxi: Add support for the A31 MIPI CSI-2 controller

[Ezequiel Garcia]

Salut Paul,

Just a minor comment about the v4l2 async API.

On Thu, 31 Dec 2020 at 11:30, Paul Kocialkowski
<paul.kocialkowski@xxxxxxxxxxx> wrote:
>
> 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 <paul.kocialkowski@xxxxxxxxxxx>
> ---
>  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
>
[..]
> +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...

> +       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).

Cheers,
Ezequiel