Hi Luca, Thanks for reviewing!
On Tue, Jul 14, 2026 at 06:34:20PM +0200, Luca Ceresoli wrote: > 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 Thanks for the link. I was not even aware of the other recommended variant, seems it is rarely used: $ find -name Kconfig -exec grep -P '\b(\w+)\b if \1\b' {} +|wc -l 7 Will change in next version that I aim at sending within a week. > > > > 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. Yes, it is unneeded, will drop the change in next version. > > @@ -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. Will fix. > > + 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? Sure, will fix. > > + unsigned long event, void *ptr) > > +{ > > + struct sii9234 *ctx = > > + container_of(self, struct sii9234, extcon_nb); > > Single line here too. Will fix. > > [ ... 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. The rest of the file uses 80 char line limit, but seems sensible to exceed it here. Will fix. > > + 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. Will fix. > > + > > + 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 {}. Will fix. > > @@ -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. Will look into it, thanks! Best regards, Henrik Grimler > > + 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 >
