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@xxxxxxxxxxx>
>>> ---
>>> 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;
>>>
>>
>>
>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
