diff mbox

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

Message ID 20170928081828.20335-4-wenyou.yang@microchip.com (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Wenyou Yang Sept. 28, 2017, 8:18 a.m. UTC 
  To meet the relationship, enable the HCLOCK and ispck during the
device probe, "isc_pck frequency is less than or equal to isc_ispck,
and isc_ispck is greater than or equal to HCLOCK."
Meanwhile, call the pm_runtime_enable() in the right place.

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

Changes in v3: None
Changes in v2: None

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

Comments

Sakari Ailus Sept. 28, 2017, 9:25 p.m. UTC | #1 
Hi Wenyou,

On Thu, Sep 28, 2017 at 04:18:26PM +0800, Wenyou Yang wrote:
> To meet the relationship, enable the HCLOCK and ispck during the
> device probe, "isc_pck frequency is less than or equal to isc_ispck,
> and isc_ispck is greater than or equal to HCLOCK."
> Meanwhile, call the pm_runtime_enable() in the right place.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/media/platform/atmel/atmel-isc.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index 0b15dc1a3a0b..f092c95587c1 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -1594,6 +1594,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>  	struct isc_subdev_entity *sd_entity;
>  	struct video_device *vdev = &isc->video_dev;
>  	struct vb2_queue *q = &isc->vb2_vidq;
> +	struct device *dev = isc->dev;
>  	int ret;
>  
>  	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
> @@ -1677,6 +1678,10 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>  		return ret;
>  	}
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_request_idle(dev);

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

What would be the reason to move it here?

> +
>  	return 0;
>  }
>  
> @@ -1856,25 +1861,37 @@ static int atmel_isc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = clk_prepare_enable(isc->hclock);
> +	if (ret) {
> +		dev_err(dev, "failed to enable hclock: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = isc_clk_init(isc);
>  	if (ret) {
>  		dev_err(dev, "failed to init isc clock: %d\n", ret);
> -		goto clean_isc_clk;
> +		goto unprepare_hclk;
>  	}
>  
>  	isc->ispck = isc->isc_clks[ISC_ISPCK].clk;
>  
> +	ret = clk_prepare_enable(isc->ispck);
> +	if (ret) {
> +		dev_err(dev, "failed to enable ispck: %d\n", ret);
> +		goto unprepare_hclk;
> +	}
> +
>  	/* ispck should be greater or equal to hclock */
>  	ret = clk_set_rate(isc->ispck, clk_get_rate(isc->hclock));
>  	if (ret) {
>  		dev_err(dev, "failed to set ispck rate: %d\n", ret);
> -		goto clean_isc_clk;
> +		goto unprepare_clk;
>  	}
>  
>  	ret = v4l2_device_register(dev, &isc->v4l2_dev);
>  	if (ret) {
>  		dev_err(dev, "unable to register v4l2 device.\n");
> -		goto clean_isc_clk;
> +		goto unprepare_clk;
>  	}
>  
>  	ret = isc_parse_dt(dev, isc);
> @@ -1907,8 +1924,6 @@ static int atmel_isc_probe(struct platform_device *pdev)
>  			break;
>  	}
>  
> -	pm_runtime_enable(dev);
> -
>  	return 0;
>  
>  cleanup_subdev:
> @@ -1917,7 +1932,11 @@ static int atmel_isc_probe(struct platform_device *pdev)
>  unregister_v4l2_device:
>  	v4l2_device_unregister(&isc->v4l2_dev);
>  
> -clean_isc_clk:
> +unprepare_clk:
> +	clk_disable_unprepare(isc->ispck);
> +unprepare_hclk:
> +	clk_disable_unprepare(isc->hclock);

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

> +
>  	isc_clk_cleanup(isc);
>  
>  	return ret;
Wenyou Yang Oct. 9, 2017, 5:49 a.m. UTC | #2 
Hi Sakari,

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

On 2017/9/29 5:25, Sakari Ailus wrote:
> Hi Wenyou,
>
> On Thu, Sep 28, 2017 at 04:18:26PM +0800, Wenyou Yang wrote:
>> To meet the relationship, enable the HCLOCK and ispck during the
>> device probe, "isc_pck frequency is less than or equal to isc_ispck,
>> and isc_ispck is greater than or equal to HCLOCK."
>> Meanwhile, call the pm_runtime_enable() in the right place.
>>
>> Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/media/platform/atmel/atmel-isc.c | 31 +++++++++++++++++++++++++------
>>   1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>> index 0b15dc1a3a0b..f092c95587c1 100644
>> --- a/drivers/media/platform/atmel/atmel-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>> @@ -1594,6 +1594,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>   	struct isc_subdev_entity *sd_entity;
>>   	struct video_device *vdev = &isc->video_dev;
>>   	struct vb2_queue *q = &isc->vb2_vidq;
>> +	struct device *dev = isc->dev;
>>   	int ret;
>>   
>>   	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
>> @@ -1677,6 +1678,10 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>   		return ret;
>>   	}
>>   
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +	pm_request_idle(dev);
> Remember that the driver's async complete function could never get called.
>
> What would be the reason to move it here?
The ISC provides the clock for the sensor, namely, it is the clock 
provider for the external sensor.
So it keeps active to make the sensor probe successfully.
Otherwise, the sensor, such as 0v7670 fails to probe due to the failure 
to clk_enable().
>
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1856,25 +1861,37 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>   		return ret;
>>   	}
>>   
>> +	ret = clk_prepare_enable(isc->hclock);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable hclock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>>   	ret = isc_clk_init(isc);
>>   	if (ret) {
>>   		dev_err(dev, "failed to init isc clock: %d\n", ret);
>> -		goto clean_isc_clk;
>> +		goto unprepare_hclk;
>>   	}
>>   
>>   	isc->ispck = isc->isc_clks[ISC_ISPCK].clk;
>>   
>> +	ret = clk_prepare_enable(isc->ispck);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable ispck: %d\n", ret);
>> +		goto unprepare_hclk;
>> +	}
>> +
>>   	/* ispck should be greater or equal to hclock */
>>   	ret = clk_set_rate(isc->ispck, clk_get_rate(isc->hclock));
>>   	if (ret) {
>>   		dev_err(dev, "failed to set ispck rate: %d\n", ret);
>> -		goto clean_isc_clk;
>> +		goto unprepare_clk;
>>   	}
>>   
>>   	ret = v4l2_device_register(dev, &isc->v4l2_dev);
>>   	if (ret) {
>>   		dev_err(dev, "unable to register v4l2 device.\n");
>> -		goto clean_isc_clk;
>> +		goto unprepare_clk;
>>   	}
>>   
>>   	ret = isc_parse_dt(dev, isc);
>> @@ -1907,8 +1924,6 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>   			break;
>>   	}
>>   
>> -	pm_runtime_enable(dev);
>> -
>>   	return 0;
>>   
>>   cleanup_subdev:
>> @@ -1917,7 +1932,11 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>   unregister_v4l2_device:
>>   	v4l2_device_unregister(&isc->v4l2_dev);
>>   
>> -clean_isc_clk:
>> +unprepare_clk:
>> +	clk_disable_unprepare(isc->ispck);
>> +unprepare_hclk:
>> +	clk_disable_unprepare(isc->hclock);
> I think you're missing clk_disable_unprepare() in the driver's remove
> callback.
Will add in next version.
>
>> +
>>   	isc_clk_cleanup(isc);
>>   
>>   	return ret;

Best Regards,
Wenyou Yang
Sakari Ailus Oct. 9, 2017, 7:58 a.m. UTC | #3 
Hi
Wenyou,

On Mon, Oct 09, 2017 at 01:49:44PM +0800, Yang, Wenyou wrote:
> Hi Sakari,
> 
> Sorry for late answer, because I was in vacation last week.
> 
> On 2017/9/29 5:25, Sakari Ailus wrote:
> > Hi Wenyou,
> > 
> > On Thu, Sep 28, 2017 at 04:18:26PM +0800, Wenyou Yang wrote:
> > > To meet the relationship, enable the HCLOCK and ispck during the
> > > device probe, "isc_pck frequency is less than or equal to isc_ispck,
> > > and isc_ispck is greater than or equal to HCLOCK."
> > > Meanwhile, call the pm_runtime_enable() in the right place.
> > > 
> > > Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
> > > ---
> > > 
> > > Changes in v3: None
> > > Changes in v2: None
> > > 
> > >   drivers/media/platform/atmel/atmel-isc.c | 31 +++++++++++++++++++++++++------
> > >   1 file changed, 25 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> > > index 0b15dc1a3a0b..f092c95587c1 100644
> > > --- a/drivers/media/platform/atmel/atmel-isc.c
> > > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > > @@ -1594,6 +1594,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> > >   	struct isc_subdev_entity *sd_entity;
> > >   	struct video_device *vdev = &isc->video_dev;
> > >   	struct vb2_queue *q = &isc->vb2_vidq;
> > > +	struct device *dev = isc->dev;
> > >   	int ret;
> > >   	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
> > > @@ -1677,6 +1678,10 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> > >   		return ret;
> > >   	}
> > > +	pm_runtime_set_active(dev);
> > > +	pm_runtime_enable(dev);
> > > +	pm_request_idle(dev);
> > Remember that the driver's async complete function could never get called.
> > 
> > What would be the reason to move it here?
> The ISC provides the clock for the sensor, namely, it is the clock provider
> for the external sensor.
> So it keeps active to make the sensor probe successfully.
> Otherwise, the sensor, such as 0v7670 fails to probe due to the failure to
> clk_enable().

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

complete callback is not necessarily called at all or could be called
multiple times. Instead, you should probably do pm_runtime_get_sync() when
the clock is enabled and put when it's disabled.
Wenyou Yang Oct. 9, 2017, 10:01 a.m. UTC | #4 
Hi Sakari,

On 2017/10/9 15:58, Sakari Ailus wrote:
> Hi
> Wenyou,
>
> On Mon, Oct 09, 2017 at 01:49:44PM +0800, Yang, Wenyou wrote:
>> Hi Sakari,
>>
>> Sorry for late answer, because I was in vacation last week.
>>
>> On 2017/9/29 5:25, Sakari Ailus wrote:
>>> Hi Wenyou,
>>>
>>> On Thu, Sep 28, 2017 at 04:18:26PM +0800, Wenyou Yang wrote:
>>>> To meet the relationship, enable the HCLOCK and ispck during the
>>>> device probe, "isc_pck frequency is less than or equal to isc_ispck,
>>>> and isc_ispck is greater than or equal to HCLOCK."
>>>> Meanwhile, call the pm_runtime_enable() in the right place.
>>>>
>>>> Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
>>>> ---
>>>>
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>    drivers/media/platform/atmel/atmel-isc.c | 31 +++++++++++++++++++++++++------
>>>>    1 file changed, 25 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>>>> index 0b15dc1a3a0b..f092c95587c1 100644
>>>> --- a/drivers/media/platform/atmel/atmel-isc.c
>>>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>>>> @@ -1594,6 +1594,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>>    	struct isc_subdev_entity *sd_entity;
>>>>    	struct video_device *vdev = &isc->video_dev;
>>>>    	struct vb2_queue *q = &isc->vb2_vidq;
>>>> +	struct device *dev = isc->dev;
>>>>    	int ret;
>>>>    	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
>>>> @@ -1677,6 +1678,10 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>>    		return ret;
>>>>    	}
>>>> +	pm_runtime_set_active(dev);
>>>> +	pm_runtime_enable(dev);
>>>> +	pm_request_idle(dev);
>>> Remember that the driver's async complete function could never get called.
>>>
>>> What would be the reason to move it here?
>> The ISC provides the clock for the sensor, namely, it is the clock provider
>> for the external sensor.
>> So it keeps active to make the sensor probe successfully.
>> Otherwise, the sensor, such as 0v7670 fails to probe due to the failure to
>> clk_enable().
> You'll still need to balance the get and put calls.
>
> complete callback is not necessarily called at all or could be called
> multiple times. Instead, you should probably do pm_runtime_get_sync() when
> the clock is enabled and put when it's disabled.
I will send v4 to update it.

Thank you for your advice.

Best Regards,
Wenyou Yang
diff mbox

Patch

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 0b15dc1a3a0b..f092c95587c1 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1594,6 +1594,7 @@  static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	struct isc_subdev_entity *sd_entity;
 	struct video_device *vdev = &isc->video_dev;
 	struct vb2_queue *q = &isc->vb2_vidq;
+	struct device *dev = isc->dev;
 	int ret;
 
 	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
@@ -1677,6 +1678,10 @@  static int isc_async_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_request_idle(dev);
+
 	return 0;
 }
 
@@ -1856,25 +1861,37 @@  static int atmel_isc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = clk_prepare_enable(isc->hclock);
+	if (ret) {
+		dev_err(dev, "failed to enable hclock: %d\n", ret);
+		return ret;
+	}
+
 	ret = isc_clk_init(isc);
 	if (ret) {
 		dev_err(dev, "failed to init isc clock: %d\n", ret);
-		goto clean_isc_clk;
+		goto unprepare_hclk;
 	}
 
 	isc->ispck = isc->isc_clks[ISC_ISPCK].clk;
 
+	ret = clk_prepare_enable(isc->ispck);
+	if (ret) {
+		dev_err(dev, "failed to enable ispck: %d\n", ret);
+		goto unprepare_hclk;
+	}
+
 	/* ispck should be greater or equal to hclock */
 	ret = clk_set_rate(isc->ispck, clk_get_rate(isc->hclock));
 	if (ret) {
 		dev_err(dev, "failed to set ispck rate: %d\n", ret);
-		goto clean_isc_clk;
+		goto unprepare_clk;
 	}
 
 	ret = v4l2_device_register(dev, &isc->v4l2_dev);
 	if (ret) {
 		dev_err(dev, "unable to register v4l2 device.\n");
-		goto clean_isc_clk;
+		goto unprepare_clk;
 	}
 
 	ret = isc_parse_dt(dev, isc);
@@ -1907,8 +1924,6 @@  static int atmel_isc_probe(struct platform_device *pdev)
 			break;
 	}
 
-	pm_runtime_enable(dev);
-
 	return 0;
 
 cleanup_subdev:
@@ -1917,7 +1932,11 @@  static int atmel_isc_probe(struct platform_device *pdev)
 unregister_v4l2_device:
 	v4l2_device_unregister(&isc->v4l2_dev);
 
-clean_isc_clk:
+unprepare_clk:
+	clk_disable_unprepare(isc->ispck);
+unprepare_hclk:
+	clk_disable_unprepare(isc->hclock);
+
 	isc_clk_cleanup(isc);
 
 	return ret;