| Message ID | 20180111180010.24357-7-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > ICP has two backlight controllers - similar to previous platforms like > BXT. > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani) > Reuse BXT code since it is very similar.(Ville) > > v3 (from Paulo): Rebase. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index fa6831f8c004..ad80cca8c110 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) > panel->backlight.set = bxt_set_backlight; > panel->backlight.get = bxt_get_backlight; > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; > - } else if (HAS_PCH_CNP(dev_priv)) { > + } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) { The commit message says reuse BXT, but the code says reuse CNP - which one should it be? > panel->backlight.setup = cnp_setup_backlight; > panel->backlight.enable = cnp_enable_backlight; > panel->backlight.disable = cnp_disable_backlight; > -- > 2.14.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > ICP has two backlight controllers - similar to previous platforms like > > BXT. > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani) > > Reuse BXT code since it is very similar.(Ville) > > > > v3 (from Paulo): Rebase. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index fa6831f8c004..ad80cca8c110 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) > > panel->backlight.set = bxt_set_backlight; > > panel->backlight.get = bxt_get_backlight; > > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; > > - } else if (HAS_PCH_CNP(dev_priv)) { > > + } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) { > > The commit message says reuse BXT, but the code says reuse CNP - which > one should it be? well, CNP is like BXT, but with only one controller. ICP is like BXT, including 2 controllers. I don't know if it makes more sense re-use the cnp or bxt functions But one way or another we have to address these lines from cnp_setup: /* * CNP has the BXT implementation of backlight, but with only * one controller. Future platforms could have multiple controll\ ers * so let's make this extensible and prepared for the future. */ panel->backlight.controller = 0; > > > panel->backlight.setup = cnp_setup_backlight; > > panel->backlight.enable = cnp_enable_backlight; > > panel->backlight.disable = cnp_disable_backlight; > > -- > > 2.14.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2018-01-11 às 15:57 -0800, Rodrigo Vivi escreveu: > On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: > > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > ICP has two backlight controllers - similar to previous platforms > > > like > > > BXT. > > > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani) > > > Reuse BXT code since it is very similar.(Ville) > > > > > > v3 (from Paulo): Rebase. > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > b/drivers/gpu/drm/i915/intel_panel.c > > > index fa6831f8c004..ad80cca8c110 100644 > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct > > > intel_panel *panel) > > > panel->backlight.set = bxt_set_backlight; > > > panel->backlight.get = bxt_get_backlight; > > > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; > > > - } else if (HAS_PCH_CNP(dev_priv)) { > > > + } else if (HAS_PCH_CNP(dev_priv) || > > > HAS_PCH_ICP(dev_priv)) { > > > > The commit message says reuse BXT, but the code says reuse CNP - > > which > > one should it be? > > well, > CNP is like BXT, but with only one controller. > ICP is like BXT, including 2 controllers. > > I don't know if it makes more sense re-use the cnp or bxt functions > > But one way or another we have to address these lines from cnp_setup: > > /* > * CNP has the BXT implementation of backlight, but with only > * one controller. Future platforms could have multiple > controll\ > ers > * so let's make this extensible and prepared for the future. > */ > panel->backlight.controller = 0; My understanding is that we're only using one of the controllers on ICP on purpose, so we can perfectly reuse the CNP code. But I'll let Anusha comment on this. > > > > > > panel->backlight.setup = cnp_setup_backlight; > > > panel->backlight.enable = cnp_enable_backlight; > > > panel->backlight.disable = > > > cnp_disable_backlight; > > > -- > > > 2.14.3 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 19, 2018 at 02:40:41PM -0200, Paulo Zanoni wrote: > Em Qui, 2018-01-11 às 15:57 -0800, Rodrigo Vivi escreveu: > > On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: > > > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > > > ICP has two backlight controllers - similar to previous platforms > > > > like > > > > BXT. > > > > > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani) > > > > Reuse BXT code since it is very similar.(Ville) > > > > > > > > v3 (from Paulo): Rebase. > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > > b/drivers/gpu/drm/i915/intel_panel.c > > > > index fa6831f8c004..ad80cca8c110 100644 > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct > > > > intel_panel *panel) > > > > panel->backlight.set = bxt_set_backlight; > > > > panel->backlight.get = bxt_get_backlight; > > > > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; > > > > - } else if (HAS_PCH_CNP(dev_priv)) { > > > > + } else if (HAS_PCH_CNP(dev_priv) || > > > > HAS_PCH_ICP(dev_priv)) { > > > > > > The commit message says reuse BXT, but the code says reuse CNP - > > > which > > > one should it be? > > > > well, > > CNP is like BXT, but with only one controller. > > ICP is like BXT, including 2 controllers. > > > > I don't know if it makes more sense re-use the cnp or bxt functions > > > > But one way or another we have to address these lines from cnp_setup: > > > > /* > > * CNP has the BXT implementation of backlight, but with only > > * one controller. Future platforms could have multiple > > controll\ > > ers > > * so let's make this extensible and prepared for the future. > > */ > > panel->backlight.controller = 0; > > My understanding is that we're only using one of the controllers on ICP > on purpose, so we can perfectly reuse the CNP code. > > But I'll let Anusha comment on this. This is intentional. Commit message is trying to tell the similarity in backlight support. But we need to reuse CNP code ultimstely. Regards, Anusha > > > > > > > > > panel->backlight.setup = cnp_setup_backlight; > > > > panel->backlight.enable = cnp_enable_backlight; > > > > panel->backlight.disable = > > > > cnp_disable_backlight; > > > > -- > > > > 2.14.3 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 19, 2018 at 05:26:02PM +0000, Anusha Srivatsa wrote: > On Fri, Jan 19, 2018 at 02:40:41PM -0200, Paulo Zanoni wrote: > > Em Qui, 2018-01-11 às 15:57 -0800, Rodrigo Vivi escreveu: > > > On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: > > > > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > > > > > ICP has two backlight controllers - similar to previous platforms > > > > > like > > > > > BXT. > > > > > > > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani) > > > > > Reuse BXT code since it is very similar.(Ville) > > > > > > > > > > v3 (from Paulo): Rebase. > > > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > > > b/drivers/gpu/drm/i915/intel_panel.c > > > > > index fa6831f8c004..ad80cca8c110 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > > @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct > > > > > intel_panel *panel) > > > > > panel->backlight.set = bxt_set_backlight; > > > > > panel->backlight.get = bxt_get_backlight; > > > > > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; > > > > > - } else if (HAS_PCH_CNP(dev_priv)) { > > > > > + } else if (HAS_PCH_CNP(dev_priv) || > > > > > HAS_PCH_ICP(dev_priv)) { > > > > > > > > The commit message says reuse BXT, but the code says reuse CNP - > > > > which > > > > one should it be? > > > > > > well, > > > CNP is like BXT, but with only one controller. > > > ICP is like BXT, including 2 controllers. > > > > > > I don't know if it makes more sense re-use the cnp or bxt functions > > > > > > But one way or another we have to address these lines from cnp_setup: > > > > > > /* > > > * CNP has the BXT implementation of backlight, but with only > > > * one controller. Future platforms could have multiple > > > controll\ > > > ers > > > * so let's make this extensible and prepared for the future. > > > */ > > > panel->backlight.controller = 0; > > > > My understanding is that we're only using one of the controllers on ICP > > on purpose, so we can perfectly reuse the CNP code. > > > > But I'll let Anusha comment on this. > > This is intentional. Commit message is trying to tell the similarity > in backlight support. But we need to reuse CNP code ultimstely. So it is probably better to update this comment here explaining that we know it has more than 1 controller but we intentionally only use the '0' one. But my question now is why? > > Regards, > Anusha > > > > > > > > > > > > panel->backlight.setup = cnp_setup_backlight; > > > > > panel->backlight.enable = cnp_enable_backlight; > > > > > panel->backlight.disable = > > > > > cnp_disable_backlight; > > > > > -- > > > > > 2.14.3 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Anusha Srivatsa
On Fri, Jan 19, 2018 at 09:26:02AM -0800, Anusha Srivatsa wrote: > On Fri, Jan 19, 2018 at 02:40:41PM -0200, Paulo Zanoni wrote: > > Em Qui, 2018-01-11 às 15:57 -0800, Rodrigo Vivi escreveu: > > > On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: > > > > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > > > > > ICP has two backlight controllers - similar to previous platforms > > > > > like > > > > > BXT. > > > > > > > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani) > > > > > Reuse BXT code since it is very similar.(Ville) > > > > > > > > > > v3 (from Paulo): Rebase. > > > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > > > b/drivers/gpu/drm/i915/intel_panel.c > > > > > index fa6831f8c004..ad80cca8c110 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > > @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct > > > > > intel_panel *panel) > > > > > panel->backlight.set = bxt_set_backlight; > > > > > panel->backlight.get = bxt_get_backlight; > > > > > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; > > > > > - } else if (HAS_PCH_CNP(dev_priv)) { > > > > > + } else if (HAS_PCH_CNP(dev_priv) || > > > > > HAS_PCH_ICP(dev_priv)) { > > > > > > > > The commit message says reuse BXT, but the code says reuse CNP - > > > > which > > > > one should it be? > > > > > > well, > > > CNP is like BXT, but with only one controller. > > > ICP is like BXT, including 2 controllers. > > > > > > I don't know if it makes more sense re-use the cnp or bxt functions > > > > > > But one way or another we have to address these lines from cnp_setup: > > > > > > /* > > > * CNP has the BXT implementation of backlight, but with only > > > * one controller. Future platforms could have multiple > > > controll\ > > > ers > > > * so let's make this extensible and prepared for the future. > > > */ > > > panel->backlight.controller = 0; > > > > My understanding is that we're only using one of the controllers on ICP > > on purpose, so we can perfectly reuse the CNP code. > > > > But I'll let Anusha comment on this. > > This is intentional. Commit message is trying to tell the similarity > in backlight support. But we need to reuse CNP code ultimstely. OK - in that case, the commit message needs to get less confusing, as it explicitly states that ICP is similar to BXT, and it explicitly states that v2 changed the commit to reuse BXT code, but the actual code is clearly using CNP code, and doesn't mention CNP (or the justification for using CNP) anywhere. :) Maybe explain that we're using CNP code intentionally, even though it supports two BL controllers, and explain *why* we're ignoring the second BL controller? I'm still not sure why that is myself :) Thanks! -James > > Regards, > Anusha > > > > > > > > > > > > panel->backlight.setup = cnp_setup_backlight; > > > > > panel->backlight.enable = cnp_enable_backlight; > > > > > panel->backlight.disable = > > > > > cnp_disable_backlight; > > > > > -- > > > > > 2.14.3 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Anusha Srivatsa
Em Sex, 2018-01-19 às 09:56 -0800, Rodrigo Vivi escreveu: > On Fri, Jan 19, 2018 at 05:26:02PM +0000, Anusha Srivatsa wrote: > > On Fri, Jan 19, 2018 at 02:40:41PM -0200, Paulo Zanoni wrote: > > > Em Qui, 2018-01-11 às 15:57 -0800, Rodrigo Vivi escreveu: > > > > On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: > > > > > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: > > > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > > > > > > > ICP has two backlight controllers - similar to previous > > > > > > platforms > > > > > > like > > > > > > BXT. > > > > > > > > > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT > > > > > > register.(Jani) > > > > > > Reuse BXT code since it is very similar.(Ville) > > > > > > > > > > > > v3 (from Paulo): Rebase. > > > > > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > > > > b/drivers/gpu/drm/i915/intel_panel.c > > > > > > index fa6831f8c004..ad80cca8c110 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > > > @@ -1865,7 +1865,7 @@ > > > > > > intel_panel_init_backlight_funcs(struct > > > > > > intel_panel *panel) > > > > > > panel->backlight.set = bxt_set_backlight; > > > > > > panel->backlight.get = bxt_get_backlight; > > > > > > panel->backlight.hz_to_pwm = > > > > > > bxt_hz_to_pwm; > > > > > > - } else if (HAS_PCH_CNP(dev_priv)) { > > > > > > + } else if (HAS_PCH_CNP(dev_priv) || > > > > > > HAS_PCH_ICP(dev_priv)) { > > > > > > > > > > The commit message says reuse BXT, but the code says reuse > > > > > CNP - > > > > > which > > > > > one should it be? > > > > > > > > well, > > > > CNP is like BXT, but with only one controller. > > > > ICP is like BXT, including 2 controllers. > > > > > > > > I don't know if it makes more sense re-use the cnp or bxt > > > > functions > > > > > > > > But one way or another we have to address these lines from > > > > cnp_setup: > > > > > > > > /* > > > > * CNP has the BXT implementation of backlight, but > > > > with only > > > > * one controller. Future platforms could have multiple > > > > controll\ > > > > ers > > > > * so let's make this extensible and prepared for the > > > > future. > > > > */ > > > > panel->backlight.controller = 0; > > > > > > My understanding is that we're only using one of the controllers > > > on ICP > > > on purpose, so we can perfectly reuse the CNP code. > > > > > > But I'll let Anusha comment on this. > > > > This is intentional. Commit message is trying to tell the > > similarity > > in backlight support. But we need to reuse CNP code ultimstely. > > So it is probably better to update this comment here > explaining that we know it has more than 1 controller but > we intentionally only use the '0' one. I think the cnp_setup_backlight comment is appropriate as-is because we're still only using one controller for ICP. We should probably only update it if we start using ICP's second controller (because we never tested controller 1 and we don't know if the code is actually 100% prepared for the future). Do you have any specific suggestions on what to write here? > > But my question now is why? Pretty much because there's no use for it for now. > > > > > Regards, > > Anusha > > > > > > > > > > > > > > > panel->backlight.setup = > > > > > > cnp_setup_backlight; > > > > > > panel->backlight.enable = > > > > > > cnp_enable_backlight; > > > > > > panel->backlight.disable = > > > > > > cnp_disable_backlight; > > > > > > -- > > > > > > 2.14.3 > > > > > > > > > > > > _______________________________________________ > > > > > > Intel-gfx mailing list > > > > > > Intel-gfx@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Anusha Srivatsa
>-----Original Message----- >From: Zanoni, Paulo R >Sent: Friday, January 19, 2018 10:25 AM >To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha ><anusha.srivatsa@intel.com> >Cc: Ausmus, James <james.ausmus@intel.com>; Nikula, Jani ><jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 6/8] drm/i915/icp: Add backlight Support for ICP > >Em Sex, 2018-01-19 às 09:56 -0800, Rodrigo Vivi escreveu: >> On Fri, Jan 19, 2018 at 05:26:02PM +0000, Anusha Srivatsa wrote: >> > On Fri, Jan 19, 2018 at 02:40:41PM -0200, Paulo Zanoni wrote: >> > > Em Qui, 2018-01-11 às 15:57 -0800, Rodrigo Vivi escreveu: >> > > > On Thu, Jan 11, 2018 at 09:48:57PM +0000, James Ausmus wrote: >> > > > > On Thu, Jan 11, 2018 at 04:00:08PM -0200, Paulo Zanoni wrote: >> > > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com> >> > > > > > >> > > > > > ICP has two backlight controllers - similar to previous >> > > > > > platforms like BXT. >> > > > > > >> > > > > > v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT >> > > > > > register.(Jani) >> > > > > > Reuse BXT code since it is very similar.(Ville) >> > > > > > >> > > > > > v3 (from Paulo): Rebase. >> > > > > > >> > > > > > Cc: Jani Nikula <jani.nikula@intel.com> >> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> > > > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > > > > > --- >> > > > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- >> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) >> > > > > > >> > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c >> > > > > > b/drivers/gpu/drm/i915/intel_panel.c >> > > > > > index fa6831f8c004..ad80cca8c110 100644 >> > > > > > --- a/drivers/gpu/drm/i915/intel_panel.c >> > > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c >> > > > > > @@ -1865,7 +1865,7 @@ >> > > > > > intel_panel_init_backlight_funcs(struct >> > > > > > intel_panel *panel) >> > > > > > panel->backlight.set = bxt_set_backlight; >> > > > > > panel->backlight.get = bxt_get_backlight; >> > > > > > panel->backlight.hz_to_pwm = bxt_hz_to_pwm; >> > > > > > - } else if (HAS_PCH_CNP(dev_priv)) { >> > > > > > + } else if (HAS_PCH_CNP(dev_priv) || >> > > > > > HAS_PCH_ICP(dev_priv)) { >> > > > > >> > > > > The commit message says reuse BXT, but the code says reuse CNP >> > > > > - which one should it be? >> > > > >> > > > well, >> > > > CNP is like BXT, but with only one controller. >> > > > ICP is like BXT, including 2 controllers. >> > > > >> > > > I don't know if it makes more sense re-use the cnp or bxt >> > > > functions >> > > > >> > > > But one way or another we have to address these lines from >> > > > cnp_setup: >> > > > >> > > > /* >> > > > * CNP has the BXT implementation of backlight, but with >> > > > only >> > > > * one controller. Future platforms could have multiple >> > > > controll\ ers >> > > > * so let's make this extensible and prepared for the >> > > > future. >> > > > */ >> > > > panel->backlight.controller = 0; >> > > >> > > My understanding is that we're only using one of the controllers >> > > on ICP on purpose, so we can perfectly reuse the CNP code. >> > > >> > > But I'll let Anusha comment on this. >> > >> > This is intentional. Commit message is trying to tell the similarity >> > in backlight support. But we need to reuse CNP code ultimstely. >> >> So it is probably better to update this comment here explaining that >> we know it has more than 1 controller but we intentionally only use >> the '0' one. > >I think the cnp_setup_backlight comment is appropriate as-is because we're still >only using one controller for ICP. We should probably only update it if we start >using ICP's second controller (because we never tested controller 1 and we don't >know if the code is actually 100% prepared for the future). Do you have any >specific suggestions on what to write here? Will something like - "We are currently using only one of two backlight controllers on ICP. Hence, reuse the CNP code." Suffice? Anusha >> >> But my question now is why? > >Pretty much because there's no use for it for now. > >> >> > >> > Regards, >> > Anusha >> > > > >> > > > > >> > > > > > panel->backlight.setup = >> > > > > > cnp_setup_backlight; >> > > > > > panel->backlight.enable = cnp_enable_backlight; >> > > > > > panel->backlight.disable = cnp_disable_backlight; >> > > > > > -- >> > > > > > 2.14.3 >> > > > > > >> > > > > > _______________________________________________ >> > > > > > Intel-gfx mailing list >> > > > > > Intel-gfx@lists.freedesktop.org >> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > > > > >> > > > > _______________________________________________ >> > > > > Intel-gfx mailing list >> > > > > Intel-gfx@lists.freedesktop.org >> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > >> > -- >> > Anusha Srivatsa
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index fa6831f8c004..ad80cca8c110 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) panel->backlight.set = bxt_set_backlight; panel->backlight.get = bxt_get_backlight; panel->backlight.hz_to_pwm = bxt_hz_to_pwm; - } else if (HAS_PCH_CNP(dev_priv)) { + } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) { panel->backlight.setup = cnp_setup_backlight; panel->backlight.enable = cnp_enable_backlight; panel->backlight.disable = cnp_disable_backlight;