linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
@ 2018-03-04 17:00 Florent Flament
  2018-03-04 22:14 ` [PATCH v2 0/1] " Florent Flament
  0 siblings, 1 reply; 13+ messages in thread
From: Florent Flament @ 2018-03-04 17:00 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, linux-kernel, linux-input; +Cc: Florent Flament

With the generic HID driver, K290 keyboards' F1 to F12 keys send
multimedia events by default, and standard keycodes when the function
key is pressed. This driver allows to configure K290 keyboards, so
that F1 to F12 have a standard behavior and send multimedia events
when the function key is pressed. The keyboard mode is set through the
fn_mode module parameter: when set to 1 (default setting) the keyboard
behaves as with the generic HID driver, when set to 0 the keyboard is
configured to work as standard keyboards.

Signed-off-by: Florent Flament <contact@florentflament.com>
---
 drivers/hid/Kconfig             |  18 ++++++++
 drivers/hid/Makefile            |   1 +
 drivers/hid/hid-ids.h           |   1 +
 drivers/hid/hid-logitech-k290.c | 100 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 drivers/hid/hid-logitech-k290.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6686da8daac6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -488,6 +488,24 @@ config HID_LOGITECH_HIDPP
 	T651, TK820), some mice (Zone Touch mouse), or even keyboards (Solar
 	Keyboard).
 
+config HID_LOGITECH_K290
+	tristate "Logitech K290 Keyboard support"
+	depends on USB_HID
+	---help---
+	This enhances support of Logitech K290 keyboards.
+
+	With the generic HID driver, K290 keyboards' F1 to F12 keys
+	send multimedia events by default, and standard keycodes when
+	the function key is pressed. This driver allows to configure
+	K290 keyboards, so that F1 to F12 have a standard behavior and
+	send multimedia events when the function key is pressed. The
+	keyboard mode is set through the fn_mode module parameter:
+	when set to 1 (default setting) the keyboard behaves as with
+	the generic HID driver, when set to 0 the keyboard is
+	configured to work as standard keyboards.
+
+	Say Y if you have a Logitech K290 keyboard.
+
 config LOGITECH_FF
 	bool "Logitech force feedback support"
 	depends on HID_LOGITECH
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92d85..78079d3a5d58 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
 obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
 obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
 obj-$(CONFIG_HID_LOGITECH_HIDPP)	+= hid-logitech-hidpp.o
+obj-$(CONFIG_HID_LOGITECH_K290)	+= hid-logitech-k290.o
 obj-$(CONFIG_HID_MAGICMOUSE)	+= hid-magicmouse.o
 obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 9454ac134ce2..68caba7e666c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -693,6 +693,7 @@
 #define USB_DEVICE_ID_LOGITECH_HARMONY_LAST 0xc14f
 #define USB_DEVICE_ID_LOGITECH_HARMONY_PS3 0x0306
 #define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
+#define USB_DEVICE_ID_LOGITECH_KEYBOARD_K290	0xc31f
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A	0xc01a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A	0xc05a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A	0xc06a
diff --git a/drivers/hid/hid-logitech-k290.c b/drivers/hid/hid-logitech-k290.c
new file mode 100644
index 000000000000..36fdb5838842
--- /dev/null
+++ b/drivers/hid/hid-logitech-k290.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for Logitech K290 keyboard
+ *
+ * Copyright (c) 2018 Florent Flament
+ *
+ * This drivers allows to configure the K290 keyboard's function key
+ * behaviour (whether function mode is activated or not by default).
+ *
+ * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
+ * (https://github.com/milgner/k290-fnkeyctl):
+ * K290_SET_FUNCTION_CMD
+ * K290_SET_FUNCTION_VAL
+ * K290_SET_FUNCTION_OFF
+ * K290_SET_FUNCTION_ON
+ *
+ * Based on hid-accutouch.c and hid-elo.c
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/stat.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+#include "hid-ids.h"
+#include "usbhid/usbhid.h"
+
+// Logitech K290 custom USB command and value to setup function key
+#define K290_SET_FUNCTION_CMD 0x02
+#define K290_SET_FUNCTION_VAL 0x001a
+
+// Have function mode turned off (as with standard keyboards)
+#define K290_SET_FUNCTION_OFF 0x0001
+// Have function mode turned on (default k290 behavior)
+#define K290_SET_FUNCTION_ON  0x0000
+
+// Function key default mode is set at module load time for every K290
+// keyboards plugged on the machine. By default fn_mode = 1, i.e
+// sending K290_SET_FUNCTION_ON (default K290 behavior).
+static bool fn_mode = 1;
+module_param(fn_mode, bool, 0444);
+MODULE_PARM_DESC(fn_mode, "Logitech K290 function key mode (default = 1)");
+
+static void k290_set_function(struct usb_device *dev, uint16_t function_mode)
+{
+	int ret;
+
+	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+			      K290_SET_FUNCTION_CMD,
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      K290_SET_FUNCTION_VAL,
+			      function_mode, 0, 0, USB_CTRL_SET_TIMEOUT);
+
+	if (ret < 0)
+		dev_err(&dev->dev,
+			"Failed to setup K290 function key, error %d\n", ret);
+}
+
+static int k290_set_function_hid_device(struct hid_device *hid)
+{
+	struct usb_device *usb_dev = hid_to_usb_dev(hid);
+
+	k290_set_function(usb_dev,
+			fn_mode ? K290_SET_FUNCTION_ON : K290_SET_FUNCTION_OFF);
+	return 0;
+}
+
+static int k290_input_configured(struct hid_device *hid,
+				 struct hid_input *hidinput)
+{
+	return k290_set_function_hid_device(hid);
+}
+
+static int k290_resume(struct hid_device *hid)
+{
+	return k290_set_function_hid_device(hid);
+}
+
+static const struct hid_device_id k290_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_KEYBOARD_K290) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, k290_devices);
+
+static struct hid_driver k290_driver = {
+	.name = "hid-logitech-k290",
+	.id_table = k290_devices,
+	.input_configured = k290_input_configured,
+	.resume = k290_resume,
+	.reset_resume = k290_resume,
+};
+
+module_hid_driver(k290_driver);
+
+MODULE_AUTHOR("Florent Flament <contact@florentflament.com>");
+MODULE_DESCRIPTION("Logitech K290 keyboard driver");
+MODULE_LICENSE("GPL v2");
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 0/1] Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-04 17:00 [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard Florent Flament
@ 2018-03-04 22:14 ` Florent Flament
  2018-03-04 22:14   ` [PATCH v2 1/1] HID: " Florent Flament
  0 siblings, 1 reply; 13+ messages in thread
From: Florent Flament @ 2018-03-04 22:14 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: jikos, benjamin.tissoires, linux-kernel, linux-input, Florent Flament

Changes in v2:
* Using SPDX ID instead of licence statement
* Removed unnecessary includes (linux/moduleparam.h and linux/mod_devicetable.h)

ps: Sorry for having broken the previous thread and sent the previous
mail without proper versioning of the patch.

Florent Flament (1):
  HID: Logitech K290: Add driver for the Logitech K290 USB keyboard

 drivers/hid/Kconfig             |  18 ++++++++
 drivers/hid/Makefile            |   1 +
 drivers/hid/hid-ids.h           |   1 +
 drivers/hid/hid-logitech-k290.c | 100 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 drivers/hid/hid-logitech-k290.c

-- 
2.14.3

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-04 22:14 ` [PATCH v2 0/1] " Florent Flament
@ 2018-03-04 22:14   ` Florent Flament
  2018-03-05  9:31     ` Nestor Lopez Casado
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Florent Flament @ 2018-03-04 22:14 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: jikos, benjamin.tissoires, linux-kernel, linux-input, Florent Flament

With the generic HID driver, K290 keyboards' F1 to F12 keys send
multimedia events by default, and standard keycodes when the function
key is pressed. This driver allows to configure K290 keyboards, so
that F1 to F12 have a standard behavior and send multimedia events
when the function key is pressed. The keyboard mode is set through the
fn_mode module parameter: when set to 1 (default setting) the keyboard
behaves as with the generic HID driver, when set to 0 the keyboard is
configured to work as standard keyboards.

Signed-off-by: Florent Flament <contact@florentflament.com>
---
 drivers/hid/Kconfig             |  18 ++++++++
 drivers/hid/Makefile            |   1 +
 drivers/hid/hid-ids.h           |   1 +
 drivers/hid/hid-logitech-k290.c | 100 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 drivers/hid/hid-logitech-k290.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6686da8daac6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -488,6 +488,24 @@ config HID_LOGITECH_HIDPP
 	T651, TK820), some mice (Zone Touch mouse), or even keyboards (Solar
 	Keyboard).
 
+config HID_LOGITECH_K290
+	tristate "Logitech K290 Keyboard support"
+	depends on USB_HID
+	---help---
+	This enhances support of Logitech K290 keyboards.
+
+	With the generic HID driver, K290 keyboards' F1 to F12 keys
+	send multimedia events by default, and standard keycodes when
+	the function key is pressed. This driver allows to configure
+	K290 keyboards, so that F1 to F12 have a standard behavior and
+	send multimedia events when the function key is pressed. The
+	keyboard mode is set through the fn_mode module parameter:
+	when set to 1 (default setting) the keyboard behaves as with
+	the generic HID driver, when set to 0 the keyboard is
+	configured to work as standard keyboards.
+
+	Say Y if you have a Logitech K290 keyboard.
+
 config LOGITECH_FF
 	bool "Logitech force feedback support"
 	depends on HID_LOGITECH
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92d85..78079d3a5d58 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
 obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
 obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
 obj-$(CONFIG_HID_LOGITECH_HIDPP)	+= hid-logitech-hidpp.o
+obj-$(CONFIG_HID_LOGITECH_K290)	+= hid-logitech-k290.o
 obj-$(CONFIG_HID_MAGICMOUSE)	+= hid-magicmouse.o
 obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 9454ac134ce2..68caba7e666c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -693,6 +693,7 @@
 #define USB_DEVICE_ID_LOGITECH_HARMONY_LAST 0xc14f
 #define USB_DEVICE_ID_LOGITECH_HARMONY_PS3 0x0306
 #define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
+#define USB_DEVICE_ID_LOGITECH_KEYBOARD_K290	0xc31f
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A	0xc01a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A	0xc05a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A	0xc06a
diff --git a/drivers/hid/hid-logitech-k290.c b/drivers/hid/hid-logitech-k290.c
new file mode 100644
index 000000000000..36fdb5838842
--- /dev/null
+++ b/drivers/hid/hid-logitech-k290.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for Logitech K290 keyboard
+ *
+ * Copyright (c) 2018 Florent Flament
+ *
+ * This drivers allows to configure the K290 keyboard's function key
+ * behaviour (whether function mode is activated or not by default).
+ *
+ * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
+ * (https://github.com/milgner/k290-fnkeyctl):
+ * K290_SET_FUNCTION_CMD
+ * K290_SET_FUNCTION_VAL
+ * K290_SET_FUNCTION_OFF
+ * K290_SET_FUNCTION_ON
+ *
+ * Based on hid-accutouch.c and hid-elo.c
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/stat.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+#include "hid-ids.h"
+#include "usbhid/usbhid.h"
+
+// Logitech K290 custom USB command and value to setup function key
+#define K290_SET_FUNCTION_CMD 0x02
+#define K290_SET_FUNCTION_VAL 0x001a
+
+// Have function mode turned off (as with standard keyboards)
+#define K290_SET_FUNCTION_OFF 0x0001
+// Have function mode turned on (default k290 behavior)
+#define K290_SET_FUNCTION_ON  0x0000
+
+// Function key default mode is set at module load time for every K290
+// keyboards plugged on the machine. By default fn_mode = 1, i.e
+// sending K290_SET_FUNCTION_ON (default K290 behavior).
+static bool fn_mode = 1;
+module_param(fn_mode, bool, 0444);
+MODULE_PARM_DESC(fn_mode, "Logitech K290 function key mode (default = 1)");
+
+static void k290_set_function(struct usb_device *dev, uint16_t function_mode)
+{
+	int ret;
+
+	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+			      K290_SET_FUNCTION_CMD,
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      K290_SET_FUNCTION_VAL,
+			      function_mode, 0, 0, USB_CTRL_SET_TIMEOUT);
+
+	if (ret < 0)
+		dev_err(&dev->dev,
+			"Failed to setup K290 function key, error %d\n", ret);
+}
+
+static int k290_set_function_hid_device(struct hid_device *hid)
+{
+	struct usb_device *usb_dev = hid_to_usb_dev(hid);
+
+	k290_set_function(usb_dev,
+			fn_mode ? K290_SET_FUNCTION_ON : K290_SET_FUNCTION_OFF);
+	return 0;
+}
+
+static int k290_input_configured(struct hid_device *hid,
+				 struct hid_input *hidinput)
+{
+	return k290_set_function_hid_device(hid);
+}
+
+static int k290_resume(struct hid_device *hid)
+{
+	return k290_set_function_hid_device(hid);
+}
+
+static const struct hid_device_id k290_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_KEYBOARD_K290) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, k290_devices);
+
+static struct hid_driver k290_driver = {
+	.name = "hid-logitech-k290",
+	.id_table = k290_devices,
+	.input_configured = k290_input_configured,
+	.resume = k290_resume,
+	.reset_resume = k290_resume,
+};
+
+module_hid_driver(k290_driver);
+
+MODULE_AUTHOR("Florent Flament <contact@florentflament.com>");
+MODULE_DESCRIPTION("Logitech K290 keyboard driver");
+MODULE_LICENSE("GPL v2");
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-04 22:14   ` [PATCH v2 1/1] HID: " Florent Flament
@ 2018-03-05  9:31     ` Nestor Lopez Casado
  2018-03-05 17:26       ` Benjamin Tissoires
  2018-03-05 23:25       ` Florent Flament
  2018-03-05 15:20     ` kbuild test robot
  2018-03-05 21:11     ` kbuild test robot
  2 siblings, 2 replies; 13+ messages in thread
From: Nestor Lopez Casado @ 2018-03-05  9:31 UTC (permalink / raw)
  To: Florent Flament
  Cc: andy.shevchenko, jikos, Benjamin Tissoires, linux-kernel,
	open list:HID CORE LAYER

Hello Florent,

In my view, this driver may not be a good idea. The default behaviour
of K290 is 'send multimedia keycodes' with the user given the choice
to change that behaviour via vendor commands. Putting a driver that
will unconditionally change that behaviour without the user's consent
might bother other users that prefer the multimedia keycodes by
default.

Besides, I'd argue that instead of a kernel module this would be best
achieved from a user space application. Something in the lines of
Solaar (github pwr/solaar) or libratbag (there's an issue open to
support keyboards) or even a specific application built for the
purpose. Anyways, please collect the input from Benjamin and Jiri as
they as they best placed to advise than myself.

Cheers,
-nestor

On Sun, Mar 4, 2018 at 11:14 PM, Florent Flament
<contact@florentflament.com> wrote:
> With the generic HID driver, K290 keyboards' F1 to F12 keys send
> multimedia events by default, and standard keycodes when the function
> key is pressed. This driver allows to configure K290 keyboards, so
> that F1 to F12 have a standard behavior and send multimedia events
> when the function key is pressed. The keyboard mode is set through the
> fn_mode module parameter: when set to 1 (default setting) the keyboard
> behaves as with the generic HID driver, when set to 0 the keyboard is
> configured to work as standard keyboards.
>
> Signed-off-by: Florent Flament <contact@florentflament.com>
> ---
>  drivers/hid/Kconfig             |  18 ++++++++
>  drivers/hid/Makefile            |   1 +
>  drivers/hid/hid-ids.h           |   1 +
>  drivers/hid/hid-logitech-k290.c | 100 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 120 insertions(+)
>  create mode 100644 drivers/hid/hid-logitech-k290.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 19c499f5623d..6686da8daac6 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -488,6 +488,24 @@ config HID_LOGITECH_HIDPP
>         T651, TK820), some mice (Zone Touch mouse), or even keyboards (Solar
>         Keyboard).
>
> +config HID_LOGITECH_K290
> +       tristate "Logitech K290 Keyboard support"
> +       depends on USB_HID
> +       ---help---
> +       This enhances support of Logitech K290 keyboards.
> +
> +       With the generic HID driver, K290 keyboards' F1 to F12 keys
> +       send multimedia events by default, and standard keycodes when
> +       the function key is pressed. This driver allows to configure
> +       K290 keyboards, so that F1 to F12 have a standard behavior and
> +       send multimedia events when the function key is pressed. The
> +       keyboard mode is set through the fn_mode module parameter:
> +       when set to 1 (default setting) the keyboard behaves as with
> +       the generic HID driver, when set to 0 the keyboard is
> +       configured to work as standard keyboards.
> +
> +       Say Y if you have a Logitech K290 keyboard.
> +
>  config LOGITECH_FF
>         bool "Logitech force feedback support"
>         depends on HID_LOGITECH
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index eb13b9e92d85..78079d3a5d58 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_HID_LENOVO)      += hid-lenovo.o
>  obj-$(CONFIG_HID_LOGITECH)     += hid-logitech.o
>  obj-$(CONFIG_HID_LOGITECH_DJ)  += hid-logitech-dj.o
>  obj-$(CONFIG_HID_LOGITECH_HIDPP)       += hid-logitech-hidpp.o
> +obj-$(CONFIG_HID_LOGITECH_K290)        += hid-logitech-k290.o
>  obj-$(CONFIG_HID_MAGICMOUSE)   += hid-magicmouse.o
>  obj-$(CONFIG_HID_MAYFLASH)     += hid-mf.o
>  obj-$(CONFIG_HID_MICROSOFT)    += hid-microsoft.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 9454ac134ce2..68caba7e666c 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -693,6 +693,7 @@
>  #define USB_DEVICE_ID_LOGITECH_HARMONY_LAST 0xc14f
>  #define USB_DEVICE_ID_LOGITECH_HARMONY_PS3 0x0306
>  #define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
> +#define USB_DEVICE_ID_LOGITECH_KEYBOARD_K290   0xc31f
>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A      0xc01a
>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A      0xc05a
>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A      0xc06a
> diff --git a/drivers/hid/hid-logitech-k290.c b/drivers/hid/hid-logitech-k290.c
> new file mode 100644
> index 000000000000..36fdb5838842
> --- /dev/null
> +++ b/drivers/hid/hid-logitech-k290.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HID driver for Logitech K290 keyboard
> + *
> + * Copyright (c) 2018 Florent Flament
> + *
> + * This drivers allows to configure the K290 keyboard's function key
> + * behaviour (whether function mode is activated or not by default).
> + *
> + * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
> + * (https://github.com/milgner/k290-fnkeyctl):
> + * K290_SET_FUNCTION_CMD
> + * K290_SET_FUNCTION_VAL
> + * K290_SET_FUNCTION_OFF
> + * K290_SET_FUNCTION_ON
> + *
> + * Based on hid-accutouch.c and hid-elo.c
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/stat.h>
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +#include "usbhid/usbhid.h"
> +
> +// Logitech K290 custom USB command and value to setup function key
> +#define K290_SET_FUNCTION_CMD 0x02
> +#define K290_SET_FUNCTION_VAL 0x001a
> +
> +// Have function mode turned off (as with standard keyboards)
> +#define K290_SET_FUNCTION_OFF 0x0001
> +// Have function mode turned on (default k290 behavior)
> +#define K290_SET_FUNCTION_ON  0x0000
> +
> +// Function key default mode is set at module load time for every K290
> +// keyboards plugged on the machine. By default fn_mode = 1, i.e
> +// sending K290_SET_FUNCTION_ON (default K290 behavior).
> +static bool fn_mode = 1;
> +module_param(fn_mode, bool, 0444);
> +MODULE_PARM_DESC(fn_mode, "Logitech K290 function key mode (default = 1)");
> +
> +static void k290_set_function(struct usb_device *dev, uint16_t function_mode)
> +{
> +       int ret;
> +
> +       ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> +                             K290_SET_FUNCTION_CMD,
> +                             USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +                             K290_SET_FUNCTION_VAL,
> +                             function_mode, 0, 0, USB_CTRL_SET_TIMEOUT);
> +
> +       if (ret < 0)
> +               dev_err(&dev->dev,
> +                       "Failed to setup K290 function key, error %d\n", ret);
> +}
> +
> +static int k290_set_function_hid_device(struct hid_device *hid)
> +{
> +       struct usb_device *usb_dev = hid_to_usb_dev(hid);
> +
> +       k290_set_function(usb_dev,
> +                       fn_mode ? K290_SET_FUNCTION_ON : K290_SET_FUNCTION_OFF);
> +       return 0;
> +}
> +
> +static int k290_input_configured(struct hid_device *hid,
> +                                struct hid_input *hidinput)
> +{
> +       return k290_set_function_hid_device(hid);
> +}
> +
> +static int k290_resume(struct hid_device *hid)
> +{
> +       return k290_set_function_hid_device(hid);
> +}
> +
> +static const struct hid_device_id k290_devices[] = {
> +       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> +                        USB_DEVICE_ID_LOGITECH_KEYBOARD_K290) },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(hid, k290_devices);
> +
> +static struct hid_driver k290_driver = {
> +       .name = "hid-logitech-k290",
> +       .id_table = k290_devices,
> +       .input_configured = k290_input_configured,
> +       .resume = k290_resume,
> +       .reset_resume = k290_resume,
> +};
> +
> +module_hid_driver(k290_driver);
> +
> +MODULE_AUTHOR("Florent Flament <contact@florentflament.com>");
> +MODULE_DESCRIPTION("Logitech K290 keyboard driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.14.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-04 22:14   ` [PATCH v2 1/1] HID: " Florent Flament
  2018-03-05  9:31     ` Nestor Lopez Casado
@ 2018-03-05 15:20     ` kbuild test robot
  2018-03-05 21:11     ` kbuild test robot
  2 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-03-05 15:20 UTC (permalink / raw)
  To: Florent Flament
  Cc: kbuild-all, andy.shevchenko, jikos, benjamin.tissoires,
	linux-kernel, linux-input, Florent Flament

Hi Florent,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hid/for-next]
[also build test WARNING on v4.16-rc4 next-20180305]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Florent-Flament/Logitech-K290-Add-driver-for-the-Logitech-K290-USB-keyboard/20180305-153311
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/hid/hid-logitech-k290.c:54:46: sparse: Using plain integer as NULL pointer

vim +54 drivers/hid/hid-logitech-k290.c

    45	
    46	static void k290_set_function(struct usb_device *dev, uint16_t function_mode)
    47	{
    48		int ret;
    49	
    50		ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
    51				      K290_SET_FUNCTION_CMD,
    52				      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
    53				      K290_SET_FUNCTION_VAL,
  > 54				      function_mode, 0, 0, USB_CTRL_SET_TIMEOUT);
    55	
    56		if (ret < 0)
    57			dev_err(&dev->dev,
    58				"Failed to setup K290 function key, error %d\n", ret);
    59	}
    60	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-05  9:31     ` Nestor Lopez Casado
@ 2018-03-05 17:26       ` Benjamin Tissoires
  2018-03-05 23:31         ` Florent Flament
  2018-03-05 23:25       ` Florent Flament
  1 sibling, 1 reply; 13+ messages in thread
From: Benjamin Tissoires @ 2018-03-05 17:26 UTC (permalink / raw)
  To: Nestor Lopez Casado
  Cc: Florent Flament, andy.shevchenko, Jiri Kosina, lkml,
	open list:HID CORE LAYER

Hi Florent,

On Mon, Mar 5, 2018 at 10:31 AM, Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
> Hello Florent,
>
> In my view, this driver may not be a good idea. The default behaviour
> of K290 is 'send multimedia keycodes' with the user given the choice
> to change that behaviour via vendor commands. Putting a driver that
> will unconditionally change that behaviour without the user's consent
> might bother other users that prefer the multimedia keycodes by
> default.
>
> Besides, I'd argue that instead of a kernel module this would be best
> achieved from a user space application. Something in the lines of
> Solaar (github pwr/solaar) or libratbag (there's an issue open to
> support keyboards) or even a specific application built for the
> purpose. Anyways, please collect the input from Benjamin and Jiri as
> they as they best placed to advise than myself.

On top of what Nestor said, this type of functionality, if we want to
have them in the kernel should probably be integrated in
hid-logitech-hidpp, in order not having some magic reports to send.

Things like reconnect of the device would be handled far more easily
in hid-logitech-hidpp while you would be reinventing the wheel here.

One other thing I do not like in this submission of the driver is the
direct use of USB while we have a full transport agnostic layer called
HID.

Cheers,
Benjamin

>
> Cheers,
> -nestor
>
> On Sun, Mar 4, 2018 at 11:14 PM, Florent Flament
> <contact@florentflament.com> wrote:
>> With the generic HID driver, K290 keyboards' F1 to F12 keys send
>> multimedia events by default, and standard keycodes when the function
>> key is pressed. This driver allows to configure K290 keyboards, so
>> that F1 to F12 have a standard behavior and send multimedia events
>> when the function key is pressed. The keyboard mode is set through the
>> fn_mode module parameter: when set to 1 (default setting) the keyboard
>> behaves as with the generic HID driver, when set to 0 the keyboard is
>> configured to work as standard keyboards.
>>
>> Signed-off-by: Florent Flament <contact@florentflament.com>
>> ---
>>  drivers/hid/Kconfig             |  18 ++++++++
>>  drivers/hid/Makefile            |   1 +
>>  drivers/hid/hid-ids.h           |   1 +
>>  drivers/hid/hid-logitech-k290.c | 100 ++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 120 insertions(+)
>>  create mode 100644 drivers/hid/hid-logitech-k290.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 19c499f5623d..6686da8daac6 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -488,6 +488,24 @@ config HID_LOGITECH_HIDPP
>>         T651, TK820), some mice (Zone Touch mouse), or even keyboards (Solar
>>         Keyboard).
>>
>> +config HID_LOGITECH_K290
>> +       tristate "Logitech K290 Keyboard support"
>> +       depends on USB_HID
>> +       ---help---
>> +       This enhances support of Logitech K290 keyboards.
>> +
>> +       With the generic HID driver, K290 keyboards' F1 to F12 keys
>> +       send multimedia events by default, and standard keycodes when
>> +       the function key is pressed. This driver allows to configure
>> +       K290 keyboards, so that F1 to F12 have a standard behavior and
>> +       send multimedia events when the function key is pressed. The
>> +       keyboard mode is set through the fn_mode module parameter:
>> +       when set to 1 (default setting) the keyboard behaves as with
>> +       the generic HID driver, when set to 0 the keyboard is
>> +       configured to work as standard keyboards.
>> +
>> +       Say Y if you have a Logitech K290 keyboard.
>> +
>>  config LOGITECH_FF
>>         bool "Logitech force feedback support"
>>         depends on HID_LOGITECH
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index eb13b9e92d85..78079d3a5d58 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -61,6 +61,7 @@ obj-$(CONFIG_HID_LENOVO)      += hid-lenovo.o
>>  obj-$(CONFIG_HID_LOGITECH)     += hid-logitech.o
>>  obj-$(CONFIG_HID_LOGITECH_DJ)  += hid-logitech-dj.o
>>  obj-$(CONFIG_HID_LOGITECH_HIDPP)       += hid-logitech-hidpp.o
>> +obj-$(CONFIG_HID_LOGITECH_K290)        += hid-logitech-k290.o
>>  obj-$(CONFIG_HID_MAGICMOUSE)   += hid-magicmouse.o
>>  obj-$(CONFIG_HID_MAYFLASH)     += hid-mf.o
>>  obj-$(CONFIG_HID_MICROSOFT)    += hid-microsoft.o
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 9454ac134ce2..68caba7e666c 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -693,6 +693,7 @@
>>  #define USB_DEVICE_ID_LOGITECH_HARMONY_LAST 0xc14f
>>  #define USB_DEVICE_ID_LOGITECH_HARMONY_PS3 0x0306
>>  #define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
>> +#define USB_DEVICE_ID_LOGITECH_KEYBOARD_K290   0xc31f
>>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A      0xc01a
>>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A      0xc05a
>>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A      0xc06a
>> diff --git a/drivers/hid/hid-logitech-k290.c b/drivers/hid/hid-logitech-k290.c
>> new file mode 100644
>> index 000000000000..36fdb5838842
>> --- /dev/null
>> +++ b/drivers/hid/hid-logitech-k290.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * HID driver for Logitech K290 keyboard
>> + *
>> + * Copyright (c) 2018 Florent Flament
>> + *
>> + * This drivers allows to configure the K290 keyboard's function key
>> + * behaviour (whether function mode is activated or not by default).
>> + *
>> + * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
>> + * (https://github.com/milgner/k290-fnkeyctl):
>> + * K290_SET_FUNCTION_CMD
>> + * K290_SET_FUNCTION_VAL
>> + * K290_SET_FUNCTION_OFF
>> + * K290_SET_FUNCTION_ON
>> + *
>> + * Based on hid-accutouch.c and hid-elo.c
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +#include <linux/stat.h>
>> +#include <linux/types.h>
>> +#include <linux/usb.h>
>> +
>> +#include "hid-ids.h"
>> +#include "usbhid/usbhid.h"
>> +
>> +// Logitech K290 custom USB command and value to setup function key
>> +#define K290_SET_FUNCTION_CMD 0x02
>> +#define K290_SET_FUNCTION_VAL 0x001a
>> +
>> +// Have function mode turned off (as with standard keyboards)
>> +#define K290_SET_FUNCTION_OFF 0x0001
>> +// Have function mode turned on (default k290 behavior)
>> +#define K290_SET_FUNCTION_ON  0x0000
>> +
>> +// Function key default mode is set at module load time for every K290
>> +// keyboards plugged on the machine. By default fn_mode = 1, i.e
>> +// sending K290_SET_FUNCTION_ON (default K290 behavior).
>> +static bool fn_mode = 1;
>> +module_param(fn_mode, bool, 0444);
>> +MODULE_PARM_DESC(fn_mode, "Logitech K290 function key mode (default = 1)");
>> +
>> +static void k290_set_function(struct usb_device *dev, uint16_t function_mode)
>> +{
>> +       int ret;
>> +
>> +       ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> +                             K290_SET_FUNCTION_CMD,
>> +                             USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>> +                             K290_SET_FUNCTION_VAL,
>> +                             function_mode, 0, 0, USB_CTRL_SET_TIMEOUT);
>> +
>> +       if (ret < 0)
>> +               dev_err(&dev->dev,
>> +                       "Failed to setup K290 function key, error %d\n", ret);
>> +}
>> +
>> +static int k290_set_function_hid_device(struct hid_device *hid)
>> +{
>> +       struct usb_device *usb_dev = hid_to_usb_dev(hid);
>> +
>> +       k290_set_function(usb_dev,
>> +                       fn_mode ? K290_SET_FUNCTION_ON : K290_SET_FUNCTION_OFF);
>> +       return 0;
>> +}
>> +
>> +static int k290_input_configured(struct hid_device *hid,
>> +                                struct hid_input *hidinput)
>> +{
>> +       return k290_set_function_hid_device(hid);
>> +}
>> +
>> +static int k290_resume(struct hid_device *hid)
>> +{
>> +       return k290_set_function_hid_device(hid);
>> +}
>> +
>> +static const struct hid_device_id k290_devices[] = {
>> +       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> +                        USB_DEVICE_ID_LOGITECH_KEYBOARD_K290) },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, k290_devices);
>> +
>> +static struct hid_driver k290_driver = {
>> +       .name = "hid-logitech-k290",
>> +       .id_table = k290_devices,
>> +       .input_configured = k290_input_configured,
>> +       .resume = k290_resume,
>> +       .reset_resume = k290_resume,
>> +};
>> +
>> +module_hid_driver(k290_driver);
>> +
>> +MODULE_AUTHOR("Florent Flament <contact@florentflament.com>");
>> +MODULE_DESCRIPTION("Logitech K290 keyboard driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.14.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-04 22:14   ` [PATCH v2 1/1] HID: " Florent Flament
  2018-03-05  9:31     ` Nestor Lopez Casado
  2018-03-05 15:20     ` kbuild test robot
@ 2018-03-05 21:11     ` kbuild test robot
  2 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-03-05 21:11 UTC (permalink / raw)
  To: Florent Flament
  Cc: kbuild-all, andy.shevchenko, jikos, benjamin.tissoires,
	linux-kernel, linux-input, Florent Flament

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

Hi Florent,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hid/for-next]
[also build test ERROR on v4.16-rc4 next-20180305]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Florent-Flament/Logitech-K290-Add-driver-for-the-Logitech-K290-USB-keyboard/20180305-153311
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All errors (new ones prefixed by >>):

>> drivers/hid/hid-logitech-k290.c:92:3: error: 'struct hid_driver' has no member named 'resume'; did you mean 'remove'?
     .resume = k290_resume,
      ^~~~~~
      remove
>> drivers/hid/hid-logitech-k290.c:92:12: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .resume = k290_resume,
               ^~~~~~~~~~~
   drivers/hid/hid-logitech-k290.c:92:12: note: (near initialization for 'k290_driver.feature_mapping')
>> drivers/hid/hid-logitech-k290.c:93:3: error: 'struct hid_driver' has no member named 'reset_resume'
     .reset_resume = k290_resume,
      ^~~~~~~~~~~~
   drivers/hid/hid-logitech-k290.c:93:18: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .reset_resume = k290_resume,
                     ^~~~~~~~~~~
   drivers/hid/hid-logitech-k290.c:93:18: note: (near initialization for 'k290_driver.bus_add_driver')
   cc1: some warnings being treated as errors

vim +92 drivers/hid/hid-logitech-k290.c

    87	
    88	static struct hid_driver k290_driver = {
    89		.name = "hid-logitech-k290",
    90		.id_table = k290_devices,
    91		.input_configured = k290_input_configured,
  > 92		.resume = k290_resume,
  > 93		.reset_resume = k290_resume,
    94	};
    95	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51495 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-05  9:31     ` Nestor Lopez Casado
  2018-03-05 17:26       ` Benjamin Tissoires
@ 2018-03-05 23:25       ` Florent Flament
  1 sibling, 0 replies; 13+ messages in thread
From: Florent Flament @ 2018-03-05 23:25 UTC (permalink / raw)
  To: Nestor Lopez Casado
  Cc: andy.shevchenko, jikos, Benjamin Tissoires, linux-kernel,
	open list:HID CORE LAYER

On Mon, 2018-03-05 at 10:31 +0100, Nestor Lopez Casado wrote:
> Hello Florent,

Hi Nestor,

> In my view, this driver may not be a good idea. The default behaviour
> of K290 is 'send multimedia keycodes' with the user given the choice
> to change that behaviour via vendor commands. Putting a driver that
> will unconditionally change that behaviour without the user's consent
> might bother other users that prefer the multimedia keycodes by
> default.

Actually, the default behavior of the proposed driver is currently to
let the K290 send multimedia keycodes by default (as if using the
generic HID driver). And this behavior can be changed by using the
fn_mode parameter. We may also add a third behavior consisting in not
doing anything, and letting a user space application managing the
keyboard, which could possibly be the default behavior.

> Besides, I'd argue that instead of a kernel module this would be best
> achieved from a user space application. Something in the lines of
> Solaar (github pwr/solaar) or libratbag (there's an issue open to
> support keyboards) or even a specific application built for the
> purpose. Anyways, please collect the input from Benjamin and Jiri as
> they as they best placed to advise than myself.

Indeed, this driver is based on a working user space application
available there https://github.com/milgner/k290-fnkeyctl . However, I
feel a bit awkward to have to install a dedicated package, or compile &
install an application to have proper support of a keyboard. I feel
like it would be more beautiful to have it supported directly by a
module, like most devices.

Regards,
Florent

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-05 17:26       ` Benjamin Tissoires
@ 2018-03-05 23:31         ` Florent Flament
  2018-03-06 23:11           ` Florent Flament
  0 siblings, 1 reply; 13+ messages in thread
From: Florent Flament @ 2018-03-05 23:31 UTC (permalink / raw)
  To: Benjamin Tissoires, Nestor Lopez Casado
  Cc: andy.shevchenko, Jiri Kosina, lkml, open list:HID CORE LAYER

On Mon, 2018-03-05 at 18:26 +0100, Benjamin Tissoires wrote:
> Hi Florent,

Hi Benjamin,

> 
> On Mon, Mar 5, 2018 at 10:31 AM, Nestor Lopez Casado
> <nlopezcasad@logitech.com> wrote:
> > Hello Florent,
> > 
> > In my view, this driver may not be a good idea. The default
> > behaviour
> > of K290 is 'send multimedia keycodes' with the user given the
> > choice
> > to change that behaviour via vendor commands. Putting a driver that
> > will unconditionally change that behaviour without the user's
> > consent
> > might bother other users that prefer the multimedia keycodes by
> > default.
> > 
> > Besides, I'd argue that instead of a kernel module this would be
> > best
> > achieved from a user space application. Something in the lines of
> > Solaar (github pwr/solaar) or libratbag (there's an issue open to
> > support keyboards) or even a specific application built for the
> > purpose. Anyways, please collect the input from Benjamin and Jiri
> > as
> > they as they best placed to advise than myself.
> 
> On top of what Nestor said, this type of functionality, if we want to
> have them in the kernel should probably be integrated in
> hid-logitech-hidpp, in order not having some magic reports to send.
> 
> Things like reconnect of the device would be handled far more easily
> in hid-logitech-hidpp while you would be reinventing the wheel here.
> 
> One other thing I do not like in this submission of the driver is the
> direct use of USB while we have a full transport agnostic layer
> called
> HID.

Fair enough, I didn't have a look at how hid-logitech-hidpp is working
yet. I'll dig into that to see if this driver can me implemented more
elegantly.

Regards,
Florent

> Cheers,
> Benjamin
> 
> > 
> > Cheers,
> > -nestor
> > 
> > On Sun, Mar 4, 2018 at 11:14 PM, Florent Flament
> > <contact@florentflament.com> wrote:
> > > With the generic HID driver, K290 keyboards' F1 to F12 keys send
> > > multimedia events by default, and standard keycodes when the
> > > function
> > > key is pressed. This driver allows to configure K290 keyboards,
> > > so
> > > that F1 to F12 have a standard behavior and send multimedia
> > > events
> > > when the function key is pressed. The keyboard mode is set
> > > through the
> > > fn_mode module parameter: when set to 1 (default setting) the
> > > keyboard
> > > behaves as with the generic HID driver, when set to 0 the
> > > keyboard is
> > > configured to work as standard keyboards.
> > > 
> > > Signed-off-by: Florent Flament <contact@florentflament.com>
> > > ---
> > >  drivers/hid/Kconfig             |  18 ++++++++
> > >  drivers/hid/Makefile            |   1 +
> > >  drivers/hid/hid-ids.h           |   1 +
> > >  drivers/hid/hid-logitech-k290.c | 100
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 120 insertions(+)
> > >  create mode 100644 drivers/hid/hid-logitech-k290.c
> > > 
> > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > index 19c499f5623d..6686da8daac6 100644
> > > --- a/drivers/hid/Kconfig
> > > +++ b/drivers/hid/Kconfig
> > > @@ -488,6 +488,24 @@ config HID_LOGITECH_HIDPP
> > >         T651, TK820), some mice (Zone Touch mouse), or even
> > > keyboards (Solar
> > >         Keyboard).
> > > 
> > > +config HID_LOGITECH_K290
> > > +       tristate "Logitech K290 Keyboard support"
> > > +       depends on USB_HID
> > > +       ---help---
> > > +       This enhances support of Logitech K290 keyboards.
> > > +
> > > +       With the generic HID driver, K290 keyboards' F1 to F12
> > > keys
> > > +       send multimedia events by default, and standard keycodes
> > > when
> > > +       the function key is pressed. This driver allows to
> > > configure
> > > +       K290 keyboards, so that F1 to F12 have a standard
> > > behavior and
> > > +       send multimedia events when the function key is pressed.
> > > The
> > > +       keyboard mode is set through the fn_mode module
> > > parameter:
> > > +       when set to 1 (default setting) the keyboard behaves as
> > > with
> > > +       the generic HID driver, when set to 0 the keyboard is
> > > +       configured to work as standard keyboards.
> > > +
> > > +       Say Y if you have a Logitech K290 keyboard.
> > > +
> > >  config LOGITECH_FF
> > >         bool "Logitech force feedback support"
> > >         depends on HID_LOGITECH
> > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > index eb13b9e92d85..78079d3a5d58 100644
> > > --- a/drivers/hid/Makefile
> > > +++ b/drivers/hid/Makefile
> > > @@ -61,6 +61,7 @@ obj-$(CONFIG_HID_LENOVO)      += hid-lenovo.o
> > >  obj-$(CONFIG_HID_LOGITECH)     += hid-logitech.o
> > >  obj-$(CONFIG_HID_LOGITECH_DJ)  += hid-logitech-dj.o
> > >  obj-$(CONFIG_HID_LOGITECH_HIDPP)       += hid-logitech-hidpp.o
> > > +obj-$(CONFIG_HID_LOGITECH_K290)        += hid-logitech-k290.o
> > >  obj-$(CONFIG_HID_MAGICMOUSE)   += hid-magicmouse.o
> > >  obj-$(CONFIG_HID_MAYFLASH)     += hid-mf.o
> > >  obj-$(CONFIG_HID_MICROSOFT)    += hid-microsoft.o
> > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > index 9454ac134ce2..68caba7e666c 100644
> > > --- a/drivers/hid/hid-ids.h
> > > +++ b/drivers/hid/hid-ids.h
> > > @@ -693,6 +693,7 @@
> > >  #define USB_DEVICE_ID_LOGITECH_HARMONY_LAST 0xc14f
> > >  #define USB_DEVICE_ID_LOGITECH_HARMONY_PS3 0x0306
> > >  #define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
> > > +#define USB_DEVICE_ID_LOGITECH_KEYBOARD_K290   0xc31f
> > >  #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A      0xc01a
> > >  #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A      0xc05a
> > >  #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A      0xc06a
> > > diff --git a/drivers/hid/hid-logitech-k290.c b/drivers/hid/hid-
> > > logitech-k290.c
> > > new file mode 100644
> > > index 000000000000..36fdb5838842
> > > --- /dev/null
> > > +++ b/drivers/hid/hid-logitech-k290.c
> > > @@ -0,0 +1,100 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * HID driver for Logitech K290 keyboard
> > > + *
> > > + * Copyright (c) 2018 Florent Flament
> > > + *
> > > + * This drivers allows to configure the K290 keyboard's function
> > > key
> > > + * behaviour (whether function mode is activated or not by
> > > default).
> > > + *
> > > + * Logitech custom commands taken from Marcus Ilgner k290-
> > > fnkeyctl
> > > + * (https://github.com/milgner/k290-fnkeyctl):
> > > + * K290_SET_FUNCTION_CMD
> > > + * K290_SET_FUNCTION_VAL
> > > + * K290_SET_FUNCTION_OFF
> > > + * K290_SET_FUNCTION_ON
> > > + *
> > > + * Based on hid-accutouch.c and hid-elo.c
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/hid.h>
> > > +#include <linux/module.h>
> > > +#include <linux/stat.h>
> > > +#include <linux/types.h>
> > > +#include <linux/usb.h>
> > > +
> > > +#include "hid-ids.h"
> > > +#include "usbhid/usbhid.h"
> > > +
> > > +// Logitech K290 custom USB command and value to setup function
> > > key
> > > +#define K290_SET_FUNCTION_CMD 0x02
> > > +#define K290_SET_FUNCTION_VAL 0x001a
> > > +
> > > +// Have function mode turned off (as with standard keyboards)
> > > +#define K290_SET_FUNCTION_OFF 0x0001
> > > +// Have function mode turned on (default k290 behavior)
> > > +#define K290_SET_FUNCTION_ON  0x0000
> > > +
> > > +// Function key default mode is set at module load time for
> > > every K290
> > > +// keyboards plugged on the machine. By default fn_mode = 1, i.e
> > > +// sending K290_SET_FUNCTION_ON (default K290 behavior).
> > > +static bool fn_mode = 1;
> > > +module_param(fn_mode, bool, 0444);
> > > +MODULE_PARM_DESC(fn_mode, "Logitech K290 function key mode
> > > (default = 1)");
> > > +
> > > +static void k290_set_function(struct usb_device *dev, uint16_t
> > > function_mode)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> > > +                             K290_SET_FUNCTION_CMD,
> > > +                             USB_DIR_OUT | USB_TYPE_VENDOR |
> > > USB_RECIP_DEVICE,
> > > +                             K290_SET_FUNCTION_VAL,
> > > +                             function_mode, 0, 0,
> > > USB_CTRL_SET_TIMEOUT);
> > > +
> > > +       if (ret < 0)
> > > +               dev_err(&dev->dev,
> > > +                       "Failed to setup K290 function key, error
> > > %d\n", ret);
> > > +}
> > > +
> > > +static int k290_set_function_hid_device(struct hid_device *hid)
> > > +{
> > > +       struct usb_device *usb_dev = hid_to_usb_dev(hid);
> > > +
> > > +       k290_set_function(usb_dev,
> > > +                       fn_mode ? K290_SET_FUNCTION_ON :
> > > K290_SET_FUNCTION_OFF);
> > > +       return 0;
> > > +}
> > > +
> > > +static int k290_input_configured(struct hid_device *hid,
> > > +                                struct hid_input *hidinput)
> > > +{
> > > +       return k290_set_function_hid_device(hid);
> > > +}
> > > +
> > > +static int k290_resume(struct hid_device *hid)
> > > +{
> > > +       return k290_set_function_hid_device(hid);
> > > +}
> > > +
> > > +static const struct hid_device_id k290_devices[] = {
> > > +       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > +                        USB_DEVICE_ID_LOGITECH_KEYBOARD_K290) },
> > > +       { }
> > > +};
> > > +MODULE_DEVICE_TABLE(hid, k290_devices);
> > > +
> > > +static struct hid_driver k290_driver = {
> > > +       .name = "hid-logitech-k290",
> > > +       .id_table = k290_devices,
> > > +       .input_configured = k290_input_configured,
> > > +       .resume = k290_resume,
> > > +       .reset_resume = k290_resume,
> > > +};
> > > +
> > > +module_hid_driver(k290_driver);
> > > +
> > > +MODULE_AUTHOR("Florent Flament <contact@florentflament.com>");
> > > +MODULE_DESCRIPTION("Logitech K290 keyboard driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.14.3
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-
> > > input" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.htm
> > > l

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-05 23:31         ` Florent Flament
@ 2018-03-06 23:11           ` Florent Flament
  0 siblings, 0 replies; 13+ messages in thread
From: Florent Flament @ 2018-03-06 23:11 UTC (permalink / raw)
  To: Benjamin Tissoires, Nestor Lopez Casado
  Cc: andy.shevchenko, Jiri Kosina, lkml, open list:HID CORE LAYER

Hi Benjamin, Nestor,

On Tue, 2018-03-06 at 00:31 +0100, Florent Flament wrote:
> On Mon, 2018-03-05 at 18:26 +0100, Benjamin Tissoires wrote:
> > Hi Florent,
> 
> Hi Benjamin,
> 
> > 
> > On Mon, Mar 5, 2018 at 10:31 AM, Nestor Lopez Casado
> > <nlopezcasad@logitech.com> wrote:
> > > Hello Florent,
> > > 
> > > In my view, this driver may not be a good idea. The default
> > > behaviour
> > > of K290 is 'send multimedia keycodes' with the user given the
> > > choice
> > > to change that behaviour via vendor commands. Putting a driver
> > > that
> > > will unconditionally change that behaviour without the user's
> > > consent
> > > might bother other users that prefer the multimedia keycodes by
> > > default.
> > > 
> > > Besides, I'd argue that instead of a kernel module this would be
> > > best
> > > achieved from a user space application. Something in the lines of
> > > Solaar (github pwr/solaar) or libratbag (there's an issue open to
> > > support keyboards) or even a specific application built for the
> > > purpose. Anyways, please collect the input from Benjamin and Jiri
> > > as
> > > they as they best placed to advise than myself.
> > 
> > On top of what Nestor said, this type of functionality, if we want
> > to
> > have them in the kernel should probably be integrated in
> > hid-logitech-hidpp, in order not having some magic reports to send.
> > 
> > Things like reconnect of the device would be handled far more
> > easily
> > in hid-logitech-hidpp while you would be reinventing the wheel
> > here.
> > 
> > One other thing I do not like in this submission of the driver is
> > the
> > direct use of USB while we have a full transport agnostic layer
> > called
> > HID.
> 
> Fair enough, I didn't have a look at how hid-logitech-hidpp is
> working
> yet. I'll dig into that to see if this driver can me implemented more
> elegantly.

I had a closer look at how the HID layer is interacting with the USB
layer. And as far as I understand, the only way to send a message to
the USB control endpoint from the HID layer is through the
hid_submit_ctrl function in drivers/hid/usbhid/hid-core.c, which does
this:

usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;
usbhid->cr->bRequest = (dir == USB_DIR_OUT) ? HID_REQ_SET_REPORT :
					      HID_REQ_GET_REPORT;
usbhid->cr->wValue = cpu_to_le16(((report->type + 1) << 8) |
				 report->id);
usbhid->cr->wIndex = cpu_to_le16(usbhid->ifnum);
usbhid->cr->wLength = cpu_to_le16(len);

dbg_hid("submitting ctrl urb: %s wValue=0x%04x wIndex=0x%04x
wLength=%u\n",
	usbhid->cr->bRequest == HID_REQ_SET_REPORT ? "Set_Report" :
						     "Get_Report",
	usbhid->cr->wValue, usbhid->cr->wIndex, usbhid->cr->wLength);

r = usb_submit_urb(usbhid->urbctrl, GFP_ATOMIC);


While this is probably fine for most HID devices, some devices (like
the Logitech K290) need to receive a vendor specific request directly
adressed to the device (i.e bRequestType = USB_TYPE_VENDOR |
USB_RECIPE_DEVICE). While in the hid_submit_ctrl function, the
bRequestType is hardcoded to USB_TYPE_CLASS | USB_RECIP_INTERFACE.

So it looks like the mechanism used by Logitech to allow switching its
K290 keyboard behavior is not HID compliant and requires to forge a
custom USB request.

Apparently this keyboard is not the only device that requires the same
kind of custom USB requests. If we look at the hid-elo driver, the
same usb_control_msg calls are performed in elo_smartset_send_get:

return usb_control_msg(dev, pipe, command,
		dir | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
		0, 0, data, ELO_SMARTSET_PACKET_SIZE,
		ELO_SMARTSET_CMD_TIMEOUT);

and in elo_flush_smartset_responses:

return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
		ELO_FLUSH_SMARTSET_RESPONSES,
		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
		0, 0, NULL, 0, USB_CTRL_SET_TIMEOUT);


So far, I don't think that it's feasible to send the control message
required to toggle the keyboard behavior from the HID layer, though I'd
be glad to have your thoughts.

Regards,
Florent Flament

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-03 16:08 ` Andy Shevchenko
@ 2018-03-04 16:51   ` Florent Flament
  0 siblings, 0 replies; 13+ messages in thread
From: Florent Flament @ 2018-03-04 16:51 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jiri Kosina, Benjamin Tissoires, Linux Kernel Mailing List, linux-input

On Sat, 2018-03-03 at 18:08 +0200, Andy Shevchenko wrote:
> On Sat, Mar 3, 2018 at 12:04 AM, Florent Flament
> <contact@florentflament.com> wrote:
> > With the generic HID driver, K290 keyboards' F1 to F12 keys send
> > multimedia events by default, and standard keycodes when the
> > function
> > key is pressed. This driver allows to configure K290 keyboards, so
> > that F1 to F12 have a standard behavior and send multimedia events
> > when the function key is pressed. The keyboard mode is set through
> > the
> > fn_mode module parameter: when set to 1 (default setting) the
> > keyboard
> > behaves as with the generic HID driver, when set to 0 the keyboard
> > is
> > configured to work as standard keyboards.
> 
> SPDX ID?
> 
> > +/*
> > + * HID driver for Logitech K290 keyboard
> > + *
> > + * Copyright (c) 2018 Florent Flament
> > + *
> > + * This drivers allows to configure the K290 keyboard's function
> > key
> > + * behaviour (whether function mode is activated or not by
> > default).
> > + *
> > + * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
> > + * (https://github.com/milgner/k290-fnkeyctl):
> > + * K290_SET_FUNCTION_CMD
> > + * K290_SET_FUNCTION_VAL
> > + * K290_SET_FUNCTION_OFF
> > + * K290_SET_FUNCTION_ON
> > + *
> > + * Based on hid-accutouch.c and hid-elo.c
> > + *
> > + * This driver is licensed under the terms of GPLv2.
> 
> ...instead of this.

Fine, I'll replace this with the appropriate SPDX ID.

> 
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/mod_devicetable.h>
> 
> Do you need these both? I suppose module.h effectively provides them.

Indeed, module.h is enough. The 2 following includes are not necessary.
Fixing this.

> 
> > +#include <linux/stat.h>
> > +#include <linux/types.h>
> > +#include <linux/usb.h>
> 
> 

Regards,
Florent

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
  2018-03-02 22:04 [PATCH] " Florent Flament
@ 2018-03-03 16:08 ` Andy Shevchenko
  2018-03-04 16:51   ` Florent Flament
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2018-03-03 16:08 UTC (permalink / raw)
  To: Florent Flament
  Cc: Jiri Kosina, Benjamin Tissoires, Linux Kernel Mailing List, linux-input

On Sat, Mar 3, 2018 at 12:04 AM, Florent Flament
<contact@florentflament.com> wrote:
> With the generic HID driver, K290 keyboards' F1 to F12 keys send
> multimedia events by default, and standard keycodes when the function
> key is pressed. This driver allows to configure K290 keyboards, so
> that F1 to F12 have a standard behavior and send multimedia events
> when the function key is pressed. The keyboard mode is set through the
> fn_mode module parameter: when set to 1 (default setting) the keyboard
> behaves as with the generic HID driver, when set to 0 the keyboard is
> configured to work as standard keyboards.

SPDX ID?

> +/*
> + * HID driver for Logitech K290 keyboard
> + *
> + * Copyright (c) 2018 Florent Flament
> + *
> + * This drivers allows to configure the K290 keyboard's function key
> + * behaviour (whether function mode is activated or not by default).
> + *
> + * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
> + * (https://github.com/milgner/k290-fnkeyctl):
> + * K290_SET_FUNCTION_CMD
> + * K290_SET_FUNCTION_VAL
> + * K290_SET_FUNCTION_OFF
> + * K290_SET_FUNCTION_ON
> + *
> + * Based on hid-accutouch.c and hid-elo.c
> + *

> + * This driver is licensed under the terms of GPLv2.

...instead of this.

> + */

> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>

> +#include <linux/moduleparam.h>
> +#include <linux/mod_devicetable.h>

Do you need these both? I suppose module.h effectively provides them.

> +#include <linux/stat.h>
> +#include <linux/types.h>
> +#include <linux/usb.h>

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
@ 2018-03-02 22:04 Florent Flament
  2018-03-03 16:08 ` Andy Shevchenko
  0 siblings, 1 reply; 13+ messages in thread
From: Florent Flament @ 2018-03-02 22:04 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, linux-kernel, linux-input; +Cc: Florent Flament

With the generic HID driver, K290 keyboards' F1 to F12 keys send
multimedia events by default, and standard keycodes when the function
key is pressed. This driver allows to configure K290 keyboards, so
that F1 to F12 have a standard behavior and send multimedia events
when the function key is pressed. The keyboard mode is set through the
fn_mode module parameter: when set to 1 (default setting) the keyboard
behaves as with the generic HID driver, when set to 0 the keyboard is
configured to work as standard keyboards.

Signed-off-by: Florent Flament <contact@florentflament.com>
---
 drivers/hid/Kconfig             |  18 +++++++
 drivers/hid/Makefile            |   1 +
 drivers/hid/hid-ids.h           |   1 +
 drivers/hid/hid-logitech-k290.c | 103 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+)
 create mode 100644 drivers/hid/hid-logitech-k290.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6686da8daac6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -488,6 +488,24 @@ config HID_LOGITECH_HIDPP
 	T651, TK820), some mice (Zone Touch mouse), or even keyboards (Solar
 	Keyboard).
 
+config HID_LOGITECH_K290
+	tristate "Logitech K290 Keyboard support"
+	depends on USB_HID
+	---help---
+	This enhances support of Logitech K290 keyboards.
+
+	With the generic HID driver, K290 keyboards' F1 to F12 keys
+	send multimedia events by default, and standard keycodes when
+	the function key is pressed. This driver allows to configure
+	K290 keyboards, so that F1 to F12 have a standard behavior and
+	send multimedia events when the function key is pressed. The
+	keyboard mode is set through the fn_mode module parameter:
+	when set to 1 (default setting) the keyboard behaves as with
+	the generic HID driver, when set to 0 the keyboard is
+	configured to work as standard keyboards.
+
+	Say Y if you have a Logitech K290 keyboard.
+
 config LOGITECH_FF
 	bool "Logitech force feedback support"
 	depends on HID_LOGITECH
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92d85..78079d3a5d58 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
 obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
 obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
 obj-$(CONFIG_HID_LOGITECH_HIDPP)	+= hid-logitech-hidpp.o
+obj-$(CONFIG_HID_LOGITECH_K290)	+= hid-logitech-k290.o
 obj-$(CONFIG_HID_MAGICMOUSE)	+= hid-magicmouse.o
 obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 9454ac134ce2..68caba7e666c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -693,6 +693,7 @@
 #define USB_DEVICE_ID_LOGITECH_HARMONY_LAST 0xc14f
 #define USB_DEVICE_ID_LOGITECH_HARMONY_PS3 0x0306
 #define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
+#define USB_DEVICE_ID_LOGITECH_KEYBOARD_K290	0xc31f
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A	0xc01a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A	0xc05a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A	0xc06a
diff --git a/drivers/hid/hid-logitech-k290.c b/drivers/hid/hid-logitech-k290.c
new file mode 100644
index 000000000000..37045da8cc02
--- /dev/null
+++ b/drivers/hid/hid-logitech-k290.c
@@ -0,0 +1,103 @@
+/*
+ * HID driver for Logitech K290 keyboard
+ *
+ * Copyright (c) 2018 Florent Flament
+ *
+ * This drivers allows to configure the K290 keyboard's function key
+ * behaviour (whether function mode is activated or not by default).
+ *
+ * Logitech custom commands taken from Marcus Ilgner k290-fnkeyctl
+ * (https://github.com/milgner/k290-fnkeyctl):
+ * K290_SET_FUNCTION_CMD
+ * K290_SET_FUNCTION_VAL
+ * K290_SET_FUNCTION_OFF
+ * K290_SET_FUNCTION_ON
+ *
+ * Based on hid-accutouch.c and hid-elo.c
+ *
+ * This driver is licensed under the terms of GPLv2.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mod_devicetable.h>
+#include <linux/stat.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+#include "hid-ids.h"
+#include "usbhid/usbhid.h"
+
+// Logitech K290 custom USB command and value to setup function key
+#define	K290_SET_FUNCTION_CMD 0x02
+#define K290_SET_FUNCTION_VAL 0x001a
+
+// Have function mode turned off (as with standard keyboards)
+#define K290_SET_FUNCTION_OFF 0x0001
+// Have function mode turned on (default k290 behavior)
+#define K290_SET_FUNCTION_ON  0x0000
+
+// Function key default mode is set at module load time for every K290
+// keyboards plugged on the machine. By default fn_mode = 1, i.e
+// sending K290_SET_FUNCTION_ON (default K290 behavior).
+static bool fn_mode = 1;
+module_param(fn_mode, bool, 0444);
+MODULE_PARM_DESC(fn_mode, "Logitech K290 function key mode (default = 1)");
+
+static void k290_set_function(struct usb_device *dev, uint16_t function_mode)
+{
+	int ret;
+
+	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+			      K290_SET_FUNCTION_CMD,
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      K290_SET_FUNCTION_VAL,
+			      function_mode, 0, 0, USB_CTRL_SET_TIMEOUT);
+
+	if (ret < 0)
+		dev_err(&dev->dev,
+			"Failed to setup K290 function key, error %d\n", ret);
+}
+
+static int k290_set_function_hid_device(struct hid_device *hid)
+{
+	struct usb_device *usb_dev = hid_to_usb_dev(hid);
+
+	k290_set_function(usb_dev,
+			fn_mode ? K290_SET_FUNCTION_ON : K290_SET_FUNCTION_OFF);
+	return 0;
+}
+
+static int k290_input_configured(struct hid_device *hid,
+				 struct hid_input *hidinput)
+{
+	return k290_set_function_hid_device(hid);
+}
+
+static int k290_resume(struct hid_device *hid)
+{
+	return k290_set_function_hid_device(hid);
+}
+
+static const struct hid_device_id k290_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_KEYBOARD_K290) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, k290_devices);
+
+static struct hid_driver k290_driver = {
+	.name = "hid-logitech-k290",
+	.id_table = k290_devices,
+	.input_configured = k290_input_configured,
+	.resume = k290_resume,
+	.reset_resume = k290_resume,
+};
+
+module_hid_driver(k290_driver);
+
+MODULE_AUTHOR("Florent Flament <contact@florentflament.com>");
+MODULE_DESCRIPTION("Logitech K290 keyboard driver");
+MODULE_LICENSE("GPL v2");
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-03-07  0:28 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-04 17:00 [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard Florent Flament
2018-03-04 22:14 ` [PATCH v2 0/1] " Florent Flament
2018-03-04 22:14   ` [PATCH v2 1/1] HID: " Florent Flament
2018-03-05  9:31     ` Nestor Lopez Casado
2018-03-05 17:26       ` Benjamin Tissoires
2018-03-05 23:31         ` Florent Flament
2018-03-06 23:11           ` Florent Flament
2018-03-05 23:25       ` Florent Flament
2018-03-05 15:20     ` kbuild test robot
2018-03-05 21:11     ` kbuild test robot
  -- strict thread matches above, loose matches on Subject: below --
2018-03-02 22:04 [PATCH] " Florent Flament
2018-03-03 16:08 ` Andy Shevchenko
2018-03-04 16:51   ` Florent Flament

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).