Hi All,
Here is v2 of my patch-series 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 the main change in v2 is the discussed fixing of the patch to export pinctrl_unregister_mappings. Can you please provide a new immutable branch with the new version (assuming the new version is ok)?
Another change on the version is the use of intel_dsi_get_hw_state() to check if the panel is on instead of relying on the current_mode pointer in "[PATCH v2 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off (v2)".
Other then that there are some small style tweaks addressing comments from Andy and Ville.
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
Currently only the drivers/pinctrl/devicetree.c code allows registering pinctrl-mappings which may later be unregistered, all other mappings are assumed to be permanent.
Non-dt platforms may also want 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.
To allow unregistering the mappings the devicetree code uses 2 internal functions: pinctrl_register_map and pinctrl_unregister_map.
pinctrl_register_map allows the devicetree code to tell the core to not memdup the mappings as it retains ownership of them and pinctrl_unregister_map does the unregistering, note this only works when the mappings where not memdupped.
The only code relying on the memdup/shallow-copy done by pinctrl_register_mappings is arch/arm/mach-u300/core.c this commit replaces the __initdata with const, so that the shallow-copy is no longer necessary.
After that we can get rid of the internal pinctrl_unregister_map function and just use pinctrl_register_mappings directly everywhere.
This commit also renames pinctrl_unregister_map to pinctrl_unregister_mappings so that its naming matches its pinctrl_register_mappings counter-part and exports it.
Together these 2 changes will allow non-dt platform code to register pinctrl-mappings from modules without breaking things on module unload (as they can now unregister the mapping on unload).
Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v2: -Drop __initdata from arch/arm/mach-u300/core.c pinctrl-map, so that we can drop the dup behavior for non device-tree callers -Stop memdupping the pinctrl-maps in some cases, remove all code for dealing with dupped maps, including the extra coded added for this in v1 of this patch -Drop the private (non-dupping) pinctrl_register_map function, now that our public pinctrl_register_mappings does not dup we can simply use it everywhere --- arch/arm/mach-u300/core.c | 2 +- drivers/pinctrl/core.c | 41 +++++++++++++-------------------- drivers/pinctrl/core.h | 4 ---- drivers/pinctrl/devicetree.c | 4 ++-- include/linux/pinctrl/machine.h | 5 ++++ 5 files changed, 24 insertions(+), 32 deletions(-)
diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c index a79fa3b0c8ed..a1694d977ec9 100644 --- a/arch/arm/mach-u300/core.c +++ b/arch/arm/mach-u300/core.c @@ -201,7 +201,7 @@ static unsigned long pin_highz_conf[] = { };
/* Pin control settings */ -static struct pinctrl_map __initdata u300_pinmux_map[] = { +static const struct pinctrl_map u300_pinmux_map[] = { /* anonymous maps for chip power and EMIFs */ PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-u300", NULL, "power"), PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-u300", NULL, "emif0"), diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 2bbd8ee93507..b0eea728455d 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1376,8 +1376,15 @@ void devm_pinctrl_put(struct pinctrl *p) } EXPORT_SYMBOL_GPL(devm_pinctrl_put);
-int pinctrl_register_map(const struct pinctrl_map *maps, unsigned num_maps, - bool dup) +/** + * pinctrl_register_mappings() - register a set of pin controller mappings + * @maps: the pincontrol mappings table to register. Note the pinctrl-core + * keeps a reference to the passed in maps, so they should _not_ be + * marked with __initdata. + * @num_maps: the number of maps in the mapping table + */ +int pinctrl_register_mappings(const struct pinctrl_map *maps, + unsigned num_maps) { int i, ret; struct pinctrl_maps *maps_node; @@ -1430,17 +1437,8 @@ int pinctrl_register_map(const struct pinctrl_map *maps, unsigned num_maps, if (!maps_node) return -ENOMEM;
+ maps_node->maps = maps; maps_node->num_maps = num_maps; - if (dup) { - maps_node->maps = kmemdup(maps, sizeof(*maps) * num_maps, - GFP_KERNEL); - if (!maps_node->maps) { - kfree(maps_node); - return -ENOMEM; - } - } else { - maps_node->maps = maps; - }
mutex_lock(&pinctrl_maps_mutex); list_add_tail(&maps_node->node, &pinctrl_maps); @@ -1448,22 +1446,14 @@ int pinctrl_register_map(const struct pinctrl_map *maps, unsigned num_maps,
return 0; } +EXPORT_SYMBOL_GPL(pinctrl_register_mappings);
/** - * pinctrl_register_mappings() - register a set of pin controller mappings - * @maps: the pincontrol mappings table to register. This should probably be - * marked with __initdata so it can be discarded after boot. This - * function will perform a shallow copy for the mapping entries. - * @num_maps: the number of maps in the mapping table + * pinctrl_unregister_mappings() - unregister a set of pin controller mappings + * @maps: the pincontrol mappings table passed to pinctrl_register_mappings() + * when registering the mappings. */ -int pinctrl_register_mappings(const struct pinctrl_map *maps, - unsigned num_maps) -{ - return pinctrl_register_map(maps, num_maps, true); -} -EXPORT_SYMBOL_GPL(pinctrl_register_mappings); - -void pinctrl_unregister_map(const struct pinctrl_map *map) +void pinctrl_unregister_mappings(const struct pinctrl_map *map) { struct pinctrl_maps *maps_node;
@@ -1478,6 +1468,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..840103c40c14 100644 --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -236,10 +236,6 @@ extern struct pinctrl_gpio_range * pinctrl_find_gpio_range_from_pin_nolock(struct pinctrl_dev *pctldev, unsigned int pin);
-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..9357f7c46cf3 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); @@ -92,7 +92,7 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, dt_map->num_maps = num_maps; list_add_tail(&dt_map->node, &p->dt_maps);
- return pinctrl_register_map(map, num_maps, false); + return pinctrl_register_mappings(map, num_maps);
err_free_map: dt_free_map(pctldev, 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) { }
On Mon, Dec 16, 2019 at 9:51 PM Hans de Goede hdegoede@redhat.com wrote:
Currently only the drivers/pinctrl/devicetree.c code allows registering pinctrl-mappings which may later be unregistered, all other mappings are assumed to be permanent.
Non-dt platforms may also want 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.
To allow unregistering the mappings the devicetree code uses 2 internal functions: pinctrl_register_map and pinctrl_unregister_map.
pinctrl_register_map allows the devicetree code to tell the core to not memdup the mappings as it retains ownership of them and pinctrl_unregister_map does the unregistering, note this only works when the mappings where not memdupped.
The only code relying on the memdup/shallow-copy done by pinctrl_register_mappings is arch/arm/mach-u300/core.c this commit replaces the __initdata with const, so that the shallow-copy is no longer necessary.
After that we can get rid of the internal pinctrl_unregister_map function and just use pinctrl_register_mappings directly everywhere.
This commit also renames pinctrl_unregister_map to pinctrl_unregister_mappings so that its naming matches its pinctrl_register_mappings counter-part and exports it.
Together these 2 changes will allow non-dt platform code to register pinctrl-mappings from modules without breaking things on module unload (as they can now unregister the mapping on unload).
Signed-off-by: Hans de Goede hdegoede@redhat.com
This v2 works fine for me, I applied it to this immutable branch in the pinctrl tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log...
And pulled that into the pinctrl "devel" branch for v5.6.
Please pull this immutable branch into the Intel DRM tree and apply the rest of the stuff on top!
Yours, Linus Walleij
Hi,
On 30-12-2019 14:31, Linus Walleij wrote:
On Mon, Dec 16, 2019 at 9:51 PM Hans de Goede hdegoede@redhat.com wrote:
Currently only the drivers/pinctrl/devicetree.c code allows registering pinctrl-mappings which may later be unregistered, all other mappings are assumed to be permanent.
Non-dt platforms may also want 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.
To allow unregistering the mappings the devicetree code uses 2 internal functions: pinctrl_register_map and pinctrl_unregister_map.
pinctrl_register_map allows the devicetree code to tell the core to not memdup the mappings as it retains ownership of them and pinctrl_unregister_map does the unregistering, note this only works when the mappings where not memdupped.
The only code relying on the memdup/shallow-copy done by pinctrl_register_mappings is arch/arm/mach-u300/core.c this commit replaces the __initdata with const, so that the shallow-copy is no longer necessary.
After that we can get rid of the internal pinctrl_unregister_map function and just use pinctrl_register_mappings directly everywhere.
This commit also renames pinctrl_unregister_map to pinctrl_unregister_mappings so that its naming matches its pinctrl_register_mappings counter-part and exports it.
Together these 2 changes will allow non-dt platform code to register pinctrl-mappings from modules without breaking things on module unload (as they can now unregister the mapping on unload).
Signed-off-by: Hans de Goede hdegoede@redhat.com
This v2 works fine for me, I applied it to this immutable branch in the pinctrl tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log...
And pulled that into the pinctrl "devel" branch for v5.6.
Please pull this immutable branch into the Intel DRM tree and apply the rest of the stuff on top!
Great, thank you!
Regards,
Hans
p.s.
Happy New year everyone.
On Mon, 30 Dec 2019, Linus Walleij linus.walleij@linaro.org wrote:
On Mon, Dec 16, 2019 at 9:51 PM Hans de Goede hdegoede@redhat.com wrote:
Currently only the drivers/pinctrl/devicetree.c code allows registering pinctrl-mappings which may later be unregistered, all other mappings are assumed to be permanent.
Non-dt platforms may also want 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.
To allow unregistering the mappings the devicetree code uses 2 internal functions: pinctrl_register_map and pinctrl_unregister_map.
pinctrl_register_map allows the devicetree code to tell the core to not memdup the mappings as it retains ownership of them and pinctrl_unregister_map does the unregistering, note this only works when the mappings where not memdupped.
The only code relying on the memdup/shallow-copy done by pinctrl_register_mappings is arch/arm/mach-u300/core.c this commit replaces the __initdata with const, so that the shallow-copy is no longer necessary.
After that we can get rid of the internal pinctrl_unregister_map function and just use pinctrl_register_mappings directly everywhere.
This commit also renames pinctrl_unregister_map to pinctrl_unregister_mappings so that its naming matches its pinctrl_register_mappings counter-part and exports it.
Together these 2 changes will allow non-dt platform code to register pinctrl-mappings from modules without breaking things on module unload (as they can now unregister the mapping on unload).
Signed-off-by: Hans de Goede hdegoede@redhat.com
This v2 works fine for me, I applied it to this immutable branch in the pinctrl tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log...
And pulled that into the pinctrl "devel" branch for v5.6.
Please pull this immutable branch into the Intel DRM tree and apply the rest of the stuff on top!
Thanks, pulled to drm-intel-next-queued.
BR, Jani.
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.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com 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..8be7d6c507aa 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 403fb09fcb63..c1edd8857af0 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); }
@@ -1867,20 +1857,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);
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.
Changes in v2: - Call intel_dsi_get_hw_state() to check if the panel is on instead of relying on the current_mode pointer
Reviewed-by: Linus Walleij linus.walleij@linaro.org 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 | 4 +++- 3 files changed, 7 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 8be7d6c507aa..4210f449553e 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 c1edd8857af0..d0efee09c593 100644 --- a/drivers/gpu/drm/i915/display/vlv_dsi.c +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c @@ -1759,6 +1759,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) struct drm_connector *connector; struct drm_display_mode *current_mode, *fixed_mode; enum port port; + enum pipe pipe;
DRM_DEBUG_KMS("\n");
@@ -1857,7 +1858,8 @@ 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, + intel_dsi_get_hw_state(intel_encoder, &pipe));
drm_connector_init(dev, connector, &intel_dsi_connector_funcs, DRM_MODE_CONNECTOR_DSI);
On Mon, 16 Dec 2019, 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.
Changes in v2:
- Call intel_dsi_get_hw_state() to check if the panel is on instead of relying on the current_mode pointer
Reviewed-by: Linus Walleij linus.walleij@linaro.org 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 | 4 +++- 3 files changed, 7 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 8be7d6c507aa..4210f449553e 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);diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c index c1edd8857af0..d0efee09c593 100644 --- a/drivers/gpu/drm/i915/display/vlv_dsi.c +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c @@ -1759,6 +1759,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) struct drm_connector *connector; struct drm_display_mode *current_mode, *fixed_mode; enum port port;
enum pipe pipe;
DRM_DEBUG_KMS("\n");
@@ -1857,7 +1858,8 @@ 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,
intel_dsi_get_hw_state(intel_encoder, &pipe));
Feels a bit scary to call into the hooks before everything is initialized, but this seems safe. Fingers crossed.
Reviewed-by: Jani Nikula jani.nikula@intel.com
drm_connector_init(dev, connector, &intel_dsi_connector_funcs, DRM_MODE_CONNECTOR_DSI);
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.
Acked-by: Linus Walleij linus.walleij@linaro.org Reviewed-by: Andy Shevchenko andriy.shevchenko@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com 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 4210f449553e..89558ccf79c8 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));
On Mon, 16 Dec 2019, 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.
Acked-by: Linus Walleij linus.walleij@linaro.org Reviewed-by: Andy Shevchenko andriy.shevchenko@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com 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(-)
Acked-by: Lee Jones lee.jones@linaro.org
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
Reviewed-by: Linus Walleij linus.walleij@linaro.org Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com 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 | 64 ++++++++++++++++++++ 2 files changed, 66 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 89558ccf79c8..0032161e0f76 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,69 @@ 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, + ARRAY_SIZE(soc_pwm_pinctrl_map)); + 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 +783,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); + } }
dri-devel@lists.freedesktop.org
-
Hans de Goede -
Jani Nikula -
Lee Jones -
Linus Walleij