[v4,2/5] media: ov2640: add async probe function

Commit Message

Josh Wu Dec. 18, 2014, 2:27 a.m. UTC

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 <[e-mail]>
---
v3 -> v4:
v2 -> v3:
v1 -> v2:
  no changes.

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

Comments

Guennadi Liakhovetski Dec. 18, 2014, 9:59 p.m. UTC | #1

Hi Josh,

Thanks for your patches!

On Thu, 18 Dec 2014, Josh Wu wrote:

> 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 <[e-mail]>
> ---
> v3 -> v4:
> v2 -> v3:
> v1 -> v2:
>   no changes.
> 
>  drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
> index 1fdce2f..9ee910d 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> @@ -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;
>  
> -	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

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

Josh Wu Dec. 19, 2014, 6:11 a.m. UTC | #2

Hi, Guennadi

Thanks for the review.

On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> Thanks for your patches!
>
> On Thu, 18 Dec 2014, Josh Wu wrote:
>
>> 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 <[e-mail]>
>> ---
>> v3 -> v4:
>> v2 -> v3:
>> v1 -> v2:
>>    no changes.
>>
>>   drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++++++----------
>>   1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
>> index 1fdce2f..9ee910d 100644
>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>> @@ -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;
>>   
>> -	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.

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


> 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

>
> Thanks
> Guennadi
>
>>   	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;
>> -- 
>> 1.9.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guennadi Liakhovetski Dec. 19, 2014, 10:16 p.m. UTC | #3

On Fri, 19 Dec 2014, Josh Wu wrote:

> Hi, Guennadi
> 
> Thanks for the review.
> 
> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> > Hi Josh,
> > 
> > Thanks for your patches!
> > 
> > On Thu, 18 Dec 2014, Josh Wu wrote:
> > 
> > > 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 <[e-mail]>
> > > ---
> > > v3 -> v4:
> > > v2 -> v3:
> > > v1 -> v2:
> > >    no changes.
> > > 
> > >   drivers/media/i2c/soc_camera/ov2640.c | 31
> > > +++++++++++++++++++++----------
> > >   1 file changed, 21 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> > > b/drivers/media/i2c/soc_camera/ov2640.c
> > > index 1fdce2f..9ee910d 100644
> > > --- a/drivers/media/i2c/soc_camera/ov2640.c
> > > +++ b/drivers/media/i2c/soc_camera/ov2640.c
> > > @@ -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);
> > >   }

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?

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

> 
> 
> > 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
> 
> > 
> > Thanks
> > Guennadi
> > 
> > >   	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;
> > > -- 
> > > 1.9.1
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Josh Wu Dec. 22, 2014, 10:27 a.m. UTC | #4

Hi, Guennadi

On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> On Fri, 19 Dec 2014, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> Thanks for the review.
>>
>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
>>> Hi Josh,
>>>
>>> Thanks for your patches!
>>>
>>> On Thu, 18 Dec 2014, Josh Wu wrote:
>>>
>>>> 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 <[e-mail]>
>>>> ---
>>>> v3 -> v4:
>>>> v2 -> v3:
>>>> v1 -> v2:
>>>>     no changes.
>>>>
>>>>    drivers/media/i2c/soc_camera/ov2640.c | 31
>>>> +++++++++++++++++++++----------
>>>>    1 file changed, 21 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>>>> b/drivers/media/i2c/soc_camera/ov2640.c
>>>> index 1fdce2f..9ee910d 100644
>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>>>> @@ -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);
>>>>    }

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

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

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

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


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

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


> Thanks
> Guennadi
>
>>
>>> 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

>> 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
>>
>>> Thanks
>>> Guennadi
>>>
>>>>    	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;
>>>> -- 
>>>> 1.9.1
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guennadi Liakhovetski Dec. 24, 2014, 10:39 p.m. UTC | #5

Hi Josh,

On Mon, 22 Dec 2014, Josh Wu wrote:

> Hi, Guennadi
> 
> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> > On Fri, 19 Dec 2014, Josh Wu wrote:
> > 
> > > Hi, Guennadi
> > > 
> > > Thanks for the review.
> > > 
> > > On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> > > > Hi Josh,
> > > > 
> > > > Thanks for your patches!
> > > > 
> > > > On Thu, 18 Dec 2014, Josh Wu wrote:
> > > > 
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > > v3 -> v4:
> > > > > v2 -> v3:
> > > > > v1 -> v2:
> > > > >     no changes.
> > > > > 
> > > > >    drivers/media/i2c/soc_camera/ov2640.c | 31
> > > > > +++++++++++++++++++++----------
> > > > >    1 file changed, 21 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> > > > > b/drivers/media/i2c/soc_camera/ov2640.c
> > > > > index 1fdce2f..9ee910d 100644
> > > > > --- a/drivers/media/i2c/soc_camera/ov2640.c
> > > > > +++ b/drivers/media/i2c/soc_camera/ov2640.c
> > > > > @@ -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);
> > > > >    }
> 
> 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.

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

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

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

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

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

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

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

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

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

> 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

> Best Regards,
> Josh Wu
> 
> > > 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
> > > 
> > > > Thanks
> > > > Guennadi
> > > > 
> > > > >    	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;
> > > > > -- 
> > > > > 1.9.1
> > > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-media"
> > > > in
> > > > the body of a message to [e-mail]
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Josh Wu Dec. 26, 2014, 6:37 a.m. UTC | #6

Hi, Guennadi

Thanks for the reply.
And Merry Christmas and happy new year.

On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Mon, 22 Dec 2014, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
>>> On Fri, 19 Dec 2014, Josh Wu wrote:
>>>
>>>> Hi, Guennadi
>>>>
>>>> Thanks for the review.
>>>>
>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
>>>>> Hi Josh,
>>>>>
>>>>> Thanks for your patches!
>>>>>
>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
>>>>>
>>>>>> 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 <[e-mail]>
>>>>>> ---
>>>>>> v3 -> v4:
>>>>>> v2 -> v3:
>>>>>> v1 -> v2:
>>>>>>      no changes.
>>>>>>
>>>>>>     drivers/media/i2c/soc_camera/ov2640.c | 31
>>>>>> +++++++++++++++++++++----------
>>>>>>     1 file changed, 21 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
>>>>>> index 1fdce2f..9ee910d 100644
>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>>>>>> @@ -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);
>>>>>>     }
>> 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.
> 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.

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.

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

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

I think I understand your point now.

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

>
>> 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
>
>> Best Regards,
>> Josh Wu
>>
>>>> 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
>>>>
>>>>> Thanks
>>>>> Guennadi
>>>>>
>>>>>>     	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;
>>>>>> -- 
>>>>>> 1.9.1
>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-media"
>>>>> in
>>>>> the body of a message to [e-mail]
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Dec. 26, 2014, 9:01 a.m. UTC | #7

Hi Josh,

On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> > On Mon, 22 Dec 2014, Josh Wu wrote:
> >> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> >>> On Fri, 19 Dec 2014, Josh Wu wrote:
> >>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> >>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
> >>>>>> 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 <[e-mail]>
> >>>>>> ---
> >>>>>> v3 -> v4:
> >>>>>> v2 -> v3:
> >>>>>> v1 -> v2:
> >>>>>>      no changes.
> >>>>>>     
> >>>>>>  drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++--------
> >>>>>>  1 file changed, 21 insertions(+), 10 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>> index 1fdce2f..9ee910d 100644
> >>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>> @@ -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);
> >>>>>>     }
> >> 
> >> 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.
> >
> > 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.
> 
> 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.
> 
> > 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.

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

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

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 ?

> I think I understand your point now.
> 
> >> 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.
> > 
> >>> 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.
> >> 
> >>> 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?
> >> 
> >> 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.
> > 
> >> 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.
> > 
> >>>>>> @@ -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.
> >>>> 
> >>>> 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.
> > 
> >>> 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.
> > 
> >>> 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.
> > 
> >>>>> 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.

> >> 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
> 
> >> 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
> > 
> >> Best Regards,
> >> Josh Wu
> >> 
> >>>> 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
> >>>> 
> >>>>> Thanks
> >>>>> Guennadi
> >>>>> 
> >>>>>>     	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;

Guennadi Liakhovetski Dec. 26, 2014, 9:14 a.m. UTC | #8

Hi Laurent,

On Fri, 26 Dec 2014, Laurent Pinchart wrote:

> Hi Josh,
> 
> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> > On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> > > On Mon, 22 Dec 2014, Josh Wu wrote:
> > >> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> > >>> On Fri, 19 Dec 2014, Josh Wu wrote:
> > >>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> > >>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
> > >>>>>> 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 <[e-mail]>
> > >>>>>> ---
> > >>>>>> v3 -> v4:
> > >>>>>> v2 -> v3:
> > >>>>>> v1 -> v2:
> > >>>>>>      no changes.
> > >>>>>>     
> > >>>>>>  drivers/media/i2c/soc_camera/ov2640.c | 31 +++++++++++++++++--------
> > >>>>>>  1 file changed, 21 insertions(+), 10 deletions(-)
> > >>>>>> 
> > >>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>> index 1fdce2f..9ee910d 100644
> > >>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>> @@ -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);
> > >>>>>>     }
> > >> 
> > >> 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.
> > >
> > > 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.
> > 
> > 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.
> > 
> > > 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.
> 
> 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?..

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

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

> > I think I understand your point now.
> > 
> > >> 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.
> > > 
> > >>> 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.
> > >> 
> > >>> 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?
> > >> 
> > >> 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.
> > > 
> > >> 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.
> > > 
> > >>>>>> @@ -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.
> > >>>> 
> > >>>> 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.
> > > 
> > >>> 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.
> > > 
> > >>> 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.
> > > 
> > >>>>> 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,

This could be done, yes, currently it returns ERR_PTR(-ENODEV) in such 
cases.

Thanks
Guennadi

> and possibly other error codes to indicate different errors.
> 
> > >> 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
> > 
> > >> 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
> > > 
> > >> Best Regards,
> > >> Josh Wu
> > >> 
> > >>>> 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
> > >>>> 
> > >>>>> Thanks
> > >>>>> Guennadi
> > >>>>> 
> > >>>>>>     	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;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>

Laurent Pinchart Dec. 26, 2014, 10:06 a.m. UTC | #9

Hi Guennadi,

On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> > On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> >> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> >>> On Mon, 22 Dec 2014, Josh Wu wrote:
> >>>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> >>>>> On Fri, 19 Dec 2014, Josh Wu wrote:
> >>>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> >>>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
> >>>>>>>> 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 <[e-mail]>
> >>>>>>>> ---
> >>>>>>>> v3 -> v4:
> >>>>>>>> v2 -> v3:
> >>>>>>>> v1 -> v2:
> >>>>>>>>      no changes.
> >>>>>>>>  
> >>>>>>>>  drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
> >>>>>>>>  1 file changed, 21 insertions(+), 10 deletions(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>> index 1fdce2f..9ee910d 100644
> >>>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>> @@ -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);
> >>>>>>>>     }
> >>>> 
> >>>> 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.
> >>> 
> >>> 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.
> >> 
> >> 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.
> >> 
> >>> 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.
> > 
> > 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 :-)

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

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

Guennadi Liakhovetski Dec. 26, 2014, 10:38 a.m. UTC | #10

On Fri, 26 Dec 2014, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> > On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> > > On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> > >> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> > >>> On Mon, 22 Dec 2014, Josh Wu wrote:
> > >>>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> > >>>>> On Fri, 19 Dec 2014, Josh Wu wrote:
> > >>>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> > >>>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
> > >>>>>>>> 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 <[e-mail]>
> > >>>>>>>> ---
> > >>>>>>>> v3 -> v4:
> > >>>>>>>> v2 -> v3:
> > >>>>>>>> v1 -> v2:
> > >>>>>>>>      no changes.
> > >>>>>>>>  
> > >>>>>>>>  drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
> > >>>>>>>>  1 file changed, 21 insertions(+), 10 deletions(-)
> > >>>>>>>> 
> > >>>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> index 1fdce2f..9ee910d 100644
> > >>>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> @@ -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);
> > >>>>>>>>     }
> > >>>> 
> > >>>> 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.
> > >>> 
> > >>> 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.
> > >> 
> > >> 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.
> > >> 
> > >>> 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.
> > > 
> > > 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 :-)

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

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

Indeed, what should we? :)

Thanks
Guennadi

> -- 
> Regards,
> 
> Laurent Pinchart

Josh Wu Dec. 29, 2014, 8:28 a.m. UTC | #11

Hi, Laurent and Guennadi

On 12/26/2014 6:06 PM, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
>> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
>>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
>>>> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
>>>>> On Mon, 22 Dec 2014, Josh Wu wrote:
>>>>>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
>>>>>>> On Fri, 19 Dec 2014, Josh Wu wrote:
>>>>>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
>>>>>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>> v3 -> v4:
>>>>>>>>>> v2 -> v3:
>>>>>>>>>> v1 -> v2:
>>>>>>>>>>       no changes.
>>>>>>>>>>   
>>>>>>>>>>   drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
>>>>>>>>>>   1 file changed, 21 insertions(+), 10 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>> index 1fdce2f..9ee910d 100644
>>>>>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>> @@ -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);
>>>>>>>>>>      }
>>>>>> 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.
>>>>> 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.
>>>> 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.
>>>>
>>>>> 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.
>>> 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 :-)

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

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

Laurent Pinchart Dec. 30, 2014, 12:15 a.m. UTC | #12

Hi Josh,

On Monday 29 December 2014 16:28:02 Josh Wu wrote:
> On 12/26/2014 6:06 PM, Laurent Pinchart wrote:
> > On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> >> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> >>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> >>>> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> >>>>> On Mon, 22 Dec 2014, Josh Wu wrote:
> >>>>>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> >>>>>>> On Fri, 19 Dec 2014, Josh Wu wrote:
> >>>>>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> >>>>>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
> >>>>>>>>>> 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 <[e-mail]>
> >>>>>>>>>> ---
> >>>>>>>>>> v3 -> v4:
> >>>>>>>>>> v2 -> v3:
> >>>>>>>>>> v1 -> v2:
> >>>>>>>>>>       no changes.
> >>>>>>>>>>   
> >>>>>>>>>> drivers/media/i2c/soc_camera/ov2640.c | 31  ++++++++++++++-------
> >>>>>>>>>> 1 file changed, 21 insertions(+), 10 deletions(-)
> >>>>>>>>>> 
> >>>>>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>>>> index 1fdce2f..9ee910d 100644
> >>>>>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> >>>>>>>>>> @@ -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);
> >>>>>>>>>>      }
> >>>>>> 
> >>>>>> 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.
> >>>>> 
> >>>>> 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.
> >>>> 
> >>>> 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.
> >>>> 
> >>>>> 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.
> >>> 
> >>> 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 :-)
> 
> 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.

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

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

> Please correct me if I misunderstand your meaning?
> 
> Best Regards,
> Josh Wu
> 
> >>>>> 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...
> >>> 
> >>> 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.
> > 
> >>> 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 ?

Laurent Pinchart Dec. 30, 2014, 12:23 a.m. UTC | #13

Hi Guennadi,

On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:
> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> > On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> >> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> >>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:

[snip]

> >>> 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 :-)
> 
> 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]

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

Guennadi Liakhovetski Dec. 30, 2014, 8:36 a.m. UTC | #14

Hi Laurent,

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

On Tue, 30 Dec 2014, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:
> > On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> > > On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> > >> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> > >>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> 
> [snip]
> 
> > >>> 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 :-)
> > 
> > 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.

> [snip]
> 
> > >>> 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.
> > > 
> > >>> 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 ?
> > 
> > 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;)

> 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

> 
> -- 
> Regards,
> 
> Laurent Pinchart
>

Laurent Pinchart Dec. 30, 2014, 8:58 a.m. UTC | #15

Hi Guennadi,

On Tuesday 30 December 2014 09:36:31 Guennadi Liakhovetski wrote:
> Hi Laurent,
> 
> First of all, sorry, I am currently on a holiday, so, replies are delayed,
> real work (reviewing or anything else) is impossible.

Sure, no worries. Enjoy your holidays without thinking too much about soc-
camera :-)

> On Tue, 30 Dec 2014, Laurent Pinchart wrote:
> > On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:
> >> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> >>> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> >>>> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> >>>>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> >
> > [snip]
> > 
> >>>>> 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 :-)
> >> 
> >> 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.
> 
> > [snip]
> > 
> >>>>> 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.
> >>> 
> >>>>> 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.

On a side note, patches to enable CCF for PXA27x have been posted, they should 
be merged in v3.20.

> >>> 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? :)
> > 
> > You're listed as the soc-camera maintainer, so you should provide an
> > answer to that question :-)
> 
> Thanks for ar reminder;)
> 
> > 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.

Agreed, this was more of a question for the next step.

Josh Wu Dec. 30, 2014, 10:02 a.m. UTC | #16

Hi, Laurent

On 12/30/2014 8:15 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> On Monday 29 December 2014 16:28:02 Josh Wu wrote:
>> On 12/26/2014 6:06 PM, Laurent Pinchart wrote:
>>> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
>>>> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
>>>>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
>>>>>> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
>>>>>>> On Mon, 22 Dec 2014, Josh Wu wrote:
>>>>>>>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
>>>>>>>>> On Fri, 19 Dec 2014, Josh Wu wrote:
>>>>>>>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
>>>>>>>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
>>>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>> v3 -> v4:
>>>>>>>>>>>> v2 -> v3:
>>>>>>>>>>>> v1 -> v2:
>>>>>>>>>>>>        no changes.
>>>>>>>>>>>>    
>>>>>>>>>>>> drivers/media/i2c/soc_camera/ov2640.c | 31  ++++++++++++++-------
>>>>>>>>>>>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>>>> index 1fdce2f..9ee910d 100644
>>>>>>>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>>>>>>>>>>>> @@ -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);
>>>>>>>>>>>>       }
>>>>>>>> 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.
>>>>>>> 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.
>>>>>> 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.
>>>>>>
>>>>>>> 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.
>>>>> 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 :-)
>> 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.

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.

Best Regards,
Josh Wu

>
>> 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.
>
>> 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 ?
>
>> Please correct me if I misunderstand your meaning?
>>
>> Best Regards,
>> Josh Wu
>>
>>>>>>> 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...
>>>>> 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.
>>>
>>>>> 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 ?

Josh Wu Dec. 30, 2014, 10:08 a.m. UTC | #17

Hi, Guennadi

On 12/30/2014 4:36 PM, Guennadi Liakhovetski wrote:
> Hi Laurent,
>
> First of all, sorry, I am currently on a holiday, so, replies are delayed,
> real work (reviewing or anything else) is impossible.

Thanks for your review in holiday. That's very helpful.
>
> On Tue, 30 Dec 2014, Laurent Pinchart wrote:
>
>> Hi Guennadi,
>>
>> On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote:
>>> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
>>>> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
>>>>> On Fri, 26 Dec 2014, Laurent Pinchart wrote:
>>>>>> On Friday 26 December 2014 14:37:14 Josh Wu wrote:
>> [snip]
>>
>>>>>> 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 :-)
>>> 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.
>
>> [snip]
>>
>>>>>> 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.
>>>>
>>>>>> 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 ?
>>> 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;)
>
>> 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.

Thanks. it's indeed true for me.

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

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guennadi Liakhovetski Dec. 30, 2014, 12:12 p.m. UTC | #18

On Tue, 30 Dec 2014, Josh Wu wrote:

[snip]

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

s/until/as lonh as/

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

Thanks
Guennadi

Laurent Pinchart Jan. 1, 2015, 5:43 p.m. UTC | #19

Hi Josh,

On Tuesday 30 December 2014 18:02:23 Josh Wu wrote:
> On 12/30/2014 8:15 AM, Laurent Pinchart wrote:
> > On Monday 29 December 2014 16:28:02 Josh Wu wrote:
> >> On 12/26/2014 6:06 PM, Laurent Pinchart wrote:
> >>> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> >>>> On Fri, 26 Dec 2014, Laurent Pinchart wrote:

[snip]

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

I'd be fine with this if DT bindings were not required to be stable. The 
driver can always be fixed later, but biding should be correct from the start. 
As the ov2640 chip requires a clock, the bindings must require the clock, and 
the driver must enforce the requirement, at least when the device is 
instantiated from DT.

Laurent Pinchart Jan. 1, 2015, 5:44 p.m. UTC | #20

Hi Guennadi,

On Tuesday 30 December 2014 13:12:27 Guennadi Liakhovetski wrote:
> On Tue, 30 Dec 2014, Josh Wu wrote:
> 
> [snip]
> 
> > > And until omap1 is in the mainline we cannot drop v4l2_clk.
> 
> s/until/as lonh as/
> 
> > 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"

It looks good at first sight.

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

Async support can go in without the clock rename, but DT support can't.

Patch

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 1fdce2f..9ee910d 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -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;
 
-	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);
 	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;