[PATCH v1 0/5] [media] soc_camera: ov9640 switch to v4l2_async

[PATCH v1 0/5] [media] soc_camera: ov9640 switch to v4l2_async

[petrcvekcz]

From: Petr Cvek <petrcvekcz@gmail.com>

This patch series transfer the ov9640 driver from the soc_camera subsystem
into a standalone v4l2 driver. There is no changes except the required
v4l2_async calls, GPIO allocation and a deleting of now unused variables.

The config symbol has been changed from CONFIG_SOC_CAMERA_OV9640 to
VIDEO_OV9640.

Also as the drivers of the soc_camera seems to be orphaned I'm volunteering
to maintain the driver (I own the hardware).

I've found the ov9640 seems to be used at least in magician and Palm Zire72.
These need to define power and reset GPIOs and remove the soc_camera
definitions. I'm debugging it on magician now (the camera was unusable
on them since the pxa_camera switched from the soc_camera).

Petr Cvek (5):
  [media] soc_camera: ov9640: move ov9640 out of soc_camera
  [media] i2c: soc_camera: remove ov9640 Kconfig and Makefile options
  [media] i2c: add ov9640 config option as a standalone v4l2 sensor
  [media] i2c: drop soc_camera code from ov9640 and switch to v4l2_async
  MAINTAINERS: Add Petr Cvek as a maintainer for the ov9640 driver

 MAINTAINERS                                 |  6 ++
 drivers/media/i2c/Kconfig                   |  7 ++
 drivers/media/i2c/Makefile                  |  1 +
 drivers/media/i2c/{soc_camera => }/ov9640.c | 76 ++++++++++++++-------
 drivers/media/i2c/{soc_camera => }/ov9640.h |  2 +
 drivers/media/i2c/soc_camera/Kconfig        |  6 --
 drivers/media/i2c/soc_camera/Makefile       |  1 -
 7 files changed, 69 insertions(+), 30 deletions(-)
 rename drivers/media/i2c/{soc_camera => }/ov9640.c (93%)
 rename drivers/media/i2c/{soc_camera => }/ov9640.h (98%)

--
2.18.0

[PATCH v1 1/5] [media] soc_camera: ov9640: move ov9640 out of soc_camera

[petrcvekcz]

From: Petr Cvek <petrcvekcz@gmail.com>

Initial part of ov9640 transition from soc_camera subsystem to a standalone
v4l2 subdevice.

Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
---
 drivers/media/i2c/{soc_camera => }/ov9640.c | 0
 drivers/media/i2c/{soc_camera => }/ov9640.h | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename drivers/media/i2c/{soc_camera => }/ov9640.c (100%)
 rename drivers/media/i2c/{soc_camera => }/ov9640.h (100%)

diff --git a/drivers/media/i2c/soc_camera/ov9640.c b/drivers/media/i2c/ov9640.c
similarity index 100%
rename from drivers/media/i2c/soc_camera/ov9640.c
rename to drivers/media/i2c/ov9640.c
diff --git a/drivers/media/i2c/soc_camera/ov9640.h b/drivers/media/i2c/ov9640.h
similarity index 100%
rename from drivers/media/i2c/soc_camera/ov9640.h
rename to drivers/media/i2c/ov9640.h
-- 
2.18.0

[PATCH v1 2/5] [media] i2c: soc_camera: remove ov9640 Kconfig and Makefile options

[petrcvekcz]

From: Petr Cvek <petrcvekcz@gmail.com>

Remove ov9640 config options from soc_camera build files

Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
---
 drivers/media/i2c/soc_camera/Kconfig  | 6 ------
 drivers/media/i2c/soc_camera/Makefile | 1 -
 2 files changed, 7 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/Kconfig b/drivers/media/i2c/soc_camera/Kconfig
index 7c2aabc8a3f6..f67499187bda 100644
--- a/drivers/media/i2c/soc_camera/Kconfig
+++ b/drivers/media/i2c/soc_camera/Kconfig
@@ -41,12 +41,6 @@ config SOC_CAMERA_OV772X
 	help
 	  This is a ov772x camera driver
 
-config SOC_CAMERA_OV9640
-	tristate "ov9640 camera support"
-	depends on SOC_CAMERA && I2C
-	help
-	  This is a ov9640 camera driver
-
 config SOC_CAMERA_OV9740
 	tristate "ov9740 camera support"
 	depends on SOC_CAMERA && I2C
diff --git a/drivers/media/i2c/soc_camera/Makefile b/drivers/media/i2c/soc_camera/Makefile
index 8c7770f62997..758810abc480 100644
--- a/drivers/media/i2c/soc_camera/Makefile
+++ b/drivers/media/i2c/soc_camera/Makefile
@@ -4,7 +4,6 @@ obj-$(CONFIG_SOC_CAMERA_MT9T112)	+= mt9t112.o
 obj-$(CONFIG_SOC_CAMERA_MT9V022)	+= mt9v022.o
 obj-$(CONFIG_SOC_CAMERA_OV5642)		+= ov5642.o
 obj-$(CONFIG_SOC_CAMERA_OV772X)		+= ov772x.o
-obj-$(CONFIG_SOC_CAMERA_OV9640)		+= ov9640.o
 obj-$(CONFIG_SOC_CAMERA_OV9740)		+= ov9740.o
 obj-$(CONFIG_SOC_CAMERA_RJ54N1)		+= rj54n1cb0c.o
 obj-$(CONFIG_SOC_CAMERA_TW9910)		+= tw9910.o
-- 
2.18.0

[PATCH v1 3/5] [media] i2c: add ov9640 config option as a standalone v4l2 sensor

[petrcvekcz]

From: Petr Cvek <petrcvekcz@gmail.com>

Add ov9640 config option VIDEO_OV9640 to the build files in media/i2c
directory.

Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
---
 drivers/media/i2c/Kconfig  | 7 +++++++
 drivers/media/i2c/Makefile | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 439f6be08b95..c948b163a567 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -771,6 +771,13 @@ config VIDEO_OV7740
 	  This is a Video4Linux2 sensor driver for the OmniVision
 	  OV7740 VGA camera sensor.
 
+config VIDEO_OV9640
+	tristate "OmniVision OV9640 sensor support"
+	depends on I2C && VIDEO_V4L2
+	help
+	  This is a Video4Linux2 sensor driver for the OmniVision
+	  OV9640 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 837c428339df..9cc951f9c041 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
 obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
 obj-$(CONFIG_VIDEO_OV772X) += ov772x.o
 obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
+obj-$(CONFIG_VIDEO_OV9640) += ov9640.o
 obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
 obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
 obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
-- 
2.18.0

[PATCH v1 4/5] [media] i2c: drop soc_camera code from ov9640 and switch to v4l2_async

[petrcvekcz]

From: Petr Cvek <petrcvekcz@gmail.com>

This patch removes the dependency on an obsoleted soc_camera from ov9640
driver and changes the code to be a standalone v4l2 async subdevice.
It also adds GPIO allocations for power and reset signals (as they are not
handled by soc_camera now).

The patch should make ov9640 again compatible with the pxa_camera driver.

Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
---
 drivers/media/i2c/ov9640.c | 76 ++++++++++++++++++++++++++------------
 drivers/media/i2c/ov9640.h |  2 +
 2 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
index c63948989688..44129c60c524 100644
--- a/drivers/media/i2c/ov9640.c
+++ b/drivers/media/i2c/ov9640.c
@@ -9,6 +9,7 @@
  * Kuninori Morimoto <morimoto.kuninori@renesas.com>
  *
  * Based on ov7670 and soc_camera_platform driver,
+ * transition from soc_camera to pxa_camera based on mt9m111
  *
  * Copyright 2006-7 Jonathan Corbet <corbet@lwn.net>
  * Copyright (C) 2008 Magnus Damm
@@ -27,10 +28,14 @@
 #include <linux/v4l2-mediabus.h>
 #include <linux/videodev2.h>
 
-#include <media/soc_camera.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+
+#include <linux/gpio/consumer.h>
 
 #include "ov9640.h"
 
@@ -323,11 +328,23 @@ static int ov9640_set_register(struct v4l2_subdev *sd,
 
 static int ov9640_s_power(struct v4l2_subdev *sd, int on)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
 	struct ov9640_priv *priv = to_ov9640_sensor(sd);
-
-	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
+	int ret = 0;
+
+	if (on) {
+		gpiod_set_value(priv->gpio_power, 1);
+		mdelay(1);
+		ret = v4l2_clk_enable(priv->clk);
+		mdelay(1);
+		gpiod_set_value(priv->gpio_reset, 0);
+	} else {
+		gpiod_set_value(priv->gpio_reset, 1);
+		mdelay(1);
+		v4l2_clk_disable(priv->clk);
+		mdelay(1);
+		gpiod_set_value(priv->gpio_power, 0);
+	}
+	return ret;
 }
 
 /* select nearest higher resolution for capture */
@@ -631,14 +648,10 @@ static const struct v4l2_subdev_core_ops ov9640_core_ops = {
 static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
-
 	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
 		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
 		V4L2_MBUS_DATA_ACTIVE_HIGH;
 	cfg->type = V4L2_MBUS_PARALLEL;
-	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
 
 	return 0;
 }
@@ -667,18 +680,27 @@ static int ov9640_probe(struct i2c_client *client,
 			const struct i2c_device_id *did)
 {
 	struct ov9640_priv *priv;
-	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
 	int ret;
 
-	if (!ssdd) {
-		dev_err(&client->dev, "Missing platform_data for driver\n");
-		return -EINVAL;
-	}
-
-	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&client->dev, sizeof(*priv),
+			    GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	priv->gpio_power = devm_gpiod_get(&client->dev, "Camera power",
+					  GPIOD_OUT_LOW);
+	if (IS_ERR_OR_NULL(priv->gpio_power)) {
+		ret = PTR_ERR(priv->gpio_power);
+		return ret;
+	}
+
+	priv->gpio_reset = devm_gpiod_get(&client->dev, "Camera reset",
+					  GPIOD_OUT_HIGH);
+	if (IS_ERR_OR_NULL(priv->gpio_reset)) {
+		ret = PTR_ERR(priv->gpio_reset);
+		return ret;
+	}
+
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
 
 	v4l2_ctrl_handler_init(&priv->hdl, 2);
@@ -692,17 +714,25 @@ static int ov9640_probe(struct i2c_client *client,
 
 	priv->clk = v4l2_clk_get(&client->dev, "mclk");
 	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);
+		ret = -EPROBE_DEFER;
 		goto eclkget;
 	}
 
 	ret = ov9640_video_probe(client);
-	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
-	}
+	if (ret)
+		goto eprobe;
 
+	priv->subdev.dev = &client->dev;
+	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret)
+		goto eprobe;
+
+	return 0;
+
+eprobe:
+	v4l2_clk_put(priv->clk);
+eclkget:
+	v4l2_ctrl_handler_free(&priv->hdl);
 	return ret;
 }
 
@@ -712,7 +742,7 @@ static int ov9640_remove(struct i2c_client *client)
 	struct ov9640_priv *priv = to_ov9640_sensor(sd);
 
 	v4l2_clk_put(priv->clk);
-	v4l2_device_unregister_subdev(&priv->subdev);
+	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
 	return 0;
 }
diff --git a/drivers/media/i2c/ov9640.h b/drivers/media/i2c/ov9640.h
index 65d13ff17536..be5e4b29ac69 100644
--- a/drivers/media/i2c/ov9640.h
+++ b/drivers/media/i2c/ov9640.h
@@ -200,6 +200,8 @@ struct ov9640_priv {
 	struct v4l2_subdev		subdev;
 	struct v4l2_ctrl_handler	hdl;
 	struct v4l2_clk			*clk;
+	struct gpio_desc		*gpio_power;
+	struct gpio_desc		*gpio_reset;
 
 	int				model;
 	int				revision;
-- 
2.18.0

[PATCH v1 5/5] MAINTAINERS: Add Petr Cvek as a maintainer for the ov9640 driver

[petrcvekcz]

From: Petr Cvek <petrcvekcz@gmail.com>

The soc_camera drivers are marked as orphaned. Add Petr Cvek as a new
maintainer for ov9640 driver after its switch from the soc_camera.

Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 40d5ec9292ca..cab3fa4ccb37 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10627,6 +10627,12 @@ S:	Maintained
 F:	drivers/media/i2c/ov7740.c
 F:	Documentation/devicetree/bindings/media/i2c/ov7740.txt
 
+OMNIVISION OV9640 SENSOR DRIVER
+M:	Petr Cvek <petrcvekcz@gmail.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	drivers/media/i2c/ov9640.*
+
 OMNIVISION OV9650 SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 R:	Akinobu Mita <akinobu.mita@gmail.com>
-- 
2.18.0

Re: [PATCH v1 1/5] [media] soc_camera: ov9640: move ov9640 out of soc_camera

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

Hi Petr,
   thanks for the patches,

On Thu, Aug 09, 2018 at 03:39:45AM +0200, petrcvekcz@gmail.com wrote:
> From: Petr Cvek <petrcvekcz@gmail.com>
>
> Initial part of ov9640 transition from soc_camera subsystem to a standalone
> v4l2 subdevice.
>
> Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
> ---
>  drivers/media/i2c/{soc_camera => }/ov9640.c | 0
>  drivers/media/i2c/{soc_camera => }/ov9640.h | 0
>  2 files changed, 0 insertions(+), 0 deletions(-)
>  rename drivers/media/i2c/{soc_camera => }/ov9640.c (100%)
>  rename drivers/media/i2c/{soc_camera => }/ov9640.h (100%)
>
> diff --git a/drivers/media/i2c/soc_camera/ov9640.c b/drivers/media/i2c/ov9640.c
> similarity index 100%
> rename from drivers/media/i2c/soc_camera/ov9640.c
> rename to drivers/media/i2c/ov9640.c
> diff --git a/drivers/media/i2c/soc_camera/ov9640.h b/drivers/media/i2c/ov9640.h
> similarity index 100%
> rename from drivers/media/i2c/soc_camera/ov9640.h
> rename to drivers/media/i2c/ov9640.h

When I've been recently doing the same for ov772x and other sensor
driver I've been suggested to first copy the driver into
drivers/media/i2c/ and leave the original soc_camera one there, so
they can be bulk removed or moved to staging. I'll let Hans confirm
this, as he's about to take care of this process.

Thanks
   j

> --
> 2.18.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 4/5] [media] i2c: drop soc_camera code from ov9640 and switch to v4l2_async

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 6457 bytes --]

Hi Petr,

On Thu, Aug 09, 2018 at 03:39:48AM +0200, petrcvekcz@gmail.com wrote:
> From: Petr Cvek <petrcvekcz@gmail.com>
>
> This patch removes the dependency on an obsoleted soc_camera from ov9640
> driver and changes the code to be a standalone v4l2 async subdevice.
> It also adds GPIO allocations for power and reset signals (as they are not
> handled by soc_camera now).
>
> The patch should make ov9640 again compatible with the pxa_camera driver.

Are there board files using this driverin mainline ? (git grep says so)
Care to port them to use the new driver if necessary? You can have a
look at the SH4 Migo-R board, which recently underwent the same
process (arch/sh/boards/mach-migor/setup.c)

I also suggest to adjust the build system in a single patch with this
changes, but that's not a big deal...

>
> Signed-off-by: Petr Cvek <petrcvekcz@gmail.com>
> ---
>  drivers/media/i2c/ov9640.c | 76 ++++++++++++++++++++++++++------------
>  drivers/media/i2c/ov9640.h |  2 +
>  2 files changed, 55 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
> index c63948989688..44129c60c524 100644
> --- a/drivers/media/i2c/ov9640.c
> +++ b/drivers/media/i2c/ov9640.c
> @@ -9,6 +9,7 @@
>   * Kuninori Morimoto <morimoto.kuninori@renesas.com>
>   *
>   * Based on ov7670 and soc_camera_platform driver,
> + * transition from soc_camera to pxa_camera based on mt9m111
>   *
>   * Copyright 2006-7 Jonathan Corbet <corbet@lwn.net>
>   * Copyright (C) 2008 Magnus Damm

While at there, drop the license text and add SPDX identifier please

> @@ -27,10 +28,14 @@
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/videodev2.h>
>
> -#include <media/soc_camera.h>
> +#include <media/v4l2-async.h>
>  #include <media/v4l2-clk.h>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/gpio/consumer.h>
>
>  #include "ov9640.h"
>
> @@ -323,11 +328,23 @@ static int ov9640_set_register(struct v4l2_subdev *sd,
>
>  static int ov9640_s_power(struct v4l2_subdev *sd, int on)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>  	struct ov9640_priv *priv = to_ov9640_sensor(sd);
> -
> -	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
> +	int ret = 0;
> +
> +	if (on) {
> +		gpiod_set_value(priv->gpio_power, 1);
> +		mdelay(1);

mdelay() is backed by a busy-waiting loop, according to
timers-howto.txt and for milliseconds-long sleeps is not suggested.
Please try to quantify the required delay and use msleep_range().

> +		ret = v4l2_clk_enable(priv->clk);

Is this required by the pxa camera driver using v4l2_clk_ APIs?
Otherwise you should use the clk API directly.

> +		mdelay(1);
> +		gpiod_set_value(priv->gpio_reset, 0);
> +	} else {
> +		gpiod_set_value(priv->gpio_reset, 1);
> +		mdelay(1);
> +		v4l2_clk_disable(priv->clk);
> +		mdelay(1);
> +		gpiod_set_value(priv->gpio_power, 0);
> +	}
> +	return ret;
>  }
>
>  /* select nearest higher resolution for capture */
> @@ -631,14 +648,10 @@ static const struct v4l2_subdev_core_ops ov9640_core_ops = {
>  static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
>  				struct v4l2_mbus_config *cfg)

g_mbus/s_mbus are deprecated. Unless the pxa camera driver wants them
all format negotiation should go through s_fmt/g_fmt pad operations

>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> -
>  	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>  		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>  		V4L2_MBUS_DATA_ACTIVE_HIGH;
>  	cfg->type = V4L2_MBUS_PARALLEL;
> -	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>
>  	return 0;
>  }
> @@ -667,18 +680,27 @@ static int ov9640_probe(struct i2c_client *client,
>  			const struct i2c_device_id *did)
>  {
>  	struct ov9640_priv *priv;
> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>  	int ret;
>
> -	if (!ssdd) {
> -		dev_err(&client->dev, "Missing platform_data for driver\n");
> -		return -EINVAL;
> -	}
> -
> -	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&client->dev, sizeof(*priv),
> +			    GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>
> +	priv->gpio_power = devm_gpiod_get(&client->dev, "Camera power",
> +					  GPIOD_OUT_LOW);
> +	if (IS_ERR_OR_NULL(priv->gpio_power)) {
> +		ret = PTR_ERR(priv->gpio_power);
> +		return ret;
> +	}
> +
> +	priv->gpio_reset = devm_gpiod_get(&client->dev, "Camera reset",
> +					  GPIOD_OUT_HIGH);
> +	if (IS_ERR_OR_NULL(priv->gpio_reset)) {
> +		ret = PTR_ERR(priv->gpio_reset);
> +		return ret;
> +	}
> +
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
>
>  	v4l2_ctrl_handler_init(&priv->hdl, 2);
> @@ -692,17 +714,25 @@ static int ov9640_probe(struct i2c_client *client,
>
>  	priv->clk = v4l2_clk_get(&client->dev, "mclk");
>  	if (IS_ERR(priv->clk)) {
> -		ret = PTR_ERR(priv->clk);
> +		ret = -EPROBE_DEFER;

Why are you forcing EPROBE_DEFER instead of returning the clk_get()
return value?

Thanks
   j

>  		goto eclkget;
>  	}
>
>  	ret = ov9640_video_probe(client);
> -	if (ret) {
> -		v4l2_clk_put(priv->clk);
> -eclkget:
> -		v4l2_ctrl_handler_free(&priv->hdl);
> -	}
> +	if (ret)
> +		goto eprobe;
>
> +	priv->subdev.dev = &client->dev;
> +	ret = v4l2_async_register_subdev(&priv->subdev);
> +	if (ret)
> +		goto eprobe;
> +
> +	return 0;
> +
> +eprobe:
> +	v4l2_clk_put(priv->clk);
> +eclkget:
> +	v4l2_ctrl_handler_free(&priv->hdl);
>  	return ret;
>  }
>
> @@ -712,7 +742,7 @@ static int ov9640_remove(struct i2c_client *client)
>  	struct ov9640_priv *priv = to_ov9640_sensor(sd);
>
>  	v4l2_clk_put(priv->clk);
> -	v4l2_device_unregister_subdev(&priv->subdev);
> +	v4l2_async_unregister_subdev(&priv->subdev);
>  	v4l2_ctrl_handler_free(&priv->hdl);
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/ov9640.h b/drivers/media/i2c/ov9640.h
> index 65d13ff17536..be5e4b29ac69 100644
> --- a/drivers/media/i2c/ov9640.h
> +++ b/drivers/media/i2c/ov9640.h
> @@ -200,6 +200,8 @@ struct ov9640_priv {
>  	struct v4l2_subdev		subdev;
>  	struct v4l2_ctrl_handler	hdl;
>  	struct v4l2_clk			*clk;
> +	struct gpio_desc		*gpio_power;
> +	struct gpio_desc		*gpio_reset;
>
>  	int				model;
>  	int				revision;
> --
> 2.18.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 4/5] [media] i2c: drop soc_camera code from ov9640 and switch to v4l2_async

[Petr Cvek]

Dne 10.8.2018 v 09:51 jacopo mondi napsal(a):
> Hi Petr,
> 
> On Thu, Aug 09, 2018 at 03:39:48AM +0200, petrcvekcz@gmail.com wrote:
>> From: Petr Cvek <petrcvekcz@gmail.com>
>>
>> This patch removes the dependency on an obsoleted soc_camera from ov9640
>> driver and changes the code to be a standalone v4l2 async subdevice.
>> It also adds GPIO allocations for power and reset signals (as they are not
>> handled by soc_camera now).
>>
>> The patch should make ov9640 again compatible with the pxa_camera driver.
> 
> Are there board files using this driverin mainline ? (git grep says so)
> Care to port them to use the new driver if necessary? You can have a
> look at the SH4 Migo-R board, which recently underwent the same
> process (arch/sh/boards/mach-migor/setup.c)
> 

Yes there are Magician and Palm Zire72 which are directly using ov9640
(and few others which are using pxa_camera with a different sensor). I'm
working on HTC magician (pxa_camera is not a soc_camera subdev anymore,
ov9640 still is).

> I also suggest to adjust the build system in a single patch with this
> changes, but that's not a big deal...
> 

OK (at the end of the patchset I suppose?)

> 
>> +		ret = v4l2_clk_enable(priv->clk);
> 
> Is this required by the pxa camera driver using v4l2_clk_ APIs?
> Otherwise you should use the clk API directly.
> 

Yes the clock is registered by pxa camera with v4l2_clk_register(). I
will probably get to that in the future, but there is stuff (bugs, dead
code from soc_camera removal, ...) with more priority in the driver for now.


>> +		mdelay(1);
>> +		gpiod_set_value(priv->gpio_reset, 0);
>> +	} else {
>> +		gpiod_set_value(priv->gpio_reset, 1);
>> +		mdelay(1);
>> +		v4l2_clk_disable(priv->clk);
>> +		mdelay(1);
>> +		gpiod_set_value(priv->gpio_power, 0);
>> +	}
>> +	return ret;
>>  }
>>
>>  /* select nearest higher resolution for capture */
>> @@ -631,14 +648,10 @@ static const struct v4l2_subdev_core_ops ov9640_core_ops = {
>>  static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
>>  				struct v4l2_mbus_config *cfg)
> 
> g_mbus/s_mbus are deprecated. Unless the pxa camera driver wants them
> all format negotiation should go through s_fmt/g_fmt pad operations
> 

Yeah it does:

  ret = sensor_call(pcdev, video, g_mbus_config, &cfg);


>>  {
>> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> -
>>  	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>>  		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>>  		V4L2_MBUS_DATA_ACTIVE_HIGH;
>>  	cfg->type = V4L2_MBUS_PARALLEL;
>> -	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>>
>>  	return 0;
>>  }
>> @@ -667,18 +680,27 @@ static int ov9640_probe(struct i2c_client *client,
>>  			const struct i2c_device_id *did)
>>  {
>>  	struct ov9640_priv *priv;
>> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>  	int ret;
>>
>> -	if (!ssdd) {
>> -		dev_err(&client->dev, "Missing platform_data for driver\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> +	priv = devm_kzalloc(&client->dev, sizeof(*priv),
>> +			    GFP_KERNEL);
>>  	if (!priv)
>>  		return -ENOMEM;
>>
>> +	priv->gpio_power = devm_gpiod_get(&client->dev, "Camera power",
>> +					  GPIOD_OUT_LOW);
>> +	if (IS_ERR_OR_NULL(priv->gpio_power)) {
>> +		ret = PTR_ERR(priv->gpio_power);
>> +		return ret;
>> +	}
>> +
>> +	priv->gpio_reset = devm_gpiod_get(&client->dev, "Camera reset",
>> +					  GPIOD_OUT_HIGH);
>> +	if (IS_ERR_OR_NULL(priv->gpio_reset)) {
>> +		ret = PTR_ERR(priv->gpio_reset);
>> +		return ret;
>> +	}
>> +
>>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
>>
>>  	v4l2_ctrl_handler_init(&priv->hdl, 2);
>> @@ -692,17 +714,25 @@ static int ov9640_probe(struct i2c_client *client,
>>
>>  	priv->clk = v4l2_clk_get(&client->dev, "mclk");
>>  	if (IS_ERR(priv->clk)) {
>> -		ret = PTR_ERR(priv->clk);
>> +		ret = -EPROBE_DEFER;
> 
> Why are you forcing EPROBE_DEFER instead of returning the clk_get()
> return value?
> 

That may be residue from testing, I will fix that.

Petr

Re: [PATCH v1 4/5] [media] i2c: drop soc_camera code from ov9640 and switch to v4l2_async

[jacopo mondi]

Hi Petr,

On Sun, Aug 12, 2018 at 03:13:39PM +0200, Petr Cvek wrote:
> Dne 10.8.2018 v 09:51 jacopo mondi napsal(a):
> > Hi Petr,
> > 
> > On Thu, Aug 09, 2018 at 03:39:48AM +0200, petrcvekcz@gmail.com wrote:
> >> From: Petr Cvek <petrcvekcz@gmail.com>
> >>
> >> This patch removes the dependency on an obsoleted soc_camera from ov9640
> >> driver and changes the code to be a standalone v4l2 async subdevice.
> >> It also adds GPIO allocations for power and reset signals (as they are not
> >> handled by soc_camera now).
> >>
> >> The patch should make ov9640 again compatible with the pxa_camera driver.
> > 
> > Are there board files using this driverin mainline ? (git grep says so)
> > Care to port them to use the new driver if necessary? You can have a
> > look at the SH4 Migo-R board, which recently underwent the same
> > process (arch/sh/boards/mach-migor/setup.c)
> > 
> 
> Yes there are Magician and Palm Zire72 which are directly using ov9640
> (and few others which are using pxa_camera with a different sensor). I'm
> working on HTC magician (pxa_camera is not a soc_camera subdev anymore,
> ov9640 still is).
> 
> > I also suggest to adjust the build system in a single patch with this
> > changes, but that's not a big deal...
> > 
> 
> OK (at the end of the patchset I suppose?)
> 

When I did the same soc_camera removal process on other drivers, I
adjusted the build system in the same patch that removed soc_camera
dependencies in the driver.

The process looked like:
01: copy the driver from drivers/media/i2c/soc_camera to
drivers/media/i2c/
02: Remove soc_camera dependencies from the driver and adjust
drivers/media/i2c/Kconfg drivers/media/i2c/Makefile accordingly
03: Port existing users of the soc_camera driver to use the new one

I guess patch ordering is not a big deal though ;)

Thanks
  j

> > 
> >> +		ret = v4l2_clk_enable(priv->clk);
> > 
> > Is this required by the pxa camera driver using v4l2_clk_ APIs?
> > Otherwise you should use the clk API directly.
> > 
> 
> Yes the clock is registered by pxa camera with v4l2_clk_register(). I
> will probably get to that in the future, but there is stuff (bugs, dead
> code from soc_camera removal, ...) with more priority in the driver for now.
> 
> 
> >> +		mdelay(1);
> >> +		gpiod_set_value(priv->gpio_reset, 0);
> >> +	} else {
> >> +		gpiod_set_value(priv->gpio_reset, 1);
> >> +		mdelay(1);
> >> +		v4l2_clk_disable(priv->clk);
> >> +		mdelay(1);
> >> +		gpiod_set_value(priv->gpio_power, 0);
> >> +	}
> >> +	return ret;
> >>  }
> >>
> >>  /* select nearest higher resolution for capture */
> >> @@ -631,14 +648,10 @@ static const struct v4l2_subdev_core_ops ov9640_core_ops = {
> >>  static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
> >>  				struct v4l2_mbus_config *cfg)
> > 
> > g_mbus/s_mbus are deprecated. Unless the pxa camera driver wants them
> > all format negotiation should go through s_fmt/g_fmt pad operations
> > 
> 
> Yeah it does:
> 
>   ret = sensor_call(pcdev, video, g_mbus_config, &cfg);
> 
> 
> >>  {
> >> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> >> -
> >>  	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
> >>  		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> >>  		V4L2_MBUS_DATA_ACTIVE_HIGH;
> >>  	cfg->type = V4L2_MBUS_PARALLEL;
> >> -	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
> >>
> >>  	return 0;
> >>  }
> >> @@ -667,18 +680,27 @@ static int ov9640_probe(struct i2c_client *client,
> >>  			const struct i2c_device_id *did)
> >>  {
> >>  	struct ov9640_priv *priv;
> >> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> >>  	int ret;
> >>
> >> -	if (!ssdd) {
> >> -		dev_err(&client->dev, "Missing platform_data for driver\n");
> >> -		return -EINVAL;
> >> -	}
> >> -
> >> -	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >> +	priv = devm_kzalloc(&client->dev, sizeof(*priv),
> >> +			    GFP_KERNEL);
> >>  	if (!priv)
> >>  		return -ENOMEM;
> >>
> >> +	priv->gpio_power = devm_gpiod_get(&client->dev, "Camera power",
> >> +					  GPIOD_OUT_LOW);
> >> +	if (IS_ERR_OR_NULL(priv->gpio_power)) {
> >> +		ret = PTR_ERR(priv->gpio_power);
> >> +		return ret;
> >> +	}
> >> +
> >> +	priv->gpio_reset = devm_gpiod_get(&client->dev, "Camera reset",
> >> +					  GPIOD_OUT_HIGH);
> >> +	if (IS_ERR_OR_NULL(priv->gpio_reset)) {
> >> +		ret = PTR_ERR(priv->gpio_reset);
> >> +		return ret;
> >> +	}
> >> +
> >>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
> >>
> >>  	v4l2_ctrl_handler_init(&priv->hdl, 2);
> >> @@ -692,17 +714,25 @@ static int ov9640_probe(struct i2c_client *client,
> >>
> >>  	priv->clk = v4l2_clk_get(&client->dev, "mclk");
> >>  	if (IS_ERR(priv->clk)) {
> >> -		ret = PTR_ERR(priv->clk);
> >> +		ret = -EPROBE_DEFER;
> > 
> > Why are you forcing EPROBE_DEFER instead of returning the clk_get()
> > return value?
> > 
> 
> That may be residue from testing, I will fix that.
> 
> Petr