On Thu Dec 11, 2025 at 5:39 PM CET, Kory Maincent (TI.com) wrote: Small nit:
> Now that tda998x_create and tda998x_destroy are called only in the probe > function, there is no need for separate functions. ^ add "and remove" here > Move the code into the tda998x_probe and tda998x_remove functions. > Rewrite the cleanup path using goto calls in probe and reorder it in the > remove function. > > Signed-off-by: Kory Maincent (TI.com) <[email protected]> > --- > drivers/gpu/drm/bridge/tda998x_drv.c | 99 > +++++++++++++++++++----------------- > 1 file changed, 51 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tda998x_drv.c > b/drivers/gpu/drm/bridge/tda998x_drv.c > index 865285ba2bd8c..4be49dd5bfc01 100644 > --- a/drivers/gpu/drm/bridge/tda998x_drv.c > +++ b/drivers/gpu/drm/bridge/tda998x_drv.c > @@ -1748,38 +1748,20 @@ static int tda998x_get_audio_ports(struct > tda998x_priv *priv, > return 0; > } > > -static void tda998x_destroy(struct device *dev) > -{ > - struct tda998x_priv *priv = dev_get_drvdata(dev); > - > - drm_bridge_remove(&priv->bridge); > - > - /* disable all IRQs and free the IRQ handler */ > - cec_write(priv, REG_CEC_RXSHPDINTENA, 0); > - reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); > - > - if (priv->audio_pdev) > - platform_device_unregister(priv->audio_pdev); > - > - if (priv->hdmi->irq) > - free_irq(priv->hdmi->irq, priv); Compared to the original code being removed... > @@ -1956,26 +1941,44 @@ static int tda998x_create(struct device *dev) > > return 0; > > -fail: > - tda998x_destroy(dev); > -err_irq: > +unregister_dev: > + i2c_unregister_device(priv->cec); > +notifier_conn_unregister: > + cec_notifier_conn_unregister(priv->cec_notify); > +free_irq: > + if (client->irq) { > + free_irq(client->irq, priv); > + cec_write(priv, REG_CEC_RXSHPDINTENA, 0); > + reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); > + } ...here you moved free_irq() before writing registers. It should stay last, as per free_irq() documentation. [...] > -static void tda998x_remove(struct i2c_client *client) > -{ > - tda998x_destroy(&client->dev); > + if (priv->audio_pdev) > + platform_device_unregister(priv->audio_pdev); > + > + i2c_unregister_device(priv->cec); > + > + cec_notifier_conn_unregister(priv->cec_notify); > + > + /* disable all IRQs and free the IRQ handler */ > + if (client->irq) { > + cec_write(priv, REG_CEC_RXSHPDINTENA, 0); > + reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); > + free_irq(priv->hdmi->irq, priv); > + } Here the order is correct. Otherwise looks good! With the above fixed and no other changes you can add my Reviewed-by: Luca Ceresoli <[email protected]> Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
