Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
2017년 04월 16일 20:51에 Tobias Jakobi 이(가) 쓴 글: > Hello Inki, > > > > Inki Dae wrote: >> 2017년 04월 11일 17:17에 Tobias Jakobi 이(가) 쓴 글: >>> Another thing that I noticed. Why wasn't the v2 that ended up in your >>> git ever submitted to the mailing list? Because it should have, in >>> particular to spot these obvious errors. >> >> Only comment about this. >> >> This patch cleans up description of struct exynos_drm_clk - removed >> unnecessary descriptions and adds one missed before. >> I think no problem to update the description without v2 because no code >> change and description enough. > I think you miss the point here. I checked the mail thread again and you > responded with "More simple and looks better." when I suggested to put > the largest part of your description in front of 'struct > exynos_drm_clk'. I even went so far as to prepare the comment for you. > And then you proceed by ignoring everything and merging some completly > different patch. I find this disrespectful. > > I'm quoting your words here (from [1]): >> I'd like to say *maintainer is really not a place for power*, and maintainer >> would implicitly have a role to encourage in contribution activity of >> contributer. > Tobias, What you want wouldn't be always what someone wants. This patch is really a trivial thing and as I already commented I thought as-is enough. Even I already picked your suggestion up, "For 'pipe_clk' I would just go with this: @pipe_clk: A pointet to the crtc's pipeline clock" > If you really mean this, you should also act accordingly. And that does > not mean saying "A" to someone and then doing "B". And I never ignore you. Instead I commented like below for you, "If you want to update the description more then you can post it" Thanks, Inki Dae > > > >> If you want to update the description more then you can post it. >> Ps. I am not a leisurely person to respond to every little thing. > I don't expect you to respond to every little thing. But I expect proper > and honest communication. Also a response delay of four weeks is simply > not acceptable. And I don't think I'm the only one on dri-devel that > thinks that way. > > With best wishes, > Tobias > > > [1] http://www.spinics.net/lists/dri-devel/msg131304.html > > >> >> Thanks, >> Inki Dae >> >>> >>> - Tobias >>> >>> >>> Tobias Jakobi wrote: Inki Dae wrote: > > > 2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글: >> Inki Dae wrote: >>> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi >>>: Hello Inki, Inki Dae wrote: > Hello Tobias, > > > 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: >> Hello Inki, >> >> >> Inki Dae wrote: >>> This patch removes unnecessary descriptions on >>> exynos_drm_crtc structure and adds one description >>> which specifies what pipe_clk member does. >>> >>> pipe_clk support had been added by below patch without any >>> description, >>> drm/exynos: add support for pipeline clock to the framework >> I would put the commit id here. That makes it easier to look up the >> commit your refer to. >> >> >>> Signed-off-by: Inki Dae >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> index 527bf1d..b0462cc 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> @@ -152,12 +152,10 @@ struct exynos_drm_clk { >>> * >>> * @base: crtc object. >>> * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. >>> - * @enabled: if the crtc is enabled or not >>> - * @event: vblank event that is currently queued for flip >>> - * @wait_update: wait all pending planes updates to finish >>> - * @pending_update: number of pending plane updates in this crtc >>> * @ops: pointer to callbacks for exynos drm specific functionality >>> * @ctx: A pointer to the crtc's implementation specific context >>> + * @pipe_clk: A pipe clock structure which includes a callback >>> function >>> + for enabling DP clock of FIMD and HDMI PHY clock. >> This is wrong/inconsistent with the rest of the documentation. >> pipe_clk >> is not a struct, but a pointer. >> >> I would suggest to follow. Just document that we have a pointer to >> > escription> here (compare docu for 'ops' and 'ctx'). >> And then put the later bits ("...callback function for enabling DP >> clock...") directly to the definition of 'struct
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Hello Inki, Inki Dae wrote: > 2017년 04월 11일 17:17에 Tobias Jakobi 이(가) 쓴 글: >> Another thing that I noticed. Why wasn't the v2 that ended up in your >> git ever submitted to the mailing list? Because it should have, in >> particular to spot these obvious errors. > > Only comment about this. > > This patch cleans up description of struct exynos_drm_clk - removed > unnecessary descriptions and adds one missed before. > I think no problem to update the description without v2 because no code > change and description enough. I think you miss the point here. I checked the mail thread again and you responded with "More simple and looks better." when I suggested to put the largest part of your description in front of 'struct exynos_drm_clk'. I even went so far as to prepare the comment for you. And then you proceed by ignoring everything and merging some completly different patch. I find this disrespectful. I'm quoting your words here (from [1]): > I'd like to say *maintainer is really not a place for power*, and maintainer > would implicitly have a role to encourage in contribution activity of > contributer. If you really mean this, you should also act accordingly. And that does not mean saying "A" to someone and then doing "B". > If you want to update the description more then you can post it. > Ps. I am not a leisurely person to respond to every little thing. I don't expect you to respond to every little thing. But I expect proper and honest communication. Also a response delay of four weeks is simply not acceptable. And I don't think I'm the only one on dri-devel that thinks that way. With best wishes, Tobias [1] http://www.spinics.net/lists/dri-devel/msg131304.html > > Thanks, > Inki Dae > >> >> - Tobias >> >> >> Tobias Jakobi wrote: >>> Inki Dae wrote: 2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글: > Inki Dae wrote: >> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi: >>> Hello Inki, >>> >>> >>> Inki Dae wrote: Hello Tobias, 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: > Hello Inki, > > > Inki Dae wrote: >> This patch removes unnecessary descriptions on >> exynos_drm_crtc structure and adds one description >> which specifies what pipe_clk member does. >> >> pipe_clk support had been added by below patch without any >> description, >> drm/exynos: add support for pipeline clock to the framework > I would put the commit id here. That makes it easier to look up the > commit your refer to. > > >> Signed-off-by: Inki Dae >> --- >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> index 527bf1d..b0462cc 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> @@ -152,12 +152,10 @@ struct exynos_drm_clk { >> * >> * @base: crtc object. >> * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. >> - * @enabled: if the crtc is enabled or not >> - * @event: vblank event that is currently queued for flip >> - * @wait_update: wait all pending planes updates to finish >> - * @pending_update: number of pending plane updates in this crtc >> * @ops: pointer to callbacks for exynos drm specific functionality >> * @ctx: A pointer to the crtc's implementation specific context >> + * @pipe_clk: A pipe clock structure which includes a callback >> function >> + for enabling DP clock of FIMD and HDMI PHY clock. > This is wrong/inconsistent with the rest of the documentation. > pipe_clk > is not a struct, but a pointer. > > I would suggest to follow. Just document that we have a pointer to > escription> here (compare docu for 'ops' and 'ctx'). > And then put the later bits ("...callback function for enabling DP > clock...") directly to the definition of 'struct exynos_drm_clk' > (which > is currently lacking any kind of docu). Thanks for pointing it out. My mistake. :) How about this simply? "A pointer to exynos_drm_clk structure for enabling and disabling DP clock of FIMD and HDMI PHY clocks" >>> Not what I meant. You're describing 'struct exynos_drm_clk', and that >>> does not belong here. If you describe 'struct exynos_drm_clk', then this >>> should go in front of the struct's definition. >>> >>> How abouting something like this in front of the struct's definition:: /* * Exynos DRM
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
2017년 04월 11일 17:17에 Tobias Jakobi 이(가) 쓴 글: > Another thing that I noticed. Why wasn't the v2 that ended up in your > git ever submitted to the mailing list? Because it should have, in > particular to spot these obvious errors. Only comment about this. This patch cleans up description of struct exynos_drm_clk - removed unnecessary descriptions and adds one missed before. I think no problem to update the description without v2 because no code change and description enough. If you want to update the description more then you can post it. Ps. I am not a leisurely person to respond to every little thing. Thanks, Inki Dae > > - Tobias > > > Tobias Jakobi wrote: >> Inki Dae wrote: >>> >>> >>> 2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글: Inki Dae wrote: > 2017-04-07 20:40 GMT+09:00 Tobias Jakobi: >> Hello Inki, >> >> >> Inki Dae wrote: >>> Hello Tobias, >>> >>> >>> 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: Hello Inki, Inki Dae wrote: > This patch removes unnecessary descriptions on > exynos_drm_crtc structure and adds one description > which specifies what pipe_clk member does. > > pipe_clk support had been added by below patch without any > description, > drm/exynos: add support for pipeline clock to the framework I would put the commit id here. That makes it easier to look up the commit your refer to. > Signed-off-by: Inki Dae > --- > drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h > b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index 527bf1d..b0462cc 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -152,12 +152,10 @@ struct exynos_drm_clk { > * > * @base: crtc object. > * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. > - * @enabled: if the crtc is enabled or not > - * @event: vblank event that is currently queued for flip > - * @wait_update: wait all pending planes updates to finish > - * @pending_update: number of pending plane updates in this crtc > * @ops: pointer to callbacks for exynos drm specific functionality > * @ctx: A pointer to the crtc's implementation specific context > + * @pipe_clk: A pipe clock structure which includes a callback > function > + for enabling DP clock of FIMD and HDMI PHY clock. This is wrong/inconsistent with the rest of the documentation. pipe_clk is not a struct, but a pointer. I would suggest to follow. Just document that we have a pointer to >>> escription> here (compare docu for 'ops' and 'ctx'). And then put the later bits ("...callback function for enabling DP clock...") directly to the definition of 'struct exynos_drm_clk' (which is currently lacking any kind of docu). >>> >>> Thanks for pointing it out. My mistake. :) >>> >>> How about this simply? >>> "A pointer to exynos_drm_clk structure for enabling and disabling DP >>> clock of FIMD and HDMI PHY clocks" >> Not what I meant. You're describing 'struct exynos_drm_clk', and that >> does not belong here. If you describe 'struct exynos_drm_clk', then this >> should go in front of the struct's definition. >> >> How abouting something like this in front of the struct's definition:: >>> /* >>> * Exynos DRM pipeline clock structure. >>> * >>> * @enable: callback to enable/disable the clock. >>> * >>> * Used for proper clock synchronization of components belonging >>> * to the same pipeline. Used e.g. for enabling the DP clock of >>> * the FIMD or the HDMI PHY clock. >>> */ >>> struct exynos_drm_clk { >>> >> >> For 'pipe_clk' I would just go with this: >>> @pipe_clk: A pointet to the crtc's pipeline clock. > > More simple and looks better. In this form (commit a07d468cb2efd347a9e279e4f68661f0f370d10f in exynos-drm-next), the description is incomplete. Please read my mails again. >>> >>> Many patches in mainline used same form. Please git log | grep "commit-id" >>> -n10. >>> Sorry but no update and no comment anymore but will use the generic form >>> later. >> I'm not referring to your use of commit-id, but to you totally ignoring >> the documentation bits for 'struct exynos_drm_clk'. Please be more >> careful when reading my mails. >> >> - Tobias >> >> >> >>> Thanks, >>> Inki Dae >>> - Tobias > > Thanks, > Inki Dae > >> >> I hope this helps. >> >> - Tobias >>
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Another thing that I noticed. Why wasn't the v2 that ended up in your git ever submitted to the mailing list? Because it should have, in particular to spot these obvious errors. - Tobias Tobias Jakobi wrote: > Inki Dae wrote: >> >> >> 2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글: >>> Inki Dae wrote: 2017-04-07 20:40 GMT+09:00 Tobias Jakobi: > Hello Inki, > > > Inki Dae wrote: >> Hello Tobias, >> >> >> 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: >>> Hello Inki, >>> >>> >>> Inki Dae wrote: This patch removes unnecessary descriptions on exynos_drm_crtc structure and adds one description which specifies what pipe_clk member does. pipe_clk support had been added by below patch without any description, drm/exynos: add support for pipeline clock to the framework >>> I would put the commit id here. That makes it easier to look up the >>> commit your refer to. >>> >>> Signed-off-by: Inki Dae --- drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 527bf1d..b0462cc 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -152,12 +152,10 @@ struct exynos_drm_clk { * * @base: crtc object. * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. - * @enabled: if the crtc is enabled or not - * @event: vblank event that is currently queued for flip - * @wait_update: wait all pending planes updates to finish - * @pending_update: number of pending plane updates in this crtc * @ops: pointer to callbacks for exynos drm specific functionality * @ctx: A pointer to the crtc's implementation specific context + * @pipe_clk: A pipe clock structure which includes a callback function + for enabling DP clock of FIMD and HDMI PHY clock. >>> This is wrong/inconsistent with the rest of the documentation. pipe_clk >>> is not a struct, but a pointer. >>> >>> I would suggest to follow. Just document that we have a pointer to >> escription> here (compare docu for 'ops' and 'ctx'). >>> And then put the later bits ("...callback function for enabling DP >>> clock...") directly to the definition of 'struct exynos_drm_clk' (which >>> is currently lacking any kind of docu). >> >> Thanks for pointing it out. My mistake. :) >> >> How about this simply? >> "A pointer to exynos_drm_clk structure for enabling and disabling DP >> clock of FIMD and HDMI PHY clocks" > Not what I meant. You're describing 'struct exynos_drm_clk', and that > does not belong here. If you describe 'struct exynos_drm_clk', then this > should go in front of the struct's definition. > > How abouting something like this in front of the struct's definition:: >> /* >> * Exynos DRM pipeline clock structure. >> * >> * @enable: callback to enable/disable the clock. >> * >> * Used for proper clock synchronization of components belonging >> * to the same pipeline. Used e.g. for enabling the DP clock of >> * the FIMD or the HDMI PHY clock. >> */ >> struct exynos_drm_clk { >> > > For 'pipe_clk' I would just go with this: >> @pipe_clk: A pointet to the crtc's pipeline clock. More simple and looks better. >>> In this form (commit a07d468cb2efd347a9e279e4f68661f0f370d10f in >>> exynos-drm-next), the description is incomplete. Please read my mails again. >> >> Many patches in mainline used same form. Please git log | grep "commit-id" >> -n10. >> Sorry but no update and no comment anymore but will use the generic form >> later. > I'm not referring to your use of commit-id, but to you totally ignoring > the documentation bits for 'struct exynos_drm_clk'. Please be more > careful when reading my mails. > > - Tobias > > > >> Thanks, >> Inki Dae >> >>> >>> - Tobias >>> >>> Thanks, Inki Dae > > I hope this helps. > > - Tobias > > >> Thanks, >> Inki Dae >> >>> >>> >>> - Tobias >>> */ struct exynos_drm_crtc { struct drm_crtc base; >>> >>> >>> >>> > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-samsung-soc" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Inki Dae wrote: > > > 2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글: >> Inki Dae wrote: >>> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi: Hello Inki, Inki Dae wrote: > Hello Tobias, > > > 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: >> Hello Inki, >> >> >> Inki Dae wrote: >>> This patch removes unnecessary descriptions on >>> exynos_drm_crtc structure and adds one description >>> which specifies what pipe_clk member does. >>> >>> pipe_clk support had been added by below patch without any description, >>> drm/exynos: add support for pipeline clock to the framework >> I would put the commit id here. That makes it easier to look up the >> commit your refer to. >> >> >>> Signed-off-by: Inki Dae >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> index 527bf1d..b0462cc 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> @@ -152,12 +152,10 @@ struct exynos_drm_clk { >>> * >>> * @base: crtc object. >>> * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. >>> - * @enabled: if the crtc is enabled or not >>> - * @event: vblank event that is currently queued for flip >>> - * @wait_update: wait all pending planes updates to finish >>> - * @pending_update: number of pending plane updates in this crtc >>> * @ops: pointer to callbacks for exynos drm specific functionality >>> * @ctx: A pointer to the crtc's implementation specific context >>> + * @pipe_clk: A pipe clock structure which includes a callback function >>> + for enabling DP clock of FIMD and HDMI PHY clock. >> This is wrong/inconsistent with the rest of the documentation. pipe_clk >> is not a struct, but a pointer. >> >> I would suggest to follow. Just document that we have a pointer to > escription> here (compare docu for 'ops' and 'ctx'). >> And then put the later bits ("...callback function for enabling DP >> clock...") directly to the definition of 'struct exynos_drm_clk' (which >> is currently lacking any kind of docu). > > Thanks for pointing it out. My mistake. :) > > How about this simply? > "A pointer to exynos_drm_clk structure for enabling and disabling DP > clock of FIMD and HDMI PHY clocks" Not what I meant. You're describing 'struct exynos_drm_clk', and that does not belong here. If you describe 'struct exynos_drm_clk', then this should go in front of the struct's definition. How abouting something like this in front of the struct's definition:: > /* > * Exynos DRM pipeline clock structure. > * > * @enable: callback to enable/disable the clock. > * > * Used for proper clock synchronization of components belonging > * to the same pipeline. Used e.g. for enabling the DP clock of > * the FIMD or the HDMI PHY clock. > */ > struct exynos_drm_clk { > For 'pipe_clk' I would just go with this: > @pipe_clk: A pointet to the crtc's pipeline clock. >>> >>> More simple and looks better. >> In this form (commit a07d468cb2efd347a9e279e4f68661f0f370d10f in >> exynos-drm-next), the description is incomplete. Please read my mails again. > > Many patches in mainline used same form. Please git log | grep "commit-id" > -n10. > Sorry but no update and no comment anymore but will use the generic form > later. I'm not referring to your use of commit-id, but to you totally ignoring the documentation bits for 'struct exynos_drm_clk'. Please be more careful when reading my mails. - Tobias > Thanks, > Inki Dae > >> >> - Tobias >> >> >>> >>> Thanks, >>> Inki Dae >>> I hope this helps. - Tobias > Thanks, > Inki Dae > >> >> >> - Tobias >> >>> */ >>> struct exynos_drm_crtc { >>> struct drm_crtc base; >>> >> >> >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html >>> -- >>> To unsubscribe from this list: send the line "unsubscribe >>> linux-samsung-soc" in >>> the body of a message to majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" >> in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > -- > To unsubscribe from this list: send
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
2017년 04월 10일 19:29에 Tobias Jakobi 이(가) 쓴 글: > Inki Dae wrote: >> 2017-04-07 20:40 GMT+09:00 Tobias Jakobi: >>> Hello Inki, >>> >>> >>> Inki Dae wrote: Hello Tobias, 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: > Hello Inki, > > > Inki Dae wrote: >> This patch removes unnecessary descriptions on >> exynos_drm_crtc structure and adds one description >> which specifies what pipe_clk member does. >> >> pipe_clk support had been added by below patch without any description, >> drm/exynos: add support for pipeline clock to the framework > I would put the commit id here. That makes it easier to look up the > commit your refer to. > > >> Signed-off-by: Inki Dae >> --- >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> index 527bf1d..b0462cc 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> @@ -152,12 +152,10 @@ struct exynos_drm_clk { >> * >> * @base: crtc object. >> * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. >> - * @enabled: if the crtc is enabled or not >> - * @event: vblank event that is currently queued for flip >> - * @wait_update: wait all pending planes updates to finish >> - * @pending_update: number of pending plane updates in this crtc >> * @ops: pointer to callbacks for exynos drm specific functionality >> * @ctx: A pointer to the crtc's implementation specific context >> + * @pipe_clk: A pipe clock structure which includes a callback function >> + for enabling DP clock of FIMD and HDMI PHY clock. > This is wrong/inconsistent with the rest of the documentation. pipe_clk > is not a struct, but a pointer. > > I would suggest to follow. Just document that we have a pointer to escription> here (compare docu for 'ops' and 'ctx'). > And then put the later bits ("...callback function for enabling DP > clock...") directly to the definition of 'struct exynos_drm_clk' (which > is currently lacking any kind of docu). Thanks for pointing it out. My mistake. :) How about this simply? "A pointer to exynos_drm_clk structure for enabling and disabling DP clock of FIMD and HDMI PHY clocks" >>> Not what I meant. You're describing 'struct exynos_drm_clk', and that >>> does not belong here. If you describe 'struct exynos_drm_clk', then this >>> should go in front of the struct's definition. >>> >>> How abouting something like this in front of the struct's definition:: /* * Exynos DRM pipeline clock structure. * * @enable: callback to enable/disable the clock. * * Used for proper clock synchronization of components belonging * to the same pipeline. Used e.g. for enabling the DP clock of * the FIMD or the HDMI PHY clock. */ struct exynos_drm_clk { >>> >>> For 'pipe_clk' I would just go with this: @pipe_clk: A pointet to the crtc's pipeline clock. >> >> More simple and looks better. > In this form (commit a07d468cb2efd347a9e279e4f68661f0f370d10f in > exynos-drm-next), the description is incomplete. Please read my mails again. Many patches in mainline used same form. Please git log | grep "commit-id" -n10. Sorry but no update and no comment anymore but will use the generic form later. Thanks, Inki Dae > > - Tobias > > >> >> Thanks, >> Inki Dae >> >>> >>> I hope this helps. >>> >>> - Tobias >>> >>> Thanks, Inki Dae > > > - Tobias > >> */ >> struct exynos_drm_crtc { >> struct drm_crtc base; >> > > > > >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe >>> linux-samsung-soc" in >>> the body of a message to majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" >> in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Inki Dae wrote: > 2017-04-07 20:40 GMT+09:00 Tobias Jakobi: >> Hello Inki, >> >> >> Inki Dae wrote: >>> Hello Tobias, >>> >>> >>> 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: Hello Inki, Inki Dae wrote: > This patch removes unnecessary descriptions on > exynos_drm_crtc structure and adds one description > which specifies what pipe_clk member does. > > pipe_clk support had been added by below patch without any description, > drm/exynos: add support for pipeline clock to the framework I would put the commit id here. That makes it easier to look up the commit your refer to. > Signed-off-by: Inki Dae > --- > drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h > b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index 527bf1d..b0462cc 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -152,12 +152,10 @@ struct exynos_drm_clk { > * > * @base: crtc object. > * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. > - * @enabled: if the crtc is enabled or not > - * @event: vblank event that is currently queued for flip > - * @wait_update: wait all pending planes updates to finish > - * @pending_update: number of pending plane updates in this crtc > * @ops: pointer to callbacks for exynos drm specific functionality > * @ctx: A pointer to the crtc's implementation specific context > + * @pipe_clk: A pipe clock structure which includes a callback function > + for enabling DP clock of FIMD and HDMI PHY clock. This is wrong/inconsistent with the rest of the documentation. pipe_clk is not a struct, but a pointer. I would suggest to follow. Just document that we have a pointer to >>> escription> here (compare docu for 'ops' and 'ctx'). And then put the later bits ("...callback function for enabling DP clock...") directly to the definition of 'struct exynos_drm_clk' (which is currently lacking any kind of docu). >>> >>> Thanks for pointing it out. My mistake. :) >>> >>> How about this simply? >>> "A pointer to exynos_drm_clk structure for enabling and disabling DP clock >>> of FIMD and HDMI PHY clocks" >> Not what I meant. You're describing 'struct exynos_drm_clk', and that >> does not belong here. If you describe 'struct exynos_drm_clk', then this >> should go in front of the struct's definition. >> >> How abouting something like this in front of the struct's definition:: >>> /* >>> * Exynos DRM pipeline clock structure. >>> * >>> * @enable: callback to enable/disable the clock. >>> * >>> * Used for proper clock synchronization of components belonging >>> * to the same pipeline. Used e.g. for enabling the DP clock of >>> * the FIMD or the HDMI PHY clock. >>> */ >>> struct exynos_drm_clk { >>> >> >> For 'pipe_clk' I would just go with this: >>> @pipe_clk: A pointet to the crtc's pipeline clock. > > More simple and looks better. In this form (commit a07d468cb2efd347a9e279e4f68661f0f370d10f in exynos-drm-next), the description is incomplete. Please read my mails again. - Tobias > > Thanks, > Inki Dae > >> >> I hope this helps. >> >> - Tobias >> >> >>> Thanks, >>> Inki Dae >>> - Tobias > */ > struct exynos_drm_crtc { > struct drm_crtc base; > >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" >> in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
2017-04-07 20:40 GMT+09:00 Tobias Jakobi: > Hello Inki, > > > Inki Dae wrote: >> Hello Tobias, >> >> >> 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: >>> Hello Inki, >>> >>> >>> Inki Dae wrote: This patch removes unnecessary descriptions on exynos_drm_crtc structure and adds one description which specifies what pipe_clk member does. pipe_clk support had been added by below patch without any description, drm/exynos: add support for pipeline clock to the framework >>> I would put the commit id here. That makes it easier to look up the >>> commit your refer to. >>> >>> Signed-off-by: Inki Dae --- drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 527bf1d..b0462cc 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -152,12 +152,10 @@ struct exynos_drm_clk { * * @base: crtc object. * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. - * @enabled: if the crtc is enabled or not - * @event: vblank event that is currently queued for flip - * @wait_update: wait all pending planes updates to finish - * @pending_update: number of pending plane updates in this crtc * @ops: pointer to callbacks for exynos drm specific functionality * @ctx: A pointer to the crtc's implementation specific context + * @pipe_clk: A pipe clock structure which includes a callback function + for enabling DP clock of FIMD and HDMI PHY clock. >>> This is wrong/inconsistent with the rest of the documentation. pipe_clk >>> is not a struct, but a pointer. >>> >>> I would suggest to follow. Just document that we have a pointer to >> escription> here (compare docu for 'ops' and 'ctx'). >>> And then put the later bits ("...callback function for enabling DP >>> clock...") directly to the definition of 'struct exynos_drm_clk' (which >>> is currently lacking any kind of docu). >> >> Thanks for pointing it out. My mistake. :) >> >> How about this simply? >> "A pointer to exynos_drm_clk structure for enabling and disabling DP clock >> of FIMD and HDMI PHY clocks" > Not what I meant. You're describing 'struct exynos_drm_clk', and that > does not belong here. If you describe 'struct exynos_drm_clk', then this > should go in front of the struct's definition. > > How abouting something like this in front of the struct's definition:: >> /* >> * Exynos DRM pipeline clock structure. >> * >> * @enable: callback to enable/disable the clock. >> * >> * Used for proper clock synchronization of components belonging >> * to the same pipeline. Used e.g. for enabling the DP clock of >> * the FIMD or the HDMI PHY clock. >> */ >> struct exynos_drm_clk { >> > > For 'pipe_clk' I would just go with this: >> @pipe_clk: A pointet to the crtc's pipeline clock. More simple and looks better. Thanks, Inki Dae > > I hope this helps. > > - Tobias > > >> Thanks, >> Inki Dae >> >>> >>> >>> - Tobias >>> */ struct exynos_drm_crtc { struct drm_crtc base; >>> >>> >>> >>> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Hello Inki, Inki Dae wrote: > Hello Tobias, > > > 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: >> Hello Inki, >> >> >> Inki Dae wrote: >>> This patch removes unnecessary descriptions on >>> exynos_drm_crtc structure and adds one description >>> which specifies what pipe_clk member does. >>> >>> pipe_clk support had been added by below patch without any description, >>> drm/exynos: add support for pipeline clock to the framework >> I would put the commit id here. That makes it easier to look up the >> commit your refer to. >> >> >>> Signed-off-by: Inki Dae>>> --- >>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> index 527bf1d..b0462cc 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> @@ -152,12 +152,10 @@ struct exynos_drm_clk { >>> * >>> * @base: crtc object. >>> * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. >>> - * @enabled: if the crtc is enabled or not >>> - * @event: vblank event that is currently queued for flip >>> - * @wait_update: wait all pending planes updates to finish >>> - * @pending_update: number of pending plane updates in this crtc >>> * @ops: pointer to callbacks for exynos drm specific functionality >>> * @ctx: A pointer to the crtc's implementation specific context >>> + * @pipe_clk: A pipe clock structure which includes a callback function >>> + for enabling DP clock of FIMD and HDMI PHY clock. >> This is wrong/inconsistent with the rest of the documentation. pipe_clk >> is not a struct, but a pointer. >> >> I would suggest to follow. Just document that we have a pointer to > escription> here (compare docu for 'ops' and 'ctx'). >> And then put the later bits ("...callback function for enabling DP >> clock...") directly to the definition of 'struct exynos_drm_clk' (which >> is currently lacking any kind of docu). > > Thanks for pointing it out. My mistake. :) > > How about this simply? > "A pointer to exynos_drm_clk structure for enabling and disabling DP clock of > FIMD and HDMI PHY clocks" Not what I meant. You're describing 'struct exynos_drm_clk', and that does not belong here. If you describe 'struct exynos_drm_clk', then this should go in front of the struct's definition. How abouting something like this in front of the struct's definition:: > /* > * Exynos DRM pipeline clock structure. > * > * @enable: callback to enable/disable the clock. > * > * Used for proper clock synchronization of components belonging > * to the same pipeline. Used e.g. for enabling the DP clock of > * the FIMD or the HDMI PHY clock. > */ > struct exynos_drm_clk { > For 'pipe_clk' I would just go with this: > @pipe_clk: A pointet to the crtc's pipeline clock. I hope this helps. - Tobias > Thanks, > Inki Dae > >> >> >> - Tobias >> >>> */ >>> struct exynos_drm_crtc { >>> struct drm_crtc base; >>> >> >> >> >> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Hello Tobias, 2017년 04월 07일 02:10에 Tobias Jakobi 이(가) 쓴 글: > Hello Inki, > > > Inki Dae wrote: >> This patch removes unnecessary descriptions on >> exynos_drm_crtc structure and adds one description >> which specifies what pipe_clk member does. >> >> pipe_clk support had been added by below patch without any description, >> drm/exynos: add support for pipeline clock to the framework > I would put the commit id here. That makes it easier to look up the > commit your refer to. > > >> Signed-off-by: Inki Dae>> --- >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> index 527bf1d..b0462cc 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> @@ -152,12 +152,10 @@ struct exynos_drm_clk { >> * >> * @base: crtc object. >> * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. >> - * @enabled: if the crtc is enabled or not >> - * @event: vblank event that is currently queued for flip >> - * @wait_update: wait all pending planes updates to finish >> - * @pending_update: number of pending plane updates in this crtc >> * @ops: pointer to callbacks for exynos drm specific functionality >> * @ctx: A pointer to the crtc's implementation specific context >> + * @pipe_clk: A pipe clock structure which includes a callback function >> + for enabling DP clock of FIMD and HDMI PHY clock. > This is wrong/inconsistent with the rest of the documentation. pipe_clk > is not a struct, but a pointer. > > I would suggest to follow. Just document that we have a pointer to escription> here (compare docu for 'ops' and 'ctx'). > And then put the later bits ("...callback function for enabling DP > clock...") directly to the definition of 'struct exynos_drm_clk' (which > is currently lacking any kind of docu). Thanks for pointing it out. My mistake. :) How about this simply? "A pointer to exynos_drm_clk structure for enabling and disabling DP clock of FIMD and HDMI PHY clocks" Thanks, Inki Dae > > > - Tobias > >> */ >> struct exynos_drm_crtc { >> struct drm_crtc base; >> > > > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: clean up description of exynos_drm_crtc
Hello Inki, Inki Dae wrote: > This patch removes unnecessary descriptions on > exynos_drm_crtc structure and adds one description > which specifies what pipe_clk member does. > > pipe_clk support had been added by below patch without any description, >drm/exynos: add support for pipeline clock to the framework I would put the commit id here. That makes it easier to look up the commit your refer to. > Signed-off-by: Inki Dae> --- > drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h > b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index 527bf1d..b0462cc 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -152,12 +152,10 @@ struct exynos_drm_clk { > * > * @base: crtc object. > * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. > - * @enabled: if the crtc is enabled or not > - * @event: vblank event that is currently queued for flip > - * @wait_update: wait all pending planes updates to finish > - * @pending_update: number of pending plane updates in this crtc > * @ops: pointer to callbacks for exynos drm specific functionality > * @ctx: A pointer to the crtc's implementation specific context > + * @pipe_clk: A pipe clock structure which includes a callback function > + for enabling DP clock of FIMD and HDMI PHY clock. This is wrong/inconsistent with the rest of the documentation. pipe_clk is not a struct, but a pointer. I would suggest to follow. Just document that we have a pointer to here (compare docu for 'ops' and 'ctx'). And then put the later bits ("...callback function for enabling DP clock...") directly to the definition of 'struct exynos_drm_clk' (which is currently lacking any kind of docu). - Tobias > */ > struct exynos_drm_crtc { > struct drm_crtc base; > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/exynos: clean up description of exynos_drm_crtc
This patch removes unnecessary descriptions on exynos_drm_crtc structure and adds one description which specifies what pipe_clk member does. pipe_clk support had been added by below patch without any description, drm/exynos: add support for pipeline clock to the framework Signed-off-by: Inki Dae--- drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 527bf1d..b0462cc 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -152,12 +152,10 @@ struct exynos_drm_clk { * * @base: crtc object. * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI. - * @enabled: if the crtc is enabled or not - * @event: vblank event that is currently queued for flip - * @wait_update: wait all pending planes updates to finish - * @pending_update: number of pending plane updates in this crtc * @ops: pointer to callbacks for exynos drm specific functionality * @ctx: A pointer to the crtc's implementation specific context + * @pipe_clk: A pipe clock structure which includes a callback function + for enabling DP clock of FIMD and HDMI PHY clock. */ struct exynos_drm_crtc { struct drm_crtc base; -- 1.9.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel