Welcome to DRM Kevin,
On Tue, 10 Dec 2019 at 08:40, Kevin Tang <kevin3.tang@xxxxxxxxx> wrote:
>
> From: Kevin Tang <kevin.tang@xxxxxxxxxx>
>
> Adds drm support for the Unisoc's display subsystem.
>
> This is drm device and gem driver. This driver provides support for the
> Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
Did you use XFree86 or Xorg to test this? The XFree86 codebase have been
missing for years.
Out of curiosity - did you try any Wayland, or bare-metal compositor?
> source "drivers/gpu/drm/mcde/Kconfig"
>
> +source "drivers/gpu/drm/sprd/Kconfig"
> +
> # Keep legacy drivers last
>
> menuconfig DRM_LEGACY
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f1c7c4..85ca211 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -122,3 +122,4 @@ obj-$(CONFIG_DRM_LIMA) += lima/
> obj-$(CONFIG_DRM_PANFROST) += panfrost/
> obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
> obj-$(CONFIG_DRM_MCDE) += mcde/
> +obj-$(CONFIG_DRM_SPRD) += sprd/
> diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig
> new file mode 100644
> index 0000000..79f286b
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Kconfig
> @@ -0,0 +1,14 @@
> +config DRM_SPRD
> + tristate "DRM Support for Unisoc SoCs Platform"
> + depends on ARCH_SPRD
> + depends on DRM && OF
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_MIPI_DSI
> + select DRM_PANEL
> + select VIDEOMODE_HELPERS
> + select BACKLIGHT_CLASS_DEVICE
> + help
> + Choose this option if you have a Unisoc chipsets.
s/chipsets/chipset/ ?
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ccflags-y += -Iinclude/drm
> +
> +subdir-ccflags-y += -I$(src)
> +
I think we can drop the includes here, unless there's a specific error
message that you're setting.
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.c
> +#define DRIVER_NAME "sprd"
> +#define DRIVER_DESC "Spreadtrum SoCs' DRM Driver"
> +#define DRIVER_DATE "20180501"
The date is mostly for cosmetic purposes. Yet we're nearly 2020 and
this reads 2018 - update?
<snip>
> +static struct drm_driver sprd_drm_drv = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET |
> + DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
There is no modeset exposed by the driver, let alone an atomic one.
Thus I would drop the following code from this patch and add it with a
patch that uses it.
- tokens - DRIVER_MODESET, DRIVER_ATOMIC)
- no-op modeset/atomic functions just above, and
- vblank/kms code (further down) in bind/unbind
<snip>
> +static int sprd_drm_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + ret = dma_set_mask_and_coherent(&pdev->dev, ~0);
> + if (ret)
> + DRM_ERROR("dma_set_mask_and_coherent failed (%d)\n", ret);
> +
Is the hardware going to work correctly the dma call fails? Should we
use "return ret;" here?
> + return sprd_drm_component_probe(&pdev->dev, &sprd_drm_component_ops);
> +}
> +
> +static int sprd_drm_remove(struct platform_device *pdev)
> +{
> + component_master_del(&pdev->dev, &sprd_drm_component_ops);
> + return 0;
> +}
> +
> +static void sprd_drm_shutdown(struct platform_device *pdev)
> +{
> + struct drm_device *drm = platform_get_drvdata(pdev);
> +
> + if (!drm) {
Can this happen in reality?
<snip>
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Unisoc Inc.
> + */
> +
> +#ifndef _SPRD_DRM_H_
> +#define _SPRD_DRM_H_
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_print.h>
> +
> +struct sprd_drm {
> + struct drm_device *drm;
> + struct drm_atomic_state *state;
> + struct device *dpu_dev;
> + struct device *gsp_dev;
These three are unused. Please add alongside the code which is using them.
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_gem.c
As Thomas pointed out, this is effectively a copy of drm_gem_cma_helper.c
Please drop this file and use the respective CMA functions, instead.
> diff --git a/drivers/gpu/drm/sprd/sprd_gem.h b/drivers/gpu/drm/sprd/sprd_gem.h
> new file mode 100644
> index 0000000..4c10d8a
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_gem.h
Just like the C file - this is effectively a copy of the existing CMA codebase.
HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
