From: Sam R. <sa...@ra...> - 2004-08-20 18:07:05
|
On Thu, Aug 19, 2004 at 03:26:07PM -0700, Erdi Chen wrote: > This patch adds two new ioctl's to the VIA Unichrome/Pro DRM driver: > DRM_IOCTL_VIA_DMA_INIT > DRM_IOCTL_VIA_CMDBUFFER > > The first call sets up an area in AGP memory that will be used as the > ring buffer. The second call copies a command buffer from user space > memory to the ring buffer. A few comments to the coding style - nothing to the functionality. [inlining your patch would make it soo much easier to review and comment] 1) Defines are mixed casisng. Usual style is upper case only except if casing match definitions in datasheet. 2) via_dma.c - remove emacs tagging "-*- linux-c -*- 3) What's the purpose of "#define __NO_VERSION__" 4) viaCheckDma => via_check_dma 5) via_dma_cleanup always return 0. No need to return anything. - Also drop extern in prototype in .h file 6) SetRag2DAGP - no mixed casing - why is vb casted to uint32_t* - vb is of correct type. - Put the define close to the (sole) user 7) viaAlingBuffer - no mixed casing in name - use tabs for indention 8) via_cmdbuf_wait - no mixed casing in name - use tabs for indention 9) In general use tabs for indention 10) What doeis this do? qwPadCount = (CMDBUF_ALIGNMENT_SIZE>>3) - ((((uint32_t)vb) & CMDBUF_ALIGNMENT_MASK) >> 3); There is _no_ guarantee a pointer will fir into uint_32 Above construction used in several places. 11) via_drm.h - Drop typedef of drm_via_dma_init - Linux style is to _not_ hide when you work with a struct - Same for drm_via_cmdbuffer_t 12) Comment about typedef is also valid for via_drv.h 13) unsigned int dmaLow - I'm suspicious this is OK on 64 bit platforms? Sam |