[PATCH v11 2/2] media: V3s: Add support for Allwinner CSI.

Hi Yong,

On Sat, Sep 29, 2018 at 06:18:44PM +0800, Yong wrote:

Hi Sakari,

On Fri, 28 Sep 2018 13:19:37 +0300
Sakari Ailus [off-list ref] wrote:

quoted

Hi Yong,

Thanks for the update! This looks pretty good; some small bits to fix
still, please see my comments below.

On Wed, Sep 26, 2018 at 04:43:40PM +0800, Yong Deng wrote:

quoted

Allwinner V3s SoC features two CSI module. CSI0 is used for MIPI CSI-2
interface and CSI1 is used for parallel interface. This is not
documented in datasheet but by test and guess.

This patch implement a v4l2 framework driver for it.

Currently, the driver only support the parallel interface. MIPI-CSI2,
ISP's support are not included in this patch.

...

quoted

quoted

+
+static void sun6i_csi_v4l2_cleanup(struct sun6i_csi *csi)
+{
+	v4l2_async_notifier_unregister(&csi->notifier);
+	v4l2_async_notifier_cleanup(&csi->notifier);
+	sun6i_video_cleanup(&csi->video);
+	v4l2_device_unregister(&csi->v4l2_dev);
+	media_device_unregister(&csi->media_dev);

Please unregister the media device as first.

I notice that the release order of intel ipu3 is:
    cio2_notifier_exit(cio2);
    cio2_fbpt_exit_dummy(cio2);
    for (i = 0; i < CIO2_QUEUES; i++) 
        cio2_queue_exit(cio2, &cio2->queue[i]);
    v4l2_device_unregister(&cio2->v4l2_dev);
    media_device_unregister(&cio2->media_dev);
    media_device_cleanup(&cio2->media_dev);
    mutex_destroy(&cio2->lock);

Well, yeah... this doesn't make a big difference after all. Unregistering
the media device first is still preferred.

quoted

You're missing freeing the control handler.

quoted

+	media_device_cleanup(&csi->media_dev);
+}
+
+static int sun6i_csi_v4l2_init(struct sun6i_csi *csi)
+{
+	int ret;
+
+	csi->media_dev.dev = csi->dev;
+	strlcpy(csi->media_dev.model, "Allwinner Video Capture Device",
+		sizeof(csi->media_dev.model));

strscpy, please.

quoted

+	csi->media_dev.hw_revision = 0;
+
+	media_device_init(&csi->media_dev);
+
+	ret = v4l2_ctrl_handler_init(&csi->ctrl_handler, 0);

Do you need controls? The driver doesn't appear to register any.

Some one suggest this. They want the drver can pass control queries to 
the subdev.

If you need that, it should be done in the user space instead. The imx
staging driver does so but it is not supported by the control framework:
you may only call v4l2_ctrl_handler_free() for the purpose of freeing the
resources of the control handler --- not while it is still accessible from
the user space.

quoted

quoted

+	if (ret) {
+		dev_err(csi->dev, "V4L2 controls handler init failed (%d)\n",
+			ret);
+		goto clean_media;
+	}
+
+	csi->v4l2_dev.mdev = &csi->media_dev;
+	csi->v4l2_dev.ctrl_handler = &csi->ctrl_handler;
+	ret = v4l2_device_register(csi->dev, &csi->v4l2_dev);
+	if (ret) {
+		dev_err(csi->dev, "V4L2 device registration failed (%d)\n",

...

quoted

quoted

+
+/*
+ * PHYS_OFFSET isn't available on all architectures. In order to
+ * accomodate for COMPILE_TEST, let's define it to something dumb.
+ */
+#ifndef PHYS_OFFSET

How about:

#if !defined(CONFIG_COMPILE_TEST) && !defined(PHYS_OFFSET)

Do you mean:
#if defined(CONFIG_COMPILE_TEST) && !defined(PHYS_OFFSET)

Oh, yes. Indeed.

-- 
Kind regards,

Sakari Ailus
sakari.ailus at linux.intel.com