Messages in this thread | | | Date | Mon, 7 Aug 2006 13:44:41 +0000 | From | Pavel Machek <> | Subject | Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access |
| |
Hi!
> The embedded controller on ThinkPad laptops has a non-standard interface > at IO ports 0x1600-0x161F (mapped to LCP channel 3 of the H8S chip). > The interface provides various system management services (currently > known: battery information and accelerometer readouts). This driver > provides access and mutual exclusion for the EC interface. > > The mainline hdaps driver already uses this hardware interface (in an > incorrect and unsafe way), and will be converted to use this module in > the following patches. Another driver using this module, tp_smapi, will > be submitted later. > > The Kconfig entry is set to tristate and will be selected by hdaps and > (eventually) tp_smapi, since thinkpad_ec does nothing by itself. > > Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
> +/* Module parameters: */ > +static int tp_debug = 0;
Static variables do not need initializers.
> +module_param_named(debug, tp_debug, int, 0600); > +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)"); > + > +/* A few macros for printk()ing: */ > +#define DPRINTK(fmt, args...) \ > + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
Is not there generic function doing this?
> +/* thinkpad_ec_lock: > + * Get exclusive lock for accesing the controller. May sleep. > + * Returns 0 iff lock acquired . > + */
Linuxdoc?
> +EXPORT_SYMBOL_GPL(thinkpad_ec_lock); > +EXPORT_SYMBOL_GPL(thinkpad_ec_try_lock); > +void thinkpad_ec_unlock(void) > +{ > + up(&thinkpad_ec_mutex); > +} > +
Do we need these wrappers? Perhaps just directly exporting the mutex?
> + /* Wait until EC starts writing its reply (~60ns on average). > + * Releasing locks before this happens may cause an EC hang > + * due to firmware bug! > + */ > + for (i=0; i<TPC_REQUEST_RETRIES; ++i) {
I'd write i++ here (and in other loops)... just for consistency with rest of kernel.
> +/*** Checking for EC hardware ***/ > + > +/* thinkpad_ec_test: > + * Ensure the EC LPC3 channel really works on this machine by making > + * an arbitrary harmless EC request and seeing if the EC follows protocol. > + * This test writes to IO ports, so execute only after checking DMI. > + */ > +static int thinkpad_ec_test(void) {
{ on new line, please.
> +/* Search all DMI device names for a given type for a substrng */ > +static int __init dmi_find_substring(int type, const char *substr) {
same here.
> + struct dmi_device *dev = NULL;
unneeded initializer.
> +static int __init thinkpad_ec_init(void) > +{ > + if (!check_dmi_for_ec()) { > + printk(KERN_ERR "thinkpad_ec: no ThinkPad embedded controller!\n"); > + return -ENODEV;
KERN_ERR is little strong here, no?
> + if (!request_region(TPC_BASE_PORT, TPC_NUM_PORTS, > + "thinkpad_ec")) > + {
{ on same line, please.
Pavel -- Thanks for all the (sleeping) penguins. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |