Hi Florian,

On Sat, Mar 26, 2016 at 09:23:17PM +0100, Florian Euchner wrote:
> The CM109 driver reported key press events of volume up / down and
> record / playback mute buttons, but release events only as soon as
> another button was pressed. Track state of volume buttons in order to
> report press and release events properly. For the record and playback
> mute buttons, only presses are registered by the CM109, therefore
> simulate press-n-release. This fixes the volume control buttons of
> various USB headsets.
> 
> Signed-off-by: Florian Euchner <florian.euch...@gmail.com>
> ---
> 
> The CM109 datasheet states that bit 0 and 1 of HID_IR0 indicate if
> volume up / down have been pressed (1) or released (0). Bits 2 and 3
> indicate a press-n-release for playback / record mute, there is no way
> to determine when the mute buttons were released.
> 
> I contacted Alfred E. Heggestad, the original author of this driver,
> but he understandably couldn't comment on this issue as he didn't work
> with the CM109 for quite some time. I cannot test this patch with the
> original USB phones the CM109 driver was intended for, I don't own one of
> those and the CM109 ones (at least those mentioned in the driver) have
> become harder to obtain, but I'd be very surprised if this patch didn't
> also work with those.
> 
>  drivers/input/misc/cm109.c | 69 
> ++++++++++++++++++++++++++++++----------------
>  1 file changed, 45 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
> index 9365535..e2c1a80 100644
> --- a/drivers/input/misc/cm109.c
> +++ b/drivers/input/misc/cm109.c
> @@ -76,8 +76,8 @@ enum {
>  
>       BUZZER_ON = 1 << 5,
>  
> -     /* up to 256 normal keys, up to 16 special keys */
> -     KEYMAP_SIZE = 256 + 16,
> +     /* up to 256 keys on the normal keymap */
> +     KEYMAP_SIZE = 256,

So we are breaking remapping of the "special" keys, why?

>  };
>  
>  /* CM109 protocol packet */
> @@ -129,25 +129,14 @@ struct cm109_dev {
>       int key_code;           /* last reported key */
>       int keybit;             /* 0=new scan  1,2,4,8=scan columns  */
>       u8 gpi;                 /* Cached value of GPI (high nibble) */
> +     bool volup_cached;      /* Cached state of volume up button */
> +     bool voldown_cached;    /* Cached state of volume down button */
>  };
>  
>  
> /******************************************************************************
>   * CM109 key interface
>   
> *****************************************************************************/
>  
> -static unsigned short special_keymap(int code)
> -{
> -     if (code > 0xff) {
> -             switch (code - 0xff) {
> -             case RECORD_MUTE:       return KEY_MUTE;
> -             case PLAYBACK_MUTE:     return KEY_MUTE;
> -             case VOLUME_DOWN:       return KEY_VOLUMEDOWN;
> -             case VOLUME_UP:         return KEY_VOLUMEUP;
> -             }
> -     }
> -     return KEY_RESERVED;
> -}
> -
>  /* Map device buttons to internal key events.
>   *
>   * The "up" and "down" keys, are symbolised by arrows on the button.
> @@ -191,7 +180,7 @@ static unsigned short keymap_kip1000(int scancode)
>       case 0x48: return KEY_ESC;                      /*   hangup     */
>       case 0x28: return KEY_LEFT;                     /*   IN         */
>       case 0x18: return KEY_RIGHT;                    /*   OUT        */
> -     default:   return special_keymap(scancode);
> +     default:   return KEY_RESERVED;
>       }
>  }
>  
> @@ -224,7 +213,7 @@ static unsigned short keymap_gtalk(int scancode)
>       case 0x28: return KEY_ESC;              /* End (red handset) */
>       case 0x48: return KEY_UP;               /* Menu up (rocker switch) */
>       case 0x88: return KEY_DOWN;             /* Menu down (rocker switch) */
> -     default:   return special_keymap(scancode);
> +     default:   return KEY_RESERVED;
>       }
>  }
>  
> @@ -253,7 +242,7 @@ static unsigned short keymap_usbph01(int scancode)
>       case 0x28: return KEY_ESC;                      /*   hangup     */
>       case 0x48: return KEY_LEFT;                     /*   IN         */
>       case 0x88: return KEY_RIGHT;                    /*   OUT        */
> -     default:   return special_keymap(scancode);
> +     default:   return KEY_RESERVED;
>       }
>  }
>  
> @@ -284,7 +273,7 @@ static unsigned short keymap_atcom(int scancode)
>       case 0x28: return KEY_ESC;                      /*   hangup     */
>       case 0x48: return KEY_LEFT;                     /* left arrow   */
>       case 0x88: return KEY_RIGHT;                    /* right arrow  */
> -     default:   return special_keymap(scancode);
> +     default:   return KEY_RESERVED;
>       }
>  }
>  
> @@ -338,9 +327,13 @@ static void cm109_submit_buzz_toggle(struct cm109_dev 
> *dev)
>  static void cm109_urb_irq_callback(struct urb *urb)
>  {
>       struct cm109_dev *dev = urb->context;
> +     struct input_dev *idev = dev->idev;
>       const int status = urb->status;
>       int error;
>  
> +     bool volup_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_UP);
> +     bool voldown_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_DOWN);
> +
>       dev_dbg(&dev->intf->dev, "### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] 
> keybit=0x%02x\n",
>            dev->irq_data->byte[0],
>            dev->irq_data->byte[1],
> @@ -356,13 +349,35 @@ static void cm109_urb_irq_callback(struct urb *urb)
>               goto out;
>       }
>  
> -     /* Special keys */
> -     if (dev->irq_data->byte[HID_IR0] & 0x0f) {
> -             const int code = (dev->irq_data->byte[HID_IR0] & 0x0f);
> -             report_key(dev, dev->keymap[0xff + code]);
> +     /* Report volume up / down button changes */
> +     if (volup_pressed != dev->volup_cached) {
> +             input_report_key(idev, KEY_VOLUMEUP, volup_pressed);
> +             input_sync(idev);
> +             dev->volup_cached = volup_pressed;

I am not sure why we do this. Why can't we have:

static void cm109_report_special(struct cm109_dev *dev)
{
        static const u8 autorelease = RECORD_MUTE | PLAYBACK_MUTE;
        struct input_dev *idev = dev->idev;
        u8 data = dev->irq_data->byte[HID_IR0];
        unsigned short keycode;
        int i;

        for (i = 0; i < 8; i++) {
                keycode = dev->keymap[0xff + BIT(i)]);
                if (keycode == KEY_RESERVED)
                        continue;

                input_report_key(idev, keycode, data & BIT(i));
                if (data & autorelease & BIT(i)) {
                        input_sync(idev);
                        input_report_key(idev, keycode, 0);
                }
        }
        input_sync(idev);
}

Thanks.

-- 
Dmitry

Reply via email to