Re: Adding new driver - looking for suggestions

Frantisek Rysanek
2007-07-10T21:35:04Z

[-- Attachment #1: Mail message body --]
[-- Type: text/plain, Size: 1300 bytes --]

On 9 Jul 2007 at 19:37, Chris Doré wrote:
>
> A current patch can be found here:
> ftp://ftp.connecttech.com/pub/misc/bhbltnptm-2.6.21.patch
>
I'm just another by-stander, I don't have any vote, let me just
present my opinion...

+++ linux-2.6.21-cti/include/linux/serial_core.h2007-07-06
03:44:52.000000000 - 0400
@@ -274,6 +274,9 @@ struct uart_port {
unsigned
intline;/* port index */
unsigned longmapbase;/* for
ioremap */
struct
device*dev;/* parent device
*/
+intlmode;
+int(*lmode_fn)(struct
uart_port *port, int ioctl, unsigned int *value);
+int(*ptm_fn)(struct uart_port
*port, int ioctl, unsigned int *value);

As far as patching the vanilla kernel is concerned:
You seem to suggest an addition of rather proprietary data (members)
to the neatly generic struct uart_port. I'd suggest adding something
like

void* priv; // device-specific driver private data

to the generic struct uart_port of the vanilla kernel, which you
could then use to hook your own private data struct, regardless of
whether the rest of your driver lives in 8250_pci.c or in a stand-
alone module. This is the way it's done in a number of other generic
device structs (or maybe the more modern way is to use a nested
struct with "parent struct decoding").

Frank Rysanek

[-- Attachment #2: Mail message body --]
[-- Type: Application/Octet-stream, Size: 6262 bytes --]

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">









On 9 Jul 2007 at 19:37, Chris Dor&#233; wrote:




>




> A current patch can be found here:


&#160;




> ftp://ftp.connecttech.com/pub/misc/bhbltnptm-2.6.21.patch


&#160;




>




I'm just another by-stander, I don't have any vote, let me just present my opinion...


















+++ linux-2.6.21-cti/include/linux/serial_core.h&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;2007-07-06 03:44:52.000000000 -
0400




@@ -274,6 +274,9 @@ struct uart_port {




&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;unsigned int&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;line;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;/*
port index */




&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;unsigned long&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;mapbase;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;/* for ioremap */




&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;struct device&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;*dev;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;/* parent
device */




+&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;int&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;lmode;




+&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;int&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;(*lmode_fn)(struct uart_port *port, int ioctl, unsigned int *value);




+&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;int&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;(*ptm_fn)(struct uart_port *port, int ioctl, unsigned int *value);


















As far as patching the vanilla kernel is concerned:




You seem to suggest an addition of rather proprietary data (members) to the neatly generic




struct uart_port. I'd suggest adding something like











&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;&#160;void* priv;&#160; // device-specific driver private data











to the generic struct uart_port of the vanilla kernel, which you could then use to hook your
own private data struct, regardless of whether the rest of your driver lives in 8250_pci.c or in
a stand-alone module. This is the way it's done in a number of other generic device structs.











Frank Rysanek






Attachments:
C:\DOCUME~1\Fanouch\LOCALS~1\Temp\WPM$252E.PM$