media: ov2640: add device tree support

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

 drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +9 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk. maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}
 
 	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
 }

drivers/media/i2c/soc_camera/ov2640.c +12 -4

 	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
 	} else {
 		dev_info(&adapter->dev, "OV2640 Probed\n");
 	}
 
+	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
 	return ret;
 }
 

@@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client *client)
 {
 	struct ov2640_priv       *priv = to_ov2640(client);
 
-	v4l2_clk_put(priv->clk);
+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
 	v4l2_device_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
 	return 0;

Signature · 1 line

1.9.1

Hi Josh,

Thanks for your patches!

Quoted 47 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

  drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++++++----------
  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +9 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk. maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}
 
  	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
  }

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client *client,
  	if (priv->hdl.error)
  		return priv->hdl.error;
 

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client *client, · line 1090

Quoted 7 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -6

-	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);
-		goto eclkget;
-	}
-
  	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

Or are there any other patched,
that change that, that I'm overseeing?

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for an example. Or did I misunderstand anything?

Thanks
Guennadi

Quoted 34 lines

drivers/media/i2c/soc_camera/ov2640.c +12 -4

  	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
  	} else {
  		dev_info(&adapter->dev, "OV2640 Probed\n");
  	}
 
+	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
  	return ret;
  }
 

@@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client *client)
  {
  	struct ov2640_priv       *priv = to_ov2640(client);
 
-	v4l2_clk_put(priv->clk);
+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
  	v4l2_device_unregister_subdev(&priv->subdev);
  	v4l2_ctrl_handler_free(&priv->hdl);
  	return 0;

Signature · 1 line

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi, Guennadi

Thanks for the review.

Quoted 45 lines · Guennadi Liakhovetski

Hi Josh,

Thanks for your patches!

Quoted 39 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

  drivers/media/i2c/soc_camera/ov2640.c | 31
+++++++++++++++++++++----------
  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +5 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))

Quoted 8 lines

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c +4 -0

+			dev_warn(&client->dev, "Cannot get the mclk. maybe
 soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}
    	return soc_camera_set_power(&client->dev, ssdd, priv->clk,

on);
}

Quoted 6 lines

Quoted 6 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -2

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client *client,
  	if (priv->hdl.error)
  		return priv->hdl.error;
  -	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client *client, · line 1090

Quoted 4 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -3

-		goto eclkget;
-	}
-
  	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

yes, you are right. In this patch, the "mclk" will requested by ov2640_s_power().

The reason why I put the getting "mclk" code from ov2640_probe() to ov2640_s_power() is : as the "mclk" here is camera host's peripheral clock. That means ov2640 still can be probed properly (read ov2640 id) even no "mclk". So when I move this code to ov2640_s_power(), otherwise the ov2640_probe() will be failed or DEFER_PROBE.

Is this true for all camera host? If it's not true, then I think use -EPROBE_DEFER would be a proper way.

Quoted 9 lines

Or are there any other patched,
that change that, that I'm overseeing?

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for an example. Or did I misunderstand anything?

Actually months ago I already done a version of ov2640 patch which use -EPROBE_DEFER way.

But now I think the ov2640 can be probed correctly without "mclk", so it is no need to return -EPROBE_DEFER.
And the v4l2 asyn API can handle the synchronization of host. So I prefer to use this way.
What do you think about this?

Best Regards,
Josh Wu

Quoted 40 lines

Thanks
Guennadi

Quoted 32 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -3

  	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
  	} else {
  		dev_info(&adapter->dev, "OV2640 Probed\n");
  	}
  +	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
  	return ret;
  }
  @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client
 *client)

  {
  	struct ov2640_priv       *priv = to_ov2640(client);
  -	v4l2_clk_put(priv->clk);

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
  	v4l2_device_unregister_subdev(&priv->subdev);
  	v4l2_ctrl_handler_free(&priv->hdl);
  	return 0;

Signature · 1 line

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 50 lines · Josh Wu

Hi, Guennadi

Thanks for the review.

Quoted 45 lines · Guennadi Liakhovetski

Hi Josh,

Thanks for your patches!

Quoted 39 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

   drivers/media/i2c/soc_camera/ov2640.c | 31
+++++++++++++++++++++----------
   1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +5 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c +4 -0

+			dev_warn(&client->dev, "Cannot get the mclk. maybe
 soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}
     	return soc_camera_set_power(&client->dev, ssdd, priv->clk,

on);
   }

Ok, think about this: you check whether priv->clk is set on each .s_power() call, which is already a bit awkward.

Such approach can be used
when there's no other way to perform a one-time action, but here we have one. But never mind, that's not the main problem. If priv->clk isn't set, you try to acquire it. But during probing, when this function is called for the first time clock isn't available yet, but you still want to succeed probing. So, you just issue a warning and continue. But then later an application opens the camera, .s_power() is called again, but for some reason the clock might still be not available, and this time you should fail.

But you don't, you succeed and then you'll fail somewhere later, presumably, with a timeout waiting for frames. Am I right?

Quoted 24 lines

Quoted 15 lines

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client *client, · line 1090

Quoted 11 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -5

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client *client,
   	if (priv->hdl.error)
   		return priv->hdl.error;
   -	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);
-		goto eclkget;
-	}
-
   	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

yes, you are right. In this patch, the "mclk" will requested by ov2640_s_power().

The reason why I put the getting "mclk" code from ov2640_probe() to ov2640_s_power() is : as the "mclk" here is camera host's peripheral clock. That means ov2640 still can be probed properly (read ov2640 id) even no "mclk". So when I move this code to ov2640_s_power(), otherwise the ov2640_probe() will be failed or DEFER_PROBE.

Quoted 2 lines

Is this true for all camera host? If it's not true, then I think use -EPROBE_DEFER would be a proper way.

Sorry, not sure what your question is.

And I'm not sure ov2640's registers
can be accessed with no running clock.

I think some camera sensors can do
this, but I have no idea about this one. How did you verify? Is it mentioned in a datasheet? Or did you really disconnected (grounded) the sensor clock input and tried to access its reqisters?

If you just
verified, that it's working without requesting the clock, are you sure your clock output isn't running permanently all the time anyway?

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Or are there any other patched,
that change that, that I'm overseeing?

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for an example. Or did I misunderstand anything?

Quoted 51 lines

Actually months ago I already done a version of ov2640 patch which use -EPROBE_DEFER way.

But now I think the ov2640 can be probed correctly without "mclk", so it is no need to return -EPROBE_DEFER.
And the v4l2 asyn API can handle the synchronization of host. So I prefer to use this way.
What do you think about this?

Best Regards,
Josh Wu

Quoted 39 lines

Thanks
Guennadi

Quoted 32 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -3

   	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
   	} else {
   		dev_info(&adapter->dev, "OV2640 Probed\n");
   	}
   +	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
   	return ret;
   }
   @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client
 *client)

   {
   	struct ov2640_priv       *priv = to_ov2640(client);
   -	v4l2_clk_put(priv->clk);

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
   	v4l2_device_unregister_subdev(&priv->subdev);
   	v4l2_ctrl_handler_free(&priv->hdl);
   	return 0;

Signature · 1 line

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi, Guennadi

Quoted 58 lines · Guennadi Liakhovetski

Quoted 56 lines · Josh Wu

Hi, Guennadi

Thanks for the review.

Quoted 51 lines · Guennadi Liakhovetski

Hi Josh,

Thanks for your patches!

Quoted 45 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk'
in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe()
function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to
soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

   drivers/media/i2c/soc_camera/ov2640.c | 31
+++++++++++++++++++++----------
   1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +0 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);

drivers/media/i2c/soc_camera/ov2640.c +6 -0

+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

maybe
soc-camera host is not probed yet.\n");

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+		else
+			priv->clk = clk;
+	}
     	return soc_camera_set_power(&client->dev, ssdd, priv->clk,

on);
   }

Just let me explained a little more details at first:

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c.
it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

That is the motivation I want ov2640 be probed even without "mclk".

Quoted 2 lines

Ok, think about this: you check whether priv->clk is set on each .s_power() call, which is already a bit awkward.

yes, it is.

Quoted 9 lines

Such approach can be used
when there's no other way to perform a one-time action, but here we have one. But never mind, that's not the main problem. If priv->clk isn't set, you try to acquire it. But during probing, when this function is called for the first time clock isn't available yet, but you still want to succeed probing. So, you just issue a warning and continue. But then later an application opens the camera, .s_power() is called again, but for some reason the clock might still be not available, and this time you should fail.

Quoted 2 lines

But you don't, you succeed and then you'll fail somewhere later, presumably, with a timeout waiting for frames. Am I right?

if the clock (v4l2 clock: mclk) is not available, then, there is no camera host available.

So the system should have no v4l2 device found.
I think in this case the application cannot call the camera sensor .s_power() via v4l2 ioctl.
So the timeout case should not happened.

Quoted 25 lines

Quoted 25 lines

Quoted 7 lines

Quoted 7 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -2

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client
 *client,
   	if (priv->hdl.error)
   		return priv->hdl.error;
   -	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client *client, · line 1090

Quoted 4 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -3

-		goto eclkget;
-	}
-
   	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

yes, you are right. In this patch, the "mclk" will requested by ov2640_s_power().

The reason why I put the getting "mclk" code from ov2640_probe() to ov2640_s_power() is : as the "mclk" here is camera host's peripheral clock.
That means ov2640 still can be probed properly (read ov2640 id) even no "mclk". So when I move this code to ov2640_s_power(), otherwise the ov2640_probe() will be failed or DEFER_PROBE.

Quoted 3 lines

Quoted 2 lines

Is this true for all camera host? If it's not true, then I think use -EPROBE_DEFER would be a proper way.

Sorry, not sure what your question is.

Sorry, I don't make me clear here.
My question should be: Are all the camera host's clock_start()/stop() only operate their peripheral clock?

Quoted 2 lines

And I'm not sure ov2640's registers
can be accessed with no running clock.

No, it seems there is a misunderstanding here.

I didn't mean ov2640 can be probed without xvclk.
What I try to say is the ov2640 can be probed without camera host's peripheral clock.

Quoted 8 lines

I think some camera sensors can do
this, but I have no idea about this one. How did you verify? Is it mentioned in a datasheet? Or did you really disconnected (grounded) the sensor clock input and tried to access its reqisters?

If you just
verified, that it's working without requesting the clock, are you sure your clock output isn't running permanently all the time anyway?

I didn't verify the those method as I only probed the ov2640 without ISI enabled. ISI peripheral clock is disabled and etc.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Or are there any other patched,
that change that, that I'm overseeing?

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for
an example. Or did I misunderstand anything?

I can implement with your method. like in probe() function, request the v4l2_clk "mclk", if failed then return -EPROBE_DEFER.

But I remember you mentioned that you will remove the v4l2 clock in future. See ff5430de commit message.
So I just want to not so depends on the v4l2_clk "mclk".

Best Regards,
Josh Wu

Quoted 54 lines

Quoted 54 lines

Actually months ago I already done a version of ov2640 patch which use -EPROBE_DEFER way.

But now I think the ov2640 can be probed correctly without "mclk", so it is no
need to return -EPROBE_DEFER.
And the v4l2 asyn API can handle the synchronization of host. So I prefer to
use this way.
What do you think about this?

Best Regards,
Josh Wu

Quoted 40 lines

Thanks
Guennadi

Quoted 32 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -3

   	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
   	} else {
   		dev_info(&adapter->dev, "OV2640 Probed\n");
   	}
   +	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
   	return ret;
   }
   @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client

*client)

   {
   	struct ov2640_priv       *priv = to_ov2640(client);
   -	v4l2_clk_put(priv->clk);

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
   	v4l2_device_unregister_subdev(&priv->subdev);
   	v4l2_ctrl_handler_free(&priv->hdl);
   	return 0;

Signature · 1 line

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Josh,

Quoted 71 lines · Josh Wu

Hi, Guennadi

Quoted 58 lines · Guennadi Liakhovetski

Quoted 56 lines · Josh Wu

Hi, Guennadi

Thanks for the review.

Quoted 51 lines · Guennadi Liakhovetski

Hi Josh,

Thanks for your patches!

Quoted 45 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk'
in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe()
function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to
soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:

     no changes.

    drivers/media/i2c/soc_camera/ov2640.c | 31
+++++++++++++++++++++----------
    1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +0 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);

drivers/media/i2c/soc_camera/ov2640.c +6 -0

+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

maybe
soc-camera host is not probed yet.\n");

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+		else
+			priv->clk = clk;
+	}
      	return soc_camera_set_power(&client->dev, ssdd, priv->clk,

on);
    }

Just let me explained a little more details at first:

Quoted 5 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c.
it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

Quoted 2 lines

That is the motivation I want ov2640 be probed even without "mclk".

ov2640 can be used with other boards and camera hosts, not only your specific board. In other configurations your change will break the driver.

Quoted 15 lines

Quoted 2 lines

Ok, think about this: you check whether priv->clk is set on each .s_power() call, which is already a bit awkward.

yes, it is.

Quoted 11 lines

Such approach can be used
when there's no other way to perform a one-time action, but here we have one. But never mind, that's not the main problem. If priv->clk isn't set, you try to acquire it. But during probing, when this function is called for the first time clock isn't available yet, but you still want to succeed probing. So, you just issue a warning and continue. But then later an application opens the camera, .s_power() is called again, but for some reason the clock might still be not available, and this time you should fail.
But you don't, you succeed and then you'll fail somewhere later, presumably, with a timeout waiting for frames. Am I right?

Quoted 2 lines

if the clock (v4l2 clock: mclk) is not available, then, there is no camera host available.

This isn't true - from the hardware perspective. The clock can be supplied by a different source.

Quoted 4 lines

So the system should have no v4l2 device found.
I think in this case the application cannot call the camera sensor .s_power() via v4l2 ioctl.
So the timeout case should not happened.

No, sorry, I meant a physical clock, not aclock object. You can register the complete framework and try to use it, but if the physical clock isn't enabled, the camera sensor won't function correctly.

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 7 lines

Quoted 7 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -2

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client
 *client,
    	if (priv->hdl.error)
    		return priv->hdl.error;
    -	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client *client, · line 1090

Quoted 4 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -3

-		goto eclkget;
-	}
-
    	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

yes, you are right. In this patch, the "mclk" will requested by ov2640_s_power().

The reason why I put the getting "mclk" code from ov2640_probe() to ov2640_s_power() is : as the "mclk" here is camera host's peripheral clock.
That means ov2640 still can be probed properly (read ov2640 id) even no "mclk". So when I move this code to ov2640_s_power(), otherwise the ov2640_probe() will be failed or DEFER_PROBE.

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

Is this true for all camera host? If it's not true, then I think use -EPROBE_DEFER would be a proper way.

Sorry, not sure what your question is.

Sorry, I don't make me clear here.
My question should be: Are all the camera host's clock_start()/stop() only operate their peripheral clock?

Yes, that's all camera hosts have. They cannot operate other unrelated clock sources.

Quoted 7 lines

Quoted 2 lines

And I'm not sure ov2640's registers
can be accessed with no running clock.

No, it seems there is a misunderstanding here.

I didn't mean ov2640 can be probed without xvclk.
What I try to say is the ov2640 can be probed without camera host's peripheral clock.

Ok, then your patch will break the driver even earlier - when trying to access ov2640 registers without enabling the clock.

Quoted 9 lines

Quoted 7 lines

I think some camera sensors can do
this, but I have no idea about this one. How did you verify? Is it mentioned in a datasheet? Or did you really disconnected (grounded) the sensor clock input and tried to access its reqisters?
If you just
verified, that it's working without requesting the clock, are you sure your clock output isn't running permanently all the time anyway?

I didn't verify the those method as I only probed the ov2640 without ISI enabled. ISI peripheral clock is disabled and etc.

Right, this means a separate (probably always-on) clock source is used on your specific board, but this doesn't have to be the case on all other boards, where ov2640 is used.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Or are there any other patched,
that change that, that I'm overseeing?

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for
an example. Or did I misunderstand anything?

I can implement with your method. like in probe() function, request the v4l2_clk "mclk", if failed then return -EPROBE_DEFER.

Yes, please, I think this would be a correct solution.

Quoted 4 lines

But I remember you mentioned that you will remove the v4l2 clock in future. See ff5430de commit message.
So I just want to not so depends on the v4l2_clk "mclk".

This might or might not happen. We so far depend on it. And we might also keep it, just adding a CCF V4L2 clock driver to handle generic clock sources like in case (c) above.

Thanks
Guennadi

Quoted 57 lines

Best Regards,
Josh Wu

Quoted 54 lines

Quoted 54 lines

Actually months ago I already done a version of ov2640 patch which use -EPROBE_DEFER way.

But now I think the ov2640 can be probed correctly without "mclk", so it is no
need to return -EPROBE_DEFER.
And the v4l2 asyn API can handle the synchronization of host. So I prefer to
use this way.
What do you think about this?

Best Regards,
Josh Wu

Quoted 40 lines

Thanks
Guennadi

Quoted 32 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -3

    	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
    	} else {
    		dev_info(&adapter->dev, "OV2640 Probed\n");
    	}
    +	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
    	return ret;
    }
    @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client

*client)

    {
    	struct ov2640_priv       *priv = to_ov2640(client);
    -	v4l2_clk_put(priv->clk);

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
    	v4l2_device_unregister_subdev(&priv->subdev);
    	v4l2_ctrl_handler_free(&priv->hdl);
    	return 0;

Signature · 1 line

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 71 lines · Guennadi Liakhovetski

Quoted 56 lines · Josh Wu

Quoted 38 lines · Guennadi Liakhovetski

Quoted 37 lines · Josh Wu

Quoted 36 lines · Guennadi Liakhovetski

Quoted 35 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

 drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++--------
 1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +0 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/media/i2c/soc_camera/ov2640.c +6 -0

+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

maybe
soc-camera host is not probed yet.\n");

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+		else
+			priv->clk = clk;
+	}
      	return soc_camera_set_power(&client->dev, ssdd, priv->clk,

on);
    }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c.
it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Quoted 21 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Quoted 8 lines

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

I think I understand your point now.

Quoted 107 lines

Quoted 1 line

That is the motivation I want ov2640 be probed even without "mclk".

ov2640 can be used with other boards and camera hosts, not only your specific board. In other configurations your change will break the driver.

Quoted 16 lines

Quoted 2 lines

Ok, think about this: you check whether priv->clk is set on each .s_power() call, which is already a bit awkward.

yes, it is.

Quoted 10 lines

Such approach can be used when there's no other way to perform a one- time action, but here we have one. But never mind, that's not the main problem. If priv->clk isn't set, you try to acquire it. But during probing, when this function is called for the first time clock isn't available yet, but you still want to succeed probing. So, you just issue a warning and continue. But then later an application opens the camera, .s_power() is called again, but for some reason the clock might still be not available, and this time you should fail. But you don't, you succeed and then you'll fail somewhere later, presumably, with a timeout waiting for frames. Am I right?

Quoted 2 lines

if the clock (v4l2 clock: mclk) is not available, then, there is no camera host available.

This isn't true - from the hardware perspective. The clock can be supplied by a different source.

Quoted 4 lines

So the system should have no v4l2 device found.
I think in this case the application cannot call the camera sensor .s_power() via v4l2 ioctl.
So the timeout case should not happened.

No, sorry, I meant a physical clock, not aclock object. You can register the complete framework and try to use it, but if the physical clock isn't enabled, the camera sensor won't function correctly.

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

Quoted 5 lines

Quoted 5 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -0

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client
 *client,
    	if (priv->hdl.error)
    		return priv->hdl.error;
 

Quoted 12 lines

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client · line 1091

Quoted 7 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -6

-	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);
-		goto eclkget;
-	}
-
    	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

yes, you are right. In this patch, the "mclk" will requested by ov2640_s_power().

The reason why I put the getting "mclk" code from ov2640_probe() to ov2640_s_power() is : as the "mclk" here is camera host's peripheral clock. That means ov2640 still can be probed properly (read ov2640 id) even no "mclk". So when I move this code to ov2640_s_power(), otherwise the ov2640_probe() will be failed or DEFER_PROBE.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Is this true for all camera host? If it's not true, then I think use -EPROBE_DEFER would be a proper way.

Sorry, not sure what your question is.

Sorry, I don't make me clear here.
My question should be: Are all the camera host's clock_start()/stop() only operate their peripheral clock?

Yes, that's all camera hosts have. They cannot operate other unrelated clock sources.

Quoted 8 lines

Quoted 2 lines

And I'm not sure ov2640's registers
can be accessed with no running clock.

No, it seems there is a misunderstanding here.

I didn't mean ov2640 can be probed without xvclk.
What I try to say is the ov2640 can be probed without camera host's peripheral clock.

Ok, then your patch will break the driver even earlier - when trying to access ov2640 registers without enabling the clock.

Quoted 9 lines

Quoted 6 lines

I think some camera sensors can do this, but I have no idea about this one. How did you verify? Is it mentioned in a datasheet? Or did you really disconnected (grounded) the sensor clock input and tried to access its reqisters? If you just verified, that it's working without requesting the clock, are you sure your clock output isn't running permanently all the time anyway?

I didn't verify the those method as I only probed the ov2640 without ISI enabled. ISI peripheral clock is disabled and etc.

Right, this means a separate (probably always-on) clock source is used on your specific board, but this doesn't have to be the case on all other boards, where ov2640 is used.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Or are there any other patched, that change that, that I'm overseeing?

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for an example. Or did I misunderstand anything?

Quoted 4 lines

Quoted 2 lines

I can implement with your method. like in probe() function, request the v4l2_clk "mclk", if failed then return -EPROBE_DEFER.

Yes, please, I think this would be a correct solution.

According to my understanding, if I implement this way I can be compatible with the camera host which provide the xvclk for ov2640.

So I will prepare the next version with this way.
BTW: do you have any comment for the 1/5 patches?

Best Regards,
Josh Wu

Quoted 72 lines

Quoted 4 lines

But I remember you mentioned that you will remove the v4l2 clock in future.
See ff5430de commit message.
So I just want to not so depends on the v4l2_clk "mclk".

This might or might not happen. We so far depend on it. And we might also keep it, just adding a CCF V4L2 clock driver to handle generic clock sources like in case (c) above.

Thanks
Guennadi

Quoted 60 lines

Best Regards,
Josh Wu

Quoted 57 lines

Quoted 57 lines

Actually months ago I already done a version of ov2640 patch which use -EPROBE_DEFER way.

But now I think the ov2640 can be probed correctly without "mclk", so it is no need to return -EPROBE_DEFER. And the v4l2 asyn API can handle the synchronization of host. So I prefer to use this way. What do you think about this?

Best Regards,
Josh Wu

Quoted 46 lines

Thanks
Guennadi

Quoted 43 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -3

    	if (ret) {
 
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
 
    	} else {
 
    		dev_info(&adapter->dev, "OV2640 Probed\n");
 
    	}
 
    +	ret = v4l2_async_register_subdev(&priv->subdev);
 
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
 
    	return ret;
 
    }
    @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client

*client)

    {
    
    	struct ov2640_priv       *priv = to_ov2640(client);
    
    -	v4l2_clk_put(priv->clk);

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
 
    	v4l2_device_unregister_subdev(&priv->subdev);
    	v4l2_ctrl_handler_free(&priv->hdl);
    	return 0;

Hi Josh,

Quoted 88 lines · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 71 lines · Guennadi Liakhovetski

Quoted 56 lines · Josh Wu

Quoted 38 lines · Guennadi Liakhovetski

Quoted 37 lines · Josh Wu

Quoted 36 lines · Guennadi Liakhovetski

Quoted 35 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

 drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++--------
 1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +0 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/media/i2c/soc_camera/ov2640.c +6 -0

+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

maybe
soc-camera host is not probed yet.\n");

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+		else
+			priv->clk = clk;
+	}
      	return soc_camera_set_power(&client->dev, ssdd, priv->clk,

on);
    }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c.
it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Quoted 3 lines

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Quoted 24 lines

Quoted 24 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Quoted 5 lines

Quoted 5 lines

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

That's at least how
I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

Quoted 109 lines

I think I understand your point now.

Quoted 107 lines

Quoted 1 line

That is the motivation I want ov2640 be probed even without "mclk".

ov2640 can be used with other boards and camera hosts, not only your specific board. In other configurations your change will break the driver.

Quoted 16 lines

Quoted 2 lines

Ok, think about this: you check whether priv->clk is set on each .s_power() call, which is already a bit awkward.

yes, it is.

Quoted 10 lines

Such approach can be used when there's no other way to perform a one- time action, but here we have one. But never mind, that's not the main problem. If priv->clk isn't set, you try to acquire it. But during probing, when this function is called for the first time clock isn't available yet, but you still want to succeed probing. So, you just issue a warning and continue. But then later an application opens the camera, .s_power() is called again, but for some reason the clock might still be not available, and this time you should fail. But you don't, you succeed and then you'll fail somewhere later, presumably, with a timeout waiting for frames. Am I right?

Quoted 2 lines

if the clock (v4l2 clock: mclk) is not available, then, there is no camera host available.

This isn't true - from the hardware perspective. The clock can be supplied by a different source.

Quoted 4 lines

So the system should have no v4l2 device found.
I think in this case the application cannot call the camera sensor .s_power() via v4l2 ioctl.
So the timeout case should not happened.

No, sorry, I meant a physical clock, not aclock object. You can register the complete framework and try to use it, but if the physical clock isn't enabled, the camera sensor won't function correctly.

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

Quoted 5 lines

Quoted 5 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -0

@@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client
 *client,
    	if (priv->hdl.error)
    		return priv->hdl.error;
 

Quoted 12 lines

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client · line 1091

Quoted 7 lines

drivers/media/i2c/soc_camera/ov2640.c +0 -6

-	priv->clk = v4l2_clk_get(&client->dev, "mclk");
-	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);
-		goto eclkget;
-	}
-
    	ret = ov2640_video_probe(client);

The first thing the above ov2640_video_probe() function will do is call ov2640_s_power(), which will request the clock. So, by moving requesting the clock from ov2640_probe() to ov2640_s_power() doesn't change how probing will be performed, am I right?

yes, you are right. In this patch, the "mclk" will requested by ov2640_s_power().

The reason why I put the getting "mclk" code from ov2640_probe() to ov2640_s_power() is : as the "mclk" here is camera host's peripheral clock. That means ov2640 still can be probed properly (read ov2640 id) even no "mclk". So when I move this code to ov2640_s_power(), otherwise the ov2640_probe() will be failed or DEFER_PROBE.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Is this true for all camera host? If it's not true, then I think use -EPROBE_DEFER would be a proper way.

Sorry, not sure what your question is.

Sorry, I don't make me clear here.
My question should be: Are all the camera host's clock_start()/stop() only operate their peripheral clock?

Yes, that's all camera hosts have. They cannot operate other unrelated clock sources.

Quoted 8 lines

Quoted 2 lines

And I'm not sure ov2640's registers
can be accessed with no running clock.

No, it seems there is a misunderstanding here.

I didn't mean ov2640 can be probed without xvclk.
What I try to say is the ov2640 can be probed without camera host's peripheral clock.

Ok, then your patch will break the driver even earlier - when trying to access ov2640 registers without enabling the clock.

Quoted 9 lines

Quoted 6 lines

I think some camera sensors can do this, but I have no idea about this one. How did you verify? Is it mentioned in a datasheet? Or did you really disconnected (grounded) the sensor clock input and tried to access its reqisters? If you just verified, that it's working without requesting the clock, are you sure your clock output isn't running permanently all the time anyway?

I didn't verify the those method as I only probed the ov2640 without ISI enabled. ISI peripheral clock is disabled and etc.

Right, this means a separate (probably always-on) clock source is used on your specific board, but this doesn't have to be the case on all other boards, where ov2640 is used.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Or are there any other patched, that change that, that I'm overseeing?

If I'm right, then I would propose an approach, already used in other drivers instead of this one: return -EPROBE_DEFER if the clock isn't available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a for an example. Or did I misunderstand anything?

That commit introduced the following code block in the mt9m111 driver.

       mt9m111->clk = v4l2_clk_get(&client->dev, "mclk");
       if (IS_ERR(mt9m111->clk))
               return -EPROBE_DEFER;

The right thing to do is to return PTR_ERR(mt9m111->clk), not a hardcoded - EPROBE_DEFER. v4l2_clk_get() should return -EPROBE_DEFER if the clock is not found,

and possibly other error codes to indicate different errors.

Quoted 86 lines

Quoted 4 lines

Quoted 2 lines

I can implement with your method. like in probe() function, request the v4l2_clk "mclk", if failed then return -EPROBE_DEFER.

Yes, please, I think this would be a correct solution.

According to my understanding, if I implement this way I can be compatible with the camera host which provide the xvclk for ov2640.

So I will prepare the next version with this way.
BTW: do you have any comment for the 1/5 patches?

Best Regards,
Josh Wu

Quoted 72 lines

Quoted 4 lines

But I remember you mentioned that you will remove the v4l2 clock in future.
See ff5430de commit message.
So I just want to not so depends on the v4l2_clk "mclk".

This might or might not happen. We so far depend on it. And we might also keep it, just adding a CCF V4L2 clock driver to handle generic clock sources like in case (c) above.

Thanks
Guennadi

Quoted 60 lines

Best Regards,
Josh Wu

Quoted 57 lines

Quoted 57 lines

Actually months ago I already done a version of ov2640 patch which use -EPROBE_DEFER way.

But now I think the ov2640 can be probed correctly without "mclk", so it is no need to return -EPROBE_DEFER. And the v4l2 asyn API can handle the synchronization of host. So I prefer to use this way. What do you think about this?

Best Regards,
Josh Wu

Quoted 46 lines

Thanks
Guennadi

Quoted 43 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -3

    	if (ret) {
 
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto evideoprobe;
 
    	} else {
 
    		dev_info(&adapter->dev, "OV2640 Probed\n");
 
    	}
 
    +	ret = v4l2_async_register_subdev(&priv->subdev);
 
+	if (ret < 0)
+		goto evideoprobe;
+
+	return 0;
+
+evideoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
 
    	return ret;
 
    }
    @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client

*client)

    {
    
    	struct ov2640_priv       *priv = to_ov2640(client);
    
    -	v4l2_clk_put(priv->clk);

drivers/media/i2c/soc_camera/ov2640.c +3 -0

+	v4l2_async_unregister_subdev(&priv->subdev);
+	if (priv->clk)
+		v4l2_clk_put(priv->clk);
 
    	v4l2_device_unregister_subdev(&priv->subdev);
    	v4l2_ctrl_handler_free(&priv->hdl);
    	return 0;

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 91 lines · Laurent Pinchart

Quoted 89 lines · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 69 lines · Guennadi Liakhovetski

Quoted 54 lines · Josh Wu

Quoted 42 lines · Guennadi Liakhovetski

Quoted 41 lines · Josh Wu

Quoted 40 lines · Guennadi Liakhovetski

Quoted 39 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

 drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
 1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +5 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c +4 -0

+			dev_warn(&client->dev, "Cannot get the mclk.
 maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}

      	return soc_camera_set_power(&client->dev, ssdd, priv
->clk, on);
    }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c. it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Quoted 2 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Quoted 4 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

Hi Guennadi,

Quoted 92 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 91 lines · Laurent Pinchart

Quoted 89 lines · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 69 lines · Guennadi Liakhovetski

Quoted 54 lines · Josh Wu

Quoted 42 lines · Guennadi Liakhovetski

Quoted 41 lines · Josh Wu

Quoted 40 lines · Guennadi Liakhovetski

Quoted 39 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

 drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
 1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +5 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c +4 -0

+			dev_warn(&client->dev, "Cannot get the mclk.
 maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}

      	return soc_camera_set_power(&client->dev, ssdd, priv
->clk, on);
    }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c. it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Quoted 8 lines

Quoted 2 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Quoted 30 lines

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

Quoted 6 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 7 lines

Quoted 4 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 3 lines · Laurent Pinchart

Quoted 2 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 1 line · Laurent Pinchart

On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Quoted 5 lines

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Sure, that'd be fine with me, if everyone else agrees.

Quoted 22 lines

Quoted 6 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 8 lines

Quoted 5 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed,
or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

Quoted 6 lines

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Indeed, what should we? :)

Hi Guennadi,

Quoted 4 lines · On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:

Quoted 3 lines · Laurent Pinchart

Quoted 2 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 1 line · Laurent Pinchart

On Friday 26 December 2014 14:37:14 Josh Wu wrote:

[snip]

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Quoted 3 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Quoted 1 line

Sure, that'd be fine with me, if everyone else agrees.

Can you submit a patch ? That's the best way to find out if anyone objects.

[snip]

Quoted 27 lines

Quoted 27 lines

Quoted 6 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 8 lines

Quoted 5 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed,
or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

Quoted 3 lines

Quoted 1 line

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Indeed, what should we? :)

You're listed as the soc-camera maintainer, so you should provide an answer to that question :-)

I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail).

Signature · 4 lines

-- 
Regards,

Laurent Pinchart

Hi Laurent,

First of all, sorry, I am currently on a holiday, so, replies are delayed, real work (reviewing or anything else) is impossible.

Quoted 24 lines · On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:

Quoted 4 lines · Laurent Pinchart

Quoted 3 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 2 lines · Laurent Pinchart

Quoted 1 line · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

[snip]

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Quoted 3 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Quoted 4 lines

Quoted 1 line

Sure, that'd be fine with me, if everyone else agrees.

Can you submit a patch ? That's the best way to find out if anyone objects.

Can do, sure, once I am back home and find time for this.

Quoted 22 lines

[snip]

Quoted 20 lines

Quoted 20 lines

Quoted 6 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 7 lines

Quoted 4 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Indeed, what should we? :)

You're listed as the soc-camera maintainer, so you should provide an answer to that question :-)

Thanks for ar reminder;)

Quoted 2 lines

I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail).

Let's see what they reply, but I don't tgink Josh will want to wait that long. And until omap1 is in the mainline we cannot drop v4l2_clk.

Hi Laurent,

First of all, sorry, I am currently on a holiday, so, replies are delayed, real work (reviewing or anything else) is impossible.

Quoted 18 lines

Hi Guennadi,

Quoted 4 lines · On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:

Quoted 3 lines · Laurent Pinchart

Quoted 2 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 1 line · Laurent Pinchart

On Friday 26 December 2014 14:37:14 Josh Wu wrote:

[snip]

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Sure, that'd be fine with me, if everyone else agrees.

Can you submit a patch ? That's the best way to find out if anyone objects.

Can do, sure, once I am back home and find time for this.

Quoted 27 lines

[snip]

Quoted 25 lines

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 7 lines

Quoted 5 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed,
or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

Quoted 6 lines

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Indeed, what should we? :)

Quoted 2 lines

You're listed as the soc-camera maintainer, so you should provide an answer to that question :-)

Thanks for ar reminder;)

Quoted 2 lines

I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail).

Let's see what they reply, but I don't tgink Josh will want to wait that long.

And until omap1 is in the mainline we cannot drop v4l2_clk.

Thanks
Guennadi

Quoted 5 lines

Signature · 3 lines

Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line

And until omap1 is in the mainline we cannot drop v4l2_clk.

So I think the better way right now for ov2640 driver is still request both the v4l2_clock: mclk, and the clock: xvclk in probe().
In that way, we can take our time to introduce other patches to merged these two clocks.
Does it make sense?

On Tue, 30 Dec 2014, Josh Wu wrote:

[snip]

Quoted 1 line

Quoted 1 line

And until omap1 is in the mainline we cannot drop v4l2_clk.

s/until/as lonh as/

Quoted 4 lines

So I think the better way right now for ov2640 driver is still request both the v4l2_clock: mclk, and the clock: xvclk in probe(). In that way, we can take our time to introduce other patches to merged these two clocks. Does it make sense?

How about this sequence, that we cat still try to get in on time for the next merge window:

1. stop using the clock name in v4l2_clk
2. add support for CCF to v4l2_clk, falling back to current behaviour if no clock is found
3. switch ov2640 to using "xvclk"

Otherwise I would propose to add asynchronous probing support to ov2640 _without_ changing the clock name. Whether or not we changee clock's name isn't related directly to async support, since the driver already is using v4l2_clk with the standarf "wrong" name. So, I would consider these two

changes separately - one is a new feature, another is a fix. The only question is in which order to apply them. In general fix-first is preferred, but since in this case the fix requires framework changes, we could go with feature-first. This also makes sense since features need to catch a merge window, whereas a fix can go in later too. So, if we don't manage the above 3-step plan, I would priposw the most primitive patch ro ov2640 just adding async support in line wuth other drivers and without changing the clock name at first.

Hi Guennadi,

Quoted 87 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 86 lines · Laurent Pinchart

Quoted 85 lines · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 67 lines · Guennadi Liakhovetski

Quoted 53 lines · Josh Wu

Quoted 42 lines · Guennadi Liakhovetski

Quoted 41 lines · Josh Wu

Quoted 40 lines · Guennadi Liakhovetski

Quoted 39 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

  drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +5 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/i2c/soc_camera/ov2640.c +4 -0

+			dev_warn(&client->dev, "Cannot get the mclk.
 maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}

       	return soc_camera_set_power(&client->dev, ssdd, priv
->clk, on);
     }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c. it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Quoted 7 lines

Quoted 2 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

Quoted 5 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 6 lines

Quoted 4 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Quoted 104 lines · Laurent Pinchart

Quoted 97 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 88 lines · Laurent Pinchart

Quoted 87 lines · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 70 lines · Guennadi Liakhovetski

Quoted 55 lines · Josh Wu

Quoted 48 lines · Guennadi Liakhovetski

Quoted 47 lines · Josh Wu

Quoted 46 lines · Guennadi Liakhovetski

Quoted 45 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------

1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +9 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk.
 maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

       	return soc_camera_set_power(&client->dev, ssdd, priv
->clk, on);
     }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c. it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Quoted 7 lines

Quoted 3 lines

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Quoted 5 lines

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Quoted 5 lines

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Sorry, I'm not clear about how to implement what you discussed here.

Do you mean, In the ov2640 driver:
1. need to remove the patch 4/5, "add a master clock for sensor"

2. need to register a "xvclk" v4l2 clock which is a CCF clock. Or this part can put in soc_camera.c
3. So in ov2640_probe(), need to call v4l2_clk_get("xvclk"), which will do a. Get CCF clock "xvclk" by call devm_clk_get("xvclk"), and if
failed then return the error code.
b. Get the v4l2 clock "mclk", if failed then return the error code.

4. In ov2640_s_power(), we'll call soc_camera_set_power(..., priv->clk, ...) to enable "xvclk" and "mclk" clock.

Please correct me if I misunderstand your meaning?

Best Regards,
Josh Wu

Quoted 57 lines

Quoted 30 lines

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

Quoted 6 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 7 lines

Quoted 4 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Hi Josh,

Quoted 103 lines · On Monday 29 December 2014 16:28:02 Josh Wu wrote:

Quoted 97 lines · Laurent Pinchart

Quoted 88 lines · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

Quoted 87 lines · Laurent Pinchart

Quoted 86 lines · On Friday 26 December 2014 14:37:14 Josh Wu wrote:

Quoted 68 lines · Guennadi Liakhovetski

Quoted 54 lines · Josh Wu

Quoted 48 lines · Guennadi Liakhovetski

Quoted 47 lines · Josh Wu

Quoted 46 lines · Guennadi Liakhovetski

Quoted 45 lines · Josh Wu

To support async probe for ov2640, we need remove the code to get 'mclk' in ov2640_probe() function. oterwise, if soc_camera host is not probed in the moment, then we will fail to get 'mclk' and quit the ov2640_probe() function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power() function. That make ov2640 survive, as we can pass a NULL (priv-clk) to soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called, then we can get the 'mclk' and that make us enable/disable soc_camera host's clock as well.

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
v2 -> v3:
v1 -> v2:
no changes.

drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------

1 file changed, 21 insertions(+), 10 deletions(-)

drivers/media/i2c/soc_camera/ov2640.c +9 -0

index 1fdce2f..9ee910d 100644
@@ -739,6 +739,15 @@ static int ov2640_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 ov2640_priv *priv = to_ov2640(client);
+	struct v4l2_clk *clk;
+
+	if (!priv->clk) {
+		clk = v4l2_clk_get(&client->dev, "mclk");
+		if (IS_ERR(clk))
+			dev_warn(&client->dev, "Cannot get the mclk.
 maybe soc-camera host is not probed yet.\n");
+		else
+			priv->clk = clk;
+	}

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

        	return soc_camera_set_power(&client->dev, ssdd, priv
->clk, on);
      }

Just let me explained a little more details at first:

Quoted 6 lines

As my understanding, current the priv->clk is a v4l2_clk: mclk, which is a wrapper clock in soc_camera.c. it can make soc_camera to call camera host's clock_start() clock_stop(). As in ov2640, the real mck (pck1) is in ov2640 dt node (xvclk). So the camera host's clock_start()/stop() only need to enable/disable his peripheral clock.

I'm looking at the ov2640 datasheet. In the block diagram I only see one input clock - the xvclk. Yes, it can be supplied by the camera host controller, in which case it is natural for the camera host driver to own and control it, or it can be a separate clock device - either static or configurable. This is just a note to myself to clarify, that it's one and the same clock pin we're talking about.

Quoted 4 lines

Now, from the hardware / DT PoV, I think, the DT should look like:

a) in the ov2640 I2C DT node we should have a clock consumer entry, linking to a board-specific source.

That's what this patch series do right now.
In my patch 5/5 DT document said, ov2640 need a clock consumer which refer to the xvclk input clock.
And it is a required property.

Quoted 5 lines

b) if the ov2640 clock is supplied by a camera host, its DT entry should have a clock source subnode, to which ov2640 clock consumer entry should link. The respective camera host driver should then parse that clock subnode and register the respective clock with the V4L2 framework, by calling v4l2_clk_register().

Ok, So in this case, I need to wait for the "mclk" in probe of ov2640 driver. So that I can be compatible for the camera host which provide the clock source.

Quoted 8 lines

Quoted 3 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Quoted 5 lines

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Quoted 4 lines

Sorry, I'm not clear about how to implement what you discussed here.

Do you mean, In the ov2640 driver:
1. need to remove the patch 4/5, "add a master clock for sensor"

No, the sensor has a clock input named "xvclk", the ov2640 driver should thus manage that clock. Patch 4/5 does the right thing.

However, I've just realized that it will cause regressions on the i.MX27, i.MX31 and i.MX37 3DS development boards that use the sensor without registering a clock named xvclk. You should fix that as part of the patch series.

Quoted 7 lines

2. need to register a "xvclk" v4l2 clock which is a CCF clock. Or this part can put in soc_camera.c
3. So in ov2640_probe(), need to call v4l2_clk_get("xvclk"), which will do a. Get CCF clock "xvclk" by call devm_clk_get("xvclk"), and if
failed then return the error code.
b. Get the v4l2 clock "mclk", if failed then return the error code.

v4l2_clk_get() was introduced as a temporary workaround for platforms that don't support CCF yet. It might be possible to use clk_get() directly here as the i.MX platforms support CCF (as far as I'm concerned you don't need to care about out-of-tree non-DT platforms). Otherwise we'll need to stick to v4l2_clk_get(), in which case the v4l2_clk_get() implementation will need to be modified to call clk_get() first and fall back to the V4L2 private clock list.

Quoted 2 lines

4. In ov2640_s_power(), we'll call soc_camera_set_power(..., priv->clk, ...) to enable "xvclk" and "mclk" clock.

And looking at the implementation of soc_camera_power_on() and soc_camera_power_off(), I realize that soc-camera expects to manage a v4l2_clk itself...

Guennadi, could you please detail the steps that Josh should follow, keeping in mind that the goal is to get rid of v4l2_clk_get() in the not too distant future ? The fact that soc-camera host drivers start their own hardware in their .clock_start() operation, called through the mclk pseudo-clock, makes all this pretty messy.

Do you think you'll have time to properly migrate soc-camera to DT in the not too distant future ?

Quoted 57 lines

Please correct me if I misunderstand your meaning?

Best Regards,
Josh Wu

Quoted 52 lines

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

c) if the ov2640 clock is supplied by a different clock source, the respective driver should parse it and also eventually call v4l2_clk_register().

Implementing case (b) above is so far up to each individual (soc-camera) camera host driver. In soc-camera host drivers don't register V4L2 clocks themselves, as you correctly noticed, they just provide a .clock_start() and a .clock_stop() callbacks. The registration is done by the soc-camera core.

If I understand correctly you have case (c). Unfortunately, this case isn't supported atm. I think, a suitable way to do this would be:

(1) modify soc-camera to not register a V4L2 clock if the host doesn't provide the required callbacks.

(2) hosts should recognise configurations, in which they don't supply the master clock to clients and not provide the callbacks then.

(3) a separate driver should register a suitable V4L2 clock.

Whereas I don't think we need to modify camera drivers. Their requesting of a V4L2 clock is correct as is.

Some more fine-print: if the clock is supplied by a generic device, it would be wrong for it to register a V4L2 clock. It should register a normal CCF clock, and a separate V4L2 driver should create a V4L2 clock from it. This isn't implemented either and we've been talking about it for a while now...

Quoted 5 lines

Quoted 2 lines

v4l2_clk_get() should try to get the clock from CCF with a call to clk_get() first, and then look at the list of v4l2-specific clocks.

Yes, how will it find the "mclk" when "xvclk" (or any other name) is requested? We did discuss this in the beginning and agreed to use a fixed clock name for the time being...

Please see above.

Quoted 6 lines

Quoted 4 lines

That's at least how I had envisioned it when v4l2_clk_get() was introduced. Let's remember that v4l2_clk was designed as a temporary workaround for platforms not implementing CCF yet. Is that still needed, or could be instead just get rid of it now ?

I didn't check, but I don't think all platforms, handled by soc-camera, support CCF yet.

After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable it yet for an unknown (to me) reason.

The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former will most likely never receive CCF support, and the latter is getting fixed. As arch/sh isn't maintained anymore I would be fine with dropping CEU support for it.

OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Quoted 4 lines · Laurent Pinchart

Quoted 3 lines · On Monday 29 December 2014 16:28:02 Josh Wu wrote:

Quoted 2 lines · Laurent Pinchart

Quoted 1 line · On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:

On Fri, 26 Dec 2014, Laurent Pinchart wrote:

Quoted 21 lines

Quoted 16 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

Talking about mclk and xvclk is quite confusing. There's no mclk from an ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").

Yes, I also was thinking about this, and yes, requesting a "xvclk" clock would be more logical. But then, as you write below, if we let the v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found. How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get() should use two names?..

Quoted 5 lines

Given that v4l2_clk_get() is only used by soc-camera drivers and that they all call it with the clock name set to "mclk", I wonder whether we couldn't just get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Quoted 4 lines

Sorry, I'm not clear about how to implement what you discussed here.

Do you mean, In the ov2640 driver:
1. need to remove the patch 4/5, "add a master clock for sensor"

Quoted 7 lines

No, the sensor has a clock input named "xvclk", the ov2640 driver should thus manage that clock. Patch 4/5 does the right thing.

However, I've just realized that it will cause regressions on the i.MX27, i.MX31 and i.MX37 3DS development boards that use the sensor without registering a clock named xvclk. You should fix that as part of the patch series.

Thanks for the information.
So I think to be compatible with i.MX series board, I have two ways:
1. Make the xvclk clock be optional in ov2640 driver. After the i.MX
series board switch to CCF, and we can change it to mandatory.
2. switch the i.MX host driver to DT, and add the xvclk to their dts.

As I am not similar with i.MX board and cannot test for them. I prefer to the #1, which is simple and work well. We can change the property when CCF & DT is introduced to i.MX boards.

Add the document for ov2640 dt.

Cc [email protected]

Signed-off-by Josh Wu <[email protected]>

---
v3 -> v4:
1. remove aggsigned-clocks as it's general.
2. refine the explation.

v2 -> v3:
1. fix incorrect description.
2. Add assigned-clocks & assigned-clock-rates.
3. resetb pin should be ACTIVE_LOW.

v1 -> v2:
1. change the compatible string to be consistent with verdor file.
2. change the clock and pins' name.
3. add missed pinctrl in example.

 .../devicetree/bindings/media/i2c/ov2640.txt       | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/media/i2c/ov2640.txt new file mode 100644
index 0000000..de11ebb

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +18 -0

@@ -0,0 +1,46 @@
+* Omnivision ov2640 CMOS sensor
+
+The Omnivision OV2640 sensor support multiple resolutions output, such as
+CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
+output format.
+
+Required Properties:
+- compatible: Must be "ovti,ov2640"
+- clocks: reference to the xvclk input clock.
+- clock-names: Must be "xvclk".
+
+Optional Properties:
+- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
+- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
+
+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.

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +18 -0

+
+			resetb-gpios = <&pioE 24 GPIO_ACTIVE_LOW>;
+			pwdn-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
+
+			clocks = <&pck1>;
+			clock-names = "xvclk";
+
+			assigned-clocks = <&pck1>;
+			assigned-clock-rates = <25000000>;
+
+			port {
+				ov2640_0: endpoint {
+					remote-endpoint = <&isi_0>;
+					bus-width = <8>;
+				};
+			};
+		};
+	};

Add the document for ov2640 dt.

Cc devicetree at vger.kernel.org

Signed-off-by Josh Wu <[email protected]>

Hi Josh,

Quoted 2 lines · Josh Wu

Add the document for ov2640 dt.

Quoted 2 lines

Cc devicetree at vger.kernel.org

Signed-off-by Josh Wu <[email protected]>

Acked-by Sylwester Nawrocki <[email protected]>

It seems "ovti" is not in the list of vendor prefixes. You may want to send a patch adding it to Documentation/devicetree/bindings/ vendor-prefixes.txt.

Hi Josh,

Quoted 4 lines · Josh Wu

Add the document for ov2640 dt.

Cc devicetree at vger.kernel.org

Signed-off-by Josh Wu <[email protected]>

Acked-by Sylwester Nawrocki <[email protected]>

It seems "ovti" is not in the list of vendor prefixes. You may want to send a patch adding it to Documentation/devicetree/bindings/ vendor-prefixes.txt.

Just few minor comments below..

Quoted 4 lines

  .../devicetree/bindings/media/i2c/ov2640.txt       | 46 ++++++++++++++++++++++
  1 file changed, 46 insertions(+)

create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

Documentation/devicetree/bindings/media/i2c/ov2640.txt line 1

Quoted 7 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +1 -0

new file mode 100644
index 0000000..de11ebb
@@ -0,0 +1,46 @@
+* Omnivision ov2640 CMOS sensor

s/ov2640/OV2640 ?

Documentation/devicetree/bindings/media/i2c/ov2640.txt line 8

Quoted 8 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +7 -0

+
+The Omnivision OV2640 sensor support multiple resolutions output, such as
+CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
+output format.
+
+Required Properties:
+- compatible: Must be "ovti,ov2640"

I believe it is preferred to put it as "Should contain", rather than "Must be".

Documentation/devicetree/bindings/media/i2c/ov2640.txt line 10

Quoted 3 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +2 -0

+- clocks: reference to the xvclk input clock.
+- clock-names: Must be "xvclk".

--
Regards,
Sylwester

Quoted 4 lines

Quoted 4 lines

+Required Properties:

Quoted 1 line

Quoted 1 line

+- compatible: Must be "ovti,ov2640"

I believe it is preferred to put it as "Should contain", rather than "Must be".

I don't have a strong opinion here. After check many documents, it seems many people use "Should be".
Is it okay?