Re: Coding style - a non-issue

[Martin Dalecki]

Russell King wrote:
> 
> On Fri, Nov 30, 2001 at 06:49:01PM +0100, Martin Dalecki wrote:
> > Well sombeody really cares apparently! Thank's.
> 
> Currently its a restructuring of serial.c to allow different uart type
> ports to be driven without duplicating serial.c many times over.  (the
> generic_serial didn't hack it either).
> 
> > Any pointers where to ahve a look at it?
> 
> I should probably put this on a web page somewhere 8)
> 
>   :pserver:[e-mail]:/mnt/src/cvsroot, module serial
>   (no password)
> 
> > BTW> Did you consider ther misc device idea? (Hooking serial at to the
> > misc device stuff).
> 
> I just caught that comment - I'll await your reply.

I'm just right now looking at the code found above.
First of all: GREAT WORK! And then you are right a bit, I just don't
see too much code duplication between the particular drivers there.
However I still don't see the need to carrige the whole tty stuff just
to drive a mouse... but I don't see a solution right now.
I wouldn't be good to introduce a scatter heap of "micro" 
driver modules like the ALSA people did as well too..

However in serial/linux/drivers/serial/serial_clps711x.c
the following wonders me a bit:

#ifdef CONFIG_DEVFS_FS
	normal_name:		SERIAL_CLPS711X_NAME,
	callout_name:		CALLOUT_CLPS711X_NAME,
#else
	normal_name:		SERIAL_CLPS711X_NAME,
	callout_name:		CALLOUT_CLPS711X_NAME,
#endif

What is the ifdef supposed to be good for?

 
One other suggestion serial_code.c could be just serial.c or code.c
or main.c, since _xxxx.c should distinguish between particulart devices.
It was a bit clumy to find the entry-point for me...
And then we have in uart_register_driver:

	normal->open		= uart_open;
	normal->close		= uart_close;
	normal->write		= uart_write;
	normal->put_char	= uart_put_char;
	normal->flush_chars	= uart_flush_chars;
	normal->write_room	= uart_write_room;
	normal->chars_in_buffer	= uart_chars_in_buffer;
	normal->flush_buffer	= uart_flush_buffer;
	normal->ioctl		= uart_ioctl;
	normal->throttle	= uart_throttle;
	normal->unthrottle	= uart_unthrottle;
	normal->send_xchar	= uart_send_xchar;
	normal->set_termios	= uart_set_termios;
	normal->stop		= uart_stop;
	normal->start		= uart_start;
	normal->hangup		= uart_hangup;
	normal->break_ctl	= uart_break_ctl;
	normal->wait_until_sent	= uart_wait_until_sent;

And so on....

Why not do:

{
     static strcut tty_driver _normal = {
     open: uart_open,
     close: uart_close,
     ...
     };

     ...

     *normal = _normal;
}
...
for the static stuff first and only initialize the
dynamically calculated addresses dynamically later, like
the double refferences...
This would save already *quite a bit* of .text ;-).

Yeah I know I'm a bit perverse about optimizations....

You already do it for the callout device nearly this
way:

	/*
	 * The callout device is just like the normal device except for
	 * the major number and the subtype code.
	 */
	*callout		= *normal;
	callout->name		= drv->callout_name;
	callout->major		= drv->callout_major;
	callout->subtype	= SERIAL_TYPE_CALLOUT;
	callout->read_proc	= NULL;
	callout->proc_entry	= NULL;

Reagrds.