Re: [PATCH v3 2/7] HID: Add driver for Logitech gaming keyboards (G15, G15 v2)

Re: [PATCH v3 2/7] HID: Add driver for Logitech gaming keyboards (G15, G15 v2)

[Jiri Kosina]

On Wed, 28 Aug 2019, Hans de Goede wrote:

> Add a driver to stop the extra "G" keys from sending F1 - F12 instead
> making them send KEY_GKEY# and also make the non-functional M1 - M3 and MR
> keys and the non-functional buttons below the LCD panel properly generated
> key events.
> 
> Note the connect_mask and gkeys_settings_output_report variables may seem
> unnecessary since they are always set to the same value, these are there in
> preparation of adding support for the G, M and LCD keys on the G510 kbd.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - s/KEY_LCD_MENU/KEY_KBD_LCD_MENU/
> ---
>  MAINTAINERS              |   7 ++
>  drivers/hid/Makefile     |   1 +
>  drivers/hid/hid-ids.h    |   2 +
>  drivers/hid/hid-lg-g15.c | 256 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 266 insertions(+)
>  create mode 100644 drivers/hid/hid-lg-g15.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6480cf7c8bb3..8dfad599d367 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9474,6 +9474,13 @@ S:	Maintained
>  F:	Documentation/admin-guide/ldm.rst
>  F:	block/partitions/ldm.*
>  
> +LOGITECH HID GAMING KEYBOARDS
> +M:	Hans de Goede <hdegoede@redhat.com>
> +L:	linux-input@vger.kernel.org
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
> +S:	Maintained
> +F:	drivers/hid/hid-lg-g15.c
> +
>  LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
>  M:	Sathya Prakash <sathya.prakash@broadcom.com>
>  M:	Chaitra P B <chaitra.basappa@broadcom.com>
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index cc5d827c9164..666f4358207e 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_HID_KYE)		+= hid-kye.o
>  obj-$(CONFIG_HID_LCPOWER)	+= hid-lcpower.o
>  obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
>  obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
> +obj-$(CONFIG_HID_LOGITECH)	+= hid-lg-g15.o

Hi Hans,

I've finished reviewing this pile, and would like to queue it for 5.5, but 
have one remaining question -- what is the reason you decided to factor 
this out as a separate driver, instead of putting it under the 
hid-logitech umbrella?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 2/7] HID: Add driver for Logitech gaming keyboards (G15, G15 v2)

[Hans de Goede]

Hi,

On 03-10-2019 15:41, Jiri Kosina wrote:
> On Wed, 28 Aug 2019, Hans de Goede wrote:
> 
>> Add a driver to stop the extra "G" keys from sending F1 - F12 instead
>> making them send KEY_GKEY# and also make the non-functional M1 - M3 and MR
>> keys and the non-functional buttons below the LCD panel properly generated
>> key events.
>>
>> Note the connect_mask and gkeys_settings_output_report variables may seem
>> unnecessary since they are always set to the same value, these are there in
>> preparation of adding support for the G, M and LCD keys on the G510 kbd.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> - s/KEY_LCD_MENU/KEY_KBD_LCD_MENU/
>> ---
>>   MAINTAINERS              |   7 ++
>>   drivers/hid/Makefile     |   1 +
>>   drivers/hid/hid-ids.h    |   2 +
>>   drivers/hid/hid-lg-g15.c | 256 +++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 266 insertions(+)
>>   create mode 100644 drivers/hid/hid-lg-g15.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6480cf7c8bb3..8dfad599d367 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9474,6 +9474,13 @@ S:	Maintained
>>   F:	Documentation/admin-guide/ldm.rst
>>   F:	block/partitions/ldm.*
>>   
>> +LOGITECH HID GAMING KEYBOARDS
>> +M:	Hans de Goede <hdegoede@redhat.com>
>> +L:	linux-input@vger.kernel.org
>> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
>> +S:	Maintained
>> +F:	drivers/hid/hid-lg-g15.c
>> +
>>   LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
>>   M:	Sathya Prakash <sathya.prakash@broadcom.com>
>>   M:	Chaitra P B <chaitra.basappa@broadcom.com>
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index cc5d827c9164..666f4358207e 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -63,6 +63,7 @@ obj-$(CONFIG_HID_KYE)		+= hid-kye.o
>>   obj-$(CONFIG_HID_LCPOWER)	+= hid-lcpower.o
>>   obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
>>   obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
>> +obj-$(CONFIG_HID_LOGITECH)	+= hid-lg-g15.o
> 
> Hi Hans,
> 
> I've finished reviewing this pile, and would like to queue it for 5.5, but

Great, thank you for reviewing this!

> have one remaining question -- what is the reason you decided to factor
> this out as a separate driver, instead of putting it under the
> hid-logitech umbrella?

I put this in a separate driver, because it is a substantial chunk of code
and the gaming keyboard stuff shares nothing what so ever with the other
Logitech bits.

Regards,

Hans

Re: [PATCH v3 2/7] HID: Add driver for Logitech gaming keyboards (G15, G15 v2)

[Jiri Kosina]

On Thu, 3 Oct 2019, Hans de Goede wrote:

> > have one remaining question -- what is the reason you decided to factor
> > this out as a separate driver, instead of putting it under the
> > hid-logitech umbrella?
> 
> I put this in a separate driver, because it is a substantial chunk of code
> and the gaming keyboard stuff shares nothing what so ever with the other
> Logitech bits.

Well, we generally keep the support really divided on the vendor 
granularity, even if code sharing for individual models is minimal.

I mostly personally prefer that due to absolute clarity for people making 
CONFIG_ decisions, but as you put that under the generic Logitech config, 
it's not a big issue.

Queued in for-5.5/logitech now.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 2/7] HID: Add driver for Logitech gaming keyboards (G15, G15 v2)

[Hans de Goede]

Hi,

On 03-10-2019 20:49, Jiri Kosina wrote:
> On Thu, 3 Oct 2019, Hans de Goede wrote:
> 
>>> have one remaining question -- what is the reason you decided to factor
>>> this out as a separate driver, instead of putting it under the
>>> hid-logitech umbrella?
>>
>> I put this in a separate driver, because it is a substantial chunk of code
>> and the gaming keyboard stuff shares nothing what so ever with the other
>> Logitech bits.
> 
> Well, we generally keep the support really divided on the vendor
> granularity, even if code sharing for individual models is minimal.
> 
> I mostly personally prefer that due to absolute clarity for people making
> CONFIG_ decisions, but as you put that under the generic Logitech config,
> it's not a big issue.
> 
> Queued in for-5.5/logitech now.

Great, thank you.

Regards,

Hans