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

[Inki Dae]

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

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

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

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

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

[Inki Dae]

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.

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.

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

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

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

- 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 :
> 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 
 ---
  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 >> 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 {
>> 
>
> 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 majord...@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 

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

[Tobias Jakobi]

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

 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 majord...@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 majord...@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 majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
> --
> To unsubscribe from this list: send 

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

[Inki Dae]

2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글:
> Inki Dae wrote:
>> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi :
>>> 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 
>> ---
>>  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  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 {
 
>>>
>>> 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 majord...@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 majord...@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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tobias Jakobi]

Inki Dae wrote:
> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi :
>> 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 
> ---
>  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 >>> 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 {
>>> 
>>
>> 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.

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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

2017-04-07 20:40 GMT+09:00 Tobias Jakobi :
> 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 
 ---
  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 >> 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 {
>> 
>
> 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tobias Jakobi]

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

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

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

Thanks,
Inki Dae

> 
> 
> - Tobias
> 
>>   */
>>  struct exynos_drm_crtc {
>>  struct drm_crtc base;
>>
> 
> 
> 
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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 
> ---
>  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  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).


- Tobias

>   */
>  struct exynos_drm_crtc {
>   struct drm_crtc base;
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

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

Signed-off-by: Inki Dae 
---
 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.
  */
 struct exynos_drm_crtc {
struct drm_crtc base;
-- 
1.9.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel