[PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Hans de Goede]

Hi All,

This is a new (completely rewritten) version of my patches to make the
i915 code control the SoC panel- and backlight-enable GPIOs on Bay Trail
devices when the VBT indicates that the SoC should be used for backlight
control. This fixes the panel not lighting up on various devices when
booted with a HDMI monitor connected, in which case the firmware skips
initializing the panel as it inits the HDMI instead.

This series has been tested on; and fixes this issue on; the following models:

Peaq C1010
Point of View MOBII TAB-P800W
Point of View MOBII TAB-P1005W
Terra Pad 1061
Thundersoft TST178
Yours Y8W81

Linus, this series starts with the already discussed pinctrl change to
export the function to unregister a pinctrl-map. We can either merge this
through drm-intel, or you could pick it up and then provide an immutable
branch with it for merging into drm-intel-next. Which option do you prefer?

Lee, I know you don't like this, but unfortunately this series introcudes
some (other) changes to drivers/mfd/intel_soc_pmic_core.c. The GPIO subsys
allows only one mapping-table per consumer, so in hindsight adding the code
which adds the mapping for the PMIC panel-enable pin to the PMIC mfd driver
was a mistake, as the PMIC code is a provider where as mapping-tables are
per consumer. The 4th patch fixes this by moving the mapping-table to the
i915 code, so that we can also add mappings for some of the pins on the SoC
itself. Since this whole series makes change to the i915 code I plan to
merge this mfd change to the drm-intel tree.

Regards,

Hans

[PATCH 1/5] pinctrl: Export pinctrl_unregister_mappings

[Hans de Goede]

Rename pinctrl_unregister_map to pinctrl_unregister_mappings so that
its naming matches its pinctrl_register_mappings counter-part and
export it.

The purpose of this patch is to allow non-dt platforms to register
pinctrl mappings from code which is build as a module, which requires
being able to unregister the mapping when the module is unloaded to
avoid dangling pointers; and to avoid registering the mapping multiple
times, when the module gets reloaded later.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/core.c          | 11 ++++++++++-
 drivers/pinctrl/core.h          |  3 ++-
 drivers/pinctrl/devicetree.c    |  2 +-
 include/linux/pinctrl/machine.h |  5 +++++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2bbd8ee93507..c98f8cc9c7ca 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1431,6 +1431,7 @@ int pinctrl_register_map(const struct pinctrl_map *maps, unsigned num_maps,
 		return -ENOMEM;
 
 	maps_node->num_maps = num_maps;
+	maps_node->dup = dup;
 	if (dup) {
 		maps_node->maps = kmemdup(maps, sizeof(*maps) * num_maps,
 					  GFP_KERNEL);
@@ -1463,7 +1464,12 @@ int pinctrl_register_mappings(const struct pinctrl_map *maps,
 }
 EXPORT_SYMBOL_GPL(pinctrl_register_mappings);
 
-void pinctrl_unregister_map(const struct pinctrl_map *map)
+/**
+ * pinctrl_unregister_mappings() - unregister a set of pin controller mappings
+ * @maps: the pincontrol mappings table passed to pinctrl_register_mappings()
+ *	when registering the mappings.
+ */
+void pinctrl_unregister_mappings(const struct pinctrl_map *map)
 {
 	struct pinctrl_maps *maps_node;
 
@@ -1471,6 +1477,8 @@ void pinctrl_unregister_map(const struct pinctrl_map *map)
 	list_for_each_entry(maps_node, &pinctrl_maps, node) {
 		if (maps_node->maps == map) {
 			list_del(&maps_node->node);
+			if (maps_node->dup)
+				kfree(maps_node->maps);
 			kfree(maps_node);
 			mutex_unlock(&pinctrl_maps_mutex);
 			return;
@@ -1478,6 +1486,7 @@ void pinctrl_unregister_map(const struct pinctrl_map *map)
 	}
 	mutex_unlock(&pinctrl_maps_mutex);
 }
+EXPORT_SYMBOL_GPL(pinctrl_unregister_mappings);
 
 /**
  * pinctrl_force_sleep() - turn a given controller device into sleep state
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 7f34167a0405..cba38ef13b1d 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -175,11 +175,13 @@ struct pin_desc {
  * @node: mapping table list node
  * @maps: array of mapping table entries
  * @num_maps: the number of entries in @maps
+ * @dup: has the mapping table been memdup-ed by us?
  */
 struct pinctrl_maps {
 	struct list_head node;
 	const struct pinctrl_map *maps;
 	unsigned num_maps;
+	bool dup;
 };
 
 #ifdef CONFIG_GENERIC_PINCTRL_GROUPS
@@ -238,7 +240,6 @@ pinctrl_find_gpio_range_from_pin_nolock(struct pinctrl_dev *pctldev,
 
 int pinctrl_register_map(const struct pinctrl_map *maps, unsigned num_maps,
 			 bool dup);
-void pinctrl_unregister_map(const struct pinctrl_map *map);
 
 extern int pinctrl_force_sleep(struct pinctrl_dev *pctldev);
 extern int pinctrl_force_default(struct pinctrl_dev *pctldev);
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index 674920daac26..a1925962a3e2 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -51,7 +51,7 @@ void pinctrl_dt_free_maps(struct pinctrl *p)
 	struct pinctrl_dt_map *dt_map, *n1;
 
 	list_for_each_entry_safe(dt_map, n1, &p->dt_maps, node) {
-		pinctrl_unregister_map(dt_map->map);
+		pinctrl_unregister_mappings(dt_map->map);
 		list_del(&dt_map->node);
 		dt_free_map(dt_map->pctldev, dt_map->map,
 			    dt_map->num_maps);
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index ddd1b2773431..e987dc9fd2af 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -153,6 +153,7 @@ struct pinctrl_map {
 
 extern int pinctrl_register_mappings(const struct pinctrl_map *map,
 				unsigned num_maps);
+extern void pinctrl_unregister_mappings(const struct pinctrl_map *map);
 extern void pinctrl_provide_dummies(void);
 #else
 
@@ -162,6 +163,10 @@ static inline int pinctrl_register_mappings(const struct pinctrl_map *map,
 	return 0;
 }
 
+static inline void pinctrl_unregister_mappings(const struct pinctrl_map *map)
+{
+}
+
 static inline void pinctrl_provide_dummies(void)
 {
 }
-- 
2.23.0

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

[Hans de Goede]

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@redhat.com>
---
 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)) {
+		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@intel.com>
  */
 
-#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

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

[Linus Walleij]

On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com>

The kernel looks prettier after than before and it seems correct so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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@redhat.com>
> ---
>  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@linux.intel.com>

> +		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@intel.com>
>   */
>  
> -#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

[PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off

[Hans de Goede]

When the LCD has not been turned on by the firmware/GOP, because e.g. the
device was booted with an external monitor connected over HDMI, we should
not turn on the panel-enable GPIO when we request it.

Turning on the panel-enable GPIO when we request it, means we turn it on
too early in the init-sequence, which causes some panels to not correctly
light up.

This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init()
and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly.

This fixes the panel not lighting up on a Thundersoft TST168 tablet when
booted with an external monitor connected over HDMI.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dsi.h     | 2 +-
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 7 +++----
 drivers/gpu/drm/i915/display/vlv_dsi.c       | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
index de7e51cd3460..675771ea91aa 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi.h
+++ b/drivers/gpu/drm/i915/display/intel_dsi.h
@@ -203,7 +203,7 @@ 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_init(struct intel_dsi *intel_dsi, bool panel_is_on);
 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);
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 5352e8c9eca5..027970348b22 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -688,17 +688,16 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
  * 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)
+void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
 {
 	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;
+	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
 
 	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
 	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
-		intel_dsi->gpio_panel =
-			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
-
+		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
 		if (IS_ERR(intel_dsi->gpio_panel)) {
 			DRM_ERROR("Failed to own gpio for panel control\n");
 			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 178d0fffba5b..e86e4a11e199 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1910,7 +1910,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
 
 	vlv_dphy_param_init(intel_dsi);
 
-	intel_dsi_vbt_gpio_init(intel_dsi);
+	intel_dsi_vbt_gpio_init(intel_dsi, current_mode != NULL);
 
 	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
 			   DRM_MODE_CONNECTOR_DSI);
-- 
2.23.0

Re: [PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off

[Linus Walleij]

On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

> When the LCD has not been turned on by the firmware/GOP, because e.g. the
> device was booted with an external monitor connected over HDMI, we should
> not turn on the panel-enable GPIO when we request it.
>
> Turning on the panel-enable GPIO when we request it, means we turn it on
> too early in the init-sequence, which causes some panels to not correctly
> light up.
>
> This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init()
> and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly.
>
> This fixes the panel not lighting up on a Thundersoft TST168 tablet when
> booted with an external monitor connected over HDMI.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off

[Ville Syrjälä]

On Sun, Dec 15, 2019 at 05:38:08PM +0100, Hans de Goede wrote:
> When the LCD has not been turned on by the firmware/GOP, because e.g. the
> device was booted with an external monitor connected over HDMI, we should
> not turn on the panel-enable GPIO when we request it.
> 
> Turning on the panel-enable GPIO when we request it, means we turn it on
> too early in the init-sequence, which causes some panels to not correctly
> light up.
> 
> This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init()
> and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly.
> 
> This fixes the panel not lighting up on a Thundersoft TST168 tablet when
> booted with an external monitor connected over HDMI.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi.h     | 2 +-
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 7 +++----
>  drivers/gpu/drm/i915/display/vlv_dsi.c       | 2 +-
>  3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
> index de7e51cd3460..675771ea91aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
> @@ -203,7 +203,7 @@ 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_init(struct intel_dsi *intel_dsi, bool panel_is_on);
>  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);
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index 5352e8c9eca5..027970348b22 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -688,17 +688,16 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>   * 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)
> +void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  {
>  	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;
> +	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;

Can't we just tell it not to change the current setting?

>  
>  	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>  	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
> -		intel_dsi->gpio_panel =
> -			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
> -
> +		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>  		if (IS_ERR(intel_dsi->gpio_panel)) {
>  			DRM_ERROR("Failed to own gpio for panel control\n");
>  			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 178d0fffba5b..e86e4a11e199 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -1910,7 +1910,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
>  
>  	vlv_dphy_param_init(intel_dsi);
>  
> -	intel_dsi_vbt_gpio_init(intel_dsi);
> +	intel_dsi_vbt_gpio_init(intel_dsi, current_mode != NULL);
>  
>  	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
>  			   DRM_MODE_CONNECTOR_DSI);
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off

[Hans de Goede]

Hi,

Thank you for the reviews.

On 16-12-2019 14:45, Ville Syrjälä wrote:
> On Sun, Dec 15, 2019 at 05:38:08PM +0100, Hans de Goede wrote:
>> When the LCD has not been turned on by the firmware/GOP, because e.g. the
>> device was booted with an external monitor connected over HDMI, we should
>> not turn on the panel-enable GPIO when we request it.
>>
>> Turning on the panel-enable GPIO when we request it, means we turn it on
>> too early in the init-sequence, which causes some panels to not correctly
>> light up.
>>
>> This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init()
>> and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly.
>>
>> This fixes the panel not lighting up on a Thundersoft TST168 tablet when
>> booted with an external monitor connected over HDMI.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsi.h     | 2 +-
>>   drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 7 +++----
>>   drivers/gpu/drm/i915/display/vlv_dsi.c       | 2 +-
>>   3 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
>> index de7e51cd3460..675771ea91aa 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
>> @@ -203,7 +203,7 @@ 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_init(struct intel_dsi *intel_dsi, bool panel_is_on);
>>   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);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> index 5352e8c9eca5..027970348b22 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -688,17 +688,16 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>    * 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)
>> +void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   {
>>   	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;
>> +	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> 
> Can't we just tell it not to change the current setting?

We could use GPIOD_ASIS for that, but with the SoC pins (when the PMIC is
not used for backlight control) things get a bit muddy, I've seen several
instances of this message from drivers/pinctrl/intel/pinctrl-baytrail.c
trigger when the GOP did not init the panel:

dev_warn(vg->dev, FW_BUG "pin %u forcibly re-configured as GPIO\n", offset);

And in that case with GPIOD_ASIS I have no idea which we initially get,
so this approach, where we clearly define which initial value we want,
seems better.

Regards,

Hans

p.s.

The intel-gfx CI seems to seriously dislike my patches lately, almost
always failing them; and usually on what at least seem to be unrelated
test-cases. Any advice on how to deal with this?




> 
>>   
>>   	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>   	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
>> -		intel_dsi->gpio_panel =
>> -			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
>> -
>> +		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>>   		if (IS_ERR(intel_dsi->gpio_panel)) {
>>   			DRM_ERROR("Failed to own gpio for panel control\n");
>>   			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 178d0fffba5b..e86e4a11e199 100644
>> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
>> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
>> @@ -1910,7 +1910,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
>>   
>>   	vlv_dphy_param_init(intel_dsi);
>>   
>> -	intel_dsi_vbt_gpio_init(intel_dsi);
>> +	intel_dsi_vbt_gpio_init(intel_dsi, current_mode != NULL);
>>   
>>   	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
>>   			   DRM_MODE_CONNECTOR_DSI);
>> -- 
>> 2.23.0
> 

Re: [PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off

[Ville Syrjälä]

On Mon, Dec 16, 2019 at 02:51:54PM +0100, Hans de Goede wrote:
> Hi,
> 
> Thank you for the reviews.
> 
> On 16-12-2019 14:45, Ville Syrjälä wrote:
> > On Sun, Dec 15, 2019 at 05:38:08PM +0100, Hans de Goede wrote:
> >> When the LCD has not been turned on by the firmware/GOP, because e.g. the
> >> device was booted with an external monitor connected over HDMI, we should
> >> not turn on the panel-enable GPIO when we request it.
> >>
> >> Turning on the panel-enable GPIO when we request it, means we turn it on
> >> too early in the init-sequence, which causes some panels to not correctly
> >> light up.
> >>
> >> This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init()
> >> and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly.
> >>
> >> This fixes the panel not lighting up on a Thundersoft TST168 tablet when
> >> booted with an external monitor connected over HDMI.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_dsi.h     | 2 +-
> >>   drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 7 +++----
> >>   drivers/gpu/drm/i915/display/vlv_dsi.c       | 2 +-
> >>   3 files changed, 5 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
> >> index de7e51cd3460..675771ea91aa 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
> >> @@ -203,7 +203,7 @@ 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_init(struct intel_dsi *intel_dsi, bool panel_is_on);
> >>   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);
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> >> index 5352e8c9eca5..027970348b22 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> >> @@ -688,17 +688,16 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
> >>    * 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)
> >> +void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
> >>   {
> >>   	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;
> >> +	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> > 
> > Can't we just tell it not to change the current setting?
> 
> We could use GPIOD_ASIS for that, but with the SoC pins (when the PMIC is
> not used for backlight control) things get a bit muddy, I've seen several
> instances of this message from drivers/pinctrl/intel/pinctrl-baytrail.c
> trigger when the GOP did not init the panel:
> 
> dev_warn(vg->dev, FW_BUG "pin %u forcibly re-configured as GPIO\n", offset);
> 
> And in that case with GPIOD_ASIS I have no idea which we initially get,
> so this approach, where we clearly define which initial value we want,
> seems better.

I suppose. Probably better to not abuse the current_mode pointer for
that though and just explicitly call encoder->get_hw_state() instead.

> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> The intel-gfx CI seems to seriously dislike my patches lately, almost
> always failing them; and usually on what at least seem to be unrelated
> test-cases. Any advice on how to deal with this?

Yeah, CI is snafu. I keep smashing retest until it gets through BAT
and then just double check the shard results to make sure nothing
relevant has tripped. If things look OK I recommend replying to the
result mail and provide a few short log snippets/other details on
what failed so that it's clear that it's irrelevant.

> 
> 
> 
> 
> > 
> >>   
> >>   	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> >>   	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
> >> -		intel_dsi->gpio_panel =
> >> -			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
> >> -
> >> +		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
> >>   		if (IS_ERR(intel_dsi->gpio_panel)) {
> >>   			DRM_ERROR("Failed to own gpio for panel control\n");
> >>   			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 178d0fffba5b..e86e4a11e199 100644
> >> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> >> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> >> @@ -1910,7 +1910,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
> >>   
> >>   	vlv_dphy_param_init(intel_dsi);
> >>   
> >> -	intel_dsi_vbt_gpio_init(intel_dsi);
> >> +	intel_dsi_vbt_gpio_init(intel_dsi, current_mode != NULL);
> >>   
> >>   	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
> >>   			   DRM_MODE_CONNECTOR_DSI);
> >> -- 
> >> 2.23.0
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off

[Hans de Goede]

Hi,

On 16-12-2019 15:14, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:51:54PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> Thank you for the reviews.
>>
>> On 16-12-2019 14:45, Ville Syrjälä wrote:
>>> On Sun, Dec 15, 2019 at 05:38:08PM +0100, Hans de Goede wrote:
>>>> When the LCD has not been turned on by the firmware/GOP, because e.g. the
>>>> device was booted with an external monitor connected over HDMI, we should
>>>> not turn on the panel-enable GPIO when we request it.
>>>>
>>>> Turning on the panel-enable GPIO when we request it, means we turn it on
>>>> too early in the init-sequence, which causes some panels to not correctly
>>>> light up.
>>>>
>>>> This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init()
>>>> and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly.
>>>>
>>>> This fixes the panel not lighting up on a Thundersoft TST168 tablet when
>>>> booted with an external monitor connected over HDMI.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dsi.h     | 2 +-
>>>>    drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 7 +++----
>>>>    drivers/gpu/drm/i915/display/vlv_dsi.c       | 2 +-
>>>>    3 files changed, 5 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
>>>> index de7e51cd3460..675771ea91aa 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
>>>> @@ -203,7 +203,7 @@ 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_init(struct intel_dsi *intel_dsi, bool panel_is_on);
>>>>    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);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>>>> index 5352e8c9eca5..027970348b22 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>>>> @@ -688,17 +688,16 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>>>     * 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)
>>>> +void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>>>    {
>>>>    	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;
>>>> +	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
>>>
>>> Can't we just tell it not to change the current setting?
>>
>> We could use GPIOD_ASIS for that, but with the SoC pins (when the PMIC is
>> not used for backlight control) things get a bit muddy, I've seen several
>> instances of this message from drivers/pinctrl/intel/pinctrl-baytrail.c
>> trigger when the GOP did not init the panel:
>>
>> dev_warn(vg->dev, FW_BUG "pin %u forcibly re-configured as GPIO\n", offset);
>>
>> And in that case with GPIOD_ASIS I have no idea which we initially get,
>> so this approach, where we clearly define which initial value we want,
>> seems better.
> 
> I suppose. Probably better to not abuse the current_mode pointer for
> that though and just explicitly call encoder->get_hw_state() instead.

Ok, I'll fix this for v2 of the patch-set.

Regards,

Hans





>> p.s.
>>
>> The intel-gfx CI seems to seriously dislike my patches lately, almost
>> always failing them; and usually on what at least seem to be unrelated
>> test-cases. Any advice on how to deal with this?
> 
> Yeah, CI is snafu. I keep smashing retest until it gets through BAT
> and then just double check the shard results to make sure nothing
> relevant has tripped. If things look OK I recommend replying to the
> result mail and provide a few short log snippets/other details on
> what failed so that it's clear that it's irrelevant.
> 
>>
>>
>>
>>
>>>
>>>>    
>>>>    	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>>>    	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
>>>> -		intel_dsi->gpio_panel =
>>>> -			gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
>>>> -
>>>> +		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>>>>    		if (IS_ERR(intel_dsi->gpio_panel)) {
>>>>    			DRM_ERROR("Failed to own gpio for panel control\n");
>>>>    			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 178d0fffba5b..e86e4a11e199 100644
>>>> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
>>>> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
>>>> @@ -1910,7 +1910,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
>>>>    
>>>>    	vlv_dphy_param_init(intel_dsi);
>>>>    
>>>> -	intel_dsi_vbt_gpio_init(intel_dsi);
>>>> +	intel_dsi_vbt_gpio_init(intel_dsi, current_mode != NULL);
>>>>    
>>>>    	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
>>>>    			   DRM_MODE_CONNECTOR_DSI);
>>>> -- 
>>>> 2.23.0
>>>
> 

[PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver

[Hans de Goede]

Move the Crystal Cove PMIC panel GPIO lookup-table from
drivers/mfd/intel_soc_pmic_core.c to the i915 driver.

The moved looked-up table is adding a GPIO lookup to the i915 PCI
device and the GPIO subsys allows only one lookup table per device,

The intel_soc_pmic_core.c code only adds lookup-table entries for the
PMIC panel GPIO (as it deals only with the PMIC), but we also need to be
able to access some GPIOs on the SoC itself, which requires entries for
these GPIOs in the lookup-table.

Since the lookup-table is attached to the i915 PCI device it really
should be part of the i915 driver, this will also allow us to extend
it with GPIOs from other sources when necessary.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 23 +++++++++++++++++++-
 drivers/mfd/intel_soc_pmic_core.c            | 19 ----------------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 027970348b22..847f04eec2a1 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -25,6 +25,7 @@
  */
 
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/mfd/intel_soc_pmic.h>
 #include <linux/slab.h>
 
@@ -686,8 +687,18 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
 
 /*
  * On some BYT/CHT devs some sequences are incomplete and we need to manually
- * control some GPIOs.
+ * control some GPIOs. We need to add a GPIO lookup table before we get these.
  */
+static struct gpiod_lookup_table pmic_panel_gpio_table = {
+	/* Intel GFX is consumer */
+	.dev_id = "0000:00:02.0",
+	.table = {
+		/* Panel EN/DISABLE */
+		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
+		{ },
+	},
+};
+
 void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
 {
 	struct drm_device *dev = intel_dsi->base.base.dev;
@@ -697,6 +708,8 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
 
 	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
 	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
+		gpiod_add_lookup_table(&pmic_panel_gpio_table);
+
 		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
 		if (IS_ERR(intel_dsi->gpio_panel)) {
 			DRM_ERROR("Failed to own gpio for panel control\n");
@@ -707,8 +720,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
 
 void intel_dsi_vbt_gpio_cleanup(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 (intel_dsi->gpio_panel) {
 		gpiod_put(intel_dsi->gpio_panel);
 		intel_dsi->gpio_panel = NULL;
 	}
+
+	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
+	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
+		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
 }
diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
index 47188df3080d..ddd64f9e3341 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -9,8 +9,6 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/machine.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
@@ -25,17 +23,6 @@
 #define BYT_CRC_HRV		2
 #define CHT_CRC_HRV		3
 
-/* Lookup table for the Panel Enable/Disable line as GPIO signals */
-static struct gpiod_lookup_table panel_gpio_table = {
-	/* Intel GFX is consumer */
-	.dev_id = "0000:00:02.0",
-	.table = {
-		/* Panel EN/DISABLE */
-		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
-		{ },
-	},
-};
-
 /* PWM consumed by the Intel GFX */
 static struct pwm_lookup crc_pwm_lookup[] = {
 	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
@@ -96,9 +83,6 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 	if (ret)
 		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
 
-	/* Add lookup table binding for Panel Control to the GPIO Chip */
-	gpiod_add_lookup_table(&panel_gpio_table);
-
 	/* Add lookup table for crc-pwm */
 	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
 
@@ -121,9 +105,6 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
 
 	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
 
-	/* Remove lookup table for Panel Control from the GPIO Chip */
-	gpiod_remove_lookup_table(&panel_gpio_table);
-
 	/* remove crc-pwm lookup table */
 	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
 
-- 
2.23.0

Re: [PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver

[Linus Walleij]

On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Move the Crystal Cove PMIC panel GPIO lookup-table from
> drivers/mfd/intel_soc_pmic_core.c to the i915 driver.
>
> The moved looked-up table is adding a GPIO lookup to the i915 PCI
> device and the GPIO subsys allows only one lookup table per device,
>
> The intel_soc_pmic_core.c code only adds lookup-table entries for the
> PMIC panel GPIO (as it deals only with the PMIC), but we also need to be
> able to access some GPIOs on the SoC itself, which requires entries for
> these GPIOs in the lookup-table.
>
> Since the lookup-table is attached to the i915 PCI device it really
> should be part of the i915 driver, this will also allow us to extend
> it with GPIOs from other sources when necessary.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks OK to me
Acked-by: Linus Walleij <linus.walleij@linaro.org>

But Lee & Andy should have a final word on this.

Yours,
Linus Walleij

Re: [PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver

[Andy Shevchenko]

On Sun, Dec 15, 2019 at 05:38:09PM +0100, Hans de Goede wrote:
> Move the Crystal Cove PMIC panel GPIO lookup-table from
> drivers/mfd/intel_soc_pmic_core.c to the i915 driver.
> 
> The moved looked-up table is adding a GPIO lookup to the i915 PCI
> device and the GPIO subsys allows only one lookup table per device,
> 
> The intel_soc_pmic_core.c code only adds lookup-table entries for the
> PMIC panel GPIO (as it deals only with the PMIC), but we also need to be
> able to access some GPIOs on the SoC itself, which requires entries for
> these GPIOs in the lookup-table.
> 
> Since the lookup-table is attached to the i915 PCI device it really
> should be part of the i915 driver, this will also allow us to extend
> it with GPIOs from other sources when necessary.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
One nit below.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 23 +++++++++++++++++++-
>  drivers/mfd/intel_soc_pmic_core.c            | 19 ----------------
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index 027970348b22..847f04eec2a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/mfd/intel_soc_pmic.h>
>  #include <linux/slab.h>
>  
> @@ -686,8 +687,18 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  
>  /*
>   * On some BYT/CHT devs some sequences are incomplete and we need to manually
> - * control some GPIOs.
> + * control some GPIOs. We need to add a GPIO lookup table before we get these.
>   */
> +static struct gpiod_lookup_table pmic_panel_gpio_table = {
> +	/* Intel GFX is consumer */
> +	.dev_id = "0000:00:02.0",
> +	.table = {
> +		/* Panel EN/DISABLE */
> +		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),

> +		{ },

Usually we don't put comma in terminator kind of lines. (Yes I see that it is
in original code, but we may have a chance to fix it without additional churn).
Rationale is to prevent some weird issues (like wrong conflict resolution)
where record may appear after terminator line and will be compiled correctly.

> +	},
> +};
> +
>  void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  {
>  	struct drm_device *dev = intel_dsi->base.base.dev;
> @@ -697,6 +708,8 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  
>  	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>  	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
> +		gpiod_add_lookup_table(&pmic_panel_gpio_table);
> +
>  		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>  		if (IS_ERR(intel_dsi->gpio_panel)) {
>  			DRM_ERROR("Failed to own gpio for panel control\n");
> @@ -707,8 +720,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  
>  void intel_dsi_vbt_gpio_cleanup(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 (intel_dsi->gpio_panel) {
>  		gpiod_put(intel_dsi->gpio_panel);
>  		intel_dsi->gpio_panel = NULL;
>  	}
> +
> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> +	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
> +		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
>  }
> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
> index 47188df3080d..ddd64f9e3341 100644
> --- a/drivers/mfd/intel_soc_pmic_core.c
> +++ b/drivers/mfd/intel_soc_pmic_core.c
> @@ -9,8 +9,6 @@
>   */
>  
>  #include <linux/acpi.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/gpio/machine.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> @@ -25,17 +23,6 @@
>  #define BYT_CRC_HRV		2
>  #define CHT_CRC_HRV		3
>  
> -/* Lookup table for the Panel Enable/Disable line as GPIO signals */
> -static struct gpiod_lookup_table panel_gpio_table = {
> -	/* Intel GFX is consumer */
> -	.dev_id = "0000:00:02.0",
> -	.table = {
> -		/* Panel EN/DISABLE */
> -		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
> -		{ },
> -	},
> -};
> -
>  /* PWM consumed by the Intel GFX */
>  static struct pwm_lookup crc_pwm_lookup[] = {
>  	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
> @@ -96,9 +83,6 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>  	if (ret)
>  		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
>  
> -	/* Add lookup table binding for Panel Control to the GPIO Chip */
> -	gpiod_add_lookup_table(&panel_gpio_table);
> -
>  	/* Add lookup table for crc-pwm */
>  	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>  
> @@ -121,9 +105,6 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
>  
>  	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
>  
> -	/* Remove lookup table for Panel Control from the GPIO Chip */
> -	gpiod_remove_lookup_table(&panel_gpio_table);
> -
>  	/* remove crc-pwm lookup table */
>  	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>  
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver

[Hans de Goede]

Hi,

On 16-12-2019 13:16, Andy Shevchenko wrote:
> On Sun, Dec 15, 2019 at 05:38:09PM +0100, Hans de Goede wrote:
>> Move the Crystal Cove PMIC panel GPIO lookup-table from
>> drivers/mfd/intel_soc_pmic_core.c to the i915 driver.
>>
>> The moved looked-up table is adding a GPIO lookup to the i915 PCI
>> device and the GPIO subsys allows only one lookup table per device,
>>
>> The intel_soc_pmic_core.c code only adds lookup-table entries for the
>> PMIC panel GPIO (as it deals only with the PMIC), but we also need to be
>> able to access some GPIOs on the SoC itself, which requires entries for
>> these GPIOs in the lookup-table.
>>
>> Since the lookup-table is attached to the i915 PCI device it really
>> should be part of the i915 driver, this will also allow us to extend
>> it with GPIOs from other sources when necessary.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Thanks.

> One nit below.
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 23 +++++++++++++++++++-
>>   drivers/mfd/intel_soc_pmic_core.c            | 19 ----------------
>>   2 files changed, 22 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> index 027970348b22..847f04eec2a1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -25,6 +25,7 @@
>>    */
>>   
>>   #include <linux/gpio/consumer.h>
>> +#include <linux/gpio/machine.h>
>>   #include <linux/mfd/intel_soc_pmic.h>
>>   #include <linux/slab.h>
>>   
>> @@ -686,8 +687,18 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>   
>>   /*
>>    * On some BYT/CHT devs some sequences are incomplete and we need to manually
>> - * control some GPIOs.
>> + * control some GPIOs. We need to add a GPIO lookup table before we get these.
>>    */
>> +static struct gpiod_lookup_table pmic_panel_gpio_table = {
>> +	/* Intel GFX is consumer */
>> +	.dev_id = "0000:00:02.0",
>> +	.table = {
>> +		/* Panel EN/DISABLE */
>> +		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
> 
>> +		{ },
> 
> Usually we don't put comma in terminator kind of lines. (Yes I see that it is
> in original code, but we may have a chance to fix it without additional churn).
> Rationale is to prevent some weird issues (like wrong conflict resolution)
> where record may appear after terminator line and will be compiled correctly.

I need to respin the set because of the pinctrl map unregister thingie anyways,
so I have fixed this for v2.

Regards,

Hans


> 
>> +	},
>> +};
>> +
>>   void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   {
>>   	struct drm_device *dev = intel_dsi->base.base.dev;
>> @@ -697,6 +708,8 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   
>>   	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>   	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
>> +		gpiod_add_lookup_table(&pmic_panel_gpio_table);
>> +
>>   		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>>   		if (IS_ERR(intel_dsi->gpio_panel)) {
>>   			DRM_ERROR("Failed to own gpio for panel control\n");
>> @@ -707,8 +720,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   
>>   void intel_dsi_vbt_gpio_cleanup(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 (intel_dsi->gpio_panel) {
>>   		gpiod_put(intel_dsi->gpio_panel);
>>   		intel_dsi->gpio_panel = NULL;
>>   	}
>> +
>> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>> +	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
>> +		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
>>   }
>> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
>> index 47188df3080d..ddd64f9e3341 100644
>> --- a/drivers/mfd/intel_soc_pmic_core.c
>> +++ b/drivers/mfd/intel_soc_pmic_core.c
>> @@ -9,8 +9,6 @@
>>    */
>>   
>>   #include <linux/acpi.h>
>> -#include <linux/gpio/consumer.h>
>> -#include <linux/gpio/machine.h>
>>   #include <linux/i2c.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/module.h>
>> @@ -25,17 +23,6 @@
>>   #define BYT_CRC_HRV		2
>>   #define CHT_CRC_HRV		3
>>   
>> -/* Lookup table for the Panel Enable/Disable line as GPIO signals */
>> -static struct gpiod_lookup_table panel_gpio_table = {
>> -	/* Intel GFX is consumer */
>> -	.dev_id = "0000:00:02.0",
>> -	.table = {
>> -		/* Panel EN/DISABLE */
>> -		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
>> -		{ },
>> -	},
>> -};
>> -
>>   /* PWM consumed by the Intel GFX */
>>   static struct pwm_lookup crc_pwm_lookup[] = {
>>   	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
>> @@ -96,9 +83,6 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>>   	if (ret)
>>   		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
>>   
>> -	/* Add lookup table binding for Panel Control to the GPIO Chip */
>> -	gpiod_add_lookup_table(&panel_gpio_table);
>> -
>>   	/* Add lookup table for crc-pwm */
>>   	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>>   
>> @@ -121,9 +105,6 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
>>   
>>   	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
>>   
>> -	/* Remove lookup table for Panel Control from the GPIO Chip */
>> -	gpiod_remove_lookup_table(&panel_gpio_table);
>> -
>>   	/* remove crc-pwm lookup table */
>>   	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>>   
>> -- 
>> 2.23.0
>>
> 

Re: [PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver

[Ville Syrjälä]

On Sun, Dec 15, 2019 at 05:38:09PM +0100, Hans de Goede wrote:
> Move the Crystal Cove PMIC panel GPIO lookup-table from
> drivers/mfd/intel_soc_pmic_core.c to the i915 driver.
> 
> The moved looked-up table is adding a GPIO lookup to the i915 PCI
> device and the GPIO subsys allows only one lookup table per device,
> 
> The intel_soc_pmic_core.c code only adds lookup-table entries for the
> PMIC panel GPIO (as it deals only with the PMIC), but we also need to be
> able to access some GPIOs on the SoC itself, which requires entries for
> these GPIOs in the lookup-table.
> 
> Since the lookup-table is attached to the i915 PCI device it really
> should be part of the i915 driver, this will also allow us to extend
> it with GPIOs from other sources when necessary.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 23 +++++++++++++++++++-
>  drivers/mfd/intel_soc_pmic_core.c            | 19 ----------------
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index 027970348b22..847f04eec2a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/mfd/intel_soc_pmic.h>
>  #include <linux/slab.h>
>  
> @@ -686,8 +687,18 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  
>  /*
>   * On some BYT/CHT devs some sequences are incomplete and we need to manually
> - * control some GPIOs.
> + * control some GPIOs. We need to add a GPIO lookup table before we get these.
>   */
> +static struct gpiod_lookup_table pmic_panel_gpio_table = {
> +	/* Intel GFX is consumer */
> +	.dev_id = "0000:00:02.0",
> +	.table = {
> +		/* Panel EN/DISABLE */
> +		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
> +		{ },
> +	},
> +};

Feels like a failure in abstraction to have these irrelevant details
exposed on the consumer side. Also slightly concerned that someone
refactoring things in the pmic driver could now break this without
realizing it. But if people want it done this way I can live with it.

> +
>  void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  {
>  	struct drm_device *dev = intel_dsi->base.base.dev;
> @@ -697,6 +708,8 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  
>  	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>  	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
> +		gpiod_add_lookup_table(&pmic_panel_gpio_table);
> +
>  		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>  		if (IS_ERR(intel_dsi->gpio_panel)) {
>  			DRM_ERROR("Failed to own gpio for panel control\n");
> @@ -707,8 +720,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  
>  void intel_dsi_vbt_gpio_cleanup(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 (intel_dsi->gpio_panel) {
>  		gpiod_put(intel_dsi->gpio_panel);
>  		intel_dsi->gpio_panel = NULL;
>  	}
> +
> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> +	    (mipi_config->pwm_blc == PPS_BLC_PMIC))

Needless parens here as well.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
>  }
> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
> index 47188df3080d..ddd64f9e3341 100644
> --- a/drivers/mfd/intel_soc_pmic_core.c
> +++ b/drivers/mfd/intel_soc_pmic_core.c
> @@ -9,8 +9,6 @@
>   */
>  
>  #include <linux/acpi.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/gpio/machine.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> @@ -25,17 +23,6 @@
>  #define BYT_CRC_HRV		2
>  #define CHT_CRC_HRV		3
>  
> -/* Lookup table for the Panel Enable/Disable line as GPIO signals */
> -static struct gpiod_lookup_table panel_gpio_table = {
> -	/* Intel GFX is consumer */
> -	.dev_id = "0000:00:02.0",
> -	.table = {
> -		/* Panel EN/DISABLE */
> -		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
> -		{ },
> -	},
> -};
> -
>  /* PWM consumed by the Intel GFX */
>  static struct pwm_lookup crc_pwm_lookup[] = {
>  	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
> @@ -96,9 +83,6 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>  	if (ret)
>  		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
>  
> -	/* Add lookup table binding for Panel Control to the GPIO Chip */
> -	gpiod_add_lookup_table(&panel_gpio_table);
> -
>  	/* Add lookup table for crc-pwm */
>  	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>  
> @@ -121,9 +105,6 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
>  
>  	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
>  
> -	/* Remove lookup table for Panel Control from the GPIO Chip */
> -	gpiod_remove_lookup_table(&panel_gpio_table);
> -
>  	/* remove crc-pwm lookup table */
>  	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>  
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver

[Hans de Goede]

Hi,

On 16-12-2019 14:56, Ville Syrjälä wrote:
> On Sun, Dec 15, 2019 at 05:38:09PM +0100, Hans de Goede wrote:
>> Move the Crystal Cove PMIC panel GPIO lookup-table from
>> drivers/mfd/intel_soc_pmic_core.c to the i915 driver.
>>
>> The moved looked-up table is adding a GPIO lookup to the i915 PCI
>> device and the GPIO subsys allows only one lookup table per device,
>>
>> The intel_soc_pmic_core.c code only adds lookup-table entries for the
>> PMIC panel GPIO (as it deals only with the PMIC), but we also need to be
>> able to access some GPIOs on the SoC itself, which requires entries for
>> these GPIOs in the lookup-table.
>>
>> Since the lookup-table is attached to the i915 PCI device it really
>> should be part of the i915 driver, this will also allow us to extend
>> it with GPIOs from other sources when necessary.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 23 +++++++++++++++++++-
>>   drivers/mfd/intel_soc_pmic_core.c            | 19 ----------------
>>   2 files changed, 22 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> index 027970348b22..847f04eec2a1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -25,6 +25,7 @@
>>    */
>>   
>>   #include <linux/gpio/consumer.h>
>> +#include <linux/gpio/machine.h>
>>   #include <linux/mfd/intel_soc_pmic.h>
>>   #include <linux/slab.h>
>>   
>> @@ -686,8 +687,18 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>   
>>   /*
>>    * On some BYT/CHT devs some sequences are incomplete and we need to manually
>> - * control some GPIOs.
>> + * control some GPIOs. We need to add a GPIO lookup table before we get these.
>>    */
>> +static struct gpiod_lookup_table pmic_panel_gpio_table = {
>> +	/* Intel GFX is consumer */
>> +	.dev_id = "0000:00:02.0",
>> +	.table = {
>> +		/* Panel EN/DISABLE */
>> +		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
>> +		{ },
>> +	},
>> +};
> 
> Feels like a failure in abstraction to have these irrelevant details
> exposed on the consumer side. Also slightly concerned that someone
> refactoring things in the pmic driver could now break this without
> realizing it. But if people want it done this way I can live with it.

Note how in the final patch we add another lookup for a GPIO called "panel"
but now on another GPIO controller. Since which GPIO controller has the
"panel" GPIO is specified by a bit in the VBT doing the lookup-table
registering from within the i915 code actually kinda makes sense.

> 
>> +
>>   void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   {
>>   	struct drm_device *dev = intel_dsi->base.base.dev;
>> @@ -697,6 +708,8 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   
>>   	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>   	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
>> +		gpiod_add_lookup_table(&pmic_panel_gpio_table);
>> +
>>   		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>>   		if (IS_ERR(intel_dsi->gpio_panel)) {
>>   			DRM_ERROR("Failed to own gpio for panel control\n");
>> @@ -707,8 +720,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   
>>   void intel_dsi_vbt_gpio_cleanup(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 (intel_dsi->gpio_panel) {
>>   		gpiod_put(intel_dsi->gpio_panel);
>>   		intel_dsi->gpio_panel = NULL;
>>   	}
>> +
>> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>> +	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
> 
> Needless parens here as well.

Will fix for v2 (will also the other case).

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks.

Regards,

Hans



> 
>> +		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
>>   }
>> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
>> index 47188df3080d..ddd64f9e3341 100644
>> --- a/drivers/mfd/intel_soc_pmic_core.c
>> +++ b/drivers/mfd/intel_soc_pmic_core.c
>> @@ -9,8 +9,6 @@
>>    */
>>   
>>   #include <linux/acpi.h>
>> -#include <linux/gpio/consumer.h>
>> -#include <linux/gpio/machine.h>
>>   #include <linux/i2c.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/module.h>
>> @@ -25,17 +23,6 @@
>>   #define BYT_CRC_HRV		2
>>   #define CHT_CRC_HRV		3
>>   
>> -/* Lookup table for the Panel Enable/Disable line as GPIO signals */
>> -static struct gpiod_lookup_table panel_gpio_table = {
>> -	/* Intel GFX is consumer */
>> -	.dev_id = "0000:00:02.0",
>> -	.table = {
>> -		/* Panel EN/DISABLE */
>> -		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
>> -		{ },
>> -	},
>> -};
>> -
>>   /* PWM consumed by the Intel GFX */
>>   static struct pwm_lookup crc_pwm_lookup[] = {
>>   	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
>> @@ -96,9 +83,6 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>>   	if (ret)
>>   		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
>>   
>> -	/* Add lookup table binding for Panel Control to the GPIO Chip */
>> -	gpiod_add_lookup_table(&panel_gpio_table);
>> -
>>   	/* Add lookup table for crc-pwm */
>>   	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>>   
>> @@ -121,9 +105,6 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
>>   
>>   	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
>>   
>> -	/* Remove lookup table for Panel Control from the GPIO Chip */
>> -	gpiod_remove_lookup_table(&panel_gpio_table);
>> -
>>   	/* remove crc-pwm lookup table */
>>   	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>>   
>> -- 
>> 2.23.0
> 

[PATCH 5/5] drm/i915/dsi: Control panel and backlight enable GPIOs on BYT

[Hans de Goede]

On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
do not control the LCD panel- and backlight-enable GPIOs. So far, when
the VBT indicates we should use the SoC for backlight control, we have
been relying on these GPIOs being configured as output and driven high by
the Video BIOS (GOP) when it initializes the panel.

This does not work when the device is booted with a HDMI monitor connected
as then the GOP will initialize the HDMI instead of the panel, leaving the
panel black, even though the i915 driver tries to output an image to it.

Likewise on some device-models when the GOP does not initialize the DSI
panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead
of muxing it to the PWM controller.

This commit makes the DSI code control the SoC GPIOs for panel- and
backlight-enable on BYT, when the VBT indicates the SoC should be used

for backlight control. It also ensures that the PWM0 pin is muxed to the
PWM controller in this case.

This fixes the LCD panel not lighting up on various devices when booted
with a HDMI monitor connected. This has been tested to fix this on the
following devices:

Peaq C1010
Point of View MOBII TAB-P800W
Point of View MOBII TAB-P1005W
Terra Pad 1061
Yours Y8W81

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dsi.h     |  3 +-
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 63 ++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
index 675771ea91aa..7481a5aa3084 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi.h
+++ b/drivers/gpu/drm/i915/display/intel_dsi.h
@@ -45,8 +45,9 @@ struct intel_dsi {
 	struct intel_dsi_host *dsi_hosts[I915_MAX_PORTS];
 	intel_wakeref_t io_wakeref[I915_MAX_PORTS];
 
-	/* GPIO Desc for CRC based Panel control */
+	/* GPIO Desc for panel and backlight control */
 	struct gpio_desc *gpio_panel;
+	struct gpio_desc *gpio_backlight;
 
 	struct intel_connector *attached_connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 847f04eec2a1..bd007d4f86e2 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -27,6 +27,8 @@
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
 #include <linux/mfd/intel_soc_pmic.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/machine.h>
 #include <linux/slab.h>
 
 #include <asm/intel-mid.h>
@@ -525,11 +527,15 @@ void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
 {
 	if (seq_id == MIPI_SEQ_POWER_ON && intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
+	if (seq_id == MIPI_SEQ_BACKLIGHT_ON && intel_dsi->gpio_backlight)
+		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 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);
+	if (seq_id == MIPI_SEQ_BACKLIGHT_OFF && intel_dsi->gpio_backlight)
+		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 0);
 }
 
 void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
@@ -688,6 +694,8 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
 /*
  * On some BYT/CHT devs some sequences are incomplete and we need to manually
  * control some GPIOs. We need to add a GPIO lookup table before we get these.
+ * If the GOP did not initialize the panel (HDMI inserted) we may need to also
+ * change the pinmux for the SoC's PWM0 pin from GPIO to PWM.
  */
 static struct gpiod_lookup_table pmic_panel_gpio_table = {
 	/* Intel GFX is consumer */
@@ -699,23 +707,68 @@ static struct gpiod_lookup_table pmic_panel_gpio_table = {
 	},
 };
 
+static struct gpiod_lookup_table soc_panel_gpio_table = {
+	.dev_id = "0000:00:02.0",
+	.table = {
+	  GPIO_LOOKUP("INT33FC:01", 10, "backlight", GPIO_ACTIVE_HIGH),
+	  GPIO_LOOKUP("INT33FC:01", 11, "panel", GPIO_ACTIVE_HIGH),
+	  { },
+	},
+};
+
+static const struct pinctrl_map soc_pwm_pinctrl_map[] = {
+	PIN_MAP_MUX_GROUP("0000:00:02.0", "soc_pwm0", "INT33FC:00",
+			  "pwm0_grp", "pwm"),
+};
+
 void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
 {
 	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;
 	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+	bool want_backlight_gpio = false;
+	bool want_panel_gpio = false;
+	struct pinctrl *pinctrl;
+	int ret;
 
 	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
 	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
 		gpiod_add_lookup_table(&pmic_panel_gpio_table);
+		want_panel_gpio = true;
+	}
+
+	if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
+		gpiod_add_lookup_table(&soc_panel_gpio_table);
+		want_panel_gpio = true;
+		want_backlight_gpio = true;
 
+		/* Ensure PWM0 pin is muxed as PWM instead of GPIO */
+		ret = pinctrl_register_mappings(soc_pwm_pinctrl_map, 1);
+		if (ret)
+			DRM_ERROR("Failed to register pwm0 pinmux mapping\n");
+
+		pinctrl = devm_pinctrl_get_select(dev->dev, "soc_pwm0");
+		if (IS_ERR(pinctrl))
+			DRM_ERROR("Failed to set pinmux to PWM\n");
+	}
+
+	if (want_panel_gpio) {
 		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
 		if (IS_ERR(intel_dsi->gpio_panel)) {
 			DRM_ERROR("Failed to own gpio for panel control\n");
 			intel_dsi->gpio_panel = NULL;
 		}
 	}
+
+	if (want_backlight_gpio) {
+		intel_dsi->gpio_backlight =
+			gpiod_get(dev->dev, "backlight", flags);
+		if (IS_ERR(intel_dsi->gpio_backlight)) {
+			DRM_ERROR("Failed to own gpio for backlight control\n");
+			intel_dsi->gpio_backlight = NULL;
+		}
+	}
 }
 
 void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
@@ -729,7 +782,17 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
 		intel_dsi->gpio_panel = NULL;
 	}
 
+	if (intel_dsi->gpio_backlight) {
+		gpiod_put(intel_dsi->gpio_backlight);
+		intel_dsi->gpio_backlight = NULL;
+	}
+
 	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
 	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
 		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
+
+	if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
+		pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
+		gpiod_remove_lookup_table(&soc_panel_gpio_table);
+	}
 }
-- 
2.23.0

Re: [PATCH 5/5] drm/i915/dsi: Control panel and backlight enable GPIOs on BYT

[Linus Walleij]

On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

> On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
> do not control the LCD panel- and backlight-enable GPIOs. So far, when
> the VBT indicates we should use the SoC for backlight control, we have
> been relying on these GPIOs being configured as output and driven high by
> the Video BIOS (GOP) when it initializes the panel.
>
> This does not work when the device is booted with a HDMI monitor connected
> as then the GOP will initialize the HDMI instead of the panel, leaving the
> panel black, even though the i915 driver tries to output an image to it.
>
> Likewise on some device-models when the GOP does not initialize the DSI
> panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead
> of muxing it to the PWM controller.
>
> This commit makes the DSI code control the SoC GPIOs for panel- and
> backlight-enable on BYT, when the VBT indicates the SoC should be used
>
> for backlight control. It also ensures that the PWM0 pin is muxed to the
> PWM controller in this case.
>
> This fixes the LCD panel not lighting up on various devices when booted
> with a HDMI monitor connected. This has been tested to fix this on the
> following devices:
>
> Peaq C1010
> Point of View MOBII TAB-P800W
> Point of View MOBII TAB-P1005W
> Terra Pad 1061
> Yours Y8W81
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 5/5] drm/i915/dsi: Control panel and backlight enable GPIOs on BYT

[Ville Syrjälä]

On Sun, Dec 15, 2019 at 05:38:10PM +0100, Hans de Goede wrote:
> On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
> do not control the LCD panel- and backlight-enable GPIOs. So far, when
> the VBT indicates we should use the SoC for backlight control, we have
> been relying on these GPIOs being configured as output and driven high by
> the Video BIOS (GOP) when it initializes the panel.
> 
> This does not work when the device is booted with a HDMI monitor connected
> as then the GOP will initialize the HDMI instead of the panel, leaving the
> panel black, even though the i915 driver tries to output an image to it.
> 
> Likewise on some device-models when the GOP does not initialize the DSI
> panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead
> of muxing it to the PWM controller.
> 
> This commit makes the DSI code control the SoC GPIOs for panel- and
> backlight-enable on BYT, when the VBT indicates the SoC should be used
> 
> for backlight control. It also ensures that the PWM0 pin is muxed to the
> PWM controller in this case.
> 
> This fixes the LCD panel not lighting up on various devices when booted
> with a HDMI monitor connected. This has been tested to fix this on the
> following devices:
> 
> Peaq C1010
> Point of View MOBII TAB-P800W
> Point of View MOBII TAB-P1005W
> Terra Pad 1061
> Yours Y8W81
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi.h     |  3 +-
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 63 ++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
> index 675771ea91aa..7481a5aa3084 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
> @@ -45,8 +45,9 @@ struct intel_dsi {
>  	struct intel_dsi_host *dsi_hosts[I915_MAX_PORTS];
>  	intel_wakeref_t io_wakeref[I915_MAX_PORTS];
>  
> -	/* GPIO Desc for CRC based Panel control */
> +	/* GPIO Desc for panel and backlight control */
>  	struct gpio_desc *gpio_panel;
> +	struct gpio_desc *gpio_backlight;
>  
>  	struct intel_connector *attached_connector;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index 847f04eec2a1..bd007d4f86e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -27,6 +27,8 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pinctrl/machine.h>
>  #include <linux/slab.h>
>  
>  #include <asm/intel-mid.h>
> @@ -525,11 +527,15 @@ void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
>  {
>  	if (seq_id == MIPI_SEQ_POWER_ON && intel_dsi->gpio_panel)
>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
> +	if (seq_id == MIPI_SEQ_BACKLIGHT_ON && intel_dsi->gpio_backlight)
> +		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 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);
> +	if (seq_id == MIPI_SEQ_BACKLIGHT_OFF && intel_dsi->gpio_backlight)
> +		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 0);
>  }
>  
>  void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
> @@ -688,6 +694,8 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  /*
>   * On some BYT/CHT devs some sequences are incomplete and we need to manually
>   * control some GPIOs. We need to add a GPIO lookup table before we get these.
> + * If the GOP did not initialize the panel (HDMI inserted) we may need to also
> + * change the pinmux for the SoC's PWM0 pin from GPIO to PWM.
>   */
>  static struct gpiod_lookup_table pmic_panel_gpio_table = {
>  	/* Intel GFX is consumer */
> @@ -699,23 +707,68 @@ static struct gpiod_lookup_table pmic_panel_gpio_table = {
>  	},
>  };
>  
> +static struct gpiod_lookup_table soc_panel_gpio_table = {
> +	.dev_id = "0000:00:02.0",
> +	.table = {
> +	  GPIO_LOOKUP("INT33FC:01", 10, "backlight", GPIO_ACTIVE_HIGH),
> +	  GPIO_LOOKUP("INT33FC:01", 11, "panel", GPIO_ACTIVE_HIGH),
> +	  { },

Some kind of indent fail here.

> +	},
> +};
> +
> +static const struct pinctrl_map soc_pwm_pinctrl_map[] = {
> +	PIN_MAP_MUX_GROUP("0000:00:02.0", "soc_pwm0", "INT33FC:00",
> +			  "pwm0_grp", "pwm"),
> +};
> +
>  void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>  {
>  	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;
>  	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> +	bool want_backlight_gpio = false;
> +	bool want_panel_gpio = false;
> +	struct pinctrl *pinctrl;
> +	int ret;
>  
>  	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>  	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
>  		gpiod_add_lookup_table(&pmic_panel_gpio_table);
> +		want_panel_gpio = true;
> +	}
> +
> +	if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
> +		gpiod_add_lookup_table(&soc_panel_gpio_table);
> +		want_panel_gpio = true;
> +		want_backlight_gpio = true;
>  
> +		/* Ensure PWM0 pin is muxed as PWM instead of GPIO */
> +		ret = pinctrl_register_mappings(soc_pwm_pinctrl_map, 1);

ARRAY_SIZE()?

> +		if (ret)
> +			DRM_ERROR("Failed to register pwm0 pinmux mapping\n");
> +
> +		pinctrl = devm_pinctrl_get_select(dev->dev, "soc_pwm0");
> +		if (IS_ERR(pinctrl))
> +			DRM_ERROR("Failed to set pinmux to PWM\n");
> +	}
> +
> +	if (want_panel_gpio) {
>  		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>  		if (IS_ERR(intel_dsi->gpio_panel)) {
>  			DRM_ERROR("Failed to own gpio for panel control\n");
>  			intel_dsi->gpio_panel = NULL;
>  		}
>  	}
> +
> +	if (want_backlight_gpio) {
> +		intel_dsi->gpio_backlight =
> +			gpiod_get(dev->dev, "backlight", flags);
> +		if (IS_ERR(intel_dsi->gpio_backlight)) {
> +			DRM_ERROR("Failed to own gpio for backlight control\n");
> +			intel_dsi->gpio_backlight = NULL;
> +		}
> +	}
>  }
>  
>  void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
> @@ -729,7 +782,17 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
>  		intel_dsi->gpio_panel = NULL;
>  	}
>  
> +	if (intel_dsi->gpio_backlight) {
> +		gpiod_put(intel_dsi->gpio_backlight);
> +		intel_dsi->gpio_backlight = NULL;
> +	}
> +
>  	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>  	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
>  		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
> +
> +	if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {

Slightly annoying to have these checks duplicated. Might be cleaner to
have a few helpers that return the correct tables and just use those in
both init and cleanup. OTOH those want_*_gpio flags and the pwm stuff is
would still be a sticking point I suppose. So maybe not cleaner in the
end after all.

Looks all right to me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


> +		pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
> +		gpiod_remove_lookup_table(&soc_panel_gpio_table);
> +	}
>  }
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 5/5] drm/i915/dsi: Control panel and backlight enable GPIOs on BYT

[Hans de Goede]

Hi,

On 16-12-2019 15:04, Ville Syrjälä wrote:
> On Sun, Dec 15, 2019 at 05:38:10PM +0100, Hans de Goede wrote:
>> On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
>> do not control the LCD panel- and backlight-enable GPIOs. So far, when
>> the VBT indicates we should use the SoC for backlight control, we have
>> been relying on these GPIOs being configured as output and driven high by
>> the Video BIOS (GOP) when it initializes the panel.
>>
>> This does not work when the device is booted with a HDMI monitor connected
>> as then the GOP will initialize the HDMI instead of the panel, leaving the
>> panel black, even though the i915 driver tries to output an image to it.
>>
>> Likewise on some device-models when the GOP does not initialize the DSI
>> panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead
>> of muxing it to the PWM controller.
>>
>> This commit makes the DSI code control the SoC GPIOs for panel- and
>> backlight-enable on BYT, when the VBT indicates the SoC should be used
>>
>> for backlight control. It also ensures that the PWM0 pin is muxed to the
>> PWM controller in this case.
>>
>> This fixes the LCD panel not lighting up on various devices when booted
>> with a HDMI monitor connected. This has been tested to fix this on the
>> following devices:
>>
>> Peaq C1010
>> Point of View MOBII TAB-P800W
>> Point of View MOBII TAB-P1005W
>> Terra Pad 1061
>> Yours Y8W81
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsi.h     |  3 +-
>>   drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 63 ++++++++++++++++++++
>>   2 files changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
>> index 675771ea91aa..7481a5aa3084 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
>> @@ -45,8 +45,9 @@ struct intel_dsi {
>>   	struct intel_dsi_host *dsi_hosts[I915_MAX_PORTS];
>>   	intel_wakeref_t io_wakeref[I915_MAX_PORTS];
>>   
>> -	/* GPIO Desc for CRC based Panel control */
>> +	/* GPIO Desc for panel and backlight control */
>>   	struct gpio_desc *gpio_panel;
>> +	struct gpio_desc *gpio_backlight;
>>   
>>   	struct intel_connector *attached_connector;
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> index 847f04eec2a1..bd007d4f86e2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -27,6 +27,8 @@
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/gpio/machine.h>
>>   #include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/pinctrl/machine.h>
>>   #include <linux/slab.h>
>>   
>>   #include <asm/intel-mid.h>
>> @@ -525,11 +527,15 @@ void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
>>   {
>>   	if (seq_id == MIPI_SEQ_POWER_ON && intel_dsi->gpio_panel)
>>   		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
>> +	if (seq_id == MIPI_SEQ_BACKLIGHT_ON && intel_dsi->gpio_backlight)
>> +		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 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);
>> +	if (seq_id == MIPI_SEQ_BACKLIGHT_OFF && intel_dsi->gpio_backlight)
>> +		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 0);
>>   }
>>   
>>   void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
>> @@ -688,6 +694,8 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>   /*
>>    * On some BYT/CHT devs some sequences are incomplete and we need to manually
>>    * control some GPIOs. We need to add a GPIO lookup table before we get these.
>> + * If the GOP did not initialize the panel (HDMI inserted) we may need to also
>> + * change the pinmux for the SoC's PWM0 pin from GPIO to PWM.
>>    */
>>   static struct gpiod_lookup_table pmic_panel_gpio_table = {
>>   	/* Intel GFX is consumer */
>> @@ -699,23 +707,68 @@ static struct gpiod_lookup_table pmic_panel_gpio_table = {
>>   	},
>>   };
>>   
>> +static struct gpiod_lookup_table soc_panel_gpio_table = {
>> +	.dev_id = "0000:00:02.0",
>> +	.table = {
>> +	  GPIO_LOOKUP("INT33FC:01", 10, "backlight", GPIO_ACTIVE_HIGH),
>> +	  GPIO_LOOKUP("INT33FC:01", 11, "panel", GPIO_ACTIVE_HIGH),
>> +	  { },
> 
> Some kind of indent fail here.

Yeah, this was intentional in the previous revision because of the
80 char limit, but this indent hack is no longer necessary, fixed for
the v2 of this set which I'm preparing.

>> +	},
>> +};
>> +
>> +static const struct pinctrl_map soc_pwm_pinctrl_map[] = {
>> +	PIN_MAP_MUX_GROUP("0000:00:02.0", "soc_pwm0", "INT33FC:00",
>> +			  "pwm0_grp", "pwm"),
>> +};
>> +
>>   void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
>>   {
>>   	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;
>>   	enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
>> +	bool want_backlight_gpio = false;
>> +	bool want_panel_gpio = false;
>> +	struct pinctrl *pinctrl;
>> +	int ret;
>>   
>>   	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>   	    (mipi_config->pwm_blc == PPS_BLC_PMIC)) {
>>   		gpiod_add_lookup_table(&pmic_panel_gpio_table);
>> +		want_panel_gpio = true;
>> +	}
>> +
>> +	if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
>> +		gpiod_add_lookup_table(&soc_panel_gpio_table);
>> +		want_panel_gpio = true;
>> +		want_backlight_gpio = true;
>>   
>> +		/* Ensure PWM0 pin is muxed as PWM instead of GPIO */
>> +		ret = pinctrl_register_mappings(soc_pwm_pinctrl_map, 1);
> 
> ARRAY_SIZE()?

Ack, will fix for v2.


> 
>> +		if (ret)
>> +			DRM_ERROR("Failed to register pwm0 pinmux mapping\n");
>> +
>> +		pinctrl = devm_pinctrl_get_select(dev->dev, "soc_pwm0");
>> +		if (IS_ERR(pinctrl))
>> +			DRM_ERROR("Failed to set pinmux to PWM\n");
>> +	}
>> +
>> +	if (want_panel_gpio) {
>>   		intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
>>   		if (IS_ERR(intel_dsi->gpio_panel)) {
>>   			DRM_ERROR("Failed to own gpio for panel control\n");
>>   			intel_dsi->gpio_panel = NULL;
>>   		}
>>   	}
>> +
>> +	if (want_backlight_gpio) {
>> +		intel_dsi->gpio_backlight =
>> +			gpiod_get(dev->dev, "backlight", flags);
>> +		if (IS_ERR(intel_dsi->gpio_backlight)) {
>> +			DRM_ERROR("Failed to own gpio for backlight control\n");
>> +			intel_dsi->gpio_backlight = NULL;
>> +		}
>> +	}
>>   }
>>   
>>   void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
>> @@ -729,7 +782,17 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
>>   		intel_dsi->gpio_panel = NULL;
>>   	}
>>   
>> +	if (intel_dsi->gpio_backlight) {
>> +		gpiod_put(intel_dsi->gpio_backlight);
>> +		intel_dsi->gpio_backlight = NULL;
>> +	}
>> +
>>   	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>   	    (mipi_config->pwm_blc == PPS_BLC_PMIC))
>>   		gpiod_remove_lookup_table(&pmic_panel_gpio_table);
>> +
>> +	if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
> 
> Slightly annoying to have these checks duplicated. Might be cleaner to
> have a few helpers that return the correct tables and just use those in
> both init and cleanup. OTOH those want_*_gpio flags and the pwm stuff is
> would still be a sticking point I suppose. So maybe not cleaner in the
> end after all.

So I tried adding a helper for the if condition, since as you mention
just returning the right table is not really helpful:

static bool intel_dsi_vbt_use_pmic_backlight_ctl(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)) {
        return (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
               mipi_config->pwm_blc == PPS_BLC_PMIC;
}

And copy and paste that for the PPS_BLC_SOC case. The result does not
look a lot better then the original, worse actually IMHO, so I'm going
to keep this as is for v2.

> Looks all right to me:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks.

Regards,

Hans




> 
> 
>> +		pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
>> +		gpiod_remove_lookup_table(&soc_panel_gpio_table);
>> +	}
>>   }
>> -- 
>> 2.23.0
> 

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Linus Walleij]

On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Linus, this series starts with the already discussed pinctrl change to
> export the function to unregister a pinctrl-map. We can either merge this
> through drm-intel, or you could pick it up and then provide an immutable
> branch with it for merging into drm-intel-next. Which option do you prefer?

I have created an immutable branch with these changes and pulled it
to my "devel" branch for v5.6:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-pinctrl-unreg-mappings

Please pull this in and put the other patches on top of that.

I had a bit of mess in my subsystems last kernel cycle so I
want to avoid that by strictly including all larger commits
in my trees.

Yours,
Linus Walleij

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Hans de Goede]

Hi,

On 16-12-2019 11:26, Linus Walleij wrote:
> On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Linus, this series starts with the already discussed pinctrl change to
>> export the function to unregister a pinctrl-map. We can either merge this
>> through drm-intel, or you could pick it up and then provide an immutable
>> branch with it for merging into drm-intel-next. Which option do you prefer?
> 
> I have created an immutable branch with these changes and pulled it
> to my "devel" branch for v5.6:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-pinctrl-unreg-mappings
> 
> Please pull this in and put the other patches on top of that.

Great, thank you.

Jani, a question about how I am supposed to push this (when this has been
also reviewed by some i915 devs and passes CI), can I just do:

dim backmerge drm-intel-next-queued linux-pinctrl/ib-pinctrl-unreg-mappings
cat other-patches | dim apply-branch drm-intel-next-queued
dim push-branch drm-intel-next-queued

Or I guess that dim backmerge is reserved for other drm branches only and I
should do:

dim checkout drm-intel-next-queued
git merge linux-pinctrl/ib-pinctrl-unreg-mappings
cat other-patches | dim apply-branch drm-intel-next-queued
dim push-branch drm-intel-next-queued

Or should I leave merging linux-pinctrl/ib-pinctrl-unreg-mappings into
drm-intel-next-queued up to you?

> I had a bit of mess in my subsystems last kernel cycle so I
> want to avoid that by strictly including all larger commits
> in my trees.

I understand.

Regards,

Hans

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Hans de Goede]

Hi,

On 16-12-2019 11:26, Linus Walleij wrote:
> On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Linus, this series starts with the already discussed pinctrl change to
>> export the function to unregister a pinctrl-map. We can either merge this
>> through drm-intel, or you could pick it up and then provide an immutable
>> branch with it for merging into drm-intel-next. Which option do you prefer?
> 
> I have created an immutable branch with these changes and pulled it
> to my "devel" branch for v5.6:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-pinctrl-unreg-mappings

Ugh, taking one last look at the "pinctrl: Export pinctrl_unregister_mappings"
patch it is no good, sorry.

I just realized that if the mapping has been dupped, the if (maps_node->maps == map)
check will never be true, because maps_node->maps is the return value from kmemdup
and map is the map originally passed in while registering.

Linus, can you please drop this from your -next ?

So I see 2 options:
1) Add an orig_map member to maps_node and use that in the comparison,
this is IMHO somewhat ugly

2) Add a new pinctrl_register_mappings_no_dup helper and document in
pinctrl_unregister_mappings kdoc that it can only be used together
with the no_dup variant.

I believe that 2 is by far the best option. Linus do you agree or
do you have any other suggestions?

Regards,

Hans

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Linus Walleij]

On Mon, Dec 16, 2019 at 12:11 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Ugh, taking one last look at the "pinctrl: Export pinctrl_unregister_mappings"
> patch it is no good, sorry.

Ooops!

> Linus, can you please drop this from your -next ?

Sure, done.

> So I see 2 options:
> 1) Add an orig_map member to maps_node and use that in the comparison,
> this is IMHO somewhat ugly
>
> 2) Add a new pinctrl_register_mappings_no_dup helper and document in
> pinctrl_unregister_mappings kdoc that it can only be used together
> with the no_dup variant.
>
> I believe that 2 is by far the best option. Linus do you agree or
> do you have any other suggestions?

What about (3) look for all calls to pinctrl_register_mappings()
in the kernel.

Hey it is 2 places in total:
arch/arm/mach-u300/core.c:      pinctrl_register_mappings(u300_pinmux_map,
drivers/pinctrl/cirrus/pinctrl-madera-core.c:           ret =
pinctrl_register_mappings(pdata->gpio_configs,

Delete  __initdata from the u300 table, the other one seems
safe. Fold this into your patch.

Go with the original idea.

Yours,
Linus Walleij

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Hans de Goede]

Hi,

On 16-12-2019 13:16, Linus Walleij wrote:
> On Mon, Dec 16, 2019 at 12:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Ugh, taking one last look at the "pinctrl: Export pinctrl_unregister_mappings"
>> patch it is no good, sorry.
> 
> Ooops!
> 
>> Linus, can you please drop this from your -next ?
> 
> Sure, done.
> 
>> So I see 2 options:
>> 1) Add an orig_map member to maps_node and use that in the comparison,
>> this is IMHO somewhat ugly
>>
>> 2) Add a new pinctrl_register_mappings_no_dup helper and document in
>> pinctrl_unregister_mappings kdoc that it can only be used together
>> with the no_dup variant.
>>
>> I believe that 2 is by far the best option. Linus do you agree or
>> do you have any other suggestions?
> 
> What about (3) look for all calls to pinctrl_register_mappings()
> in the kernel.
> 
> Hey it is 2 places in total:
> arch/arm/mach-u300/core.c:      pinctrl_register_mappings(u300_pinmux_map,
> drivers/pinctrl/cirrus/pinctrl-madera-core.c:           ret =
> pinctrl_register_mappings(pdata->gpio_configs,
> 
> Delete  __initdata from the u300 table, the other one seems
> safe. Fold this into your patch.
> 
> Go with the original idea.

That indeed sounds like a cleaner solution I will prepare a new version of
the patch (and this series for the i915 CI) with this approach.

Thanks & Regards,

Hans

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Andy Shevchenko]

On Sun, Dec 15, 2019 at 05:38:05PM +0100, Hans de Goede wrote:
> Hi All,
> 
> This is a new (completely rewritten) version of my patches to make the
> i915 code control the SoC panel- and backlight-enable GPIOs on Bay Trail
> devices when the VBT indicates that the SoC should be used for backlight
> control. This fixes the panel not lighting up on various devices when
> booted with a HDMI monitor connected, in which case the firmware skips
> initializing the panel as it inits the HDMI instead.
> 
> This series has been tested on; and fixes this issue on; the following models:
> 
> Peaq C1010
> Point of View MOBII TAB-P800W
> Point of View MOBII TAB-P1005W
> Terra Pad 1061
> Thundersoft TST178
> Yours Y8W81
> 
> Linus, this series starts with the already discussed pinctrl change to
> export the function to unregister a pinctrl-map. We can either merge this
> through drm-intel, or you could pick it up and then provide an immutable
> branch with it for merging into drm-intel-next. Which option do you prefer?
> 
> Lee, I know you don't like this, but unfortunately this series introcudes
> some (other) changes to drivers/mfd/intel_soc_pmic_core.c. The GPIO subsys
> allows only one mapping-table per consumer, so in hindsight adding the code
> which adds the mapping for the PMIC panel-enable pin to the PMIC mfd driver
> was a mistake, as the PMIC code is a provider where as mapping-tables are
> per consumer. The 4th patch fixes this by moving the mapping-table to the
> i915 code, so that we can also add mappings for some of the pins on the SoC
> itself. Since this whole series makes change to the i915 code I plan to
> merge this mfd change to the drm-intel tree.

FWIW, Lee, I believe there will be no (significant) changes in the driver Hans
touched. For the record it seems only Hans is touching drivers for old Intel
platforms (such as Baytrail and Cherryview).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT

[Lee Jones]

On Mon, 16 Dec 2019, Andy Shevchenko wrote:

> On Sun, Dec 15, 2019 at 05:38:05PM +0100, Hans de Goede wrote:
> > Hi All,
> > 
> > This is a new (completely rewritten) version of my patches to make the
> > i915 code control the SoC panel- and backlight-enable GPIOs on Bay Trail
> > devices when the VBT indicates that the SoC should be used for backlight
> > control. This fixes the panel not lighting up on various devices when
> > booted with a HDMI monitor connected, in which case the firmware skips
> > initializing the panel as it inits the HDMI instead.
> > 
> > This series has been tested on; and fixes this issue on; the following models:
> > 
> > Peaq C1010
> > Point of View MOBII TAB-P800W
> > Point of View MOBII TAB-P1005W
> > Terra Pad 1061
> > Thundersoft TST178
> > Yours Y8W81
> > 
> > Linus, this series starts with the already discussed pinctrl change to
> > export the function to unregister a pinctrl-map. We can either merge this
> > through drm-intel, or you could pick it up and then provide an immutable
> > branch with it for merging into drm-intel-next. Which option do you prefer?
> > 
> > Lee, I know you don't like this, but unfortunately this series introcudes
> > some (other) changes to drivers/mfd/intel_soc_pmic_core.c. The GPIO subsys
> > allows only one mapping-table per consumer, so in hindsight adding the code
> > which adds the mapping for the PMIC panel-enable pin to the PMIC mfd driver
> > was a mistake, as the PMIC code is a provider where as mapping-tables are
> > per consumer. The 4th patch fixes this by moving the mapping-table to the
> > i915 code, so that we can also add mappings for some of the pins on the SoC
> > itself. Since this whole series makes change to the i915 code I plan to
> > merge this mfd change to the drm-intel tree.
> 
> FWIW, Lee, I believe there will be no (significant) changes in the driver Hans
> touched. For the record it seems only Hans is touching drivers for old Intel
> platforms (such as Baytrail and Cherryview).

More exceptions, yay!

Again, in *this* case, it's probably fine.  What I want to know is;
what happens when it's not fine?  What happens when you or someone
else starts changing MFD and DRM on more active files?  Then I will
have to insist on an immutable branch.  So it would be better for the
DRM tree to be able to handle that use-case sooner rather than later,
regardless of who has the most churn.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog