lkml.org 
[lkml]   [2010]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 04/11] hwmon: applesmc: Introduce a register lookup table (rev2)
    From
    Date
    On Tue, 2010-11-09 at 10:15 -0500, Henrik Rydberg wrote:
    > One main problem with the current driver is the inability to quickly
    > search for supported keys, resulting in detailed feature maps per
    > machine model which are cumbersome to maintain.
    >
    > This patch adds a register lookup table, which enables binary search
    > for supported keys. The lookup also reduces the io frequency, so the
    > original mutex is replaced by locks around the actual io.
    >
    > Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

    Hi Henrik,

    looks pretty good now. Couple of additional comments.

    > ---
    > drivers/hwmon/applesmc.c | 528 +++++++++++++++++++++++++---------------------
    > 1 files changed, 286 insertions(+), 242 deletions(-)
    >
    > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
    > index 5f67e39..623f71c 100644
    > --- a/drivers/hwmon/applesmc.c
    > +++ b/drivers/hwmon/applesmc.c
    [ ... ]
    > +/*
    > + * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
    > + */
    > +static int applesmc_init_smcreg_try(void)
    > +{
    > + struct applesmc_registers *s = &smcreg;
    > + int ret;
    > +
    > + if (s->init_complete)
    > + return 0;
    > +
    > + mutex_init(&s->mutex);
    > +
    I am a bit concerned that mutex_init() can be called multiple times. Are
    you sure this is safe ?

    > + ret = read_register_count(&s->key_count);
    > + if (ret)
    > + return ret;
    > +
    > + if (!s->cache)
    > + s->cache = kcalloc(s->key_count, sizeof(*s->cache), GFP_KERNEL);
    > + if (!s->cache)
    > + return -ENOMEM;
    > +
    > + s->init_complete = true;
    > +
    > + pr_info("key=%d\n", s->key_count);
    > +
    Hope that means more to macbook users than it does to me ;).

    > + return 0;
    > +}
    > +
    > +/*
    > + * applesmc_init_smcreg - Initialize register cache.
    > + *
    > + * Retries until initialization is successful, or the operation times out.
    > + *
    > + */
    > +static int applesmc_init_smcreg(void)
    > +{
    > + int ms, ret;
    > +
    > + for (ms = 0; ms < INIT_TIMEOUT_MSECS; ms += INIT_WAIT_MSECS) {
    > + ret = applesmc_init_smcreg_try();
    > + if (!ret)
    > + return 0;
    > + pr_warn("slow init, retrying\n");

    INIT_WAIT_MS is 50ms, so you issue this warning every 50ms for up to
    five seconds. Pretty noisy... sure that is what you want ? Also, does it
    really make sense to retry if the error is ENOMEM ?

    Thanks,
    Guenter




    \
     
     \ /
      Last update: 2010-11-09 20:07    [W:0.031 / U:0.240 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site