Re: [PATCH 3/3] drm/i915: DSI: select correct PWM controller to use based on the VBT

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 19-11-2019 16:47, Ville Syrjälä wrote:
> On Tue, Nov 19, 2019 at 04:18:18PM +0100, Hans de Goede wrote:
>> At least Bay Trail (BYT) and Cherry Trail (CHT) devices can use 1 of 2
>> different PWM controllers for controlling the LCD's backlight brightness.
>> Either the one integrated into the PMIC or the one integrated into the
>> SoC (the 1st LPSS PWM controller).
>>
>> So far in the LPSS code on BYT we have skipped registering the LPSS PWM
>> controller "pwm_backlight" lookup entry when a Crystal Cove PMIC is
>> present, assuming that in this case the PMIC PWM controller will be used.
>>
>> On CHT we have been relying on only 1 of the 2 PWM controllers being
>> enabled in the DSDT at the same time; and always registered the lookup.
>>
>> So far this has been working, but the correct way to determine which PWM
>> controller needs to be used is by checking a bit in the VBT table and
>> recently I've learned about 2 different BYT devices:
>> Point of View MOBII TAB-P800W
>> Acer Switch 10 SW5-012
>>
>> Which use a Crystal Cove PMIC, yet the LCD is connected to the SoC/LPSS
>> PWM controller (and the VBT correctly indicates this), so here our old
>> heuristics fail.
>>
>> This commit fixes using the wrong PWM controller on these devices by
>> calling pwm_get() for the right PWM controller based on the
>> VBT dsi.config.pwm_blc bit.
>>
>> Note this is part of a series which contains 2 other patches which renames
>> the PWM lookup for the 1st SoC/LPSS PWM from "pwm_backlight" to
>> "pwm_pmic_backlight" and the PWM lookup for the Crystal Cove PMIC PWM
>> from "pwm_backlight" to "pwm_pmic_backlight".
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_panel.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
>> index bc14e9c0285a..ddcf311d1114 100644
>> --- a/drivers/gpu/drm/i915/display/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
>> @@ -1840,13 +1840,22 @@ static int pwm_setup_backlight(struct intel_connector *connector,
>>   			       enum pipe pipe)
>>   {
>>   	struct drm_device *dev = connector->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>   	struct intel_panel *panel = &connector->panel;
>> +	const char *desc;
>>   	int retval;
>>   
>> -	/* Get the PWM chip for backlight control */
>> -	panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight");
>> +	/* Get the right PWM chip for DSI backlight according to VBT */
>> +	if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) {
>> +		panel->backlight.pwm = pwm_get(dev->dev, "pwm_pmic_backlight");
>> +		desc = "PMIC";
>> +	} else {
>> +		panel->backlight.pwm = pwm_get(dev->dev, "pwm_soc_backlight");
>> +		desc = "SoC";
>> +	}
> 
> Might we want the same thing for the panel enable gpio?


TL;DR: yes but that is for a separate series, which currently only exists in my head.

Longer story:

It looks like on BYT we need to control both VLV_GPIO_NC_10_PANEL1_BKLTEN and
VLV_GPIO_NC_11_PANEL1_BKLTCTL from vlv_dsi.c when the LPSS is used for PWM.
With BKLTCTL working as a panel_enable (needs to be driven high early on
when initializing the panel) and BKLTEN is just a backlight enable/disable
GPIO.

Without this DSI panels will not light-up on BYT when a HDMI monitor is
connected and the GOP chooses to initialize the HDMI rather then the panel,
since then these 2 pins stay low.

On CHT the MIPI power on/off sequences seem to take care of this themselves.

I still want to run some more tests. Currently if I export the 2 gpios in
question in sysfs (since their not claimed yet) and read them they always
read 0. I have the feeling this is caused by the input-buffer not being
enabled on these GPIOs, and that they really are high. So I want to do
a little hack to enable the input buffer and then see if indeed they
are high when the GOP has initialized the panel.

Testing has already shown that driving them high manualy before loading
i915 when the GOP did not init the panel fixes the panel not lighting up.
So I'm pretty sure that this is the case, but I want to verify this before
writing a series for that.

Regards,

Hans



> 
>> +
>>   	if (IS_ERR(panel->backlight.pwm)) {
>> -		DRM_ERROR("Failed to own the pwm chip\n");
>> +		DRM_ERROR("Failed to get the %s PWM chip\n", desc);
>>   		panel->backlight.pwm = NULL;
>>   		return -ENODEV;
>>   	}
>> @@ -1873,6 +1882,7 @@ static int pwm_setup_backlight(struct intel_connector *connector,
>>   				 CRC_PMIC_PWM_PERIOD_NS);
>>   	panel->backlight.enabled = panel->backlight.level != 0;
>>   
>> +	DRM_INFO("Using %s PWM for LCD backlight control\n", desc);
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.23.0
>