[PATCH] drm/exynos: clean up description of exynos_drm_crtc

[Tobias Jakobi]

Hello Inki,


Inki Dae wrote:
> Hello Tobias,
> 
> 
> 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글:
>> Hello Inki,
>>
>>
>> Inki Dae wrote:
>>> This patch removes unnecessary descriptions on
>>> exynos_drm_crtc structure and adds one description
>>> which specifies what pipe_clk member does.
>>>
>>> pipe_clk support had been added by below patch without any description,
>>> 	 drm/exynos: add support for pipeline clock to the framework
>> I would put the commit id here. That makes it easier to look up the
>> commit your refer to.
>>
>>
>>> Signed-off-by: Inki Dae <inki.dae at samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> index 527bf1d..b0462cc 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> @@ -152,12 +152,10 @@ struct exynos_drm_clk {
>>>   *
>>>   * @base: crtc object.
>>>   * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI.
>>> - * @enabled: if the crtc is enabled or not
>>> - * @event: vblank event that is currently queued for flip
>>> - * @wait_update: wait all pending planes updates to finish
>>> - * @pending_update: number of pending plane updates in this crtc
>>>   * @ops: pointer to callbacks for exynos drm specific functionality
>>>   * @ctx: A pointer to the crtc's implementation specific context
>>> + * @pipe_clk: A pipe clock structure which includes a callback function
>>> +	      for enabling DP clock of FIMD and HDMI PHY clock.
>> This is wrong/inconsistent with the rest of the documentation. pipe_clk
>> is not a struct, but a pointer.
>>
>> I would suggest to follow. Just document that we have a pointer to <add
>> escription> here (compare docu for 'ops' and 'ctx').
>> And then put the later bits ("...callback function for enabling DP
>> clock...") directly to the definition of 'struct exynos_drm_clk' (which
>> is currently lacking any kind of docu).
> 
> Thanks for pointing it out. My mistake. :)
> 
> How about this simply?
> "A pointer to exynos_drm_clk structure for enabling and disabling DP clock of FIMD and HDMI PHY clocks"
Not what I meant. You're describing 'struct exynos_drm_clk', and that
does not belong here. If you describe 'struct exynos_drm_clk', then this
should go in front of the struct's definition.

How abouting something like this in front of the struct's definition::
> /*
>  * Exynos DRM pipeline clock structure.
>  *
>  * @enable: callback to enable/disable the clock.
>  *
>  * Used for proper clock synchronization of components belonging
>  * to the same pipeline. Used e.g. for enabling the DP clock of
>  * the FIMD or the HDMI PHY clock.
>  */
> struct exynos_drm_clk {
> <snip>

For 'pipe_clk' I would just go with this:
> @pipe_clk: A pointet to the crtc's pipeline clock. 

I hope this helps.

- Tobias


> Thanks,
> Inki Dae
> 
>>
>>
>> - Tobias
>>
>>>   */
>>>  struct exynos_drm_crtc {
>>>  	struct drm_crtc			base;
>>>
>>
>>
>>
>>