[Intel-gfx] [BXT MIPI PATCH v3 11/14] drm/i915/bxt: Modify BXT BLC according to VBT changes

[Shankar, Uma]

>-----Original Message-----
>From: Nikula, Jani
>Sent: Friday, September 18, 2015 7:21 PM
>To: Shankar, Uma; intel-gfx at lists.freedesktop.org
>Cc: Kumar, Shobhit; Deak, Imre; Kamath, Sunil; Kannan, Vandana; Shankar, Uma
>Subject: Re: [BXT MIPI PATCH v3 11/14] drm/i915/bxt: Modify BXT BLC
>according to VBT changes
>
>On Tue, 01 Sep 2015, Uma Shankar <uma.shankar at intel.com> wrote:
>> From: Sunil Kamath <sunil.kamath at intel.com>
>>
>> Latest VBT mentions which set of registers will be used for BLC, as
>> controller number field. Making use of this field in BXT BLC
>> implementation. Also, the registers are used in case control pin
>> indicates display DDI. Adding a check for this.
>> According to Bspec, BLC_PWM_*_2 uses the display utility pin for output.
>> To use backlight 2, enable the utility pin with mode = PWM
>>    v2: Jani's review comments
>>    addressed
>>        - Add a prefix _ to BXT BLC registers definitions.
>>        - Add "bxt only" comment for u8 controller
>>        - Remove control_pin check for DDI controller
>>        - Check for valid controller values
>>        - Set pipe bits in UTIL_PIN_CTL
>>        - Enable/Disable UTIL_PIN_CTL in enable/disable_backlight()
>>        - If BLC 2 is used, read active_low_pwm from UTIL_PIN polarity
>>    Satheesh's review comment addressed
>>        - If UTIL PIN is already enabled, BIOS would have programmed it. No
>>        need to disable and enable again.
>>    v3: Jani's review comments
>>        - add UTIL_PIN_PIPE_MASK and UTIL_PIN_MODE_MASK
>>        - Disable UTIL_PIN if controller 1 is used
>>        - Mask out UTIL_PIN_PIPE_MASK and UTIL_PIN_MODE_MASK before
>enabling
>>        UTIL_PIN
>>        - check valid controller value in intel_bios.c
>>        - add backlight.util_pin_active_low
>>        - disable util pin before enabling
>>    v4: Change for BXT-PO branch:
>>    Stubbed unwanted definition which was existing before
>>    because of DC6 patch.
>>    UTIL_PIN_MODE_PWM     (0x1b << 24)
>>
>> v2: Fixed Jani's review comment.
>>
>> v3: Split the backight PWM frequency programming into separate patch,
>>     in cases BIOS doesn't initializes it.
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
>> Signed-off-by: Sunil Kamath <sunil.kamath at intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    |   28 ++++++++----
>>  drivers/gpu/drm/i915/intel_drv.h   |    2 +
>>  drivers/gpu/drm/i915/intel_panel.c |   84 ++++++++++++++++++++++++++++--
>------
>>  3 files changed, 89 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index e43b053..8407b5c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3512,17 +3512,29 @@ enum skl_disp_power_wells {
>>  #define UTIL_PIN_CTL		0x48400
>>  #define   UTIL_PIN_ENABLE	(1 << 31)
>>
>>  void intel_panel_disable_backlight(struct intel_connector *connector)
>> @@ -988,16 +998,39 @@ static void bxt_enable_backlight(struct
>intel_connector *connector)
>>  	struct drm_device *dev = connector->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>> -	u32 pwm_ctl;
>> +	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> +	u32 pwm_ctl, val;
>> +
>> +	/* To use 2nd set of backlight registers, utility pin has to be
>> +	 * enabled with PWM mode.
>> +	 * The field should only be changed when the utility pin is disabled
>> +	 */
>> +	if (panel->backlight.controller == 1) {
>> +		val = I915_READ(UTIL_PIN_CTL);
>> +		if (val & UTIL_PIN_ENABLE) {
>> +			DRM_DEBUG_KMS("util pin already enabled\n");
>> +			val &= ~UTIL_PIN_ENABLE;
>> +			I915_WRITE(UTIL_PIN_CTL, val);
>> +		}
>> +		/* mask out UTIL_PIN_PIPE and UTIL_PIN_MODE */
>> +		val &= ~(UTIL_PIN_PIPE_MASK | UTIL_PIN_MODE_MASK);
>
>Please start out with 0 val instead of modifying existing state. This is the style
>across backlight enabling, apart from setup which gathers the needed state.
>
>With that fixed,
>
>Reviewed-by: Jani Nikula <jani.nikula at intel.com>
>

Thanks for the comment and spotting it out. Will update this in subsequent patchset.

Regards,
Uma Shankar