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

Inki Dae inki.dae at samsung.com
Wed Apr 19 01:39:48 UTC 2017 


2017년 04월 16일 20:51에 Tobias Jakobi 이(가) 쓴 글:
> 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.
> 

Tobias,

What you want wouldn't be always what someone wants. This patch is really a trivial thing and as I already commented I thought as-is enough.
Even I already picked your suggestion up,
"For 'pipe_clk' I would just go with this:
@pipe_clk: A pointet to the crtc's pipeline clock"

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

And I never ignore you. Instead I commented like below for you,
"If you want to update the description more then you can post it"

Thanks,
Inki Dae

> 
> 
> 
>> 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
>>>>
>>>
>>>
>>>
>>>
> 
> 
> 
>

More information about the dri-devel mailing list