Hi Sean,
On Tuesday 29 of October 2013 12:13:00 Sean Paul wrote:
> This patch trims exynos_drm_hdmi out of the driver. The reason it
> existed in the first place was to make up for the mixture of
> display/overlay/manager ops being spread across hdmi and mixer. With
> that code now rationalized, mixer and hdmi map directly to
> exynos_drm_crtc and exynos_drm_encoder, respectively. Since there is a
> 1:1 mapping, we no longer need this layer.
>
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
>
> Changes in v2:
> - hdmi/mixer ops now take display/manager instead of context
> Changes in v3:
> - Moved removal of exynos_drm_hdmi.c file here
>
> drivers/gpu/drm/exynos/Makefile | 3 +-
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 13 -
> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 416
> -------------------------------
> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 69 -----
> drivers/gpu/drm/exynos/exynos_hdmi.c | 162 +++++++-----
> drivers/gpu/drm/exynos/exynos_mixer.c | 204 ++++++++-------
> drivers/gpu/drm/exynos/exynos_mixer.h | 20 ++
> 7 files changed, 222 insertions(+), 665 deletions(-)
> delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> create mode 100644 drivers/gpu/drm/exynos/exynos_mixer.h
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index e106309..9e12fb7 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
[snip]
> +static struct exynos_drm_display hdmi_display = {
> + .type = EXYNOS_DISPLAY_TYPE_HDMI,
> + .ops = &hdmi_display_ops,
> +};
> +
> static irqreturn_t hdmi_irq_thread(int irq, void *arg)
> {
> - struct exynos_drm_hdmi_context *ctx = arg;
> - struct hdmi_context *hdata = ctx->ctx;
> + struct hdmi_context *hdata = arg;
>
> mutex_lock(&hdata->hdmi_mutex);
> hdata->hpd = gpio_get_value(hdata->hpd_gpio);
> @@ -1887,7 +1944,6 @@ static struct of_device_id hdmi_match_types[] = {
> static int hdmi_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> struct hdmi_context *hdata;
> struct s5p_hdmi_platform_data *pdata;
> struct resource *res;
> @@ -1902,20 +1958,13 @@ static int hdmi_probe(struct platform_device *pdev)
> if (!pdata)
> return -EINVAL;
>
> - drm_hdmi_ctx = devm_kzalloc(dev, sizeof(*drm_hdmi_ctx), GFP_KERNEL);
> - if (!drm_hdmi_ctx)
> - return -ENOMEM;
> -
> hdata = devm_kzalloc(dev, sizeof(struct hdmi_context), GFP_KERNEL);
> if (!hdata)
> return -ENOMEM;
>
> mutex_init(&hdata->hdmi_mutex);
>
> - drm_hdmi_ctx->ctx = (void *)hdata;
> - hdata->parent_ctx = (void *)drm_hdmi_ctx;
> -
> - platform_set_drvdata(pdev, drm_hdmi_ctx);
> + platform_set_drvdata(pdev, &hdmi_display);
This looks like a step backwards. Originally the driver had kind of per
instance data, while now it has a single global struct, making it harder
to properly support multiple HDMI instances, what is not unlikely to show
up in future SoCs.
So instead, I would embed an exynos_drm_display struct inside struct
hdmi_context and initialize it at runtime and use it to get access to
hdata, using container_of.
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
> b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 60935c4..39aed3e 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
[snip]
> @@ -1184,21 +1190,17 @@ static struct of_device_id mixer_match_types[] = {
> static int mixer_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> struct mixer_context *ctx;
> struct mixer_drv_data *drv;
> int ret;
>
> dev_info(dev, "probe start\n");
>
> - drm_hdmi_ctx = devm_kzalloc(dev, sizeof(*drm_hdmi_ctx),
> - GFP_KERNEL);
> - if (!drm_hdmi_ctx)
> - return -ENOMEM;
> -
> - ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> - if (!ctx)
> + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx) {
> + DRM_ERROR("failed to alloc mixer context.\n");
> return -ENOMEM;
> + }
>
> mutex_init(&ctx->mixer_mutex);
>
> @@ -1211,18 +1213,14 @@ static int mixer_probe(struct platform_device *pdev)
> platform_get_device_id(pdev)->driver_data;
> }
>
> - ctx->dev = dev;
> - ctx->parent_ctx = (void *)drm_hdmi_ctx;
> - drm_hdmi_ctx->ctx = (void *)ctx;
> + ctx->dev = &pdev->dev;
> ctx->vp_enabled = drv->is_vp_enabled;
> ctx->mxr_ver = drv->version;
> DRM_INIT_WAITQUEUE(&ctx->wait_vsync_queue);
> atomic_set(&ctx->wait_vsync_event, 0);
>
> - platform_set_drvdata(pdev, drm_hdmi_ctx);
> -
> /* acquire resources: regs, irqs, clocks */
> - ret = mixer_resources_init(drm_hdmi_ctx, pdev);
> + ret = mixer_resources_init(ctx, pdev);
> if (ret) {
> DRM_ERROR("mixer_resources_init failed\n");
> goto fail;
> @@ -1230,18 +1228,16 @@ static int mixer_probe(struct platform_device *pdev)
>
> if (ctx->vp_enabled) {
> /* acquire vp resources: regs, irqs, clocks */
> - ret = vp_resources_init(drm_hdmi_ctx, pdev);
> + ret = vp_resources_init(ctx, pdev);
> if (ret) {
> DRM_ERROR("vp_resources_init failed\n");
> goto fail;
> }
> }
>
> - /* attach mixer driver to common hdmi. */
> - exynos_mixer_drv_attach(drm_hdmi_ctx);
> -
> - /* register specific callback point to common hdmi. */
> - exynos_mixer_ops_register(&mixer_ops);
> + mixer_manager.ctx = ctx;
> + platform_set_drvdata(pdev, &mixer_manager);
> + exynos_drm_manager_register(&mixer_manager);
Ditto.
Best regards,
Tomasz
