lkml.org 
[lkml]   [2006]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
    Hi!

    > Thanks for the sign-offs!

    No problem.

    > >> +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?
    >
    > None that I found. Many drivers do it this way.

    linux/kernel.h : pr_debug() looks similar.

    > >> +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?
    >
    > Yes, we may end up needing to lock away other systems (ACPI?) that
    > touch the same ports. Apparently not an issue right now, but could
    > change with new firmware.
    > Also, that's what Alan Cox told me to do. :-)

    Okay... but do we really need try_lock variant? lock/unlock would be
    okay, but what is try_lock semantics when taking multiple locks...?

    > >> + struct dmi_device *dev = NULL;
    > >
    > >unneeded initializer.
    >
    > On a local variable?!

    You were right, but see the other mail.

    > >> +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?
    >
    > Not sure what's the right one. The user tried to load a module and the
    > module can't do that; I saw some drivers use KERN_ERR some
    > KERN_WARNING in similar cases. Is there some guideline on choosing
    > printk levels?

    Well, this will also trigger for thinkpad module compiled into kernel,
    right?
    Pavel

    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
    -
    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/

    \
     
     \ /
      Last update: 2006-08-08 01:19    [W:0.047 / U:0.008 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site