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

[Tobias Jakobi]

Inki Dae wrote:
> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi <tjakobi at math.uni-bielefeld.de>:
>> 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.
> 
> More simple and looks better.
In this form (commit a07d468cb2efd347a9e279e4f68661f0f370d10f in
exynos-drm-next), the description is incomplete. Please read my mails again.

- Tobias


> 
> Thanks,
> Inki Dae
> 
>>
>> I hope this helps.
>>
>> - Tobias
>>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>>
>>>>
>>>> - Tobias
>>>>
>>>>>   */
>>>>>  struct exynos_drm_crtc {
>>>>>     struct drm_crtc                 base;
>>>>>
>>>>
>>>>
>>>>
>>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>