Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp4610764ybb; Tue, 7 Apr 2020 10:49:30 -0700 (PDT) X-Google-Smtp-Source: APiQypL9VwoRnORiUqnuM4HeH5c1WmwPbUNYjFBkNang3EKvyX0RLmT5s470V26IhNSSfx+u8ht2 X-Received: by 2002:a05:6808:649:: with SMTP id z9mr2376oih.11.1586281770720; Tue, 07 Apr 2020 10:49:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1586281770; cv=none; d=google.com; s=arc-20160816; b=F8eGJcLUw/xTErDdK3EhemSp1a1awE4BHeeTEeBDK6PV7UKwTDVWuahhRaiasm8Uk/ LquRE4cyfxq4TK2avfvmLmwBwc/IaiZttaImu+suwa8nEPzMAiAPbGE0Ah+LDD3Fk+nI o7aQupIwexHC3oX6EdKcz1o+OhAIZET38f1H/9TmwgV4/4unTXwRip7mz9xk25AqoR3z wtQnwgf5ANnpPDc/2ETwxSEa7Tx13Ow8YWfMLsLjVmUi6XGegh5lUVEj+U6OIemxCAwJ IReXW+CaM7RFnrKJg8657DRpX5ikKz+MfEg96Qq571NqAq6b9M8F+5NIw4+e/OW/jVWT Ma7g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=WqF+Bu6mC5pGnGwU2YM8fiP9WBp1qnCcNtRukrd6zaQ=; b=R+NN+vcHKKcDxjdXDfph4b8LWrwPrfyYFVAiPoneg1i9teR8hoIqhNfcYZUQiAA/+K M29109dKRLtCOvylXwg8RNzH1Bm+/8cUTtRktsAxO3c1OTnNd++39D6Ho3da52md9Qi3 eqFy9sQcDIcj9P9osUK72RNKxDNkGocImqB+zFWBQJWDmpsGNIzkPxU8ycC2JQt9+hbl bKaSF5erYljDfz/bsAEzr0kycygW/8XvDlDktW4gTnckuOGBz+buOAz5Bq7ZnXKHyogP v86zHizmyUZCPkRd1rpGaLym/SEid9ChDf49MlZm2Rwvn/uNtnvQgC5Eq516yejJOOZR 29Gw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w16si57062oog.87.2020.04.07.10.49.15; Tue, 07 Apr 2020 10:49:30 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726536AbgDGRrS (ORCPT + 99 others); Tue, 7 Apr 2020 13:47:18 -0400 Received: from asavdk4.altibox.net ([109.247.116.15]:41806 "EHLO asavdk4.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726510AbgDGRrS (ORCPT ); Tue, 7 Apr 2020 13:47:18 -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 8B8E18048A; Tue, 7 Apr 2020 19:47:06 +0200 (CEST) Date: Tue, 7 Apr 2020 19:47:04 +0200 From: Sam Ravnborg To: Angelo Ribeiro Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, maarten.lankhorst@linux.intel.com, mripard@kernel.org, airlied@linux.ie, daniel@ffwll.ch, Gustavo.Pimentel@synopsys.com, Joao.Pinto@synopsys.com Subject: Re: [PATCH v2 2/4] drm: ipk: Add DRM driver for DesignWare IPK DSI Message-ID: <20200407174704.GA6356@ravnborg.org> References: <488ff0f31581967517607e6860ab520839e29635.1586174459.git.angelo.ribeiro@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <488ff0f31581967517607e6860ab520839e29635.1586174459.git.angelo.ribeiro@synopsys.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=XpTUx2N9 c=1 sm=1 tr=0 a=UWs3HLbX/2nnQ3s7vZ42gw==:117 a=UWs3HLbX/2nnQ3s7vZ42gw==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=QyXUC8HyAAAA:8 a=VwQbUJbxAAAA:8 a=jIQo8A4GAAAA:8 a=e5mUnYsNAAAA:8 a=4Off-LMMQjLtHQkNATwA:9 a=BqbDF8qqGYvOUDtP:21 a=wzVLAJofguk4pnjZ:21 a=CjuIK1q_8ugA:10 a=AjGcO6oz07-iQ99wixmX:22 a=Lf5xNeLK5dgiOs8hzIjU: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 Angelo. Has it been evaluated if the drm_simple_* stuff can be used? It looks like this is a single plane, single connector thing so a good candidate. Some nits below. Sam On Mon, Apr 06, 2020 at 03:24:12PM +0200, Angelo Ribeiro wrote: > Add support for Synopsys DesignWare VPG (Video Pattern Generator) and > DRM driver for Synopsys DesignWare DSI Host IPK solution. > > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: David Airlie > Cc: Daniel Vetter > Cc: Gustavo Pimentel > Cc: Joao Pinto > Signed-off-by: Angelo Ribeiro > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/ipk/Kconfig | 13 + > drivers/gpu/drm/ipk/Makefile | 6 + > drivers/gpu/drm/ipk/dw-drv.c | 189 +++++++++++++++ > drivers/gpu/drm/ipk/dw-ipk.h | 30 +++ > drivers/gpu/drm/ipk/dw-vpg.c | 559 +++++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/ipk/dw-vpg.h | 55 +++++ > 8 files changed, 855 insertions(+) > create mode 100644 drivers/gpu/drm/ipk/Kconfig > create mode 100644 drivers/gpu/drm/ipk/Makefile > create mode 100644 drivers/gpu/drm/ipk/dw-drv.c > create mode 100644 drivers/gpu/drm/ipk/dw-ipk.h > create mode 100644 drivers/gpu/drm/ipk/dw-vpg.c > create mode 100644 drivers/gpu/drm/ipk/dw-vpg.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 4359497..29ea1d1 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -388,6 +388,8 @@ source "drivers/gpu/drm/mcde/Kconfig" > > source "drivers/gpu/drm/tidss/Kconfig" > > +source "drivers/gpu/drm/ipk/Kconfig" > + > # Keep legacy drivers last > > menuconfig DRM_LEGACY > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 183c600..5bcc1c1 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -125,3 +125,4 @@ obj-$(CONFIG_DRM_PANFROST) += panfrost/ > obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/ > obj-$(CONFIG_DRM_MCDE) += mcde/ > obj-$(CONFIG_DRM_TIDSS) += tidss/ > +obj-$(CONFIG_DRM_IPK) += ipk/ > diff --git a/drivers/gpu/drm/ipk/Kconfig b/drivers/gpu/drm/ipk/Kconfig > new file mode 100644 > index 0000000..1f87444 > --- /dev/null > +++ b/drivers/gpu/drm/ipk/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config DRM_IPK > + tristate "DRM Support for Synopsys DesignWare IPK DSI" > + depends on DRM > + select DRM_KMS_HELPER > + select DRM_GEM_CMA_HELPER > + select DRM_KMS_CMA_HELPER > + select DRM_PANEL_BRIDGE > + select VIDEOMODE_HELPERS > + help > + Enable support for the Synopsys DesignWare DRM DSI. > + To compile this driver as a module, choose M here: the module > + will be called ipk-drm. > diff --git a/drivers/gpu/drm/ipk/Makefile b/drivers/gpu/drm/ipk/Makefile > new file mode 100644 > index 0000000..51d2774 > --- /dev/null > +++ b/drivers/gpu/drm/ipk/Makefile > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +ipk-drm-y := \ > + dw-drv.o \ > + dw-vpg.o There is no point in adding '\' here. A simple: ipk-drm-y := dw-drv.o dw-vpg.o Lot's of Makefile uses the ugly '\' where they should use a += instead :-( > +obj-$(CONFIG_DRM_IPK) += ipk-drm.o > diff --git a/drivers/gpu/drm/ipk/dw-drv.c b/drivers/gpu/drm/ipk/dw-drv.c > new file mode 100644 > index 0000000..6205f1c > --- /dev/null > +++ b/drivers/gpu/drm/ipk/dw-drv.c > @@ -0,0 +1,189 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019-2020 Synopsys, Inc. and/or its affiliates. > + * Synopsys DesignWare MIPI DSI DRM driver > + * > + * Author: Angelo Ribeiro > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "dw-ipk.h" > +#include "dw-vpg.h" Please order thei ncludes like this: #include [empty line] #include [empty line] #include "dw*.h" Within each block - sort alphabetically. Drop unused includes - drm_probe_helper is IIRC for legacy stuff mostly/only. > + > +static const struct drm_mode_config_funcs dw_ipk_drm_modecfg_funcs = { > + .fb_create = drm_gem_fb_create_with_dirty, > + .atomic_check = drm_atomic_helper_check, > + .atomic_commit = drm_atomic_helper_commit, > +}; > + > +static int dw_ipk_load(struct drm_device *drm) > +{ > + int ret; > + > + drm_mode_config_init(drm); drmm_mode_config_init(drm); And remember to check return value. > + > + drm->mode_config.min_width = 0; > + drm->mode_config.min_height = 0; > + > + /* To handle orientation */ > + drm->mode_config.max_width = 2048; > + drm->mode_config.max_height = 2048; > + > + drm->mode_config.funcs = &dw_ipk_drm_modecfg_funcs; > + > + /* TODO > + * Optional framebuffer memory resources allocation > + */ > + > + ret = vpg_load(drm); > + if (ret) > + return ret; > + > + /* Calls all the crtc's, encoder's and connector's reset */ > + drm_mode_config_reset(drm); > + > + /* Initialize and enable output polling */ > + drm_kms_helper_poll_init(drm); > + > + return ret; > +} > + > +static void dw_ipk_unload(struct drm_device *drm) > +{ > + DRM_DEBUG_DRIVER("\n"); when you have a drm_device - then please use drm_xxx for debug/warnings/errors - and also drm_WARN as applicable. This goes for everywhere that DRM_XXX is used. In this case use: drm_dbg(drm, "\n"); > + > + drm_kms_helper_poll_fini(drm); > + vpg_unload(drm); > +} > + > +DEFINE_DRM_GEM_CMA_FOPS(ipk_drm_driver_fops); > + > +static int ipk_gem_cma_dumb_create(struct drm_file *file, > + struct drm_device *dev, > + struct drm_mode_create_dumb *args) > +{ > + unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8); > + int err; > + > + /* > + * In order to optimize data transfer, pitch is aligned on > + * 128 bytes, height is aligned on 4 bytes > + */ > + args->pitch = roundup(min_pitch, 128); > + args->height = roundup(args->height, 4); > + > + err = drm_gem_cma_dumb_create_internal(file, dev, args); > + if (err < 0) > + drm_err(dev, "dumb_create failed %d\n", err); > + > + return err; > +} > + > +static struct drm_driver dw_ipk_drm_driver = { > + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC, > + .name = "dw_ipk", > + .desc = "DW IPK DSI Host Controller", > + .date = "20190725", > + .major = 1, > + .minor = 0, > + .patchlevel = 0, > + .fops = &ipk_drm_driver_fops, > + .dumb_create = ipk_gem_cma_dumb_create, > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > + .gem_free_object_unlocked = drm_gem_cma_free_object, From the documentation: This is deprecated and should not be used by new drivers. Use &drm_gem_object_funcs.free instead. > + .gem_vm_ops = &drm_gem_cma_vm_ops, > + .gem_prime_export = drm_gem_prime_export, From documentation: Export hook for GEM drivers. Deprecated in favour of &drm_gem_object_funcs.export. > + .gem_prime_import = drm_gem_prime_import, From documentation: This defaults to drm_gem_prime_import() if not set. > + .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > + .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > + .gem_prime_vmap = drm_gem_cma_prime_vmap, > + .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > + .gem_prime_mmap = drm_gem_cma_prime_mmap, > +}; Did not check all - please go through drm_drv.h - and fix so the driver do not use any deprecated stuff. > + > +static int dw_ipk_drm_platform_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct drm_device *drm; > + struct ipk_device *ipk; > + int ret; > + > + DRM_DEBUG_DRIVER("\n"); > + > + ipk = kzalloc(sizeof(*ipk), GFP_KERNEL); drmm_kzalloc() - but see feedback from Daniel too. > + if (!ipk) > + return -ENOMEM; > + > + ipk->platform = pdev; > + drm = &ipk->drm; > + > + ret = drm_dev_init(&ipk->drm, &dw_ipk_drm_driver, dev); > + if (ret) { > + kfree(ipk); > + return ret; > + } > + > + platform_set_drvdata(pdev, drm); > + > + ret = dw_ipk_load(drm); > + if (ret) > + goto err_put; > + > + ret = drm_dev_register(drm, 0); > + if (ret) > + goto err_put; > + > + drm_fbdev_generic_setup(drm, 24); > + > + return ret; > + > +err_put: > + drm_dev_put(drm); > + return ret; > +} > + > +static int dw_ipk_drm_platform_remove(struct platform_device *pdev) > +{ > + struct drm_device *drm = platform_get_drvdata(pdev); > + > + drm_dev_unregister(drm); > + dw_ipk_unload(drm); > + drm_dev_put(drm); > + > + return 0; > +} > + > +static const struct of_device_id dw_ipk_dt_ids[] = { > + {.compatible = "snps,dw-ipk-vpg"}, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, dw_ipk_dt_ids); > + > +static struct platform_driver dw_ipk_drm_platform_driver = { > + .probe = dw_ipk_drm_platform_probe, > + .remove = dw_ipk_drm_platform_remove, > + .driver = { > + .name = "dw-ipk-drm", > + .of_match_table = dw_ipk_dt_ids, > + }, > +}; > + > +module_platform_driver(dw_ipk_drm_platform_driver); > + > +MODULE_DESCRIPTION("Synopsys DesignWare IPK DRM driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Angelo Ribeiro "); > diff --git a/drivers/gpu/drm/ipk/dw-ipk.h b/drivers/gpu/drm/ipk/dw-ipk.h > new file mode 100644 > index 0000000..4abb6dd > --- /dev/null > +++ b/drivers/gpu/drm/ipk/dw-ipk.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2019-2020 Synopsys, Inc. and/or its affiliates. > + * Synopsys DesignWare MIPI DSI Controller > + */ > + > +#ifndef _DW_IPK_H > +#define _DW_IPK_H > + > +#include "drm/drm_device.h" > +#include Add the inlcudes so the header file is standalone . the header file should not relay on other files being includes. Use forward as appropriate - like for example: struct drm_bridge; struct drm_framebuffer; stuct drm_plane; Keep the forwards alphabetically sorted. > + > +struct ipk_pipeline { > + struct drm_framebuffer *fb; > + struct drm_crtc crtc; > + struct drm_plane *plane; > + struct drm_bridge *bridge; > +}; > + > +struct ipk_device { > + struct drm_device drm; > + struct platform_device *platform; > + struct ipk_pipeline pipeline; > + struct vpg_device *vpg; > +}; > + > +#define drm_device_to_ipk_device(target) \ > + container_of(target, struct ipk_device, drm) > + > +#endif /* _DW_IPK_H */ > diff --git a/drivers/gpu/drm/ipk/dw-vpg.c b/drivers/gpu/drm/ipk/dw-vpg.c > new file mode 100644 > index 0000000..feb3e90 > --- /dev/null > +++ b/drivers/gpu/drm/ipk/dw-vpg.c > @@ -0,0 +1,559 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019-2020 Synopsys, Inc. and/or its affiliates. > + * Synopsys DesignWare MIPI DSI controller > + * > + * Author: Angelo Ribeiro > + * Author: Luis Oliveira > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include