Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Le jeu. 25 avr. 2019 à 11:35, Benjamin Tissoires
 a écrit :
>
> On Thu, Apr 25, 2019 at 11:29 AM Clément VUCHENER
>  wrote:
> >
> > Le jeu. 25 avr. 2019 à 10:45, Benjamin Tissoires
> >  a écrit :
> > >
> > > On Thu, Apr 25, 2019 at 10:25 AM Clément VUCHENER
> > >  wrote:
> > > >
> > > > Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
> > > >  a écrit :
> > > > >
> > > > > Hi Clément,
> > > > >
> > > > > On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
> > > > >  wrote:
> > > > > >
> > > > > > Hi Benjamin,
> > > > > >
> > > > > > I tried again to add hi-res wheel support for the G500 with Hans de
> > > > > > Goede's latest patch series you've just merged in for-5.2/logitech, 
> > > > > > it
> > > > > > is much better but there is still some issues.
> > > > > >
> > > > > > The first one is the device index, I need to use device index 0
> > > > > > instead 0xff. I added a quick and dirty quirk (stealing in the
> > > > > > QUIRK_CLASS range since the normal quirk range looks full) to change
> > > > > > the device index assigned in __hidpp_send_report. After that the
> > > > > > device is correctly initialized and the wheel multiplier is set.
> > > > >
> > > > > Hmm, maybe we should restrain a little bit the reserved quirks...
> > > > > But actually, .driver_data and .quirks are both unsigned long, so you
> > > > > should be able to use the 64 bits.
> > > >
> > > > Only on 64 bits architectures, or is the kernel forcing long integers
> > > > to be 64 bits everywhere?
> > >
> > > Damnit, you are correct, there is no such enforcement :/
> > >
> > > >
> > > > >
> > > > > >
> > > > > > The second issue is that wheel values are not actually scaled
> > > > > > according to the multiplier. I get 7/8 full scroll event for each
> > > > > > wheel step. I think it happens because the mouse is split in two
> > > > > > devices. The first device has the wheel events, and the second 
> > > > > > device
> > > > > > has the HID++ reports. The wheel multiplier is only set on the 
> > > > > > second
> > > > > > device (where the hi-res mode is enabled) and does not affect the
> > > > > > wheel events from the first one.
> > > > >
> > > > > I would think this have to do with the device not accepting the
> > > > > command instead. Can you share some raw logs of the events (ideally
> > > > > with hid-recorder from
> > > > > https://gitlab.freedesktop.org/libevdev/hid-tools)?
> > > >
> > > > I already checked with usbmon and double-checked by querying the
> > > > register. The flag is set and accepted by the device and it behaves
> > > > accordingly: it sends about 8 wheel events per step.
> > >
> > > OK, that's what I wanted to see.
> > >
> > > >
> > > > hid-recorder takes hidraw nodes as parameters, how do I use it to
> > > > record the initialization by the driver?
> > >
> > > You can't. But it doesn't really matter here because I wanted to check
> > > what your mouse is actually sending after the initialization.
> > >
> > > So if I read correctly: the mouse is sending high res data but
> > > evemu-recorder shows one REL_WHEEL event every 7/8 clicks?
> >
> > It is HID++ 1.0, there is no special high res data report, it sends
> > standard HID mouse wheel report, but more of them.
> >
> > To be clear, here is a recording using the modified kernel. I moved
> > the wheel one step up (8 events are generated), then one step down (8
> > events again + a 2-event bump).
> >
> > # EVEMU 1.3
> [snipped]
> > E: 1.975014 8 00 00 00 00 00 00 01 00
> >
> > The hi-res events should +/-15 instead of +/-120, and less REL_WHEEL
> > events should be generated. This looks like the multiplier is set to 1
> > instead of 8.
> >
> > kernel log shows the multiplier is set but only for one of the two devices:
> >
> > input: Logitech G500 as
> > /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:046D:C068.0006/input/input25
> > hid-generic 0003:046D:C068.0006: input,hidraw5: USB

Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Le jeu. 25 avr. 2019 à 10:45, Benjamin Tissoires
 a écrit :
>
> On Thu, Apr 25, 2019 at 10:25 AM Clément VUCHENER
>  wrote:
> >
> > Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
> >  a écrit :
> > >
> > > Hi Clément,
> > >
> > > On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
> > >  wrote:
> > > >
> > > > Hi Benjamin,
> > > >
> > > > I tried again to add hi-res wheel support for the G500 with Hans de
> > > > Goede's latest patch series you've just merged in for-5.2/logitech, it
> > > > is much better but there is still some issues.
> > > >
> > > > The first one is the device index, I need to use device index 0
> > > > instead 0xff. I added a quick and dirty quirk (stealing in the
> > > > QUIRK_CLASS range since the normal quirk range looks full) to change
> > > > the device index assigned in __hidpp_send_report. After that the
> > > > device is correctly initialized and the wheel multiplier is set.
> > >
> > > Hmm, maybe we should restrain a little bit the reserved quirks...
> > > But actually, .driver_data and .quirks are both unsigned long, so you
> > > should be able to use the 64 bits.
> >
> > Only on 64 bits architectures, or is the kernel forcing long integers
> > to be 64 bits everywhere?
>
> Damnit, you are correct, there is no such enforcement :/
>
> >
> > >
> > > >
> > > > The second issue is that wheel values are not actually scaled
> > > > according to the multiplier. I get 7/8 full scroll event for each
> > > > wheel step. I think it happens because the mouse is split in two
> > > > devices. The first device has the wheel events, and the second device
> > > > has the HID++ reports. The wheel multiplier is only set on the second
> > > > device (where the hi-res mode is enabled) and does not affect the
> > > > wheel events from the first one.
> > >
> > > I would think this have to do with the device not accepting the
> > > command instead. Can you share some raw logs of the events (ideally
> > > with hid-recorder from
> > > https://gitlab.freedesktop.org/libevdev/hid-tools)?
> >
> > I already checked with usbmon and double-checked by querying the
> > register. The flag is set and accepted by the device and it behaves
> > accordingly: it sends about 8 wheel events per step.
>
> OK, that's what I wanted to see.
>
> >
> > hid-recorder takes hidraw nodes as parameters, how do I use it to
> > record the initialization by the driver?
>
> You can't. But it doesn't really matter here because I wanted to check
> what your mouse is actually sending after the initialization.
>
> So if I read correctly: the mouse is sending high res data but
> evemu-recorder shows one REL_WHEEL event every 7/8 clicks?

It is HID++ 1.0, there is no special high res data report, it sends
standard HID mouse wheel report, but more of them.

To be clear, here is a recording using the modified kernel. I moved
the wheel one step up (8 events are generated), then one step down (8
events again + a 2-event bump).

# EVEMU 1.3
# Kernel: 5.0.0logitech+
# DMI: 
dmi:bvnAmericanMegatrendsInc.:bvr1.40:bd01/17/2019:svnMicro-StarInternationalCo.,Ltd.:pnMS-7B98:pvr1.0:rvnMicro-StarInternationalCo.,Ltd.:rnZ390-APRO(MS-7B98):rvr1.0:cvnMicro-StarInternationalCo.,Ltd.:ct3:cvr1.0:
# Input device name: "Logitech G500"
# Input device ID: bus 0x03 vendor 0x46d product 0xc068 version 0x111
# Supported events:
#   Event type 0 (EV_SYN)
# Event code 0 (SYN_REPORT)
# Event code 1 (SYN_CONFIG)
# Event code 2 (SYN_MT_REPORT)
# Event code 3 (SYN_DROPPED)
# Event code 4 ((null))
# Event code 5 ((null))
# Event code 6 ((null))
# Event code 7 ((null))
# Event code 8 ((null))
# Event code 9 ((null))
# Event code 10 ((null))
# Event code 11 ((null))
# Event code 12 ((null))
# Event code 13 ((null))
# Event code 14 ((null))
# Event code 15 (SYN_MAX)
#   Event type 1 (EV_KEY)
# Event code 272 (BTN_LEFT)
# Event code 273 (BTN_RIGHT)
# Event code 274 (BTN_MIDDLE)
# Event code 275 (BTN_SIDE)
# Event code 276 (BTN_EXTRA)
# Event code 277 (BTN_FORWARD)
# Event code 278 (BTN_BACK)
# Event code 279 (BTN_TASK)
# Event code 280 ((null))
# Event code 281 ((null))
# Event code 282 ((null))
# Event code 283 ((null))
# Event code 284 ((null))
# Event code 285 ((null))
# Event code 286 ((null))
# Event code 287 ((null))
#   Event type 2 (EV_REL)
# Event code 0 (REL_X)
# Event code 1 (REL_Y)
# Event code 6 (REL_HWHEEL)
# Event c

Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
 a écrit :
>
> Hi Clément,
>
> On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
>  wrote:
> >
> > Hi Benjamin,
> >
> > I tried again to add hi-res wheel support for the G500 with Hans de
> > Goede's latest patch series you've just merged in for-5.2/logitech, it
> > is much better but there is still some issues.
> >
> > The first one is the device index, I need to use device index 0
> > instead 0xff. I added a quick and dirty quirk (stealing in the
> > QUIRK_CLASS range since the normal quirk range looks full) to change
> > the device index assigned in __hidpp_send_report. After that the
> > device is correctly initialized and the wheel multiplier is set.
>
> Hmm, maybe we should restrain a little bit the reserved quirks...
> But actually, .driver_data and .quirks are both unsigned long, so you
> should be able to use the 64 bits.

Only on 64 bits architectures, or is the kernel forcing long integers
to be 64 bits everywhere?

>
> >
> > The second issue is that wheel values are not actually scaled
> > according to the multiplier. I get 7/8 full scroll event for each
> > wheel step. I think it happens because the mouse is split in two
> > devices. The first device has the wheel events, and the second device
> > has the HID++ reports. The wheel multiplier is only set on the second
> > device (where the hi-res mode is enabled) and does not affect the
> > wheel events from the first one.
>
> I would think this have to do with the device not accepting the
> command instead. Can you share some raw logs of the events (ideally
> with hid-recorder from
> https://gitlab.freedesktop.org/libevdev/hid-tools)?

I already checked with usbmon and double-checked by querying the
register. The flag is set and accepted by the device and it behaves
accordingly: it sends about 8 wheel events per step.

hid-recorder takes hidraw nodes as parameters, how do I use it to
record the initialization by the driver?

>
> Cheers,
> Benjamin
>
> >
> > Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
> >  a écrit :
> > >
> > > On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> > >  wrote:
> > > >
> > > > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > > >  a écrit :
> > > > >
> > > > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts  a 
> > > > > écrit :
> > > > > >
> > > > > > Hi Clement,
> > > > > >
> > > > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > > >  wrote:
> > > > > > > Hi, The G500s (and the G500 too, I think) does support the 
> > > > > > > "scrolling
> > > > > > > acceleration" bit. If I set it, I get around 8 events for each 
> > > > > > > wheel
> > > > > > > "click", this is what this driver expects, right? If I understood
> > > > > > > correctly, I should try this patch with the
> > > > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > > > >
> > > > > > Thanks for the info! Yes, that should work.
> > > > >
> > > > > Well, it is not that simple. I get "Device not connected" errors for
> > > > > both interfaces of the mouse.
> > > >
> > > > I suspect the device is not responding because the hid device is not
> > > > started. When is hid_hw_start supposed to be called? It is called
> > > > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > > > device is not started when hidpp_is_connected is called. Is this
> > > > because most of the device in this driver are not real HID devices but
> > > > DJ devices? How should non-DJ devices be treated?
> > >
> > > Hi Clement,
> > >
> > > I have a series I sent last September that allows to support non DJ
> > > devices on logitech-hidpp
> > > (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
> > >
> > > In its current form, with the latest upstream kernel, the series will
> > > oops during the .event() callback, which is easy enough to fix.
> > > However, I am currently trying to make it better as a second or third
> > > reading made me realized that there was a bunch of non-sense in it and
> > > a proper support would require slightly more work for the non unifying
> > > receiver case.
> > >
> > > I hope I'll be able to send out something by the end of the week.
> > >
> > > Cheers,
> > > Benjamin

Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Hi Benjamin,

I tried again to add hi-res wheel support for the G500 with Hans de
Goede's latest patch series you've just merged in for-5.2/logitech, it
is much better but there is still some issues.

The first one is the device index, I need to use device index 0
instead 0xff. I added a quick and dirty quirk (stealing in the
QUIRK_CLASS range since the normal quirk range looks full) to change
the device index assigned in __hidpp_send_report. After that the
device is correctly initialized and the wheel multiplier is set.

The second issue is that wheel values are not actually scaled
according to the multiplier. I get 7/8 full scroll event for each
wheel step. I think it happens because the mouse is split in two
devices. The first device has the wheel events, and the second device
has the HID++ reports. The wheel multiplier is only set on the second
device (where the hi-res mode is enabled) and does not affect the
wheel events from the first one.

Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
 a écrit :
>
> On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
>  wrote:
> >
> > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> >  a écrit :
> > >
> > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts  a écrit :
> > > >
> > > > Hi Clement,
> > > >
> > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > >  wrote:
> > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > "click", this is what this driver expects, right? If I understood
> > > > > correctly, I should try this patch with the
> > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > >
> > > > Thanks for the info! Yes, that should work.
> > >
> > > Well, it is not that simple. I get "Device not connected" errors for
> > > both interfaces of the mouse.
> >
> > I suspect the device is not responding because the hid device is not
> > started. When is hid_hw_start supposed to be called? It is called
> > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > device is not started when hidpp_is_connected is called. Is this
> > because most of the device in this driver are not real HID devices but
> > DJ devices? How should non-DJ devices be treated?
>
> Hi Clement,
>
> I have a series I sent last September that allows to support non DJ
> devices on logitech-hidpp
> (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
>
> In its current form, with the latest upstream kernel, the series will
> oops during the .event() callback, which is easy enough to fix.
> However, I am currently trying to make it better as a second or third
> reading made me realized that there was a bunch of non-sense in it and
> a proper support would require slightly more work for the non unifying
> receiver case.
>
> I hope I'll be able to send out something by the end of the week.
>
> Cheers,
> Benjamin

Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
 a écrit :
>
> Le ven. 14 déc. 2018 à 19:37, Harry Cutts  a écrit :
> >
> > Hi Clement,
> >
> > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> >  wrote:
> > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > "click", this is what this driver expects, right? If I understood
> > > correctly, I should try this patch with the
> > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> >
> > Thanks for the info! Yes, that should work.
>
> Well, it is not that simple. I get "Device not connected" errors for
> both interfaces of the mouse.

I suspect the device is not responding because the hid device is not
started. When is hid_hw_start supposed to be called? It is called
early for HID_QUIRK_CLASS_G920 but later for other device. So the
device is not started when hidpp_is_connected is called. Is this
because most of the device in this driver are not real HID devices but
DJ devices? How should non-DJ devices be treated?

Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Le ven. 14 déc. 2018 à 19:37, Harry Cutts  a écrit :
>
> Hi Clement,
>
> On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
>  wrote:
> > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > acceleration" bit. If I set it, I get around 8 events for each wheel
> > "click", this is what this driver expects, right? If I understood
> > correctly, I should try this patch with the
> > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
>
> Thanks for the info! Yes, that should work.

Well, it is not that simple. I get "Device not connected" errors for
both interfaces of the mouse.

logitech-hidpp-device 0003:046D:C24E.000E: Device not connected
logitech-hidpp-device 0003:046D:C24E.000F: Device not connected

Here is the usbmon trace when connecting a G500s:

0cd42cc0 3.313741 C Ii:3:001:1 0:2048 1 =
04
0cd42cc0 3.313750 S Ii:3:001:1 -:2048 4 <
17b49a80 3.313764 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.313775 C Ci:3:001:0 0 4 =
01010100
17b49a80 3.313781 S Co:3:001:0 s 23 01 0010 0002  0
17b49a80 3.313789 C Co:3:001:0 0 0
17b49a80 3.313792 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.313797 C Ci:3:001:0 0 4 =
0101
17b49a80 3.340415 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.340438 C Ci:3:001:0 0 4 =
0101
17b49a80 3.367434 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.367448 C Ci:3:001:0 0 4 =
0101
17b49a80 3.394434 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.394449 C Ci:3:001:0 0 4 =
0101
17b49a80 3.421416 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.421431 C Ci:3:001:0 0 4 =
0101
17b49a80 3.421690 S Co:3:001:0 s 23 03 0004 0002  0
17b49a80 3.421707 C Co:3:001:0 0 0
17b49a80 3.483421 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b49a80 3.483436 C Ci:3:001:0 0 4 =
1101
17b499c0 3.545429 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b499c0 3.545442 C Ci:3:001:0 0 4 =
03011000
17b499c0 3.545448 S Co:3:001:0 s 23 01 0014 0002  0
17b499c0 3.545456 C Co:3:001:0 0 0
17b499c0 3.597659 S Ci:3:000:0 s 80 06 0100  0040 64 <
17b499c0 3.597851 C Ci:3:000:0 0 8 =
12010002 0008
17b499c0 3.597870 S Co:3:001:0 s 23 03 0004 0002  0
17b499c0 3.597884 C Co:3:001:0 0 0
17b499c0 3.659419 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b499c0 3.659434 C Ci:3:001:0 0 4 =
1101
17b499c0 3.721421 S Ci:3:001:0 s a3 00  0002 0004 4 <
17b499c0 3.721435 C Ci:3:001:0 0 4 =
03011000
17b499c0 3.721442 S Co:3:001:0 s 23 01 0014 0002  0
17b499c0 3.721451 C Co:3:001:0 0 0
17b49600 3.788420 S Ci:3:007:0 s 80 06 0100  0012 18 <
17b49600 3.788897 C Ci:3:007:0 0 18 =
12010002 0008 6d044ec2 01840102 0301
 . . . .  . . . .  m . N .  . . . .  . .
17b49600 3.788920 S Ci:3:007:0 s 80 06 0600  000a 10 <
17b49600 3.789057 C Ci:3:007:0 -32 0
17b49600 3.789092 S Ci:3:007:0 s 80 06 0600  000a 10 <
17b49600 3.789438 C Ci:3:007:0 -32 0
17b49600 3.789459 S Ci:3:007:0 s 80 06 0600  000a 10 <
17b49600 3.789827 C Ci:3:007:0 -32 0
17b49600 3.789850 S Ci:3:007:0 s 80 06 0200  0009 9 <
17b49600 3.790272 C Ci:3:007:0 0 9 =
09023b00 020104a0 31
 . . ; .  . . . .  1
17b49600 3.790285 S Ci:3:007:0 s 80 06 0200  003b 59 <
17b49600 3.791004 C Ci:3:007:0 0 59 =
09023b00 020104a0 31090400 00010301 02000921 11010001 22430007 05810308
 . . ; .  . . . .  1 . . .  . . . .  . . . !  . . . .  " C . .  . . . .
17b49600 3.791021 S Ci:3:007:0 s 80 06 0300  00ff 255 <
17b49600 3.791203 C Ci:3:007:0 0 4 =
04030904
17b49600 3.791212 S Ci:3:007:0 s 80 06 0302 0409 00ff 255 <
17b49600 3.791829 C Ci:3:007:0 0 50 =
32034700 35003000 30007300 20004c00 61007300 65007200 20004700 61006d00
 2 . G .  5 . 0 .  0 . s .. L .  a . s .  e . r .. G .  a . m .
17b49600 3.791844 S Ci:3:007:0 s 80 06 0301 0409 00ff 255 <
17b49600 3.792141 C Ci:3:007:0 0 18 =
12034c00 6f006700 69007400 65006300 6800
 . . L .  o . g .  i . t .  e . c .  h .
17b49600 3.792154 S Ci:3:007:0 s 80 06 0303 0409 00ff 255 <
17b49600 3.792563 C Ci:3:007:0 0 30 =
1e033300 34004500 31003300 38003500 30003800 36003000 30003000 3900
 . . 3 .  4 . E .  1 . 3 .  8 . 5 .  0 . 8 .  6 . 0 .  0 . 0 .  9 .
17b49300 3.795944 S Co:3:007:0 s 00 09 0001   0
17b49300 3.795998 C Co:3:007:0 0 0
17b49300 3.796025 S Ci:3:007:0 s 80 06 0304 0409 00ff 255 <
17b49300 3.796392 C Ci:3:007:0 0 26 =
1a035500 38003400 2e003000 31005f00 42003000 30003000 3900
 . . U .  8 . 4 .  . . 0 .  1 . _ .  B . 0 .  0 . 0 .  9 .
17b49900 3.796473 S Ci:3:007:0 s 80 06 0303 0409 00ff 255 <
17b49900 3.796883 C Ci:3:007:0 0 30 =
1e033300 34004500 31003300 38003500 30003800 36003000 30003000 3900
 . . 3 .  4 . E .  1 . 3 .  8 . 5 .  0 . 8 .  6 . 0 .  0 . 0 .  9 .
17b49900 3.796914 S Co:3:007:0 s 21 0a    0
17b49900 3.797027 C Co:3:007:0 -32 0
17b49

Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

[Clément VUCHENER]

Le mer. 5 déc. 2018 à 01:44, Peter Hutterer  a écrit :
>
> From: Harry Cutts 
>
> There are three features used by various Logitech mice for
> high-resolution scrolling: the scrolling acceleration bit in HID++ 1.0,
> and the x2120 and x2121 features in HID++ 2.0 and above. This patch
> supports all three, and uses the multiplier reported by the mouse for
> the HID++ 2.0+ features.

Hi, The G500s (and the G500 too, I think) does support the "scrolling
acceleration" bit. If I set it, I get around 8 events for each wheel
"click", this is what this driver expects, right? If I understood
correctly, I should try this patch with the
HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-03-19 21:08 GMT+01:00 Rodrigo Rivas Costa :
> On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
>
> Now, what I would really want is a review by Valve of my set-lizard function:
>
> static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> {
> if (enabled) {
> steam_send_report_byte(steam, 0x8e); //enable mouse
> steam_send_report_byte(steam, 0x85); //enable esc, enter and 
> cursor keys
> } else {
> steam_send_report_byte(steam, 0x81); //disable esc, enter and 
> cursor keys
> steam_write_register(steam, 0x08, 0x07); //disable mouse 
> (cmd: 0x87)
> }
> }
>
> While it works, I find its asymmetry quite uncanny. I'm afraid that some
> of these are there for a side effect, this is not their real purpose.
> Could you give me a hint about this?
>

If I remember correctly, you can also enable the mouse with "87 03 08
00 00". But that do not explain the asymmetry or why there are two
ways of doing it. I always found it weird that the "enable" value was
0x and the "disable" value 0x0007.

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-03-19 21:08 GMT+01:00 Rodrigo Rivas Costa :
> On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
>
> Now, what I would really want is a review by Valve of my set-lizard function:
>
> static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> {
> if (enabled) {
> steam_send_report_byte(steam, 0x8e); //enable mouse
> steam_send_report_byte(steam, 0x85); //enable esc, enter and 
> cursor keys
> } else {
> steam_send_report_byte(steam, 0x81); //disable esc, enter and 
> cursor keys
> steam_write_register(steam, 0x08, 0x07); //disable mouse 
> (cmd: 0x87)
> }
> }
>
> While it works, I find its asymmetry quite uncanny. I'm afraid that some
> of these are there for a side effect, this is not their real purpose.
> Could you give me a hint about this?
>

If I remember correctly, you can also enable the mouse with "87 03 08
00 00". But that do not explain the asymmetry or why there are two
ways of doing it. I always found it weird that the "enable" value was
0x and the "disable" value 0x0007.

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivasco...@gmail.com>:
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
>
> Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>
> @Pierre-Loup and @Clément, could you please have another look at this and
> check if it is worthy? Benjamin will not commit it without an express ACK from
> Valve. Of course he is right to be cautious, but I checked this driver with
> the Steam Client and all seems to go just fine. I think that there is a lot of
> Linux out of the desktop that could use this driver and cannot use the Steam
> Client. Worst case scenario, this driver can now be blacklisted, but I hope
> that will not be needed.

I tested the driver with my 4.15 fedora kernel (I only built the
module not the whole kernel) and I got double inputs (your driver
input device + steam uinput device) when testing Shovel Knight with
Steam Big Picture. It seems to work fine when the inputs are the same,
but after changing the controller configuration in Steam, the issue
became apparent.

And without Steam and your external tool, you get double inputs too. I
tried RetroArch and it was unusable because of the keyboard inputs
from the lizard mode (e.g. pressing B also presses Esc and quits
RetroArch). Having to download and compile an external tool to make
the driver work properly may be too difficult for the user. Your goal
was to provide an alternative to user space drivers but now you
actually depend on (a very simple) one.

Also the button and axis codes do not match the gamepad API doc
(https://www.kernel.org/doc/Documentation/input/gamepad.txt).

>
> For full reference, I'm adding a full changelog of this patchset.
>
> Changes in v5:
>  * Fix license SPDX to GPL-2.0+.
>  * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>
> Changes in v4:
>  * Add command to check the wireless connection status on probe, without
>waiting for a message (thanks to Clément Vuchener for the tip).
>  * Removed the error code on redundant connection/disconnection messages. That
>was harmless but polluted dmesg.
>  * Added buttons for touching the left-pad and right-pad.
>  * Fixed a misplaced #include from 2/4 to 1/4.
>
> Changes in v3:
>  * Use RCU to do the dynamic connec/disconnect of wireless devices.
>  * Remove entries in hid-quirks.c as they are no longer needed. This allows
>this module to be blacklisted without side effects.
>  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>existing use cases (lizard mode). A user-space tool to do that is
>linked.
>  * Fully separated axes for joystick and left-pad. As it happens.
>  * Add fuzz values for left/right pad axes, they are a little wiggly.
>
> Changes in v2:
>  * Remove references to USB. Now the interesting interfaces are selected by
>looking for the ones with feature reports.
>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>  * Feature report length is checked, to avoid overflows in case of
>corrupt/malicius USB devices.
>  * Resolution added to the ABS axes.
>  * A lot of minor cleanups.
>
> Rodrigo Rivas Costa (4):
>   HID: add driver for Valve Steam Controller
>   HID: steam: add serial number information.
>   HID: steam: command to check wireless connection
>   HID: steam: add battery device.
>
>  drivers/hid/Kconfig |   8 +
>  drivers/hid/Makefile|   1 +
>  drivers/hid/hid-ids.h   |   4 +
>  drivers/hid/hid-steam.c | 794 
> 
>  4 files changed, 807 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.2
>

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa :
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
>
> Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>
> @Pierre-Loup and @Clément, could you please have another look at this and
> check if it is worthy? Benjamin will not commit it without an express ACK from
> Valve. Of course he is right to be cautious, but I checked this driver with
> the Steam Client and all seems to go just fine. I think that there is a lot of
> Linux out of the desktop that could use this driver and cannot use the Steam
> Client. Worst case scenario, this driver can now be blacklisted, but I hope
> that will not be needed.

I tested the driver with my 4.15 fedora kernel (I only built the
module not the whole kernel) and I got double inputs (your driver
input device + steam uinput device) when testing Shovel Knight with
Steam Big Picture. It seems to work fine when the inputs are the same,
but after changing the controller configuration in Steam, the issue
became apparent.

And without Steam and your external tool, you get double inputs too. I
tried RetroArch and it was unusable because of the keyboard inputs
from the lizard mode (e.g. pressing B also presses Esc and quits
RetroArch). Having to download and compile an external tool to make
the driver work properly may be too difficult for the user. Your goal
was to provide an alternative to user space drivers but now you
actually depend on (a very simple) one.

Also the button and axis codes do not match the gamepad API doc
(https://www.kernel.org/doc/Documentation/input/gamepad.txt).

>
> For full reference, I'm adding a full changelog of this patchset.
>
> Changes in v5:
>  * Fix license SPDX to GPL-2.0+.
>  * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>
> Changes in v4:
>  * Add command to check the wireless connection status on probe, without
>waiting for a message (thanks to Clément Vuchener for the tip).
>  * Removed the error code on redundant connection/disconnection messages. That
>was harmless but polluted dmesg.
>  * Added buttons for touching the left-pad and right-pad.
>  * Fixed a misplaced #include from 2/4 to 1/4.
>
> Changes in v3:
>  * Use RCU to do the dynamic connec/disconnect of wireless devices.
>  * Remove entries in hid-quirks.c as they are no longer needed. This allows
>this module to be blacklisted without side effects.
>  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>existing use cases (lizard mode). A user-space tool to do that is
>linked.
>  * Fully separated axes for joystick and left-pad. As it happens.
>  * Add fuzz values for left/right pad axes, they are a little wiggly.
>
> Changes in v2:
>  * Remove references to USB. Now the interesting interfaces are selected by
>looking for the ones with feature reports.
>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>  * Feature report length is checked, to avoid overflows in case of
>corrupt/malicius USB devices.
>  * Resolution added to the ABS axes.
>  * A lot of minor cleanups.
>
> Rodrigo Rivas Costa (4):
>   HID: add driver for Valve Steam Controller
>   HID: steam: add serial number information.
>   HID: steam: command to check wireless connection
>   HID: steam: add battery device.
>
>  drivers/hid/Kconfig |   8 +
>  drivers/hid/Makefile|   1 +
>  drivers/hid/hid-ids.h   |   4 +
>  drivers/hid/hid-steam.c | 794 
> 
>  4 files changed, 807 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.2
>

Re: [PATCH v3 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-02-26 10:50 GMT+01:00 Benjamin Tissoires :
> Hi Rodrigo,
>
> On Sun, Feb 25, 2018 at 7:52 PM, Rodrigo Rivas Costa
>  wrote:
>> This patchset implements a driver for Valve Steam Controller, based on a
>> reverse analysis by myself.
>
> To me, the code looks OK now. I haven't got the time to do a better
> review, so giving my:
> Acked-by: Bnejamin Tissoires 
>
> However, before we include it, I'd like to have the ACK from
> Pierre-Loup and Clément. They should be more qualified than me to say
> if this will interfere with the official Steam client (I think we are
> good now, but that's just my opinion).

I am not qualified to speak about the steam client, it has been a long
time since I have a look at how it uses the controller and I think it
changed a lot.

I checked the code and I think you are not handling wireless
connection correctly. If a wireless controller is already connected
when the driver is loaded, you will not receive a connection event and
forget to register the controller. You can send a 0xb4 request with
empty parameters (0xb4, 0x00, ... ) to force the receiver to send its
connection status (as the type 3 event you are already parsing). Since
user-space program may also send this request, you should make sure
the driver works if it receives a (dis)connected event when it is
already (dis)connected.

>
> Cheers,
> Benjamin
>
>>
>> This is reroll v3, codenamed "lazy lizard". Changes from v2:
>>  * Use RCU to do the dynamic connec/disconnect of wireless devices. Please,
>>to anybody that knows their way around RCU, review.
>>  * Remove entries in hid-quirks.c as they are no longer needed. This allows
>>this module to be blacklisted without side effects.
>>  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>>existing use cases (lizard mode). A user-space tool to do that is
>>linked.
>>  * Fully separated axes for joystick and left-pad. As it happens, there are
>>people with too many fingers.
>>  * Add fuzz values for left/right pad axes, they are a little wiggly.
>>
>> Notable changes from patchset v1:
>>  * Remove references to USB. Now the interesting interfaces are selected by
>>looking for the ones with feature reports.
>>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>>  * Feature report length is checked, to avoid overflows in case of
>>corrupt/malicius USB devices.
>>  * Resolution added to the ABS axes.
>>  * A lot of minor cleanups.
>>
>> Rodrigo Rivas Costa (3):
>>   HID: add driver for Valve Steam Controller
>>   HID: steam: add serial number information.
>>   HID: steam: add battery device.
>>
>>  drivers/hid/Kconfig |   8 +
>>  drivers/hid/Makefile|   1 +
>>  drivers/hid/hid-ids.h   |   4 +
>>  drivers/hid/hid-steam.c | 777 
>> 
>>  4 files changed, 790 insertions(+)
>>  create mode 100644 drivers/hid/hid-steam.c
>>
>> --
>> 2.16.2
>>

Re: [PATCH v3 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-02-26 10:50 GMT+01:00 Benjamin Tissoires :
> Hi Rodrigo,
>
> On Sun, Feb 25, 2018 at 7:52 PM, Rodrigo Rivas Costa
>  wrote:
>> This patchset implements a driver for Valve Steam Controller, based on a
>> reverse analysis by myself.
>
> To me, the code looks OK now. I haven't got the time to do a better
> review, so giving my:
> Acked-by: Bnejamin Tissoires 
>
> However, before we include it, I'd like to have the ACK from
> Pierre-Loup and Clément. They should be more qualified than me to say
> if this will interfere with the official Steam client (I think we are
> good now, but that's just my opinion).

I am not qualified to speak about the steam client, it has been a long
time since I have a look at how it uses the controller and I think it
changed a lot.

I checked the code and I think you are not handling wireless
connection correctly. If a wireless controller is already connected
when the driver is loaded, you will not receive a connection event and
forget to register the controller. You can send a 0xb4 request with
empty parameters (0xb4, 0x00, ... ) to force the receiver to send its
connection status (as the type 3 event you are already parsing). Since
user-space program may also send this request, you should make sure
the driver works if it receives a (dis)connected event when it is
already (dis)connected.

>
> Cheers,
> Benjamin
>
>>
>> This is reroll v3, codenamed "lazy lizard". Changes from v2:
>>  * Use RCU to do the dynamic connec/disconnect of wireless devices. Please,
>>to anybody that knows their way around RCU, review.
>>  * Remove entries in hid-quirks.c as they are no longer needed. This allows
>>this module to be blacklisted without side effects.
>>  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>>existing use cases (lizard mode). A user-space tool to do that is
>>linked.
>>  * Fully separated axes for joystick and left-pad. As it happens, there are
>>people with too many fingers.
>>  * Add fuzz values for left/right pad axes, they are a little wiggly.
>>
>> Notable changes from patchset v1:
>>  * Remove references to USB. Now the interesting interfaces are selected by
>>looking for the ones with feature reports.
>>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>>  * Feature report length is checked, to avoid overflows in case of
>>corrupt/malicius USB devices.
>>  * Resolution added to the ABS axes.
>>  * A lot of minor cleanups.
>>
>> Rodrigo Rivas Costa (3):
>>   HID: add driver for Valve Steam Controller
>>   HID: steam: add serial number information.
>>   HID: steam: add battery device.
>>
>>  drivers/hid/Kconfig |   8 +
>>  drivers/hid/Makefile|   1 +
>>  drivers/hid/hid-ids.h   |   4 +
>>  drivers/hid/hid-steam.c | 777 
>> 
>>  4 files changed, 790 insertions(+)
>>  create mode 100644 drivers/hid/hid-steam.c
>>
>> --
>> 2.16.2
>>

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-02-22 1:13 GMT+01:00 Pierre-Loup A. Griffais :
>
> On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>>
>> Related to that, Benjamin Tissoires replied to 1/3:

 --- a/drivers/hid/hid-quirks.c
 +++ b/drivers/hid/hid-quirks.c
 @@ -629,6 +629,10 @@ static const struct hid_device_id
 hid_have_special_driver[] = {
   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
  { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
  USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
   #endif
 +#if IS_ENABLED(CONFIG_HID_STEAM)
 +   { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 USB_DEVICE_ID_STEAM_CONTROLLER) },
 +   { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
 +#endif
>>>
>>>
>>> In addition to the discussion in 0/3, I wonder if you should not
>>> remove this hunk. Unless having hid-generic binding the device before
>>> your hid-steam driver, it would be better not force the Steam boxes to
>>> use your driver.
>>
>>
>> I don't really see the point on that. If we do that I'll have to unbind
>> and bind the device manually, and that is a pain, and impossible without
>> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
>>
>> And anyway this driver is mostly harmless, the only visible changes from
>> userspace are the new input and power devices, and that the virtual
>> keyboard/mouse are no more. If the virtual devices are really missed we
>> could use:
>>
>> hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>
>> on them, maybe with a module parameter? Note that one of the first
>> things the Steam Client does is to disable the virtual devices (with a
>> command to the device).
>> (TBH I always had the impression that those virtual devices are there
>> to move the Grub menu...)
>>
>> If Valve people really feel that this driver is not needed for SteamOS,
>> because the Steam Client is always running, then SteamOS can always do
>> CONFIG_HID_STEAM=n.
>
>
> Yes, certainly; that isn't really the usecase I'm worried about. What I'm
> worried about is behavior changing for existing users of the controller on
> normal desktop distributions. Currently the Steam client will program these
> pieces of state (enable/disable direct keyboard/mouse HID functionality,
> enable/disable gyro/accel) based on the user's configuration, and a user
> getting a kernel update that includes a driver that also programs that
> without their intervention is bound to affect the behavior. If there was a
> way to make it so the states won't be programmed until it's pretty clear the
> user is trying to use that driver's functionality, that would feel safer.

hid_have_special_driver is no longer required (see patch "HID: core:
remove the absolute need of hid_have_special_driver[]" [1]). If I
understand Benjamin's intent correctly, the driver will still be used,
but if you have a conflict you can simply unload/blacklist the special
driver module and hid-generic will handle the device. This way there
is no need to recompile the kernel to disable the special driver while
keeping standard HID features working. User-space driver packagers
(e.g. steam packagers) could simply add a modprobe conf file to
blacklist the module and avoid conflicts.

[1]: https://patchwork.kernel.org/patch/10066303/

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-02-22 1:13 GMT+01:00 Pierre-Loup A. Griffais :
>
> On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>>
>> Related to that, Benjamin Tissoires replied to 1/3:

 --- a/drivers/hid/hid-quirks.c
 +++ b/drivers/hid/hid-quirks.c
 @@ -629,6 +629,10 @@ static const struct hid_device_id
 hid_have_special_driver[] = {
   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
  { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
  USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
   #endif
 +#if IS_ENABLED(CONFIG_HID_STEAM)
 +   { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 USB_DEVICE_ID_STEAM_CONTROLLER) },
 +   { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
 +#endif
>>>
>>>
>>> In addition to the discussion in 0/3, I wonder if you should not
>>> remove this hunk. Unless having hid-generic binding the device before
>>> your hid-steam driver, it would be better not force the Steam boxes to
>>> use your driver.
>>
>>
>> I don't really see the point on that. If we do that I'll have to unbind
>> and bind the device manually, and that is a pain, and impossible without
>> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
>>
>> And anyway this driver is mostly harmless, the only visible changes from
>> userspace are the new input and power devices, and that the virtual
>> keyboard/mouse are no more. If the virtual devices are really missed we
>> could use:
>>
>> hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>
>> on them, maybe with a module parameter? Note that one of the first
>> things the Steam Client does is to disable the virtual devices (with a
>> command to the device).
>> (TBH I always had the impression that those virtual devices are there
>> to move the Grub menu...)
>>
>> If Valve people really feel that this driver is not needed for SteamOS,
>> because the Steam Client is always running, then SteamOS can always do
>> CONFIG_HID_STEAM=n.
>
>
> Yes, certainly; that isn't really the usecase I'm worried about. What I'm
> worried about is behavior changing for existing users of the controller on
> normal desktop distributions. Currently the Steam client will program these
> pieces of state (enable/disable direct keyboard/mouse HID functionality,
> enable/disable gyro/accel) based on the user's configuration, and a user
> getting a kernel update that includes a driver that also programs that
> without their intervention is bound to affect the behavior. If there was a
> way to make it so the states won't be programmed until it's pretty clear the
> user is trying to use that driver's functionality, that would feel safer.

hid_have_special_driver is no longer required (see patch "HID: core:
remove the absolute need of hid_have_special_driver[]" [1]). If I
understand Benjamin's intent correctly, the driver will still be used,
but if you have a conflict you can simply unload/blacklist the special
driver module and hid-generic will handle the device. This way there
is no need to recompile the kernel to disable the special driver while
keeping standard HID features working. User-space driver packagers
(e.g. steam packagers) could simply add a modprobe conf file to
blacklist the module and avoid conflicts.

[1]: https://patchwork.kernel.org/patch/10066303/

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

Hi Rodrigo,

I have written a kernel driver [1], some time ago. I did not submit it
for merging in the main-line because I thought that would mess with
user-space drivers. If your driver create an input device, a
user-space driver will have to disable it before creating its own. I
guess libusb based drivers will detach the kernel driver and will be
fine. But it is not as simple if you use hidraw. And there may be some
state problem as Pierre-Loup already said.

A few more tips about your discussion with Pierre-Loup (sorry I am not
replying to the answers directly, it's hard to insert myself in this
long discussion):

Beside Ynsta's driver, you may have a look at my own user-space driver
[2] or Dennis Hamester's library scraw [3].

You can enable and disable the accel and gyro when the separate input
device is opened or closed to save battery. My kernel driver does it,
IIRC I have taken the idea from the wiimote driver. Note that there is
an issue with joydev using the accel/gyro input device. hid-sony
solved it by adding an exception in joydev driver (joydev_blacklist in
drivers/input/joydev.c).

In my driver, I split the left pad and left stick events by looking at
the left pad touch bit. It works well as long as they are not used at
the same time. When they both being used the data alternate between
left pad and left stick data, the touch bit is set accordingly so axis
data is easy to use, but it looks like the left touch pad is being
tapped very fast.

[1]: https://github.com/cvuchener/steamcontroller-linux-kernel
[2]: 
https://github.com/cvuchener/input-scripts/tree/master/src/daemon/steamcontroller
[3]: https://gitlab.com/dennis-hamester/scraw)

2018-02-20 20:33 GMT+01:00 Rodrigo Rivas Costa :
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
>
> Notable changes from patchset v1:
>  * Remove references to USB. Now the interesting interfaces are selected by
>looking for the ones with feature reports.
>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>  * Feature report length is checked, to avoid overflows in case of
>corrupt/malicius USB devices.
>  * Resolution added to the ABS axes.
>  * A lot of minor cleanups.
>
> Rodrigo Rivas Costa (3):
>   HID: add driver for Valve Steam Controller
>   HID: steam: add serial number information.
>   HID: steam: add battery device.
>
>  drivers/hid/Kconfig  |   8 +
>  drivers/hid/Makefile |   1 +
>  drivers/hid/hid-ids.h|   4 +
>  drivers/hid/hid-quirks.c |   4 +
>  drivers/hid/hid-steam.c  | 703 
> +++
>  5 files changed, 720 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

Hi Rodrigo,

I have written a kernel driver [1], some time ago. I did not submit it
for merging in the main-line because I thought that would mess with
user-space drivers. If your driver create an input device, a
user-space driver will have to disable it before creating its own. I
guess libusb based drivers will detach the kernel driver and will be
fine. But it is not as simple if you use hidraw. And there may be some
state problem as Pierre-Loup already said.

A few more tips about your discussion with Pierre-Loup (sorry I am not
replying to the answers directly, it's hard to insert myself in this
long discussion):

Beside Ynsta's driver, you may have a look at my own user-space driver
[2] or Dennis Hamester's library scraw [3].

You can enable and disable the accel and gyro when the separate input
device is opened or closed to save battery. My kernel driver does it,
IIRC I have taken the idea from the wiimote driver. Note that there is
an issue with joydev using the accel/gyro input device. hid-sony
solved it by adding an exception in joydev driver (joydev_blacklist in
drivers/input/joydev.c).

In my driver, I split the left pad and left stick events by looking at
the left pad touch bit. It works well as long as they are not used at
the same time. When they both being used the data alternate between
left pad and left stick data, the touch bit is set accordingly so axis
data is easy to use, but it looks like the left touch pad is being
tapped very fast.

[1]: https://github.com/cvuchener/steamcontroller-linux-kernel
[2]: 
https://github.com/cvuchener/input-scripts/tree/master/src/daemon/steamcontroller
[3]: https://gitlab.com/dennis-hamester/scraw)

2018-02-20 20:33 GMT+01:00 Rodrigo Rivas Costa :
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
>
> Notable changes from patchset v1:
>  * Remove references to USB. Now the interesting interfaces are selected by
>looking for the ones with feature reports.
>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>  * Feature report length is checked, to avoid overflows in case of
>corrupt/malicius USB devices.
>  * Resolution added to the ABS axes.
>  * A lot of minor cleanups.
>
> Rodrigo Rivas Costa (3):
>   HID: add driver for Valve Steam Controller
>   HID: steam: add serial number information.
>   HID: steam: add battery device.
>
>  drivers/hid/Kconfig  |   8 +
>  drivers/hid/Makefile |   1 +
>  drivers/hid/hid-ids.h|   4 +
>  drivers/hid/hid-quirks.c |   4 +
>  drivers/hid/hid-steam.c  | 703 
> +++
>  5 files changed, 720 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RE][PATCH] drivers: input: joystick: Add PSX(Play Staion 1/2) pad with SPI driver Add PSX(Play Staion 1/2) pad with SPI driver. Pads can be connected directry SPI bus.

[Clément VUCHENER]

2017-04-21 1:15 GMT+02:00 AZO :
> To Linux kernel input driver mainteners
>
> I mailed to mainteners a week ago.
> Please can anyone answer?

You should send the email directly to the maintainers in addition to
the mailing list. You can use the get_maintainer.pl script to know who
that is.

>
> Regard.
>
> =
> AZO

Re: [RE][PATCH] drivers: input: joystick: Add PSX(Play Staion 1/2) pad with SPI driver Add PSX(Play Staion 1/2) pad with SPI driver. Pads can be connected directry SPI bus.

[Clément VUCHENER]

2017-04-21 1:15 GMT+02:00 AZO :
> To Linux kernel input driver mainteners
>
> I mailed to mainteners a week ago.
> Please can anyone answer?

You should send the email directly to the maintainers in addition to
the mailing list. You can use the get_maintainer.pl script to know who
that is.

>
> Regard.
>
> =
> AZO

Re: [PATCH 0/2] hid: corsair: Driver simplification and new supported device

[Clément VUCHENER]

2016-03-24 15:30 GMT+01:00 Jiri Kosina :
> On Wed, 23 Mar 2016, =?UTF-8?q?Cl=C3=A9ment=20Vuchener?= wrote:
>
>> So, I decided to move all USB related features in user-space (as far as
>> I know, I was the only user, but if someone is looking for a
>> replacement, I wrote a small tool available here:
>> https://github.com/cvuchener/corsair-usb-config). This simplification
>> only leaves the usage code remapping part and the driver no longer
>> depends on USB and LED subsystems. This should make the driver easier to
>> maintain or to add new supported devices.
>
> While you are performing this move, is there anything that's actually
> preventing you from doing the remapping from userspace as well?
>
> HID subsystem has for long time been providing the setkeycode() hook for
> remapping usages, and udev (well, more precisely, that s*d thing) is
> actually shipping a lot of hw-specific remap data these days.

Thanks for the tip, is it possible to ignore some usages with this
method (done in the default case)?

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH 0/2] hid: corsair: Driver simplification and new supported device

[Clément VUCHENER]

2016-03-24 15:30 GMT+01:00 Jiri Kosina :
> On Wed, 23 Mar 2016, =?UTF-8?q?Cl=C3=A9ment=20Vuchener?= wrote:
>
>> So, I decided to move all USB related features in user-space (as far as
>> I know, I was the only user, but if someone is looking for a
>> replacement, I wrote a small tool available here:
>> https://github.com/cvuchener/corsair-usb-config). This simplification
>> only leaves the usage code remapping part and the driver no longer
>> depends on USB and LED subsystems. This should make the driver easier to
>> maintain or to add new supported devices.
>
> While you are performing this move, is there anything that's actually
> preventing you from doing the remapping from userspace as well?
>
> HID subsystem has for long time been providing the setkeycode() hook for
> remapping usages, and udev (well, more precisely, that s*d thing) is
> actually shipping a lot of hw-specific remap data these days.

Thanks for the tip, is it possible to ignore some usages with this
method (done in the default case)?

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

[RESEND PATCH v3 1/1] Add Corsair Vengeance K90 driver

[Clément Vuchener]

This patch implements a HID driver for the Corsair Vengeance K90 keyboard.

It fixes the behaviour of the keys using incorrect HID usage codes and exposes 
the macro playback mode and current profile to the user space through sysfs 
attributes. It also adds two LED class devices controlling the "record" LED and 
the backlight.

Signed-off-by: Clément Vuchener 
---
 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 673 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 703 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair 
b/Documentation/ABI/testing/sysfs-driver-hid-corsair
new file mode 100644
index 000..b8827f0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-corsair
@@ -0,0 +1,15 @@
+What:  /sys/bus/drivers/corsair//macro_mode
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get/set the current playback mode. "SW" for software mode
+   where G-keys triggers their regular key codes. "HW" for
+   hardware playback mode where the G-keys play their macro
+   from the on-board memory.
+
+
+What:  /sys/bus/drivers/corsair//current_profile
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 6ab51ae..3fe9678 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -171,6 +171,16 @@ config HID_CHICONY
---help---
Support for Chicony Tactical pad.
 
+config HID_CORSAIR
+   tristate "Corsair devices"
+   depends on HID && USB && LEDS_CLASS
+   ---help---
+   Support for Corsair devices that are not fully compliant with the
+   HID standard.
+
+   Supported devices:
+   - Vengeance K90
+
 config HID_PRODIKEYS
tristate "Prodikeys PC-MIDI Keyboard support"
depends on HID && SND
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e6441bc..edaa0f2 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_HID_BELKIN)  += hid-belkin.o
 obj-$(CONFIG_HID_BETOP_FF) += hid-betopff.o
 obj-$(CONFIG_HID_CHERRY)   += hid-cherry.o
 obj-$(CONFIG_HID_CHICONY)  += hid-chicony.o
+obj-$(CONFIG_HID_CORSAIR)  += hid-corsair.o
 obj-$(CONFIG_HID_CP2112)   += hid-cp2112.o
 obj-$(CONFIG_HID_CYPRESS)  += hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 70a11ac..0e3baae 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1828,6 +1828,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_ACER_SWITCH12) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, 
USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, 
USB_DEVICE_ID_CYPRESS_BARCODE_1) },
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
new file mode 100644
index 000..cd3429e
--- /dev/null
+++ b/drivers/hid/hid-corsair.c
@@ -0,0 +1,673 @@
+/*
+ * HID driver for Corsair devices
+ *
+ * Supported devices:
+ *  - Vengeance K90 Keyboard
+ *
+ * Copyright (c) 2015 Clement Vuchener
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+#define CORSAIR_USE_K90_MACRO  (1<<0)
+#define CORSAIR_USE_K90_BACKLIGHT  (1<<1)
+
+struct k90_led {
+   struct led_classdev cdev;
+   int brightness;
+   struct work_struct work;
+   int removed;
+};
+
+struct k90_drvdata {
+   struct k90_led record_led;
+};
+
+struct corsair_drvdata {
+   unsigned long quirks;
+   struct k90_drvdata *k90;
+   struct k90_led *backlight;
+};
+
+#define K90_GKEY_COUNT 18
+
+static int corsair_usage_to_gkey(unsigne

[RESEND PATCH v3 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

I have split the special functions between backlight and macro functions. This 
should make it easier to
test new devices. I think the macro functions will only be reused with the K95. 
While backlight is more
common feature, though I have no idea it is done with other Corsair hardware.

I have changed most sysfs attributes and LEDs so that the current value is 
queried from the hardware instead
of tracking events. I started this way when I did not know how to read the 
value, but I think it is better done
this way when I can. I don't know how to read the state of the record LED, so 
this one still use events to update
the state.

I removed the color from the LEDs name. I understand it is only necessary when 
having LEDs with several
colors. This way the names will stay the same across different hardware with 
different backlight color. I
don't think it is an useful information here.

I also added event for the MR (macro record) button and profile switch buttons. 
I think that userspace
program may want to know about these events. For example for using profile keys 
to start some
configuration program.

changes in v3:
 - query the hardware instead of tracking the value with events when possible 
(except record_led)
 - added quirks for activating special functions (macro functions and backlight)
 - allocation  of led name use kzalloc instead of devm_kzalloc (free mem when 
initialization failed)
 - renamed led devices (without colors)
 - added key codes for record and profile keys

changes in v2:
 - Removed the k90_profile class and devices
 - Renamed driver for a more generic name ("corsair" driver in hid-corsair.c)
 - Fixed led devices clean up (hang when unplugging and led state reset)
 - Added dependency on USB and LEDS_CLASS in Kconfig

Clément Vuchener (1):
  Add Corsair Vengeance K90 driver

 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 673 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 703 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RESEND PATCH v3 1/1] Add Corsair Vengeance K90 driver

[Clément Vuchener]

This patch implements a HID driver for the Corsair Vengeance K90 keyboard.

It fixes the behaviour of the keys using incorrect HID usage codes and exposes 
the macro playback mode and current profile to the user space through sysfs 
attributes. It also adds two LED class devices controlling the "record" LED and 
the backlight.

Signed-off-by: Clément Vuchener <clement.vuche...@gmail.com>
---
 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 673 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 703 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair 
b/Documentation/ABI/testing/sysfs-driver-hid-corsair
new file mode 100644
index 000..b8827f0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-corsair
@@ -0,0 +1,15 @@
+What:  /sys/bus/drivers/corsair//macro_mode
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener <clement.vuche...@gmail.com>
+Description:   Get/set the current playback mode. "SW" for software mode
+   where G-keys triggers their regular key codes. "HW" for
+   hardware playback mode where the G-keys play their macro
+   from the on-board memory.
+
+
+What:  /sys/bus/drivers/corsair//current_profile
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener <clement.vuche...@gmail.com>
+Description:   Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 6ab51ae..3fe9678 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -171,6 +171,16 @@ config HID_CHICONY
---help---
Support for Chicony Tactical pad.
 
+config HID_CORSAIR
+   tristate "Corsair devices"
+   depends on HID && USB && LEDS_CLASS
+   ---help---
+   Support for Corsair devices that are not fully compliant with the
+   HID standard.
+
+   Supported devices:
+   - Vengeance K90
+
 config HID_PRODIKEYS
tristate "Prodikeys PC-MIDI Keyboard support"
depends on HID && SND
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e6441bc..edaa0f2 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_HID_BELKIN)  += hid-belkin.o
 obj-$(CONFIG_HID_BETOP_FF) += hid-betopff.o
 obj-$(CONFIG_HID_CHERRY)   += hid-cherry.o
 obj-$(CONFIG_HID_CHICONY)  += hid-chicony.o
+obj-$(CONFIG_HID_CORSAIR)  += hid-corsair.o
 obj-$(CONFIG_HID_CP2112)   += hid-cp2112.o
 obj-$(CONFIG_HID_CYPRESS)  += hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 70a11ac..0e3baae 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1828,6 +1828,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_ACER_SWITCH12) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, 
USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, 
USB_DEVICE_ID_CYPRESS_BARCODE_1) },
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
new file mode 100644
index 000..cd3429e
--- /dev/null
+++ b/drivers/hid/hid-corsair.c
@@ -0,0 +1,673 @@
+/*
+ * HID driver for Corsair devices
+ *
+ * Supported devices:
+ *  - Vengeance K90 Keyboard
+ *
+ * Copyright (c) 2015 Clement Vuchener
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+#define CORSAIR_USE_K90_MACRO  (1<<0)
+#define CORSAIR_USE_K90_BACKLIGHT  (1<<1)
+
+struct k90_led {
+   struct led_classdev cdev;
+   int brightness;
+   struct work_struct work;
+   int removed;
+};
+
+struct k90_drvdata {
+   struct k90_led record_led;
+};
+
+struct corsair_drvdata {
+   unsigned long quirks;
+   struct k90_drvdata *k90;
+   

[RESEND PATCH v3 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

I have split the special functions between backlight and macro functions. This 
should make it easier to
test new devices. I think the macro functions will only be reused with the K95. 
While backlight is more
common feature, though I have no idea it is done with other Corsair hardware.

I have changed most sysfs attributes and LEDs so that the current value is 
queried from the hardware instead
of tracking events. I started this way when I did not know how to read the 
value, but I think it is better done
this way when I can. I don't know how to read the state of the record LED, so 
this one still use events to update
the state.

I removed the color from the LEDs name. I understand it is only necessary when 
having LEDs with several
colors. This way the names will stay the same across different hardware with 
different backlight color. I
don't think it is an useful information here.

I also added event for the MR (macro record) button and profile switch buttons. 
I think that userspace
program may want to know about these events. For example for using profile keys 
to start some
configuration program.

changes in v3:
 - query the hardware instead of tracking the value with events when possible 
(except record_led)
 - added quirks for activating special functions (macro functions and backlight)
 - allocation  of led name use kzalloc instead of devm_kzalloc (free mem when 
initialization failed)
 - renamed led devices (without colors)
 - added key codes for record and profile keys

changes in v2:
 - Removed the k90_profile class and devices
 - Renamed driver for a more generic name ("corsair" driver in hid-corsair.c)
 - Fixed led devices clean up (hang when unplugging and led state reset)
 - Added dependency on USB and LEDS_CLASS in Kconfig

Clément Vuchener (1):
  Add Corsair Vengeance K90 driver

 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 673 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 703 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

On Mon, Sep 07, 2015 at 05:58:36PM +0200, Clément Vuchener wrote:
> I removed the k90_profile class completely. I cannot write a good enough ABI 
> with what I know of the keyboard so I am leaving that part out of the kernel. 
> If I change my mind in the future, it will be done in another patch.
> 
> I also fixed a bug I had when unregistering the led device. Work was being 
> scheduled after the led device was unregistered.
> 
> On the name change, I kept a lot of K90 references. As far as I know, the 
> only similar keyboard is the K60 that shares the same firmware but does not 
> have all the special keys and backlight, and for which the hid-generic driver 
> should be enough. The more recent RGB keyboard series uses a different 
> protocol from what I have seen from the unofficial userspace driver (CKB from 
> MSC).
Actually, I may have been wrong, other keyboard may share at least the 
incorrect HID usage code part. I will investigate more and send a new patch. I 
am still interested in comments on this one, especially on the choice of key 
codes for the programmable keys.
> 
> changes in v2:
>  - Removed the k90_profile class and devices
>  - Renamed driver for a more generic name ("corsair" driver in hid-corsair.c)
>  - Fixed led devices clean up (hang when unplugging and led state reset)
>  - Added dependency on USB and LEDS_CLASS in Kconfig
> 
> Clément Vuchener (1):
>   Add Corsair Vengeance K90 driver
> 
>  Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
>  drivers/hid/Kconfig|  10 +
>  drivers/hid/Makefile   |   1 +
>  drivers/hid/hid-core.c |   1 +
>  drivers/hid/hid-corsair.c  | 555 
> +
>  drivers/hid/hid-ids.h  |   3 +
>  6 files changed, 585 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
>  create mode 100644 drivers/hid/hid-corsair.c
> 
> -- 
> 2.4.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

On Mon, Sep 07, 2015 at 05:58:36PM +0200, Clément Vuchener wrote:
> I removed the k90_profile class completely. I cannot write a good enough ABI 
> with what I know of the keyboard so I am leaving that part out of the kernel. 
> If I change my mind in the future, it will be done in another patch.
> 
> I also fixed a bug I had when unregistering the led device. Work was being 
> scheduled after the led device was unregistered.
> 
> On the name change, I kept a lot of K90 references. As far as I know, the 
> only similar keyboard is the K60 that shares the same firmware but does not 
> have all the special keys and backlight, and for which the hid-generic driver 
> should be enough. The more recent RGB keyboard series uses a different 
> protocol from what I have seen from the unofficial userspace driver (CKB from 
> MSC).
Actually, I may have been wrong, other keyboard may share at least the 
incorrect HID usage code part. I will investigate more and send a new patch. I 
am still interested in comments on this one, especially on the choice of key 
codes for the programmable keys.
> 
> changes in v2:
>  - Removed the k90_profile class and devices
>  - Renamed driver for a more generic name ("corsair" driver in hid-corsair.c)
>  - Fixed led devices clean up (hang when unplugging and led state reset)
>  - Added dependency on USB and LEDS_CLASS in Kconfig
> 
> Clément Vuchener (1):
>   Add Corsair Vengeance K90 driver
> 
>  Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
>  drivers/hid/Kconfig|  10 +
>  drivers/hid/Makefile   |   1 +
>  drivers/hid/hid-core.c |   1 +
>  drivers/hid/hid-corsair.c  | 555 
> +
>  drivers/hid/hid-ids.h  |   3 +
>  6 files changed, 585 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
>  create mode 100644 drivers/hid/hid-corsair.c
> 
> -- 
> 2.4.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/1] Add Corsair Vengeance K90 driver

[Clément Vuchener]

This patch implements a HID driver for the Corsair Vengeance K90 keyboard. 

It fixes the behaviour of the keys using incorrect HID usage codes and exposes 
the macro playback mode and current profile to the user space through sysfs 
attributes. It also adds two LED class devices controlling the "record" LED and 
the backlight.

Signed-off-by: Clément Vuchener 
---
 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 555 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 585 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair 
b/Documentation/ABI/testing/sysfs-driver-hid-corsair
new file mode 100644
index 000..b8827f0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-corsair
@@ -0,0 +1,15 @@
+What:  /sys/bus/drivers/corsair//macro_mode
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get/set the current playback mode. "SW" for software mode
+   where G-keys triggers their regular key codes. "HW" for
+   hardware playback mode where the G-keys play their macro
+   from the on-board memory.
+
+
+What:  /sys/bus/drivers/corsair//current_profile
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 6ab51ae..3fe9678 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -171,6 +171,16 @@ config HID_CHICONY
---help---
Support for Chicony Tactical pad.
 
+config HID_CORSAIR
+   tristate "Corsair devices"
+   depends on HID && USB && LEDS_CLASS
+   ---help---
+   Support for Corsair devices that are not fully compliant with the
+   HID standard.
+
+   Supported devices:
+   - Vengeance K90
+
 config HID_PRODIKEYS
tristate "Prodikeys PC-MIDI Keyboard support"
depends on HID && SND
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e6441bc..edaa0f2 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_HID_BELKIN)  += hid-belkin.o
 obj-$(CONFIG_HID_BETOP_FF) += hid-betopff.o
 obj-$(CONFIG_HID_CHERRY)   += hid-cherry.o
 obj-$(CONFIG_HID_CHICONY)  += hid-chicony.o
+obj-$(CONFIG_HID_CORSAIR)  += hid-corsair.o
 obj-$(CONFIG_HID_CP2112)   += hid-cp2112.o
 obj-$(CONFIG_HID_CYPRESS)  += hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bcd914a..d5fc4d1 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1828,6 +1828,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_ACER_SWITCH12) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, 
USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, 
USB_DEVICE_ID_CYPRESS_BARCODE_1) },
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
new file mode 100644
index 000..580c214
--- /dev/null
+++ b/drivers/hid/hid-corsair.c
@@ -0,0 +1,555 @@
+/*
+ * HID driver for Corsair devices
+ *
+ * Supported devices:
+ *  - Vengeance K90 Keyboard
+ *
+ * Copyright (c) 2015 Clement Vuchener
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+struct k90_led {
+   struct led_classdev cdev;
+   int brightness;
+   struct work_struct work;
+   int removed;
+};
+
+struct k90_drvdata {
+   int current_profile;
+   int macro_mode;
+   int meta_locked;
+   struct k90_led backlight;
+   struct k90_led record_led;
+};
+
+#define K90_GKEY_COUNT 18
+
+static int k90_usage_to_gkey(unsigned int usage)
+{
+   /* G1 (0xd0) to G16 (0xdf) */
+   if (usage >= 0xd0 && usage <= 0xdf)
+   

[PATCH v2 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

I removed the k90_profile class completely. I cannot write a good enough ABI 
with what I know of the keyboard so I am leaving that part out of the kernel. 
If I change my mind in the future, it will be done in another patch.

I also fixed a bug I had when unregistering the led device. Work was being 
scheduled after the led device was unregistered.

On the name change, I kept a lot of K90 references. As far as I know, the only 
similar keyboard is the K60 that shares the same firmware but does not have all 
the special keys and backlight, and for which the hid-generic driver should be 
enough. The more recent RGB keyboard series uses a different protocol from what 
I have seen from the unofficial userspace driver (CKB from MSC).

changes in v2:
 - Removed the k90_profile class and devices
 - Renamed driver for a more generic name ("corsair" driver in hid-corsair.c)
 - Fixed led devices clean up (hang when unplugging and led state reset)
 - Added dependency on USB and LEDS_CLASS in Kconfig

Clément Vuchener (1):
  Add Corsair Vengeance K90 driver

 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 555 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 585 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

I removed the k90_profile class completely. I cannot write a good enough ABI 
with what I know of the keyboard so I am leaving that part out of the kernel. 
If I change my mind in the future, it will be done in another patch.

I also fixed a bug I had when unregistering the led device. Work was being 
scheduled after the led device was unregistered.

On the name change, I kept a lot of K90 references. As far as I know, the only 
similar keyboard is the K60 that shares the same firmware but does not have all 
the special keys and backlight, and for which the hid-generic driver should be 
enough. The more recent RGB keyboard series uses a different protocol from what 
I have seen from the unofficial userspace driver (CKB from MSC).

changes in v2:
 - Removed the k90_profile class and devices
 - Renamed driver for a more generic name ("corsair" driver in hid-corsair.c)
 - Fixed led devices clean up (hang when unplugging and led state reset)
 - Added dependency on USB and LEDS_CLASS in Kconfig

Clément Vuchener (1):
  Add Corsair Vengeance K90 driver

 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 555 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 585 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/1] Add Corsair Vengeance K90 driver

[Clément Vuchener]

This patch implements a HID driver for the Corsair Vengeance K90 keyboard. 

It fixes the behaviour of the keys using incorrect HID usage codes and exposes 
the macro playback mode and current profile to the user space through sysfs 
attributes. It also adds two LED class devices controlling the "record" LED and 
the backlight.

Signed-off-by: Clément Vuchener <clement.vuche...@gmail.com>
---
 Documentation/ABI/testing/sysfs-driver-hid-corsair |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair.c  | 555 +
 drivers/hid/hid-ids.h  |   3 +
 6 files changed, 585 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
 create mode 100644 drivers/hid/hid-corsair.c

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair 
b/Documentation/ABI/testing/sysfs-driver-hid-corsair
new file mode 100644
index 000..b8827f0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-corsair
@@ -0,0 +1,15 @@
+What:  /sys/bus/drivers/corsair//macro_mode
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener <clement.vuche...@gmail.com>
+Description:   Get/set the current playback mode. "SW" for software mode
+   where G-keys triggers their regular key codes. "HW" for
+   hardware playback mode where the G-keys play their macro
+   from the on-board memory.
+
+
+What:  /sys/bus/drivers/corsair//current_profile
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener <clement.vuche...@gmail.com>
+Description:   Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 6ab51ae..3fe9678 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -171,6 +171,16 @@ config HID_CHICONY
---help---
Support for Chicony Tactical pad.
 
+config HID_CORSAIR
+   tristate "Corsair devices"
+   depends on HID && USB && LEDS_CLASS
+   ---help---
+   Support for Corsair devices that are not fully compliant with the
+   HID standard.
+
+   Supported devices:
+   - Vengeance K90
+
 config HID_PRODIKEYS
tristate "Prodikeys PC-MIDI Keyboard support"
depends on HID && SND
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e6441bc..edaa0f2 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_HID_BELKIN)  += hid-belkin.o
 obj-$(CONFIG_HID_BETOP_FF) += hid-betopff.o
 obj-$(CONFIG_HID_CHERRY)   += hid-cherry.o
 obj-$(CONFIG_HID_CHICONY)  += hid-chicony.o
+obj-$(CONFIG_HID_CORSAIR)  += hid-corsair.o
 obj-$(CONFIG_HID_CP2112)   += hid-cp2112.o
 obj-$(CONFIG_HID_CYPRESS)  += hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bcd914a..d5fc4d1 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1828,6 +1828,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, 
USB_DEVICE_ID_CHICONY_ACER_SWITCH12) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, 
USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, 
USB_DEVICE_ID_CYPRESS_BARCODE_1) },
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
new file mode 100644
index 000..580c214
--- /dev/null
+++ b/drivers/hid/hid-corsair.c
@@ -0,0 +1,555 @@
+/*
+ * HID driver for Corsair devices
+ *
+ * Supported devices:
+ *  - Vengeance K90 Keyboard
+ *
+ * Copyright (c) 2015 Clement Vuchener
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+struct k90_led {
+   struct led_classdev cdev;
+   int brightness;
+   struct work_struct work;
+   int removed;
+};
+
+struct k90_drvdata {
+   int current_profile;
+   int macro_mode;
+   int meta_locked;
+   struct k90_led backlight;
+   struct k90_led record_led;
+};
+
+#define K90_GKEY_COUNT 18
+
+static int k90_usage_to_gkey(unsigned int usage)
+{
+   

Re: [PATCH 1/1] Corsair Vengeance K90 driver

[Clément Vuchener]

On 09/04/2015 03:27 PM, Jiri Kosina wrote:
> On Sat, 29 Aug 2015, Clément Vuchener wrote:
>
>
> This is missing changelog and Signed-off-by: line. I know that you have 
> provided that in your cover letter, but cover letter never make it to the 
> actual GIT commits.
>
> So please fix that and resend the patch, thanks.
>
>> ---
>>  Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
>>  .../ABI/testing/sysfs-driver-hid-corsair-k90   |  15 +
>>  drivers/hid/Kconfig|  10 +
>>  drivers/hid/Makefile   |   1 +
>>  drivers/hid/hid-core.c |   1 +
>>  drivers/hid/hid-corsair-k90.c  | 690
>> +
>>  drivers/hid/hid-ids.h  |   3 +
>>  7 files changed, 775 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
>>  create mode 100644 drivers/hid/hid-corsair-k90.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile
>> b/Documentation/ABI/testing/sysfs-class-k90_profile
>> new file mode 100644
>> index 000..3275c16
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-k90_profile
>> @@ -0,0 +1,55 @@
>> +What:   /sys/class/k90_profile//profile_number
>> +Date:   August 2015
>> +KernelVersion:  4.2
>> +Contact:Clement Vuchener 
>> +Description:Get the number of the profile.
>> +
>> +What:   /sys/class/k90_profile//bindings
>> +Date:   August 2015
>> +KernelVersion:  4.2
>> +Contact:Clement Vuchener 
>> +Description:Write bindings to the keyboard on-board profile.
>> +The data structure is:
>> + - number of bindings in this structure (1 byte)
>> + - size of this data structure (2 bytes big endian)
>> + - size of the macro data written to
>> +   /sys/class/k90_profile//data (2 bytes big endian)
>> + - array of bindings referencing the data from
>> +   /sys/class/k90_profile//data, each containing:
>> +   * 0x10 for a key usage, 0x20 for a macro (1 byte)
>> +   * offset of the key usage/macro data (2 bytes big endian)
>> +   * length of the key usage/macro data (2 bytes big endian)
>> +
> This looks like violation of one-value-per-attribute rule for sysfs ABI. 
> Could you please think about it once again with this aspect on mind?
Per key attributes would be nice but I don't think I can do that. The profile 
must be written all at once and I don't know any way to read it from the 
hardware (the windows driver I studied does not do it). So writing one value 
only would erase all the others.
I think I will remove this part from the driver. The same thing can easily be 
done in user space through libusb and writing profile should be exceptional 
enough that it is not a problem to detach the driver while doing it. That part 
of the driver is not really useful with the current ABI.
>
> [ ... snip ... ]
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index e6fce23..f0d9125 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1807,6 +1807,7 @@ static const struct hid_device_id
>> hid_have_special_driver[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS2) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
>> +{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
>> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>> USB_DEVICE_ID_CYPRESS_BARCODE_1) },
> Your mail client is corrupting long lines, making tha patch application 
> impossible. Please fix that for your following submissions.
>
>> diff --git a/drivers/hid/hid-corsair-k90.c b/drivers/hid/hid-corsair-k90.c
>> new file mode 100644
>> index 000..67c1095
>> --- /dev/null
>> +++ b/drivers/hid/hid-corsair-k90.c
>> @@ -0,0 +1,690 @@
>> +/*
>> + * HID driver for Corsair Vengeance K90 Keyboard
> Usually we try to be a little bit more generic, and name the driver 
> according to the vendor (and fold all the vedor-specific stuff into the 
> on

Re: [PATCH 1/1] Corsair Vengeance K90 driver

[Clément Vuchener]

On 09/04/2015 03:27 PM, Jiri Kosina wrote:
> On Sat, 29 Aug 2015, Clément Vuchener wrote:
>
>
> This is missing changelog and Signed-off-by: line. I know that you have 
> provided that in your cover letter, but cover letter never make it to the 
> actual GIT commits.
>
> So please fix that and resend the patch, thanks.
>
>> ---
>>  Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
>>  .../ABI/testing/sysfs-driver-hid-corsair-k90   |  15 +
>>  drivers/hid/Kconfig|  10 +
>>  drivers/hid/Makefile   |   1 +
>>  drivers/hid/hid-core.c |   1 +
>>  drivers/hid/hid-corsair-k90.c  | 690
>> +
>>  drivers/hid/hid-ids.h  |   3 +
>>  7 files changed, 775 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
>>  create mode 100644 drivers/hid/hid-corsair-k90.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile
>> b/Documentation/ABI/testing/sysfs-class-k90_profile
>> new file mode 100644
>> index 000..3275c16
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-k90_profile
>> @@ -0,0 +1,55 @@
>> +What:   /sys/class/k90_profile//profile_number
>> +Date:   August 2015
>> +KernelVersion:  4.2
>> +Contact:Clement Vuchener <clement.vuche...@gmail.com>
>> +Description:Get the number of the profile.
>> +
>> +What:   /sys/class/k90_profile//bindings
>> +Date:   August 2015
>> +KernelVersion:  4.2
>> +Contact:Clement Vuchener <clement.vuche...@gmail.com>
>> +Description:Write bindings to the keyboard on-board profile.
>> +The data structure is:
>> + - number of bindings in this structure (1 byte)
>> + - size of this data structure (2 bytes big endian)
>> + - size of the macro data written to
>> +   /sys/class/k90_profile//data (2 bytes big endian)
>> + - array of bindings referencing the data from
>> +   /sys/class/k90_profile//data, each containing:
>> +   * 0x10 for a key usage, 0x20 for a macro (1 byte)
>> +   * offset of the key usage/macro data (2 bytes big endian)
>> +   * length of the key usage/macro data (2 bytes big endian)
>> +
> This looks like violation of one-value-per-attribute rule for sysfs ABI. 
> Could you please think about it once again with this aspect on mind?
Per key attributes would be nice but I don't think I can do that. The profile 
must be written all at once and I don't know any way to read it from the 
hardware (the windows driver I studied does not do it). So writing one value 
only would erase all the others.
I think I will remove this part from the driver. The same thing can easily be 
done in user space through libusb and writing profile should be exceptional 
enough that it is not a problem to detach the driver while doing it. That part 
of the driver is not really useful with the current ABI.
>
> [ ... snip ... ]
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index e6fce23..f0d9125 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1807,6 +1807,7 @@ static const struct hid_device_id
>> hid_have_special_driver[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS2) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
>> +{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
>> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>> USB_DEVICE_ID_CYPRESS_BARCODE_1) },
> Your mail client is corrupting long lines, making tha patch application 
> impossible. Please fix that for your following submissions.
>
>> diff --git a/drivers/hid/hid-corsair-k90.c b/drivers/hid/hid-corsair-k90.c
>> new file mode 100644
>> index 000..67c1095
>> --- /dev/null
>> +++ b/drivers/hid/hid-corsair-k90.c
>> @@ -0,0 +1,690 @@
>> +/*
>> + * HID driver for Corsair Vengeance K90 Keyboard
> Usually we try to be a little bit more generic, and name the driver 
> accordin

[PATCH 1/1] Corsair Vengeance K90 driver

[Clément Vuchener]

---
 Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
 .../ABI/testing/sysfs-driver-hid-corsair-k90   |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair-k90.c  | 690 
+

 drivers/hid/hid-ids.h  |   3 +
 7 files changed, 775 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
 create mode 100644 drivers/hid/hid-corsair-k90.c

diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile 
b/Documentation/ABI/testing/sysfs-class-k90_profile

new file mode 100644
index 000..3275c16
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-k90_profile
@@ -0,0 +1,55 @@
+What:  /sys/class/k90_profile//profile_number
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get the number of the profile.
+
+What:  /sys/class/k90_profile//bindings
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Write bindings to the keyboard on-board profile.
+   The data structure is:
+- number of bindings in this structure (1 byte)
+- size of this data structure (2 bytes big endian)
+- size of the macro data written to
+  /sys/class/k90_profile//data (2 bytes big endian)
+- array of bindings referencing the data from
+  /sys/class/k90_profile//data, each containing:
+  * 0x10 for a key usage, 0x20 for a macro (1 byte)
+  * offset of the key usage/macro data (2 bytes big endian)
+  * length of the key usage/macro data (2 bytes big endian)
+
+What:  /sys/class/k90_profile//keys
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Write the list of the keys used by the bindings and how
+   the macros are repeated.
+   The data struct is:
+- number of keys in this structure (1 byte)
+- array of keys and repeat mode:
+  * key usage (G1 to G16 are 0xD0 to 0xDF, G17 and
+G18 are 0xE8 and 0xE9) (1 byte)
+  * repeat mode (1 byte):
+  1: play when pressed
+  2: repeat while key is pressed
+  3: repeat until the key is pressed again
+
+What:  /sys/class/k90_profile//data
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Write the key usage and macros used by
+   /sys/class/k90_profile//bindings
+   Macro items are:
+- Key event:
+  * item type: 0x84 (1 byte)
+  * HID usage (1 byte)
+  * new state: 0 released, 1 pressed (1 byte)
+- Delay
+  * item type: 0x87 (1 byte)
+  * delay in milliseconds (2 bytes big endian)
+- Macro end
+  * item type: 0x86 (1 byte)
+  * repeat count (2 bytes big endian)
diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair-k90 
b/Documentation/ABI/testing/sysfs-driver-hid-corsair-k90

new file mode 100644
index 000..16d50ae
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
@@ -0,0 +1,15 @@
+What:  /sys/bus/drivers/k90//macro_mode
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get/set the current playback mode. "SW" for software mode
+   where G-keys triggers their regular key codes. "HW" for
+   hardware playback mode where the G-keys play their macro
+   from the on-board memory.
+
+
+What:  /sys/bus/drivers/k90//current_profile
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener 
+Description:   Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index cc4c664..db6d8a5 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -171,6 +171,16 @@ config HID_CHICONY
---help---
Support for Chicony Tactical pad.
 +config HID_CORSAIR
+   tristate "Corsair devices"
+   depends on HID
+   ---help---
+   Support for Corsair devices that are not fully compliant with the
+   HID standard.
+
+   Supported devices:
+   - Vengeance K90
+
 config HID_PRODIKEYS
tristate "Prodikeys PC-MIDI Keyboard support"
depends on HID && SND
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 2f8a41d..e94a0a5 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 

[PATCH 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

This patch implements a HID driver for the Corsair Vengeance K90 keyboard.

This keyboard has a backlight, macro keys (G1 to G18) and some other 
special keys (macro recording, profile switching, changing the light 
level or disabling the super/meta/"windows logo" keys). The macro 
playback can be switched between hardware or software (from the driver 
not a key). Using the generic HID driver, the macro keys and special 
keys send wrong key code or no key code at all (because of the HID usage 
code used by the keyboard).
The purpose of this driver is to fix the behaviour of the keys using 
incorrect HID usage codes and expose the other features to the user space.


This is my first Linux driver and I would like to get feedback for both 
my code and my choices for the user space interface.


For the keyboard to work in software mode, a user program needs to be 
able to get key events. I am not sure about which key codes should be 
used for that. I choose the BTN_TRIGGER_HAPPY* as they seem to be used 
for devices with extra buttons.


The other special keys does send any key codes. They are used to change 
the hardware state and user space should not need them directly.


Playback mode and current profile can be read and written through simple 
sysfs attributes.


The backlight is managed through a LED class device.

Hardware macro profiles (there are three of them) are managed thanks to 
devices of the new k90_profile class. Data is sent through binary 
attributes in the same format sent to the hardware. I choose to do so to 
keep the driver simple. Should it use a more abstract ABI? Those 
attributes are write-only as is does not look like there is way to read 
them back (the Windows driver does not do it).


The macro recording is not actually done from hardware and this driver 
does not implement it. The recording LED has its device, but having it 
on or pressing the button does not record anything.


Clément Vuchener (1):
  Corsair Vengeance K90 driver

 Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
 .../ABI/testing/sysfs-driver-hid-corsair-k90   |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair-k90.c  | 690 
+

 drivers/hid/hid-ids.h  |   3 +
 7 files changed, 775 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
 create mode 100644 drivers/hid/hid-corsair-k90.c

--
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/1] Corsair Vengeance K90 driver

[Clément Vuchener]

---
 Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
 .../ABI/testing/sysfs-driver-hid-corsair-k90   |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair-k90.c  | 690 
+

 drivers/hid/hid-ids.h  |   3 +
 7 files changed, 775 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
 create mode 100644 drivers/hid/hid-corsair-k90.c

diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile 
b/Documentation/ABI/testing/sysfs-class-k90_profile

new file mode 100644
index 000..3275c16
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-k90_profile
@@ -0,0 +1,55 @@
+What:  /sys/class/k90_profile/profile/profile_number
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener clement.vuche...@gmail.com
+Description:   Get the number of the profile.
+
+What:  /sys/class/k90_profile/profile/bindings
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener clement.vuche...@gmail.com
+Description:   Write bindings to the keyboard on-board profile.
+   The data structure is:
+- number of bindings in this structure (1 byte)
+- size of this data structure (2 bytes big endian)
+- size of the macro data written to
+  /sys/class/k90_profile/profile/data (2 bytes big endian)
+- array of bindings referencing the data from
+  /sys/class/k90_profile/profile/data, each containing:
+  * 0x10 for a key usage, 0x20 for a macro (1 byte)
+  * offset of the key usage/macro data (2 bytes big endian)
+  * length of the key usage/macro data (2 bytes big endian)
+
+What:  /sys/class/k90_profile/profile/keys
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener clement.vuche...@gmail.com
+Description:   Write the list of the keys used by the bindings and how
+   the macros are repeated.
+   The data struct is:
+- number of keys in this structure (1 byte)
+- array of keys and repeat mode:
+  * key usage (G1 to G16 are 0xD0 to 0xDF, G17 and
+G18 are 0xE8 and 0xE9) (1 byte)
+  * repeat mode (1 byte):
+  1: play when pressed
+  2: repeat while key is pressed
+  3: repeat until the key is pressed again
+
+What:  /sys/class/k90_profile/profile/data
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener clement.vuche...@gmail.com
+Description:   Write the key usage and macros used by
+   /sys/class/k90_profile/profile/bindings
+   Macro items are:
+- Key event:
+  * item type: 0x84 (1 byte)
+  * HID usage (1 byte)
+  * new state: 0 released, 1 pressed (1 byte)
+- Delay
+  * item type: 0x87 (1 byte)
+  * delay in milliseconds (2 bytes big endian)
+- Macro end
+  * item type: 0x86 (1 byte)
+  * repeat count (2 bytes big endian)
diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair-k90 
b/Documentation/ABI/testing/sysfs-driver-hid-corsair-k90

new file mode 100644
index 000..16d50ae
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
@@ -0,0 +1,15 @@
+What:  /sys/bus/drivers/k90/dev/macro_mode
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener clement.vuche...@gmail.com
+Description:   Get/set the current playback mode. SW for software mode
+   where G-keys triggers their regular key codes. HW for
+   hardware playback mode where the G-keys play their macro
+   from the on-board memory.
+
+
+What:  /sys/bus/drivers/k90/dev/current_profile
+Date:  August 2015
+KernelVersion: 4.2
+Contact:   Clement Vuchener clement.vuche...@gmail.com
+Description:   Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index cc4c664..db6d8a5 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -171,6 +171,16 @@ config HID_CHICONY
---help---
Support for Chicony Tactical pad.
 +config HID_CORSAIR
+   tristate Corsair devices
+   depends on HID
+   ---help---
+   Support for Corsair devices that are not fully compliant with the
+   HID standard.
+
+   Supported devices:
+   - Vengeance K90
+
 config HID_PRODIKEYS
tristate Prodikeys 

[PATCH 0/1] Corsair Vengeance K90 driver

[Clément Vuchener]

This patch implements a HID driver for the Corsair Vengeance K90 keyboard.

This keyboard has a backlight, macro keys (G1 to G18) and some other 
special keys (macro recording, profile switching, changing the light 
level or disabling the super/meta/windows logo keys). The macro 
playback can be switched between hardware or software (from the driver 
not a key). Using the generic HID driver, the macro keys and special 
keys send wrong key code or no key code at all (because of the HID usage 
code used by the keyboard).
The purpose of this driver is to fix the behaviour of the keys using 
incorrect HID usage codes and expose the other features to the user space.


This is my first Linux driver and I would like to get feedback for both 
my code and my choices for the user space interface.


For the keyboard to work in software mode, a user program needs to be 
able to get key events. I am not sure about which key codes should be 
used for that. I choose the BTN_TRIGGER_HAPPY* as they seem to be used 
for devices with extra buttons.


The other special keys does send any key codes. They are used to change 
the hardware state and user space should not need them directly.


Playback mode and current profile can be read and written through simple 
sysfs attributes.


The backlight is managed through a LED class device.

Hardware macro profiles (there are three of them) are managed thanks to 
devices of the new k90_profile class. Data is sent through binary 
attributes in the same format sent to the hardware. I choose to do so to 
keep the driver simple. Should it use a more abstract ABI? Those 
attributes are write-only as is does not look like there is way to read 
them back (the Windows driver does not do it).


The macro recording is not actually done from hardware and this driver 
does not implement it. The recording LED has its device, but having it 
on or pressing the button does not record anything.


Clément Vuchener (1):
  Corsair Vengeance K90 driver

 Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
 .../ABI/testing/sysfs-driver-hid-corsair-k90   |  15 +
 drivers/hid/Kconfig|  10 +
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-corsair-k90.c  | 690 
+

 drivers/hid/hid-ids.h  |   3 +
 7 files changed, 775 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
 create mode 100644 drivers/hid/hid-corsair-k90.c

--
2.4.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/