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

[Anusha Srivatsa <anusha.srivatsa@intel.com>]

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

-- 
Anusha Srivatsa
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx