| Message ID | 1531015538-32268-6-git-send-email-hyun.kwon@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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_ */
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" + 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. 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); + 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) + 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; + + 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; + +struct zynqmp_dpsub { + struct zynqmp_dp *dp; + struct zynqmp_disp *disp; + struct device *master; +}; + +#endif /* _ZYNQMP_DPSUB_H_ */