lkml.org 
[lkml]   [2009]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] Add a driver for the Winbond WPCD376I IR functionality.
    On Sat, 27 Jun 2009 07:18:32 +0200
    David H__rdeman <david@hardeman.nu> wrote:

    > This patch adds a driver for the the Consumer IR (CIR) functionality
    > of the Winbond WPCD376I chipset (found on e.g. Intel DG45FC
    > motherboards).
    >
    > Changes since the driver was last posted to the list:
    > o Homebrew dprintk -> dev_dbg
    > o Some magic values changed to defines
    > o Fixed a bug where the wake-ir-command mask/value would not be
    > written properly
    > o Fixed the use of the invert parameter both for wake and normal
    > IR reception.
    >
    >
    > ...
    >
    > +static void
    > +wbcir_set_bits(unsigned long addr, u8 bits, u8 mask)
    > +{
    > + u8 val;
    > +
    > + val = inb(addr);
    > + val = ((val & ~mask) | (bits & mask));
    > + outb(val, addr);
    > +}

    What locking prevents the races which could occur here?

    Whatever it is, it would be good to document that here in a code
    comment.

    >
    > ...
    >
    > +static u8
    > +wbcir_revbyte(u8 byte)
    > +{
    > + byte = ((byte >> 1) & 0x55) | ((byte << 1) & 0xAA);
    > + byte = ((byte >> 2) & 0x33) | ((byte << 2) & 0xCC);
    > + return (byte >> 4) | (byte<<4);
    > +}

    Can this use the facilities in include/linux/bitrev.h?

    > +static u8
    > +wbcir_to_rc6cells(u8 val)
    > +{
    > + u8 coded = 0x00;
    > + int i;
    > +
    > + val &= 0x0F;
    > + for (i = 0; i < 4; i++) {
    > + if (val & 0x01)
    > + coded |= 0x02 << (i * 2);
    > + else
    > + coded |= 0x01 << (i * 2);
    > + val >>= 1;
    > + }
    > +
    > + return coded;
    > +}

    geeze, what does that do?

    Code needs comments..

    >
    > ...
    >
    > +static int
    > +wbcir_getkeycode(struct input_dev *dev, int sscancode, int *keycode)
    > +{
    > + unsigned int scancode = (unsigned int)sscancode;

    unneeded cast.

    > + struct wbcir_data *data = input_get_drvdata(dev);
    > +
    > + if (scancode < 0 || scancode > 0xFFFFFFFF)

    Neither of the comparisons in this expression can ever be true.

    > + return -EINVAL;
    > +
    > + *keycode = (int)wbcir_do_getkeycode(data, (u32)scancode);

    uneeded casts.

    Something has gone wrong with the types here. Where does the fault lie
    - this driver, or input core?

    > + return 0;
    > +}
    > +
    > +static int
    > +wbcir_setkeycode(struct input_dev *dev, int sscancode, int keycode)
    > +{
    > + struct wbcir_data *data = input_get_drvdata(dev);
    > + struct wbcir_keyentry *keyentry;
    > + struct wbcir_keyentry *new_keyentry;
    > + unsigned long flags;
    > + unsigned int old_keycode = KEY_RESERVED;
    > + unsigned int scancode = (unsigned int)sscancode;
    > +
    > + if (scancode < 0 || scancode > 0xFFFFFFFF)

    various dittoes.

    > + return -EINVAL;
    > +
    > + if (keycode < 0 || keycode > KEY_MAX)
    > + return -EINVAL;
    > +
    > + new_keyentry = kmalloc(sizeof(*new_keyentry), GFP_KERNEL);
    > + if (!new_keyentry)
    > + return -ENOMEM;
    > +
    > + write_lock_irqsave(&keytable_lock, flags);
    > +
    > + list_for_each_entry(keyentry, &data->keytable, list) {
    > + if (keyentry->key.scancode != scancode)
    > + continue;
    > +
    > + old_keycode = keyentry->key.keycode;
    > + keyentry->key.keycode = keycode;
    > +
    > + if (keyentry->key.keycode == KEY_RESERVED) {
    > + list_del(&keyentry->list);
    > + kfree(keyentry);
    > + }
    > +
    > + break;
    > + }
    > +
    > + set_bit(keycode, dev->keybit);
    > +
    > + if (old_keycode == KEY_RESERVED) {
    > + new_keyentry->key.scancode = (u32)scancode;
    > + new_keyentry->key.keycode = (unsigned int)keycode;
    > + list_add(&new_keyentry->list, &data->keytable);
    > + } else {
    > + kfree(new_keyentry);
    > + clear_bit(old_keycode, dev->keybit);
    > + list_for_each_entry(keyentry, &data->keytable, list) {
    > + if (keyentry->key.keycode == old_keycode) {
    > + set_bit(old_keycode, dev->keybit);
    > + break;
    > + }
    > + }
    > + }
    > +
    > + write_unlock_irqrestore(&keytable_lock, flags);
    > + return 0;
    > +}
    > +
    > +static void
    > +wbcir_keyup(unsigned long cookie)

    It would be useful to add a comment telling the reader that this is a
    timer-handler function.

    > +{
    > + struct wbcir_data *data = (struct wbcir_data *)cookie;
    > + unsigned long flags;
    > +
    > + /*
    > + * data->keyup_jiffies is used to prevent a race condition if a
    > + * hardware interrupt occurs at this point and the keyup timer
    > + * event is moved further into the future as a result.
    > + */

    hm. I don't see what the race is, nor how the comparison fixes it. If
    I _did_ understand this, perhaps I could suggest alternative fixes.
    But I don't so I can't. Oh well.

    > + spin_lock_irqsave(&wbcir_lock, flags);
    > +
    > + if (time_is_after_eq_jiffies(data->keyup_jiffies) && data->keypressed) {
    > + data->keypressed = 0;
    > + led_trigger_event(data->rxtrigger, LED_OFF);
    > + input_report_key(data->input_dev, data->last_keycode, 0);
    > + input_sync(data->input_dev);
    > + }
    > +
    > + spin_unlock_irqrestore(&wbcir_lock, flags);
    > +}
    > +
    >
    > ...
    >
    > +static void
    > +add_irdata_bit(struct wbcir_data *data, int set)
    > +{
    > + if (set)
    > + data->irdata[data->irdata_count / 8] |=
    > + 0x01 << (data->irdata_count % 8);

    Can/should this use generic___set_le_bit() or similar, rather than
    open-coding it?

    > + data->irdata_count++;
    > +}
    > +
    > +/* Gets count bits of irdata */
    > +static u16
    > +get_bits(struct wbcir_data *data, int count)
    > +{
    > + u16 val = 0x0;
    > +
    > + if (data->irdata_count - data->irdata_off < count) {
    > + data->irdata_error = 1;
    > + return 0x0;
    > + }
    > +
    > + while (count > 0) {
    > + val <<= 1;
    > + if (data->irdata[data->irdata_off / 8] &
    > + (0x01 << (data->irdata_off % 8)))
    > + val |= 0x1;

    ditto

    > + count--;
    > + data->irdata_off++;
    > + }
    > +
    > + return val;
    > +}
    > +
    >
    > ...
    >



    \
     
     \ /
      Last update: 2009-06-30 23:27    [W:3.098 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site