Hi Laurent,
Some minor stuff inside.
Thanks
Sascha+
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@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/Kconfig | 2 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/shmobile/Kconfig | 10 +
> drivers/gpu/drm/shmobile/Makefile | 7 +
> drivers/gpu/drm/shmobile/shmob_drm_backlight.c | 90 +++
> drivers/gpu/drm/shmobile/shmob_drm_backlight.h | 23 +
> drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 768 ++++++++++++++++++++++++
> drivers/gpu/drm/shmobile/shmob_drm_crtc.h | 60 ++
> drivers/gpu/drm/shmobile/shmob_drm_drv.c | 360 +++++++++++
> drivers/gpu/drm/shmobile/shmob_drm_drv.h | 52 ++
> drivers/gpu/drm/shmobile/shmob_drm_kms.c | 160 +++++
> drivers/gpu/drm/shmobile/shmob_drm_kms.h | 34 +
> drivers/gpu/drm/shmobile/shmob_drm_plane.c | 263 ++++++++
> drivers/gpu/drm/shmobile/shmob_drm_plane.h | 22 +
> drivers/gpu/drm/shmobile/shmob_drm_regs.h | 304 ++++++++++
> include/drm/shmob_drm.h | 99 +++
> 16 files changed, 2255 insertions(+), 0 deletions(-)
> create mode 100644 drivers/gpu/drm/shmobile/Kconfig
> create mode 100644 drivers/gpu/drm/shmobile/Makefile
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_backlight.c
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_backlight.h
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_crtc.h
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_drv.c
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_drv.h
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_kms.c
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_kms.h
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_plane.c
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_plane.h
> create mode 100644 drivers/gpu/drm/shmobile/shmob_drm_regs.h
> create mode 100644 include/drm/shmob_drm.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 98ba7d5..0b40bf2 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -208,3 +208,5 @@ source "drivers/gpu/drm/ast/Kconfig"
> source "drivers/gpu/drm/mgag200/Kconfig"
>
> source "drivers/gpu/drm/cirrus/Kconfig"
> +
> +source "drivers/gpu/drm/shmobile/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 58961b9..2ff5cef 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -47,4 +47,5 @@ obj-$(CONFIG_DRM_EXYNOS) +=exynos/
> obj-$(CONFIG_DRM_GMA500) += gma500/
> obj-$(CONFIG_DRM_UDL) += udl/
> obj-$(CONFIG_DRM_AST) += ast/
> +obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
> obj-y += i2c/
> 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.
[...]
> +
> +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.
[...]
> +
> +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 (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.
> + 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.
> +};
> +
> +#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?
> + *
> + * Copyright (C) 2012 Renesas Corporation
> + *
> + * Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> + *
> + * 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?
> +}
> 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@xxxxxxxxxxxxxxxx)
> + *
> + * 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/
> + *
> + * Copyright (C) 2012 Renesas Corporation
> + *
> + * Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> + *
> + * 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?
> +}
> +
> +#endif /* __SHMOB_DRM_REGS_H__ */
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
