Hi Marek,
On Thu, Aug 14, 2025 at 01:26:33PM +0200, Marek Szyprowski wrote:
> On 24.07.2025 20:50, Henrik Grimler wrote:
> > 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.
> >
> > Signed-off-by: Henrik Grimler <hen...@grimler.se>
> > ---
> > v2: add dependency on extcon. Issue reported by kernel test robot
> > <l...@intel.com>
> > ---
> > drivers/gpu/drm/bridge/Kconfig | 1 +
> > drivers/gpu/drm/bridge/sii9234.c | 89
> > ++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index
> > b9e0ca85226a603a24f90c6879d1499f824060cb..f18a083f6e1c6fe40bde5e65a1548acc61a162ae
> > 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -303,6 +303,7 @@ config DRM_SII902X
> > config DRM_SII9234
> > tristate "Silicon Image SII9234 HDMI/MHL bridge"
> > depends on OF
> > + select 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
> > 0e0bb1bf71fdcef788715cfd6fa158a6992def33..4d84ba01ea76816bebdbc29d48a041c9c6cd508e
> > 100644
> > --- a/drivers/gpu/drm/bridge/sii9234.c
> > +++ b/drivers/gpu/drm/bridge/sii9234.c
[ ...]
> > +
> > + edev = extcon_find_edev_by_node(muic);
> > + of_node_put(muic);
> > + if (IS_ERR(edev)) {
> > + dev_err_probe(ctx->dev, PTR_ERR(edev),
> > + "invalid or missing extcon\n");
> > + }
>
> It looks that the original logic got lost somehow in the above code
> block, what causes kernel oops if compiled as module and loaded before
> extcon provider. Please handle -EPROBE_DEFER and propagate error value,
> like the original code did in sii8620 driver:
>
> if (IS_ERR(edev)) {
> if (PTR_ERR(edev) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
> dev_err(ctx->dev, "Invalid or missing extcon\n");
> return PTR_ERR(edev);
> }
Thanks for detecting the issue! I think my code is just missing return
before dev_err_probe (same mistake as I did on patch 2). With return
added I have not been able to reproduce any kernel oops, but if
CONFIG_DRM_SII9234=y and CONFIG_EXTCON_MAX77693=m then it seems like
linux gets stuck probing sii9234 and waiting for the extcon provider
(verified with some printf debugging). This happens for me both with:
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");
}
and
edev = extcon_find_edev_by_node(muic);
of_node_put(muic);
if (IS_ERR(edev)) {
if (PTR_ERR(edev) == -EPROBE_DEFER)
return -EPROBE_DEFER;
dev_err(ctx->dev, "Invalid or missing extcon\n");
return PTR_ERR(edev);
}
I am not sure what to do to fix the issue, as far as I can see probe
logic and extcon handling is the same as in sil-sii8620 and ite-it6505
(i.e. the other bridges that use extcon). Will investigate further.
Best regards,
Henrik Grimler
> > +
> > + ctx->extcon = edev;
> > + ctx->extcon_nb.notifier_call = sii9234_extcon_notifier;
> > + INIT_WORK(&ctx->extcon_wq, sii9234_extcon_work);
> > + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
> > + if (ret) {
> > + dev_err(ctx->dev, "failed to register notifier for MHL\n");
> > + return ret;
> > + }
> > +
> > + 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)
> > @@ -916,12 +986,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;
> > }
> > @@ -930,7 +1005,15 @@ static void sii9234_remove(struct i2c_client *client)
> > {
> > struct sii9234 *ctx = i2c_get_clientdata(client);
> >
> > - sii9234_cable_out(ctx);
> > + if (ctx->extcon) {
> > + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,
> > + &ctx->extcon_nb);
> > + flush_work(&ctx->extcon_wq);
> > + if (ctx->cable_state > 0)
> > + sii9234_cable_out(ctx);
> > + } else {
> > + sii9234_cable_out(ctx);
> > + }
> > drm_bridge_remove(&ctx->bridge);
> > }
> >
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
