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

[Srivatsa, Anusha]

>-----Original Message-----
>From: Zanoni, Paulo R
>Sent: Friday, January 19, 2018 10:25 AM
>To: Vivi, Rodrigo <rodrigo.vivi at intel.com>; Srivatsa, Anusha
><anusha.srivatsa at intel.com>
>Cc: Ausmus, James <james.ausmus at intel.com>; Nikula, Jani
><jani.nikula at intel.com>; intel-gfx at 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 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.
>>
>> 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 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