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 v3:
- incorporated series by Jani
- incorporated couple of precursor patches by Hans
- added Rb tag for used to be first three patches (Andi)
- rebased on top of the above changes
- fixed indexing for multi-community devices, such as Cherry View
In v2:
- added a few cleanup patches
- reworked to use dynamic GPIO lookup tables
- converted CHV as well
Andy Shevchenko (8):
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_set_value() helper
drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO
communities
drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
drm/i915/iosf: Drop unused APIs
Hans de Goede (2):
drm/i915/dsi: Remove GPIO lookup table at the end of
intel_dsi_vbt_gpio_init()
drm/i915/dsi: Fix wrong initial value for GPIOs in bxt_exec_gpio()
Jani Nikula (5):
drm/i915/dsi: assume BXT gpio works for non-native GPIO
drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915
drm/i915/dsi: clarify GPIO exec sequence
drm/i915/dsi: rename platform specific *_exec_gpio() to
*_gpio_set_value()
drm/i915/dsi: bxt/icl GPIO set value do not need gpio source
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 355 +++++++------------
drivers/gpu/drm/i915/vlv_sideband.c | 17 -
drivers/gpu/drm/i915/vlv_sideband.h | 3 -
3 files changed, 137 insertions(+), 238 deletions(-)
--
2.40.0.1.gaa8946217a0b
From: Jani Nikula <[email protected]>
With the various sequence versions and pointer increments interleaved,
it's a bit hard to decipher what's going on. Add separate paths for
different sequence versions.
Cc: Andy Shevchenko <[email protected]>
Cc: Hans de Goede <[email protected]>
Signed-off-by: Jani Nikula <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 31 +++++++++++---------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8b962f2ac475..11073efe26c0 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -456,26 +456,29 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
struct drm_device *dev = intel_dsi->base.base.dev;
struct drm_i915_private *i915 = to_i915(dev);
struct intel_connector *connector = intel_dsi->attached_connector;
- u8 gpio_source, gpio_index = 0, gpio_number;
+ u8 gpio_source = 0, gpio_index = 0, gpio_number;
bool value;
+ int size;
bool native = DISPLAY_VER(i915) >= 11;
- if (connector->panel.vbt.dsi.seq_version >= 3)
- gpio_index = *data++;
+ if (connector->panel.vbt.dsi.seq_version >= 3) {
+ size = 3;
- gpio_number = *data++;
+ gpio_index = data[0];
+ gpio_number = data[1];
+ value = data[2] & BIT(0);
- /* gpio source in sequence v2 only */
- if (connector->panel.vbt.dsi.seq_version == 2)
- gpio_source = (*data >> 1) & 3;
- else
- gpio_source = 0;
+ if (connector->panel.vbt.dsi.seq_version >= 4 && data[2] & BIT(1))
+ native = false;
+ } else {
+ size = 2;
- if (connector->panel.vbt.dsi.seq_version >= 4 && *data & BIT(1))
- native = false;
+ gpio_number = data[0];
+ value = data[1] & BIT(0);
- /* pull up/down */
- value = *data++ & 1;
+ if (connector->panel.vbt.dsi.seq_version == 2)
+ gpio_source = (data[1] >> 1) & 3;
+ }
drm_dbg_kms(&i915->drm, "GPIO index %u, number %u, source %u, native %s, set to %s\n",
gpio_index, gpio_number, gpio_source, str_yes_no(native), str_on_off(value));
@@ -491,7 +494,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
else
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
- return data;
+ return data + size;
}
#ifdef CONFIG_ACPI
--
2.40.0.1.gaa8946217a0b
From: Jani Nikula <[email protected]>
Drop the unused parameter.
Cc: Andy Shevchenko <[email protected]>
Cc: Hans de Goede <[email protected]>
Signed-off-by: Jani Nikula <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index f977d63a0ad4..4af43cf3cee0 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -346,7 +346,7 @@ static void chv_gpio_set_value(struct intel_connector *connector,
}
static void bxt_gpio_set_value(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+ u8 gpio_index, bool value)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -486,13 +486,13 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(i915, gpio_number, value);
else if (DISPLAY_VER(i915) >= 11)
- bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
+ bxt_gpio_set_value(connector, gpio_index, value);
else if (IS_VALLEYVIEW(i915))
vlv_gpio_set_value(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(i915))
chv_gpio_set_value(connector, gpio_source, gpio_number, value);
else
- bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
+ bxt_gpio_set_value(connector, gpio_index, value);
return data + size;
}
--
2.40.0.1.gaa8946217a0b
From: Jani Nikula <[email protected]>
Purely a guess. Drop the nop function.
Cc: Andy Shevchenko <[email protected]>
Cc: Hans de Goede <[email protected]>
Signed-off-by: Jani Nikula <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 24b2cbcfc1ef..b2c0cc11f8c1 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -372,14 +372,6 @@ static void bxt_exec_gpio(struct intel_connector *connector,
gpiod_set_value(gpio_desc, value);
}
-static void icl_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);
-
- drm_dbg_kms(&dev_priv->drm, "Skipping ICL GPIO element execution\n");
-}
-
enum {
MIPI_RESET_1 = 0,
MIPI_AVDD_EN_1,
@@ -491,7 +483,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(dev_priv, gpio_number, value);
else if (DISPLAY_VER(dev_priv) >= 11)
- icl_exec_gpio(connector, gpio_source, gpio_index, value);
+ bxt_exec_gpio(connector, gpio_source, gpio_index, value);
else if (IS_VALLEYVIEW(dev_priv))
vlv_exec_gpio(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(dev_priv))
--
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.
Reviewed-by: Andi Shyti <[email protected]>
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 290a112f1b63..4ed5ede9ec5b 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;
}
@@ -670,8 +670,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,
@@ -865,36 +865,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 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.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +++++---------------
1 file changed, 10 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index b1736c1301ea..ffc65c943b11 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
#define CHV_GPIO_IDX_START_SW 100
#define CHV_GPIO_IDX_START_SE 198
-#define CHV_VBT_MAX_PINS_PER_FMLY 15
-
-#define CHV_GPIO_PAD_CFG0(f, i) (0x4400 + (f) * 0x400 + (i) * 8)
-#define CHV_GPIO_GPIOEN (1 << 15)
-#define CHV_GPIO_GPIOCFG_GPIO (0 << 8)
-#define CHV_GPIO_GPIOCFG_GPO (1 << 8)
-#define CHV_GPIO_GPIOCFG_GPI (2 << 8)
-#define CHV_GPIO_GPIOCFG_HIZ (3 << 8)
-#define CHV_GPIO_GPIOTXSTATE(state) ((!!(state)) << 1)
-
-#define CHV_GPIO_PAD_CFG1(f, i) (0x4400 + (f) * 0x400 + (i) * 8 + 4)
-#define CHV_GPIO_CFGLOCK (1 << 31)
-
/* ICL DSI Display GPIO Pins */
#define ICL_GPIO_DDSP_HPD_A 0
#define ICL_GPIO_L_VDDEN_1 1
@@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector *connector,
u8 gpio_source, u8 gpio_index, bool value)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
- u16 cfg0, cfg1;
- u16 family_num;
- u8 port;
if (connector->panel.vbt.dsi.seq_version >= 3) {
if (gpio_index >= CHV_GPIO_IDX_START_SE) {
/* XXX: it's unclear whether 255->57 is part of SE. */
- gpio_index -= CHV_GPIO_IDX_START_SE;
- port = CHV_IOSF_PORT_GPIO_SE;
+ soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
+ gpio_index - CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
- gpio_index -= CHV_GPIO_IDX_START_SW;
- port = CHV_IOSF_PORT_GPIO_SW;
+ soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
+ gpio_index - CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
- gpio_index -= CHV_GPIO_IDX_START_E;
- port = CHV_IOSF_PORT_GPIO_E;
+ soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
+ gpio_index - CHV_GPIO_IDX_START_E, value);
} else {
- port = CHV_IOSF_PORT_GPIO_N;
+ soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
+ gpio_index - CHV_GPIO_IDX_START_N, value);
}
} else {
/* XXX: The spec is unclear about CHV GPIO on seq v2 */
@@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector *connector,
return;
}
- port = CHV_IOSF_PORT_GPIO_N;
+ soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
+ gpio_index - CHV_GPIO_IDX_START_N, value);
}
-
- family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
- gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
-
- cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
- cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
-
- vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
- vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
- vlv_iosf_sb_write(dev_priv, port, cfg0,
- CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
- CHV_GPIO_GPIOTXSTATE(value));
- vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
}
static void bxt_gpio_set_value(struct intel_connector *connector,
--
2.40.0.1.gaa8946217a0b
On Thu, Nov 02, 2023 at 05:12:13PM +0200, Andy Shevchenko wrote:
> 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?
Subject should be "3rd attempt ..." :-)
> In v3:
> - incorporated series by Jani
> - incorporated couple of precursor patches by Hans
> - added Rb tag for used to be first three patches (Andi)
> - rebased on top of the above changes
> - fixed indexing for multi-community devices, such as Cherry View
>
> In v2:
> - added a few cleanup patches
> - reworked to use dynamic GPIO lookup tables
> - converted CHV as well
--
With Best Regards,
Andy Shevchenko
On Thu, 02 Nov 2023, Andy Shevchenko <[email protected]> wrote:
> 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 v3:
> - incorporated series by Jani
> - incorporated couple of precursor patches by Hans
> - added Rb tag for used to be first three patches (Andi)
> - rebased on top of the above changes
> - fixed indexing for multi-community devices, such as Cherry View
>
> In v2:
> - added a few cleanup patches
> - reworked to use dynamic GPIO lookup tables
> - converted CHV as well
>
> Andy Shevchenko (8):
> 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_set_value() helper
> drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
> drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO
> communities
> drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
> drm/i915/iosf: Drop unused APIs
>
> Hans de Goede (2):
> drm/i915/dsi: Remove GPIO lookup table at the end of
> intel_dsi_vbt_gpio_init()
> drm/i915/dsi: Fix wrong initial value for GPIOs in bxt_exec_gpio()
Assuming it all still works, and I do trust Hans' testing here quite a
bit, the above is
Acked-by: Jani Nikula <[email protected]>
Thanks for doing this!
>
> Jani Nikula (5):
> drm/i915/dsi: assume BXT gpio works for non-native GPIO
> drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915
> drm/i915/dsi: clarify GPIO exec sequence
> drm/i915/dsi: rename platform specific *_exec_gpio() to
> *_gpio_set_value()
> drm/i915/dsi: bxt/icl GPIO set value do not need gpio source
>
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 355 +++++++------------
> drivers/gpu/drm/i915/vlv_sideband.c | 17 -
> drivers/gpu/drm/i915/vlv_sideband.h | 3 -
> 3 files changed, 137 insertions(+), 238 deletions(-)
--
Jani Nikula, Intel
Hi,
On 11/2/23 16:12, 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.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +++++---------------
> 1 file changed, 10 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index b1736c1301ea..ffc65c943b11 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
> #define CHV_GPIO_IDX_START_SW 100
> #define CHV_GPIO_IDX_START_SE 198
>
> -#define CHV_VBT_MAX_PINS_PER_FMLY 15
> -
> -#define CHV_GPIO_PAD_CFG0(f, i) (0x4400 + (f) * 0x400 + (i) * 8)
> -#define CHV_GPIO_GPIOEN (1 << 15)
> -#define CHV_GPIO_GPIOCFG_GPIO (0 << 8)
> -#define CHV_GPIO_GPIOCFG_GPO (1 << 8)
> -#define CHV_GPIO_GPIOCFG_GPI (2 << 8)
> -#define CHV_GPIO_GPIOCFG_HIZ (3 << 8)
> -#define CHV_GPIO_GPIOTXSTATE(state) ((!!(state)) << 1)
> -
> -#define CHV_GPIO_PAD_CFG1(f, i) (0x4400 + (f) * 0x400 + (i) * 8 + 4)
> -#define CHV_GPIO_CFGLOCK (1 << 31)
> -
> /* ICL DSI Display GPIO Pins */
> #define ICL_GPIO_DDSP_HPD_A 0
> #define ICL_GPIO_L_VDDEN_1 1
> @@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector *connector,
> u8 gpio_source, u8 gpio_index, bool value)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> - u16 cfg0, cfg1;
> - u16 family_num;
> - u8 port;
>
> if (connector->panel.vbt.dsi.seq_version >= 3) {
> if (gpio_index >= CHV_GPIO_IDX_START_SE) {
> /* XXX: it's unclear whether 255->57 is part of SE. */
> - gpio_index -= CHV_GPIO_IDX_START_SE;
> - port = CHV_IOSF_PORT_GPIO_SE;
> + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
> + gpio_index - CHV_GPIO_IDX_START_SW, value);
The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index - CHV_GPIO_IDX_START_SE".
Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to compile ...
Regards,
Hans
> } else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
> - gpio_index -= CHV_GPIO_IDX_START_SW;
> - port = CHV_IOSF_PORT_GPIO_SW;
> + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
> + gpio_index - CHV_GPIO_IDX_START_SW, value);
> } else if (gpio_index >= CHV_GPIO_IDX_START_E) {
> - gpio_index -= CHV_GPIO_IDX_START_E;
> - port = CHV_IOSF_PORT_GPIO_E;
> + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
> + gpio_index - CHV_GPIO_IDX_START_E, value);
> } else {
> - port = CHV_IOSF_PORT_GPIO_N;
> + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
> + gpio_index - CHV_GPIO_IDX_START_N, value);
> }
> } else {
> /* XXX: The spec is unclear about CHV GPIO on seq v2 */
> @@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector *connector,
> return;
> }
>
> - port = CHV_IOSF_PORT_GPIO_N;
> + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
> + gpio_index - CHV_GPIO_IDX_START_N, value);
> }
> -
> - family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
> - gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
> -
> - cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
> - cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
> -
> - vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
> - vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
> - vlv_iosf_sb_write(dev_priv, port, cfg0,
> - CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
> - CHV_GPIO_GPIOTXSTATE(value));
> - vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
> }
>
> static void bxt_gpio_set_value(struct intel_connector *connector,
On Thu, Nov 02, 2023 at 04:47:41PM +0100, Hans de Goede wrote:
> On 11/2/23 16:12, Andy Shevchenko wrote:
...
> > + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
> > + gpio_index - CHV_GPIO_IDX_START_SW, value);
>
> The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index - CHV_GPIO_IDX_START_SE".
>
> Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to compile ...
Ah, indeed. I looks like I run the test build, but forgot to look into the result. :-(
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 02, 2023 at 05:12:14PM +0200, Andy Shevchenko wrote:
> From: Jani Nikula <[email protected]>
>
> Purely a guess. Drop the nop function.
>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Signed-off-by: Jani Nikula <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index 24b2cbcfc1ef..b2c0cc11f8c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -372,14 +372,6 @@ static void bxt_exec_gpio(struct intel_connector *connector,
> gpiod_set_value(gpio_desc, value);
> }
>
> -static void icl_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);
> -
> - drm_dbg_kms(&dev_priv->drm, "Skipping ICL GPIO element execution\n");
> -}
> -
> enum {
> MIPI_RESET_1 = 0,
> MIPI_AVDD_EN_1,
> @@ -491,7 +483,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
> if (native)
> icl_native_gpio_set_value(dev_priv, gpio_number, value);
> else if (DISPLAY_VER(dev_priv) >= 11)
> - icl_exec_gpio(connector, gpio_source, gpio_index, value);
> + bxt_exec_gpio(connector, gpio_source, gpio_index, value);
We could just drop this whole branch since we end up in bxt_exec_gpio()
in the end anyway. Or we drop the final else and make this one check for
DISPLAY_VER >=9.
> else if (IS_VALLEYVIEW(dev_priv))
> vlv_exec_gpio(connector, gpio_source, gpio_number, value);
> else if (IS_CHERRYVIEW(dev_priv))
> --
> 2.40.0.1.gaa8946217a0b
--
Ville Syrj?l?
Intel
On Thu, Nov 02, 2023 at 07:10:09PM +0200, Ville Syrj?l? wrote:
> On Thu, Nov 02, 2023 at 05:12:14PM +0200, Andy Shevchenko wrote:
...
> > if (native)
> > icl_native_gpio_set_value(dev_priv, gpio_number, value);
> > else if (DISPLAY_VER(dev_priv) >= 11)
> > - icl_exec_gpio(connector, gpio_source, gpio_index, value);
> > + bxt_exec_gpio(connector, gpio_source, gpio_index, value);
>
> We could just drop this whole branch since we end up in bxt_exec_gpio()
> in the end anyway. Or we drop the final else and make this one check for
> DISPLAY_VER >=9.
Looking at the code, I'm not sure how we can get rid of it, but the second
option is feasible.
> > else if (IS_VALLEYVIEW(dev_priv))
> > vlv_exec_gpio(connector, gpio_source, gpio_number, value);
> > else if (IS_CHERRYVIEW(dev_priv))
--
With Best Regards,
Andy Shevchenko
Hi Andy,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes linus/master v6.6 next-20231103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/drm-i915-dsi-assume-BXT-gpio-works-for-non-native-GPIO/20231103-064642
base: git://anongit.freedesktop.org/drm-intel for-linux-next
patch link: https://lore.kernel.org/r/20231102151228.668842-15-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v3 14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231104/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_dsi_vbt.c:272:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
^
drivers/gpu/drm/i915/display/intel_dsi_vbt.c:275:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
^
drivers/gpu/drm/i915/display/intel_dsi_vbt.c:278:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
^
drivers/gpu/drm/i915/display/intel_dsi_vbt.c:281:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
^
drivers/gpu/drm/i915/display/intel_dsi_vbt.c:299:3: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
^
5 errors generated.
vim +/soc_exec_opaque_gpio +272 drivers/gpu/drm/i915/display/intel_dsi_vbt.c
263
264 static void chv_gpio_set_value(struct intel_connector *connector,
265 u8 gpio_source, u8 gpio_index, bool value)
266 {
267 struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
268
269 if (connector->panel.vbt.dsi.seq_version >= 3) {
270 if (gpio_index >= CHV_GPIO_IDX_START_SE) {
271 /* XXX: it's unclear whether 255->57 is part of SE. */
> 272 soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
273 gpio_index - CHV_GPIO_IDX_START_SW, value);
274 } else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
275 soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
276 gpio_index - CHV_GPIO_IDX_START_SW, value);
277 } else if (gpio_index >= CHV_GPIO_IDX_START_E) {
278 soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
279 gpio_index - CHV_GPIO_IDX_START_E, value);
280 } else {
281 soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
282 gpio_index - CHV_GPIO_IDX_START_N, value);
283 }
284 } else {
285 /* XXX: The spec is unclear about CHV GPIO on seq v2 */
286 if (gpio_source != 0) {
287 drm_dbg_kms(&dev_priv->drm,
288 "unknown gpio source %u\n", gpio_source);
289 return;
290 }
291
292 if (gpio_index >= CHV_GPIO_IDX_START_E) {
293 drm_dbg_kms(&dev_priv->drm,
294 "invalid gpio index %u for GPIO N\n",
295 gpio_index);
296 return;
297 }
298
299 soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
300 gpio_index - CHV_GPIO_IDX_START_N, value);
301 }
302 }
303
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki