Hi Jacopo,

thank you for the review!

On Tue, 2018-09-04 at 10:04 +0200, jacopo mondi wrote:

Hi Philipp,

On Fri, Aug 10, 2018 at 05:18:22PM +0200, Philipp Zabel wrote:

[...]

quoted

+static struct pxp_fmt formats[] = {
+	{
+		.fourcc	= V4L2_PIX_FMT_XBGR32,
+		.depth	= 32,
+		/* Both capture and output format */
+		.types	= MEM2MEM_CAPTURE | MEM2MEM_OUTPUT,

[...]

quoted

+	}, {
+		.fourcc = V4L2_PIX_FMT_YUV32,
+		.depth	= 16,

According to:
https://www.linuxtv.org/downloads/v4l-dvb-apis-old/packed-yuv.html
shouldn't this be 32?

Yes, I'll change depth to 32.

[...]

quoted

+}
+

Multiple blank lines (in a few other places too)

quoted

+

Found them, will fix them.

[...]

quoted

+	/* Enable clocks and dump registers */
+	clk_prepare_enable(dev->clk);
+	pxp_soft_reset(dev);
+
+	spin_lock_init(&dev->irqlock);
+
+	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
+	if (ret)
+		return ret;
+
+	atomic_set(&dev->num_inst, 0);
+	mutex_init(&dev->dev_mutex);
+
+	dev->vfd = pxp_videodev;

The name of the video device is the same for all instances of this
driver. Is this ok?

I expect that there is only ever going to be one instance on the SoC.

[...]

quoted

+	v4l2_device_unregister(&dev->v4l2_dev);

Disable clock?

Yes, I'll fix the clock handling.

[...]

quoted

+MODULE_DESCRIPTION("i.MX PXP mem2mem scaler/CSC/rotator");
+MODULE_AUTHOR("Philipp Zabel [off-list ref]");
+MODULE_LICENSE("GPL");

SPDX header says GPL2.0+

See include/linux/module.h:

/*
?* The following license idents are currently accepted as indicating free
?* software modules
?*
?*??????"GPL"???????????????????????????[GNU Public License v2 or later]
 * [...]
 */

This already seems to be the right choice.

regards
Philipp