[PATCH v5] drm/bridge: sii9234: use extcon cable detection logic to detect MHL

[PATCH v5] drm/bridge: sii9234: use extcon cable detection logic to detect MHL

[Henrik Grimler]

To use MHL we currently need the MHL chip to be permanently on, which
consumes unnecessary power. Let's use extcon attached to MUIC to enable
the MHL chip only if it detects an MHL cable.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Henrik Grimler <henrik@grimler.se>
---
Hi,

Fix so HDMI through the sii9234 MHL chip works when cable is
hotplugged, by making the MHL chip use extcon cable detection
functions. The patch is heavily inspired by commit 688838442147
("drm/bridge/sii8620: use micro-USB cable detection logic to detect
MHL") by Maciej Purski.

Before these changes, HDMI only worked if cable was plugged in before
booting. If no cable was connected, then wlr-randr still showed HDMI
as connected, with 0x0 px, which confused at least some UIs (phosh)
and caused problems:
https://gitlab.gnome.org/World/Phosh/phosh/-/issues/828

Tested on exynos4412-i9305.
---
Changes in v5:
- Rebase and drop applied patches
- Fix review comments by Luca:
  - Remove unnecessary line breaks and braces
  - Change Kconfig extcon dependency
  - Use devm_extcon_register_notifier
- Link to v4: https://lore.kernel.org/r/20260113-exynos4-sii9234-driver-v4-0-6e8c0ac14f84@grimler.se

Changes in v4:
- Collect tags
- Link to v3: https://lore.kernel.org/r/20250824-exynos4-sii9234-driver-v3-0-80849e716a37@grimler.se

Changes in v3:
- Fix return of dev_err_probe in patch 2 and patch 3, spotted by
  Dmitry and Marek respectively.
- Change to depends on EXTCON || !EXTCON instead of select
- Collect tags for patch 1 (not 3 since there were (minor) changes)
- Link to v2: https://lore.kernel.org/r/20250724-exynos4-sii9234-driver-v2-0-faee244f1d40@grimler.se

Changes in v2:
- Add dependency on extcon in patch 3. Issue reported by kernel test robot <lkp@intel.com>
- Link to v1: https://lore.kernel.org/r/20250721-exynos4-sii9234-driver-v1-0-2e47ed02f677@grimler.se

To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <rfoss@kernel.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/Kconfig   |  1 +
 drivers/gpu/drm/bridge/sii9234.c | 78 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 4a57d49b4c6d..84236e816080 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -333,6 +333,7 @@ config DRM_SII902X
 config DRM_SII9234
 	tristate "Silicon Image SII9234 HDMI/MHL bridge"
 	depends on OF
+	depends on EXTCON if EXTCON
 	help
 	  Say Y here if you want support for the MHL interface.
 	  It is an I2C driver, that detects connection of MHL bridge
diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index be67642ab7d3..bf211906fefc 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -20,6 +20,7 @@
 
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/extcon.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -27,6 +28,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -173,6 +175,10 @@ struct sii9234 {
 	struct gpio_desc *gpio_reset;
 	int i2c_error;
 	struct regulator_bulk_data supplies[4];
+	struct extcon_dev *extcon;
+	struct notifier_block extcon_nb;
+	struct work_struct extcon_wq;
+	int cable_state;
 
 	struct mutex lock; /* Protects fields below and device registers */
 	enum sii9234_state state;
@@ -864,6 +870,63 @@ static int sii9234_init_resources(struct sii9234 *ctx,
 	return 0;
 }
 
+static void sii9234_extcon_work(struct work_struct *work)
+{
+	struct sii9234 *ctx = container_of(work, struct sii9234, extcon_wq);
+	int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
+
+	if (state == ctx->cable_state)
+		return;
+
+	ctx->cable_state = state;
+
+	if (state > 0)
+		sii9234_cable_in(ctx);
+	else
+		sii9234_cable_out(ctx);
+}
+
+static int sii9234_extcon_notifier(struct notifier_block *nb,
+			unsigned long event, void *ptr)
+{
+	struct sii9234 *ctx = container_of(nb, struct sii9234, extcon_nb);
+
+	schedule_work(&ctx->extcon_wq);
+
+	return NOTIFY_DONE;
+}
+
+static int sii9234_extcon_init(struct sii9234 *ctx)
+{
+	struct extcon_dev *edev;
+	struct device_node *musb, *muic;
+	int ret;
+
+	/* Get micro-USB connector node */
+	musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
+	/* Then get micro-USB Interface Controller node */
+	muic = of_get_next_parent(musb);
+
+	if (!muic) {
+		dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");
+		return 0;
+	}
+
+	edev = extcon_find_edev_by_node(muic);
+	of_node_put(muic);
+	if (IS_ERR(edev))
+		return dev_err_probe(ctx->dev, PTR_ERR(edev), "invalid or missing extcon\n");
+
+	ctx->extcon = edev;
+	ctx->extcon_nb.notifier_call = sii9234_extcon_notifier;
+	INIT_WORK(&ctx->extcon_wq, sii9234_extcon_work);
+	ret = devm_extcon_register_notifier(ctx->dev, edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
+	if (ret)
+		dev_err_probe(ctx->dev, ret, "failed to register notifier for MHL\n");
+
+	return 0;
+}
+
 static enum drm_mode_status sii9234_mode_valid(struct drm_bridge *bridge,
 					 const struct drm_display_info *info,
 					 const struct drm_display_mode *mode)
@@ -919,12 +982,17 @@ static int sii9234_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	ret = sii9234_extcon_init(ctx);
+	if (ret < 0)
+		return ret;
+
 	i2c_set_clientdata(client, ctx);
 
 	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
-	sii9234_cable_in(ctx);
+	if (!ctx->extcon)
+		sii9234_cable_in(ctx);
 
 	return 0;
 }
@@ -933,7 +1001,13 @@ static void sii9234_remove(struct i2c_client *client)
 {
 	struct sii9234 *ctx = i2c_get_clientdata(client);
 
-	sii9234_cable_out(ctx);
+	if (ctx->extcon) {
+		flush_work(&ctx->extcon_wq);
+		if (ctx->cable_state > 0)
+			sii9234_cable_out(ctx);
+	} else {
+		sii9234_cable_out(ctx);
+	}
 	drm_bridge_remove(&ctx->bridge);
 }
 

---
base-commit: c182a2ae13886463f7ae36e034449477306d182b
change-id: 20231218-exynos4-sii9234-driver-d817d4b511d5

Best regards,
--  
Henrik Grimler <henrik@grimler.se>

Re: [PATCH v5] drm/bridge: sii9234: use extcon cable detection logic to detect MHL

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free due to race between `flush_work()` and devres teardown.
- [High] Missing initial state check breaks display on coldplug.
- [Medium] Missing `return` statement after `dev_err_probe()` ignores initialization errors.
--

--- Patch [1]: [PATCH v5] drm/bridge: sii9234: use extcon cable detection logic to detect MHL ---
commit 15547772b748ad5e9e79681b157e1cae684d850d
Author: Henrik Grimler <henrik@grimler.se>

drm/bridge: sii9234: use extcon cable detection logic to detect MHL

To use MHL we currently need the MHL chip to be permanently on, which
consumes unnecessary power. Let's use extcon attached to MUIC to enable
the MHL chip only if it detects an MHL cable.

> diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
> index be67642ab7d31..bf211906fefc6 100644
> --- a/drivers/gpu/drm/bridge/sii9234.c
> +++ b/drivers/gpu/drm/bridge/sii9234.c
[ ... ]
> @@ -864,6 +870,63 @@ static int sii9234_init_resources(struct sii9234 *ctx,
>  	return 0;
>  }
>  
> +static void sii9234_extcon_work(struct work_struct *work)
> +{
> +	struct sii9234 *ctx = container_of(work, struct sii9234, extcon_wq);
> +	int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
> +
> +	if (state == ctx->cable_state)
> +		return;
> +
> +	ctx->cable_state = state;
> +
> +	if (state > 0)
> +		sii9234_cable_in(ctx);
> +	else
> +		sii9234_cable_out(ctx);
> +}
[ ... ]
> +static int sii9234_extcon_init(struct sii9234 *ctx)
> +{
[ ... ]
> +	ctx->extcon = edev;
> +	ctx->extcon_nb.notifier_call = sii9234_extcon_notifier;
> +	INIT_WORK(&ctx->extcon_wq, sii9234_extcon_work);
> +	ret = devm_extcon_register_notifier(ctx->dev, edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
> +	if (ret)
> +		dev_err_probe(ctx->dev, ret, "failed to register notifier for MHL\n");
> +
> +	return 0;
> +}

[Severity: Medium]
Will this ignore initialization errors?

If devm_extcon_register_notifier() fails (for example, returning -EPROBE_DEFER
or -ENOMEM), the error is logged but ignored since there is no return statement
after dev_err_probe(). The probe function completes successfully and assumes
ctx->extcon is active.

[ ... ]
> @@ -919,12 +982,17 @@ static int sii9234_probe(struct i2c_client *client)
[ ... ]
>  	i2c_set_clientdata(client, ctx);
>  
>  	ctx->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ctx->bridge);
>  
> -	sii9234_cable_in(ctx);
> +	if (!ctx->extcon)
> +		sii9234_cable_in(ctx);
>  
>  	return 0;
>  }

[Severity: High]
Does this code skip setting the initial state if the cable is already plugged
in during boot?

Extcon notifiers trigger on state changes but do not automatically emit an
event for the current state upon registration. In sii9234_probe(), if an
extcon device is found, sii9234_cable_in() is skipped, but the driver never
queries the initial state.

If the cable is already connected when the driver probes, the hardware will
never be powered on, and the display will remain non-functional until the user
physically unplugs and replugs the cable.

> @@ -933,7 +1001,13 @@ static void sii9234_remove(struct i2c_client *client)
>  {
>  	struct sii9234 *ctx = i2c_get_clientdata(client);
>  
> -	sii9234_cable_out(ctx);
> +	if (ctx->extcon) {
> +		flush_work(&ctx->extcon_wq);

[Severity: High]
Does this code introduce a race condition with the devres teardown?

Since the notifier is registered via devm_extcon_register_notifier(), it will
only be unregistered after sii9234_remove() completes. If an extcon state
change occurs right after flush_work(&ctx->extcon_wq),
sii9234_extcon_notifier() will queue the work again.

Once sii9234_remove() completes, devres will unregister the notifier and
free ctx (allocated via devm_drm_bridge_alloc()). When the queued work executes
in sii9234_extcon_work():

	struct sii9234 *ctx = container_of(work, struct sii9234, extcon_wq);
	int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);

it will dereference the freed ctx, resulting in a use-after-free.

> +		if (ctx->cable_state > 0)
> +			sii9234_cable_out(ctx);
> +	} else {
> +		sii9234_cable_out(ctx);
> +	}
>  	drm_bridge_remove(&ctx->bridge);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-exynos4-sii9234-driver-v5-1-44b31b5597bd@grimler.se?part=1