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

[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 <inki.dae@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.
  */
 struct exynos_drm_crtc {
 	struct drm_crtc			base;
-- 
1.9.1

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 <inki.dae@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).


- Tobias

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

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 <inki.dae@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"

Thanks,
Inki Dae

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

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 <inki.dae@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. 

I hope this helps.

- Tobias


> Thanks,
> Inki Dae
> 
>>
>>
>> - Tobias
>>
>>>   */
>>>  struct exynos_drm_crtc {
>>>  	struct drm_crtc			base;
>>>
>>
>>
>>
>>

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

[Inki Dae]

2017-04-07 20:40 GMT+09:00 Tobias Jakobi <tjakobi@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@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@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 <tjakobi@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@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.

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

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

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 <tjakobi@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@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@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@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@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@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]

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

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 <tjakobi@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@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@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@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@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@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@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월 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@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@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@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@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@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@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@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