Re: [RESEND PATCH] Fujitsu tablet pc extras driver

[Matthew Garrett <mjg59@srcf.ucam.org>]

On Tue, Mar 29, 2011 at 12:03:22PM +0200, Robert Gerlach wrote:

> +#define INTERRUPT 5
> +#define IO_BASE 0xfd70

You're binding to an ACPI device here - does it have a _CRS method? If 
so, you should retrieve the resource information from acpipnp rather 
than hardcoding it. That'll involve reworking it as an acpi driver 
rather than a platform one, but that should be easy enough.

> +/*** HELPER *******************************************************************/

Does checkpatch really not complain about that? I don't think it's a 
terribly helpful comment in any case :)

> +static int fujitsu_busywait(void)

You're sleeping, so it's not really a busywait...

> +static void fujitsu_reset(void)
> +{
> +	fujitsu_ack();
> +	if (fujitsu_busywait())
> +		printk(KERN_WARNING MODULENAME ": timeout, real reset needed!\n");
> +}

We have no idea how to do a "real" reset, I guess?

> +	error = request_irq(INTERRUPT, fujitsu_isr,
> +			IRQF_SHARED, MODULENAME, fujitsu_isr);

Any risk of the irq firing between you doing setup and requesting the 
IRQ?

Other than the above, this looks fine from the driver point of view - 
Cc:ing Dmitry so he can have a quick look at the input side of things.

-- 
Matthew Garrett | mjg59@srcf.ucam.org