On Thu, 7 Jun 2001, Greg KH wrote:
> On Tue, Jun 05, 2001 at 10:27:22PM +0200, Kai Germaschewski wrote:
> > The first question is about where to put the source. AFAICS, all USB
> > drivers are kept in drivers/usb. So I'd suggest to put the USB interface
> > module there, too. The main part, the extension to the hisax module needs
> > to be in drivers/isdn/hisax, because it will be linked into hisax.o
>
> Putting the driver someplace outside of the usb directory is done in
> some cases. Look at the irda usb driver for example. And since you
> need to link it to a separate file, it sounds like it makes sense to put
> it in the isdn directory.
Yes, I agree for the time being. In the future (2.5) however, it makes
sense to split hardware and protocol handling (at the module level), so
the file locations can be reconsidered then.
> Try sending it to the list. I'm sure people would be glad to look it
> over (I sure would.)
Okay, attached is the tarball Frode sent. I added some comments at various
places while looking at the code, so I'm appending a diff with these
comments, too.
diff -u st5481_cvs_0.6/st5481.c st5481_cvs_0.6.kai/st5481.c
--- st5481_cvs_0.6/st5481.c Tue Jun 5 15:36:21 2001
+++ st5481_cvs_0.6.kai/st5481.c Tue Jun 5 23:20:26 2001
@@ -1,4 +1,4 @@
-/*
+/* $Id: hisax.h,v 2.52.6.5 2001/05/26 15:19:57 kai Exp $
*
* HiSax ISDN driver - chip specific routines for ST5481 USB ISDN modem
*
@@ -72,6 +72,7 @@
return -1;
}
+ // FIXME use spinlock
save_flags(flags);
cli();
@@ -145,6 +146,7 @@
return (urbs[0]==urb ? 0 : 1);
}
+// FIXME why not inline?
#define submit_urb(urb) \
{ \
int status; \
@@ -193,7 +195,7 @@
}
/*
- Make the transfer_buffer contigous by
+ Make the transfer_buffer contiguous by
copying from the iso descriptors if necessary.
*/
static int
@@ -265,7 +267,7 @@
(unsigned char *)&hw->ctrl_msg_fifo->data[r_index];
-#if DEBUG
+#if DEBUG // FIXME name ?
{
struct ctrl_msg *ctrl_msg = (struct ctrl_msg *)urb->setup_packet;
DBG(1,"request=0x%02x,value=0x%04x,index=%x",
@@ -399,6 +401,8 @@
B channel functions
*/
+// FIXME?
+
static void
st5481B_sched_event(struct BCState *bcs, int event)
{
@@ -448,11 +452,12 @@
if ((skb = bcs->tx_skb)) {
DUMP_SKB(0x10, skb);
DBG(4,"B%d,len=%d",bcs->channel+1,skb->len);
-
+// FIXME what about transparent?
len += hdlc_encode(hw->hdlc_state_out,
skb->data, skb->len, &bytes_sent,
urb->transfer_buffer+len, buf_size-len);
+// FIXME can that happen? what happens in that case?
skb_pull(skb, bytes_sent);
if (!skb->len) {
@@ -464,6 +469,7 @@
(PACKET_NOACK != skb->pkt_type))
bcs->st->lli.l1writewakeup(bcs->st, tmp);
dev_kfree_skb_any(skb);
+// FIXME ?
if (!(bcs->tx_skb = skb_dequeue(&bcs->squeue))) {
st5481B_sched_event(bcs, B_XMTBUFREADY);
}
@@ -524,7 +530,7 @@
}
/*
- Start transferring (flags or data) on the B channel, since
+ Start transfering (flags or data) on the B channel, since
FIFO counters has been set to a non-zero value.
*/
static void
@@ -587,6 +593,7 @@
if (bcs->mode) {
// Open the B channel
+// FIXME 56K?
hdlc_rcv_init(hw->hdlc_state_in,
bcs->mode==L1_MODE_HDLC_56K, bcs->mode==L1_MODE_TRANS);
hdlc_out_init(hw->hdlc_state_out,0,
@@ -603,6 +610,7 @@
usb_device_ctrl_msg(bcs->cs, OUT_B1_COUNTER+(bcs->channel*2), 32,
NULL, NULL);
usb_device_ctrl_msg(bcs->cs, IN_B1_COUNTER+(bcs->channel*2), 32,
st5481B_start_xfer, bcs);
+// FIXME handle like Elsa?
#if NUMBER_OF_LEDS == 4
if (bc == 0) {
bcs->cs->hw.st5481.leds |= B1_LED;
@@ -646,6 +654,7 @@
switch (pr) {
case (PH_DATA | REQUEST):
+ // FIXME?
save_flags(flags);
cli();
if (st->l1.bcs->tx_skb) {
@@ -2411,16 +2420,12 @@
char tmp[64];
struct usb_device *dev;
int i;
-
- if (cs->typ != ISDN_CTYPE_ST5481) {
- return (0);
- }
-
+
+ // FIXME: broken for sizeof(void *) != sizeof(int)
+ // affects PCMCIA, too
dev = (struct usb_device *)card->para[1];
- if (!dev) {
- WARN("no usb_device");
- return (0);
- }
+ if (!dev)
+ BUG();
strcpy(tmp, st5481_revision);
INFO("Rev. %s",HiSax_getrev(tmp));
Only in st5481_cvs_0.6.kai/: st5481.c%
diff -u st5481_cvs_0.6/st5481_usb.c st5481_cvs_0.6.kai/st5481_usb.c
--- st5481_cvs_0.6/st5481_usb.c Tue Jun 5 15:36:21 2001
+++ st5481_cvs_0.6.kai/st5481_usb.c Tue Jun 5 23:21:08 2001
@@ -30,6 +30,9 @@
int protocol;
int cardnr;
} st5481_usb_adapter;
+
+// FIXME: What about more than one adapter?
+
static st5481_usb_adapter usb_adapter_instance;
@@ -48,14 +51,11 @@
printk( KERN_INFO __FILE__ ": "__FUNCTION__ ": VendorId %04x,ProductId %04x\n",
dev->descriptor.idVendor,dev->descriptor.idProduct);
- if (dev->descriptor.idVendor != ST_VENDOR_ID) {
- return NULL;
- }
+ if (dev->descriptor.idVendor != ST_VENDOR_ID)
+ BUG();
- if ((dev->descriptor.idProduct & ST5481_PRODUCT_ID_MASK) !=
- ST5481_PRODUCT_ID) {
- return NULL;
- }
+ if ((dev->descriptor.idProduct & ST5481_PRODUCT_ID_MASK) != ST5481_PRODUCT_ID)
+ BUG();
usb_adapter = &usb_adapter_instance;
usb_adapter->busy_flag = 0;
@@ -121,7 +121,7 @@
id_table: st5481_ids,
};
-int
+static int __init
st5481_usb_init(void)
{
st5481_usb_adapter *usb_adapter = &usb_adapter_instance;
@@ -140,7 +140,7 @@
return 0;
}
-void
+static void __exit
st5481_usb_cleanup(void)
{
st5481_usb_adapter *usb_adapter = &usb_adapter_instance;
st5481_cvs_0.6.tar.gz
