DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
talking to GPIO IP behind the actual driver's back. A second attempt
to fix that is here.
If I understood correctly, my approach should work in the similar way as
the current IOSF GPIO.
Hans, I believe you have some devices that use this piece of code,
is it possible to give a test run on (one of) them?
In v2:
- added a few cleanup patches
- reworked to use dynamic GPIO lookup tables
- converted CHV as well
Andy Shevchenko (7):
drm/i915/dsi: Replace while(1) with one with clear exit condition
drm/i915/dsi: Get rid of redundant 'else'
drm/i915/dsi: Replace check with a (missing) MIPI sequence name
drm/i915/dsi: Extract common soc_gpio_exec() helper
drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
drm/i915/iosf: Drop unused APIs
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 260 +++++++------------
drivers/gpu/drm/i915/vlv_sideband.c | 17 --
drivers/gpu/drm/i915/vlv_sideband.h | 3 -
3 files changed, 96 insertions(+), 184 deletions(-)
--
2.40.0.1.gaa8946217a0b
In the snippets like the following
if (...)
return / goto / break / continue ...;
else
...
the 'else' is redundant. Get rid of it.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 58 ++++++++++----------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index a6a6f1814967..22b89e68e6de 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -142,7 +142,7 @@ static enum port intel_dsi_seq_port_to_port(struct intel_dsi *intel_dsi,
if (seq_port) {
if (intel_dsi->ports & BIT(PORT_B))
return PORT_B;
- else if (intel_dsi->ports & BIT(PORT_C))
+ if (intel_dsi->ports & BIT(PORT_C))
return PORT_C;
}
@@ -675,8 +675,8 @@ static const char *sequence_name(enum mipi_seq seq_id)
{
if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
return seq_name[seq_id];
- else
- return "(unknown)";
+
+ return "(unknown)";
}
static void intel_dsi_vbt_exec(struct intel_dsi *intel_dsi,
@@ -870,36 +870,34 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
* multiply by 100 to preserve remainder
*/
if (intel_dsi->video_mode == BURST_MODE) {
- if (mipi_config->target_burst_mode_freq) {
- u32 bitrate = intel_dsi_bitrate(intel_dsi);
+ u32 bitrate;
- /*
- * Sometimes the VBT contains a slightly lower clock,
- * then the bitrate we have calculated, in this case
- * just replace it with the calculated bitrate.
- */
- if (mipi_config->target_burst_mode_freq < bitrate &&
- intel_fuzzy_clock_check(
- mipi_config->target_burst_mode_freq,
- bitrate))
- mipi_config->target_burst_mode_freq = bitrate;
-
- if (mipi_config->target_burst_mode_freq < bitrate) {
- drm_err(&dev_priv->drm,
- "Burst mode freq is less than computed\n");
- return false;
- }
-
- burst_mode_ratio = DIV_ROUND_UP(
- mipi_config->target_burst_mode_freq * 100,
- bitrate);
-
- intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * burst_mode_ratio, 100);
- } else {
- drm_err(&dev_priv->drm,
- "Burst mode target is not set\n");
+ if (mipi_config->target_burst_mode_freq == 0) {
+ drm_err(&dev_priv->drm, "Burst mode target is not set\n");
return false;
}
+
+ bitrate = intel_dsi_bitrate(intel_dsi);
+
+ /*
+ * Sometimes the VBT contains a slightly lower clock, then
+ * the bitrate we have calculated, in this case just replace it
+ * with the calculated bitrate.
+ */
+ if (mipi_config->target_burst_mode_freq < bitrate &&
+ intel_fuzzy_clock_check(mipi_config->target_burst_mode_freq,
+ bitrate))
+ mipi_config->target_burst_mode_freq = bitrate;
+
+ if (mipi_config->target_burst_mode_freq < bitrate) {
+ drm_err(&dev_priv->drm, "Burst mode freq is less than computed\n");
+ return false;
+ }
+
+ burst_mode_ratio =
+ DIV_ROUND_UP(mipi_config->target_burst_mode_freq * 100, bitrate);
+
+ intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * burst_mode_ratio, 100);
} else
burst_mode_ratio = 100;
--
2.40.0.1.gaa8946217a0b
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 40ecab551232 ("pinctrl:
baytrail: Really serialize all register accesses") for the details.
Taking all this into consideration replace the hack with proper
GPIO APIs being used.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 98 ++++++--------------
1 file changed, 28 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 3fb85b6d320e..8fc82aceae14 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -55,43 +55,6 @@
#define MIPI_VIRTUAL_CHANNEL_SHIFT 1
#define MIPI_PORT_SHIFT 3
-/* base offsets for gpio pads */
-#define VLV_GPIO_NC_0_HV_DDI0_HPD 0x4130
-#define VLV_GPIO_NC_1_HV_DDI0_DDC_SDA 0x4120
-#define VLV_GPIO_NC_2_HV_DDI0_DDC_SCL 0x4110
-#define VLV_GPIO_NC_3_PANEL0_VDDEN 0x4140
-#define VLV_GPIO_NC_4_PANEL0_BKLTEN 0x4150
-#define VLV_GPIO_NC_5_PANEL0_BKLTCTL 0x4160
-#define VLV_GPIO_NC_6_HV_DDI1_HPD 0x4180
-#define VLV_GPIO_NC_7_HV_DDI1_DDC_SDA 0x4190
-#define VLV_GPIO_NC_8_HV_DDI1_DDC_SCL 0x4170
-#define VLV_GPIO_NC_9_PANEL1_VDDEN 0x4100
-#define VLV_GPIO_NC_10_PANEL1_BKLTEN 0x40E0
-#define VLV_GPIO_NC_11_PANEL1_BKLTCTL 0x40F0
-
-#define VLV_GPIO_PCONF0(base_offset) (base_offset)
-#define VLV_GPIO_PAD_VAL(base_offset) ((base_offset) + 8)
-
-struct gpio_map {
- u16 base_offset;
- bool init;
-};
-
-static struct gpio_map vlv_gpio_table[] = {
- { VLV_GPIO_NC_0_HV_DDI0_HPD },
- { VLV_GPIO_NC_1_HV_DDI0_DDC_SDA },
- { VLV_GPIO_NC_2_HV_DDI0_DDC_SCL },
- { VLV_GPIO_NC_3_PANEL0_VDDEN },
- { VLV_GPIO_NC_4_PANEL0_BKLTEN },
- { VLV_GPIO_NC_5_PANEL0_BKLTCTL },
- { VLV_GPIO_NC_6_HV_DDI1_HPD },
- { VLV_GPIO_NC_7_HV_DDI1_DDC_SDA },
- { VLV_GPIO_NC_8_HV_DDI1_DDC_SCL },
- { VLV_GPIO_NC_9_PANEL1_VDDEN },
- { VLV_GPIO_NC_10_PANEL1_BKLTEN },
- { VLV_GPIO_NC_11_PANEL1_BKLTCTL },
-};
-
struct i2c_adapter_lookup {
u16 slave_addr;
struct intel_dsi *intel_dsi;
@@ -269,52 +232,47 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
}
}
+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;
+
+ lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
+ if (!lookup)
+ return;
+
+ lookup->dev_id = "0000:00:02.0";
+ lookup->table[0] =
+ 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);
+}
+
static void vlv_exec_gpio(struct intel_connector *connector,
u8 gpio_source, u8 gpio_index, bool value)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
- struct gpio_map *map;
- u16 pconf0, padval;
- u32 tmp;
- u8 port;
- if (gpio_index >= ARRAY_SIZE(vlv_gpio_table)) {
- drm_dbg_kms(&dev_priv->drm, "unknown gpio index %u\n",
- gpio_index);
- return;
- }
-
- map = &vlv_gpio_table[gpio_index];
-
- if (connector->panel.vbt.dsi.seq_version >= 3) {
- /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
- port = IOSF_PORT_GPIO_NC;
- } else {
- if (gpio_source == 0) {
- port = IOSF_PORT_GPIO_NC;
- } else if (gpio_source == 1) {
+ /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
+ if (connector->panel.vbt.dsi.seq_version < 3) {
+ if (gpio_source == 1) {
drm_dbg_kms(&dev_priv->drm, "SC gpio not supported\n");
return;
- } else {
+ }
+ if (gpio_source > 1) {
drm_dbg_kms(&dev_priv->drm,
"unknown gpio source %u\n", gpio_source);
return;
}
}
- pconf0 = VLV_GPIO_PCONF0(map->base_offset);
- padval = VLV_GPIO_PAD_VAL(map->base_offset);
-
- vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
- if (!map->init) {
- /* FIXME: remove constant below */
- vlv_iosf_sb_write(dev_priv, port, pconf0, 0x2000CC00);
- map->init = true;
- }
-
- tmp = 0x4 | value;
- vlv_iosf_sb_write(dev_priv, port, padval, tmp);
- vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
+ soc_exec_opaque_gpio(connector, "INT33FC:01", "Panel N", gpio_index, value);
}
static void chv_exec_gpio(struct intel_connector *connector,
--
2.40.0.1.gaa8946217a0b
Hi Andy,
On Tue, Oct 24, 2023 at 06:57:34PM +0300, Andy Shevchenko wrote:
> In the snippets like the following
>
> if (...)
> return / goto / break / continue ...;
> else
> ...
>
> the 'else' is redundant. Get rid of it.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Andi