Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization."

[<Claudiu.Beznea@microchip.com>]

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
>