Re: [PATCH][v1] powerpc/fsl: 85xx: add cache-sram support

[Philipp Ittershagen <lists@gate-nine.de>]

Hi Harninder,

On Wed, 2010-10-13 at 14:47 +0530, harninder.rai@freescale.com wrote:
> +int __init instantiate_cache_sram(struct platform_device *dev,
> +		struct sram_parameters sram_params)
> +{
> +	if (cache_sram) {
> +		dev_err(&dev->dev, "Already initialized cache-sram\n");
> +		return -EBUSY;
> +	}
> +
> +	cache_sram = kzalloc(sizeof(struct mpc85xx_cache_sram), GFP_KERNEL);
> +	if (!cache_sram) {
> +		dev_err(&dev->dev, "Out of memory for cache_sram structure\n");
> +		return -ENOMEM;
> +	}
> +
> +	cache_sram->base_phys = sram_params.sram_offset;
> +	cache_sram->size = sram_params.sram_size;
> +
> +	if (!request_mem_region(cache_sram->base_phys, cache_sram->size,
> +						"fsl_85xx_cache_sram")) {
> +		dev_err(&dev->dev, "%s: request memory failed\n",
> +				dev->dev.of_node->full_name);
> +		kfree(cache_sram);
> +		return -ENXIO;
> +	}
> +
> +	cache_sram->base_virt = ioremap_flags(cache_sram->base_phys,
> +				cache_sram->size, _PAGE_COHERENT | PAGE_KERNEL);
> +	if (!cache_sram->base_virt) {
> +		dev_err(&dev->dev, "%s: ioremap_flags failed\n",
> +				dev->dev.of_node->full_name);
> +		release_mem_region(cache_sram->base_phys, cache_sram->size);
> +		kfree(cache_sram);
> +		return -ENOMEM;
> +	}
> +
> +	cache_sram->rh = rh_create(sizeof(unsigned int));
> +	if (IS_ERR(cache_sram->rh)) {
> +		dev_err(&dev->dev, "%s: Unable to create remote heap\n",
> +				dev->dev.of_node->full_name);
> +		iounmap(cache_sram->base_virt);
> +		release_mem_region(cache_sram->base_phys, cache_sram->size);
> +		kfree(cache_sram);
> +		return PTR_ERR(cache_sram->rh);
> +	}
> +
> +	rh_attach_region(cache_sram->rh, 0, cache_sram->size);
> +	spin_lock_init(&cache_sram->lock);
> +
> +	dev_info(&dev->dev, "[base:0x%llx, size:0x%x] configured and loaded\n",
> +		(unsigned long long)cache_sram->base_phys, cache_sram->size);
> +	return 0;
> +}

You could save some redundant code by using goto statements for error
handling.

For example:

...

int ret;

if (!request_mem_region(...)) {
	ret = -ENXIO;
	goto out_free;
}


if (!ioremap(...)) {
	ret = -ENOMEM;
	goto out_release;
}

if (!IS_ERR(cache_sram->rh)) {
	ret = PTR_ERR(...);
	goto out_unmap;
}

...

return 0;

out_unmap:
	iounmap(...);
out_release:
	release_mem_region(...);
out_free:
	kfree(...);
	return ret;
}



-- Philipp