Re: [rfc][patch] DRM initial function table support.

[Dave Jones <davej@redhat.com>]

On Tue, Aug 31, 2004 at 02:11:11PM +0100, Dave Airlie wrote:

 > Okay this is the first driver function table patch for the DRM, something
 > similar has been running in DRM CVS for a month or so (underneath another
 > 5-10 patches removing all the rest of the macros..)

I like this. Lots.
Mostly because of this..

(16:14:11:davej@delerium:~)$ grep ^-# foo.diff  | wc -l
87

It's like someone revoked the drm folks preprocessor license 8-)

 > One thing we've discussed on dri-devel was changing the
 > if (dev->fn_tbl.function)
 >         dev->fn_tbl.function();
 > 
 > to something else but in future patches I've had slightly more complex
 > checks to do and this check looks to be the most obvious from a
 > readability point of view, if the driver supports the function call it..

Looks ok, and pretty much matches similar conventions used in other
parts of the kernel.

 > The other option was to use default noop fns, this also caused issues
 > later on with some places where the code did more than just call the
 > function..
 > 
 > So please any comments on it?

One thing that would make things even nicer would be..

instead of this..

 > +void gamma_driver_register_fns(drm_device_t *dev)
 > +{
 > +	dev->fn_tbl.preinit = gamma_driver_preinit;
 > +	dev->fn_tbl.pretakedown = gamma_driver_pretakedown;
 > +	dev->fn_tbl.dma_ready = gamma_driver_dma_ready;
 > +	dev->fn_tbl.dma_quiescent = gamma_driver_dma_quiescent;
 > +	dev->fn_tbl.dma_flush_block_and_flush = gamma_flush_block_and_flush;
 > +	dev->fn_tbl.dma_flush_unblock = gamma_flush_unblock;
 > +}

having a per-driver struct with regular C99 initialisers..

struct gamma_driver_fntbl {
	.preinit = gamma_driver_preinit,
	.pretakedown = gamma_driver_pretakedown,
	.dma_ready = gamma_driver_dma_ready,
	.dma_quiescent = gamma_driver_dma_quiescent,
	.dma_flush_block_and_flush = gamma_flush_block_and_flush,
	.dma_flush_unblock = gamma_flush_unblock,
};

Thanks for doing this work, it really is starting to look a little more
like a Linux driver 8-)

		Dave