[Intel-gfx] [PATCH 6/8] drm/i915/icp: Add backlight Support for ICP

[James Ausmus]

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 at 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 at intel.com>
> > > > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Anusha Srivatsa