Re: [PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[Philippe Ombredanne]

On Fri, Jan 12, 2018 at 2:38 PM, jacopo mondi  wrote:
> Hi Philippe, Laurent, Geert,
>
> On Fri, Jan 12, 2018 at 11:36:31AM +0100, Philippe Ombredanne wrote:
>> On Tue, Jan 9, 2018 at 5:25 PM, Jacopo Mondi  
>> wrote:
>> > Add driver for Renesas Capture Engine Unit (CEU).
>>
>> 
>>
>> > --- /dev/null
>> > +++ b/drivers/media/platform/renesas-ceu.c
>> > @@ -0,0 +1,1648 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>>
>> 
>>
>> > +MODULE_DESCRIPTION("Renesas CEU camera driver");
>> > +MODULE_AUTHOR("Jacopo Mondi ");
>> > +MODULE_LICENSE("GPL");
>>
>> Jacopo,
>> the MODULE_LICENSE does not match the SPDX tag. Per module.h "GPL"
>> means GPL-2.0 or later ;)
>>
>> It should be instead:
>>
>> > +MODULE_LICENSE("GPL v2");
>>
>> ... to match your
>>
>> > +// SPDX-License-Identifier: GPL-2.0
>
> I will update this in next v5.
> Laurent, Geert: I'll keep SPDX identifier to "GPL-2.0" until kernel
> doc does not get updated.

Thanks. Sorry for the late reply!

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[jacopo mondi]

Hi Philippe, Laurent, Geert,

On Fri, Jan 12, 2018 at 11:36:31AM +0100, Philippe Ombredanne wrote:
> On Tue, Jan 9, 2018 at 5:25 PM, Jacopo Mondi  
> wrote:
> > Add driver for Renesas Capture Engine Unit (CEU).
>
> 
>
> > --- /dev/null
> > +++ b/drivers/media/platform/renesas-ceu.c
> > @@ -0,0 +1,1648 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> 
>
> > +MODULE_DESCRIPTION("Renesas CEU camera driver");
> > +MODULE_AUTHOR("Jacopo Mondi ");
> > +MODULE_LICENSE("GPL");
>
> Jacopo,
> the MODULE_LICENSE does not match the SPDX tag. Per module.h "GPL"
> means GPL-2.0 or later ;)
>
> It should be instead:
>
> > +MODULE_LICENSE("GPL v2");
>
> ... to match your
>
> > +// SPDX-License-Identifier: GPL-2.0

I will update this in next v5.
Laurent, Geert: I'll keep SPDX identifier to "GPL-2.0" until kernel
doc does not get updated.

Thanks
   j

>
> I know this can be confusing, but updating the MODULE_LICENSE tags
> definitions in module.h to match SPDX tags is unlikely to happen as it
> would create mayhem for everyone and every module loader relying on
> this established convention.
>
> --
> Cordially
> Philippe Ombredanne

Re: [PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[Philippe Ombredanne]

On Tue, Jan 9, 2018 at 5:25 PM, Jacopo Mondi  wrote:
> Add driver for Renesas Capture Engine Unit (CEU).



> --- /dev/null
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -0,0 +1,1648 @@
> +// SPDX-License-Identifier: GPL-2.0



> +MODULE_DESCRIPTION("Renesas CEU camera driver");
> +MODULE_AUTHOR("Jacopo Mondi ");
> +MODULE_LICENSE("GPL");

Jacopo,
the MODULE_LICENSE does not match the SPDX tag. Per module.h "GPL"
means GPL-2.0 or later ;)

It should be instead:

> +MODULE_LICENSE("GPL v2");

... to match your

> +// SPDX-License-Identifier: GPL-2.0

I know this can be confusing, but updating the MODULE_LICENSE tags
definitions in module.h to match SPDX tags is unlikely to happen as it
would create mayhem for everyone and every module loader relying on
this established convention.

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[Laurent Pinchart]

Hi Geert,

On Friday, 12 January 2018 11:01:18 EET Geert Uytterhoeven wrote:
> On Fri, Jan 12, 2018 at 12:12 AM, Laurent Pinchart wrote:
> > On Tuesday, 9 January 2018 18:25:25 EET Jacopo Mondi wrote:
> >> Add driver for Renesas Capture Engine Unit (CEU).
> >> 
> >> The CEU interface supports capturing 'data' (YUV422) and 'images'
> >> (NV[12|21|16|61]).
> >> 
> >> This driver aims to replace the soc_camera-based sh_mobile_ceu one.
> >> 
> >> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> >> platform GR-Peach.
> >> 
> >> Tested with ov7725 camera sensor on SH4 platform Migo-R.
> >> 
> >> Signed-off-by: Jacopo Mondi 
> >> ---
> >> 
> >>  drivers/media/platform/Kconfig   |9 +
> >>  drivers/media/platform/Makefile  |1 +
> >>  drivers/media/platform/renesas-ceu.c | 1648 
> >>  3 files changed, 1658 insertions(+)
> >> 
> >>  create mode 100644 drivers/media/platform/renesas-ceu.c
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/platform/renesas-ceu.c
> >> b/drivers/media/platform/renesas-ceu.c new file mode 100644
> >> index 000..d261704
> >> --- /dev/null
> >> +++ b/drivers/media/platform/renesas-ceu.c
> >> @@ -0,0 +1,1648 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> > 
> > It was recently brought to my attention that SPDX headers should use
> > either GPL-2.0-only or GPL-2.0-or-later, no the ambiguous GPL-2.0. Could
> > you please update all patches in this series ?
> 
> IMHO it's a bit premature to do that.
> As long as Documentation/process/license-rules.rst isn't updated, I wouldn't
> follow this change.
> 
> See also https://www.spinics.net/lists/linux-xfs/msg14536.html

Thank you for bringing this to my attention. I agree with you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Jan 12, 2018 at 12:12 AM, Laurent Pinchart
 wrote:
> On Tuesday, 9 January 2018 18:25:25 EET Jacopo Mondi wrote:
>> Add driver for Renesas Capture Engine Unit (CEU).
>>
>> The CEU interface supports capturing 'data' (YUV422) and 'images'
>> (NV[12|21|16|61]).
>>
>> This driver aims to replace the soc_camera-based sh_mobile_ceu one.
>>
>> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
>> platform GR-Peach.
>>
>> Tested with ov7725 camera sensor on SH4 platform Migo-R.
>>
>> Signed-off-by: Jacopo Mondi 
>> ---
>>  drivers/media/platform/Kconfig   |9 +
>>  drivers/media/platform/Makefile  |1 +
>>  drivers/media/platform/renesas-ceu.c | 1648
>> ++ 3 files changed, 1658 insertions(+)
>>  create mode 100644 drivers/media/platform/renesas-ceu.c
>
> [snip]
>
>> diff --git a/drivers/media/platform/renesas-ceu.c
>> b/drivers/media/platform/renesas-ceu.c new file mode 100644
>> index 000..d261704
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas-ceu.c
>> @@ -0,0 +1,1648 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> It was recently brought to my attention that SPDX headers should use either
> GPL-2.0-only or GPL-2.0-or-later, no the ambiguous GPL-2.0. Could you please
> update all patches in this series ?

IMHO it's a bit premature to do that.
As long as Documentation/process/license-rules.rst isn't updated, I wouldn't
follow this change.

See also https://www.spinics.net/lists/linux-xfs/msg14536.html

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[Laurent Pinchart]

On Tuesday, 9 January 2018 18:25:25 EET Jacopo Mondi wrote:
> Add driver for Renesas Capture Engine Unit (CEU).
> 
> The CEU interface supports capturing 'data' (YUV422) and 'images'
> (NV[12|21|16|61]).
> 
> This driver aims to replace the soc_camera-based sh_mobile_ceu one.
> 
> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> platform GR-Peach.
> 
> Tested with ov7725 camera sensor on SH4 platform Migo-R.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/media/platform/Kconfig   |9 +
>  drivers/media/platform/Makefile  |1 +
>  drivers/media/platform/renesas-ceu.c | 1648
> ++ 3 files changed, 1658 insertions(+)
>  create mode 100644 drivers/media/platform/renesas-ceu.c

[snip]

> diff --git a/drivers/media/platform/renesas-ceu.c
> b/drivers/media/platform/renesas-ceu.c new file mode 100644
> index 000..d261704
> --- /dev/null
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -0,0 +1,1648 @@
> +// SPDX-License-Identifier: GPL-2.0

It was recently brought to my attention that SPDX headers should use either 
GPL-2.0-only or GPL-2.0-or-later, no the ambiguous GPL-2.0. Could you please 
update all patches in this series ?

[snip]

> +/*
> + * struct ceu_data - Platform specific CEU data
> + * @irq_mask: CETCR mask with all interrupt sources enabled. The mask
> differs
> + * between SH4 and RZ platforms.
> + */
> +struct ceu_data {
> + const u32 irq_mask;
> +};
> +
> +const struct ceu_data ceu_data_rz = {
> + .irq_mask = CEU_CETCR_ALL_IRQS_RZ,
> +};
> +
> +const struct ceu_data ceu_data_sh4 = {
> + .irq_mask = CEU_CETCR_ALL_IRQS_SH4,
> +};

I meant static and not const in my last review (as in static const struct 
ceu_data ...). Adding a const keyword in front of the u32 irq_mask field 
definition isn't very useful.

With that fixed,

Reviewed-by: Laurent Pinchart 

-- 
Regards,

Laurent Pinchart

[PATCH v4 3/9] v4l: platform: Add Renesas CEU driver

[Jacopo Mondi]

Add driver for Renesas Capture Engine Unit (CEU).

The CEU interface supports capturing 'data' (YUV422) and 'images'
(NV[12|21|16|61]).

This driver aims to replace the soc_camera-based sh_mobile_ceu one.

Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
platform GR-Peach.

Tested with ov7725 camera sensor on SH4 platform Migo-R.

Signed-off-by: Jacopo Mondi 
---
 drivers/media/platform/Kconfig   |9 +
 drivers/media/platform/Makefile  |1 +
 drivers/media/platform/renesas-ceu.c | 1648 ++
 3 files changed, 1658 insertions(+)
 create mode 100644 drivers/media/platform/renesas-ceu.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index fd0c998..fe7bd26 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -144,6 +144,15 @@ config VIDEO_STM32_DCMI
  To compile this driver as a module, choose M here: the module
  will be called stm32-dcmi.
 
+config VIDEO_RENESAS_CEU
+   tristate "Renesas Capture Engine Unit (CEU) driver"
+   depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
+   depends on ARCH_SHMOBILE || ARCH_R7S72100 || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select V4L2_FWNODE
+   ---help---
+ This is a v4l2 driver for the Renesas CEU Interface
+
 source "drivers/media/platform/soc_camera/Kconfig"
 source "drivers/media/platform/exynos4-is/Kconfig"
 source "drivers/media/platform/am437x/Kconfig"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 003b0bb..6580a6b 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_SH_VOU)+= sh_vou.o
 obj-$(CONFIG_SOC_CAMERA)   += soc_camera/
 
 obj-$(CONFIG_VIDEO_RCAR_DRIF)  += rcar_drif.o
+obj-$(CONFIG_VIDEO_RENESAS_CEU)+= renesas-ceu.o
 obj-$(CONFIG_VIDEO_RENESAS_FCP)+= rcar-fcp.o
 obj-$(CONFIG_VIDEO_RENESAS_FDP1)   += rcar_fdp1.o
 obj-$(CONFIG_VIDEO_RENESAS_JPU)+= rcar_jpu.o
diff --git a/drivers/media/platform/renesas-ceu.c 
b/drivers/media/platform/renesas-ceu.c
new file mode 100644
index 000..d261704
--- /dev/null
+++ b/drivers/media/platform/renesas-ceu.c
@@ -0,0 +1,1648 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * V4L2 Driver for Renesas Capture Engine Unit (CEU) interface
+ * Copyright (C) 2017-2018 Jacopo Mondi 
+ *
+ * Based on soc-camera driver "soc_camera/sh_mobile_ceu_camera.c"
+ * Copyright (C) 2008 Magnus Damm
+ *
+ * Based on V4L2 Driver for PXA camera host - "pxa_camera.c",
+ * Copyright (C) 2006, Sascha Hauer, Pengutronix
+ * Copyright (C) 2008, Guennadi Liakhovetski 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define DRIVER_NAME"renesas-ceu"
+
+/* CEU registers offsets and masks. */
+#define CEU_CAPSR  0x00 /* Capture start register  */
+#define CEU_CAPCR  0x04 /* Capture control register*/
+#define CEU_CAMCR  0x08 /* Capture interface control register  */
+#define CEU_CAMOR  0x10 /* Capture interface offset register   */
+#define CEU_CAPWR  0x14 /* Capture interface width register*/
+#define CEU_CAIFR  0x18 /* Capture interface input format register */
+#define CEU_CRCNTR 0x28 /* CEU register control register   */
+#define CEU_CRCMPR 0x2c /* CEU register forcible control register  */
+#define CEU_CFLCR  0x30 /* Capture filter control register */
+#define CEU_CFSZR  0x34 /* Capture filter size clip register   */
+#define CEU_CDWDR  0x38 /* Capture destination width register  */
+#define CEU_CDAYR  0x3c /* Capture data address Y register */
+#define CEU_CDACR  0x40 /* Capture data address C register */
+#define CEU_CFWCR  0x5c /* Firewall operation control register */
+#define CEU_CDOCR  0x64 /* Capture data output control register*/
+#define CEU_CEIER  0x70 /* Capture event interrupt enable register */
+#define CEU_CETCR  0x74 /* Capture event flag clear register   */
+#define CEU_CSTSR  0x7c /* Capture status register */
+#define CEU_CSRTR  0x80 /* Capture software reset register */
+
+/* Data synchronous fetch mode. */
+#define CEU_CAMCR_JPEG BIT(4)
+
+/* Input components ordering: CEU_CAMCR.DTARY field. */
+#define CEU_CAMCR_DTARY_8_UYVY (0x00 << 8)
+#define CEU_CAMCR_DTARY_8_VYUY (0x01 << 8)
+#define CEU_CAMCR_DTARY_8_YUYV (0x02 << 8)
+#define CEU_CAMCR_DTARY_8_YVYU (0x03 << 8)
+/* TODO: input