Messages in this thread | | | Subject | Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework | From | "Andrew F. Davis" <> | Date | Fri, 15 Mar 2019 15:24:44 -0500 |
| |
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.
| |