On Fri, Jul 12, 2024 at 05:32:33PM GMT, 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 display engine driver as a master binds FrameGen and
> TCon drivers as components. While at it, the display engine driver
> is a component to be bound with the upcoming DRM driver.
Generic question: why do you need so many small subdrivers? Are they
used to represent the flexibility of the pipeline? Can you instantiate
these units directly from the de(?) driver and reference created
structures without the need to create subdevices?
>
> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> ---
> 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 | 5 +
> drivers/gpu/drm/imx/dc/Makefile | 5 +
> drivers/gpu/drm/imx/dc/dc-de.c | 151 +++++++++++++
> 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 | 24 +++
> drivers/gpu/drm/imx/dc/dc-fg.c | 366 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/imx/dc/dc-tc.c | 137 ++++++++++++
> 10 files changed, 784 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
>
> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> index 03535a15dd8f..3e8c6edbc17c 100644
> --- a/drivers/gpu/drm/imx/Kconfig
> +++ b/drivers/gpu/drm/imx/Kconfig
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
>
> +source "drivers/gpu/drm/imx/dc/Kconfig"
> source "drivers/gpu/drm/imx/dcss/Kconfig"
> source "drivers/gpu/drm/imx/ipuv3/Kconfig"
> source "drivers/gpu/drm/imx/lcdc/Kconfig"
> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
> index 86f38e7c7422..c7b317640d71 100644
> --- a/drivers/gpu/drm/imx/Makefile
> +++ b/drivers/gpu/drm/imx/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
>
> +obj-$(CONFIG_DRM_IMX8_DC) += dc/
> obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
> obj-$(CONFIG_DRM_IMX) += ipuv3/
> obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
> new file mode 100644
> index 000000000000..32d7471c49d0
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/Kconfig
> @@ -0,0 +1,5 @@
> +config DRM_IMX8_DC
> + tristate "Freescale i.MX8 Display Controller Graphics"
> + depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
> + help
> + enable Freescale i.MX8 Display Controller(DC) graphics support
> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
> new file mode 100644
> index 000000000000..56de82d53d4d
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
> +
> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
> new file mode 100644
> index 000000000000..2c8268b76b08
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-de.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/component.h>
> +#include <linux/container_of.h>
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <drm/drm_managed.h>
> +
> +#include "dc-de.h"
> +#include "dc-drv.h"
> +
> +#define POLARITYCTRL 0xc
> +#define POLEN_HIGH BIT(2)
> +
> +struct dc_de_priv {
> + struct dc_de engine;
> + void __iomem *reg_top;
> +};
> +
> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
> +{
> + return container_of(de, struct dc_de_priv, engine);
> +}
> +
> +static inline void
> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
> +{
> + struct dc_de_priv *priv = to_de_priv(de);
> +
> + writel(value, priv->reg_top + offset);
Is there a point in this wrapper? Can you call writel directly? This
question generally applies to the driver. I see a lot of small functions
which can be inlined without losing the clarity.
> +}
> +
> +static void dc_dec_init(struct dc_de *de)
> +{
> + dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
> +}
> +
> +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;
> + struct dc_de_priv *priv;
> + int ret;
> +
> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
> + if (IS_ERR(priv->reg_top))
> + return PTR_ERR(priv->reg_top);
> +
> + priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
> + if (priv->engine.irq_shdld < 0)
> + return priv->engine.irq_shdld;
> +
> + priv->engine.irq_framecomplete =
> + platform_get_irq_byname(pdev, "framecomplete");
> + if (priv->engine.irq_framecomplete < 0)
> + return priv->engine.irq_framecomplete;
> +
> + priv->engine.irq_seqcomplete =
> + platform_get_irq_byname(pdev, "seqcomplete");
> + if (priv->engine.irq_seqcomplete < 0)
> + return priv->engine.irq_seqcomplete;
> +
> + priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");
Is this alias documented somewhere? Is it Acked by DT maintainers?
> + if (priv->engine.id < 0) {
> + dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
> + return priv->engine.id;
> + }
> +
> + priv->engine.dev = dev;
> +
> + dev_set_drvdata(dev, priv);
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return ret;
> +
> + dc_drm->de[priv->engine.id] = &priv->engine;
> +
> + return 0;
> +}
> +
--
With best wishes
Dmitry
