Date:   Thu, 14 Jan 2021 10:30:01 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     linux-media <linux-media@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-doc@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-sunxi@...glegroups.com, Yong Deng <yong.deng@...ewell.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Jonathan Corbet <corbet@....net>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>,
        Helen Koike <helen.koike@...labora.com>,
        Dafna Hirschfeld <dafna.hirschfeld@...labora.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        kevin.lhopital@...mail.com
Subject: Re: [linux-sunxi] [PATCH v4 09/15] media: sunxi: Add support for the
 A31 MIPI CSI-2 controller

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@...tlin.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@...tlin.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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists