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

Sun Apr 16 11:51:06 UTC 2017

Hello Inki,

Inki Dae wrote:
> 2017년 04월 11일 17:17에 Tobias Jakobi 이(가) 쓴 글:
>> Another thing that I noticed. Why wasn't the v2 that ended up in your
>> git ever submitted to the mailing list? Because it should have, in
>> particular to spot these obvious errors.
> 
> Only comment about this.
> 
> This patch cleans up description of struct exynos_drm_clk - removed unnecessary descriptions and adds one missed before.
> I think no problem to update the description without v2 because no code change and description enough.
I think you miss the point here. I checked the mail thread again and you
responded with "More simple and looks better." when I suggested to put
the largest part of your description in front of 'struct
exynos_drm_clk'. I even went so far as to prepare the comment for you.
And then you proceed by ignoring everything and merging some completly
different patch. I find this disrespectful.

I'm quoting your words here (from [1]):
> I'd like to say *maintainer is really not a place for power*, and maintainer would implicitly have a role to encourage in contribution activity of contributer.

If you really mean this, you should also act accordingly. And that does
not mean saying "A" to someone and then doing "B".

> If you want to update the description more then you can post it.
> Ps. I am not a leisurely person to respond to every little thing.
I don't expect you to respond to every little thing. But I expect proper
and honest communication. Also a response delay of four weeks is simply
not acceptable. And I don't think I'm the only one on dri-devel that
thinks that way.

With best wishes,
Tobias

[1] http://www.spinics.net/lists/dri-devel/msg131304.html

> 
> Thanks,
> Inki Dae
> 
>>
>> - Tobias
>>
>>
>> Tobias Jakobi wrote:
>>> Inki Dae wrote:
>>>>
>>>>
>>>> 2017년 04월 10일 19:29에 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.
>>>>
>>>> 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.
>>> I'm not referring to your use of commit-id, but to you totally ignoring
>>> the documentation bits for 'struct exynos_drm_clk'. Please be more
>>> careful when reading my mails.
>>>
>>> - Tobias
>>>
>>>
>>>
>>>> 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 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
>>>>>>
>>>>>
>>>>> --
>>>>> 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
>>>>
>>>
>>> --
>>> 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
>>>
>>
>>
>>
>>