Re: [PATCH 2/2] MAINTAINERS: mtd/nand: update Microchip nand entry

[Yang, Wenyou]

On 2018/1/9 21:46, Nicolas Ferre wrote:

Update Wenyou Yang email address.
Take advantage of this update to move this entry to the MICROCHIP / ATMEL
location and add the DT binding documentation link.

Signed-off-by: Nicolas Ferre 

Acked-by: Wenyou Yang 

---
Hi,

Patch against next-20180109.
This patch is somehow dependent on the previous one in the series
("MAINTAINERS: linux-media: update Microchip ISI and ISC entries") but can be
rebased easily.

I don't know if it's better to have them added at the end of the development
cycle or just after rc1: let me know if you plan to take them or if I need to
rebase them for next cycle.

Best regards,
   Nicolas


  MAINTAINERS | 15 ---
  1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 65c4b59b582f..b48e217d41fb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2373,13 +2373,6 @@ F:   
Documentation/devicetree/bindings/input/atmel,maxtouch.txt
  F:drivers/input/touchscreen/atmel_mxt_ts.c
  F:include/linux/platform_data/atmel_mxt_ts.h
  
-ATMEL NAND DRIVER

-M: Wenyou Yang 
-M: Josh Wu 
-L: linux-...@lists.infradead.org
-S: Supported
-F: drivers/mtd/nand/atmel/*
-
  ATMEL SAMA5D2 ADC DRIVER
  M:Ludovic Desroches 
  L:linux-...@vger.kernel.org
@@ -9110,6 +9103,14 @@ F:   drivers/media/platform/atmel/atmel-isi.c
  F:include/media/atmel-isi.h
  F:Documentation/devicetree/bindings/media/atmel-isi.txt
  
+MICROCHIP / ATMEL NAND DRIVER

+M: Wenyou Yang 
+M: Josh Wu 
+L: linux-...@lists.infradead.org
+S: Supported
+F: drivers/mtd/nand/atmel/*
+F: Documentation/devicetree/bindings/mtd/atmel-nand.txt
+
  MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER
  M:Woojung Huh 
  M:Microchip Linux Driver Support 


Best Regards,
Wenyou Yang

Re: [PATCH 1/2] MAINTAINERS: linux-media: update Microchip ISI and ISC entries

[Yang, Wenyou]

On 2018/1/9 21:46, Nicolas Ferre wrote:

These two image capture interface drivers are now handled
by Wenyou Yang.
I benefit from this change to update the two entries by correcting the
binding documentation link for ISC and moving the ISI to the new
MICROCHIP / ATMEL location.

Signed-off-by: Nicolas Ferre 

Acked-by: Wenyou Yang 

---
Hi,

Patch against next-20180109.
Note that I didn't find it useful to have several patches for these changes.
Tell me if you feel differently.

I would like to have the Ack from Ludovic and Wenyou obviously. I don't know if
Songjun can answer as he's not with Microchip anymore.

Best regards,
   Nicolas

  MAINTAINERS | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index a7d10a2bb980..65c4b59b582f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2353,13 +2353,6 @@ L:   linux-...@vger.kernel.org
  S:Supported
  F:drivers/i2c/busses/i2c-at91.c
  
-ATMEL ISI DRIVER

-M: Ludovic Desroches 
-L: linux-media@vger.kernel.org
-S: Supported
-F: drivers/media/platform/atmel/atmel-isi.c
-F: include/media/atmel-isi.h
-
  ATMEL LCDFB DRIVER
  M:Nicolas Ferre 
  L:linux-fb...@vger.kernel.org
@@ -9102,12 +9095,20 @@ S:  Maintained
  F:drivers/crypto/atmel-ecc.*
  
  MICROCHIP / ATMEL ISC DRIVER

-M: Songjun Wu 
+M: Wenyou Yang 
  L:linux-media@vger.kernel.org
  S:Supported
  F:drivers/media/platform/atmel/atmel-isc.c
  F:drivers/media/platform/atmel/atmel-isc-regs.h
-F: devicetree/bindings/media/atmel-isc.txt
+F: Documentation/devicetree/bindings/media/atmel-isc.txt
+
+MICROCHIP / ATMEL ISI DRIVER
+M: Wenyou Yang 
+L: linux-media@vger.kernel.org
+S: Supported
+F: drivers/media/platform/atmel/atmel-isi.c
+F: include/media/atmel-isi.h
+F: Documentation/devicetree/bindings/media/atmel-isi.txt
  
  MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER

  M:Woojung Huh 

Best Regards,
Wenyou Yang

Re: [PATCH] media: i2c: ov7740: add media-controller dependency

[Yang, Wenyou]

Hello Arnd,


On 2018/1/8 20:52, Arnd Bergmann wrote:

Without CONFIG_MEDIA_CONTROLLER, the new driver fails to build:

drivers/perf/arm_dsu_pmu.c: In function 'dsu_pmu_probe_pmu':
drivers/perf/arm_dsu_pmu.c:661:2: error: implicit declaration of function 
'bitmap_from_u32array'; did you mean 'bitmap_from_arr32'? 
[-Werror=implicit-function-declaration]

This adds a dependency similar to what we have for other drivers
like this.

Fixes: 39c5c4471b8d ("media: i2c: Add the ov7740 image sensor driver")
Signed-off-by: Arnd Bergmann 
---

Indeed.
Thank you for your fix.

Acked-by: Wenyou Yang 


  drivers/media/i2c/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9f18cd296841..03cf3a1a1e06 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -667,7 +667,7 @@ config VIDEO_OV7670
  
  config VIDEO_OV7740

tristate "OmniVision OV7740 sensor support"
-   depends on I2C && VIDEO_V4L2
+   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
depends on MEDIA_CAMERA_SUPPORT
---help---
  This is a Video4Linux2 sensor-level driver for the OmniVision


Best Regards,
Wenyou Yang

Re: [PATCH -next] media: atmel-isc: Make local symbol fmt_configs_list static

[Yang, Wenyou]

On 2017/12/23 9:57, Wei Yongjun wrote:

Fixes the following sparse warning:

drivers/media/platform/atmel/atmel-isc.c:338:19: warning:
  symbol 'fmt_configs_list' was not declared. Should it be static?

Signed-off-by: Wei Yongjun 
---

Acked-by: Wenyou Yang 


  drivers/media/platform/atmel/atmel-isc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 0c26356..2dd72fc 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -335,7 +335,7 @@ struct isc_device {
},
  };
  
-struct fmt_config fmt_configs_list[] = {

+static struct fmt_config fmt_configs_list[] = {
{
.fourcc = V4L2_PIX_FMT_SBGGR8,
.pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,

Re: [PATCH v8 2/2] media: i2c: Add the ov7740 image sensor driver

[Yang, Wenyou]

Hi Philippe,


On 2017/12/8 21:14, Philippe Ombredanne wrote:

Wenyou,

On Fri, Dec 8, 2017 at 2:55 AM, Wenyou Yang  wrote:

The ov7740 (color) image sensor is a high performance VGA CMOS
image snesor, which supports for output formats: RAW RGB and YUV
and image sizes: VGA, and QVGA, CIF and any size smaller.

Signed-off-by: Songjun Wu 
Signed-off-by: Wenyou Yang 

[]

--- /dev/null
+++ b/drivers/media/i2c/ov7740.c
@@ -0,0 +1,1226 @@
+/*
+ * Copyright (c) 2017 Microchip Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */

Have you considered using the new SPDX ids instead of this fine legalese?
e.g.:

// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017 Microchip Corporation.

Short and neat! Check also Thomas doc patches and Linus comments on
why he prefers the C++ comment style for these.

Thank you for your suggestion and information.


Best Regards,
Wenyou Yang

Re: [PATCH v7 2/2] media: i2c: Add the ov7740 image sensor driver

[Yang, Wenyou]

Hi Sakari,


On 2017/12/7 21:16, Sakari Ailus wrote:

Hi Wenyou,

On Wed, Dec 06, 2017 at 10:23:43AM +0800, Wenyou Yang wrote:

The ov7740 (color) image sensor is a high performance VGA CMOS
image snesor, which supports for output formats: RAW RGB and YUV
and image sizes: VGA, and QVGA, CIF and any size smaller.

Signed-off-by: Songjun Wu 
Signed-off-by: Wenyou Yang 
---

Changes in v7:
  - Fix the wrong handle of default register configuration.
  - Add the missed assignment of ov7740->frmsize.

Changes in v6:
  - Remove unnecessary #include .
  - Remove unnecessary comments and extra newline.
  - Add const for some structures.
  - Add the check of the return value from regmap_write().
  - Simplify the calling of __v4l2_ctrl_handler_setup().
  - Add the default format initialization function.
  - Integrate the set_power() and enable/disable the clock into
one function.

Changes in v5:
  - Squash the driver and MAINTAINERS entry patches to one.
  - Precede the driver patch with the bindings patch.

Changes in v4:
  - Assign 'val' a initial value to avoid warning: 'val' may be
used uninitialized.
  - Rename REG_REG15 to avoid warning: "REG_REG15" redefined.

Changes in v3:
  - Put the MAINTAINERS change to a separate patch.

Changes in v2:
  - Split off the bindings into a separate patch.
  - Add a new entry to the MAINTAINERS file.

  MAINTAINERS|8 +
  drivers/media/i2c/Kconfig  |8 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov7740.c | 1234 
  4 files changed, 1251 insertions(+)
  create mode 100644 drivers/media/i2c/ov7740.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7a52a66aa991..1de965009b13 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10053,6 +10053,14 @@ S: Maintained
  F:drivers/media/i2c/ov7670.c
  F:Documentation/devicetree/bindings/media/i2c/ov7670.txt
  
+OMNIVISION OV7740 SENSOR DRIVER

+M: Wenyou Yang 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/ov7740.c
+F: Documentation/devicetree/bindings/media/i2c/ov7740.txt
+
  ONENAND FLASH DRIVER
  M:Kyungmin Park 
  L:linux-...@lists.infradead.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index cb5d7ff82915..00b1c4c031d4 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -665,6 +665,14 @@ config VIDEO_OV7670
  OV7670 VGA camera.  It currently only works with the M88ALP01
  controller.
  
+config VIDEO_OV7740

+   tristate "OmniVision OV7740 sensor support"
+   depends on I2C && VIDEO_V4L2
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV7740 VGA camera sensor.
+
  config VIDEO_OV9650
tristate "OmniVision OV9650/OV9652 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 548a9efce966..9b19ec7fcaf4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
  obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
+obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
  obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
  obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
new file mode 100644
index ..26b56a627377
--- /dev/null
+++ b/drivers/media/i2c/ov7740.c
@@ -0,0 +1,1234 @@
+/*
+ * Copyright (c) 2017 Microchip Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_OUTSIZE_LSB 0x34
+
+/* OV7740 register tables */
+#define REG_GAIN   0x00/* Gain lower 8 bits (rest in vref) */
+#define REG_BGAIN  0x01/* blue gain */
+#define REG_RGAIN  0x02/* red gain */
+#define REG_GGAIN  0x03/* green gain */
+#define REG_REG04  0x04/* analog setting, dont change*/
+#define REG_BAVG   0x05/* b channel average */
+#define REG_GAVG   0x06/* g channel average */
+#define REG_RAVG   0x07/* r channel average */
+
+#define REG_REG0C  0x0C/* filp enable */
+#define 

Re: [PATCH v6 2/2] media: i2c: Add the ov7740 image sensor driver

[Yang, Wenyou]

Hi Sakari,


On 2017/12/5 18:45, Sakari Ailus wrote:

Hi Wenyou,

On Mon, Dec 04, 2017 at 02:58:58PM +0800, Wenyou Yang wrote:

The ov7740 (color) image sensor is a high performance VGA CMOS
image snesor, which supports for output formats: RAW RGB and YUV
and image sizes: VGA, and QVGA, CIF and any size smaller.

Signed-off-by: Songjun Wu 
Signed-off-by: Wenyou Yang 
---

Changes in v6:
  - Remove unnecessary #include .
  - Remove unnecessary comments and extra newline.
  - Add const for some structures.
  - Add the check of the return value from regmap_write().
  - Simplify the calling of __v4l2_ctrl_handler_setup().
  - Add the default format initialization function.
  - Integrate the set_power() and enable/disable the clock into
one function.

Changes in v5:
  - Squash the driver and MAINTAINERS entry patches to one.
  - Precede the driver patch with the bindings patch.

Changes in v4:
  - Assign 'val' a initial value to avoid warning: 'val' may be
used uninitialized.
  - Rename REG_REG15 to avoid warning: "REG_REG15" redefined.

Changes in v3:
  - Put the MAINTAINERS change to a separate patch.

Changes in v2:
  - Split off the bindings into a separate patch.
  - Add a new entry to the MAINTAINERS file.

  MAINTAINERS|8 +
  drivers/media/i2c/Kconfig  |8 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov7740.c | 1226 
  4 files changed, 1243 insertions(+)
  create mode 100644 drivers/media/i2c/ov7740.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7a52a66aa991..1de965009b13 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10053,6 +10053,14 @@ S: Maintained
  F:drivers/media/i2c/ov7670.c
  F:Documentation/devicetree/bindings/media/i2c/ov7670.txt
  
+OMNIVISION OV7740 SENSOR DRIVER

+M: Wenyou Yang 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/ov7740.c
+F: Documentation/devicetree/bindings/media/i2c/ov7740.txt
+
  ONENAND FLASH DRIVER
  M:Kyungmin Park 
  L:linux-...@lists.infradead.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index cb5d7ff82915..00b1c4c031d4 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -665,6 +665,14 @@ config VIDEO_OV7670
  OV7670 VGA camera.  It currently only works with the M88ALP01
  controller.
  
+config VIDEO_OV7740

+   tristate "OmniVision OV7740 sensor support"
+   depends on I2C && VIDEO_V4L2
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV7740 VGA camera sensor.
+
  config VIDEO_OV9650
tristate "OmniVision OV9650/OV9652 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 548a9efce966..9b19ec7fcaf4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
  obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
+obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
  obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
  obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
new file mode 100644
index ..42c25277d005
--- /dev/null
+++ b/drivers/media/i2c/ov7740.c
@@ -0,0 +1,1226 @@
+/*
+ * Copyright (c) 2017 Microchip Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_OUTSIZE_LSB 0x34
+
+/* OV7740 register tables */
+#define REG_GAIN   0x00/* Gain lower 8 bits (rest in vref) */
+#define REG_BGAIN  0x01/* blue gain */
+#define REG_RGAIN  0x02/* red gain */
+#define REG_GGAIN  0x03/* green gain */
+#define REG_REG04  0x04/* analog setting, dont change*/
+#define REG_BAVG   0x05/* b channel average */
+#define REG_GAVG   0x06/* g channel average */
+#define REG_RAVG   0x07/* r channel average */
+
+#define REG_REG0C  0x0C/* filp enable */
+#define REG0C_IMG_FLIP 0x80
+#define REG0C_IMG_MIRROR   0x40
+
+#define REG_REG0E  0x0E/* blc line */
+#define REG_HAEC 

Re: [PATCH v5 2/2] media: i2c: Add the ov7740 image sensor driver

[Yang, Wenyou]

Hi Sakari,

Sorry for late answer.

Thank you for your review.

On 2017/11/28 20:06, Sakari Ailus wrote:

Hi Wenyou,

Thanks for the patch. Some comments below.

On Tue, Nov 28, 2017 at 01:22:59PM +0800, Wenyou Yang wrote:

The ov7740 (color) image sensor is a high performance VGA CMOS
image snesor, which supports for output formats: RAW RGB and YUV
and image sizes: VGA, and QVGA, CIF and any size smaller.

Signed-off-by: Songjun Wu 
Signed-off-by: Wenyou Yang 
---

Changes in v5:
  - Squash the driver and MAINTAINERS entry patches to one.
  - Precede the driver patch with the bindings patch.

Changes in v4:
  - Assign 'val' a initial value to avoid warning: 'val' may be
used uninitialized.
  - Rename REG_REG15 to avoid warning: "REG_REG15" redefined.

Changes in v3:
  - Put the MAINTAINERS change to a separate patch.

Changes in v2:
  - Split off the bindings into a separate patch.
  - Add a new entry to the MAINTAINERS file.

  MAINTAINERS|8 +
  drivers/media/i2c/Kconfig  |8 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov7740.c | 1220 
  4 files changed, 1237 insertions(+)
  create mode 100644 drivers/media/i2c/ov7740.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aa71ab52fd76..19086a073ae9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10053,6 +10053,14 @@ S: Maintained
  F:drivers/media/i2c/ov7670.c
  F:Documentation/devicetree/bindings/media/i2c/ov7670.txt
  
+OMNIVISION OV7740 SENSOR DRIVER

+M: Wenyou Yang 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/ov7740.c
+F: Documentation/devicetree/bindings/media/i2c/ov7740.txt
+
  ONENAND FLASH DRIVER
  M:Kyungmin Park 
  L:linux-...@lists.infradead.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 3c6d6428f525..ac484bb82fae 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -665,6 +665,14 @@ config VIDEO_OV7670
  OV7670 VGA camera.  It currently only works with the M88ALP01
  controller.
  
+config VIDEO_OV7740

+   tristate "OmniVision OV7740 sensor support"
+   depends on I2C && VIDEO_V4L2
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV7740 VGA camera sensor.
+
  config VIDEO_OV9650
tristate "OmniVision OV9650/OV9652 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 548a9efce966..9b19ec7fcaf4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
  obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
+obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
  obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
  obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
new file mode 100644
index ..b2ec015bf3f6
--- /dev/null
+++ b/drivers/media/i2c/ov7740.c
@@ -0,0 +1,1220 @@
+/*
+ * Copyright (c) 2017 Microchip Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 

Do you need init.h?

No, will remove it.



+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_OUTSIZE_LSB 0x34
+
+/* OV7740 register tables */
+#define REG_GAIN   0x00/* Gain lower 8 bits (rest in vref) */
+#define REG_BGAIN  0x01/* blue gain */
+#define REG_RGAIN  0x02/* red gain */
+#define REG_GGAIN  0x03/* green gain */
+#define REG_REG04  0x04/* analog setting, dont change*/
+#define REG_BAVG   0x05/* b channel average */
+#define REG_GAVG   0x06/* g channel average */
+#define REG_RAVG   0x07/* r channel average */
+
+#define REG_REG0C  0x0C/* filp enable */
+#define REG0C_IMG_FLIP 0x80
+#define REG0C_IMG_MIRROR   0x40
+
+#define REG_REG0E  0x0E/* blc line */
+#define REG_HAEC   0x0F/* auto exposure cntrl */
+#define REG_AEC0x10/* auto exposure cntrl */
+
+#define REG_CLK0x11/* Clock control */
+#define REG_REG55  0x55/* Clock PLL DIV/PreDiv */
+

Re: [PATCH] media: atmel-isc: get rid of an unused var

[Yang, Wenyou]

On 2017/10/31 17:37, Mauro Carvalho Chehab wrote:

drivers/media/platform/atmel/atmel-isc.c: In function 'isc_async_complete':
drivers/media/platform/atmel/atmel-isc.c:1900:28: warning: variable 'sd_entity' 
set but not used [-Wunused-but-set-variable]
   struct isc_subdev_entity *sd_entity;
 ^

Signed-off-by: Mauro Carvalho Chehab 


Acked-by: Wenyou Yang 


---
  drivers/media/platform/atmel/atmel-isc.c | 3 ---
  1 file changed, 3 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 2c40a7886542..8b37656f035d 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1897,7 +1897,6 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
  {
struct isc_device *isc = container_of(notifier->v4l2_dev,
  struct isc_device, v4l2_dev);
-   struct isc_subdev_entity *sd_entity;
struct video_device *vdev = >video_dev;
struct vb2_queue *q = >vb2_vidq;
int ret;
@@ -1910,8 +1909,6 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
  
  	isc->current_subdev = container_of(notifier,

   struct isc_subdev_entity, notifier);
-   sd_entity = isc->current_subdev;
-
mutex_init(>lock);
init_completion(>comp);
  


Best Regards,
Wenyou Yang

Re: [PATCH v2 2/2] media: ov7740: Document device tree bindings

[Yang, Wenyou]

Hi Sakari,

Thank you very much for your review and advice.

I will send a new version to fix it.

On 2017/10/27 18:59, Sakari Ailus wrote:

Hi Wenyou,

On Fri, Oct 27, 2017 at 03:42:20PM +0800, Wenyou Yang wrote:

Add the device tree binding documentation for ov7740 driver and
add a new entry of ov7740 to the MAINTAINERS file.

Signed-off-by: Wenyou Yang 
---

Changes in v2:
  - Split off the bindings into a separate patch.
  - Add a new entry to the MAINTAINERS file.

  .../devicetree/bindings/media/i2c/ov7740.txt   | 43 ++
  MAINTAINERS|  8 
  2 files changed, 51 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7740.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov7740.txt 
b/Documentation/devicetree/bindings/media/i2c/ov7740.txt
new file mode 100644
index ..b306e5aa97bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov7740.txt
@@ -0,0 +1,43 @@
+* Omnivision OV7740 CMOS image sensor
+
+The Omnivision OV7740 image sensor supports multiple output image
+size, such as VGA, and QVGA, CIF and any size smaller. It also
+supports the RAW RGB and YUV output formats.
+
+Required Properties:
+- compatible: should be "ovti,ov7740"
+- clocks: reference to the xvclk input clock.
+- clock-names: should be "xvclk".
+
+Optional Properties:
+- reset-gpios: reference to the GPIO connected to the reset_b pin,
+  if any. Active low with pull-ip resistor.
+- powerdown-gpios: reference to the GPIO connected to the pwdn pin,
+  if any. Active high with pull-down resistor.
+
+The device node must contain one 'port' child node for its digital
+output video port, in accordance with the video interface bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.

Could you add there's a single endpoint node as well, and explicitly
document the relevant properties? E.g. as in
Documentation/devicetree/bindings/media/i2c/nokia,smia.txt .


+
+Example:
+
+   i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_sensor_power 
_sensor_reset>;
+   clocks = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+   reset-gpios = < 43 GPIO_ACTIVE_LOW>;
+   powerdown-gpios = < 44 GPIO_ACTIVE_HIGH>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+   };
+   };
diff --git a/MAINTAINERS b/MAINTAINERS
index 90230fe020f3..f0f3f121d1d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9965,6 +9965,14 @@ S:   Maintained
  F:drivers/media/i2c/ov7670.c
  F:Documentation/devicetree/bindings/media/i2c/ov7670.txt
  
+OMNIVISION OV7740 SENSOR DRIVER

+M: Wenyou Yang 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/ov7740.c
+F: Documentation/devicetree/bindings/media/i2c/ov7740.txt
+
  ONENAND FLASH DRIVER
  M:Kyungmin Park 
  L:linux-...@lists.infradead.org

Please put the MAINTAINERS change to the driver patch after the DT binding
patch.


Best Regards,
Wenyou Yang

Re: [PATCH v5 0/5] media: atmel-isc: Rework the format list and clock provider

[Yang, Wenyou]

Hi Hans,


On 2017/10/27 20:41, Hans Verkuil wrote:

Hi Wenyou,

Unfortunately the v4 patch series was just merged instead of v5. Can you make a 
new
patch applying just the v4 -> v5 changes?

Thank you for your merging.

Of course, I will send a patch to fix it.



Thanks!

Hans

On 10/27/2017 05:21 AM, Wenyou Yang wrote:

To improve the readability of code, rework the format list table,
split the format array into two. Meanwhile, fix the issue of the
clock provider operation and the pm runtime support.

Changes in v5:
  - Fix the clock ID which enters the runtime suspend should be
ISC_ISPCK, instead of ISC_MCK for clk_prepare/clk_unprepare().
  - Fix the clock ID to ISC_ISPCK, instead of ISC_MCK for
isc_clk_is_enabled().

Changes in v4:
  - Call pm_runtime_get_sync() and pm_runtime_put_sync() in ->prepare
and ->unprepare callback.
  - Move pm_runtime_enable() call from the complete callback to the
end of probe.
  - Call pm_runtime_get_sync() and pm_runtime_put_sync() in
->is_enabled() callbacks.
  - Call clk_disable_unprepare() in ->remove callback.

Changes in v3:
  - Fix the wrong used spinlock.
  - s/_/- on the subject.
  - Add a new flag for Raw Bayer format to remove MAX_RAW_FMT_INDEX define.
  - Add the comments for define of the format flag.
  - Rebase media_tree/master.

Changes in v2:
  - Add the new patch to remove the unnecessary member from
isc_subdev_entity struct.
  - Rebase on the patch set,
 [PATCH 0/6] [media] Atmel: Adjustments for seven function 
implementations
 https://www.mail-archive.com/linux-media@vger.kernel.org/msg118342.html

Wenyou Yang (5):
   media: atmel-isc: Add spin lock for clock enable ops
   media: atmel-isc: Add prepare and unprepare ops
   media: atmel-isc: Enable the clocks during probe
   media: atmel-isc: Remove unnecessary member
   media: atmel-isc: Rework the format list

  drivers/media/platform/atmel/atmel-isc-regs.h |   1 +
  drivers/media/platform/atmel/atmel-isc.c  | 629 --
  2 files changed, 498 insertions(+), 132 deletions(-)


Best Regards,
Wenyou Yang

Re: [PATCH] media: i2c: Add the ov7740 image sensor driver

[Yang, Wenyou]

On 2017/10/27 15:10, Hans Verkuil wrote:

On 10/27/2017 05:30 AM, Wenyou Yang wrote:

From: Songjun Wu 

The ov7740 (color) image sensor is a high performance VGA CMOS
image snesor, which supports for output formats: RAW RGB and YUV
and image sizes: VGA, and QVGA, CIF and any size smaller.

Two short remarks: please split off the bindings into a separate
patch. And add a new entry to the MAINTAINERS file.

Thanks a lot.
Will be done in the next version.



Regards,

Hans


Signed-off-by: Songjun Wu 
Signed-off-by: Wenyou Yang 
---

  .../devicetree/bindings/media/i2c/ov7740.txt   |   43 +
  drivers/media/i2c/Kconfig  |8 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov7740.c | 1220 
  4 files changed, 1272 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7740.txt
  create mode 100644 drivers/media/i2c/ov7740.c

diff --git a/Documentation/devicetree/bindings/media/i2c/ov7740.txt 
b/Documentation/devicetree/bindings/media/i2c/ov7740.txt
new file mode 100644
index ..b306e5aa97bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov7740.txt
@@ -0,0 +1,43 @@
+* Omnivision OV7740 CMOS image sensor
+
+The Omnivision OV7740 image sensor supports multiple output image
+size, such as VGA, and QVGA, CIF and any size smaller. It also
+supports the RAW RGB and YUV output formats.
+
+Required Properties:
+- compatible: should be "ovti,ov7740"
+- clocks: reference to the xvclk input clock.
+- clock-names: should be "xvclk".
+
+Optional Properties:
+- reset-gpios: reference to the GPIO connected to the reset_b pin,
+  if any. Active low with pull-ip resistor.
+- powerdown-gpios: reference to the GPIO connected to the pwdn pin,
+  if any. Active high with pull-down resistor.
+
+The device node must contain one 'port' child node for its digital
+output video port, in accordance with the video interface bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+   i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_sensor_power 
_sensor_reset>;
+   clocks = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+   reset-gpios = < 43 GPIO_ACTIVE_LOW>;
+   powerdown-gpios = < 44 GPIO_ACTIVE_HIGH>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+   };
+   };
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 47113774a297..402ad0e4024c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -657,6 +657,14 @@ config VIDEO_OV7670
  OV7670 VGA camera.  It currently only works with the M88ALP01
  controller.
  
+config VIDEO_OV7740

+   tristate "OmniVision OV7740 sensor support"
+   depends on I2C && VIDEO_V4L2
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV7740 VGA camera sensor.
+
  config VIDEO_OV9650
tristate "OmniVision OV9650/OV9652 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c843c181dfb9..f94cb2a8ed91 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
  obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
+obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
  obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
  obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
new file mode 100644
index ..d68add05b46e
--- /dev/null
+++ b/drivers/media/i2c/ov7740.c
@@ -0,0 +1,1220 @@
+/*
+ * Copyright (c) 2017 Microchip Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 

Re: [PATCH 0/4] media: ov7670: add media controller support

[Yang, Wenyou]

Hi Sakari,


On 2017/10/14 4:48, Sakari Ailus wrote:

Hi Akinobu,

On Fri, Oct 13, 2017 at 01:21:13AM +0900, Akinobu Mita wrote:

This series adds media controller support and other related changes to the
OV7670 which is cheap and highly available CMOS image sensor for hobbyists.

This enables to control a video pipeline system with the OV7670.  I've
tested this with the xilinx video IP pipeline.

Akinobu Mita (4):
   media: ov7670: create subdevice device node
   media: ov7670: use v4l2_async_unregister_subdev()
   media: ov7670: add media controller support
   media: ov7670: add get_fmt() pad ops callback

  drivers/media/i2c/ov7670.c | 55 +-
  1 file changed, 54 insertions(+), 1 deletion(-)

I understand Wenyou has submitted a set with similar functionality +
runtime PM as well, but it has issues. There hasn't been updated for some
time on that. Wenyou, what's the status of your patchset?

Sorry for I missed your previous mail.

I will update the patchset soon.

The set was submitted on the list under subject "[PATCH v5 0/3] media:
ov7670: Add entity init and power operation"



Best Regards,
Wenyou Yang

Re: [PATCH v3 3/5] media: atmel-isc: Enable the clocks during probe

[Yang, Wenyou]

Hi Sakari,

On 2017/10/9 15:58, Sakari Ailus wrote:

Hi
Wenyou,

On Mon, Oct 09, 2017 at 01:49:44PM +0800, Yang, Wenyou wrote:

Hi Sakari,

Sorry for late answer, because I was in vacation last week.

On 2017/9/29 5:25, Sakari Ailus wrote:

Hi Wenyou,

On Thu, Sep 28, 2017 at 04:18:26PM +0800, Wenyou Yang wrote:

To meet the relationship, enable the HCLOCK and ispck during the
device probe, "isc_pck frequency is less than or equal to isc_ispck,
and isc_ispck is greater than or equal to HCLOCK."
Meanwhile, call the pm_runtime_enable() in the right place.

Signed-off-by: Wenyou Yang <wenyou.y...@microchip.com>
---

Changes in v3: None
Changes in v2: None

   drivers/media/platform/atmel/atmel-isc.c | 31 +--
   1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 0b15dc1a3a0b..f092c95587c1 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1594,6 +1594,7 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
struct isc_subdev_entity *sd_entity;
struct video_device *vdev = >video_dev;
struct vb2_queue *q = >vb2_vidq;
+   struct device *dev = isc->dev;
int ret;
ret = v4l2_device_register_subdev_nodes(>v4l2_dev);
@@ -1677,6 +1678,10 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
return ret;
}
+   pm_runtime_set_active(dev);
+   pm_runtime_enable(dev);
+   pm_request_idle(dev);

Remember that the driver's async complete function could never get called.

What would be the reason to move it here?

The ISC provides the clock for the sensor, namely, it is the clock provider
for the external sensor.
So it keeps active to make the sensor probe successfully.
Otherwise, the sensor, such as 0v7670 fails to probe due to the failure to
clk_enable().

You'll still need to balance the get and put calls.

complete callback is not necessarily called at all or could be called
multiple times. Instead, you should probably do pm_runtime_get_sync() when
the clock is enabled and put when it's disabled.

I will send v4 to update it.

Thank you for your advice.

Best Regards,
Wenyou Yang

Re: [PATCH v3 3/5] media: atmel-isc: Enable the clocks during probe

[Yang, Wenyou]

Hi Sakari,

Sorry for late answer, because I was in vacation last week.

On 2017/9/29 5:25, Sakari Ailus wrote:

Hi Wenyou,

On Thu, Sep 28, 2017 at 04:18:26PM +0800, Wenyou Yang wrote:

To meet the relationship, enable the HCLOCK and ispck during the
device probe, "isc_pck frequency is less than or equal to isc_ispck,
and isc_ispck is greater than or equal to HCLOCK."
Meanwhile, call the pm_runtime_enable() in the right place.

Signed-off-by: Wenyou Yang 
---

Changes in v3: None
Changes in v2: None

  drivers/media/platform/atmel/atmel-isc.c | 31 +--
  1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 0b15dc1a3a0b..f092c95587c1 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1594,6 +1594,7 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
struct isc_subdev_entity *sd_entity;
struct video_device *vdev = >video_dev;
struct vb2_queue *q = >vb2_vidq;
+   struct device *dev = isc->dev;
int ret;
  
  	ret = v4l2_device_register_subdev_nodes(>v4l2_dev);

@@ -1677,6 +1678,10 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
return ret;
}
  
+	pm_runtime_set_active(dev);

+   pm_runtime_enable(dev);
+   pm_request_idle(dev);

Remember that the driver's async complete function could never get called.

What would be the reason to move it here?
The ISC provides the clock for the sensor, namely, it is the clock 
provider for the external sensor.

So it keeps active to make the sensor probe successfully.
Otherwise, the sensor, such as 0v7670 fails to probe due to the failure 
to clk_enable().



+
return 0;
  }
  
@@ -1856,25 +1861,37 @@ static int atmel_isc_probe(struct platform_device *pdev)

return ret;
}
  
+	ret = clk_prepare_enable(isc->hclock);

+   if (ret) {
+   dev_err(dev, "failed to enable hclock: %d\n", ret);
+   return ret;
+   }
+
ret = isc_clk_init(isc);
if (ret) {
dev_err(dev, "failed to init isc clock: %d\n", ret);
-   goto clean_isc_clk;
+   goto unprepare_hclk;
}
  
  	isc->ispck = isc->isc_clks[ISC_ISPCK].clk;
  
+	ret = clk_prepare_enable(isc->ispck);

+   if (ret) {
+   dev_err(dev, "failed to enable ispck: %d\n", ret);
+   goto unprepare_hclk;
+   }
+
/* ispck should be greater or equal to hclock */
ret = clk_set_rate(isc->ispck, clk_get_rate(isc->hclock));
if (ret) {
dev_err(dev, "failed to set ispck rate: %d\n", ret);
-   goto clean_isc_clk;
+   goto unprepare_clk;
}
  
  	ret = v4l2_device_register(dev, >v4l2_dev);

if (ret) {
dev_err(dev, "unable to register v4l2 device.\n");
-   goto clean_isc_clk;
+   goto unprepare_clk;
}
  
  	ret = isc_parse_dt(dev, isc);

@@ -1907,8 +1924,6 @@ static int atmel_isc_probe(struct platform_device *pdev)
break;
}
  
-	pm_runtime_enable(dev);

-
return 0;
  
  cleanup_subdev:

@@ -1917,7 +1932,11 @@ static int atmel_isc_probe(struct platform_device *pdev)
  unregister_v4l2_device:
v4l2_device_unregister(>v4l2_dev);
  
-clean_isc_clk:

+unprepare_clk:
+   clk_disable_unprepare(isc->ispck);
+unprepare_hclk:
+   clk_disable_unprepare(isc->hclock);

I think you're missing clk_disable_unprepare() in the driver's remove
callback.

Will add in next version.



+
isc_clk_cleanup(isc);
  
  	return ret;


Best Regards,
Wenyou Yang

Re: [PATCH v2 1/5] media: atmel_isc: Add spin lock for clock enable ops

[Yang, Wenyou]

Hi Sakari,


On 2017/9/27 15:16, Sakari Ailus wrote:

Hi Wenyou,

On subject:

s/_/-/

On Mon, Sep 18, 2017 at 02:39:21PM +0800, Wenyou Yang wrote:

Add the spin lock for the clock enable and disable operations.

Signed-off-by: Wenyou Yang 
---

Changes in v2: None

  drivers/media/platform/atmel/atmel-isc.c | 14 +-
  1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 2f8e345d297e..78114193af4c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -65,6 +65,7 @@ struct isc_clk {
struct clk_hw   hw;
struct clk  *clk;
struct regmap   *regmap;
+   spinlock_t  *lock;

Can this work? I don't see lock being assigned anywhere. Did you mean

spinlock_t  lock;

?

Ooh. I made a mistake.

Thank you very much.



u8  id;
u8  parent_id;
u32 div;
@@ -312,26 +313,37 @@ static int isc_clk_enable(struct clk_hw *hw)
struct isc_clk *isc_clk = to_isc_clk(hw);
u32 id = isc_clk->id;
struct regmap *regmap = isc_clk->regmap;
+   unsigned long flags;
+   unsigned int status;
  
  	dev_dbg(isc_clk->dev, "ISC CLK: %s, div = %d, parent id = %d\n",

__func__, isc_clk->div, isc_clk->parent_id);
  
+	spin_lock_irqsave(isc_clk->lock, flags);

regmap_update_bits(regmap, ISC_CLKCFG,
   ISC_CLKCFG_DIV_MASK(id) | ISC_CLKCFG_SEL_MASK(id),
   (isc_clk->div << ISC_CLKCFG_DIV_SHIFT(id)) |
   (isc_clk->parent_id << ISC_CLKCFG_SEL_SHIFT(id)));
  
  	regmap_write(regmap, ISC_CLKEN, ISC_CLK(id));

+   spin_unlock_irqrestore(isc_clk->lock, flags);
  
-	return 0;

+   regmap_read(regmap, ISC_CLKSR, );
+   if (status & ISC_CLK(id))
+   return 0;
+   else
+   return -EINVAL;
  }
  
  static void isc_clk_disable(struct clk_hw *hw)

  {
struct isc_clk *isc_clk = to_isc_clk(hw);
u32 id = isc_clk->id;
+   unsigned long flags;
  
+	spin_lock_irqsave(isc_clk->lock, flags);

regmap_write(isc_clk->regmap, ISC_CLKDIS, ISC_CLK(id));
+   spin_unlock_irqrestore(isc_clk->lock, flags);
  }
  
  static int isc_clk_is_enabled(struct clk_hw *hw)

Best Regards,
Wenyou Yang

Re: [PATCH v2 5/5] media: atmel-isc: Rework the format list

[Yang, Wenyou]

On 2017/9/27 16:03, Hans Verkuil wrote:

On 09/27/2017 07:15 AM, Yang, Wenyou wrote:

Hi Hans,

Thank  you very much for your review.

On 2017/9/25 21:24, Hans Verkuil wrote:

Hi Wenyou,

On 18/09/17 08:39, Wenyou Yang wrote:

To improve the readability of code, split the format array into two,
one for the format description, other for the register configuration.
Meanwhile, add the flag member to indicate the format can be achieved
from the sensor or be produced by the controller, and rename members
related to the register configuration.

Also add more formats support: GREY, ARGB444, ARGB555 and ARGB32.

Signed-off-by: Wenyou Yang <wenyou.y...@microchip.com>

This looks better. Just a few comments, see below.


---

Changes in v2:
   - Add the new patch to remove the unnecessary member from
 isc_subdev_entity struct.
   - Rebase on the patch set,
  [PATCH 0/6] [media] Atmel: Adjustments for seven function 
implementations
  
https://www.mail-archive.com/linux-media@vger.kernel.org/msg118342.html

   drivers/media/platform/atmel/atmel-isc.c | 524 
---
   1 file changed, 405 insertions(+), 119 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 2d876903da71..90bd0b28a975 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -89,34 +89,56 @@ struct isc_subdev_entity {
struct list_head list;
   };
   
+#define FMT_FLAG_FROM_SENSOR		BIT(0)

+#define FMT_FLAG_FROM_CONTROLLER   BIT(1)

Document the meaning of these flags.

Will add it in next version.

+
   /*
* struct isc_format - ISC media bus format information
* @fourcc:  Fourcc code for this format
* @mbus_code:   V4L2 media bus format code.
+ * flags:  Indicate format from sensor or converted by controller
* @bpp: Bits per pixel (when stored in memory)
- * @reg_bps:   reg value for bits per sample
*   (when transferred over a bus)
- * @pipeline:  pipeline switch
* @sd_support:  Subdev supports this format
* @isc_support: ISC can convert raw format to this format
*/
+
   struct isc_format {
u32 fourcc;
u32 mbus_code;
+   u32 flags;
u8  bpp;
   
-	u32	reg_bps;

-   u32 reg_bay_cfg;
-   u32 reg_rlp_mode;
-   u32 reg_dcfg_imode;
-   u32 reg_dctrl_dview;
-
-   u32 pipeline;
-
boolsd_support;
boolisc_support;
   };
   
+/* Pipeline bitmap */

+#define WB_ENABLE  BIT(0)
+#define CFA_ENABLE BIT(1)
+#define CC_ENABLE  BIT(2)
+#define GAM_ENABLE BIT(3)
+#define GAM_BENABLEBIT(4)
+#define GAM_GENABLEBIT(5)
+#define GAM_RENABLEBIT(6)
+#define CSC_ENABLE BIT(7)
+#define CBC_ENABLE BIT(8)
+#define SUB422_ENABLE  BIT(9)
+#define SUB420_ENABLE  BIT(10)
+
+#define GAM_ENABLES(GAM_RENABLE | GAM_GENABLE | GAM_BENABLE | GAM_ENABLE)
+
+struct fmt_config {
+   u32 fourcc;
+
+   u32 pfe_cfg0_bps;
+   u32 cfa_baycfg;
+   u32 rlp_cfg_mode;
+   u32 dcfg_imode;
+   u32 dctrl_dview;
+
+   u32 bits_pipeline;
+};
   
   #define HIST_ENTRIES		512

   #define HIST_BAYER   (ISC_HIS_CFG_MODE_B + 1)
@@ -181,80 +203,321 @@ struct isc_device {
struct list_headsubdev_entities;
   };
   
-#define RAW_FMT_IND_START0

-#define RAW_FMT_IND_END  11
-#define ISC_FMT_IND_START12
-#define ISC_FMT_IND_END  14
-
-static struct isc_format isc_formats[] = {
-   { V4L2_PIX_FMT_SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-
-   { V4L2_PIX_FMT_SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKE

Re: [PATCH v2 5/5] media: atmel-isc: Rework the format list

[Yang, Wenyou]

Hi Hans,

Thank  you very much for your review.

On 2017/9/25 21:24, Hans Verkuil wrote:

Hi Wenyou,

On 18/09/17 08:39, Wenyou Yang wrote:

To improve the readability of code, split the format array into two,
one for the format description, other for the register configuration.
Meanwhile, add the flag member to indicate the format can be achieved
from the sensor or be produced by the controller, and rename members
related to the register configuration.

Also add more formats support: GREY, ARGB444, ARGB555 and ARGB32.

Signed-off-by: Wenyou Yang 

This looks better. Just a few comments, see below.


---

Changes in v2:
  - Add the new patch to remove the unnecessary member from
isc_subdev_entity struct.
  - Rebase on the patch set,
 [PATCH 0/6] [media] Atmel: Adjustments for seven function 
implementations
 https://www.mail-archive.com/linux-media@vger.kernel.org/msg118342.html

  drivers/media/platform/atmel/atmel-isc.c | 524 ---
  1 file changed, 405 insertions(+), 119 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 2d876903da71..90bd0b28a975 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -89,34 +89,56 @@ struct isc_subdev_entity {
struct list_head list;
  };
  
+#define FMT_FLAG_FROM_SENSOR		BIT(0)

+#define FMT_FLAG_FROM_CONTROLLER   BIT(1)

Document the meaning of these flags.

Will add it in next version.



+
  /*
   * struct isc_format - ISC media bus format information
   * @fourcc:   Fourcc code for this format
   * @mbus_code:V4L2 media bus format code.
+ * flags:  Indicate format from sensor or converted by controller
   * @bpp:  Bits per pixel (when stored in memory)
- * @reg_bps:   reg value for bits per sample
   *(when transferred over a bus)
- * @pipeline:  pipeline switch
   * @sd_support:   Subdev supports this format
   * @isc_support:  ISC can convert raw format to this format
   */
+
  struct isc_format {
u32 fourcc;
u32 mbus_code;
+   u32 flags;
u8  bpp;
  
-	u32	reg_bps;

-   u32 reg_bay_cfg;
-   u32 reg_rlp_mode;
-   u32 reg_dcfg_imode;
-   u32 reg_dctrl_dview;
-
-   u32 pipeline;
-
boolsd_support;
boolisc_support;
  };
  
+/* Pipeline bitmap */

+#define WB_ENABLE  BIT(0)
+#define CFA_ENABLE BIT(1)
+#define CC_ENABLE  BIT(2)
+#define GAM_ENABLE BIT(3)
+#define GAM_BENABLEBIT(4)
+#define GAM_GENABLEBIT(5)
+#define GAM_RENABLEBIT(6)
+#define CSC_ENABLE BIT(7)
+#define CBC_ENABLE BIT(8)
+#define SUB422_ENABLE  BIT(9)
+#define SUB420_ENABLE  BIT(10)
+
+#define GAM_ENABLES(GAM_RENABLE | GAM_GENABLE | GAM_BENABLE | GAM_ENABLE)
+
+struct fmt_config {
+   u32 fourcc;
+
+   u32 pfe_cfg0_bps;
+   u32 cfa_baycfg;
+   u32 rlp_cfg_mode;
+   u32 dcfg_imode;
+   u32 dctrl_dview;
+
+   u32 bits_pipeline;
+};
  
  #define HIST_ENTRIES		512

  #define HIST_BAYER(ISC_HIS_CFG_MODE_B + 1)
@@ -181,80 +203,321 @@ struct isc_device {
struct list_headsubdev_entities;
  };
  
-#define RAW_FMT_IND_START0

-#define RAW_FMT_IND_END  11
-#define ISC_FMT_IND_START12
-#define ISC_FMT_IND_END  14
-
-static struct isc_format isc_formats[] = {
-   { V4L2_PIX_FMT_SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-
-   { V4L2_PIX_FMT_SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-   { V4L2_PIX_FMT_SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10, 16,
- 

Re: [PATCH v4 3/3] media: ov7670: Add the s_power operation

[Yang, Wenyou]

Hi Sakari,


On 2017/9/18 15:35, Sakari Ailus wrote:

Hi Wenyou,

Thanks for the update.

The driver exposes controls that are accessible through the sub-device node
even if the device hasn't been powered on.

Many ISP and bridge drivers will also power the sensor down once the last
user of the user space device nodes disappears. You could keep the device
powered at all times or change the behaviour so that controls can be
accessed when the power is off.

The best option would be to convert the driver to use runtime PM.

Yes, I agree with you.
I also noticed there are a lot of things needed to improve except for 
it, such as the platform data via device tree.

I would like do it in another patch set. I will continue to work on it.

An example of this can be found in drivers/media/i2c/ov13858.c .

A good example, thank you for your providing.


On Mon, Sep 18, 2017 at 02:45:14PM +0800, Wenyou Yang wrote:

Add the s_power operation which is responsible for manipulating the
power dowm mode through the PWDN pin and the reset operation through
the RESET pin.

Signed-off-by: Wenyou Yang 
---

Changes in v4: None
Changes in v3: None
Changes in v2:
  - Add the patch to support the get_fmt ops.
  - Remove the redundant invoking ov7670_init_gpio().

  drivers/media/i2c/ov7670.c | 32 +++-
  1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 456f48057605..304abc769a67 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1542,6 +1542,22 @@ static int ov7670_s_register(struct v4l2_subdev *sd, 
const struct v4l2_dbg_regis
  }
  #endif
  
+static int ov7670_s_power(struct v4l2_subdev *sd, int on)

+{
+   struct ov7670_info *info = to_state(sd);
+
+   if (info->pwdn_gpio)
+   gpiod_direction_output(info->pwdn_gpio, !on);
+   if (on && info->resetb_gpio) {
+   gpiod_set_value(info->resetb_gpio, 1);
+   usleep_range(500, 1000);
+   gpiod_set_value(info->resetb_gpio, 0);
+   usleep_range(3000, 5000);
+   }
+
+   return 0;
+}
+
  static void ov7670_get_default_format(struct v4l2_subdev *sd,
  struct v4l2_mbus_framefmt *format)
  {
@@ -1575,6 +1591,7 @@ static const struct v4l2_subdev_core_ops ov7670_core_ops 
= {
.g_register = ov7670_g_register,
.s_register = ov7670_s_register,
  #endif
+   .s_power = ov7670_s_power,
  };
  
  static const struct v4l2_subdev_video_ops ov7670_video_ops = {

@@ -1692,23 +1709,25 @@ static int ov7670_probe(struct i2c_client *client,
if (ret)
return ret;
  
-	ret = ov7670_init_gpio(client, info);

-   if (ret)
-   goto clk_disable;
-
info->clock_speed = clk_get_rate(info->clk) / 100;
if (info->clock_speed < 10 || info->clock_speed > 48) {
ret = -EINVAL;
goto clk_disable;
}
  
+	ret = ov7670_init_gpio(client, info);

+   if (ret)
+   goto clk_disable;
+
+   ov7670_s_power(sd, 1);
+
/* Make sure it's an ov7670 */
ret = ov7670_detect(sd);
if (ret) {
v4l_dbg(1, debug, client,
"chip found @ 0x%x (%s) is not an ov7670 chip.\n",
client->addr << 1, client->adapter->name);
-   goto clk_disable;
+   goto power_off;
}
v4l_info(client, "chip found @ 0x%02x (%s)\n",
client->addr << 1, client->adapter->name);
@@ -1787,6 +1806,8 @@ static int ov7670_probe(struct i2c_client *client,
  #endif
  hdl_free:
v4l2_ctrl_handler_free(>hdl);
+power_off:
+   ov7670_s_power(sd, 0);
  clk_disable:
clk_disable_unprepare(info->clk);
return ret;
@@ -1804,6 +1825,7 @@ static int ov7670_remove(struct i2c_client *client)
  #if defined(CONFIG_MEDIA_CONTROLLER)
media_entity_cleanup(>sd.entity);
  #endif
+   ov7670_s_power(sd, 0);
return 0;
  }
  
--

2.13.0



Best Regards,
Wenyou Yang

Re: [PATCH v4 2/3] media: ov7670: Add the get_fmt callback

[Yang, Wenyou]

Hi Sakari,


On 2017/9/18 15:36, Sakari Ailus wrote:

Hi Wenyou,

On Mon, Sep 18, 2017 at 02:45:13PM +0800, Wenyou Yang wrote:

@@ -998,8 +1002,15 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
ret = ov7670_try_fmt_internal(sd, >format, NULL, NULL);
if (ret)
return ret;
-   cfg->try_fmt = format->format;
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+   struct v4l2_mbus_framefmt *mbus_fmt;

This will emit a compiler warning at least.

Thank you for your review.
Will be fixed in next version.



+
+   mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
+   *mbus_fmt = format->format;
return 0;
+#else
+   return -ENOTTY;
+#endif
}
  
  	ret = ov7670_try_fmt_internal(sd, >format, , );


Best Regards,
Wenyou Yang

Re: [PATCH 0/6] [media] Atmel: Adjustments for seven function implementations

[Yang, Wenyou]

Hi,


On 2017/9/5 4:04, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Mon, 4 Sep 2017 21:44:55 +0200

A few update suggestions were taken into account
from static source code analysis.

Thank you for your patches.

You can add my Acked-by for the patch series.

Acked-by: Wenyou Yang 



Markus Elfring (6):
   Delete an error message for a failed memory allocation in isc_formats_init()
   Improve a size determination in isc_formats_init()
   Adjust three checks for null pointers
   Delete an error message for a failed memory allocation in two functions
   Improve three size determinations
   Adjust a null pointer check in three functions

  drivers/media/platform/atmel/atmel-isc.c | 12 +---
  drivers/media/platform/atmel/atmel-isi.c | 20 
  2 files changed, 13 insertions(+), 19 deletions(-)



Best Regards,
Wenyou Yang

Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

[Yang, Wenyou]

Hi Hans,


On 2017/8/24 14:41, Hans Verkuil wrote:

On 08/24/2017 08:25 AM, Yang, Wenyou wrote:


On 2017/8/23 18:37, Hans Verkuil wrote:

On 08/22/17 09:30, wenyou.y...@microchip.com wrote:

Hi Hans,


-Original Message-
From: Hans Verkuil [mailto:hverk...@xs4all.nl]
Sent: 2017年8月22日 15:00
To: Wenyou Yang - A41535 <wenyou.y...@microchip.com>; Mauro Carvalho
Chehab <mche...@s-opensource.com>
Cc: Nicolas Ferre - M43238 <nicolas.fe...@microchip.com>; linux-
ker...@vger.kernel.org; Sakari Ailus <sakari.ai...@iki.fi>; Jonathan Corbet
<cor...@lwn.net>; linux-arm-ker...@lists.infradead.org; Linux Media Mailing List
<linux-media@vger.kernel.org>
Subject: Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

On 08/22/2017 03:18 AM, Yang, Wenyou wrote:

Hi Hans,

On 2017/8/21 22:07, Hans Verkuil wrote:

On 08/17/2017 09:16 AM, Wenyou Yang wrote:

The 12-bit parallel interface supports the Raw Bayer, YCbCr,
Monochrome and JPEG Compressed pixel formats from the external
sensor, not support RBG pixel format.

Signed-off-by: Wenyou Yang <wenyou.y...@microchip.com>
---

drivers/media/platform/atmel/atmel-isc.c | 5 +
1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index d4df3d4ccd85..535bb03783fe 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1478,6 +1478,11 @@ static int isc_formats_init(struct isc_device *isc)
while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
   NULL, _code)) {
mbus_code.index++;
+
+   /* Not support the RGB pixel formats from sensor */
+   if ((mbus_code.code & 0xf000) == 0x1000)
+   continue;

Am I missing something? Here you skip any RGB mediabus formats, but
in patch 3/3 you add RGB mediabus formats. But this patch prevents
those new formats from being selected, right?

This patch prevents getting the RGB format from the sensor directly.
The RGB format can be produced by ISC controller by itself.

OK, I think I see what is going on here. The isc_formats array really is two 
arrays
in one: up to RAW_FMT_IND_END it describes what it can receive from the
source, and after that it describes what it can convert it to.

Not exactly.

Yes, up to RAW_FMT_IND_END, these formats must be got from the senor, they are 
RAW formats.
  From ISC_FMT_IND_START to ISC_FMT_IND_END, they can be generated by the ISC 
controller.
It is possible they can be got from the sensor too, the driver will check it.
If it can be got from both the sensor and the ISC controller, the user can use the 
"sensor_preferred" parameter to decide from which one to get.
The RBG formats are the exception.


But if you can't handle RGB formats from the sensor, then why not make sure
none of the mbus codes in isc_formats uses RGB? That makes much more sense.

E.g.:

  { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG,
ISC_RLP_CFG_MODE_RGB565,
ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b,
false, false },

Why use MEDIA_BUS_FMT_RGB565_2X8_LE if this apparently is not supported?

This array is also the lists of all formats supported by the ISC(including got 
from the sensor).
The RGB formats are only generated by the ISC controller, not from the sensor.

You're adding code that skips any entries of the table where mbus_code is an
RGB code. But this can also be done by not having RGB mbus codes in the table
in the first place since they make no sense if the HW cannot handle that!
Set the mbus_code to e.g. 0 for such entries, that makes more sense.

I also strongly suggest changing how the table is organized since those
_FMT_IND_ indices are all to easy to get wrong (and frankly hard to understand).

Yes, you are right, I will change it. Do you have some advice?

Two options spring to mind: split into two tables or add a bool that tells 
whether
the format can be created by the isc or not.
I reworked the format table, 
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-August/529683.html

Please give your comments.



Regards,

Hans


Best Regards,
Wenyou Yang

Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

[Yang, Wenyou]

On 2017/8/23 18:37, Hans Verkuil wrote:

On 08/22/17 09:30, wenyou.y...@microchip.com wrote:

Hi Hans,


-Original Message-
From: Hans Verkuil [mailto:hverk...@xs4all.nl]
Sent: 2017年8月22日 15:00
To: Wenyou Yang - A41535 <wenyou.y...@microchip.com>; Mauro Carvalho
Chehab <mche...@s-opensource.com>
Cc: Nicolas Ferre - M43238 <nicolas.fe...@microchip.com>; linux-
ker...@vger.kernel.org; Sakari Ailus <sakari.ai...@iki.fi>; Jonathan Corbet
<cor...@lwn.net>; linux-arm-ker...@lists.infradead.org; Linux Media Mailing List
<linux-media@vger.kernel.org>
Subject: Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

On 08/22/2017 03:18 AM, Yang, Wenyou wrote:

Hi Hans,

On 2017/8/21 22:07, Hans Verkuil wrote:

On 08/17/2017 09:16 AM, Wenyou Yang wrote:

The 12-bit parallel interface supports the Raw Bayer, YCbCr,
Monochrome and JPEG Compressed pixel formats from the external
sensor, not support RBG pixel format.

Signed-off-by: Wenyou Yang <wenyou.y...@microchip.com>
---

   drivers/media/platform/atmel/atmel-isc.c | 5 +
   1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index d4df3d4ccd85..535bb03783fe 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1478,6 +1478,11 @@ static int isc_formats_init(struct isc_device *isc)
while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
   NULL, _code)) {
mbus_code.index++;
+
+   /* Not support the RGB pixel formats from sensor */
+   if ((mbus_code.code & 0xf000) == 0x1000)
+   continue;

Am I missing something? Here you skip any RGB mediabus formats, but
in patch 3/3 you add RGB mediabus formats. But this patch prevents
those new formats from being selected, right?

This patch prevents getting the RGB format from the sensor directly.
The RGB format can be produced by ISC controller by itself.

OK, I think I see what is going on here. The isc_formats array really is two 
arrays
in one: up to RAW_FMT_IND_END it describes what it can receive from the
source, and after that it describes what it can convert it to.

Not exactly.

Yes, up to RAW_FMT_IND_END, these formats must be got from the senor, they are 
RAW formats.
 From ISC_FMT_IND_START to ISC_FMT_IND_END, they can be generated by the ISC 
controller.
It is possible they can be got from the sensor too, the driver will check it.
If it can be got from both the sensor and the ISC controller, the user can use the 
"sensor_preferred" parameter to decide from which one to get.
The RBG formats are the exception.


But if you can't handle RGB formats from the sensor, then why not make sure
none of the mbus codes in isc_formats uses RGB? That makes much more sense.

E.g.:

 { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
   ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG,
ISC_RLP_CFG_MODE_RGB565,
   ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b,
   false, false },

Why use MEDIA_BUS_FMT_RGB565_2X8_LE if this apparently is not supported?

This array is also the lists of all formats supported by the ISC(including got 
from the sensor).
The RGB formats are only generated by the ISC controller, not from the sensor.

You're adding code that skips any entries of the table where mbus_code is an
RGB code. But this can also be done by not having RGB mbus codes in the table
in the first place since they make no sense if the HW cannot handle that!
Set the mbus_code to e.g. 0 for such entries, that makes more sense.

I also strongly suggest changing how the table is organized since those
_FMT_IND_ indices are all to easy to get wrong (and frankly hard to understand).

Yes, you are right, I will change it. Do you have some advice?

Thank you.



Regards,

Hans


Regards,

Hans


Regards,

Hans


+
fmt = find_format_by_code(mbus_code.code, );
if (!fmt)
continue;


Best Regards,
Wenyou Yang


Best Regards,
Wenyou Yang

Best Regards,
Wenyou Yang

Re: [PATCH 3/3] media: atmel-isc: Add more format configurations

[Yang, Wenyou]

Hi Hans,


On 2017/8/22 14:54, Hans Verkuil wrote:

On 08/17/2017 09:16 AM, Wenyou Yang wrote:

Add the configuration of formats: GREY, ARGB444, ARGB555 and ARGB32.

Signed-off-by: Wenyou Yang 
---

  drivers/media/platform/atmel/atmel-isc.c | 22 --
  1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index d91f4e5f8a8d..4e18fe1104c8 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -184,7 +184,7 @@ struct isc_device {
  #define RAW_FMT_IND_START0
  #define RAW_FMT_IND_END  11
  #define ISC_FMT_IND_START12
-#define ISC_FMT_IND_END  14
+#define ISC_FMT_IND_END  18

Shouldn't this be 19?
From ISC_FMT_IND_START to ISC_FMT_IND_END, it describes the formats 
they can be got from both the sensor (possibly) and the ISC controller.


The last format (19) is in not this category. It maybe be got from the 
sensor, can't be generated by the controller.

Regards,

Hans

  
  static struct isc_format isc_formats[] = {

/* 0 */
@@ -246,12 +246,30 @@ static struct isc_format isc_formats[] = {
{ V4L2_PIX_FMT_YUV422P, 0x0, 16,
  ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
  ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb },
+
/* 14 */
+   { V4L2_PIX_FMT_GREY, MEDIA_BUS_FMT_Y8_1X8, 8,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DATY8,
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x1fb },
+
+   /* 15 */
+   { V4L2_PIX_FMT_ARGB444, MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE, 16,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB444,
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
+   /* 16 */
+   { V4L2_PIX_FMT_ARGB555, MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE, 16,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB555,
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
+   /* 17 */
{ V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
  ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_RGB565,
  ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
+   /* 18 */
+   { V4L2_PIX_FMT_ARGB32, MEDIA_BUS_FMT_ARGB_1X32, 32,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB32,
+ ISC_DCFG_IMODE_PACKED32, ISC_DCTRL_DVIEW_PACKED, 0x7b },
  
-	/* 15 */

+   /* 19 */
{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8, 16,
  ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
  ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },


Best Regards,
Wenyou Yang

Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

[Yang, Wenyou]

Hi Hans,

On 2017/8/21 22:07, Hans Verkuil wrote:

On 08/17/2017 09:16 AM, Wenyou Yang wrote:

The 12-bit parallel interface supports the Raw Bayer, YCbCr,
Monochrome and JPEG Compressed pixel formats from the external
sensor, not support RBG pixel format.

Signed-off-by: Wenyou Yang 
---

  drivers/media/platform/atmel/atmel-isc.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index d4df3d4ccd85..535bb03783fe 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1478,6 +1478,11 @@ static int isc_formats_init(struct isc_device *isc)
while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
   NULL, _code)) {
mbus_code.index++;
+
+   /* Not support the RGB pixel formats from sensor */
+   if ((mbus_code.code & 0xf000) == 0x1000)
+   continue;

Am I missing something? Here you skip any RGB mediabus formats, but in patch 3/3
you add RGB mediabus formats. But this patch prevents those new formats from 
being
selected, right?

This patch prevents getting the RGB format from the sensor directly.
The RGB format can be produced by ISC controller by itself.


Regards,

Hans


+
fmt = find_format_by_code(mbus_code.code, );
if (!fmt)
continue;



Best Regards,
Wenyou Yang

Re: [PATCH 1/2] media: ov7670: Add entity pads initialization

[Yang, Wenyou]

Hi Sakari,


On 2017/8/16 0:23, Sakari Ailus wrote:

Hi Wenyou,

On Thu, Aug 10, 2017 at 05:06:44PM +0800, Wenyou Yang wrote:

Add the media entity pads initialization.

Signed-off-by: Wenyou Yang 

The patch itself seems fine. However the driver is lacking support for
get_fmt which I think would be necessary for the user space API to work
properly. Without sub-device nodes it might not have been an issue with
certain master drivers.

Could you add that in v2, in a separate patch before this one, please?

Okay, I will do.
Thank you for your review.




---

  drivers/media/i2c/ov7670.c | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index e88549f0e704..5c8460ee65c3 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -213,6 +213,7 @@ struct ov7670_devtype {
  struct ov7670_format_struct;  /* coming later */
  struct ov7670_info {
struct v4l2_subdev sd;
+   struct media_pad pad;
struct v4l2_ctrl_handler hdl;
struct {
/* gain cluster */
@@ -1688,14 +1689,23 @@ static int ov7670_probe(struct i2c_client *client,
v4l2_ctrl_auto_cluster(2, >auto_exposure,
   V4L2_EXPOSURE_MANUAL, false);
v4l2_ctrl_cluster(2, >saturation);
+
+   info->pad.flags = MEDIA_PAD_FL_SOURCE;
+   info->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+   ret = media_entity_pads_init(>sd.entity, 1, >pad);
+   if (ret < 0)
+   goto hdl_free;
+
v4l2_ctrl_handler_setup(>hdl);
  
  	ret = v4l2_async_register_subdev(>sd);

if (ret < 0)
-   goto hdl_free;
+   goto entity_cleanup;
  
  	return 0;
  
+entity_cleanup:

+   media_entity_cleanup(>sd.entity);
  hdl_free:
v4l2_ctrl_handler_free(>hdl);
  clk_disable:
@@ -1712,6 +1722,7 @@ static int ov7670_remove(struct i2c_client *client)
v4l2_device_unregister_subdev(sd);
v4l2_ctrl_handler_free(>hdl);
clk_disable_unprepare(info->clk);
+   media_entity_cleanup(>sd.entity);
return 0;
  }
  
--

2.13.0


Best Regards,
Wenyou Yang