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@xxxxxxxxxxxxxxxx>
[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@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?
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@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/
Fixed.
> > + *
> > + * 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?
Good idea.
> > +}
> > +
> > +#endif /* __SHMOB_DRM_REGS_H__ */
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
