On Mon, Dec 23, 2024 at 02:41:37PM +0800, Liu Ying wrote:
> i.MX8qxp Display Controller display engine consists of all processing
> units that operate in a display clock domain. Add minimal feature
> support with FrameGen and TCon so that the engine can output display
> timings. The FrameGen driver, TCon driver and display engine driver
> are components to be aggregated by a master registered in the upcoming
> DRM driver.
>
> Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> ---
> v7:
> * Add kernel doc for struct dc_drm_device. (Dmitry)
> * Fix regmap_config definitions by correcting name field, correcting read
> ranges and setting max_register field.
> * Get instance numbers from device data(compatible strings) instead of OF
> aliases.
Unfortunately no. Your instances are compatible on the hardware level
(at least they were with the previous versions, I don't think that
there was a silicon change).
> * Collect Maxime's R-b tag.
> * Trivial tweaks.
>
> v6:
> * No change.
>
> v5:
> * Replace .remove_new with .remove in dc-{de,fg,tc}.c. (Uwe)
> * Select REGMAP and REGMAP_MMIO Kconfig options.
> * Fix commit message to state that display engine driver is a component driver
> instead of a master/aggregate driver.
>
> v4:
> * Use regmap to define register map for all registers. (Dmitry)
> * Use regmap APIs to access registers. (Dmitry)
> * Inline some small functions. (Dmitry)
> * Move dc_fg_displaymode() and dc_fg_panic_displaymode() function calls from
> KMS routine to initialization stage. (Dmitry)
> * Use devm_kzalloc() to drmm_kzalloc() to allocate dc_* data strutures.
> * Drop unnecessary private struct dc_*_priv.
> * Set suppress_bind_attrs driver flag to true to avoid unnecessary sys
> interfaces to bind/unbind the drivers.
>
> v3:
> * No change.
>
> v2:
> * Use OF alias id to get instance id.
> * Add dev member to struct dc_tc.
>
> drivers/gpu/drm/imx/Kconfig | 1 +
> drivers/gpu/drm/imx/Makefile | 1 +
> drivers/gpu/drm/imx/dc/Kconfig | 7 +
> drivers/gpu/drm/imx/dc/Makefile | 5 +
> drivers/gpu/drm/imx/dc/dc-de.c | 153 +++++++++++++
> drivers/gpu/drm/imx/dc/dc-de.h | 62 ++++++
> drivers/gpu/drm/imx/dc/dc-drv.c | 32 +++
> drivers/gpu/drm/imx/dc/dc-drv.h | 29 +++
> drivers/gpu/drm/imx/dc/dc-fg.c | 378 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/imx/dc/dc-tc.c | 142 ++++++++++++
> 10 files changed, 810 insertions(+)
> create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
> create mode 100644 drivers/gpu/drm/imx/dc/Makefile
> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
> create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
> create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
>
[...]
> +
> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct dc_drm_device *dc_drm = data;
> + void __iomem *base_top;
> + struct dc_de *de;
> + int ret;
> +
> + de = devm_kzalloc(dev, sizeof(*de), GFP_KERNEL);
> + if (!de)
> + return -ENOMEM;
> +
> + de->id = (enum dc_de_id)(uintptr_t)device_get_match_data(dev);
> +
> + base_top = devm_platform_ioremap_resource_byname(pdev, "top");
> + if (IS_ERR(base_top))
> + return PTR_ERR(base_top);
> +
> + de->reg_top = devm_regmap_init_mmio(dev, base_top,
> + &dc_de_top_regmap_config);
> + if (IS_ERR(de->reg_top))
> + return PTR_ERR(de->reg_top);
> +
> + de->irq_shdld = platform_get_irq_byname(pdev, "shdload");
Nit: shdload or shdld? Which one is used in the documentation?
> + if (de->irq_shdld < 0)
> + return de->irq_shdld;
> +
> + de->irq_framecomplete = platform_get_irq_byname(pdev, "framecomplete");
> + if (de->irq_framecomplete < 0)
> + return de->irq_framecomplete;
> +
> + de->irq_seqcomplete = platform_get_irq_byname(pdev, "seqcomplete");
> + if (de->irq_seqcomplete < 0)
> + return de->irq_seqcomplete;
> +
> + de->dev = dev;
> +
> + dev_set_drvdata(dev, de);
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return ret;
> +
> + dc_drm->de[de->id] = de;
> +
> + return 0;
> +}
> +
[...]
> +
> +struct dc_de {
> + struct device *dev;
> + struct regmap *reg_top;
> + struct dc_fg *fg;
> + struct dc_tc *tc;
> + int irq_shdld;
> + int irq_framecomplete;
> + int irq_seqcomplete;
> + enum dc_de_id id;
Why do you need to store it? This patch makes use of it just for the
registration.
> +};
> +
[...]
> +static int dc_fg_bind(struct device *dev, struct device *master, void *data)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct dc_drm_device *dc_drm = data;
> + void __iomem *base;
> + enum dc_fg_id id;
> + struct dc_fg *fg;
> + struct dc_de *de;
> +
> + fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL);
> + if (!fg)
> + return -ENOMEM;
> +
> + id = (enum dc_fg_id)(uintptr_t)device_get_match_data(dev);
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + fg->reg = devm_regmap_init_mmio(dev, base, &dc_fg_regmap_config);
> + if (IS_ERR(fg->reg))
> + return PTR_ERR(fg->reg);
> +
> + fg->clk_disp = devm_clk_get(dev, NULL);
> + if (IS_ERR(fg->clk_disp))
> + return dev_err_probe(dev, PTR_ERR(fg->clk_disp),
> + "failed to get display clock\n");
> +
> + fg->dev = dev;
> +
> + de = dc_drm->de[id];
This depends on a particular order of component's being bound. If the
order changes for whatever reason (e.g. due to component.c
implementation being changed) then your driver might crash here.
This applies to several other places in the driver.
> + de->fg = fg;
> +
> + return 0;
> +}
> +
--
With best wishes
Dmitry
