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) 0xFFFF) {
+ 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 0xFFFF (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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/