Hi Anastasia,
thank you for picking this up and apologies for my late reply.
On Tue, Sep 10, 2024 at 5:17 PM Anastasia Belova <abelova@xxxxxxxxxxxxx> wrote:
[...]
> @@ -215,7 +210,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> regs = devm_platform_ioremap_resource_byname(pdev, "vpu");
> if (IS_ERR(regs)) {
> ret = PTR_ERR(regs);
> - goto free_drm;
> + goto remove_encoders;
can't we just return PTR_ERR(regs) here since all resources above are
now automatically free (as they are devm_* managed)?
> }
>
> priv->io_base = regs;
> @@ -223,13 +218,13 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hhi");
> if (!res) {
> ret = -EINVAL;
> - goto free_drm;
> + goto remove_encoders;
same here, can't we just return -EINVAL directly?
> }
> /* Simply ioremap since it may be a shared register zone */
> regs = devm_ioremap(dev, res->start, resource_size(res));
> if (!regs) {
> ret = -EADDRNOTAVAIL;
> - goto free_drm;
> + goto remove_encoders;
also just return -EADDRNOTAVAIL directly
> }
>
> priv->hhi = devm_regmap_init_mmio(dev, regs,
> @@ -237,18 +232,18 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> if (IS_ERR(priv->hhi)) {
> dev_err(&pdev->dev, "Couldn't create the HHI regmap\n");
> ret = PTR_ERR(priv->hhi);
either return PTR_ERR(priv->hhi) here or convert the dev_err to
dev_err_probe and return it's value
> - goto free_drm;
> + goto remove_encoders;
> }
>
> priv->canvas = meson_canvas_get(dev);
> if (IS_ERR(priv->canvas)) {
> ret = PTR_ERR(priv->canvas);
> - goto free_drm;
> + goto remove_encoders;
return PTR_ERR(priv->canvas);
> }
>
> ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_osd1);
> if (ret)
> - goto free_drm;
> + goto remove_encoders;
meson_canvas_alloc() is the first non devm_* managed allocation.
so this is the last time we can simply "return ret;", starting with
the next error case we need goto for cleaning up resources.
> ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_vd1_0);
> if (ret)
> goto free_canvas_osd1;
(starting from here the goto free_... is needed and this one is
actually already correct)
> @@ -261,7 +256,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>
> priv->vsync_irq = platform_get_irq(pdev, 0);
>
> - ret = drm_vblank_init(drm, 1);
> + ret = drm_vblank_init(&priv->drm, 1);
> if (ret)
> goto free_canvas_vd1_2;
>
> @@ -284,10 +279,10 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> ret = drmm_mode_config_init(drm);
> if (ret)
> goto free_canvas_vd1_2;
> - drm->mode_config.max_width = 3840;
> - drm->mode_config.max_height = 2160;
> - drm->mode_config.funcs = &meson_mode_config_funcs;
> - drm->mode_config.helper_private = &meson_mode_config_helpers;
> + priv->drm.mode_config.max_width = 3840;
> + priv->drm.mode_config.max_height = 2160;
> + priv->drm.mode_config.funcs = &meson_mode_config_funcs;
> + priv->drm.mode_config.helper_private = &meson_mode_config_helpers;
>
> /* Hardware Initialization */
>
> @@ -308,9 +303,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> goto exit_afbcd;
>
> if (has_components) {
> - ret = component_bind_all(dev, drm);
> + ret = component_bind_all(dev, &priv->drm);
> if (ret) {
> - dev_err(drm->dev, "Couldn't bind all components\n");
> + dev_err(priv->drm.dev, "Couldn't bind all components\n");
> /* Do not try to unbind */
> has_components = false;
> goto exit_afbcd;
just below this we have:
ret = meson_encoder_hdmi_probe(priv);
if (ret)
goto exit_afbcd;
I think this is the place where we need to call component_unbind_all,
so we need some kind of "goto unbind_components;" here.
All other "goto exit_afbcd;" below will need to be converted to "goto
unbind_components;" (or whichever name you choose) as well.
Also the ordering where component_unbind_all() is incorrect. It's been
incorrect even before your patch - but maybe now is the right time to
clean it up?
I had to double check because I was surprised about the calls to
meson_encoder_{cvbs,dsi,hdmi}_remove(priv);
It turns out that it's safe to call these three functions at any time
because they only ever do something if their _probe() counterpart has
been called.
Best regards,
Martin
