lkml.org 
[lkml]   [2019]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
From
Date
On 3/15/19 3:54 AM, Christoph Hellwig wrote:
>> +static int dma_heap_release(struct inode *inode, struct file *filp)
>> +{
>> + filp->private_data = NULL;
>> +
>> + return 0;
>> +}
>
> No point in clearing ->private_data, the file is about to be freed.
>

This was leftover from when release had some memory to free, will remove.

>> +
>> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd,
>> + unsigned long arg)
>
> Pleae don't use the weird legacy filp naming, file is a perfectly
> valid and readable default name for struct file pointers.
>

Thanks for info, I saw both used and this was used where I found the
prototype so I used it too, will fix.

>> +{
>> + switch (cmd) {
>> + case DMA_HEAP_IOC_ALLOC:
>> + {
>> + struct dma_heap_allocation_data heap_allocation;
>> + struct dma_heap *heap = filp->private_data;
>> + int fd;
>
> Please split each ioctl into a separate function from the very start,
> otherwise this will grow into a spaghetty mess sooner than you can
> see cheese.
>

Good idea, will fix.

>> + dev_ret = device_create(dma_heap_class,
>> + NULL,
>> + heap->heap_devt,
>> + NULL,
>> + heap->name);
>
> No need this weird argument alignment.
>

I kinda like it this way, if everything cant fit on one line then
everything gets its own line, seems more consistent. If there is strong
objection I can fix.

\
 
 \ /
  Last update: 2019-03-15 21:26    [W:0.166 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site