Re: [PATCH 2/5] drm/i915/dsi: Move poking of panel-enable GPIO to intel_dsi_vbt.c

[Ville Syrjälä]

On Sun, Dec 15, 2019 at 05:38:07PM +0100, Hans de Goede wrote:
> On some older devices (BYT, CHT) which may use v2 VBT MIPI-sequences,
> we need to manually control the panel enable GPIO as v2 sequences do
> not do this.
> 
> So far we have been carrying the code to do this on BYT/CHT devices
> with a Crystal Cove PMIC in vlv_dsi.c, but as this really is a shortcoming
> of the VBT MIPI-sequences, intel_dsi_vbt.c is a better place for this,
> so move it there.
> 
> This is a preparation patch for adding panel-enable and backlight-enable
> GPIO support for BYT devices where instead of the PMIC the SoC is used
> for backlight control.
> 
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi.h     |  2 +
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 46 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/vlv_dsi.c       | 27 +-----------
>  3 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
> index b15be5814599..de7e51cd3460 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
> @@ -203,6 +203,8 @@ void bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);
>  
>  /* intel_dsi_vbt.c */
>  bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
> +void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi);
> +void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi);
>  void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
>  				 enum mipi_seq seq_id);
>  void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec);
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index f90946c912ee..5352e8c9eca5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -453,8 +453,8 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  		return "(unknown)";
>  }
>  
> -void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
> -				 enum mipi_seq seq_id)
> +static void intel_dsi_vbt_exec(struct intel_dsi *intel_dsi,
> +			       enum mipi_seq seq_id)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>  	const u8 *data;
> @@ -519,6 +519,18 @@ void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
>  	}
>  }
>  
> +void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
> +				 enum mipi_seq seq_id)
> +{
> +	if (seq_id == MIPI_SEQ_POWER_ON && intel_dsi->gpio_panel)
> +		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
> +
> +	intel_dsi_vbt_exec(intel_dsi, seq_id);
> +
> +	if (seq_id == MIPI_SEQ_POWER_OFF && intel_dsi->gpio_panel)
> +		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
> +}
> +
>  void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> @@ -671,3 +683,33 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  
>  	return true;
>  }
> +
> +/*
> + * On some BYT/CHT devs some sequences are incomplete and we need to manually
> + * control some GPIOs.
> + */
> +void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi)
> +{
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
> +
> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> +	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {

Pointless parens around ==

Otherwise seems fine.

Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

> +		intel_dsi->gpio_panel =
> +			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
> +
> +		if (IS_ERR(intel_dsi->gpio_panel)) {
> +			DRM_ERROR("Failed to own gpio for panel control\n");
> +			intel_dsi->gpio_panel = NULL;
> +		}
> +	}
> +}
> +
> +void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
> +{
> +	if (intel_dsi->gpio_panel) {
> +		gpiod_put(intel_dsi->gpio_panel);
> +		intel_dsi->gpio_panel = NULL;
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 6865fd4b5883..178d0fffba5b 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -23,7 +23,6 @@
>   * Author: Jani Nikula <jani.nikula@xxxxxxxxx>
>   */
>  
> -#include <linux/gpio/consumer.h>
>  #include <linux/slab.h>
>  
>  #include <drm/drm_atomic_helper.h>
> @@ -797,9 +796,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	if (!IS_GEMINILAKE(dev_priv))
>  		intel_dsi_prepare(encoder, pipe_config);
>  
> -	/* Power on, try both CRC pmic gpio and VBT */
> -	if (intel_dsi->gpio_panel)
> -		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
>  	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>  	intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay);
>  
> @@ -943,11 +939,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	/* Assert reset */
>  	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>  
> -	/* Power off, try both CRC pmic gpio and VBT */
>  	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
>  	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
> -	if (intel_dsi->gpio_panel)
> -		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
>  
>  	/*
>  	 * FIXME As we do with eDP, just make a note of the time here
> @@ -1539,10 +1532,7 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
>  {
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
>  
> -	/* dispose of the gpios */
> -	if (intel_dsi->gpio_panel)
> -		gpiod_put(intel_dsi->gpio_panel);
> -
> +	intel_dsi_vbt_gpio_cleanup(intel_dsi);
>  	intel_encoder_destroy(encoder);
>  }
>  
> @@ -1920,20 +1910,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
>  
>  	vlv_dphy_param_init(intel_dsi);
>  
> -	/*
> -	 * In case of BYT with CRC PMIC, we need to use GPIO for
> -	 * Panel control.
> -	 */
> -	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> -	    (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC)) {
> -		intel_dsi->gpio_panel =
> -			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
> -
> -		if (IS_ERR(intel_dsi->gpio_panel)) {
> -			DRM_ERROR("Failed to own gpio for panel control\n");
> -			intel_dsi->gpio_panel = NULL;
> -		}
> -	}
> +	intel_dsi_vbt_gpio_init(intel_dsi);
>  
>  	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
>  			   DRM_MODE_CONNECTOR_DSI);
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel