Re: [patch 3/5] FB: Add support for the ILI9320 video display controller.

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 22 Jun 2008 22:04:42 +0100 Ben Dooks <ben-linux@fluff.org> wrote:

> Provide support for the ILI9320 display controller chip
> which is found in many LCD displays. Included with this
> is support for an example LCD using this chip, the
> VGG2432A4.
> 

Please pass this patch (and indeed all patches) through
scripts/checkpatch.pl.  It finds things which you wouldn't have sent
had you known they were there.

>
> ...
>
> +int __devinit ili9320_probe_spi(struct spi_device *spi,
> +				struct ili9320_client *client)
> +{
> +	struct ili9320_platdata *cfg = spi->dev.platform_data;
> +	struct device *dev = &spi->dev;
> +	struct ili9320 *ili;
> +	struct lcd_device *lcd;
> +	int ret = 0;
> +
> +	/* verify we where given some information */
> +
> +	if (cfg == NULL) {
> +		dev_err(dev, "no platform data supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	if (cfg->hsize <= 0 || cfg->vsize <= 0 || cfg->reset == NULL) {
> +		dev_err(dev, "invalid platform data supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	/* allocate and initialse our state */
> +
> +	ili = kzalloc(sizeof(struct ili9320), GFP_KERNEL);
> +	if (ili == NULL) {
> +		dev_err(dev, "no memory for device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ili->access.spi.id = ILI9320_SPI_IDCODE | ILI9320_SPI_ID(1);
> +
> +	ili->dev = dev;
> +	ili->client = client;
> +	ili->power = FB_BLANK_POWERDOWN;
> +	ili->platdata = cfg;
> +
> +	dev_set_drvdata(&spi->dev, ili);
> +
> +	ili9320_setup_spi(ili, spi);
> +
> +	lcd = lcd_device_register("ili9320", dev, ili, &ili9320_ops);
> +	if (IS_ERR(lcd)) {
> +		dev_err(dev, "failed to register lcd device\n");
> +		ret = PTR_ERR(lcd);
> +		goto err_free;
> +	}
> +
> +	ili->lcd = lcd;
> +
> +	dev_info(dev, "initialising %s\n", client->name);
> +
> +	ret = ili9320_power(ili, FB_BLANK_UNBLANK);
> +	if (ret != 0) {
> +		dev_err(dev, "failed to set lcd power state\n");
> +		goto err_unregister;
> +	}
> +
> +	return 0;
> +
> + err_unregister:
> +	lcd_device_unregister(lcd);
> +
> + err_free:
> +	kfree(lcd);
> +	return ret;
> +}

urgh.  lcd_device_register() returns kmalloced memory which the caller
must kfree() after having run lcd_device_unregister()?  Who thought
that scheme up?  :(

Does this function leaks *ili on errors?

> +EXPORT_SYMBOL_GPL(ili9320_probe_spi);
> +
> +int __devexit ili9320_remove(struct ili9320 *lcd)
> +{
> +	ili9320_power(lcd, FB_BLANK_POWERDOWN);
> +
> +	lcd_device_unregister(lcd->lcd);
> +	kfree(lcd);
> +
> +	return 0;
> +}
> +
> +EXPORT_SYMBOL_GPL(ili9320_remove);

Did ili get freed in here?

>
> ...
>
> +/* Holder for register and value pairs. */
> +struct ili9320_reg {
> +	unsigned short		address;
> +	unsigned short		value;
> +};
> +
> +struct ili9320;
> +
> +struct ili9320_client {
> +	const char	*name;
> +	int	(*init)(struct ili9320 *ili, struct ili9320_platdata *cfg);
> +
> +};
> +/* Device attached via an SPI bus. */
> +struct  ili9320_spi {

s/  / /

> +	struct spi_device	*dev;
> +	struct spi_message	message;
> +	struct spi_transfer	xfer[2];
> +
> +	unsigned char		id;
> +	unsigned char		buffer_addr[4];
> +	unsigned char		buffer_data[4];
> +};
> +
>
> ...
>
> +static int vgg2432a4_lcd_init(struct ili9320 *lcd,
> +			      struct ili9320_platdata *cfg)
> +{
> +	unsigned int addr;
> +	int ret;
> +
> +	/* Set VCore before anything else (VGG243237-6UFLWA) */
> +	ret = ili9320_write(lcd, 0x00e5, 0x8000);
> +	if (ret)
> +		goto err_initial;
> +
> +	/* Start the oscillator up before we can do anything else. */
> +	ret = ili9320_write(lcd, ILI9320_OSCILATION, ILI9320_OSCILATION_OSC);
> +	if (ret)
> +		goto err_initial;
> +
> +	/* must wait at-lesat 10ms after starting */
> +	mdelay(15);

msleep()?  And everywhere else.  This function alone consumes maybe
half a second CPU time.

>
> ...
>

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php