Messages in this thread | | | From | David Sterba <> | Subject | Re: [PATCH] ipwireless: driver for 3G PC Card | Date | Tue, 29 Jan 2008 14:40:05 +0100 |
| |
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_SIZE 1 > > + > > +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
| |