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

[Inki Dae]

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.

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