[Intel-gfx] [PATCH v8 34/38] drm/i915/icl: Add changes to program DSI panel GPIOs

[Jani Nikula]

On Tue, 30 Oct 2018, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Tue, Oct 30, 2018 at 01:56:40PM +0200, Jani Nikula wrote:
>> From: Madhav Chauhan <madhav.chauhan at intel.com>
>> 
>> For ICELAKE DSI, Display Pins are the only GPIOs
>> that need to be programmed. So DSI driver should have
>> its own implementation to toggle these pins based on
>> GPIO info coming from VBT sequences instead of using
>> platform specific GPIO driver.
>> 
>> Signed-off-by: Madhav Chauhan <madhav.chauhan at intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_vbt.c | 46 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 45 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> index 8177305b9c87..04423248bbd7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> @@ -334,6 +334,48 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
>>  	gpiod_set_value(gpio_desc, value);
>>  }
>>  
>> +static void icl_exec_gpio(struct drm_i915_private *dev_priv,
>> +			  u8 gpio_source, u8 gpio_index, bool value)
>> +{
>> +	u32 val;
>> +
>> +	switch (gpio_index) {
>> +	case ICL_GPIO_DDSP_HPD_A:
>> +		val = I915_READ(SHOTPLUG_CTL_DDI);
>> +		val &= ~ICP_DDIA_HPD_ENABLE;
>> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
>> +		val = I915_READ(SHOTPLUG_CTL_DDI);
>> +		if (value)
>> +			val |= ICP_DDIA_HPD_OP_DRIVE_1;
>> +		else
>> +			val &= ~ICP_DDIA_HPD_OP_DRIVE_1;
>> +
>> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
>
> How badly is this thing going to race with the hotplug irq handler?
>
>> +		break;
>> +	case ICL_GPIO_L_VDDEN_1:
>> +		val = I915_READ(ICP_PP_CONTROL(1));
>> +		if (value)
>> +			val |= PWR_STATE_TARGET;
>> +		else
>> +			val &= ~PWR_STATE_TARGET;
>> +		I915_WRITE(ICP_PP_CONTROL(1), val);
>> +		break;
>> +	case ICL_GPIO_L_BKLTEN_1:
>> +		val = I915_READ(ICP_PP_CONTROL(1));
>> +		if (value)
>> +			val |= BACKLIGHT_ENABLE;
>> +		else
>> +			val &= ~BACKLIGHT_ENABLE;
>> +		I915_WRITE(ICP_PP_CONTROL(1), val);
>
> :( What a horror show. So basically we're trying to pretend the power 
> sequencer state machine doesn't even exist. Is there some bit somewhere
> we can actually use to disable the state machine? If not I think this
> thing needs much more care.

Frankly I didn't look at the patches towards the end of the series all
that much. Just included them all for completeness.

Agreed, looks pretty bad. :(

BR,
Jani.

>
>> +		break;
>> +	default:
>> +		/* TODO: Add support for remaining GPIOs */
>> +		DRM_ERROR("Invalid GPIO no from VBT\n");
>> +		break;
>> +	}
>> +}
>> +
>>  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  {
>>  	struct drm_device *dev = intel_dsi->base.base.dev;
>> @@ -357,7 +399,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  	/* pull up/down */
>>  	value = *data++ & 1;
>>  
>> -	if (IS_VALLEYVIEW(dev_priv))
>> +	if (IS_ICELAKE(dev_priv))
>> +		icl_exec_gpio(dev_priv, gpio_source, gpio_index, value);
>> +	else if (IS_VALLEYVIEW(dev_priv))
>>  		vlv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>>  	else if (IS_CHERRYVIEW(dev_priv))
>>  		chv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>> -- 
>> 2.11.0

-- 
Jani Nikula, Intel Open Source Graphics Center