On Tue, 02 Jun 2026 20:36:50 +0200, Henrik Grimler <[email protected]> wrote:
Hello Henrik, > 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 <[email protected]> > Signed-off-by: Henrik Grimler <[email protected]> I have several minor comments, but overall looks OK to me. > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 4a57d49b4c6d..11171b4054b5 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 || !EXTCON IMO the other syntax is more readable: depends on EXTCON if EXTCON See https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html > > diff --git a/drivers/gpu/drm/bridge/sii9234.c > b/drivers/gpu/drm/bridge/sii9234.c > index e43248e515b3..72c6aeed6e12 100644 > --- a/drivers/gpu/drm/bridge/sii9234.c > +++ b/drivers/gpu/drm/bridge/sii9234.c > @@ -170,8 +172,12 @@ struct sii9234 { > struct drm_bridge bridge; > struct device *dev; > 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; > + int i2c_error; Moving i2c_error is unrelated, and apparently unneeded unless I'm missing something. > @@ -863,6 +869,70 @@ 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); Can fit on a single line. > + 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 *self, "self" is not a very descriptive name. What about "nb", being a notifier block? > + unsigned long event, void *ptr) > +{ > + struct sii9234 *ctx = > + container_of(self, struct sii9234, extcon_nb); Single line here too. > [ ... skip 14 lines ... ] > + /* 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"); Can fit in a line. > + 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"); > + } You can drop the curly braces, and perhaps the dev_err_probe() can fit in a 100-chars line. > + > + 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; > + } Use dev_err_probe(), and so remove the {}. > @@ -929,7 +1004,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); I guess you can use devm_extcon_register_notifier() to simplify a bit the code. > + flush_work(&ctx->extcon_wq); > + if (ctx->cable_state > 0) > + sii9234_cable_out(ctx); > + } else { > + sii9234_cable_out(ctx); > + } > drm_bridge_remove(&ctx->bridge); > } > Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
