Hi Thierry, Thank you very much for your patient and careful guidance. I'm updating my driver according to your comments. > -----Original Message----- > From: Thierry Reding [mailto:thierry.reding@xxxxxxxxx] > Sent: Tuesday, July 14, 2015 6:49 PM > To: Wang Jianwei-B52261 > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > airlied@xxxxxxxx; daniel.vetter@xxxxxxxx; mark.yao@xxxxxxxxxxxxxx; Wood > Scott-B07421; Wang Huan-B18965; Xiubo Li; Wang Jianwei-B52261 > Subject: Re: [PATCH v9 1/4] drm/layerscape: Add Freescale DCU DRM driver > > On Mon, Jul 13, 2015 at 06:51:29PM +0800, Jianwei Wang wrote: > > This patch add support for Two Dimensional Animation and Compositing > > Engine (2D-ACE) on the Freescale SoCs. > > > > 2D-ACE is a Freescale display controller. 2D-ACE describes the > > functionality of the module extremely well its name is a value that > > cannot be used as a token in programming languages. > > Instead the valid token "DCU" is used to tag the register names and > > function names. > > > > The Display Controller Unit (DCU) module is a system master that > > fetches graphics stored in internal or external memory and displays > > them on a TFT LCD panel. A wide range of panel sizes is supported and > > the timing of the interface signals is highly configurable. > > Graphics are read directly from memory and then blended in real-time, > > which allows for dynamic content creation with minimal CPU > > intervention. > > Is this for some non-i.MX SoC? Yes. Ls1021a and i.MX use different video IP modules. > > > The features: > > (1) Full RGB888 output to TFT LCD panel. > > (2) For the current LCD panel, WQVGA "480x272" is supported. > > Would be more useful to describe the actual capabilities of the display > controller rather than what's supported by the panel that you happened to > have attached to it. Ok, I'll remove it > > > (3) Blending of each pixel using up to 4 source layers dependent on > > size of panel. > > (4) Each graphic layer can be placed with one pixel resolution in > > either axis. > > (5) Each graphic layer support RGB565 and RGB888 direct colors without > > alpha channel and BGRA8888 BGRA4444 ARGB1555 direct colors with an > > alpha channel and YUV422 format. > > (6) Each graphic layer support alpha blending with 8-bit resolution. > > > > This is a simplified version, only one primary plane, one framebuffer, > > one crtc, one connector and one encoder for TFT LCD panel. > > > > Signed-off-by: Alison Wang <b18965@xxxxxxxxxxxxx> > > Signed-off-by: Xiubo Li <lixiubo@xxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Jianwei Wang <b52261@xxxxxxxxxxxxx> > > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Reviewed-by: Alison Wang <alison.wang@xxxxxxxxxxxxx> > [...] > > diff --git a/MAINTAINERS b/MAINTAINERS index 6761318..fffb8c9 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3404,6 +3404,15 @@ S: Maintained > > F: drivers/gpu/drm/imx/ > > F: Documentation/devicetree/bindings/drm/imx/ > > > > +DRM DRIVERS FOR FREESCALE DCU > > +M: Jianwei Wang <jianwei.wang@xxxxxxxxxxxxx> > > +M: Alison Wang <alison.wang@xxxxxxxxxxxxx> > > +L: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > +S: Supported > > +F: drivers/gpu/drm/fsl-dcu/ > > +F: Documentation/devicetree/bindings/drm/fsl-dcu/ > > +F: Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt > > This binding has got nothing to do with the display driver, so it > shouldn't be listed here. > > Also, please use tabs consistently (the above two lines use spaces). Sorry, I made this mistake when formatting patches. I'll be more careful next time. > > > DRM DRIVERS FOR NVIDIA TEGRA > > M: Thierry Reding <thierry.reding@xxxxxxxxx> > > M: Terje Bergström <tbergstrom@xxxxxxxxxx> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index > > c46ca31..9cfd14e 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -231,6 +231,8 @@ source "drivers/gpu/drm/virtio/Kconfig" > > > > source "drivers/gpu/drm/msm/Kconfig" > > > > +source "drivers/gpu/drm/fsl-dcu/Kconfig" > > + > > As mentioned in a different email, I'm somewhat annoyed by the random > placement of these source statements. But we can probably clean that up in > one go and insist on proper ordering when there is one. > Ok, I plan to move it to the last line. Is it ok? > > diff --git a/drivers/gpu/drm/fsl-dcu/Makefile > > b/drivers/gpu/drm/fsl-dcu/Makefile > > new file mode 100644 > > index 0000000..336b4a6 > > --- /dev/null > > +++ b/drivers/gpu/drm/fsl-dcu/Makefile > > @@ -0,0 +1,7 @@ > > +fsl-dcu-drm-y := fsl_dcu_drm_drv.o \ > > + fsl_dcu_drm_kms.o \ > > + fsl_dcu_drm_connector.o \ > > + fsl_dcu_drm_plane.o \ > > + fsl_dcu_drm_crtc.o \ > > + fsl_dcu_drm_fbdev.o > > I don't get what kind of indentation this is supposed to be. Either align > everything with the first object file, or use a single tab rather than a > tab followed by a couple of spaces. > OK! > > +obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu-drm.o > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_connector.c > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_connector.c > > new file mode 100644 > > index 0000000..41dd1d0 > > --- /dev/null > > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_connector.c > > @@ -0,0 +1,187 @@ > > +/* > > + * Copyright 2015 Freescale Semiconductor, Inc. > > + * > > + * Freescale DCU drm device driver > > + * > > + * This program is free software; you can redistribute it and/or > > +modify > > + * it under the terms of the GNU General Public License as published > > +by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#include <linux/backlight.h> > > + > > +#include <drm/drmP.h> > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_panel.h> > > + > > +#include "fsl_dcu_drm_drv.h" > > +#include "fsl_dcu_drm_connector.h" > > + > > +static void fsl_dcu_drm_encoder_disable(struct drm_encoder *encoder) > > +{ } > > + > > +static int > > +fsl_dcu_drm_encoder_atomic_check(struct drm_encoder *encoder, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) { > > + return 0; > > +} > > + > > +static void fsl_dcu_drm_encoder_destroy(struct drm_encoder *encoder) > > +{ > > + drm_encoder_cleanup(encoder); > > +} > > + > > +static void fsl_dcu_drm_encoder_enable(struct drm_encoder *encoder) { > > +} > > + > > +static const struct drm_encoder_helper_funcs encoder_helper_funcs = { > > + .enable = fsl_dcu_drm_encoder_enable, > > + .disable = fsl_dcu_drm_encoder_disable, > > + .atomic_check = fsl_dcu_drm_encoder_atomic_check, }; > > + > > +static const struct drm_encoder_funcs encoder_funcs = { > > + .destroy = fsl_dcu_drm_encoder_destroy, }; > > For ease of maintenance and review, you might want to reorder your > function definitions to match the structure layout and put them closer > together. > > The same applies to other places in this file. > OK! > > +int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev, > > + struct drm_crtc *crtc) > > +{ > > + struct drm_encoder *encoder = &fsl_dev->encoder; > > + int ret; > > + > > + encoder->possible_crtcs = 1; > > + ret = drm_encoder_init(fsl_dev->ddev, encoder, &encoder_funcs, > > + DRM_MODE_ENCODER_LVDS); > > + if (ret < 0) > > + return ret; > > + > > + drm_encoder_helper_add(encoder, &encoder_helper_funcs); > > + encoder->crtc = crtc; > > I think you're supposed to let the DRM core make this association. > I'll remove it > > + > > + return 0; > > +} > > + > > +#define to_fsl_dcu_connector(connector) \ > > + container_of(connector, struct fsl_dcu_drm_connector, connector) > > + > > It's common to put this near the definition of the structure that the > upcast is for. It's also preferred (though not by everyone it seems) to > make this a static inline function rather than a #define. > Ok! > > +static int fsl_dcu_drm_connector_get_modes(struct drm_connector > > +*connector) { > > + struct fsl_dcu_drm_connector *fsl_connector; > > + int num_modes = 0; > > + > > + fsl_connector = to_fsl_dcu_connector(connector); > > + if (fsl_connector->panel && fsl_connector->panel->funcs && > > + fsl_connector->panel->funcs->get_modes) > > + num_modes = fsl_connector->panel->funcs->get_modes > > + (fsl_connector->panel); > > You could perhaps use some temporary variables to make this more readable. > Ok! > > +int fsl_dcu_drm_connector_create(struct fsl_dcu_drm_device *fsl_dev, > > + struct drm_encoder *encoder) > > +{ > > + struct drm_connector *connector = &fsl_dev->connector.connector; > > + struct device_node *panel_node; > > + int ret; > > + > > + fsl_dev->connector.encoder = encoder; > > Why do you need this? The DRM core should set this for you when setting > the initial configuration. > This connector is a private structure fsl_dcu_drm_connector. I set it for select best encoder. > > + > > + connector->display_info.width_mm = 0; > > + connector->display_info.height_mm = 0; > > Why do you need to zero these out? I'll remove it > > > + ret = drm_connector_init(fsl_dev->ddev, connector, > > + &fsl_dcu_drm_connector_funcs, > > + DRM_MODE_CONNECTOR_LVDS); > > + if (ret < 0) > > + return ret; > > + > > + connector->dpms = DRM_MODE_DPMS_OFF; > > This looks like it really belongs in the ->reset() callback. OK! I'll adjust it > > > + drm_connector_helper_add(connector, &connector_helper_funcs); > > + ret = drm_connector_register(connector); > > + if (ret < 0) > > + goto err_cleanup; > > + > > + ret = drm_mode_connector_attach_encoder(connector, encoder); > > + if (ret < 0) > > + goto err_sysfs; > > + > > + connector->encoder = encoder; > > You did this before already, why repeat it? Why even do it in the first > place? Does this somehow not work if you omit this explicit assignment? > I'll remove it > > + drm_object_property_set_value > > + (&connector->base, fsl_dev->ddev->mode_config.dpms_property, > > + DRM_MODE_DPMS_OFF); > > This is badly aligned. "&connector->base," should go on the first line and > the second line (all subsequent ones, really) should align with it. > Ok! > > + > > + panel_node = of_parse_phandle(fsl_dev->np, "panel", 0); > > This isn't a standard property, so technically you'd need to prefix it > with a vendor prefix. > Ok! > > + if (panel_node) { > > + fsl_dev->connector.panel = of_drm_find_panel(panel_node); > > + if (!fsl_dev->connector.panel) { > > + of_node_put(panel_node); > > + ret = -EPROBE_DEFER; > > + goto err_sysfs; > > + } > > + } > > + of_node_put(panel_node); > > You're leaking the OF node reference on -EPROBE_DEFER. > Got it! > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > [...] > > +#include <linux/regmap.h> > > +#include <linux/clk.h> > > You should keep these sorted alphabetically, that'll save you headaches > later on. > Ok! > > +static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) { > > + struct drm_device *dev = crtc->dev; > > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > + struct drm_display_mode *mode = &crtc->state->mode; > > + uint32_t hbp, hfp, hsw, vbp, vfp, vsw, div, index; > > + int err; > > + > > + DBG(": set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x", > > + mode->base.id, mode->name, > > + mode->vrefresh, mode->clock, > > + mode->hdisplay, mode->hsync_start, > > + mode->hsync_end, mode->htotal, > > + mode->vdisplay, mode->vsync_start, > > + mode->vsync_end, mode->vtotal, > > + mode->type, mode->flags); > > The KMS helpers already output this line, don't they? Yes, I'll remove it > > > + index = drm_crtc_index(crtc); > > + div = (uint32_t)clk_get_rate(fsl_dev->clk) / mode->clock / 1000; > > You should probably not cast the clk_get_rate() return value. You risk > running into problems if you ever need to support rates higher than 4 GHz. > I'll grant you that it's unlikely to happen any time soon, but it > shouldn't be a reason not to write future-proof code. > Got it > > + > > + /* Configure timings: */ > > + hbp = mode->htotal - mode->hsync_end; > > + hfp = mode->hsync_start - mode->hdisplay; > > + hsw = mode->hsync_end - mode->hsync_start; > > + vbp = mode->vtotal - mode->vsync_end; > > + vfp = mode->vsync_start - mode->vdisplay; > > + vsw = mode->vsync_end - mode->vsync_start; > > + > > + err = regmap_write(fsl_dev->regmap, DCU_HSYN_PARA, > > + DCU_HSYN_PARA_BP(hbp) | > > + DCU_HSYN_PARA_PW(hsw) | > > + DCU_HSYN_PARA_FP(hfp)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_VSYN_PARA, > > + DCU_VSYN_PARA_BP(vbp) | > > + DCU_VSYN_PARA_PW(vsw) | > > + DCU_VSYN_PARA_FP(vfp)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_DISP_SIZE, > > + DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) | > > + DCU_DISP_SIZE_DELTA_X(mode->hdisplay)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE, > > + DCU_UPDATE_MODE_READREG); > > + if (err) > > + goto set_failed; > > + return; > > +set_failed: > > + dev_err(dev->dev, "set DCU register failed\n"); } > [...] > > +static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc) { } > > + > > +static int fsl_dcu_drm_crtc_atomic_check(struct drm_crtc *crtc, > > + struct drm_crtc_state *state) > > +{ > > + return 0; > > +} > > + > > +static void fsl_dcu_drm_crtc_atomic_begin(struct drm_crtc *crtc) { } > > + > > +static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc) { } > > + > > +static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) { } > > Really? Then how does the CRTC get enabled or disabled, I wonder? > I'll implement crtc enable and disable function. > > +int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) { > > + struct drm_plane *primary; > > + struct drm_crtc *crtc = &fsl_dev->crtc; > > + int i, ret; > > i could be unsigned int. > Ok! > > + > > + primary = fsl_dcu_drm_primary_create_plane(fsl_dev->ddev); > > + ret = drm_crtc_init_with_planes(fsl_dev->ddev, crtc, primary, NULL, > > + &fsl_dcu_drm_crtc_funcs); > > + if (ret < 0) > > + return ret; > > + > > + drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs); > > + > > + for (i = 0; i < DCU_TOTAL_LAYER_NUM; i++) { > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_1(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_2(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_3(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_4(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_5(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_6(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_7(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_8(i), 0); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_9(i), 0); > > + if (ret) > > + goto init_failed; > > This is highly repetitive, perhaps make a DCU_CTRLDESCLN(x, y) instead and > use nested loops here? > Good idea, I'll do it > > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) { > > + ret = regmap_write(fsl_dev->regmap, > > + DCU_CTRLDESCLN_10(i), 0); > > + if (ret) > > + goto init_failed; > > + } > > + } > > Might be better to introduce some sort of per-SoC capability structure to > parameterize, so that you can avoid this sort of check on the OF node's > compatible string. You typically do that by setting the OF match table's > entries' .data field to the instance of the structure for the particular > SoC or hardware block. > I'm adjusting my driver. Much will be move to platform related file. Thanks for your guidance. > > + ret = regmap_write(fsl_dev->regmap, DCU_SYN_POL, > > + DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW); > > + if (ret) > > + goto init_failed; > > This looks like it should be part of the ->mode_set_nofb() implementation > and depend on parameters of the mode. > Ok, I'll move them(including of some registers below) to the right place. > > + ret = regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) | > > + DCU_BGND_G(0) | DCU_BGND_B(0)); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_DCU_MODE, > > + DCU_MODE_BLEND_ITER(1) | DCU_MODE_RASTER_EN); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_THRESHOLD, > > + DCU_THRESHOLD_LS_BF_VS(BF_VS_VAL) | > > + DCU_THRESHOLD_OUT_BUF_HIGH(BUF_MAX_VAL) | > > + DCU_THRESHOLD_OUT_BUF_LOW(BUF_MIN_VAL)); > > + if (ret) > > + goto init_failed; > > + ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > > + DCU_MODE_DCU_MODE_MASK, > > + DCU_MODE_DCU_MODE(DCU_MODE_OFF)); > > + if (ret) > > + goto init_failed; > > + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE, > > + DCU_UPDATE_MODE_READREG); > > + if (ret) > > + goto init_failed; > > + > > + return 0; > > +init_failed: > > + dev_err(fsl_dev->dev, "init DCU register failed\n"); > > + return ret; > > +} > [...] > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > [...] > > +static int fsl_dcu_unload(struct drm_device *dev) { > > + drm_mode_config_cleanup(dev); > > + drm_vblank_cleanup(dev); > > + drm_irq_uninstall(dev); > > + > > + dev->dev_private = NULL; > > + > > + return 0; > > +} > > + > > +static struct regmap_config fsl_dcu_regmap_config = { > > static const > Got it > > + .reg_bits = 32, > > + .reg_stride = 4, > > + .val_bits = 32, > > +}; > > + > > +static int fsl_dcu_bypass_tcon(struct fsl_dcu_drm_device *fsl_dev, > > + struct device_node *np) > > +{ > > + struct device_node *tcon_np; > > + struct platform_device *pdev; > > + struct clk *tcon_clk; > > + struct resource *res; > > + void __iomem *base; > > + int ret; > > + > > + tcon_np = of_parse_phandle(np, "tcon-controller", 0); > > + if (!tcon_np) > > + return -EINVAL; > > + > > + pdev = of_find_device_by_node(tcon_np); > > + if (!pdev) > > + return -EINVAL; > > + > > + tcon_clk = devm_clk_get(&pdev->dev, "tcon"); > > + if (IS_ERR(tcon_clk)) > > + return PTR_ERR(tcon_clk); > > + ret = clk_prepare(tcon_clk); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to prepare tcon clk\n"); > > + return ret; > > + } > > + ret = clk_enable(tcon_clk); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to enable tcon clk\n"); > > + clk_unprepare(tcon_clk); > > + return ret; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -ENODEV; > > + > > + base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + fsl_dev->tcon_regmap = devm_regmap_init_mmio(&pdev->dev, > > + base, &fsl_dcu_regmap_config); > > + if (IS_ERR(fsl_dev->tcon_regmap)) { > > + dev_err(&pdev->dev, "regmap init failed\n"); > > + return PTR_ERR(fsl_dev->tcon_regmap); > > + } > > + > > + ret = regmap_write(fsl_dev->tcon_regmap, > > + TCON_CTRL1, TCON_BYPASS_ENABLE); > > + if (ret) { > > + dev_err(&pdev->dev, "set TCON_CTRL1 failed\n"); > > + return ret; > > + } > > + return 0; > > +} > > Erm... no. You should have a proper TCON driver to take care of this. If > it's only used by the display driver, the TCON driver could be part of the > DRM driver. > > But as it is, you're side-stepping the driver model and are leaving > resource crumbs all over the place that noone will get a chance to clean > up. > Ok, I'll move it out > > + > > +static void dcu_pixclk_enable(void) > > +{ > > + struct regmap *scfg_regmap; > > + > > + scfg_regmap = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg"); > > + if (IS_ERR(scfg_regmap)) { > > + pr_err("No syscfg phandle specified\n"); > > + return; > > + } > > + > > + regmap_write(scfg_regmap, SCFG_PIXCLKCR, PXCK_ENABLE); } > > This should be a clock driver. Chances are that you're going to need to > change the pixel clock frequency at some point, in which case you're going > to need a proper pixel clock anyway. > > That said, you already have a "dcu" clock that you use. According to the > modeset code it's some kind of parent of the pixel clock (you derive a > divider based on the "dcu" clock frequency and the mode clock). You should > really model this properly as a clock tree. > Ok! > > +static int fsl_dcu_drm_irq_init(struct drm_device *dev) { > > + struct platform_device *pdev = to_platform_device(dev->dev); > > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > + unsigned int int_mask; > > + int ret; > > + > > + ret = drm_irq_install(dev, platform_get_irq(pdev, 0)); > > + if (ret < 0) > > + dev_err(&pdev->dev, "failed to install IRQ handler\n"); > > If you think you'll ever need to support multiple interrupts, I'd suggest > you don't use drm_irq_install() but rather set up the interrupt yourself. > It's not difficult to do and will give you some more flexibility in turn. > > > + dev->irq_enabled = true; > > + dev->vblank_disable_allowed = true; > > + > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0); > > + if (ret) > > + dev_err(&pdev->dev, "set DCU_INT_STATUS failed\n"); > > + ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &int_mask); > > + if (ret) > > + dev_err(&pdev->dev, "read DCU_INT_MASK failed\n"); > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, int_mask & > > + ~DCU_INT_MASK_VBLANK); > > + if (ret) > > + dev_err(&pdev->dev, "set DCU_INT_MASK failed\n"); > > + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE, > > + DCU_UPDATE_MODE_READREG); > > What's this DCU_UPDATE_MODE_READREG bit? > In fact, Only when setting DCU_UPDATE_MODE_READREG bit, it begin to transfer register values. So setting DCU_UPDATE_MODE_READREG bit is a must after registers updating. > > +static int fsl_dcu_load(struct drm_device *ddev, unsigned long flags) > > +{ > > + struct device *dev = ddev->dev; > > + struct platform_device *pdev = to_platform_device(dev); > > + struct fsl_dcu_drm_device *fsl_dev; > > + struct resource *res; > > + void __iomem *base; > > + int ret; > > + > > + fsl_dev = devm_kzalloc(dev, sizeof(*fsl_dev), GFP_KERNEL); > > + if (!fsl_dev) > > + return -ENOMEM; > > + > > + fsl_dev->dev = dev; > > + fsl_dev->ddev = ddev; > > + fsl_dev->np = dev->of_node; > > + ddev->dev_private = fsl_dev; > > + dev_set_drvdata(dev, fsl_dev); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) { > > + dev_err(dev, "could not get memory IO resource\n"); > > + return -ENODEV; > > + } > > + > > + base = devm_ioremap_resource(dev, res); > > + if (IS_ERR(base)) { > > + ret = PTR_ERR(base); > > + return ret; > > + } > > + > > + fsl_dev->clk = devm_clk_get(dev, "dcu"); > > + if (IS_ERR(fsl_dev->clk)) { > > + ret = PTR_ERR(fsl_dev->clk); > > + dev_err(dev, "failed to get dcu clock\n"); > > + return ret; > > + } > > + ret = clk_prepare(fsl_dev->clk); > > + if (ret < 0) { > > + dev_err(dev, "failed to prepare dcu clk\n"); > > + return ret; > > + } > > + ret = clk_enable(fsl_dev->clk); > > + if (ret < 0) { > > + dev_err(dev, "failed to enable dcu clk\n"); > > + clk_unprepare(fsl_dev->clk); > > + return ret; > > + } > > + > > + fsl_dev->regmap = devm_regmap_init_mmio(dev, base, > > + &fsl_dcu_regmap_config); > > + if (IS_ERR(fsl_dev->regmap)) { > > + dev_err(dev, "regmap init failed\n"); > > + return PTR_ERR(fsl_dev->regmap); > > + } > > + > > + /* Put TCON in bypass mode, so the input signals from DCU are passed > > + * through TCON unchanged */ > > + fsl_dcu_bypass_tcon(fsl_dev, fsl_dev->np); > > + > > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) > > + dcu_pixclk_enable(); > > All of the above really belongs in ->probe(). If you do that you don't > need to involve anything of DRM until after all resources have been > successfully claimed. Also it allows you to get rid of the ugly upcast > from struct device to struct platform_device, because ->probe() will > directly give you a platform device. > Got it > > + ret = fsl_dcu_drm_modeset_init(fsl_dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to initialize mode setting\n"); > > + return ret; > > + } > > + > > + ret = drm_vblank_init(ddev, ddev->mode_config.num_crtc); > > + if (ret < 0) { > > + dev_err(dev, "failed to initialize vblank\n"); > > + goto done; > > + } > > + > > + ret = fsl_dcu_drm_irq_init(ddev); > > + if (ret < 0) > > + goto done; > > + > > + fsl_dcu_fbdev_init(ddev); > > + > > + return 0; > > +done: > > + if (ret) > > + fsl_dcu_unload(ddev); > > + > > + return ret; > > +} > > + > > +static void fsl_dcu_drm_preclose(struct drm_device *dev, struct > > +drm_file *file) { } > > + > > +static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg) { > > + struct drm_device *dev = arg; > > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > + unsigned int int_status; > > + int ret; > > + > > + regmap_read(fsl_dev->regmap, DCU_INT_STATUS, &int_status); > > No error checking here? > Ok I'll do it > > + if (int_status & DCU_INT_STATUS_VBLANK) > > + drm_handle_vblank(dev, 0); > > + > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0xffffffff); > > + if (ret) > > + dev_err(dev->dev, "set DCU_INT_STATUS failed\n"); > > + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE, > > + DCU_UPDATE_MODE_READREG); > > + if (ret) > > + dev_err(dev->dev, "set DCU_UPDATE_MODE failed\n"); > > Here it is again. Is this some sort of manual trigger for screen updates? > > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int fsl_dcu_drm_enable_vblank(struct drm_device *dev, int > > +crtc) { > > + return 0; > > +} > > + > > +static void fsl_dcu_drm_disable_vblank(struct drm_device *dev, int > > +crtc) { } > > You do have a way to enable and disable VBLANK, why don't you use it? > Ok, I'll implement vblank enable and disable functions. > > +static struct drm_driver fsl_dcu_drm_driver = { > > + .driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET > > + | DRIVER_PRIME | DRIVER_ATOMIC, > > + .load = fsl_dcu_load, > > + .unload = fsl_dcu_unload, > > + .preclose = fsl_dcu_drm_preclose, > > + .irq_handler = fsl_dcu_drm_irq, > > + .get_vblank_counter = drm_vblank_count, > > + .enable_vblank = fsl_dcu_drm_enable_vblank, > > + .disable_vblank = fsl_dcu_drm_disable_vblank, > > + .gem_free_object = drm_gem_cma_free_object, > > + .gem_vm_ops = &drm_gem_cma_vm_ops, > > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > + .gem_prime_import = drm_gem_prime_import, > > + .gem_prime_export = drm_gem_prime_export, > > + .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, > > + .dumb_create = drm_gem_cma_dumb_create, > > + .dumb_map_offset = drm_gem_cma_dumb_map_offset, > > + .dumb_destroy = drm_gem_dumb_destroy, > > + .fops = &fsl_dcu_drm_fops, > > + .name = "fsl-dcu-drm", > > + .desc = "Freescale DCU DRM", > > + .date = "20150213", > > + .major = 1, > > + .minor = 0, > > +}; > > + > > +#ifdef CONFIG_PM_SLEEP > > +static void dcu_pixclk_disable(void) > > +{ > > + struct regmap *scfg_regmap; > > + > > + scfg_regmap = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg"); > > + if (IS_ERR(scfg_regmap)) { > > + pr_err("No syscfg phandle specified\n"); > > + return; > > + } > > + > > + regmap_write(scfg_regmap, SCFG_PIXCLKCR, PXCK_DISABLE); } > > Again, if this was modelled as a clock, you could hide all this > integration glue from the display driver. > Ok! > > +static int fsl_dcu_drm_pm_suspend(struct device *dev) { > > + struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev); > > + > > + if (!fsl_dev) > > + return 0; > > + > > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) > > + dcu_pixclk_disable(); > > + > > + drm_kms_helper_poll_disable(fsl_dev->ddev); > > + regcache_cache_only(fsl_dev->regmap, true); > > + regcache_mark_dirty(fsl_dev->regmap); > > + clk_disable(fsl_dev->clk); > > Why not unprepare the clock at the same time? > > > + > > + if (fsl_dev->tcon_regmap) { > > + regcache_cache_only(fsl_dev->tcon_regmap, true); > > + regcache_mark_dirty(fsl_dev->tcon_regmap); > > + clk_disable(fsl_dev->tcon_clk); > > Same here. > Ok, I'll do it > > + } > > + > > + return 0; > > +} > > + > > +static int fsl_dcu_drm_pm_resume(struct device *dev) { > > + struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev); > > + int ret; > > + > > + if (!fsl_dev) > > + return 0; > > + > > + /* Enable clocks and restore all registers */ > > + if (fsl_dev->tcon_regmap) { > > + ret = clk_enable(fsl_dev->tcon_clk); > > + if (ret < 0) { > > + dev_err(dev, "failed to enable tcon clk\n"); > > + clk_unprepare(fsl_dev->tcon_clk); > > + return ret; > > + } > > + regcache_cache_only(fsl_dev->tcon_regmap, false); > > + regcache_sync(fsl_dev->tcon_regmap); > > + } > > + > > + ret = clk_enable(fsl_dev->clk); > > + if (ret < 0) { > > + dev_err(dev, "failed to enable dcu clk\n"); > > + clk_unprepare(fsl_dev->clk); > > + return ret; > > + } > > + > > + drm_kms_helper_poll_enable(fsl_dev->ddev); > > + regcache_cache_only(fsl_dev->regmap, false); > > + regcache_sync(fsl_dev->regmap); > > + > > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) > > + dcu_pixclk_enable(); > > + > > + return 0; > > +} > > +#endif > > + > > +static const struct dev_pm_ops fsl_dcu_drm_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(fsl_dcu_drm_pm_suspend, > > +fsl_dcu_drm_pm_resume) }; > > + > > +static int fsl_dcu_drm_probe(struct platform_device *pdev) { > > + struct drm_driver *driver = &fsl_dcu_drm_driver; > > + struct drm_device *drm; > > + int err; > > + > > + drm = drm_dev_alloc(driver, &pdev->dev); > > + if (!drm) > > + return -ENOMEM; > > + > > + drm_dev_set_unique(drm, dev_name(&pdev->dev)); > > + > > + err = drm_dev_register(drm, 0); > > + if (err < 0) > > + goto unref; > > + > > + DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n", driver->name, > > + driver->major, driver->minor, driver->patchlevel, > > + driver->date, drm->primary->index); > > + > > + return 0; > > + > > +unref: > > + drm_dev_unref(drm); > > + return err; > > +} > > + > > +static int fsl_dcu_drm_remove(struct platform_device *pdev) { > > + struct fsl_dcu_drm_device *fsl_dev = platform_get_drvdata(pdev); > > + > > + drm_put_dev(fsl_dev->ddev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id fsl_dcu_of_match[] = { > > + { .compatible = "fsl,ls1021a-dcu", }, > > + { .compatible = "fsl,vf610-dcu", }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, fsl_dcu_of_match); > > + > > +static struct platform_driver fsl_dcu_drm_platform_driver = { > > + .probe = fsl_dcu_drm_probe, > > + .remove = fsl_dcu_drm_remove, > > + .driver = { > > + .name = "fsl,dcu", > > I don't think it's typical to use "vendor-prefix," in platform driver > names. Perhaps a more typical name would be "fsl-dcu". > Ok > > + .pm = &fsl_dcu_drm_pm_ops, > > + .of_match_table = fsl_dcu_of_match, > > + }, > > +}; > > + > > +module_platform_driver(fsl_dcu_drm_platform_driver); > > + > > +MODULE_DESCRIPTION("Freescale DCU DRM Driver"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > [...] > > +#define DCU_DISP_SIZE 0x0018 > > +#define DCU_DISP_SIZE_DELTA_Y(x) ((x) << 16) > > +/*Regisiter value 1/16 of horizontal resolution*/ > > +#define DCU_DISP_SIZE_DELTA_X(x) ((x) >> 4) > > Does this mean the display controller only supports horizontal resolutions > that are a multiple of 16? > Yes. > > +#ifdef CONFIG_SOC_VF610 > > +#define DCU_TOTAL_LAYER_NUM 64 > > +#define DCU_LAYER_NUM_MAX 6 > > +#else > > +#define DCU_TOTAL_LAYER_NUM 16 > > +#define DCU_LAYER_NUM_MAX 4 > > +#endif > > Should this not be a runtime decision so that the same driver can run on > VF610 and LS1021A platforms? > Yes, Both VF610 and LS1021A use DCU as video IP module. And there are a bit of differences on each SoCs. > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_fbdev.c > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_fbdev.c > [...] > > new file mode 100644 > > index 0000000..f8ef0e1 > > --- /dev/null > > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_fbdev.c > > @@ -0,0 +1,26 @@ > > +/* > > + * Copyright 2015 Freescale Semiconductor, Inc. > > + * > > + * This program is free software; you can redistribute it and/or > > +modify it > > + * under the terms of the GNU General Public License version 2 as > > +published by > > + * the Free Software Foundation. > > The rest of your driver is GPL v2 (or later), this file isn't. Pick one. > Ok > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > [...] > > +#include <drm/drmP.h> > > +#include <drm/drm_crtc.h> > > +#include <drm/drm_crtc_helper.h> > > +#include <drm/drm_fb_cma_helper.h> > > +#include <drm/drm_gem_cma_helper.h> > > +#include <linux/regmap.h> > > +#include <drm/drm_plane_helper.h> > > +#include <drm/drm_atomic_helper.h> > > + > > +#include "fsl_dcu_drm_drv.h" > > +#include "fsl_dcu_drm_kms.h" > > +#include "fsl_dcu_drm_plane.h" > > + > > +#define to_fsl_dcu_plane(plane) \ > > + container_of(plane, struct fsl_dcu_drm_plane, plane) > > + > > +static int > > +fsl_dcu_drm_plane_prepare_fb(struct drm_plane *plane, > > + struct drm_framebuffer *fb, > > + const struct drm_plane_state *new_state) { > > + return 0; > > +} > > + > > +static void > > +fsl_dcu_drm_plane_cleanup_fb(struct drm_plane *plane, > > + struct drm_framebuffer *fb, > > + const struct drm_plane_state *new_state) { } > > + > > +static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane, > > + struct drm_plane_state *state) { > > + return 0; > > +} > > + > > +static void fsl_dcu_drm_plane_atomic_disable(struct drm_plane *plane, > > + struct drm_plane_state *old_state) { } > > This really should disable the plane. Perhaps clear DCU_CTRLDESCLN_4_EN > here? > Yes, I'll do it > > +void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane, > > + struct drm_plane_state *old_state) { > > + struct fsl_dcu_drm_device *fsl_dev = plane->dev->dev_private; > > + struct drm_plane_state *state = plane->state; > > + struct drm_framebuffer *fb = plane->state->fb; > > + struct drm_gem_cma_object *gem; > > + struct fsl_dcu_drm_plane *fsl_plane = to_fsl_dcu_plane(plane); > > + u32 index, alpha, bpp; > > + int err; > > + > > + if (!fb) > > + return; > > + > > + index = fsl_plane->index; > > + gem = drm_fb_cma_get_gem_obj(fb, 0); > > + > > + switch (fb->pixel_format) { > > + case DRM_FORMAT_RGB565: > > + bpp = FSL_DCU_RGB565; > > + alpha = 0xff; > > + break; > > + case DRM_FORMAT_RGB888: > > + bpp = FSL_DCU_RGB888; > > + alpha = 0xff; > > + break; > > + case DRM_FORMAT_ARGB8888: > > + bpp = FSL_DCU_ARGB8888; > > + alpha = 0xff; > > + break; > > + case DRM_FORMAT_BGRA4444: > > + bpp = FSL_DCU_ARGB4444; > > + alpha = 0xff; > > + break; > > + case DRM_FORMAT_ARGB1555: > > + bpp = FSL_DCU_ARGB1555; > > + alpha = 0xff; > > + break; > > + case DRM_FORMAT_YUV422: > > + bpp = FSL_DCU_YUV422; > > + alpha = 0xff; > > + break; > > + default: > > + return; > > + } > > You should do this in ->atomic_check() already so that you can guarantee > that the selected format can be set. The DRM core should already check it > for you, but silently aborting here still doesn't seem like the right > solution. > Ok, I'll add format check in atomic_check(). Here is transfer pixel_format to value wrote to register. > > + > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_1(index), > > + DCU_CTRLDESCLN_1_HEIGHT(state->crtc_h) | > > + DCU_CTRLDESCLN_1_WIDTH(state->crtc_w)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_2(index), > > + DCU_CTRLDESCLN_2_POSY(state->crtc_y) | > > + DCU_CTRLDESCLN_2_POSX(state->crtc_x)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, > > + DCU_CTRLDESCLN_3(index), gem->paddr); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_4(index), > > + DCU_CTRLDESCLN_4_EN | > > + DCU_CTRLDESCLN_4_TRANS(alpha) | > > + DCU_CTRLDESCLN_4_BPP(bpp) | > > + DCU_CTRLDESCLN_4_AB(0)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_5(index), > > + DCU_CTRLDESCLN_5_CKMAX_R(0xFF) | > > + DCU_CTRLDESCLN_5_CKMAX_G(0xFF) | > > + DCU_CTRLDESCLN_5_CKMAX_B(0xFF)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_6(index), > > + DCU_CTRLDESCLN_6_CKMIN_R(0) | > > + DCU_CTRLDESCLN_6_CKMIN_G(0) | > > + DCU_CTRLDESCLN_6_CKMIN_B(0)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_7(index), 0); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_8(index), > > + DCU_CTRLDESCLN_8_FG_FCOLOR(0)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_9(index), > > + DCU_CTRLDESCLN_9_BG_BCOLOR(0)); > > + if (err) > > + goto set_failed; > > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) { > > + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_10(index), > > + DCU_CTRLDESCLN_10_POST_SKIP(0) | > > + DCU_CTRLDESCLN_10_PRE_SKIP(0)); > > + if (err) > > + goto set_failed; > > + } > > + err = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > > + DCU_MODE_DCU_MODE_MASK, > > + DCU_MODE_DCU_MODE(DCU_MODE_NORMAL)); > > + if (err) > > + goto set_failed; > > + err = regmap_write(fsl_dev->regmap, > > + DCU_UPDATE_MODE, DCU_UPDATE_MODE_READREG); > > + if (err) > > + goto set_failed; > > + return; > > + > > +set_failed: > > + dev_err(fsl_dev->dev, "set DCU register failed\n"); } > > + > > +int fsl_dcu_drm_plane_disable(struct drm_plane *plane) { > > + return 0; > > +} > > + > > +void fsl_dcu_drm_plane_destroy(struct drm_plane *plane) { > > + fsl_dcu_drm_plane_disable(plane); > > Hmm? This function doesn't do anything, so why not just drop it? > I'll implement fsl_dcu_drm_plane_disable(plane) > > + drm_plane_cleanup(plane); > > +} > > Also this function and ->atomic_update() should be static. Perhaps make it > a habit of running your build tests with C=1 occasionally to get notified > of this kind of error. > > Thierry One question: How can I set C=1? Thanks again. Jianwei _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
