Hi Sascha,
Thanks for the review.
On Fri, Jul 20, 2012 at 03:04:22PM +0200, Laurent Pinchart wrote:
The SH Mobile LCD controller (LCDC) DRM driver supports the main graphics plane in RGB and YUV formats, as well as the overlay planes (in alpha-blending mode only).
Only flat panel outputs using the parallel interface are supported. Support for SYS panels, HDMI and DSI is currently not implemented.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
[snip]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c new file mode 100644 index 0000000..c7be5f7 --- /dev/null +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c @@ -0,0 +1,768 @@
[...]
+static void shmob_drm_encoder_destroy(struct drm_encoder *encoder) +{
- drm_encoder_cleanup(encoder);
+}
+static const struct drm_encoder_funcs encoder_funcs = {
- .destroy = shmob_drm_encoder_destroy,
You could add drm_encoder_cleanup here.
Indeed, will fix.
[...]
+static enum drm_connector_status +shmob_drm_connector_detect(struct drm_connector *connector, bool force) +{
- return connector_status_unknown;
+}
Shouldn't you return connector_status_connected here? I mean all that you handle is a dumb display which is always connected.
OK.
[...]
+static int __devinit shmob_drm_setup_clocks(struct shmob_drm_device *sdev,
enum shmob_drm_clk_source clksrc)+{
- struct clk *clk;
- char *clkname;
- switch (clksrc) {
- case SHMOB_DRM_CLK_BUS:
clkname = "bus_clk";sdev->lddckr = LDDCKR_ICKSEL_BUS;break;- case SHMOB_DRM_CLK_PERIPHERAL:
clkname = "peripheral_clk";sdev->lddckr = LDDCKR_ICKSEL_MIPI;break;- case SHMOB_DRM_CLK_EXTERNAL:
clkname = NULL;sdev->lddckr = LDDCKR_ICKSEL_HDMI;break;- default:
return -EINVAL;- }
- clk = clk_get(sdev->dev, clkname);
- if (IS_ERR(clk)) {
dev_err(sdev->dev, "cannot get dot clock %s\n", clkname);return PTR_ERR(clk);- }
- sdev->clock = clk;
- return 0;
+}
+/*
---- + * DRM operations
- */
+static int shmob_drm_unload(struct drm_device *dev) +{
- struct shmob_drm_device *sdev = dev->dev_private;
- drm_kms_helper_poll_fini(dev);
- drm_vblank_cleanup(dev);
- drm_irq_uninstall(dev);
- if (sdev->clock)
clk_put(sdev->clock);When the clk_get above failed sdev->clock may be a error pointer here which you clk_put.
If clk_get() returns an error pointer sdev->clock is not assigned:
clk = clk_get(sdev->dev, clkname); if (IS_ERR(clk)) { dev_err(sdev->dev, "cannot get dot clock %s\n", clkname); return PTR_ERR(clk); }
sdev->clock = clk;
so sdev->clock should stay NULL in that case.
- if (sdev->mmio)
iounmap(sdev->mmio);- dev->dev_private = NULL;
- kfree(sdev);
- return 0;
+}
+static int shmob_drm_load(struct drm_device *dev, unsigned long flags) +{
- struct shmob_drm_platform_data *pdata = dev->dev->platform_data;
- struct platform_device *pdev = dev->platformdev;
- struct shmob_drm_device *sdev;
- struct resource *res;
- unsigned int i;
- int ret;
- if (pdata == NULL) {
dev_err(dev->dev, "no platform data\n");return -EINVAL;- }
- sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
- if (sdev == NULL) {
dev_err(dev->dev, "failed to allocate private data\n");return -ENOMEM;- }
- sdev->dev = &pdev->dev;
- sdev->pdata = pdata;
- spin_lock_init(&sdev->irq_lock);
- sdev->ddev = dev;
- dev->dev_private = sdev;
- /* I/O resources and clocks */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (res == NULL) {
dev_err(&pdev->dev, "failed to get memory resource\n");ret = -EINVAL;goto done;- }
- sdev->mmio = ioremap_nocache(res->start, resource_size(res));
- if (sdev->mmio == NULL) {
dev_err(&pdev->dev, "failed to remap memory resource\n");ret = -ENOMEM;goto done;- }
- ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
- if (ret < 0)
goto done;- ret = shmob_drm_init_interface(sdev);
- if (ret < 0)
goto done;- ret = shmob_drm_modeset_init(sdev);
- if (ret < 0) {
dev_err(&pdev->dev, "failed to initialize mode setting\n");goto done;- }
- for (i = 0; i < 4; ++i) {
ret = shmob_drm_plane_create(sdev, i);I think you are loosing the resources allocated from shmob_drm_plane_create in case of an error below.
shmob_drm_unload() is called in the error path of shmob_drm_load(). However, it's missing a call to drm_mode_config_cleanup(). I'll add it.
if (ret < 0) {dev_err(&pdev->dev, "failed to create plane %u\n", i);goto done;}- }
- ret = drm_vblank_init(dev, 1);
- if (ret < 0) {
dev_err(&pdev->dev, "failed to initialize vblank\n");goto done;- }
- ret = drm_irq_install(dev);
- if (ret < 0) {
dev_err(&pdev->dev, "failed to install IRQ handler\n");goto done;- }
+done:
- if (ret)
shmob_drm_unload(dev);- return ret;
+}
[...]
+struct shmob_drm_device {
- struct device *dev;
- const struct shmob_drm_platform_data *pdata;
- void __iomem *mmio;
- struct clk *clock;
- struct sh_mobile_meram_info *meram;
- u32 lddckr;
- u32 ldmt1r;
- spinlock_t irq_lock; /* Protects hardware LDINTR register */
- struct drm_device *ddev;
- struct shmob_drm_crtc crtc;
- struct shmob_drm_encoder encoder;
- struct shmob_drm_connector connector;
- void *dma_mem;
- unsigned long dma_size;
- dma_addr_t dma_handle;
These three variables are unused.
Oops, good catch.
+};
+#endif /* __SHMOB_DRM_DRV_H__ */ diff --git a/drivers/gpu/drm/shmobile/shmob_drm_kms.c b/drivers/gpu/drm/shmobile/shmob_drm_kms.c new file mode 100644 index 0000000..c291ee3 --- /dev/null +++ b/drivers/gpu/drm/shmobile/shmob_drm_kms.c
[...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c new file mode 100644 index 0000000..d22644d --- /dev/null +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c @@ -0,0 +1,263 @@ +/*
- shmob_drm_crtc.c -- SH Mobile DRM CRTCs
s/shmob_drm_crtc.c/shmob_drm_plane.c/ and something about planes in the comment?
Sure :-)
- Copyright (C) 2012 Renesas Corporation
- Laurent Pinchart (laurent.pinchart@ideasonboard.com)
- 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.
- */
[...]
+int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index) +{
- struct shmob_drm_plane *splane;
- splane = kzalloc(sizeof(*splane), GFP_KERNEL);
- if (splane == NULL)
return -ENOMEM;- splane->index = index;
- splane->alpha = 255;
- return drm_plane_init(sdev->ddev, &splane->plane, 1,
&shmob_drm_plane_funcs, formats,ARRAY_SIZE(formats), false);Shouldn't splane be freed if drm_plane_init fails?
It definitely should.
+} diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.h b/drivers/gpu/drm/shmobile/shmob_drm_plane.h new file mode 100644 index 0000000..99623d0 --- /dev/null +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.h @@ -0,0 +1,22 @@ +/*
- shmob_drm_plane.h -- SH Mobile DRM Planes
- Copyright (C) 2012 Renesas Corporation
- Laurent Pinchart (laurent.pinchart@ideasonboard.com)
- 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.
- */
+#ifndef __SHMOB_DRM_PLANE_H__ +#define __SHMOB_DRM_PLANE_H__
+struct shmob_drm_device;
+int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index); +void shmob_drm_plane_setup(struct drm_plane *plane);
+#endif /* __SHMOB_DRM_PLANE_H__ */ diff --git a/drivers/gpu/drm/shmobile/shmob_drm_regs.h b/drivers/gpu/drm/shmobile/shmob_drm_regs.h new file mode 100644 index 0000000..a90517e --- /dev/null +++ b/drivers/gpu/drm/shmobile/shmob_drm_regs.h @@ -0,0 +1,304 @@ +/*
- shmob_drm_regs.g -- SH Mobile DRM registers
s/shmob_drm_regs.g/shmob_drm_regs.h/
Fixed.
- Copyright (C) 2012 Renesas Corporation
- Laurent Pinchart (laurent.pinchart@ideasonboard.com)
- 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.
- */
[...]
+static inline void lcdc_write_mirror(struct shmob_drm_device *sdev, u32 reg, + u32 data) +{
- iowrite32(data, sdev->mmio + reg + LCDC_MIRROR_OFFSET);
+}
+static inline void lcdc_write(struct shmob_drm_device *sdev, u32 reg, u32 data) +{
- iowrite32(data, sdev->mmio + reg);
- if (lcdc_is_banked(reg))
iowrite32(data, sdev->mmio + reg + LCDC_SIDE_B_OFFSET);+}
+static inline u32 lcdc_read(struct shmob_drm_device *sdev, u32 reg) +{
- return ioread32(sdev->mmio + reg);
+}
+static inline void lcdc_wait_bit(struct shmob_drm_device *sdev, u32 reg,
u32 mask, u32 until)+{
- while ((lcdc_read(sdev, reg) & mask) != until)
cpu_relax();Add a timeout here?
Good idea.
+}
+#endif /* __SHMOB_DRM_REGS_H__ */