From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754611AbdDKJSB (ORCPT ); Tue, 11 Apr 2017 05:18:01 -0400 Received: from mail-io0-f180.google.com ([209.85.223.180]:33978 "EHLO mail-io0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754446AbdDKJR6 (ORCPT ); Tue, 11 Apr 2017 05:17:58 -0400 MIME-Version: 1.0 In-Reply-To: <20170411011801.15788-1-eric@anholt.net> References: <20170411011801.15788-1-eric@anholt.net> From: Linus Walleij Date: Tue, 11 Apr 2017 11:17:51 +0200 Message-ID: Subject: Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111 To: Eric Anholt Cc: "open list:DRM PANEL DRIVERS" , Tom Cooksey , Russell King , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 11, 2017 at 3:18 AM, Eric Anholt wrote: > From: Tom Cooksey Well that can be debated at this point. I think it should have your Author: tag and just Tom in the Signed-off-by, then mention the history in the commit message. > This is a modesetting driver for the pl111 CLCD display controller > found on various ARM platforms such as the Versatile Express. The > driver has only been tested on the bcm911360_entphn platform so far, > with PRIME-based buffer sharing between vc4 and clcd. > > It reuses the existing devicetree binding, while not using quite as > many of its properties as the fbdev driver does (those are left for > future work). > > v2: Nearly complete rewrite by anholt, cutting 2/3 of the code thanks > to DRM core's excellent new helpers. > v3: Don't match pl110 any more, don't attach if we don't have a DRM > panel, use DRM_GEM_CMA_FOPS, update MAINTAINERS, use the simple > display helper, use drm_gem_cma_dumb_create (same as our wrapper). > v4: Change the driver's .name to not clash with fbdev in sysfs, drop > platform alias, drop redundant "drm" in DRM driver name, hook up > .prepare_fb to the CMA helper so that DMA fences should work. > v5: Move register definitions inside the driver directory, fix build > in COMPILE_TEST and !AMBA mode. > > Signed-off-by: Tom Cooksey > Signed-off-by: Eric Anholt Reviewed-by: Linus Walleij This is as good starting point as any. We need to get moving with this. Some minor things below that can just as well be fixed later. > create mode 100644 drivers/gpu/drm/pl111/Kconfig Maybe someone would want to place this under drivers/gpu/drm/arm since it is obviously an ARM block. But since ARM has pretty much dropped the ball on this IP it is probably best to keep it separate like this. > +++ b/drivers/gpu/drm/pl111/Kconfig > @@ -0,0 +1,12 @@ > +config DRM_PL111 > + tristate "DRM Support for PL111 CLCD Controller" > + depends on DRM > + depends on ARM || ARM64 || COMPILE_TEST > + select DRM_KMS_HELPER > + select DRM_KMS_CMA_HELPER > + select DRM_GEM_CMA_HELPER > + select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE > + help > + Choose this option for DRM support for the PL111 CLCD controller. > + If M is selected the module will be called pl111_drm. I must say the driver is *really* slim and readable with all the new helpers from DRM, good job all who refactored the DRM support for simple framebuffer systems. > + panel = of_drm_find_panel(panel_node); > + of_node_put(panel_node); Was meaning to ask whether the panel bindings used by the fbdev drivers are 100% compatible with what DRM is using and from your code it seems like yes, they are? > +irqreturn_t pl111_irq(int irq, void *data) > +{ > + struct pl111_drm_dev_private *priv = data; > + u32 irq_stat; > + irqreturn_t status = IRQ_NONE; > + > + irq_stat = readl(priv->regs + CLCD_PL111_MIS); > + > + if (!irq_stat) > + return IRQ_NONE; > + > + if (irq_stat & CLCD_IRQ_NEXTBASE_UPDATE) { > + drm_crtc_handle_vblank(&priv->pipe.crtc); > + > + status = IRQ_HANDLED; > + } > + > + /* Clear the interrupt once done */ > + writel(irq_stat, priv->regs + CLCD_PL111_ICR); > + > + return status; > +} And this is the first time in history that the vblank interrupt on PL11x is actually used for something, which in itself is a good reason to migrate to this driver. Yours, Linus Walleij