[parisc-linux] The final patch for the keyboard problem

[Richard Hirst]

Hi Thimas,

On Tue, Aug 07, 2001 at 09:27:41AM +0200, Thomas Marteau wrote:

> +static int cmd_status;

Calling this awaiting_ack, and setting it true/false might make the code
more readable.  Also I suspect it should be volatile really, as it is
modified via an interrupt handler.

> +	cmd_status=2;
> +	while (cmd_status!=0) {
> +		write_output(KBD_CMD_SET_LEDS, lasikbd_hpa);
> +		mdelay(1); 
> +	}

Nasty, as it will hang the kernel if the keyboard never ACKs.  Should at
least limit the muber of times you loop here.

> +		while ((read_status(hpa) & LASI_STAT_TBNE)==0x01);

Why not simply

		while ((read_status(hpa) & LASI_STAT_TBNE))
			;

> +	do {
> +		while ((read_status(hpa) & LASI_STAT_TBNE)==0x01);
> +		gsc_writeb(0x00, hpa+LASI_XMTDATA);
> +		
> +		while ((read_status(hpa) & LASI_STAT_RBNE)==0x00);
> +   
> +		while ((read_status(hpa) & LASI_STAT_RBNE)==0x01) {
> +			data=read_input(hpa);
> +			fail++; 
> +			if(fail==10) break; 
> +		}
> +   	}while(data!=AUX_REPLY_ACK); 

Do you really want the third 'while' loop in that block of code?

	loop = 10;
	do {
		while ((read_status(hpa) & LASI_STAT_TBNE))
			;
		gsc_writeb(0x00, hpa+LASI_XMTDATA);
		while (!(read_status(hpa) & LASI_STAT_RBNE))
			;
		data = read_input(hpa);
		loop--;
	} while (data != AUX_REPLY_ACK && loop);

One less while loop, but it can still hang in either of the first two while
loops if the h/w misbehaves.

> +        /* allocate the irq and memory region for that device */
> +	if (!(irq = busdevice_alloc_irq(d)))
> +		return -ENODEV;
> +		    
> +	if (request_irq(irq, lasikbd_interrupt, 0, name, hpa))
> +		return -ENODEV;
> +	
> +	if (!request_mem_region((unsigned long)hpa, LASI_STATUS + 4, name))
> +		return -ENODEV;

Typically a driver would release resources before exiting.  e.g. if
request_mem_region() fails you should release_irq().

There are a couple of compiler warnings also.

Anyway, I'm about to try it on a couple of machines here; I'll let you
know how it goes.

Cheers,
  Richard