| Message ID | 1491441669-5446-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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; >> > > > >
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; >>> >> >> >> >>
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
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 >
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 > > >
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 >
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 >
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 >> > > > >
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 >>> >> >> >> >>
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 >>>> >>> >>> >>> >>> > > > >
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;
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(-)