[PATCH 3/3] add CMA heap

[Andy Green]

On 03/01/2012 10:39 PM, Somebody in the thread at some point said:
> From: benjamin gaignard <benjamin.gaignard at linaro.org>
> 
> New heap, allocation is done with dma_alloc_coherent API.
> device coherent_dma_mask is set to DMA_BIT_MASK(32)

Hi -

I don't know much about ion, but just some small style comments.
However I think it's great to bind it to the standardized cma.

It might be better to repost on linaro-dev / linaro-kernel to get
interest of people who do know about ion.

> +#include <linux/device.h>
> +#include <linux/ion.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/miscdevice.h>
> +
> +/* for ion_heap_ops structure */
> +#include "../ion_priv.h"
> +
> +#include <linux/dma-mapping.h>

Group all the same subpath includes together (ie, put with other
<linux/... )

> +#define ION_CMA_ALLOCATE_FAILED -1
> +
> +static u64 dma_mask = 0xffffffffULL;

You used DMA_BIT_MASK(32) in the code, maybe here too.

> +/* return physical address in addr */
> +int ion_cma_phys(struct ion_heap *heap, struct ion_buffer *buffer,
> +		 ion_phys_addr_t *addr, size_t *len)
> +{
> +	struct ion_device *idev = buffer->dev;
> +	struct device *dev = idev->dev.parent;
> +	struct ion_cma_buffer_info *info = buffer->priv_virt;
> +
> +	dev_dbg(dev, "Return buffer %p physical address 0x%x\n", buffer,
> +		virt_to_phys(info->cpu_addr));
> +	*addr = virt_to_phys(info->cpu_addr);
> +	*len = buffer->size;

I'm not sure if you're planning on sending this to mainline, if you are
it's worth running it through checkpatch.pl which will tell you to add a
blank line here, etc.

> +	return 0;
> +}

> +int ion_cma_mmap(struct ion_heap *mapper, struct ion_buffer *buffer,
> +		 struct vm_area_struct *vma)
...
> +static struct ion_heap_ops ion_cma_ops = {
> +	.allocate = ion_cma_allocate,
> +	.free = ion_cma_free,
> +	.phys = ion_cma_phys,
> +	.map_user = ion_cma_mmap,
> +};

If these functions are only used by this api table, they can be marked
up as static scope at their definitions.

> +struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
> +{
> +	struct ion_heap *heap;
> +
> +	heap = ion_heap_create(data);
> +	if (!heap)
> +		return NULL;
> +	heap->ops = &ion_cma_ops;
> +	return heap;
> +}

Can be simplified to

	if (heap)
		heap->ops = &ion_cma_ops;

	return heap;

> +/* ION CMA heap platform driver functions */
> +int ion_cma_probe(struct platform_device *pdev)
> +{
> +	struct ion_platform_data *pdata = pdev->dev.platform_data;
> +	int num_heaps = pdata->nr;
> +	int i, err;
> +
> +	heaps = kzalloc(sizeof(struct ion_heap *) * num_heaps, GFP_KERNEL);
> +
> +	dev_info(&pdev->dev, "Create ION CMA device %p\n", &pdev->dev);
> +	ion_cma_device = ion_device_create(NULL);
> +	if (IS_ERR_OR_NULL(ion_cma_device)) {
> +		kfree(heaps);
> +		dev_err(&pdev->dev, "Can't create ION CMA device\n");
> +		return PTR_ERR(ion_cma_device);
> +	}
> +
> +	ion_cma_device->dev.parent = &(pdev->dev);

() not needed

> +err:
> +	for (i = 0; i < num_heaps; i++) {
> +		if (heaps[i])
> +			ion_heap_destroy(heaps[i]);
> +	}

{ } are not needed

> +	kfree(heaps);
> +	return err;
> +}
> +
> +int ion_cma_remove(struct platform_device *pdev)
> +{
> +	struct ion_device *idev = platform_get_drvdata(pdev);
> +	struct ion_platform_data *pdata = pdev->dev.platform_data;
> +	int i, num_heaps;
> +
> +	num_heaps = pdata->nr;
> +
> +	/* remove ION device */
> +	ion_device_destroy(idev);
> +
> +	/* remove all heaps create by the driver */
> +	for (i = 0; i < num_heaps; i++) {
> +		if (heaps[i])
> +			ion_heap_destroy(heaps[i]);
> +	}
> +	return 0;
> +}

{ } not needed

> +static struct platform_driver ion_cma_driver = {
> +	.probe = ion_cma_probe,
> +	.remove = ion_cma_remove,
> +	.driver = {
> +		   .name = "ion-cma"}

} should be on next line indented

-Andy
-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog