RE: [PATCH 1/3] drm/db9000: Add Digital Blocks DB9000 LCD Controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sam/Daniel,

This is all very useful feedback, thank you. 

On Tue, Apr 28, 2020 at 19:24 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
> 
> On Tue, Apr 28, 2020 at 8:18 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
> >
> > 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 <gareth.williams.jx@xxxxxxxxxxx>
> > > ---
> > >  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.
That's okay with me, I'll merge the two files and move it to tinydrm.

> >
> > 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.
I'll take a look at this in more detail. I've tested with drm_simple and
it is feasible. Thanks for the direction.

> >
> > 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'll add an introduction to the commit message and cover letter.

> 
> One more since I just landed my series: Please use devm_drm_dev_alloc.
> There's a ton of examples now in-tree.
> -Daniel
Thanks Daniel, I might have missed that otherwise.

> 
> >
> > 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?
I'll correct this in the next version, thanks.

> >
> > > + *
> > > + * Author: Gareth Williams <gareth.williams.jx@xxxxxxxxxxx>
> > > + *
> > > + * Based on ltdc.c
> > > + * Copyright (C) STMicroelectronics SA 2017
> > > + *
> > > + * Authors: Philippe Cornu <philippe.cornu@xxxxxx>
> > > + *          Yannick Fertre <yannick.fertre@xxxxxx>
> > > + *          Fabien Dessenne <fabien.dessenne@xxxxxx>
> > > + *          Mickael Reulier <mickael.reulier@xxxxxx>
> > > + */
> > > +#include <drm/drm_atomic.h>
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_device.h>
> > > +#include <drm/drm_fb_cma_helper.h>
> > > +#include <drm/drm_fb_helper.h>
> > > +#include <drm/drm_fourcc.h>
> > > +#include <drm/drm_gem_framebuffer_helper.h>
> > > +#include <drm/drm_gem_cma_helper.h> #include <drm/drm_of.h>
> > > +#include <drm/drm_panel.h> #include <drm/drm_plane_helper.h>
> > > +#include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>
> > > +#include <drm/drm_drv.h>
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#include <video/display_timing.h>
> > > +#include <video/videomode.h>
> > > +
> > > +#include "db9000-du.h"
> > > +
> > > +#define NR_CRTC              1
> > > +#define DB9000_FB_MAX_WIDTH  4095
> > > +#define DB9000_FB_MAX_HEIGHT 4095
> > > +#define RZN1_REGS            ((void *) 1)
> > > +
> > > +static const u32 db9000_fmts[] = {
> > > +     DRM_FORMAT_RGB888,
> > > +     DRM_FORMAT_RGB565,
> > > +     DRM_FORMAT_XRGB1555,
> > > +     DRM_FORMAT_XRGB8888,
> > > +     DRM_FORMAT_BGR888,
> > > +     DRM_FORMAT_BGR565,
> > > +     DRM_FORMAT_XBGR1555,
> > > +     DRM_FORMAT_XBGR8888
> > > +};
> > > +
> > > +static u32 reg_read(void __iomem *base, u32 reg) {
> > > +     return readl(base + reg);
> > > +}
> > > +
> > > +static void reg_write(void __iomem *base, u32 reg, u32 val) {
> > > +     writel(val, base + reg);
> > > +}
> > Thiese helpers do not really add any value.
> > Consider using db9000_device* as first argument, so you do not need to
> > do a "->regs" for every use.
> > Or drop them.
I'll change the first argument as that might help readability as well.

> >
> > db9000_device is so much more than a device.
> > Why not just name it struct db9000, and then use db9000 as the
> > preferred variable name?
> > Just a personal preference, feel free to ignore.
I think this is a good observation. The struct expanded in purpose
over time, I will adjust the name. 

> >
> >
> > > +
> > > +static struct db9000_device *crtc_to_db9000(struct drm_crtc *crtc)
> > > +{
> > > +     return container_of(crtc->dev, struct db9000_device, drm); }
> > > +
> > > +static struct db9000_device *plane_to_db9000(struct drm_plane
> > > +*plane) {
> > > +     return container_of(plane->dev, struct db9000_device, drm); }
> > > +
> > > +static struct db9000_device *pwm_chip_to_db9000(struct pwm_chip
> > > +*chip) {
> > > +     return container_of(chip, struct db9000_device, pwm); }
> > > +
> > > +void db9000_bpp_setup(struct db9000_device *db9000_dev, int bpp,
> int bus_width,
> > > +                   bool pixelSelect)
> >
> > The linux kernel style is small caps and underscore like this:
> > pixel_select
I'll correct this in the next version, thanks.

> >
> > > +{
> > > +     u32 format;
> > > +     u32 reg_cr1 = reg_read(db9000_dev->regs, DB9000_CR1);
> > > +
> > > +     /* reset the BPP bits */
> > > +     reg_cr1 &= ~DB9000_CR1_BPP(7);
> > > +     reg_cr1 &= ~DB9000_CR1_OPS(5);
> > > +     reg_cr1 &= ~DB9000_CR1_OPS(1);
> > > +     db9000_dev->bpp = bpp;
> > > +
> > > +     switch (bpp) {
> > > +     case 16:
> > > +             if (pixelSelect) {
> > > +                     reg_cr1 |= DB9000_CR1_OPS(5);
> > > +                     reg_cr1 |= DB9000_CR1_OPS(1);
> > > +             }
> > > +
> > > +             format = DB9000_CR1_BPP(DB9000_CR1_BPP_16bpp);
> > > +             break;
> > > +     case 24:
> > > +     case 32:
> > > +     default:
> > > +             format = DB9000_CR1_BPP(DB9000_CR1_BPP_24bpp);
> > > +     }
> > > +
> > > +     if (bpp <= 16 && bus_width == 24)
> > > +             reg_cr1 |= DB9000_CR1_OPS(2);
> > > +     else
> > > +             reg_cr1 &= ~DB9000_CR1_OPS(2);
> > > +
> > > +     if (bpp == 24)
> > > +             reg_cr1 |= DB9000_CR1_FBP;
> > > +     else
> > > +             reg_cr1 &= ~DB9000_CR1_FBP;
> > > +
> > > +     reg_cr1 |= format;
> > > +     reg_write(db9000_dev->regs, DB9000_CR1, reg_cr1); }
> > > +
> > > +void db9000_toggle_controller(struct db9000_device *db9000_dev,
> > > +bool on)
> >
> > USe two helpers here:
> > db9000_controller_on()
> > db9000_controller_off()
> >
> > More descriptive than the bool flag.
I'll implement this as two helpers.

> >
> > > +{
> > > +     u32 isr;
> > > +     u32 reg_cr1 = reg_read(db9000_dev->regs, DB9000_CR1);
> > > +     unsigned long flags;
> > > +
> > > +     if (on) {
> > > +             /* Enable controller */
> > > +             reg_cr1 |= DB9000_CR1_LCE;
> > > +             reg_cr1 |= DB9000_CR1_LPE;
> > > +
> > > +             /* DMA Burst Size */
> > > +             reg_cr1 |= DB9000_CR1_FDW(2);
> > > +
> > > +             /* Release pixel clock domain reset */
> > > +             reg_write(db9000_dev->regs, DB9000_PCTR,
> > > + DB9000_PCTR_PCR);
> > > +
> > > +             /* Enable BAU event for IRQ */
> > > +             spin_lock_irqsave(&db9000_dev->lock, flags);
> > > +             isr = reg_read(db9000_dev->regs, DB9000_ISR);
> > > +             reg_write(db9000_dev->regs, DB9000_ISR, isr |
> DB9000_ISR_BAU);
> > > +             reg_write(db9000_dev->regs, DB9000_IMR,
> DB9000_IMR_BAUM);
> > > +             spin_unlock_irqrestore(&db9000_dev->lock, flags);
> > > +
> > > +     } else {
> > > +             /* Disable controller */
> > > +             reg_cr1 &= ~DB9000_CR1_LCE;
> > > +             reg_cr1 &= ~DB9000_CR1_LPE;
> > > +     }
> > > +
> > > +     reg_write(db9000_dev->regs, DB9000_CR1, reg_cr1); }
> > > +
> > > +/* CRTC Functions */
> > > +static void db9000_crtc_atomic_enable(struct drm_crtc *crtc,
> > > +                                   struct drm_crtc_state
> > > +*old_state) {
> > > +     struct db9000_device *db9000_dev = crtc_to_db9000(crtc);
> > > +     u32 imr;
> > > +
> > > +     /* Enable IRQ */
> > > +     imr = reg_read(db9000_dev->regs, DB9000_IMR);
> > > +     reg_write(db9000_dev->regs, DB9000_IMR, imr |
> > > +DB9000_IMR_BAUM); }
> > > +
> > > +static void db9000_crtc_atomic_disable(struct drm_crtc *crtc,
> > > +                                    struct drm_crtc_state
> > > +*old_state) {
> > > +     struct db9000_device *db9000_dev = crtc_to_db9000(crtc);
> > > +     u32 imr;
> > > +
> > > +     /* disable IRQ */
> > > +     imr = reg_read(db9000_dev->regs, DB9000_IMR);
> > > +     reg_write(db9000_dev->regs, DB9000_IMR, imr &
> > > +~DB9000_IMR_BAUM); }
> > > +
> > > +static void db9000_crtc_mode_set_nofb(struct drm_crtc *crtc) {
> > > +     struct drm_display_mode *mode = &crtc->state->adjusted_mode;
> > > +     struct db9000_device *db9000_dev = crtc_to_db9000(crtc);
> > > +     struct videomode vm;
> > > +     int bus_flags = db9000_dev->connector->display_info.bus_flags;
> > > +     u32 vtr1, vtr2, hvter, htr, hpplor, dear_offset;
> > > +     u32 reg_cr1 = reg_read(db9000_dev->regs, DB9000_CR1);
> > > +
> > > +     drm_display_mode_to_videomode(mode, &vm);
> > There is no need for this conversion, the timing can be fetched from
> > display_mode.
> > And this is the only use of videomode, so you can drop the select in
> > Kconfig too, and the include.
I have it working from the display mode, so I can include it in the next
patch version.

> >
> > > +
> > > +     if (mode->flags & DISPLAY_FLAGS_HSYNC_HIGH)
> > > +             reg_cr1 |= DB9000_CR1_HSP;
> > > +     else
> > > +             reg_cr1 &= ~DB9000_CR1_HSP;
> > > +
> > > +     if (mode->flags & DISPLAY_FLAGS_VSYNC_HIGH)
> > > +             reg_cr1 |= DB9000_CR1_VSP;
> > > +     else
> > > +             reg_cr1 &= ~DB9000_CR1_VSP;
> > > +
> > > +     if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
> > > +             reg_cr1 |= DB9000_CR1_DEP;
> > > +     else
> > > +             reg_cr1 &= ~DB9000_CR1_DEP;
> > > +
> > > +     /* Horizontal Timing Register */
> > > +     htr =   DB9000_HTR_HSW(vm.hsync_len) |
> > > +             DB9000_HTR_HBP(vm.hback_porch) |
> > > +             /* Pixels per line */
> > > +             DB9000_HTR_HFP(vm.hfront_porch);
> > > +
> > > +     /* Horizontal Pixels-Per-Line Override */
> > > +     hpplor = vm.hactive | DB9000_HPOE;
> > > +
> > > +     /* Vertical timing registers setup */
> > > +     vtr1 =  DB9000_VTR1_VBP(vm.vback_porch) |
> > > +             DB9000_VTR1_VFP(vm.vfront_porch) |
> > > +             DB9000_VTR1_VSW(vm.vsync_len);
> > > +
> > > +     vtr2 = DB9000_VTR2_LPP(vm.vactive);
> > > +
> > > +     /* Vertical and Horizontal Timing Extension write */
> > > +     hvter = DB9000_HVTER_HFPE(vm.hfront_porch) |
> > > +             DB9000_HVTER_HBPE(vm.hback_porch) |
> > > +             DB9000_HVTER_VFPE(vm.vback_porch) |
> > > +             DB9000_HVTER_VBPE(vm.vfront_porch);
> > > +
> > > +     db9000_dev->frame_size = vm.hactive * vm.vactive;
> > > +
> > > +     /* DEAR register must be configured to the block end + 8 */
> > > +     dear_offset =
> > > +             (db9000_dev->frame_size * db9000_dev->bpp) / 8 + 8;
> > > +
> > > +     reg_write(db9000_dev->regs, DB9000_CR1, reg_cr1);
> > > +     reg_write(db9000_dev->regs, DB9000_HTR, htr);
> > > +     reg_write(db9000_dev->regs, DB9000_VTR1, vtr1);
> > > +     reg_write(db9000_dev->regs, DB9000_VTR2, vtr2);
> > > +     reg_write(db9000_dev->regs, DB9000_HPPLOR, hpplor);
> > > +     reg_write(db9000_dev->regs, DB9000_HVTER, hvter);
> > > +
> > > +     DRM_DEBUG_DRIVER("CRTC:%d mode:%s\n", crtc->base.id, mode-
> >name);
> > > +     DRM_DEBUG_DRIVER("Video mode: %dx%d", vm.hactive,
> vm.vactive);
> > > +     DRM_DEBUG_DRIVER(" hfp %d hbp %d hsl %d vfp %d vbp %d vsl
> %d\n",
> > > +                      vm.hfront_porch, vm.hback_porch, vm.hsync_len,
> > > +                      vm.vfront_porch, vm.vback_porch,
> > > + vm.vsync_len);
> > As a general comment:
> > use drm_dbg(db9000->drm, "foo") and not the deprecated
> DRM_DEBUG_DRIVER.
Okay, I will correct this.

> >
> > > +}
> > > +
> > > +static void db9000_crtc_atomic_flush(struct drm_crtc *crtc,
> > > +                                  struct drm_crtc_state
> > > +*old_crtc_state) {
> > > +     struct drm_pending_vblank_event *event = crtc->state->event;
> > > +
> > > +     if (event) {
> > > +             crtc->state->event = NULL;
> > > +
> > > +             spin_lock_irq(&crtc->dev->event_lock);
> > > +             drm_crtc_send_vblank_event(crtc, event);
> > > +             spin_unlock_irq(&crtc->dev->event_lock);
> > > +     }
> > > +}
> > > +
> > > +static const struct drm_crtc_helper_funcs db9000_crtc_helper_funcs = {
> > > +     .mode_set_nofb = db9000_crtc_mode_set_nofb,
> > > +     .atomic_flush = db9000_crtc_atomic_flush,
> > > +     .atomic_enable = db9000_crtc_atomic_enable,
> > > +     .atomic_disable = db9000_crtc_atomic_disable, };
> > > +
> > > +static const struct drm_crtc_funcs db9000_crtc_funcs = {
> > > +     .destroy = drm_crtc_cleanup,
> > > +     .set_config = drm_atomic_helper_set_config,
> > > +     .page_flip = drm_atomic_helper_page_flip,
> > > +     .reset = drm_atomic_helper_crtc_reset,
> > > +     .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> > > +     .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> > > +     .gamma_set = drm_atomic_helper_legacy_gamma_set,
> > > +};
> > > + 
> > > +/*
> > > + * DRM_PLANE
> > > + */
> > > +static void db9000_plane_atomic_update(struct drm_plane *plane,
> > > +                                    struct drm_plane_state
> > > +*oldstate) {
> > > +     struct db9000_device *db9000_dev = plane_to_db9000(plane);
> > > +     struct drm_plane_state *state = plane->state;
> > > +     struct drm_framebuffer *fb = state->fb;
> > > +     unsigned long flags;
> > > +     u32 isr, paddr, dear_offset;
> > > +     u32 format;
> > > +
> > > +     if (!state->crtc || !fb) {
> > > +             DRM_DEBUG_DRIVER("fb or crtc NULL\n");
> > > +             return;
> > > +     }
> > > +
> > > +     format = fb->format->format;
> > > +
> > > +     /* The single plane is turning visible, so turn on the display */
> > > +     if (!oldstate->visible && state->visible)
> > > +             db9000_toggle_controller(db9000_dev, false);
> > > +
> > > +     /* The plane is no longer visible */
> > > +     if (oldstate->visible && !state->visible)
> > > +             db9000_toggle_controller(db9000_dev, true);
> > > +
> > > +     /* Check for format changes */
> > > +     if (format == DRM_FORMAT_RGB565 || format ==
> DRM_FORMAT_BGR565)
> > > +             db9000_bpp_setup(db9000_dev, 16, db9000_dev->bus_width,
> false);
> > > +     else if (format == DRM_FORMAT_XRGB1555 || format ==
> DRM_FORMAT_XBGR1555)
> > > +             db9000_bpp_setup(db9000_dev, 16, db9000_dev->bus_width,
> true);
> > > +     else if (format == DRM_FORMAT_RGB888 || format ==
> DRM_FORMAT_BGR888)
> > > +             db9000_bpp_setup(db9000_dev, 24, db9000_dev->bus_width,
> false);
> > > +     else if (format == DRM_FORMAT_XRGB8888 || format ==
> DRM_FORMAT_XBGR8888)
> > > +             db9000_bpp_setup(db9000_dev, 32,
> > > + db9000_dev->bus_width, false);
> > > +
> > > +     dear_offset = (db9000_dev->frame_size * db9000_dev->bpp) / 8 +
> > > + 8;
> > > +
> > > +     /* The frame buffer has changed, so get the new FB address */
> > > +     if (oldstate->fb != state->fb || state->crtc->state->mode_changed) {
> > > +             paddr = (u32)drm_fb_cma_get_gem_addr(fb, state, 0);
> > > +
> > > +             DRM_DEBUG_DRIVER("fb: phys 0x%08x\n", paddr);
> > > +             reg_write(db9000_dev->regs, DB9000_DBAR, paddr);
> > > +             reg_write(db9000_dev->regs, DB9000_DEAR,
> > > +                       paddr + dear_offset);
> > > +     }
> > > +
> > > +     /* Enable BAU event */
> > > +     spin_lock_irqsave(&db9000_dev->lock, flags);
> > > +     isr = reg_read(db9000_dev->regs, DB9000_ISR);
> > > +     reg_write(db9000_dev->regs, DB9000_ISR, isr | DB9000_ISR_BAU);
> > > +     reg_write(db9000_dev->regs, DB9000_IMR, DB9000_IMR_BAUM);
> > > +     spin_unlock_irqrestore(&db9000_dev->lock, flags);
> > > +
> > > +     db9000_dev->plane_fpsi->counter++;
> > > +
> > > +     if (isr & DB9000_ISR_MBE) {
> > > +             if (isr & DB9000_ISR_OFU)
> > > +                     DRM_ERROR("Output FIFO Underrun\n");
> > > +
> > > +             if (isr & DB9000_ISR_OFO)
> > > +                     DRM_ERROR("Output FIFO Overrun\n");
> > > +
> > > +             if (isr & DB9000_ISR_IFU)
> > > +                     DRM_ERROR("Input FIFO Underrun\n");
> > > +
> > > +             if (isr & DB9000_ISR_IFO)
> > > +                     DRM_ERROR("Input FIFO Overrun\n");
> > > +     }
> > > +}
> > > +
> > > +static void db9000_plane_atomic_print_state(struct drm_printer *p,
> > > +                                         const struct
> > > +drm_plane_state *state) {
> > > +     struct drm_plane *plane = state->plane;
> > > +     struct db9000_device *db9000_dev = plane_to_db9000(plane);
> > > +     struct fps_info *fpsi = db9000_dev->plane_fpsi;
> > > +     int ms_since_last;
> > > +     ktime_t now;
> > > +
> > > +     now = ktime_get();
> > > +     ms_since_last = ktime_to_ms(ktime_sub(now,
> > > + fpsi->last_timestamp));
> > > +
> > > +     drm_printf(p, "\tuser_updates=%dfps\n",
> > > +                DIV_ROUND_CLOSEST(fpsi->counter * 1000,
> > > + ms_since_last));
> > > +
> > > +     fpsi->last_timestamp = now;
> > > +     fpsi->counter = 0;
> > > +}
> > > +
> > > +static const struct drm_plane_funcs db9000_plane_funcs = {
> > > +     .update_plane = drm_atomic_helper_update_plane,
> > > +     .disable_plane = drm_atomic_helper_disable_plane,
> > > +     .destroy = drm_plane_cleanup,
> > > +     .reset = drm_atomic_helper_plane_reset,
> > > +     .atomic_duplicate_state =
> drm_atomic_helper_plane_duplicate_state,
> > > +     .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > +     .atomic_print_state = db9000_plane_atomic_print_state, };
> > > +
> > > +static const struct drm_plane_helper_funcs
> db9000_plane_helper_funcs = {
> > > +     .atomic_update = db9000_plane_atomic_update, };
> > > +
> > > +static struct drm_plane *db9000_plane_create(struct drm_device
> *ddev,
> > > +                                          enum drm_plane_type type)
> > > +{
> > > +     struct device *dev = ddev->dev;
> > > +     struct drm_plane *plane;
> > > +     int ret;
> > > +
> > > +     plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
> > > +     if (!plane)
> > > +             return NULL;
> > > +
> > > +     ret = drm_universal_plane_init(ddev, plane, NR_CRTC,
> > > +                                    &db9000_plane_funcs,
> > > +                                    db9000_fmts,
> > > +                                    ARRAY_SIZE(db9000_fmts), NULL,
> > > +                                    type, "rzn1_primary_rgb");
> > > +     if (ret < 0)
> > > +             return NULL;
> > > +
> > > +     drm_plane_helper_add(plane, &db9000_plane_helper_funcs);
> > > +
> > > +     DRM_DEBUG_DRIVER("plane:%d created\n", plane->base.id);
> > > +
> > > +     return plane;
> > > +}
> > > +
> > > +static void db9000_plane_destroy_all(struct drm_device *ddev) {
> > > +     struct drm_plane *plane, *plane_temp;
> > > +
> > > +     list_for_each_entry_safe(plane, plane_temp,
> > > +                              &ddev->mode_config.plane_list, head)
> > > +                              drm_plane_cleanup(plane); }
> > > +
> > > +static int db9000_crtc_init(struct drm_device *ddev, struct
> > > +drm_crtc *crtc) {
> > > +     struct drm_plane *primary;
> > > +     int ret;
> > > +
> > > +     primary = db9000_plane_create(ddev,
> DRM_PLANE_TYPE_PRIMARY);
> > > +     if (!primary) {
> > > +             DRM_ERROR("Can not create primary plane\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     ret = drm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> > > +                                     &db9000_crtc_funcs, NULL);
> > > +     if (ret) {
> > > +             DRM_ERROR("Can not initialize CRTC\n");
> > > +             goto cleanup;
> > > +     }
> > > +
> > > +     drm_crtc_helper_add(crtc, &db9000_crtc_helper_funcs);
> > > +     DRM_DEBUG_DRIVER("CRTC:%d created\n", crtc->base.id);
> > > +     return 0;
> > > +
> > > +cleanup:
> > > +     db9000_plane_destroy_all(ddev);
> > > +     return ret;
> > > +}
> > > +
> > > +/*
> > > + * DRM_ENCODER
> > > + */
> > > +static const struct drm_encoder_funcs db9000_encoder_funcs = {
> > > +     .destroy = drm_encoder_cleanup, };
> > > +
> > > +static int db9000_encoder_init(struct drm_device *ddev,
> > > +                            struct drm_bridge *bridge) {
> > > +     struct drm_encoder *encoder;
> > > +     int ret;
> > > +
> > > +     encoder = devm_kzalloc(ddev->dev, sizeof(*encoder),
> GFP_KERNEL);
> > > +     if (!encoder)
> > > +             return -ENOMEM;
> > > +
> > > +     encoder->possible_crtcs = NR_CRTC;
> > > +     encoder->possible_clones = 0; /* No cloning support */
> > > +
> > > +     drm_encoder_init(ddev, encoder, &db9000_encoder_funcs,
> > > +                      DRM_MODE_ENCODER_NONE, NULL);
> > > +
> > > +     ret = drm_bridge_attach(encoder, bridge, NULL);
> > > +     if (ret) {
> > > +             drm_encoder_cleanup(encoder);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     DRM_DEBUG_DRIVER("Bridge encoder:%d created\n",
> > > + encoder->base.id);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +void __maybe_unused db9000_suspend(struct drm_device *ddev) {
> > > +     struct db9000_device *db9000_dev = container_of(ddev,
> > > +                                                 struct
> > > +db9000_device, drm);
> > > +
> > > +     clk_disable_unprepare(db9000_dev->lcd_eclk);
> > > +}
> > > +
> > > +int __maybe_unused db9000_resume(struct drm_device *ddev) {
> > > +     struct db9000_device *db9000_dev = container_of(ddev,
> > > +                                                 struct db9000_device, drm);
> > > +     int ret;
> > > +
> > > +     ret = clk_prepare_enable(db9000_dev->lcd_eclk);
> > > +     if (ret) {
> > > +             DRM_ERROR("failed to enable pixel clock (%d)\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int db9000_pwm_request(struct pwm_chip *chip, struct
> > > +pwm_device *pwm) {
> > > +     return pm_runtime_get_sync(chip->dev); }
> > > +
> > > +static void db9000_pwm_free(struct pwm_chip *chip, struct
> > > +pwm_device *pwm) {
> > > +     pm_runtime_put(chip->dev);
> > > +}
> > > +
> > > +static int db9000_pwm_enable(struct db9000_device *db9000_dev) {
> > > +     u32 reg_pwmfr = reg_read(db9000_dev->regs,
> > > +db9000_dev->addr_pwmfr);
> > > +
> > > +     reg_pwmfr |= DB9000_PWMFR_PWMFCE;
> > > +     reg_write(db9000_dev->regs, db9000_dev->addr_pwmfr,
> > > + reg_pwmfr);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void db9000_pwm_disable(struct db9000_device *db9000_dev) {
> > > +     u32 reg_pwmfr = reg_read(db9000_dev->regs,
> > > +db9000_dev->addr_pwmfr);
> > > +
> > > +     reg_pwmfr &= ~DB9000_PWMFR_PWMFCE;
> > > +     reg_write(db9000_dev->regs, db9000_dev->addr_pwmfr,
> > > +reg_pwmfr); }
> > > +
> > > +static int db9000_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > > +                         struct pwm_state *state) {
> > > +     struct pwm_state cur_state;
> > > +     struct db9000_device *db9000_dev = pwm_chip_to_db9000(chip);
> > > +     int ret;
> > > +
> > > +     pwm_get_state(pwm, &cur_state);
> > > +
> > > +     if (state->enabled)
> > > +             ret = db9000_pwm_enable(db9000_dev);
> > > +     else
> > > +             db9000_pwm_disable(db9000_dev);
> > > +
> > > +     if (state->period != cur_state.period) {
> > > +             u32 pwmfcd;
> > > +
> > > +             pwmfcd = clk_get_rate(db9000_dev->lcd_eclk) / 256;
> > > +             pwmfcd *= state->period;
> > > +             pwmfcd = DB9000_PWMFR_PWMFCD(pwmfcd - 1);
> > > +             reg_write(db9000_dev->regs, db9000_dev->addr_pwmfr,
> pwmfcd);
> > > +     }
> > > +
> > > +     if (state->duty_cycle == 0) {
> > > +             db9000_pwm_disable(db9000_dev);
> > > +     } else if (state->period != cur_state.period ||
> > > +         state->duty_cycle != cur_state.duty_cycle) {
> > > +             u32 dcr = div_u64((state->duty_cycle * ULL(256)),
> > > +                                state->period) - 1;
> > > +
> > > +             reg_write(db9000_dev->regs, db9000_dev->addr_pwmdcr, dcr);
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static const struct pwm_ops db9000_pwm_ops = {
> > > +     .request = db9000_pwm_request,
> > > +     .free = db9000_pwm_free,
> > > +     .apply = db9000_pwm_apply,
> > > +     .owner = THIS_MODULE,
> > > +};
> > > +
> > > +static int db9000_pwm_setup(struct device *dev,
> > > +                         struct db9000_device *db9000_dev) {
> > > +     struct pwm_chip *db9000_pwm;
> > > +     int ret;
> > > +
> > > +     db9000_pwm = devm_kzalloc(dev, sizeof(*db9000_pwm),
> GFP_KERNEL);
> > > +     if (db9000_pwm == NULL)
> > > +             return -ENOMEM;
> > > +
> > > +     db9000_pwm = &db9000_dev->pwm;
> > > +
> > > +     db9000_pwm->dev = dev;
> > > +     db9000_pwm->ops = &db9000_pwm_ops;
> > > +     db9000_pwm->base = -1;
> > > +     db9000_pwm->npwm = 1;
> > > +
> > > +     ret = pwmchip_add(db9000_pwm);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "failed to register PWM chip: %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     pm_runtime_enable(dev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +int db9000_load(struct drm_device *ddev, int rzn1_pwm) {
> > > +     struct platform_device *pdev = to_platform_device(ddev->dev);
> > > +     struct device *dev = ddev->dev;
> > > +     struct db9000_device *db9000_dev = container_of(ddev,
> > > +                                                 struct db9000_device, drm);
> > > +     struct device_node *np = dev->of_node;
> > > +     struct drm_bridge *bridge;
> > > +     struct drm_panel *panel;
> > > +     struct drm_crtc *crtc;
> > > +     struct reset_control *rstc;
> > > +     struct resource *res;
> > > +     int ret;
> > > +
> > > +     spin_lock_init(&db9000_dev->lock);
> > > +
> > > +     if (rzn1_pwm) {
> > > +             db9000_dev->addr_pwmfr = DB9000_PWMFR_RZN1;
> > > +             db9000_dev->addr_pwmdcr = DB9000_PWMDCR_RZN1;
> > > +     } else {
> > > +             db9000_dev->addr_pwmfr = DB9000_PWMFR_0;
> > > +             db9000_dev->addr_pwmdcr = DB9000_PWMDCR_0;
> > > +     }
> > > +
> > > +     /* Panel Setup */
> > > +     ret = drm_of_find_panel_or_bridge(np, 0, 0, &panel, &bridge);
> > > +     if (ret != 0) {
> > > +             DRM_ERROR("Could not get Panel or bridge");
> > > +             return ret;
> > > +     }
> > > +
> > > +     rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > +
> > > +     /* Clock setup */
> > > +     db9000_dev->lcd_eclk = devm_clk_get(dev, "lcd_eclk");
> > > +
> > > +     if (IS_ERR(db9000_dev->lcd_eclk)) {
> > > +             DRM_ERROR("Unable to get pixel clock\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     if (clk_prepare_enable(db9000_dev->lcd_eclk)) {
> > > +             DRM_ERROR("Unable to prepare pixel clock\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     /* Memory setup */
> > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     if (!res) {
> > > +             DRM_ERROR("Could not retrieve platform resources\n");
> > > +             ret = -EINVAL;
> > > +             goto err;
> > > +     }
> > > +
> > > +     db9000_dev->regs = devm_ioremap_resource(dev, res);
> > > +     if (IS_ERR(db9000_dev->regs)) {
> > > +             DRM_ERROR("Unable to get memory resource\n");
> > > +             ret = PTR_ERR(db9000_dev->regs);
> > > +             goto err;
> > > +     }
> > > +
> > > +     db9000_bpp_setup(db9000_dev, db9000_dev->bpp, db9000_dev-
> >bus_width,
> > > +                      false);
> > > +
> > > +     db9000_toggle_controller(db9000_dev, true);
> > > +
> > > +     /* Add endpoints panels or bridges if any */
> > > +     if (panel) {
> > > +             bridge = drm_panel_bridge_add(panel,
> > > +                                           DRM_MODE_CONNECTOR_Unknown);
> > > +             if (IS_ERR(bridge)) {
> > > +                     DRM_ERROR("panel-bridge endpoint\n");
> > > +                     ret = PTR_ERR(bridge);
> > > +                     goto err;
> > > +             }
> > > +     }
> > > +
> > > +     if (bridge) {
> > > +             ret = db9000_encoder_init(ddev, bridge);
> > > +             if (ret) {
> > > +                     DRM_ERROR("init encoder endpoint\n");
> > > +                     goto err;
> > > +             }
> > > +     }
> > > +
> > > +     db9000_dev->connector = panel->connector;
> > > +
> > > +     /* CRTC setup  */
> > > +     crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
> > > +     if (!crtc) {
> > > +             DRM_ERROR("Failed to allocate crtc\n");
> > > +             ret = -ENOMEM;
> > > +             goto err;
> > > +     }
> > > +
> > > +     ret = db9000_crtc_init(&db9000_dev->drm, crtc);
> > > +     if (ret) {
> > > +             DRM_ERROR("Failed to init crtc\n");
> > > +             goto err;
> > > +     }
> > > +
> > > +     if (!IS_ERR(rstc)) {
> > > +             reset_control_assert(rstc);
> > > +             usleep_range(10, 20);
> > > +             reset_control_deassert(rstc);
> > > +     }
> > > +
> > > +     return db9000_pwm_setup(dev, db9000_dev);
> > > +
> > > +err:
> > > +     drm_panel_bridge_remove(bridge);
> > > +
> > > +     clk_disable_unprepare(db9000_dev->lcd_eclk);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +void db9000_unload(struct drm_device *ddev) {
> > > +     struct db9000_device *db9000_dev;
> > > +
> > > +     db9000_dev = container_of(ddev, struct db9000_device, drm);
> > > +
> > > +     drm_of_panel_bridge_remove(ddev->dev->of_node, 0, 0);
> > > +     clk_disable_unprepare(db9000_dev->lcd_eclk);
> > > +}
> > > +
> > > +static const struct drm_mode_config_funcs drv_mode_config_funcs = {
> > > +     .fb_create = drm_gem_fb_create,
> > > +     .atomic_check = drm_atomic_helper_check,
> > > +     .atomic_commit = drm_atomic_helper_commit, };
> > > +
> > > +static int db9000_gem_cma_dumb_create(struct drm_file *file,
> > > +                                   struct drm_device *dev,
> > > +                                   struct drm_mode_create_dumb
> > > +*args) {
> > > +     return drm_gem_cma_dumb_create_internal(file, dev, args); }
> > This wrapper seems like it can be dropped.
I agree, I will remove this.

> >
> > > +
> > > +DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
> > > +
> > > +static struct drm_driver drv_driver = {
> > > +     .driver_features = DRIVER_MODESET | DRIVER_GEM |
> DRIVER_PRIME |
> > > +                        DRIVER_ATOMIC,
> > > +     .name = "drm-db9000",
> > > +     .desc = "Digital Blocks DB9000 DRM Driver",
> > > +     .date = "20190825",
> > > +     .major = 1,
> > > +     .minor = 0,
> > > +     .patchlevel = 0,
> > > +     .fops = &drv_driver_fops,
> > > +     .dumb_create = db9000_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,
> > > +     .gem_vm_ops = &drm_gem_cma_vm_ops,
> > > +     .gem_prime_export = drm_gem_prime_export,
> > > +     .gem_prime_import = drm_gem_prime_import,
> > > +     .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, };
> > I think if you look up these operations, then you have assinged the
> > default operations in mayny cases.
> > Please see if you can trim the list.
> >
> > Also double check that no legacy operatiosn are used.
I'll simplify this as much as possible.

> >
> > > +
> > > +static int drv_load(struct drm_device *ddev, u32 bpp,
> > > +                 u32 bus_width, int rzn1_pwm) {
> > > +     struct platform_device *pdev = to_platform_device(ddev->dev);
> > > +     struct db9000_device *db9000_dev = container_of(ddev,
> > > +                                                 struct db9000_device, drm);
> > > +     int ret;
> > > +
> > > +     drm_mode_config_init(ddev);
> > > +
> > > +     /*
> > > +      * set max width and height as default value.
> > > +      * this value would be used to check framebuffer size limitation
> > > +      * at drm_mode_addfb().
> > > +      */
> > > +     ddev->mode_config.min_width = 0;
> > > +     ddev->mode_config.min_height = 0;
> > > +     ddev->mode_config.max_width = DB9000_FB_MAX_WIDTH;
> > > +     ddev->mode_config.max_height = DB9000_FB_MAX_WIDTH;
> > > +     ddev->mode_config.funcs = &drv_mode_config_funcs;
> > > +
> > > +     db9000_dev->bus_width = bus_width;
> > > +
> > > +     ret = db9000_load(ddev, rzn1_pwm);
> > > +
> > > +     if (ret)
> > > +             goto err;
> > > +
> > > +     drm_mode_config_reset(ddev);
> > > +
> > > +     drm_kms_helper_poll_init(ddev);
> > > +
> > > +     platform_set_drvdata(pdev, ddev);
> > > +
> > > +     return 0;
> > > +
> > > +err:
> > > +     drm_mode_config_cleanup(ddev);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static void drv_unload(struct drm_device *ddev) {
> > > +     drm_kms_helper_poll_fini(ddev);
> > > +     db9000_unload(ddev);
> > > +     drm_mode_config_cleanup(ddev); }
> > > +
> > > +static __maybe_unused int db9000_drv_suspend(struct device *dev) {
> > > +     struct drm_device *ddev = dev_get_drvdata(dev);
> > > +     struct db9000_device *db9000_dev = container_of(ddev,
> > > +                                                     struct db9000_device,
> > > +                                                     drm);
> > > +     struct drm_atomic_state *state;
> > > +
> > > +     db9000_toggle_controller(db9000_dev, false);
> > > +
> > > +     drm_kms_helper_poll_disable(ddev);
> > > +     state = drm_atomic_helper_suspend(ddev);
> > > +     if (IS_ERR(state)) {
> > > +             drm_kms_helper_poll_enable(ddev);
> > > +             return PTR_ERR(state);
> > > +     }
> > > +     db9000_dev->suspend_state = state;
> > > +     db9000_suspend(ddev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static __maybe_unused int db9000_drv_resume(struct device *dev) {
> > > +     struct drm_device *ddev = dev_get_drvdata(dev);
> > > +     struct db9000_device *db9000_dev = container_of(ddev,
> > > +                                                     struct db9000_device,
> > > +                                                     drm);
> > > +
> > > +     db9000_resume(ddev);
> > > +     drm_atomic_helper_resume(ddev, db9000_dev->suspend_state);
> > > +     drm_kms_helper_poll_enable(ddev);
> > > +     db9000_toggle_controller(db9000_dev, true);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops drv_pm_ops = {
> > > +     SET_SYSTEM_SLEEP_PM_OPS(db9000_drv_suspend,
> db9000_drv_resume)
> > > +};
> > > +
> > > +static int db9000_drm_platform_probe(struct platform_device *pdev)
> > > +{
> > > +     struct device *dev = &pdev->dev;
> > > +     struct drm_device *ddev;
> > > +     struct db9000_device *db9000_dev;
> > > +     struct device_node *np = dev->of_node;
> > > +     u32 bpp;
> > > +     u32 bus_width;
> > > +     int rzn1_pwm = 0;
> > > +     int ret;
> > > +
> > > +     dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> > > +
> > > +     db9000_dev = kzalloc(sizeof(*db9000_dev), GFP_KERNEL);
> > > +     if (!db9000_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     drm_dev_init(&db9000_dev->drm, &drv_driver, dev);
> > > +     ddev = &db9000_dev->drm;
> > Use the newly merged drmm stuff here.
> >
> > > +
> > > +     /* Parse the DTB */
> > > +     ret = of_property_read_u32(np, "bits-per-pixel", &bpp);
> > > +     if (ret)
> > > +             bpp = 16;
> > > +
> > > +     if (bpp != 16 && bpp != 24 && bpp != 32) {
> > > +             DRM_WARN("bits-per-pixel value invalid, default is 16 bpp");
> > > +             bpp = 16;
> > > +     }
> > > +
> > > +     ret = of_property_read_u32(np, "bus-width", &bus_width);
> > > +     if (ret)
> > > +             bus_width = 24;
> > > +
> > > +     rzn1_pwm = (int) of_device_get_match_data(dev);
> > > +
> > > +     ret = drv_load(ddev, bpp, bus_width, rzn1_pwm);
> > > +     if (ret)
> > > +             goto err_put;
> > > +
> > > +     ret = drm_dev_register(ddev, 0);
> > > +     if (ret)
> > > +             goto err_put;
> > > +
> > > +     ret = drm_fbdev_generic_setup(ddev, bpp);
> > > +     if (ret)
> > > +             goto err_put;
> > > +
> > > +     dev_info(dev, "DB9000 LCD Controller Ready");
> > > +
> > > +     return 0;
> > > +
> > > +err_put:
> > > +     drm_dev_put(ddev);
> > With drmm_ support this put goes aways.
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int db9000_drm_platform_remove(struct platform_device *pdev)
> > > +{
> > > +     struct drm_device *ddev = platform_get_drvdata(pdev);
> > > +
> > > +     drv_unload(ddev);
> > > +     drm_dev_put(ddev);
> > With drmm_ support this put goes aways.
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct of_device_id drv_dt_ids[] = {
> > > +     { .compatible = "digital-blocks,drm-db9000"},
> > > +     { .compatible = "digital-blocks,drm-rzn1", .data = RZN1_REGS },
> > > +     { /* end node */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, drv_dt_ids);
> > > +
> > > +static struct platform_driver db9000_drm_platform_driver = {
> > > +     .probe = db9000_drm_platform_probe,
> > > +     .remove = db9000_drm_platform_remove,
> > > +     .driver = {
> > > +             .name = "drm-db9000",
> > > +             .of_match_table = drv_dt_ids,
> > > +             .pm = &drv_pm_ops,
> > > +     },
> > > +};
> > > +
> > > +module_platform_driver(db9000_drm_platform_driver);
> > > +
> > > +MODULE_AUTHOR("Gareth Williams
> <gareth.williams.jx@xxxxxxxxxxx>");
> > > +MODULE_DESCRIPTION("Digital Blocks DB9000 LCD Controller Driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/drivers/gpu/drm/digital-blocks/db9000-du.h
> > > b/drivers/gpu/drm/digital-blocks/db9000-du.h
> > > new file mode 100644
> > > index 0000000..325c9f0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/digital-blocks/db9000-du.h
> > > @@ -0,0 +1,192 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/* Copyright (C) 2019 Renesas Electronics Europe Ltd.
> > > + *
> > > + * Author: Gareth Williams <gareth.williams.jx@xxxxxxxxxxx>
> > > + *
> > > + * Based on ltdc.h
> > > + * Copyright (C) STMicroelectronics SA 2017
> > > + *
> > > + * Authors: Philippe Cornu <philippe.cornu@xxxxxx>
> > > + *          Yannick Fertre <yannick.fertre@xxxxxx>
> > > + *          Fabien Dessenne <fabien.dessenne@xxxxxx>
> > > + *          Mickael Reulier <mickael.reulier@xxxxxx>
> > > + */
> > > +#ifndef __DB9000_DU_H__
> > > +#define __DB9000_DU_H__
> > > +
> > > +#include <linux/backlight.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/pwm.h>
> > > +
> > > +#define DB9000_MAX_LAYER             1
> > > +
> > > +/* LCD Controller Control Register 1 */
> > > +#define DB9000_CR1                   0x000
> > > +/* Horizontal Timing Register */
> > > +#define DB9000_HTR                   0x008
> > > +/* Vertical Timing Register 1 */
> > > +#define DB9000_VTR1                  0x00C
> > > +/* Vertical Timing Register 2 */
> > > +#define DB9000_VTR2                  0x010
> > > +/* Pixel Clock Timing Register */
> > > +#define DB9000_PCTR                  0x014
> > > +/* Interrupt Status Register */
> > > +#define DB9000_ISR                   0x018
> > > +/* Interrupt Mask Register */
> > > +#define DB9000_IMR                   0x01C
> > > +/* Interrupt Vector Register */
> > > +#define DB9000_IVR                   0x020
> > > +/* Interrupt Scan Compare Register */
> > > +#define DB9000_ISCR                  0x024
> > > +/* DMA Base Address Register */
> > > +#define DB9000_DBAR                  0x028
> > > +/* DMA Current Address Register */
> > > +#define DB9000_DCAR                  0x02C
> > > +/* DMA End Address Register */
> > > +#define DB9000_DEAR                  0x030
> > > +/* DMA Horizontal and Vertical Timing Extension Register */
> > > +#define DB9000_HVTER                 0x044
> > > +/* Horizontal Pixels-Per-Line Override Control */
> > > +#define DB9000_HPPLOR                        0x048
> > > +/* Horizontal Pixels-Per-Line Override Enable */
> > > +#define DB9000_HPOE                  BIT(31)
> > > +/* GPIO Register */
> > > +#define DB9000_GPIOR                 0x1F8
> > > +/* Core Identification Register */
> > > +#define DB9000_CIR                   0x1FC
> > > +/* Palette Data Words */
> > > +#define DB9000_PALT                  0x200
> > > +
> > > +/* Control Register 1, Offset 0x000 */
> > > +/* LCD Controller Enable */
> > > +#define DB9000_CR1_LCE                       BIT(0)
> > > +/* LCD Power Enable */
> > > +#define DB9000_CR1_LPE                       BIT(1)
> > > +/* LCD Bits per Pixel */
> > > +#define DB9000_CR1_BPP(x)            (((x) & 0x7) << 2)
> > > +/* RGB or BGR Format */
> > > +#define DB9000_CR1_RGB                       BIT(5)
> > > +/* Data Enable Polarity */
> > > +#define DB9000_CR1_DEP                       BIT(8)
> > > +/* Pixel Clock Polarity */
> > > +#define DB9000_CR1_PCP                       BIT(9)
> > > +/* Horizontal Sync Polarity */
> > > +#define DB9000_CR1_HSP                       BIT(10)
> > > +/* Vertical Sync Polarity */
> > > +#define DB9000_CR1_VSP                       BIT(11)
> > > +/* Output Pixel Select */
> > > +#define DB9000_CR1_OPS(x)            (((x) & 0x7) << 12)
> > > +/* FIFO DMA Request Words */
> > > +#define DB9000_CR1_FDW(x)            (((x) & 0x3) << 16)
> > > +/* LCD 1 or Port Select */
> > > +#define DB9000_CR1_LPS                       BIT(18)
> > > +/* Frame Buffer 24bpp Packed Word */
> > > +#define DB9000_CR1_FBP                       BIT(19)
> > > +
> > > +enum DB9000_CR1_BPP {
> > > +     /* 1 bit per pixel */
> > > +     DB9000_CR1_BPP_1bpp,
> > > +     /* 2 bits per pixel */
> > > +     DB9000_CR1_BPP_2bpp,
> > > +     /* 4 bits per pixel */
> > > +     DB9000_CR1_BPP_4bpp,
> > > +     /* 8 bits per pixel */
> > > +     DB9000_CR1_BPP_8bpp,
> > > +     /* 16 bits per pixel */
> > > +     DB9000_CR1_BPP_16bpp,
> > > +     /* 18 bits per pixel */
> > > +     DB9000_CR1_BPP_18bpp,
> > > +     /* 24 bits per pixel */
> > > +     DB9000_CR1_BPP_24bpp
> > > +} DB9000_CR1_BPP_VAL;
> > > +
> > > +/* Horizontal Timing Register, Offset 0x008 */
> > > +/* Horizontal Front Porch */
> > > +#define DB9000_HTR_HFP(x)            (((x) & 0xff) << 0)
> > > +/* Pixels per Line */
> > > +#define DB9000_HTR_PPL(x)            (((x) & 0xff) << 8)
> > > +/* Horizontal Back Porch */
> > > +#define DB9000_HTR_HBP(x)            (((x) & 0xff) << 16)
> > > +/* Horizontal Sync Width */
> > > +#define DB9000_HTR_HSW(x)            (((x) & 0xff) << 24)
> > > +
> > > +/* Vertical Timing Register 1, Offset 0x00C */
> > > +/* Vertical Sync Width */
> > > +#define DB9000_VTR1_VSW(x)           (((x) & 0xff) << 0)
> > > +/* Vertical Front Porch */
> > > +#define DB9000_VTR1_VFP(x)           (((x) & 0xff) << 8)
> > > +/* Vertical Back Porch */
> > > +#define DB9000_VTR1_VBP(x)           (((x) & 0xff) << 16)
> > > +
> > > +/* Vertical Timing Register 2, Offset 0x010 */
> > > +/* Lines Per Panel */
> > > +#define DB9000_VTR2_LPP(x)           (((x) & 0xfff) << 0)
> > > +
> > > +/* Vertical and Horizontal Timing Extension Register, Offset 0x044
> > > +*/
> > > +/* Horizontal Front Porch Extension */
> > > +#define DB9000_HVTER_HFPE(x)         ((((x) >> 8) & 0x3) << 0)
> > > +/* Horizontal Back Porch Extension */
> > > +#define DB9000_HVTER_HBPE(x)         ((((x) >> 8) & 0x3) << 4)
> > > +/* Vertical Front Porch Extension */
> > > +#define DB9000_HVTER_VFPE(x)         ((((x) >> 8) & 0x3) << 8)
> > > +/* Vertical Back Porch Extension */
> > > +#define DB9000_HVTER_VBPE(x)         ((((x) >> 8) & 0x3) << 12)
> > > +
> > > +/* clock reset select */
> > > +#define DB9000_PCTR_PCR                      BIT(10)
> > > +
> > > +/* Interrupt Status Register, Offset 0x018 */
> > > +#define DB9000_ISR_OFU                       BIT(0) /* Output FIFO Underrun */
> > > +#define DB9000_ISR_OFO                       BIT(1) /* Output FIFO Overrun */
> > > +#define DB9000_ISR_IFU                       BIT(2) /* Input FIFO Underrun */
> > > +#define DB9000_ISR_IFO                       BIT(3) /* Input FIFO Overrun */
> > > +#define DB9000_ISR_FER                       BIT(4) /* OR of OFU, OFO, IFU, IFO
> */
> > > +#define DB9000_ISR_MBE                       BIT(5) /* Master Bus Error */
> > > +#define DB9000_ISR_VCT                       BIT(6) /* Vertical Compare
> Triggered */
> > > +#define DB9000_ISR_BAU                       BIT(7) /* DMA Base Address
> Register Update to CAR */
> > > +#define DB9000_ISR_LDD                       BIT(8) /* LCD Controller Disable
> Done */
> > > +
> > > +/* Interrupt Mask Register, Offset 0x01C */
> > > +#define DB9000_IMR_OFUM                      BIT(0) /* Output FIFO Underrun
> - Mask */
> > > +#define DB9000_IMR_OFOM                      BIT(1) /* Output FIFO Overrun -
> Mask */
> > > +#define DB9000_IMR_IFUM                      BIT(2) /* Input FIFO Underrun -
> Mask */
> > > +#define DB9000_IMR_IFOM                      BIT(3) /* Input FIFO Overrun -
> Mask */
> > > +#define DB9000_IMR_FERM                      BIT(4) /* OR of OFU, OFO, IFU,
> IFO - Mask */
> > > +#define DB9000_IMR_MBEM                      BIT(5) /* Master Bus Error -
> Mask */
> > > +#define DB9000_IMR_VCTM                      BIT(6) /* Vertical Compare
> Triggered - Mask */
> > > +/* DMA Base Address Register Update to CAR - Mask */
> > > +#define DB9000_IMR_BAUM                      BIT(7)
> > > +#define DB9000_IMR_LDDM                      BIT(8) /* LCD Controller Disable
> Done - Mask */
> > > +
> > > +/* PWM Frequency Register */
> > > +#define DB9000_PWMFR_0                       0x034
> > > +#define DB9000_PWMFR_RZN1            0x04C
> > > +/* PWM Duty Cycle Register */
> > > +#define DB9000_PWMDCR_0                      0x038
> > > +#define DB9000_PWMDCR_RZN1           0x050
> > > +/* PWM Frequency Registers, Offset 0x034 and 0x04c */
> > > +#define DB9000_PWMFR_PWMFCD(x)               (((x) & 0x3fffff) << 0)
> > > +#define DB9000_PWMFR_PWMFCE          BIT(22)
> > > +
> > > +struct fps_info {
> > > +     unsigned int counter;
> > > +     ktime_t last_timestamp;
> > > +};
> > > +
> > > +struct db9000_device {
> > > +     struct drm_device drm;
> > > +     struct pwm_chip pwm;
> > > +     struct drm_connector *connector;
> > > +     void __iomem *regs;
> > > +     spinlock_t lock;
> > > +     struct clk *lcd_eclk;
> > > +     struct fps_info plane_fpsi[DB9000_MAX_LAYER];
> > > +     struct drm_atomic_state *suspend_state;
> > > +     u8 bpp;
> > > +     int bus_width;
> > > +     size_t frame_size;
> > > +     u32 addr_pwmfr;
> > > +     u32 addr_pwmdcr;
> > > +};
> > > +
> > > +#endif /* __DB9000_DU_H__ */
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Gareth Williams
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux