Re: [PATCH] ipwireless: driver for 3G PC Card
Hi!
> > > +static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
> > > + struct ipw_hardware *hw)
> > > +{
> > > + unsigned short irqn;
> > > + unsigned short ack;
> > > +
> > > + irqn = inw(hw->base_port + IOIR);
> > > +
> > > + /* Check if card is present */
> > > + if (irqn == (unsigned short) 0x) {
> > > + if (++hw->bad_interrupt_count >= 100) {
> > > + /*
> > > + * It is necessary to disable the interrupt at this
> > > + * point, or the kernel hangs, interrupting repeatedly
> > > + * forever.
> > > + */
> > > + hw->irq = irq;
> > > + hw->removed = 1;
> > > + disable_irq_nosync(irq);
> > > + printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
> > > + ": Mr. Fluffy is not happy!\n");
> > > + }
> > > + return IRQ_HANDLED;
> >
> > Not sure how this is supposed to work. If you assume unshared
> > interrupts, it should be possible to return something and make core
> > care.
> >
> > If you are assuming shared interrupts, either you should disable on
> > first 0x (are you sure cast is needed, btw?), or not at all,
> > because it could be the other device sedning you 100 of those...
> >
> > ...so which one is it?
>
> Shared. It'll check if device has interrupt pending, else exit.
can you remove bad_interrupt_count? It seems very random in presence
of shared interrupt.
> > > +static int config_ipwireless(struct ipw_dev *ipw)
> > > +{
> > > + struct pcmcia_device *link = ipw->link;
> > > + int ret;
> > > + config_info_t conf;
> > > + tuple_t tuple;
> > > + unsigned short buf[64];
> > > + cisparse_t parse;
> > > + unsigned short cor_value;
> > > + win_req_t reqAM;
> > > + win_req_t reqCM;
> >
> > Hiding structs BehindTypedefsIsEvil.
>
> Unfortunatelly PCMCIA subsystem is full of these and all drivers use them.
> I'll stay consistent for now.
Sorry if they were not yours.
> Updated patch v4 will follow.
Thanks!
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi,
> > +/* DCR bits */
> > +#define DCR_RXDONE ((unsigned short) 0x1)
> > +#define DCR_TXDONE ((unsigned short) 0x2)
> > +#define DCR_RXRESET ((unsigned short) 0x4)
> > +#define DCR_TXRESET ((unsigned short) 0x8)
>
> Are those casts neccessary?
No, removed.
> > +/* I/O ports and bit definitions for version 2 of the hardware */
> > +
> > +struct MEMCCR {
> > + unsigned short PCCOR; /* Configuration Option Register */
> > + unsigned short PCCSR; /* Configuration and Status Register */
> > + unsigned short PCPRR; /* Pin Replacemant Register */
> > + unsigned short PCSCR; /* Socket and Copy Register */
> > + unsigned short PCESR; /* Extendend Status Register */
> > + unsigned short PCIOB; /* I/O Base Register */
> > +};
>
> Could we get better names? PCIOB is cryptic, pci_io_base is pretty
> good.
Ok, changed to eg. reg_config_and_status.
> > +/* Signals from DTE */
> > +enum ComCtrl_DTESignal {
> > + ComCtrl_RTS = 0,
> > + ComCtrl_DTR = 1
> > +};
>
> CamelCaseIsEvil.
Converted to underscores.
>
> > +static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
> > + struct ipw_hardware *hw)
> > +{
> > + unsigned short irqn;
> > + unsigned short ack;
> > +
> > + irqn = inw(hw->base_port + IOIR);
> > +
> > + /* Check if card is present */
> > + if (irqn == (unsigned short) 0x) {
> > + if (++hw->bad_interrupt_count >= 100) {
> > + /*
> > +* It is necessary to disable the interrupt at this
> > +* point, or the kernel hangs, interrupting repeatedly
> > +* forever.
> > +*/
> > + hw->irq = irq;
> > + hw->removed = 1;
> > + disable_irq_nosync(irq);
> > + printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
> > + ": Mr. Fluffy is not happy!\n");
> > + }
> > + return IRQ_HANDLED;
>
> Not sure how this is supposed to work. If you assume unshared
> interrupts, it should be possible to return something and make core
> care.
>
> If you are assuming shared interrupts, either you should disable on
> first 0x (are you sure cast is needed, btw?), or not at all,
> because it could be the other device sedning you 100 of those...
>
> ...so which one is it?
Shared. It'll check if device has interrupt pending, else exit.
> Is some locking needed around *hw?
Some items are set during initial phase and read only afterwards. This don't
need to be protected. Nevetheless, I've found some missing locking around
tx_ready and rx_ready which might cause bugs (eg. hangs).
>
> > +int ipwireless_send_packet(struct ipw_hardware *hw, unsigned int
> > channel_idx, + unsigned char *data, unsigned int
> > length,
> > + void (*callback) (void *cb, unsigned int length),
> > + void *callback_data)
> > +{
> > + struct ipw_tx_packet *packet;
> > +
> > + packet = alloc_data_packet(length,
> > + (unsigned char) (channel_idx + 1),
> > + TL_PROTOCOLID_COM_DATA);
> > + if (!packet)
> > + return -1;
>
> -ENOMEM would be more usual calling convention.
Done.
>
> > +struct ipw_setup_get_version_query_packet {
> > + struct ipw_tx_packet header;
> > + struct TlSetupGetVersionQry body;
> > +};
>
> MoreEvilCamelCase.
Converted.
> > +static int config_ipwireless(struct ipw_dev *ipw)
> > +{
> > + struct pcmcia_device *link = ipw->link;
> > + int ret;
> > + config_info_t conf;
> > + tuple_t tuple;
> > + unsigned short buf[64];
> > + cisparse_t parse;
> > + unsigned short cor_value;
> > + win_req_t reqAM;
> > + win_req_t reqCM;
>
> Hiding structs BehindTypedefsIsEvil.
Unfortunatelly PCMCIA subsystem is full of these and all drivers use them.
I'll stay consistent for now.
Updated patch v4 will follow.
dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi,
+/* DCR bits */
+#define DCR_RXDONE ((unsigned short) 0x1)
+#define DCR_TXDONE ((unsigned short) 0x2)
+#define DCR_RXRESET ((unsigned short) 0x4)
+#define DCR_TXRESET ((unsigned short) 0x8)
Are those casts neccessary?
No, removed.
+/* I/O ports and bit definitions for version 2 of the hardware */
+
+struct MEMCCR {
+ unsigned short PCCOR; /* Configuration Option Register */
+ unsigned short PCCSR; /* Configuration and Status Register */
+ unsigned short PCPRR; /* Pin Replacemant Register */
+ unsigned short PCSCR; /* Socket and Copy Register */
+ unsigned short PCESR; /* Extendend Status Register */
+ unsigned short PCIOB; /* I/O Base Register */
+};
Could we get better names? PCIOB is cryptic, pci_io_base is pretty
good.
Ok, changed to eg. reg_config_and_status.
+/* Signals from DTE */
+enum ComCtrl_DTESignal {
+ ComCtrl_RTS = 0,
+ ComCtrl_DTR = 1
+};
CamelCaseIsEvil.
Converted to underscores.
+static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
+ struct ipw_hardware *hw)
+{
+ unsigned short irqn;
+ unsigned short ack;
+
+ irqn = inw(hw-base_port + IOIR);
+
+ /* Check if card is present */
+ if (irqn == (unsigned short) 0x) {
+ if (++hw-bad_interrupt_count = 100) {
+ /*
+* It is necessary to disable the interrupt at this
+* point, or the kernel hangs, interrupting repeatedly
+* forever.
+*/
+ hw-irq = irq;
+ hw-removed = 1;
+ disable_irq_nosync(irq);
+ printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
+ : Mr. Fluffy is not happy!\n);
+ }
+ return IRQ_HANDLED;
Not sure how this is supposed to work. If you assume unshared
interrupts, it should be possible to return something and make core
care.
If you are assuming shared interrupts, either you should disable on
first 0x (are you sure cast is needed, btw?), or not at all,
because it could be the other device sedning you 100 of those...
...so which one is it?
Shared. It'll check if device has interrupt pending, else exit.
Is some locking needed around *hw?
Some items are set during initial phase and read only afterwards. This don't
need to be protected. Nevetheless, I've found some missing locking around
tx_ready and rx_ready which might cause bugs (eg. hangs).
+int ipwireless_send_packet(struct ipw_hardware *hw, unsigned int
channel_idx, + unsigned char *data, unsigned int
length,
+ void (*callback) (void *cb, unsigned int length),
+ void *callback_data)
+{
+ struct ipw_tx_packet *packet;
+
+ packet = alloc_data_packet(length,
+ (unsigned char) (channel_idx + 1),
+ TL_PROTOCOLID_COM_DATA);
+ if (!packet)
+ return -1;
-ENOMEM would be more usual calling convention.
Done.
+struct ipw_setup_get_version_query_packet {
+ struct ipw_tx_packet header;
+ struct TlSetupGetVersionQry body;
+};
MoreEvilCamelCase.
Converted.
+static int config_ipwireless(struct ipw_dev *ipw)
+{
+ struct pcmcia_device *link = ipw-link;
+ int ret;
+ config_info_t conf;
+ tuple_t tuple;
+ unsigned short buf[64];
+ cisparse_t parse;
+ unsigned short cor_value;
+ win_req_t reqAM;
+ win_req_t reqCM;
Hiding structs BehindTypedefsIsEvil.
Unfortunatelly PCMCIA subsystem is full of these and all drivers use them.
I'll stay consistent for now.
Updated patch v4 will follow.
dave
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi!
+static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
+ struct ipw_hardware *hw)
+{
+ unsigned short irqn;
+ unsigned short ack;
+
+ irqn = inw(hw-base_port + IOIR);
+
+ /* Check if card is present */
+ if (irqn == (unsigned short) 0x) {
+ if (++hw-bad_interrupt_count = 100) {
+ /*
+ * It is necessary to disable the interrupt at this
+ * point, or the kernel hangs, interrupting repeatedly
+ * forever.
+ */
+ hw-irq = irq;
+ hw-removed = 1;
+ disable_irq_nosync(irq);
+ printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
+ : Mr. Fluffy is not happy!\n);
+ }
+ return IRQ_HANDLED;
Not sure how this is supposed to work. If you assume unshared
interrupts, it should be possible to return something and make core
care.
If you are assuming shared interrupts, either you should disable on
first 0x (are you sure cast is needed, btw?), or not at all,
because it could be the other device sedning you 100 of those...
...so which one is it?
Shared. It'll check if device has interrupt pending, else exit.
can you remove bad_interrupt_count? It seems very random in presence
of shared interrupt.
+static int config_ipwireless(struct ipw_dev *ipw)
+{
+ struct pcmcia_device *link = ipw-link;
+ int ret;
+ config_info_t conf;
+ tuple_t tuple;
+ unsigned short buf[64];
+ cisparse_t parse;
+ unsigned short cor_value;
+ win_req_t reqAM;
+ win_req_t reqCM;
Hiding structs BehindTypedefsIsEvil.
Unfortunatelly PCMCIA subsystem is full of these and all drivers use them.
I'll stay consistent for now.
Sorry if they were not yours.
Updated patch v4 will follow.
Thanks!
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Thu 2008-01-31 10:29:30, Stephen Blackheath [to Foxconn] wrote:
> Pavel & all,
>
>
> Pavel Machek wrote:
>
> >> +/* I/O ports and bit definitions for version 2 of the hardware */
> >> +
> >> +struct MEMCCR {
> >> + unsigned short PCCOR; /* Configuration Option Register */
> >> + unsigned short PCCSR; /* Configuration and Status Register */
> >> + unsigned short PCPRR; /* Pin Replacemant Register */
> >> + unsigned short PCSCR; /* Socket and Copy Register */
> >> + unsigned short PCESR; /* Extendend Status Register */
> >> + unsigned short PCIOB; /* I/O Base Register */
> >> +};
> > Could we get better names? PCIOB is cryptic, pci_io_base is pretty
> > good.
> >
> >
> We should keep these names because they are part of the interface
> between host and card defined by the manufacturer.
No. Use sensible names, and put manufacturer-defined 5-letter crap in
the comments. Heck, notice that they just took first letter of each
word of good name
> > Is some locking needed around *hw?
> >
> I don't think so, but I'm happy to be corrected.
You have a structure, and are accessing its fields from interrupts. I
assume you access the fields outside interrupt, too? As the fields are
not of atomic_t, I believe you need locking.
(Oh, and I should have said that earlier: Thanks for the driver and
congratulations for getting it this far).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Pavel & all,
Pavel Machek wrote:
>> +/* I/O ports and bit definitions for version 2 of the hardware */
>> +
>> +struct MEMCCR {
>> +unsigned short PCCOR; /* Configuration Option Register */
>> +unsigned short PCCSR; /* Configuration and Status Register */
>> +unsigned short PCPRR; /* Pin Replacemant Register */
>> +unsigned short PCSCR; /* Socket and Copy Register */
>> +unsigned short PCESR; /* Extendend Status Register */
>> +unsigned short PCIOB; /* I/O Base Register */
>> +};
>>
>
> Could we get better names? PCIOB is cryptic, pci_io_base is pretty
> good.
>
>
We should keep these names because they are part of the interface
between host and card defined by the manufacturer.
>> +static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
>> + struct ipw_hardware *hw)
>> +{
>> +unsigned short irqn;
>> +unsigned short ack;
>> +
>> +irqn = inw(hw->base_port + IOIR);
>> +
>> +/* Check if card is present */
>> +if (irqn == (unsigned short) 0x) {
>> +if (++hw->bad_interrupt_count >= 100) {
>> +/*
>> + * It is necessary to disable the interrupt at this
>> + * point, or the kernel hangs, interrupting repeatedly
>> + * forever.
>> + */
>> +hw->irq = irq;
>> +hw->removed = 1;
>> +disable_irq_nosync(irq);
>> +printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
>> +": Mr. Fluffy is not happy!\n");
>> +}
>> +return IRQ_HANDLED;
>>
>
> Not sure how this is supposed to work. If you assume unshared
> interrupts, it should be possible to return something and make core
> care.
>
> If you are assuming shared interrupts, either you should disable on
> first 0x (are you sure cast is needed, btw?), or not at all,
> because it could be the other device sedning you 100 of those...
>
> ...so which one is it?
>
This code is obsolete (a workaround to an embedded system bug) and
should be removed - sorry I didn't step on it earlier. It can removed
with minimal risk of destabilizing the driver. It should look like this:
static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
ipw_hardware_t *hw)
{
u_short irqn;
u_short ack;
irqn = inw(hw->base_port + IOIR);
if (irqn == (u_short) 0x)
return IRQ_NONE;
else if (irqn != 0) {
ack = 0;
/* Transmit complete. */
if (irqn & IR_TXINTR) {
hw->tx_ready++;
ack |= IR_TXINTR;
}
/* Received data */
if (irqn & IR_RXINTR) {
ack |= IR_RXINTR;
hw->rx_ready++;
}
if (ack != 0) {
outw(ack, hw->base_port + IOIR);
/* Perform the I/O retrieval in a tasklet,
because the ppp_generic
may be called from a tasklet, but not from a
hardware interrupt. */
if (!hw->tasklet_pending) {
hw->tasklet_pending = 1;
tasklet_schedule(>tasklet);
}
}
return IRQ_HANDLED;
} else
return IRQ_NONE;
}
The v2_v3 handler should not have it either, and should start like this:
static irqreturn_t ipwireless_handle_v2_v3_interrupt(int irq,
ipw_hardware_t *hw)
{
int tx = 0;
int rx = 0;
int rx_repeat = 0;
int b_try_MemTX_OLD;
do {
u_short memtx = ioread16(hw->MemTX);
u_short memtx_serial;
u_short memrxdone = ioread16(>memInfReg->MemRXDone);
b_try_MemTX_OLD = 0;
/* check whether the interrupt was generated by ipwireless card */
if (!(memtx & MEMTX_TX) && !(memrxdone & MEMRX_RX_DONE))
return IRQ_NONE;
/* See if the card is physically present. Note that while it is
* powering up, it appears not to be present. */
if (ioread32(>memInfReg->MemCardPresent) !=
CARD_PRESENT_VALUE) {
u_short csr = ioread16(>memCCR->PCCSR);
csr &= 0xfffd;
iowrite16(csr, >memCCR->PCCSR);
return IRQ_HANDLED;
}
> Is some locking needed around *hw?
>
I don't think so, but I'm happy to be corrected.
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi!
> +/* I/O ports and bit definitions for version 1 of the hardware */
> +
> +/* IER bits*/
> +#define IER_RXENABLED ((unsigned short) 0x1)
> +#define IER_TXENABLED ((unsigned short) 0x2)
> +
> +/* ISR bits */
> +#define IR_RXINTR ((unsigned short) 0x1)
> +#define IR_TXINTR ((unsigned short) 0x2)
> +
> +/* DCR bits */
> +#define DCR_RXDONE ((unsigned short) 0x1)
> +#define DCR_TXDONE ((unsigned short) 0x2)
> +#define DCR_RXRESET ((unsigned short) 0x4)
> +#define DCR_TXRESET ((unsigned short) 0x8)
Are those casts neccessary?
> +/* I/O ports and bit definitions for version 2 of the hardware */
> +
> +struct MEMCCR {
> + unsigned short PCCOR; /* Configuration Option Register */
> + unsigned short PCCSR; /* Configuration and Status Register */
> + unsigned short PCPRR; /* Pin Replacemant Register */
> + unsigned short PCSCR; /* Socket and Copy Register */
> + unsigned short PCESR; /* Extendend Status Register */
> + unsigned short PCIOB; /* I/O Base Register */
> +};
Could we get better names? PCIOB is cryptic, pci_io_base is pretty
good.
> +/* Signals from DTE */
> +enum ComCtrl_DTESignal {
> + ComCtrl_RTS = 0,
> + ComCtrl_DTR = 1
> +};
CamelCaseIsEvil.
> +static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
> + struct ipw_hardware *hw)
> +{
> + unsigned short irqn;
> + unsigned short ack;
> +
> + irqn = inw(hw->base_port + IOIR);
> +
> + /* Check if card is present */
> + if (irqn == (unsigned short) 0x) {
> + if (++hw->bad_interrupt_count >= 100) {
> + /*
> + * It is necessary to disable the interrupt at this
> + * point, or the kernel hangs, interrupting repeatedly
> + * forever.
> + */
> + hw->irq = irq;
> + hw->removed = 1;
> + disable_irq_nosync(irq);
> + printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
> + ": Mr. Fluffy is not happy!\n");
> + }
> + return IRQ_HANDLED;
Not sure how this is supposed to work. If you assume unshared
interrupts, it should be possible to return something and make core
care.
If you are assuming shared interrupts, either you should disable on
first 0x (are you sure cast is needed, btw?), or not at all,
because it could be the other device sedning you 100 of those...
...so which one is it?
Is some locking needed around *hw?
> +int ipwireless_send_packet(struct ipw_hardware *hw, unsigned int channel_idx,
> + unsigned char *data, unsigned int length,
> + void (*callback) (void *cb, unsigned int length),
> + void *callback_data)
> +{
> + struct ipw_tx_packet *packet;
> +
> + packet = alloc_data_packet(length,
> +(unsigned char) (channel_idx + 1),
> +TL_PROTOCOLID_COM_DATA);
> + if (!packet)
> + return -1;
-ENOMEM would be more usual calling convention.
> +struct ipw_setup_get_version_query_packet {
> + struct ipw_tx_packet header;
> + struct TlSetupGetVersionQry body;
> +};
MoreEvilCamelCase.
> +static int config_ipwireless(struct ipw_dev *ipw)
> +{
> + struct pcmcia_device *link = ipw->link;
> + int ret;
> + config_info_t conf;
> + tuple_t tuple;
> + unsigned short buf[64];
> + cisparse_t parse;
> + unsigned short cor_value;
> + win_req_t reqAM;
> + win_req_t reqCM;
Hiding structs BehindTypedefsIsEvil.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Mon 2008-01-28 20:53:57, Alexey Dobriyan wrote:
> On Mon, Jan 28, 2008 at 06:19:29PM +0100, David Sterba wrote:
> > ipwireless: driver for PC Card, 3G internet connection
>
> > +struct nl_first_paket_header {
> > +#if defined(__BIG_ENDIAN)
> > + unsigned char packet_rank:2;
> > + unsigned char address:3;
> > + unsigned char protocol:3;
> > +#else
> > + unsigned char protocol:3;
> > + unsigned char address:3;
> > + unsigned char packet_rank:2;
> > +#endif
> > + unsigned char length_lsb;
> > + unsigned char length_msb;
> > +};
> > +
> > +struct nl_packet_header {
> > +#if defined(__BIG_ENDIAN)
> > + unsigned char packet_rank:2;
> > + unsigned char address:3;
> > + unsigned char protocol:3;
> > +#else
> > + unsigned char protocol:3;
> > + unsigned char address:3;
> > + unsigned char packet_rank:2;
> > +#endif
> > +};
>
> You want __BIG_ENDIAN_BITFIELD here.
Actually, you probably want to avoid bitfields here, and just do bit
arithmetics by hand.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Mon 2008-01-28 20:53:57, Alexey Dobriyan wrote:
On Mon, Jan 28, 2008 at 06:19:29PM +0100, David Sterba wrote:
ipwireless: driver for PC Card, 3G internet connection
+struct nl_first_paket_header {
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char address:3;
+ unsigned char packet_rank:2;
+#endif
+ unsigned char length_lsb;
+ unsigned char length_msb;
+};
+
+struct nl_packet_header {
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char address:3;
+ unsigned char packet_rank:2;
+#endif
+};
You want __BIG_ENDIAN_BITFIELD here.
Actually, you probably want to avoid bitfields here, and just do bit
arithmetics by hand.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi!
+/* I/O ports and bit definitions for version 1 of the hardware */
+
+/* IER bits*/
+#define IER_RXENABLED ((unsigned short) 0x1)
+#define IER_TXENABLED ((unsigned short) 0x2)
+
+/* ISR bits */
+#define IR_RXINTR ((unsigned short) 0x1)
+#define IR_TXINTR ((unsigned short) 0x2)
+
+/* DCR bits */
+#define DCR_RXDONE ((unsigned short) 0x1)
+#define DCR_TXDONE ((unsigned short) 0x2)
+#define DCR_RXRESET ((unsigned short) 0x4)
+#define DCR_TXRESET ((unsigned short) 0x8)
Are those casts neccessary?
+/* I/O ports and bit definitions for version 2 of the hardware */
+
+struct MEMCCR {
+ unsigned short PCCOR; /* Configuration Option Register */
+ unsigned short PCCSR; /* Configuration and Status Register */
+ unsigned short PCPRR; /* Pin Replacemant Register */
+ unsigned short PCSCR; /* Socket and Copy Register */
+ unsigned short PCESR; /* Extendend Status Register */
+ unsigned short PCIOB; /* I/O Base Register */
+};
Could we get better names? PCIOB is cryptic, pci_io_base is pretty
good.
+/* Signals from DTE */
+enum ComCtrl_DTESignal {
+ ComCtrl_RTS = 0,
+ ComCtrl_DTR = 1
+};
CamelCaseIsEvil.
+static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
+ struct ipw_hardware *hw)
+{
+ unsigned short irqn;
+ unsigned short ack;
+
+ irqn = inw(hw-base_port + IOIR);
+
+ /* Check if card is present */
+ if (irqn == (unsigned short) 0x) {
+ if (++hw-bad_interrupt_count = 100) {
+ /*
+ * It is necessary to disable the interrupt at this
+ * point, or the kernel hangs, interrupting repeatedly
+ * forever.
+ */
+ hw-irq = irq;
+ hw-removed = 1;
+ disable_irq_nosync(irq);
+ printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
+ : Mr. Fluffy is not happy!\n);
+ }
+ return IRQ_HANDLED;
Not sure how this is supposed to work. If you assume unshared
interrupts, it should be possible to return something and make core
care.
If you are assuming shared interrupts, either you should disable on
first 0x (are you sure cast is needed, btw?), or not at all,
because it could be the other device sedning you 100 of those...
...so which one is it?
Is some locking needed around *hw?
+int ipwireless_send_packet(struct ipw_hardware *hw, unsigned int channel_idx,
+ unsigned char *data, unsigned int length,
+ void (*callback) (void *cb, unsigned int length),
+ void *callback_data)
+{
+ struct ipw_tx_packet *packet;
+
+ packet = alloc_data_packet(length,
+(unsigned char) (channel_idx + 1),
+TL_PROTOCOLID_COM_DATA);
+ if (!packet)
+ return -1;
-ENOMEM would be more usual calling convention.
+struct ipw_setup_get_version_query_packet {
+ struct ipw_tx_packet header;
+ struct TlSetupGetVersionQry body;
+};
MoreEvilCamelCase.
+static int config_ipwireless(struct ipw_dev *ipw)
+{
+ struct pcmcia_device *link = ipw-link;
+ int ret;
+ config_info_t conf;
+ tuple_t tuple;
+ unsigned short buf[64];
+ cisparse_t parse;
+ unsigned short cor_value;
+ win_req_t reqAM;
+ win_req_t reqCM;
Hiding structs BehindTypedefsIsEvil.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Pavel all,
Pavel Machek wrote:
+/* I/O ports and bit definitions for version 2 of the hardware */
+
+struct MEMCCR {
+unsigned short PCCOR; /* Configuration Option Register */
+unsigned short PCCSR; /* Configuration and Status Register */
+unsigned short PCPRR; /* Pin Replacemant Register */
+unsigned short PCSCR; /* Socket and Copy Register */
+unsigned short PCESR; /* Extendend Status Register */
+unsigned short PCIOB; /* I/O Base Register */
+};
Could we get better names? PCIOB is cryptic, pci_io_base is pretty
good.
We should keep these names because they are part of the interface
between host and card defined by the manufacturer.
+static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
+ struct ipw_hardware *hw)
+{
+unsigned short irqn;
+unsigned short ack;
+
+irqn = inw(hw-base_port + IOIR);
+
+/* Check if card is present */
+if (irqn == (unsigned short) 0x) {
+if (++hw-bad_interrupt_count = 100) {
+/*
+ * It is necessary to disable the interrupt at this
+ * point, or the kernel hangs, interrupting repeatedly
+ * forever.
+ */
+hw-irq = irq;
+hw-removed = 1;
+disable_irq_nosync(irq);
+printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
+: Mr. Fluffy is not happy!\n);
+}
+return IRQ_HANDLED;
Not sure how this is supposed to work. If you assume unshared
interrupts, it should be possible to return something and make core
care.
If you are assuming shared interrupts, either you should disable on
first 0x (are you sure cast is needed, btw?), or not at all,
because it could be the other device sedning you 100 of those...
...so which one is it?
This code is obsolete (a workaround to an embedded system bug) and
should be removed - sorry I didn't step on it earlier. It can removed
with minimal risk of destabilizing the driver. It should look like this:
static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
ipw_hardware_t *hw)
{
u_short irqn;
u_short ack;
irqn = inw(hw-base_port + IOIR);
if (irqn == (u_short) 0x)
return IRQ_NONE;
else if (irqn != 0) {
ack = 0;
/* Transmit complete. */
if (irqn IR_TXINTR) {
hw-tx_ready++;
ack |= IR_TXINTR;
}
/* Received data */
if (irqn IR_RXINTR) {
ack |= IR_RXINTR;
hw-rx_ready++;
}
if (ack != 0) {
outw(ack, hw-base_port + IOIR);
/* Perform the I/O retrieval in a tasklet,
because the ppp_generic
may be called from a tasklet, but not from a
hardware interrupt. */
if (!hw-tasklet_pending) {
hw-tasklet_pending = 1;
tasklet_schedule(hw-tasklet);
}
}
return IRQ_HANDLED;
} else
return IRQ_NONE;
}
The v2_v3 handler should not have it either, and should start like this:
static irqreturn_t ipwireless_handle_v2_v3_interrupt(int irq,
ipw_hardware_t *hw)
{
int tx = 0;
int rx = 0;
int rx_repeat = 0;
int b_try_MemTX_OLD;
do {
u_short memtx = ioread16(hw-MemTX);
u_short memtx_serial;
u_short memrxdone = ioread16(hw-memInfReg-MemRXDone);
b_try_MemTX_OLD = 0;
/* check whether the interrupt was generated by ipwireless card */
if (!(memtx MEMTX_TX) !(memrxdone MEMRX_RX_DONE))
return IRQ_NONE;
/* See if the card is physically present. Note that while it is
* powering up, it appears not to be present. */
if (ioread32(hw-memInfReg-MemCardPresent) !=
CARD_PRESENT_VALUE) {
u_short csr = ioread16(hw-memCCR-PCCSR);
csr = 0xfffd;
iowrite16(csr, hw-memCCR-PCCSR);
return IRQ_HANDLED;
}
Is some locking needed around *hw?
I don't think so, but I'm happy to be corrected.
Steve
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Thu 2008-01-31 10:29:30, Stephen Blackheath [to Foxconn] wrote:
Pavel all,
Pavel Machek wrote:
+/* I/O ports and bit definitions for version 2 of the hardware */
+
+struct MEMCCR {
+ unsigned short PCCOR; /* Configuration Option Register */
+ unsigned short PCCSR; /* Configuration and Status Register */
+ unsigned short PCPRR; /* Pin Replacemant Register */
+ unsigned short PCSCR; /* Socket and Copy Register */
+ unsigned short PCESR; /* Extendend Status Register */
+ unsigned short PCIOB; /* I/O Base Register */
+};
Could we get better names? PCIOB is cryptic, pci_io_base is pretty
good.
We should keep these names because they are part of the interface
between host and card defined by the manufacturer.
No. Use sensible names, and put manufacturer-defined 5-letter crap in
the comments. Heck, notice that they just took first letter of each
word of good name
Is some locking needed around *hw?
I don't think so, but I'm happy to be corrected.
You have a structure, and are accessing its fields from interrupts. I
assume you access the fields outside interrupt, too? As the fields are
not of atomic_t, I believe you need locking.
(Oh, and I should have said that earlier: Thanks for the driver and
congratulations for getting it this far).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Tuesday 29 of January 2008 00:18:03 Jiri Slaby wrote: > On 01/28/2008 07:08 PM, Randy Dunlap wrote: > >> +/* > >> + * @return 1 if something has been received from hw > > > > What's with the '@'? > > ... > > >> + > >> +/*! > > > > and the '!' ? > > > >> + * @return true if the card is physically present. > >> + */ > > What exactly is wrong with @ and !? It's perfectly doxygen-styled. Well, I've removed them, this was not consistent neither within the driver nor with kernel documenting style. > >> diff --git a/drivers/char/pcmcia/ipwireless/main.c > >> b/drivers/char/pcmcia/ipwireless/main.c new file mode 100644 > >> index 000..cab5722 > >> --- /dev/null > >> +++ b/drivers/char/pcmcia/ipwireless/main.c > >> @@ -0,0 +1,496 @@ > > [...] > > >> +module_param(major, int, 0); > >> +module_param(ipwireless_debug, int, 0); > >> +module_param(ipwireless_loopback, int, 0); > >> +module_param(ipwireless_out_queue, int, 0); > >> +MODULE_PARM_DESC(major, "ttyIPWp major number [0]"); > >> +MODULE_PARM_DESC(ipwireless_debug, "switch on debug messages [0]"); > >> +MODULE_PARM_DESC(ipwireless_debug, "switch on loopback mode [0]"); > >> +MODULE_PARM_DESC(ipwireless_debug, "set size of outgoing queue [1]"); > > > > Will these parameters be documented anywhere? > > Anyway the descs are wrong. Those 3 are for the only one variable. Yes this was my copy error, thanks. Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Monday 28 of January 2008 19:08:54 Randy Dunlap wrote:
> > +/* Number of bytes in NL packet header (can not do
>
> cannot
Fixed.
>
> > + * sizeof(nl_packet_header) since it's a bitfield) */
> > +#define NL_FOLLOWING_PACKET_HEADER_SIZE1
> > +
> > +struct nl_first_paket_header {
>
>packet ?
Yes, packet of course.
>
> > +#if defined(__BIG_ENDIAN)
> > + unsigned char packet_rank:2;
> > + unsigned char address:3;
> > + unsigned char protocol:3;
> > +#else
> > + unsigned char protocol:3;
> > + unsigned char address:3;
> > + unsigned char packet_rank:2;
> > +#endif
>
> From C99 spec:
> "The order of allocation of bit-fields within a unit (high-order to
> low-order or low-order to high-order) is implementation-defined."
>
> so if the order/location of these bitfields is important (from one
> system to another), you should use bit masks instead of bitfields
> for them.
I've changed it to __BIG_ENDIAN_BITFIELDS, as suggested by Alexey. The order
is important. I'll add it to my todo to convert it to bitmasks.
> > +/*
> > + * @return 1 if something has been received from hw
>
> What's with the '@'?
Removed, and converted to plaintext as suggested by others.
> Need ":" and/or space between CARD_NAME and following string.
> (in several places)
Found a few of them and fixed.
> > +module_param(major, int, 0);
> > +module_param(ipwireless_debug, int, 0);
> > +module_param(ipwireless_loopback, int, 0);
> > +module_param(ipwireless_out_queue, int, 0);
> > +MODULE_PARM_DESC(major, "ttyIPWp major number [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "switch on debug messages [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "switch on loopback mode [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "set size of outgoing queue [1]");
>
> Will these parameters be documented anywhere?
The options are intended for debugging. Normal user does not need to tweak
them. Adjusted the description to reflect their debugging purpose.
> > +#ifdef IPWIRELESS_STATE_DEBUG
> > +int ipwireless_dump_network_state(char *p, struct ipw_network *network)
> > +{
> > + int idx = 0;
> > +
> > + idx += sprintf(p + idx, "debug: ppp_blocked=%d\n",
> > + network->ppp_blocked);
> > + idx += sprintf(p + idx, "debug: outgoing_packets_queued=%d\n",
> > + network->outgoing_packets_queued);
> > + idx += sprintf(p + idx, "debug: network.shutting_down=%d\n",
> > + network->shutting_down);
>
> check for overflow of 'p'?
Converted to one snprintf too.
> > +/* Syncronous start-messages */
>
> Synchronous
Fixed.
Updated patch will follow.
Thank you for comments.
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Tuesday 29 of January 2008 00:18:03 Jiri Slaby wrote: On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. Well, I've removed them, this was not consistent neither within the driver nor with kernel documenting style. diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c new file mode 100644 index 000..cab5722 --- /dev/null +++ b/drivers/char/pcmcia/ipwireless/main.c @@ -0,0 +1,496 @@ [...] +module_param(major, int, 0); +module_param(ipwireless_debug, int, 0); +module_param(ipwireless_loopback, int, 0); +module_param(ipwireless_out_queue, int, 0); +MODULE_PARM_DESC(major, ttyIPWp major number [0]); +MODULE_PARM_DESC(ipwireless_debug, switch on debug messages [0]); +MODULE_PARM_DESC(ipwireless_debug, switch on loopback mode [0]); +MODULE_PARM_DESC(ipwireless_debug, set size of outgoing queue [1]); Will these parameters be documented anywhere? Anyway the descs are wrong. Those 3 are for the only one variable. Yes this was my copypaste error, thanks. Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Monday 28 of January 2008 19:08:54 Randy Dunlap wrote:
+/* Number of bytes in NL packet header (can not do
cannot
Fixed.
+ * sizeof(nl_packet_header) since it's a bitfield) */
+#define NL_FOLLOWING_PACKET_HEADER_SIZE1
+
+struct nl_first_paket_header {
packet ?
Yes, packet of course.
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char address:3;
+ unsigned char packet_rank:2;
+#endif
From C99 spec:
The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined.
so if the order/location of these bitfields is important (from one
system to another), you should use bit masks instead of bitfields
for them.
I've changed it to __BIG_ENDIAN_BITFIELDS, as suggested by Alexey. The order
is important. I'll add it to my todo to convert it to bitmasks.
+/*
+ * @return 1 if something has been received from hw
What's with the '@'?
Removed, and converted to plaintext as suggested by others.
Need : and/or space between CARD_NAME and following string.
(in several places)
Found a few of them and fixed.
+module_param(major, int, 0);
+module_param(ipwireless_debug, int, 0);
+module_param(ipwireless_loopback, int, 0);
+module_param(ipwireless_out_queue, int, 0);
+MODULE_PARM_DESC(major, ttyIPWp major number [0]);
+MODULE_PARM_DESC(ipwireless_debug, switch on debug messages [0]);
+MODULE_PARM_DESC(ipwireless_debug, switch on loopback mode [0]);
+MODULE_PARM_DESC(ipwireless_debug, set size of outgoing queue [1]);
Will these parameters be documented anywhere?
The options are intended for debugging. Normal user does not need to tweak
them. Adjusted the description to reflect their debugging purpose.
+#ifdef IPWIRELESS_STATE_DEBUG
+int ipwireless_dump_network_state(char *p, struct ipw_network *network)
+{
+ int idx = 0;
+
+ idx += sprintf(p + idx, debug: ppp_blocked=%d\n,
+ network-ppp_blocked);
+ idx += sprintf(p + idx, debug: outgoing_packets_queued=%d\n,
+ network-outgoing_packets_queued);
+ idx += sprintf(p + idx, debug: network.shutting_down=%d\n,
+ network-shutting_down);
check for overflow of 'p'?
Converted to one snprintf too.
+/* Syncronous start-messages */
Synchronous
Fixed.
Updated patch will follow.
Thank you for comments.
Dave
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi, On Jan 29, 2008 1:33 AM, Randy Dunlap <[EMAIL PROTECTED]> wrote: > > > What part of kernel documentation uses doxygen? > > > > So then, what's the problem? > > Why is it there? We have a kernel documentation language. > Please use it or plain text. Yes please. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Tue, 29 Jan 2008 00:29:19 +0100 Jiri Slaby wrote: > On 01/29/2008 12:28 AM, Randy Dunlap wrote: > > On Tue, 29 Jan 2008 00:18:03 +0100 Jiri Slaby wrote: > > > >> On 01/28/2008 07:08 PM, Randy Dunlap wrote: > +/* > + * @return 1 if something has been received from hw > >>> What's with the '@'? > >> ... > + > +/*! > >>> and the '!' ? > >>> > + * @return true if the card is physically present. > + */ > >> What exactly is wrong with @ and !? It's perfectly doxygen-styled. > > > > What part of kernel documentation uses doxygen? > > So then, what's the problem? Why is it there? We have a kernel documentation language. Please use it or plain text. --- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On 01/29/2008 12:28 AM, Randy Dunlap wrote: On Tue, 29 Jan 2008 00:18:03 +0100 Jiri Slaby wrote: On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. What part of kernel documentation uses doxygen? So then, what's the problem? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Tue, 29 Jan 2008 00:18:03 +0100 Jiri Slaby wrote: > On 01/28/2008 07:08 PM, Randy Dunlap wrote: > >> +/* > >> + * @return 1 if something has been received from hw > > > > What's with the '@'? > ... > >> + > >> +/*! > > > > and the '!' ? > > > >> + * @return true if the card is physically present. > >> + */ > > What exactly is wrong with @ and !? It's perfectly doxygen-styled. What part of kernel documentation uses doxygen? > >> diff --git a/drivers/char/pcmcia/ipwireless/main.c > >> b/drivers/char/pcmcia/ipwireless/main.c > >> new file mode 100644 > >> index 000..cab5722 > >> --- /dev/null > >> +++ b/drivers/char/pcmcia/ipwireless/main.c > >> @@ -0,0 +1,496 @@ > [...] > >> +module_param(major, int, 0); > >> +module_param(ipwireless_debug, int, 0); > >> +module_param(ipwireless_loopback, int, 0); > >> +module_param(ipwireless_out_queue, int, 0); > >> +MODULE_PARM_DESC(major, "ttyIPWp major number [0]"); > >> +MODULE_PARM_DESC(ipwireless_debug, "switch on debug messages [0]"); > >> +MODULE_PARM_DESC(ipwireless_debug, "switch on loopback mode [0]"); > >> +MODULE_PARM_DESC(ipwireless_debug, "set size of outgoing queue [1]"); > > > > Will these parameters be documented anywhere? > > Anyway the descs are wrong. Those 3 are for the only one variable. Ack, thanks. --- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c new file mode 100644 index 000..cab5722 --- /dev/null +++ b/drivers/char/pcmcia/ipwireless/main.c @@ -0,0 +1,496 @@ [...] +module_param(major, int, 0); +module_param(ipwireless_debug, int, 0); +module_param(ipwireless_loopback, int, 0); +module_param(ipwireless_out_queue, int, 0); +MODULE_PARM_DESC(major, "ttyIPWp major number [0]"); +MODULE_PARM_DESC(ipwireless_debug, "switch on debug messages [0]"); +MODULE_PARM_DESC(ipwireless_debug, "switch on loopback mode [0]"); +MODULE_PARM_DESC(ipwireless_debug, "set size of outgoing queue [1]"); Will these parameters be documented anywhere? Anyway the descs are wrong. Those 3 are for the only one variable. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Mon, 28 Jan 2008 18:19:29 +0100 David Sterba wrote:
[resending due to send problems, sorry about any dups]
> Hi Linus,
>
> I'm submitting driver for IPWireless PC Card modem for inclusion to 2.6.25.
>
> The driver has been in -mm series as ipwireless_cs.git tree for
> some time and has passed through lkml (http://lkml.org/lkml/2007/12/12/165).
> The PCMCIA subsystem is unmaintained, so I'm sending it directly as Andrew
> suggested.
>
> David Sterba
> ---
> From: David Sterba <[EMAIL PROTECTED]>
>
> ipwireless: driver for PC Card, 3G internet connection
>
> The driver is manufactured by IPWireless.
>
> Rewieved-by: Jiri Slaby <[EMAIL PROTECTED]>
> Signed-off-by: Ben Martel <[EMAIL PROTECTED]>
> Signed-off-by: Stephen Blackheath <[EMAIL PROTECTED]>
> Signed-off-by: David Sterba <[EMAIL PROTECTED]>
> Signed-off-by: Jiri Kosina <[EMAIL PROTECTED]>
> ---
> MAINTAINERS |8
> drivers/char/pcmcia/Kconfig |9
> drivers/char/pcmcia/Makefile|2
> drivers/char/pcmcia/ipwireless/Makefile | 10
> drivers/char/pcmcia/ipwireless/hardware.c | 1784
>
> drivers/char/pcmcia/ipwireless/hardware.h | 63
> drivers/char/pcmcia/ipwireless/main.c | 496 ++
> drivers/char/pcmcia/ipwireless/main.h | 70
> drivers/char/pcmcia/ipwireless/network.c| 513 ++
> drivers/char/pcmcia/ipwireless/network.h| 54
> drivers/char/pcmcia/ipwireless/setup_protocol.h | 108 +
> drivers/char/pcmcia/ipwireless/tty.c| 687 +
> drivers/char/pcmcia/ipwireless/tty.h| 48
> 13 files changed, 3852 insertions(+)
> ---
> diff --git a/drivers/char/pcmcia/ipwireless/hardware.c
> b/drivers/char/pcmcia/ipwireless/hardware.c
> new file mode 100644
> index 000..de31f48
> --- /dev/null
> +++ b/drivers/char/pcmcia/ipwireless/hardware.c
> @@ -0,0 +1,1784 @@
> +/*
> + * IPWireless 3G PCMCIA Network Driver
> + *
> + * Original code
> + * by Stephen Blackheath <[EMAIL PROTECTED]>,
> + * Ben Martel <[EMAIL PROTECTED]>
> + *
> + * Copyrighted as follows:
> + * Copyright (C) 2004 by Symmetric Systems Ltd (NZ)
> + *
> + * Various driver changes and rewrites, port to new kernels
> + * Copyright (C) 2006-2007 Jiri Kosina
> + *
> + * Misc code cleanups and updates
> + * Copyright (C) 2007 David Sterba
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "hardware.h"
> +#include "setup_protocol.h"
> +#include "network.h"
> +#include "main.h"
> +
> +/* Function prototypes */
> +static void ipw_send_setup_packet(struct ipw_hardware *hw);
> +static void handle_received_SETUP_packet(struct ipw_hardware *ipw,
> + unsigned int address,
> + unsigned char *data, int len,
> + int is_last);
> +static void ipwireless_setup_timer(unsigned long data);
> +static void handle_received_CTRL_packet(struct ipw_hardware *hw,
> + unsigned int channel_idx, unsigned char *data, int len);
> +
> +/*#define TIMING_DIAGNOSTICS*/
> +
> +#ifdef TIMING_DIAGNOSTICS
> +
> +static struct timing_stats {
> + unsigned long last_report_time;
> + unsigned long read_time;
> + unsigned long write_time;
> + unsigned long read_bytes;
> + unsigned long write_bytes;
> + unsigned long start_time;
> +};
> +
...
> +/* Protocol ids */
> +enum {
> + /* Identifier for the Com Data protocol */
> + TL_PROTOCOLID_COM_DATA = 0,
> +
> + /* Identifier for the Com Control protocol */
> + TL_PROTOCOLID_COM_CTRL = 1,
> +
> + /* Identifier for the Setup protocol */
> + TL_PROTOCOLID_SETUP = 2
> +};
> +
> +/* Number of bytes in NL packet header (can not do
cannot
> + * sizeof(nl_packet_header) since it's a bitfield) */
> +#define NL_FIRST_PACKET_HEADER_SIZE3
> +
> +/* Number of bytes in NL packet header (can not do
cannot
> + * sizeof(nl_packet_header) since it's a bitfield) */
> +#define NL_FOLLOWING_PACKET_HEADER_SIZE1
> +
> +struct nl_first_paket_header {
packet ?
> +#if defined(__BIG_ENDIAN)
> + unsigned char packet_rank:2;
> + unsigned char address:3;
> + unsigned char protocol:3;
> +#else
> + unsigned char protocol:3;
> + unsigned char address:3;
> + unsigned char packet_rank:2;
> +#endif
>From C99 spec:
"The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined."
so if the order/location of these bitfields is important (from one
system to another), you should use bit masks instead of bitfields
for them.
> + unsigned char length_lsb;
> + unsigned char length_msb;
> +};
> +
> +struct nl_packet_header {
> +#if defined(__BIG_ENDIAN)
> + unsigned char packet_rank:2;
> + unsigned char address:3;
> +
Re: [PATCH] ipwireless: driver for 3G PC Card
On Mon, Jan 28, 2008 at 06:19:29PM +0100, David Sterba wrote:
> ipwireless: driver for PC Card, 3G internet connection
> +struct nl_first_paket_header {
> +#if defined(__BIG_ENDIAN)
> + unsigned char packet_rank:2;
> + unsigned char address:3;
> + unsigned char protocol:3;
> +#else
> + unsigned char protocol:3;
> + unsigned char address:3;
> + unsigned char packet_rank:2;
> +#endif
> + unsigned char length_lsb;
> + unsigned char length_msb;
> +};
> +
> +struct nl_packet_header {
> +#if defined(__BIG_ENDIAN)
> + unsigned char packet_rank:2;
> + unsigned char address:3;
> + unsigned char protocol:3;
> +#else
> + unsigned char protocol:3;
> + unsigned char address:3;
> + unsigned char packet_rank:2;
> +#endif
> +};
You want __BIG_ENDIAN_BITFIELD here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Mon, Jan 28, 2008 at 06:19:29PM +0100, David Sterba wrote:
ipwireless: driver for PC Card, 3G internet connection
+struct nl_first_paket_header {
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char address:3;
+ unsigned char packet_rank:2;
+#endif
+ unsigned char length_lsb;
+ unsigned char length_msb;
+};
+
+struct nl_packet_header {
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char address:3;
+ unsigned char packet_rank:2;
+#endif
+};
You want __BIG_ENDIAN_BITFIELD here.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c new file mode 100644 index 000..cab5722 --- /dev/null +++ b/drivers/char/pcmcia/ipwireless/main.c @@ -0,0 +1,496 @@ [...] +module_param(major, int, 0); +module_param(ipwireless_debug, int, 0); +module_param(ipwireless_loopback, int, 0); +module_param(ipwireless_out_queue, int, 0); +MODULE_PARM_DESC(major, ttyIPWp major number [0]); +MODULE_PARM_DESC(ipwireless_debug, switch on debug messages [0]); +MODULE_PARM_DESC(ipwireless_debug, switch on loopback mode [0]); +MODULE_PARM_DESC(ipwireless_debug, set size of outgoing queue [1]); Will these parameters be documented anywhere? Anyway the descs are wrong. Those 3 are for the only one variable. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Tue, 29 Jan 2008 00:18:03 +0100 Jiri Slaby wrote: On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. What part of kernel documentation uses doxygen? diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c new file mode 100644 index 000..cab5722 --- /dev/null +++ b/drivers/char/pcmcia/ipwireless/main.c @@ -0,0 +1,496 @@ [...] +module_param(major, int, 0); +module_param(ipwireless_debug, int, 0); +module_param(ipwireless_loopback, int, 0); +module_param(ipwireless_out_queue, int, 0); +MODULE_PARM_DESC(major, ttyIPWp major number [0]); +MODULE_PARM_DESC(ipwireless_debug, switch on debug messages [0]); +MODULE_PARM_DESC(ipwireless_debug, switch on loopback mode [0]); +MODULE_PARM_DESC(ipwireless_debug, set size of outgoing queue [1]); Will these parameters be documented anywhere? Anyway the descs are wrong. Those 3 are for the only one variable. Ack, thanks. --- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On 01/29/2008 12:28 AM, Randy Dunlap wrote: On Tue, 29 Jan 2008 00:18:03 +0100 Jiri Slaby wrote: On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. What part of kernel documentation uses doxygen? So then, what's the problem? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Tue, 29 Jan 2008 00:29:19 +0100 Jiri Slaby wrote: On 01/29/2008 12:28 AM, Randy Dunlap wrote: On Tue, 29 Jan 2008 00:18:03 +0100 Jiri Slaby wrote: On 01/28/2008 07:08 PM, Randy Dunlap wrote: +/* + * @return 1 if something has been received from hw What's with the '@'? ... + +/*! and the '!' ? + * @return true if the card is physically present. + */ What exactly is wrong with @ and !? It's perfectly doxygen-styled. What part of kernel documentation uses doxygen? So then, what's the problem? Why is it there? We have a kernel documentation language. Please use it or plain text. --- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
Hi, On Jan 29, 2008 1:33 AM, Randy Dunlap [EMAIL PROTECTED] wrote: What part of kernel documentation uses doxygen? So then, what's the problem? Why is it there? We have a kernel documentation language. Please use it or plain text. Yes please. Pekka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipwireless: driver for 3G PC Card
On Mon, 28 Jan 2008 18:19:29 +0100 David Sterba wrote:
[resending due to send problems, sorry about any dups]
Hi Linus,
I'm submitting driver for IPWireless PC Card modem for inclusion to 2.6.25.
The driver has been in -mm series as ipwireless_cs.git tree for
some time and has passed through lkml (http://lkml.org/lkml/2007/12/12/165).
The PCMCIA subsystem is unmaintained, so I'm sending it directly as Andrew
suggested.
David Sterba
---
From: David Sterba [EMAIL PROTECTED]
ipwireless: driver for PC Card, 3G internet connection
The driver is manufactured by IPWireless.
Rewieved-by: Jiri Slaby [EMAIL PROTECTED]
Signed-off-by: Ben Martel [EMAIL PROTECTED]
Signed-off-by: Stephen Blackheath [EMAIL PROTECTED]
Signed-off-by: David Sterba [EMAIL PROTECTED]
Signed-off-by: Jiri Kosina [EMAIL PROTECTED]
---
MAINTAINERS |8
drivers/char/pcmcia/Kconfig |9
drivers/char/pcmcia/Makefile|2
drivers/char/pcmcia/ipwireless/Makefile | 10
drivers/char/pcmcia/ipwireless/hardware.c | 1784
drivers/char/pcmcia/ipwireless/hardware.h | 63
drivers/char/pcmcia/ipwireless/main.c | 496 ++
drivers/char/pcmcia/ipwireless/main.h | 70
drivers/char/pcmcia/ipwireless/network.c| 513 ++
drivers/char/pcmcia/ipwireless/network.h| 54
drivers/char/pcmcia/ipwireless/setup_protocol.h | 108 +
drivers/char/pcmcia/ipwireless/tty.c| 687 +
drivers/char/pcmcia/ipwireless/tty.h| 48
13 files changed, 3852 insertions(+)
---
diff --git a/drivers/char/pcmcia/ipwireless/hardware.c
b/drivers/char/pcmcia/ipwireless/hardware.c
new file mode 100644
index 000..de31f48
--- /dev/null
+++ b/drivers/char/pcmcia/ipwireless/hardware.c
@@ -0,0 +1,1784 @@
+/*
+ * IPWireless 3G PCMCIA Network Driver
+ *
+ * Original code
+ * by Stephen Blackheath [EMAIL PROTECTED],
+ * Ben Martel [EMAIL PROTECTED]
+ *
+ * Copyrighted as follows:
+ * Copyright (C) 2004 by Symmetric Systems Ltd (NZ)
+ *
+ * Various driver changes and rewrites, port to new kernels
+ * Copyright (C) 2006-2007 Jiri Kosina
+ *
+ * Misc code cleanups and updates
+ * Copyright (C) 2007 David Sterba
+ */
+
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/irq.h
+#include linux/kernel.h
+#include linux/list.h
+#include linux/slab.h
+
+#include hardware.h
+#include setup_protocol.h
+#include network.h
+#include main.h
+
+/* Function prototypes */
+static void ipw_send_setup_packet(struct ipw_hardware *hw);
+static void handle_received_SETUP_packet(struct ipw_hardware *ipw,
+ unsigned int address,
+ unsigned char *data, int len,
+ int is_last);
+static void ipwireless_setup_timer(unsigned long data);
+static void handle_received_CTRL_packet(struct ipw_hardware *hw,
+ unsigned int channel_idx, unsigned char *data, int len);
+
+/*#define TIMING_DIAGNOSTICS*/
+
+#ifdef TIMING_DIAGNOSTICS
+
+static struct timing_stats {
+ unsigned long last_report_time;
+ unsigned long read_time;
+ unsigned long write_time;
+ unsigned long read_bytes;
+ unsigned long write_bytes;
+ unsigned long start_time;
+};
+
...
+/* Protocol ids */
+enum {
+ /* Identifier for the Com Data protocol */
+ TL_PROTOCOLID_COM_DATA = 0,
+
+ /* Identifier for the Com Control protocol */
+ TL_PROTOCOLID_COM_CTRL = 1,
+
+ /* Identifier for the Setup protocol */
+ TL_PROTOCOLID_SETUP = 2
+};
+
+/* Number of bytes in NL packet header (can not do
cannot
+ * sizeof(nl_packet_header) since it's a bitfield) */
+#define NL_FIRST_PACKET_HEADER_SIZE3
+
+/* Number of bytes in NL packet header (can not do
cannot
+ * sizeof(nl_packet_header) since it's a bitfield) */
+#define NL_FOLLOWING_PACKET_HEADER_SIZE1
+
+struct nl_first_paket_header {
packet ?
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char address:3;
+ unsigned char packet_rank:2;
+#endif
From C99 spec:
The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined.
so if the order/location of these bitfields is important (from one
system to another), you should use bit masks instead of bitfields
for them.
+ unsigned char length_lsb;
+ unsigned char length_msb;
+};
+
+struct nl_packet_header {
+#if defined(__BIG_ENDIAN)
+ unsigned char packet_rank:2;
+ unsigned char address:3;
+ unsigned char protocol:3;
+#else
+ unsigned char protocol:3;
+ unsigned char
