Hi,
On 10/31/23 17:07, Hans de Goede wrote:
> Hi Andy,
>
> On 10/24/23 18:11, Andy Shevchenko wrote:
>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>>> It's a dirty hack in the driver that pokes GPIO registers behind
>>> the driver's back. Moreoever it might be problematic as simultaneous
>>> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
>>> cherryview: prevent concurrent access to GPIO controllers") for
>>> the details. Taking all this into consideration replace the hack
>>> with proper GPIO APIs being used.
>>
>> Ah, just realised that this won't work if it happens to request to GPIOs with
>> the same index but different communities. I will fix that in v3, but will
>> wait
>> for Hans to test VLV and it might even work in most of the cases on CHV as it
>> seems quite unlikely that the above mentioned assertion is going to happen in
>> real life.
>
> I have added patches 1-5 to my personal tree + a small debug patch on top
> which logs when soc_exec_opaque_gpio() actually gets called.
>
> So these patches will now get run every time I run some tests on
> one my tablets.
>
> I'll get back to you with testing results when I've found a device where
> the new soc_exec_opaque_gpio() actually gets called.
>
> As for the CHT support, I have not added that to my tree yet, I would
> prefer to directly test the correct/fixed patch.
And I hit the "jackpot" on the first device I tried and the code needed
some fixing to actually work, so here is something to fold into v3 to
fix things:
>From 144fae4de91a6b5ed993b1722a07cca679f74cbe Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdego...@redhat.com>
Date: Tue, 31 Oct 2023 17:04:35 +0100
Subject: [PATCH] drm/i915/dsi: Fix GPIO lookup table used by
soc_exec_opaque_gpio()
There already is a GPIO lookup table for device "0000:00:02.0" and
there can be only one GPIO lookup per device.
Instead add an extra empty entry to the GPIO lookup table
registered by intel_dsi_vbt_gpio_init() and use that extra entry
in soc_exec_opaque_gpio().
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 60 ++++++++++----------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8fc82aceae14..70f1d2c411e8 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector
*connector, const char *con_id,
} else {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
con_id, gpio_index,
- value ? GPIOD_OUT_LOW :
- GPIOD_OUT_HIGH);
+ value ? GPIOD_OUT_HIGH :
GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(&dev_priv->drm,
"GPIO index %u request failed (%pe)\n",
@@ -232,26 +231,20 @@ static void soc_exec_gpio(struct intel_connector
*connector, const char *con_id,
}
}
+static struct gpiod_lookup *soc_exec_opaque_gpiod_lookup;
+
static void soc_exec_opaque_gpio(struct intel_connector *connector,
const char *chip, const char *con_id,
u8 gpio_index, bool value)
{
- struct gpiod_lookup_table *lookup;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
- lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
- if (!lookup)
- return;
-
- lookup->dev_id = "0000:00:02.0";
- lookup->table[0] =
+ *soc_exec_opaque_gpiod_lookup =
GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index,
GPIO_ACTIVE_HIGH);
- gpiod_add_lookup_table(lookup);
-
soc_exec_gpio(connector, con_id, gpio_index, value);
- gpiod_remove_lookup_table(lookup);
- kfree(lookup);
+ soc_exec_opaque_gpiod_lookup->key = NULL;
}
static void vlv_exec_gpio(struct intel_connector *connector,
@@ -898,6 +891,7 @@ static struct gpiod_lookup_table pmic_panel_gpio_table = {
.table = {
/* Panel EN/DISABLE */
GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
{ }
},
};
@@ -907,6 +901,15 @@ static struct gpiod_lookup_table soc_panel_gpio_table = {
.table = {
GPIO_LOOKUP("INT33FC:01", 10, "backlight", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("INT33FC:01", 11, "panel", GPIO_ACTIVE_HIGH),
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
+ { }
+ },
+};
+
+static struct gpiod_lookup_table empty_gpio_table = {
+ .dev_id = "0000:00:02.0",
+ .table = {
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
{ }
},
};
@@ -916,6 +919,8 @@ static const struct pinctrl_map soc_pwm_pinctrl_map[] = {
"pwm0_grp", "pwm"),
};
+static struct gpiod_lookup_table *gpiod_lookup_table;
+
void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
{
struct drm_device *dev = intel_dsi->base.base.dev;
@@ -926,16 +931,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi,
bool panel_is_on)
bool want_backlight_gpio = false;
bool want_panel_gpio = false;
struct pinctrl *pinctrl;
- int ret;
+ int i, 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);
+ gpiod_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);
+ gpiod_lookup_table = &soc_panel_gpio_table;
want_panel_gpio = true;
want_backlight_gpio = true;
@@ -952,6 +957,15 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi,
bool panel_is_on)
"Failed to set pinmux to PWM\n");
}
+ if (!gpiod_lookup_table)
+ gpiod_lookup_table = &empty_gpio_table;
+
+ /* Find first empty entry for soc_exec_opaque_gpiod_lookup */
+ for (i = 0; gpiod_lookup_table->table[i].key; i++) { }
+ soc_exec_opaque_gpiod_lookup = &gpiod_lookup_table->table[i];
+
+ gpiod_add_lookup_table(gpiod_lookup_table);
+
if (want_panel_gpio) {
intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
if (IS_ERR(intel_dsi->gpio_panel)) {
@@ -974,11 +988,6 @@ 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 intel_connector *connector = intel_dsi->attached_connector;
- struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
-
if (intel_dsi->gpio_panel) {
gpiod_put(intel_dsi->gpio_panel);
intel_dsi->gpio_panel = NULL;
@@ -989,12 +998,5 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi
*intel_dsi)
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);
- }
+ gpiod_remove_lookup_table(gpiod_lookup_table);
}
--
2.41.0
Regards,
Hans
