From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 53E1AC83004 for ; Tue, 28 Apr 2020 18:19:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 24DB820B1F for ; Tue, 28 Apr 2020 18:19:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728584AbgD1STC (ORCPT ); Tue, 28 Apr 2020 14:19:02 -0400 Received: from asavdk4.altibox.net ([109.247.116.15]:55688 "EHLO asavdk4.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728392AbgD1STB (ORCPT ); Tue, 28 Apr 2020 14:19:01 -0400 Received: from ravnborg.org (unknown [158.248.194.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by asavdk4.altibox.net (Postfix) with ESMTPS id 53B42804B9; Tue, 28 Apr 2020 20:18:46 +0200 (CEST) Date: Tue, 28 Apr 2020 20:18:45 +0200 From: Sam Ravnborg To: Gareth Williams Cc: Maarten Lankhorst , Maxime Ripard , Sean Paul , David Airlie , Daniel Vetter , Phil Edworthy , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 1/3] drm/db9000: Add Digital Blocks DB9000 LCD Controller Message-ID: <20200428181845.GD27234@ravnborg.org> References: <1587975709-2092-1-git-send-email-gareth.williams.jx@renesas.com> <1587975709-2092-2-git-send-email-gareth.williams.jx@renesas.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1587975709-2092-2-git-send-email-gareth.williams.jx@renesas.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=MOBOZvRl c=1 sm=1 tr=0 a=UWs3HLbX/2nnQ3s7vZ42gw==:117 a=UWs3HLbX/2nnQ3s7vZ42gw==:17 a=kj9zAlcOel0A:10 a=yC-0_ovQAAAA:8 a=8b9GpE9nAAAA:8 a=e5mUnYsNAAAA:8 a=hNxGGNvzTs52B8eoPUEA:9 a=_NC1VaQZqfszIdzZ:21 a=E2HU7c-VcNwjPWcH:21 a=ARyByvqPOCuah2br:21 a=CjuIK1q_8ugA:10 a=QsnFDINu91a9xkgZirup:22 a=T3LWEMljR5ZiDmsYVIUa:22 a=Vxmtnl_E_bksehYqCbjh:22 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Gareth. On Mon, Apr 27, 2020 at 09:21:47AM +0100, Gareth Williams wrote: > Add DRM support for the Digital Blocks DB9000 LCD Controller > with the Renesas RZ/N1 specific changes. > > Signed-off-by: Gareth Williams > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/digital-blocks/Kconfig | 13 + > drivers/gpu/drm/digital-blocks/Makefile | 3 + > drivers/gpu/drm/digital-blocks/db9000-du.c | 953 +++++++++++++++++++++++++++++ > drivers/gpu/drm/digital-blocks/db9000-du.h | 192 ++++++ > 6 files changed, 1164 insertions(+) > create mode 100644 drivers/gpu/drm/digital-blocks/Kconfig > create mode 100644 drivers/gpu/drm/digital-blocks/Makefile > create mode 100644 drivers/gpu/drm/digital-blocks/db9000-du.c > create mode 100644 drivers/gpu/drm/digital-blocks/db9000-du.h The general impression is a well written driver. It looks like it was wrtten some tiem ago and thus fail to take full benefit from the improvements impemented the last year or so. The driver has a size so it is a candidate to belong in the tiny/ directory. If you do not see any other DRM drivers for digital-blocks or that this driver should be extended, then please consider a move to tiny/ If you do so embed the header file in the .c file so it is a single file driver. Other general comments: The driver looks like a one plane - one crtc - one encoder driver. Please use drm_simple - or explain why you cannot use drm_simple. For the encoder use drm_simple_encoder_init A small intro to the driver would be good. For example that is includes a pwm etc. I provided a mix of diverse comments in the following. Looks forward for the next revision! Sam > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 3c88420..159832d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -280,6 +280,8 @@ source "drivers/gpu/drm/mgag200/Kconfig" > > source "drivers/gpu/drm/cirrus/Kconfig" > > +source "drivers/gpu/drm/digital-blocks/Kconfig" > + > source "drivers/gpu/drm/armada/Kconfig" > > source "drivers/gpu/drm/atmel-hlcdc/Kconfig" > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 9f0d2ee..f525807 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -72,6 +72,7 @@ obj-$(CONFIG_DRM_MGAG200) += mgag200/ > obj-$(CONFIG_DRM_V3D) += v3d/ > obj-$(CONFIG_DRM_VC4) += vc4/ > obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus/ > +obj-$(CONFIG_DRM_DB9000) += digital-blocks/ > obj-$(CONFIG_DRM_SIS) += sis/ > obj-$(CONFIG_DRM_SAVAGE)+= savage/ > obj-$(CONFIG_DRM_VMWGFX)+= vmwgfx/ > diff --git a/drivers/gpu/drm/digital-blocks/Kconfig b/drivers/gpu/drm/digital-blocks/Kconfig > new file mode 100644 > index 0000000..436a7c0 > --- /dev/null > +++ b/drivers/gpu/drm/digital-blocks/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0 > +config DRM_DB9000 > + bool "DRM Support for DB9000 LCD Controller" > + depends on DRM && (ARCH_MULTIPLATFORM || COMPILE_TEST) > + select DRM_KMS_HELPER > + select DRM_GEM_CMA_HELPER > + select DRM_KMS_CMA_HELPER > + select DRM_PANEL_BRIDGE > + select VIDEOMODE_HELPERS > + select FB_PROVIDE_GET_FB_UNMAPPED_AREA if FB > + > + help > + Enable DRM support for the DB9000 LCD controller. > diff --git a/drivers/gpu/drm/digital-blocks/Makefile b/drivers/gpu/drm/digital-blocks/Makefile > new file mode 100644 > index 0000000..9f78492 > --- /dev/null > +++ b/drivers/gpu/drm/digital-blocks/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +obj-$(CONFIG_DRM_DB9000) += db9000-du.o > diff --git a/drivers/gpu/drm/digital-blocks/db9000-du.c b/drivers/gpu/drm/digital-blocks/db9000-du.c > new file mode 100644 > index 0000000..d84d446 > --- /dev/null > +++ b/drivers/gpu/drm/digital-blocks/db9000-du.c > @@ -0,0 +1,953 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (C) 2019 Renesas Electronics Europe Ltd. 2020? > + * > + * Author: Gareth Williams > + * > + * Based on ltdc.c > + * Copyright (C) STMicroelectronics SA 2017 > + * > + * Authors: Philippe Cornu > + * Yannick Fertre > + * Fabien Dessenne > + * Mickael Reulier > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include