On Tue, Mar 29, 2011 at 11:33:22AM +0100, Matthew Garrett wrote:
> 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.
Yes, it have a _CRS, but I removed the code because it is always the same
IRQ and IO range. I'll reimplement with the first device it make it
necessary.
> > +/*** HELPER
> > *******************************************************************/
>
> Does checkpatch really not complain about that? I don't think it's a
> terribly helpful comment in any case :)
True, I'll remove it.
> > +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?
No, I'm not sure if it possible to reset the device. But in over 4 years,
I never see this happen. But I want to keep it, maybe I need to
reinvestigate someday.
> > + 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?
Theoretically yes. I will check it.
> 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.
I'll make a new patch.
Thank you.
> --
> 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
