[PATCH v2 7/7] drm: Renesas SH Mobile DRM driver

Sascha Hauer s.hauer at pengutronix.de
Fri Aug 3 05:48:39 PDT 2012


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 at ideasonboard.com>
> ---
>  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 at 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?

> +}
> 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 at 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/

> + *
> + * Copyright (C) 2012 Renesas Corporation
> + *
> + * Laurent Pinchart (laurent.pinchart at 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?

> +}
> +
> +#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 |


More information about the dri-devel mailing list