Re: [PATCH v8 5/5] drm: xlnx: ZynqMP DP subsystem DRM KMS driver

Hi Hyun,

Thank you for the patch.

On Sat, Jul 07, 2018 at 07:05:38PM -0700, Hyun Kwon wrote:

This is a wrapper around the ZynqMP Display and DisplayPort drivers.

This isn't quite accurate. There's a single driver, named zynqmp-dpsub, split in a few source files. You've split the code in several patches to ease review (thank you for that !), but this patch isn't a wrapper, it's the top-level entry point for the driver.

Signed-off-by: Hyun Kwon hyun.kwon@xilinx.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch

---

v8

• Support reserved memory through memory-region dt binding

v6

• Accomodate the migration of logical master from platform device to device
• Remove the duplicate license paragraphs
• Do complete forward declaration in the header

v5

• Add an error handling of pipeline initialization

v4

• Use the newly added xlnx pipeline calls to initialize drm device

v2

• Change the SPDX identifier format

---

---

drivers/gpu/drm/xlnx/Kconfig | 11 +++ drivers/gpu/drm/xlnx/Makefile | 3 + drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 158 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 23 ++++++ 4 files changed, 195 insertions(+) create mode 100644 drivers/gpu/drm/xlnx/zynqmp_dpsub.c create mode 100644 drivers/gpu/drm/xlnx/zynqmp_dpsub.h

diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig index 19fd7cd..7c5529c 100644 --- a/drivers/gpu/drm/xlnx/Kconfig +++ b/drivers/gpu/drm/xlnx/Kconfig @@ -10,3 +10,14 @@ config DRM_XLNX display pipeline using Xilinx IPs in FPGA. This module provides the kernel mode setting functionalities for Xilinx display drivers.

+config DRM_ZYNQMP_DPSUB

• tristate "ZynqMP DP Subsystem Driver"

Should we spell DisplayPort in full here to make it easier to understand, or is "ZynqMP DP" used through the Xiinx documentation in a way that makes the configuration option title impossible to mistake for users ?

• depends on ARCH_ZYNQMP && OF && DRM_XLNX && COMMON_CLK
• select DMA_ENGINE
• select GENERIC_PHY
• help

•  DRM KMS driver for ZynqMP DP Subsystem controller. Choose
  

•  this option if you have a Xilinx ZynqMP SoC with DisplayPort
  

•  subsystem. The driver provides the kernel mode setting
  

•  functionlaities for ZynqMP DP subsystem.
  

s/functionlaities/functionality/

Or maybe just "The driver provides kernel mode setting for the ZynqMP DP subsystem". Or drop that sentence as it says pretty much the same thing as the first one :-)

diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile index c60a281..064a05a 100644 --- a/drivers/gpu/drm/xlnx/Makefile +++ b/drivers/gpu/drm/xlnx/Makefile @@ -1,2 +1,5 @@ xlnx_drm-objs += xlnx_crtc.o xlnx_drv.o xlnx_fb.o xlnx_gem.o obj-$(CONFIG_DRM_XLNX) += xlnx_drm.o

+zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o +obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c new file mode 100644 index 0000000..34e81d0 --- /dev/null +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • ZynqMP DP Subsystem Driver
• • Copyright (C) 2017 - 2018 Xilinx, Inc.
• • Author: Hyun Woo Kwon hyun.kwon@xilinx.com
• */

+#include <linux/component.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/of_reserved_mem.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>

+#include "xlnx_drv.h"

+#include "zynqmp_disp.h" +#include "zynqmp_dp.h" +#include "zynqmp_dpsub.h"

+static int +zynqmp_dpsub_bind(struct device *dev, struct device *master, void *data) +{

• int ret;
• ret = zynqmp_disp_bind(dev, master, data);

This isn't a component helper. You can pass it pointers to zynqmp_dpsub and drm_device instead of retrieving them internally through platform device data. The pointer to master is not needed. Same for zynqmp_dp_bind() below and the corresponding unbind functions.

• if (ret)

• return ret;
  

• /* zynqmp_disp should bind first, so zynqmp_dp encoder can find crtc */
• ret = zynqmp_dp_bind(dev, master, data);
• if (ret)

Shouldn't you call zynqmp_disp_unbind() here ?

• return ret;
  

• return 0;

+}

+static void +zynqmp_dpsub_unbind(struct device *dev, struct device *master, void *data) +{

• zynqmp_dp_unbind(dev, master, data);
• zynqmp_disp_unbind(dev, master, data);

+}

+static const struct component_ops zynqmp_dpsub_component_ops = {

• .bind = zynqmp_dpsub_bind,
• .unbind = zynqmp_dpsub_unbind,

+};

+static int zynqmp_dpsub_probe(struct platform_device *pdev) +{

• struct zynqmp_dpsub *dpsub;
• int ret;
• dpsub = devm_kzalloc(&pdev->dev, sizeof(*dpsub), GFP_KERNEL);
• if (!dpsub)

• return -ENOMEM;
  

• /* Sub-driver will access dpsub from drvdata */
• platform_set_drvdata(pdev, dpsub);
• pm_runtime_enable(&pdev->dev);
• /*

• * DP should be probed first so that the zynqmp_disp can set the output
  

• * format accordingly.
  

• */
  

• ret = zynqmp_dp_probe(pdev);
• if (ret)

• goto err_pm;
  

• ret = zynqmp_disp_probe(pdev);
• if (ret)

• goto err_dp;
  

• ret = component_add(&pdev->dev, &zynqmp_dpsub_component_ops);
• if (ret)

• goto err_disp;
  

• /* Try the reserved memory. Proceed if there's none */
• of_reserved_mem_device_init(&pdev->dev);
• /* Populate the sound child nodes */
• ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
• if (ret) {

• dev_err(&pdev->dev, "failed to populate child nodes\n");
  

• goto err_rmem;
  

• }
• dpsub->master = xlnx_drm_pipeline_init(&pdev->dev);
• if (IS_ERR(dpsub->master)) {

• dev_err(&pdev->dev, "failed to initialize the drm pipeline\n");
  

• goto err_populate;
  

• }
• dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
• return 0;

+err_populate:

• of_platform_depopulate(&pdev->dev);

+err_rmem:

• of_reserved_mem_device_release(&pdev->dev);
• component_del(&pdev->dev, &zynqmp_dpsub_component_ops);

+err_disp:

• zynqmp_disp_remove(pdev);

+err_dp:

• zynqmp_dp_remove(pdev);

+err_pm:

• pm_runtime_disable(&pdev->dev);
• return ret;

+}

+static int zynqmp_dpsub_remove(struct platform_device *pdev) +{

• struct zynqmp_dpsub *dpsub = platform_get_drvdata(pdev);
• int err, ret = 0;
• xlnx_drm_pipeline_exit(dpsub->master);
• of_platform_depopulate(&pdev->dev);
• of_reserved_mem_device_release(&pdev->dev);
• component_del(&pdev->dev, &zynqmp_dpsub_component_ops);
• err = zynqmp_disp_remove(pdev);
• if (err)

• ret = -EIO;
  

• err = zynqmp_dp_remove(pdev);
• if (err)

• ret = -EIO;
  

These two functions always return 0, let's make them void, and drop error handling here.

• pm_runtime_disable(&pdev->dev);
• return err;

+}

+static const struct of_device_id zynqmp_dpsub_of_match[] = {

• { .compatible = "xlnx,zynqmp-dpsub-1.7", },
• { /* end of table */ },

+}; +MODULE_DEVICE_TABLE(of, zynqmp_dpsub_of_match);

+static struct platform_driver zynqmp_dpsub_driver = {

• .probe = zynqmp_dpsub_probe,
• .remove = zynqmp_dpsub_remove,
• .driver = {

• .owner		= THIS_MODULE,
  

• .name		= "zynqmp-display",
  

• .of_match_table	= zynqmp_dpsub_of_match,
  

• },

+};

+module_platform_driver(zynqmp_dpsub_driver);

+MODULE_AUTHOR("Xilinx, Inc."); +MODULE_DESCRIPTION("ZynqMP DP Subsystem Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h new file mode 100644 index 0000000..95239bb --- /dev/null +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • ZynqMP DPSUB Subsystem Driver
• • Copyright (C) 2017 - 2018 Xilinx, Inc.
• • Author: Hyun Woo Kwon hyun.kwon@xilinx.com
• */

+#ifndef _ZYNQMP_DPSUB_H_ +#define _ZYNQMP_DPSUB_H_

+struct device; +struct zynqmp_dp; +struct zynqmp_disp;

zynqmp_disp first in alphabetical order.

+struct zynqmp_dpsub {

• struct zynqmp_dp *dp;
• struct zynqmp_disp *disp;
• struct device *master;

+};

+#endif /* _ZYNQMP_DPSUB_H_ */