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

[Inki Dae <inki.dae@samsung.com>]

2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글:
> Inki Dae wrote:
>> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi <tjakobi@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@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.

Many patches in mainline used same form. Please git log | grep "commit-id" -n10.
Sorry but no update and no comment anymore but will use the generic form later.

Thanks,
Inki Dae

> 
> - 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@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@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
>