On 03/01/2012 10:39 PM, Somebody in the thread at some point said:
From: benjamin gaignard benjamin.gaignard@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