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 | [email protected]
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
