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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 4487 bytes --]

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
> <paul.kocialkowski@bootlin.com> 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@bootlin.com>
> > ---
> >  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).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]