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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
