lkml.org 
[lkml]   [2009]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: Input: add MAX7359 key switch controller driver
    From
    Hi Kim,

    >>
    >> Could you please restrict the commit text line to 80/72 columns? It
    >> will look good.
    >
    > Is this restriction for commit text mandatory? It seems like I got an
    > email from Andrew Morton that my patch was wordwraped even commit
    > text.

    This helps reader of the commit text to not give much stretch on his eyes :)

    >>
    >>> +
    >>> +static int __devinit max7359_probe(struct i2c_client *client,
    >>> +                                       const struct i2c_device_id *id)
    >>
    >> You may want to remove __devinit as it is i2c client driver . I have
    >> added linux-i2c for i2c specific review.
    >
    > Actually I don't want to remove it :), Is there any reason why you think so?

    Sorry I have mistaken it as __init. This is fine.

    >
    >>> +static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
    >>> +{
    >>> +       /* If no keys are pressed, enter sleep mode for 8192 ms */
    >>> +       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
    >>> +
    >>> +       return 0;
    >>> +}
    >>> +
    >>> +static int max7359_resume(struct i2c_client *client)
    >>> +{
    >>> +       /* Restore the default setting (autosleep for 256 ms) */
    >>> +       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
    >>> +
    >>> +       return 0;
    >>> +}
    >>
    >> Protect these two function with #ifdef CONFIG_PM please.
    >
    > This is what I really wanted to ask. I'm trying not to use '#ifdef'.
    > By the way, why most of drivers use protect suspend/resume functions
    > with #ifdef? Just for saving code size?

    suspend and resume is only important if the user of this driver has
    CONFIG_PM enabled in it's
    defconfig, right? So, if CONFIG_PM is not defined suspend/resume
    should equal to NULL.

    >>
    >>
    >>> +};
    >>> +
    >>> +
    >>> +MODULE_AUTHOR("Kim Kyuwon <q1.kim@samsung.com>");
    >>> +MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
    >>> +MODULE_LICENSE("GPL v2");
    >>
    >> MODULE_ALIAS ??
    >
    > In 'include/linux/module.h'
    >
    >  96 /* For userspace: you can also call me... */
    >  97 #define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias)
    >
    > From comment above, I think MODULE_ALIAS is optional. Do you still
    > think MODULE_ALIAS needs?
    >

    Yes, it is not __must__.

    --
    ---Trilok Soni
    http://triloksoni.wordpress.com
    http://www.linkedin.com/in/triloksoni
    --
    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: 2009-05-07 12:19    [W:0.028 / U:0.172 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site