Re: [PATCH 6/8] drm/i915/icp: Add backlight Support for ICP

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx