On Sat, 27 Jun 2009 07:18:32 +0200
David H__rdeman <david@xxxxxxxxxxx> 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;
> +}
> +
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
