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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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__ */

-- 
Regards,

Laurent Pinchart