soc_camera: ov9640 switch to v4l2_async

--
2.18.0

From: Petr Cvek <[email protected]>

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 <[email protected]>

---
 drivers/media/i2c/ov9640.c | 76 ++++++++++++++++++++++++++------------
 drivers/media/i2c/ov9640.h |  2 +
 2 files changed, 55 insertions(+), 23 deletions(-)

drivers/media/i2c/ov9640.c +0 -0

index c63948989688..44129c60c524 100644

drivers/media/i2c/ov9640.c +5 -1

@@ -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)
 {

drivers/media/i2c/ov9640.c +10 -0

+		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 */

drivers/media/i2c/ov9640.c +16 -11

 {
-	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);

drivers/media/i2c/ov9640.c +14 -6

 		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;
 }

drivers/media/i2c/ov9640.h +2 -1

index 65d13ff17536..be5e4b29ac69 100644
@@ -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

Hi Petr,

Quoted 7 lines · On Thu, Aug 09, 2018 at 03:39:48AM +0200, [email protected] wrote:

From: Petr Cvek <[email protected]>

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).

Quoted 1 line

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...

drivers/media/i2c/ov9640.c static int ov9640_set_register(struct v4l2_subdev *sd, · line 337

Quoted 2 lines

drivers/media/i2c/ov9640.c +1 -0

+		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.

Quoted 12 lines

drivers/media/i2c/ov9640.c +10 -0

+		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;
 }

drivers/media/i2c/ov9640.c static const struct v4l2_subdev_core_ops ov9640_core_ops = { · line 649

Quoted 4 lines

/* select nearest higher resolution for capture */

drivers/media/i2c/ov9640.c +0 -0

@@ -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

Quoted 49 lines

drivers/media/i2c/ov9640.c +16 -11

 {
-	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,
 

drivers/media/i2c/ov9640.c static int ov9640_probe(struct i2c_client *client, · line 717

Quoted 4 lines

drivers/media/i2c/ov9640.c +1 -1

 	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?

Dne 10.8.2018 v 09:51 jacopo mondi napsal(a):

Quoted 10 lines

Hi Petr,

Quoted 7 lines · On Thu, Aug 09, 2018 at 03:39:48AM +0200, [email protected] wrote:

From: Petr Cvek <[email protected]>

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).

Quoted 7 lines

Quoted 1 line

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).

Quoted 3 lines

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?)

Quoted 6 lines

drivers/media/i2c/ov9640.c static int ov9640_set_register(struct v4l2_subdev *sd, · line 337

Quoted 2 lines

drivers/media/i2c/ov9640.c +1 -0

+		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.

Quoted 12 lines

Quoted 12 lines

drivers/media/i2c/ov9640.c +10 -0

+		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;
 }

Quoted 8 lines

drivers/media/i2c/ov9640.c static const struct v4l2_subdev_core_ops ov9640_core_ops = { · line 649

Quoted 4 lines

/* select nearest higher resolution for capture */

drivers/media/i2c/ov9640.c +0 -0

@@ -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);

Quoted 49 lines

Quoted 49 lines

drivers/media/i2c/ov9640.c +16 -11

 {
-	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,
 

Quoted 8 lines

drivers/media/i2c/ov9640.c static int ov9640_probe(struct i2c_client *client, · line 717

Quoted 4 lines

drivers/media/i2c/ov9640.c +1 -1

 	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