Re: Coding style - a non-issue

From: Martin Dalecki (dalecki@evision-ventures.com)
Date: Fri Nov 30 2001 - 13:31:55 EST


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:cvs@pubcvs.arm.linux.org.uk:/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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Fri Nov 30 2001 - 21:00:40 EST