lkml.org 
[lkml]   [2010]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35
    From
    Date
    On Fri, 2010-09-03 at 16:15 +0900, Masayuki Ohtak wrote:
    []
    > +#define pch_dbg(adap, fmt, arg...) \
    > + dev_dbg(adap->pch_adapter.dev.parent, "%s :"fmt, __func__, ##arg)
    > +
    > +#define pch_err(adap, fmt, arg...) \
    > + dev_err(adap->pch_adapter.dev.parent, "%s :"fmt, __func__, ##arg)
    > +
    > +#define pch_pci_err(pdev, fmt, arg...) \
    > + dev_err(&pdev->dev, "%s :"fmt, __func__, ##arg)
    > +#define pch_pci_dbg(pdev, fmt, arg...) \
    > + dev_dbg(&pdev->dev, "%s :"fmt, __func__, ##arg)

    OK, but it seems careless because the two types
    are not uniformly indented, there's a blank line
    between pch_dbg and pch_err, and the two pch_pci_<level>
    defines are in the reverse order without a blank line
    between them.

    I think it's better to use separate multiple strings
    that are concatentated by the preprocessor like:
    "%s :" fmt
    not
    "%s :"fmt

    Almost all code in kernel uses "%s: " to format __func__.
    Some use "%s(): ". I think "%s :" is unique.

    The rest of the logging messages look good.

    Some other comments:

    > + if ((pch_wait_for_xfer_complete(adap) == 0) &&
    > + (pch_getack(adap) == 0)) {

    This would look better as:

    if ((pch_wait_for_xfer_complete(adap) == 0) &&
    (pch_getack(adap) == 0)) {

    > + if ((pch_wait_for_xfer_complete(adap) == 0)
    > + && (pch_getack(adap) == 0)) {

    Here too.

    > + for (i = 0; i < PCH_MAX_CHN; i++) {
    > + while ((adap_info->pch_data[i].pch_xfer_in_progress)) {
    > + /* Wait until all channel transfers are completed */
    > + msleep(1);
    > + }
    > + /* Disable the i2c interrupts */
    > + pch_disbl_int(&adap_info->pch_data[i]);
    > + }

    Would it be better to disable all possible interrupts first
    or do you need to disable them in order?

    Something like:

    bool *disabled = kzalloc(PCH_MAX_CHN * sizeof(bool), GFP_KERNEL);
    /*
    * or a static with a memset, or check something
    * like pch_is_int_enabled(&adap_info->pch_data[i])
    * then remove the else because the kzalloc couldn't fail.
    */
    if (disabled) {
    bool alldone;
    do {
    alldone = true;
    for (i = 0; i < PCH_MAX_CHN; i++) {
    if (!adap_info->pch_data[i].pch_xfer_in_progress &&
    !disabled[i])) {
    pch_disbl_int(&adap_info->pch_data[i]);
    disabled[i] = true;
    } else
    alldone = false;
    }
    if (!alldone) {
    /* Wait until all channel transfers are completed */
    msleep(1);
    }
    } while (!alldone);
    kfree(disabled);

    /* remove the else if there's a static etc */

    } else {
    for (i = 0; i < PCH_MAX_CHN; i++) {
    while ((adap_info->pch_data[i].pch_xfer_in_progress)) {
    /* Wait until all channel transfers are completed */
    msleep(1);
    }
    /* Disable the i2c interrupts */
    pch_disbl_int(&adap_info->pch_data[i]);
    }
    }

    cheers, Joe



    \
     
     \ /
      Last update: 2010-09-03 10:13    [W:0.033 / U:60.596 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site