[5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization."
diff mbox series

Message ID 1575984287-26787-6-git-send-email-claudiu.beznea@microchip.com
State New
Headers show
Series
  • fixes for atmel-hlcdc
Related show

Commit Message

Claudiu Beznea Dec. 10, 2019, 1:24 p.m. UTC
This reverts commit d2c755e66617620b729041c625a6396c81d1231c.
("drm: atmel-hlcdc: enable sys_clk during initalization."). With
commit "drm: atmel-hlcdc: enable clock before configuring timing engine"
there is no need for this patch. Code is also simpler.

Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

Comments

Sam Ravnborg Dec. 10, 2019, 8:34 p.m. UTC | #1
Hi Cladiu

On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote:
> This reverts commit d2c755e66617620b729041c625a6396c81d1231c.
> ("drm: atmel-hlcdc: enable sys_clk during initalization."). With
> commit "drm: atmel-hlcdc: enable clock before configuring timing engine"
> there is no need for this patch. Code is also simpler.
> 
> Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Getting further in the patches tells me you looked at the
patch I referenced in previous mail.
Please squash the two patches together - that would make it
easier to follow what is done.

With the two patches applied sysclk is enabled only in mode_set_nofb()
and atomic_enable(). And disabled in atomic_disable().
This is simpler and we drop the conditionals. Also good.
So the end result looks OK.

	Sam

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 8dc917a1270b..112aa5066cee 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>  	dc->hlcdc = dev_get_drvdata(dev->dev->parent);
>  	dev->dev_private = dc;
>  
> -	if (dc->desc->fixed_clksrc) {
> -		ret = clk_prepare_enable(dc->hlcdc->sys_clk);
> -		if (ret) {
> -			dev_err(dev->dev, "failed to enable sys_clk\n");
> -			goto err_destroy_wq;
> -		}
> -	}
> -
>  	ret = clk_prepare_enable(dc->hlcdc->periph_clk);
>  	if (ret) {
>  		dev_err(dev->dev, "failed to enable periph_clk\n");
> -		goto err_sys_clk_disable;
> +		goto err_destroy_wq;
>  	}
>  
>  	pm_runtime_enable(dev->dev);
> @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>  err_periph_clk_disable:
>  	pm_runtime_disable(dev->dev);
>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
> -err_sys_clk_disable:
> -	if (dc->desc->fixed_clksrc)
> -		clk_disable_unprepare(dc->hlcdc->sys_clk);
>  
>  err_destroy_wq:
>  	destroy_workqueue(dc->wq);
> @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
>  
>  	pm_runtime_disable(dev->dev);
>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
> -	if (dc->desc->fixed_clksrc)
> -		clk_disable_unprepare(dc->hlcdc->sys_clk);
>  	destroy_workqueue(dc->wq);
>  }
>  
> @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>  	regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
>  	regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
> -	if (dc->desc->fixed_clksrc)
> -		clk_disable_unprepare(dc->hlcdc->sys_clk);
>  
>  	return 0;
>  }
> @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>  
> -	if (dc->desc->fixed_clksrc)
> -		clk_prepare_enable(dc->hlcdc->sys_clk);
>  	clk_prepare_enable(dc->hlcdc->periph_clk);
>  	regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>  
> -- 
> 2.7.4
Claudiu Beznea Dec. 11, 2019, 11:55 a.m. UTC | #2
On 10.12.2019 22:34, Sam Ravnborg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Cladiu
> 
> On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote:
>> This reverts commit d2c755e66617620b729041c625a6396c81d1231c.
>> ("drm: atmel-hlcdc: enable sys_clk during initalization."). With
>> commit "drm: atmel-hlcdc: enable clock before configuring timing engine"
>> there is no need for this patch. Code is also simpler.
>>
>> Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Getting further in the patches tells me you looked at the
> patch I referenced in previous mail.
> Please squash the two patches together - that would make it
> easier to follow what is done.

Wouldn't this lead to a patch doing 2 things?
1/ fix the timeout of the timing engine after setting pixel clock which is
   from the beginning of the driver and has nothing to do with patch
   reverted here (but, actually we wouldn't had reach the point of
   introducing the patch reverted here with that fix)
2/ revert a previous functionality as a result of fixing the timeout.

With this in mind would you still want to squash them?

Thank you,
Claudiu Beznea

> 
> With the two patches applied sysclk is enabled only in mode_set_nofb()
> and atomic_enable(). And disabled in atomic_disable().
> This is simpler and we drop the conditionals. Also good.
> So the end result looks OK.
> 
>         Sam
> 
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------
>>  1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> index 8dc917a1270b..112aa5066cee 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>>       dc->hlcdc = dev_get_drvdata(dev->dev->parent);
>>       dev->dev_private = dc;
>>
>> -     if (dc->desc->fixed_clksrc) {
>> -             ret = clk_prepare_enable(dc->hlcdc->sys_clk);
>> -             if (ret) {
>> -                     dev_err(dev->dev, "failed to enable sys_clk\n");
>> -                     goto err_destroy_wq;
>> -             }
>> -     }
>> -
>>       ret = clk_prepare_enable(dc->hlcdc->periph_clk);
>>       if (ret) {
>>               dev_err(dev->dev, "failed to enable periph_clk\n");
>> -             goto err_sys_clk_disable;
>> +             goto err_destroy_wq;
>>       }
>>
>>       pm_runtime_enable(dev->dev);
>> @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>>  err_periph_clk_disable:
>>       pm_runtime_disable(dev->dev);
>>       clk_disable_unprepare(dc->hlcdc->periph_clk);
>> -err_sys_clk_disable:
>> -     if (dc->desc->fixed_clksrc)
>> -             clk_disable_unprepare(dc->hlcdc->sys_clk);
>>
>>  err_destroy_wq:
>>       destroy_workqueue(dc->wq);
>> @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
>>
>>       pm_runtime_disable(dev->dev);
>>       clk_disable_unprepare(dc->hlcdc->periph_clk);
>> -     if (dc->desc->fixed_clksrc)
>> -             clk_disable_unprepare(dc->hlcdc->sys_clk);
>>       destroy_workqueue(dc->wq);
>>  }
>>
>> @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>>       regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
>>       regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
>>       clk_disable_unprepare(dc->hlcdc->periph_clk);
>> -     if (dc->desc->fixed_clksrc)
>> -             clk_disable_unprepare(dc->hlcdc->sys_clk);
>>
>>       return 0;
>>  }
>> @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>>       struct drm_device *drm_dev = dev_get_drvdata(dev);
>>       struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>>
>> -     if (dc->desc->fixed_clksrc)
>> -             clk_prepare_enable(dc->hlcdc->sys_clk);
>>       clk_prepare_enable(dc->hlcdc->periph_clk);
>>       regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>>
>> --
>> 2.7.4
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 8dc917a1270b..112aa5066cee 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -721,18 +721,10 @@  static int atmel_hlcdc_dc_load(struct drm_device *dev)
 	dc->hlcdc = dev_get_drvdata(dev->dev->parent);
 	dev->dev_private = dc;
 
-	if (dc->desc->fixed_clksrc) {
-		ret = clk_prepare_enable(dc->hlcdc->sys_clk);
-		if (ret) {
-			dev_err(dev->dev, "failed to enable sys_clk\n");
-			goto err_destroy_wq;
-		}
-	}
-
 	ret = clk_prepare_enable(dc->hlcdc->periph_clk);
 	if (ret) {
 		dev_err(dev->dev, "failed to enable periph_clk\n");
-		goto err_sys_clk_disable;
+		goto err_destroy_wq;
 	}
 
 	pm_runtime_enable(dev->dev);
@@ -768,9 +760,6 @@  static int atmel_hlcdc_dc_load(struct drm_device *dev)
 err_periph_clk_disable:
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
-err_sys_clk_disable:
-	if (dc->desc->fixed_clksrc)
-		clk_disable_unprepare(dc->hlcdc->sys_clk);
 
 err_destroy_wq:
 	destroy_workqueue(dc->wq);
@@ -795,8 +784,6 @@  static void atmel_hlcdc_dc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
-	if (dc->desc->fixed_clksrc)
-		clk_disable_unprepare(dc->hlcdc->sys_clk);
 	destroy_workqueue(dc->wq);
 }
 
@@ -910,8 +897,6 @@  static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
 	regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
 	regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
-	if (dc->desc->fixed_clksrc)
-		clk_disable_unprepare(dc->hlcdc->sys_clk);
 
 	return 0;
 }
@@ -921,8 +906,6 @@  static int atmel_hlcdc_dc_drm_resume(struct device *dev)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
 
-	if (dc->desc->fixed_clksrc)
-		clk_prepare_enable(dc->hlcdc->sys_clk);
 	clk_prepare_enable(dc->hlcdc->periph_clk);
 	regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);