* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
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
In-Reply-To: <CAE7qMrpF4dGCUrtnXbxb4MLFPZ0Wf13mrQEPypwPbEHaB9e44A@mail.gmail.com>
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
* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
From: kbuild test robot @ 2018-03-05 15:20 UTC (permalink / raw)
Cc: kbuild-all, andy.shevchenko, jikos, benjamin.tissoires,
linux-kernel, linux-input, Florent Flament
In-Reply-To: <20180304221425.28611-2-contact@florentflament.com>
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
* Re: [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
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
In-Reply-To: <20180304221425.28611-2-contact@florentflament.com>
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
* [PATCH v2 1/1] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
From: Florent Flament @ 2018-03-04 22:14 UTC (permalink / raw)
To: andy.shevchenko
Cc: jikos, benjamin.tissoires, linux-kernel, linux-input,
Florent Flament
In-Reply-To: <20180304221425.28611-1-contact@florentflament.com>
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
* [PATCH v2 0/1] Logitech K290: Add driver for the Logitech K290 USB keyboard
From: Florent Flament @ 2018-03-04 22:14 UTC (permalink / raw)
To: andy.shevchenko
Cc: jikos, benjamin.tissoires, linux-kernel, linux-input,
Florent Flament
In-Reply-To: <20180304170004.26553-1-contact@florentflament.com>
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
* [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
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
* Re: [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
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
In-Reply-To: <CAHp75VeLaXb+wOUJOXojOwXSm68=oaiA7=RCKH_tOsBT7OzUxg@mail.gmail.com>
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
* Re: [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
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
In-Reply-To: <20180302220425.27707-1-contact@florentflament.com>
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
* Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action
From: Brian Norris @ 2018-03-03 1:10 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, briannorris, heiko, dtor, dianders, Guenter Roeck,
Enric Balletbo i Serra, Thomas Gleixner, Joseph Lo, stephen lu,
Dmitry Torokhov, Kate Stewart, linux-input, Greg Kroah-Hartman,
Philippe Ombredanne, Arvind Yadav
In-Reply-To: <20180302035102.10084-2-jeffy.chen@rock-chips.com>
Hi Jeffy,
On Fri, Mar 02, 2018 at 11:51:00AM +0800, Jeffy Chen wrote:
> Add support for specifying event actions to trigger wakeup when using
> the gpio-keys input device as a wakeup source.
>
> This would allow the device to configure when to wakeup the system. For
> example a gpio-keys input device for pen insert, may only want to wakeup
> the system when ejecting the pen.
>
> Suggested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v3:
> Adding more comments as Brian suggested.
>
> Changes in v2:
> Specify wakeup event action instead of irq trigger type as Brian
> suggested.
>
> drivers/input/keyboard/gpio_keys.c | 39 ++++++++++++++++++++++++++++++++++
> include/linux/gpio_keys.h | 2 ++
> include/uapi/linux/input-event-codes.h | 9 ++++++++
> 3 files changed, 50 insertions(+)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 87e613dc33b8..607f3960c886 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
...
> @@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>
> isr = gpio_keys_irq_isr;
> irqflags = 0;
> +
> + /*
> + * For IRQ buttons, the irq trigger type for press and release
> + * are the same. So we don't need to reconfigure the trigger
> + * type for wakeup.
> + */
Ah, I think I misunderstood what the (non-GPIO) IRQ-based trigger was
doing. It's just emulating an instantaneous press/release (or, with a
configurable release delay), so there isn't really a way to say "only
wake me on one edge".
So that still doesn't really mean we use the "same" trigger for all
wakeup trigger types. It really just means we can't support any
customization here -- we wake based on whatever the IRQ represents
(which today is probably just EV_ACT_ANY).
I guess it's fine to just ignore the wakeup-trigger-type property here,
but it would also be equally valid to reject it outright I think.
Anyway, that's only a mild complaint. For the whole series:
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Thanks!
> }
>
> bdata->code = &ddata->keymap[idx];
...
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 53fbae27b280..d7917b0bd438 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -32,6 +32,15 @@
> #define INPUT_PROP_CNT (INPUT_PROP_MAX + 1)
>
> /*
> + * Event action types
> + */
> +#define EV_ACT_ANY 0x00 /* asserted or deasserted */
> +#define EV_ACT_ASSERTED 0x01 /* asserted */
> +#define EV_ACT_DEASSERTED 0x02 /* deasserted */
> +#define EV_ACT_MAX 0x02
> +#define EV_ACT_CNT (EV_ACT_MAX+1)
> +
> +/*
> * Event types
> */
>
> --
> 2.11.0
>
>
^ permalink raw reply
* [PATCH] HID: Logitech K290: Add driver for the Logitech K290 USB keyboard
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
* [PATCH] Input: trackpoint: document sysfs interface
From: Aishwarya Pant @ 2018-03-02 17:30 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input, linux-kernel, Jonathan Corbet,
Greg KH
Cc: Julia Lawall, linux-doc
Descriptions have been collected from git commit logs, code commits and
the TrackPoint System Version 4.0 Engineering Specification.
Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
.../ABI/testing/sysfs-devices-platform-trackpoint | 115 +++++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-trackpoint
diff --git a/Documentation/ABI/testing/sysfs-devices-platform-trackpoint b/Documentation/ABI/testing/sysfs-devices-platform-trackpoint
new file mode 100644
index 000000000000..df11901a6b3d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-platform-trackpoint
@@ -0,0 +1,115 @@
+What: /sys/devices/platform/i8042/.../sensitivity
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Trackpoint sensitivity.
+
+What: /sys/devices/platform/i8042/.../intertia
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Negative inertia factor. High values cause the cursor to
+ snap backward when the trackpoint is released.
+
+What: /sys/devices/platform/i8042/.../reach
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Backup range for z-axis press.
+
+What: /sys/devices/platform/i8042/.../draghys
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) The drag hysteresis controls how hard it is to drag with
+ z-axis pressed.
+
+What: /sys/devices/platform/i8042/.../mindrag
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Minimum amount of force needed to trigger dragging.
+
+What: /sys/devices/platform/i8042/.../speed
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Speed of the trackpoint cursor.
+
+What: /sys/devices/platform/i8042/.../thresh
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Minimum value for z-axis force required to trigger a press
+ or release, relative to the running average.
+
+What: /sys/devices/platform/i8042/.../upthresh
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) The offset from the running average required to generate a
+ select (click) on z-axis on release.
+
+What: /sys/devices/platform/i8042/.../ztime
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) This attribute determines how sharp a press has to be in
+ order to be recognized.
+
+What: /sys/devices/platform/i8042/.../jenks
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Minimum curvature in degrees required to generate a double
+ click without a release.
+
+What: /sys/devices/platform/i8042/.../skipback
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) When the skipback bit is set, backup cursor movement during
+ releases from drags will be suppressed. The default value for
+ this bit is 0.
+
+What: /sys/devices/platform/i8042/.../ext_dev
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Disable (0) or enable (1) external pointing device.
+
+What: /sys/devices/platform/i8042/.../press_to_select
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Writing a value of 1 to this file will enable the Press to
+ Select functions like tapping the control stick to simulate a
+ left click, and writing 0 will disable it.
+
+What: /sys/devices/platform/i8042/.../drift_time
+Date: Dec, 2014
+KernelVersion: 3.19
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) This parameter controls the period of time to test for a
+ ‘hands off’ condition (i.e. when no force is applied) before a
+ drift (noise) calibration occurs.
+
+ IBM Trackpoints have a feature to compensate for drift by
+ recalibrating themselves periodically. By default, if for 0.5
+ seconds there is no change in position, it's used as the new
+ zero. This duration is too low. Often, the calibration happens
+ when the trackpoint is in fact being used.
--
2.16.2
^ permalink raw reply related
* [PATCH] HID: logitech-hidpp: document sysfs interface
From: Aishwarya Pant @ 2018-03-02 13:16 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Jonathan Corbet, Greg KH
Cc: Julia Lawall, linux-doc
Descriptions have been collected from git commit logs.
Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
.../ABI/testing/sysfs-driver-hid-logitech-hidpp | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp
diff --git a/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp b/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp
new file mode 100644
index 000000000000..d8f831f2d6b5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp
@@ -0,0 +1,19 @@
+What: /sys/bus/hid/drivers/logitech-hidpp-device/<dev>/range
+Date: Jan, 2016
+KernelVersion: 4.6
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) This attribute controls the amount of 'turn' permitted in
+ Logitech G920 wheel. Reading from the file shows the current
+ range of the steering wheel. Writing a value within the min and
+ max boundary sets the range of the wheel.
+
+What: /sys/bus/hid/drivers/logitech-hidpp-device/<dev>/builtin_power_supply
+Date: Apr, 2017
+KernelVersion: 4.12
+Contact: linux-input@vger.kernel.org
+Description:
+ Presence of this file indicates that HID++ driver is capable of
+ handling battery properties in the kernel. This way, upower can
+ add a udev rule to decide whether or not it should use the
+ internal unifying support or the generic kernel one.
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()
From: Claudiu Beznea @ 2018-03-02 9:28 UTC (permalink / raw)
To: Jani Nikula, Thierry Reding
Cc: mark.rutland, milo.kim, linux-fbdev, sean, devicetree, airlied,
mturquette, joonas.lahtinen, kamil, dri-devel, linux-kernel,
alexandre.belloni, pavel, lee.jones, linux-clk, linux-leds,
daniel.thompson, linux-samsung-soc, shc_work, corbet, linux-doc,
linux, krzk, kgene, linux-input, linux, linux-media, linux-pwm,
jdelvare, b.zolnierkie, intel-gfx, robh+dt, jacek.anaszewski,
rodrigo.vivi, mchehab, linux-arm-kernel
In-Reply-To: <87r2p4hod7.fsf@intel.com>
On 28.02.2018 22:04, Jani Nikula wrote:
> On Wed, 28 Feb 2018, Thierry Reding <thierry.reding@gmail.com> wrote:
>> Anyone that needs something other than normal mode should use the new
>> atomic PWM API.
>
> At the risk of revealing my true ignorance, what is the new atomic PWM
> API? Where? Examples of how one would convert old code over to the new
> API?
As far as I know, the old PWM core code uses config(), set_polarity(),
enable(), disable() methods of driver, registered as pwm_ops:
struct pwm_ops {
int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);
void (*free)(struct pwm_chip *chip, struct pwm_device *pwm);
int (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
int duty_ns, int period_ns);
int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,
enum pwm_polarity polarity);
int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_capture *result, unsigned long timeout);
int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state);
void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state);
#ifdef CONFIG_DEBUG_FS
void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s);
#endif
struct module *owner;
};
to do settings on hardware. In order to so settings on a PWM the users
should have been follow the below steps:
->config()
->set_polarity()
->enable()
Moreover, if the PWM was previously enabled it should have been first
disable and then to follow the above steps in order to apply a new settings
on hardware.
The driver should have been provide, at probe, all the above function:
->config(), ->set_polarity(), ->disable(), ->enable(), function that were
used by PWM core.
Now, having atomic PWM, the driver should provide one function to PWM core,
which is ->apply() function. Every PWM has a state associated, which keeps
the period, duty cycle, polarity and enable/disable status. The driver's
->apply() function takes as argument the state that should be applied and
it takes care of applying this new state directly without asking user to
call ->disable(), then ->config()/->set_polarity(), then ->enable() to
apply new hardware settings.
The PWM consumer could set a new state for PWM it uses, using
pwm_apply_state(pwm, new_state);
Regarding the models to switch on atomic PWM, on the controller side you
can check for drivers that registers apply function at probe time.
Regarding the PWM users, you can look for pwm_apply_state()
(drivers/hwmon/pwm-fan.c or drivers/input/misc/pwm-beeper.c are some examples).
Thierry, please correct me if I'm wrong.
Thank you,
Claudiu Beznea
>
> BR,
> Jani.
>
^ permalink raw reply
* Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()
From: Claudiu Beznea @ 2018-03-02 9:19 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, milo.kim, linux-fbdev, sean, devicetree, airlied,
mturquette, joonas.lahtinen, kamil, dri-devel, linux-kernel,
alexandre.belloni, pavel, lee.jones, linux-clk, linux-leds,
daniel.thompson, linux-samsung-soc, shc_work, corbet, linux-doc,
linux, krzk, kgene, linux-input, linux, linux-media, linux-pwm,
jdelvare, b.zolnierkie, intel-gfx, jani.nikula, robh+dt,
jacek.anaszewski, rodrigo.vivi, mchehab
In-Reply-To: <20180228194429.GD22932@mithrandir>
On 28.02.2018 21:44, Thierry Reding wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>> arch/arm/mach-s3c24xx/mach-rx1950.c | 11 +++++++++--
>> drivers/bus/ts-nbus.c | 2 +-
>> drivers/clk/clk-pwm.c | 3 ++-
>> drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
>> drivers/hwmon/pwm-fan.c | 2 +-
>> drivers/input/misc/max77693-haptic.c | 2 +-
>> drivers/input/misc/max8997_haptic.c | 6 +++++-
>> drivers/leds/leds-pwm.c | 5 ++++-
>> drivers/media/rc/ir-rx51.c | 5 ++++-
>> drivers/media/rc/pwm-ir-tx.c | 5 ++++-
>> drivers/video/backlight/lm3630a_bl.c | 4 +++-
>> drivers/video/backlight/lp855x_bl.c | 4 +++-
>> drivers/video/backlight/lp8788_bl.c | 5 ++++-
>> drivers/video/backlight/pwm_bl.c | 11 +++++++++--
>> drivers/video/fbdev/ssd1307fb.c | 3 ++-
>> include/linux/pwm.h | 6 ++++--
>> 16 files changed, 70 insertions(+), 21 deletions(-)
>
> I don't think it makes sense to leak mode support into the legacy API.
> The pwm_config() function is considered legacy
I missed this aspect.
and should eventually go
> away. As such it doesn't make sense to integrate a new feature such as
> PWM modes into it.
Agree.
All users of pwm_config() assume normal mode, and
> that's what pwm_config() should provide.
Agree.
>
> Anyone that needs something other than normal mode should use the new
> atomic PWM API.
Agree.
>
> Thierry
>
^ permalink raw reply
* [PATCH] HID: ntrig: document sysfs interface
From: Aishwarya Pant @ 2018-03-02 5:30 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Jonathan Corbet, Greg KH
Cc: Julia Lawall, linux-doc
Add sysfs documentation for N-Trig touchscreens under Documentation/ABI.
Descriptions have been collected from code comments.
Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
Documentation/ABI/testing/sysfs-driver-hid-ntrig | 70 ++++++++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-ntrig
diff --git a/Documentation/ABI/testing/sysfs-driver-hid-ntrig b/Documentation/ABI/testing/sysfs-driver-hid-ntrig
new file mode 100644
index 000000000000..e574a5625efe
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-ntrig
@@ -0,0 +1,70 @@
+What: /sys/bus/hid/drivers/ntrig/<dev>/activate_slack
+Date: May, 2010
+KernelVersion: 2.6.35
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Number of contact frames ignored before acknowledging the
+ start of activity (activating touch).
+
+
+What: /sys/bus/hid/drivers/ntrig/<dev>/decativate_slack
+Date: May, 2010
+KernelVersion: 2.6.35
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Number of empty (no contact) frames ignored before
+ acknowledging the end of activity (deactivating touch).
+
+ When the last finger is removed from the device, it sends a
+ number of empty frames. By holding off on deactivation for a few
+ frames false erroneous disconnects can be tolerated, where the
+ sensor may mistakenly not detect a finger that is still present.
+
+
+What: /sys/bus/hid/drivers/ntrig/<dev>/activation_width
+What: /sys/bus/hid/drivers/ntrig/<dev>/activation_height
+Date: May, 2010
+KernelVersion: 2.6.35
+Contact: linux-input@vger.kernel.org
+Description:
+ Threholds to override activation slack.
+
+ activation_width: (RW) Width threshold to immediately
+ start processing touch events.
+
+ activation_height: (RW) Height threshold to immediately
+ start processing touch events.
+
+
+What: /sys/bus/hid/drivers/ntrig/<dev>/min_width
+What: /sys/bus/hid/drivers/ntrig/<dev>/min_height
+Date: May, 2010
+KernelVersion: 2.6.35
+Contact: linux-input@vger.kernel.org
+Description:
+ Minimum size contact accepted.
+
+ min_width: (RW) Minimum touch contact width to decide
+ activation and activity.
+
+ min_height: (RW) Minimum touch contact height to decide
+ activation and activity.
+
+
+What: /sys/bus/hid/drivers/ntrig/<dev>/sensor_physical_width
+What: /sys/bus/hid/drivers/ntrig/<dev>/sensor_physical_height
+Date: May, 2010
+KernelVersion: 2.6.35
+Contact: linux-input@vger.kernel.org
+Description:
+ (RO) These are internal ranges not used for normal events but
+ useful for tuning.
+
+
+What: /sys/bus/hid/drivers/ntrig/<dev>/sensor_logical_width
+What: /sys/bus/hid/drivers/ntrig/<dev>/sensor_logical_height
+Date: May, 2010
+KernelVersion: 2.6.35
+Contact: linux-input@vger.kernel.org
+Description:
+ (RO) The range for positions reported during activity.
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
From: JeffyChen @ 2018-03-02 3:57 UTC (permalink / raw)
To: Brian Norris
Cc: linux-kernel, briannorris, dtor, dianders, Enric Balletbo i Serra,
Thomas Gleixner, Joseph Lo, stephen lu, Dmitry Torokhov,
Kate Stewart, linux-input, Greg Kroah-Hartman,
Philippe Ombredanne, Arvind Yadav
In-Reply-To: <20180302023229.GA164361@rodete-desktop-imager.corp.google.com>
Hi Brain,
Thanks for your reply.
On 03/02/2018 10:32 AM, Brian Norris wrote:
>>> > >What about the 'else' case? Shouldn't we try to handle that?
>> >i think the else case is for irq key, which would generate down and up
>> >events in one irq, so it would use the same trigger type for all these 3
>> >cases.
> Not necessarily. It uses whatever trigger was provided in
> platform/DT/etc. data. You could retrieve that with
> irq_get_trigger_type() and try to interpret that. Or you could just
> outlaw using that combination (e.g., in the binding documentation).
i think for the IRQ button case the assert/deassert/any are using the
same irq trigger type, so it should be ok to leave the wakeup trigger
type to be 0(not reconfigure the trigger type)...
i've made a v3 to add a comment about that, but forgot to send it :(
>
> Brian
>
^ permalink raw reply
* [PATCH v3 2/3] Input: gpio-keys - allow setting wakeup event action in DT
From: Jeffy Chen @ 2018-03-02 3:51 UTC (permalink / raw)
To: linux-kernel
Cc: briannorris, heiko, dtor, dianders, Jeffy Chen, devicetree,
Rob Herring, Dmitry Torokhov, linux-input, Mark Rutland
In-Reply-To: <20180302035102.10084-1-jeffy.chen@rock-chips.com>
Allow specifying event actions to trigger wakeup when using the
gpio-keys input device as a wakeup source.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
Changes in v3: None
Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.
Documentation/devicetree/bindings/input/gpio-keys.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
index a94940481e55..996ce84352cb 100644
--- a/Documentation/devicetree/bindings/input/gpio-keys.txt
+++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
@@ -26,6 +26,14 @@ Optional subnode-properties:
If not specified defaults to 5.
- wakeup-source: Boolean, button can wake-up the system.
(Legacy property supported: "gpio-key,wakeup")
+ - wakeup-event-action: Specifies whether the key should wake the
+ system when asserted, when deasserted, or both. This property is
+ only valid for keys that wake up the system (e.g., when the
+ "wakeup-source" property is also provided).
+ Supported values are defined in linux-event-codes.h:
+ EV_ACT_ASSERTED - asserted
+ EV_ACT_DEASSERTED - deasserted
+ EV_ACT_ANY - both asserted and deasserted
- linux,can-disable: Boolean, indicates that button is connected
to dedicated (not shared) interrupt which can be disabled to
suppress events from the button.
--
2.11.0
^ permalink raw reply related
* [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action
From: Jeffy Chen @ 2018-03-02 3:51 UTC (permalink / raw)
To: linux-kernel
Cc: briannorris, heiko, dtor, dianders, Jeffy Chen, Guenter Roeck,
Enric Balletbo i Serra, Thomas Gleixner, Joseph Lo, stephen lu,
Dmitry Torokhov, Kate Stewart, linux-input, Greg Kroah-Hartman,
Philippe Ombredanne, Arvind Yadav
In-Reply-To: <20180302035102.10084-1-jeffy.chen@rock-chips.com>
Add support for specifying event actions to trigger wakeup when using
the gpio-keys input device as a wakeup source.
This would allow the device to configure when to wakeup the system. For
example a gpio-keys input device for pen insert, may only want to wakeup
the system when ejecting the pen.
Suggested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
Changes in v3:
Adding more comments as Brian suggested.
Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.
drivers/input/keyboard/gpio_keys.c | 39 ++++++++++++++++++++++++++++++++++
include/linux/gpio_keys.h | 2 ++
include/uapi/linux/input-event-codes.h | 9 ++++++++
3 files changed, 50 insertions(+)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 87e613dc33b8..607f3960c886 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -45,6 +45,8 @@ struct gpio_button_data {
unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */
unsigned int irq;
+ unsigned int irq_trigger_type;
+ unsigned int wakeup_trigger_type;
spinlock_t lock;
bool disabled;
bool key_pressed;
@@ -540,6 +542,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
}
if (bdata->gpiod) {
+ int active_low = gpiod_is_active_low(bdata->gpiod);
+
if (button->debounce_interval) {
error = gpiod_set_debounce(bdata->gpiod,
button->debounce_interval * 1000);
@@ -568,6 +572,22 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
isr = gpio_keys_gpio_isr;
irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+ switch (button->wakeup_event_action) {
+ case EV_ACT_ASSERTED:
+ bdata->wakeup_trigger_type = active_low ?
+ IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
+ break;
+ case EV_ACT_DEASSERTED:
+ bdata->wakeup_trigger_type = active_low ?
+ IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
+ break;
+ default:
+ /*
+ * For other cases, we are OK letting suspend/resume
+ * not reconfigure the trigger type.
+ */
+ break;
+ }
} else {
if (!button->irq) {
dev_err(dev, "Found button without gpio or irq\n");
@@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
isr = gpio_keys_irq_isr;
irqflags = 0;
+
+ /*
+ * For IRQ buttons, the irq trigger type for press and release
+ * are the same. So we don't need to reconfigure the trigger
+ * type for wakeup.
+ */
}
bdata->code = &ddata->keymap[idx];
@@ -618,6 +644,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
return error;
}
+ bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);
+
return 0;
}
@@ -718,6 +746,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
/* legacy name */
fwnode_property_read_bool(child, "gpio-key,wakeup");
+ fwnode_property_read_u32(child, "wakeup-event-action",
+ &button->wakeup_event_action);
+
button->can_disable =
fwnode_property_read_bool(child, "linux,can-disable");
@@ -854,6 +885,10 @@ static int __maybe_unused gpio_keys_suspend(struct device *dev)
if (device_may_wakeup(dev)) {
for (i = 0; i < ddata->pdata->nbuttons; i++) {
struct gpio_button_data *bdata = &ddata->data[i];
+
+ if (bdata->button->wakeup && bdata->wakeup_trigger_type)
+ irq_set_irq_type(bdata->irq,
+ bdata->wakeup_trigger_type);
if (bdata->button->wakeup)
enable_irq_wake(bdata->irq);
bdata->suspended = true;
@@ -878,6 +913,10 @@ static int __maybe_unused gpio_keys_resume(struct device *dev)
if (device_may_wakeup(dev)) {
for (i = 0; i < ddata->pdata->nbuttons; i++) {
struct gpio_button_data *bdata = &ddata->data[i];
+
+ if (bdata->button->wakeup && bdata->wakeup_trigger_type)
+ irq_set_irq_type(bdata->irq,
+ bdata->irq_trigger_type);
if (bdata->button->wakeup)
disable_irq_wake(bdata->irq);
bdata->suspended = false;
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index d06bf77400f1..7160df54a6fe 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -13,6 +13,7 @@ struct device;
* @desc: label that will be attached to button's gpio
* @type: input event type (%EV_KEY, %EV_SW, %EV_ABS)
* @wakeup: configure the button as a wake-up source
+ * @wakeup_event_action: event action to trigger wakeup
* @debounce_interval: debounce ticks interval in msecs
* @can_disable: %true indicates that userspace is allowed to
* disable button via sysfs
@@ -26,6 +27,7 @@ struct gpio_keys_button {
const char *desc;
unsigned int type;
int wakeup;
+ int wakeup_event_action;
int debounce_interval;
bool can_disable;
int value;
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 53fbae27b280..d7917b0bd438 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -32,6 +32,15 @@
#define INPUT_PROP_CNT (INPUT_PROP_MAX + 1)
/*
+ * Event action types
+ */
+#define EV_ACT_ANY 0x00 /* asserted or deasserted */
+#define EV_ACT_ASSERTED 0x01 /* asserted */
+#define EV_ACT_DEASSERTED 0x02 /* deasserted */
+#define EV_ACT_MAX 0x02
+#define EV_ACT_CNT (EV_ACT_MAX+1)
+
+/*
* Event types
*/
--
2.11.0
^ permalink raw reply related
* [PATCH v3 0/3] gpio-keys: Add support for specifying wakeup event action
From: Jeffy Chen @ 2018-03-02 3:50 UTC (permalink / raw)
To: linux-kernel
Cc: briannorris, heiko, dtor, dianders, Jeffy Chen, Guenter Roeck,
Arnd Bergmann, Joseph Lo, Rob Herring, Catalin Marinas, Mark Yao,
Enric Balletbo i Serra, Brian Norris, Thomas Gleixner,
Philippe Ombredanne, linux-rockchip, Kate Stewart, linux-input,
Will Deacon, Matthias Kaehlcke, devicetree, stephen lu, Greg
On chromebook kevin, we are using gpio-keys for pen insert event. But
we only want it to wakeup the system when ejecting the pen.
So we may need to change the interrupt trigger type during suspending.
Changes in v3:
Adding more comments as Brian suggested.
Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.
Specify wakeup event action instead of irq trigger type as Brian
suggested.
Specify wakeup event action instead of irq trigger type as Brian
suggested.
Jeffy Chen (3):
Input: gpio-keys - add support for wakeup event action
Input: gpio-keys - allow setting wakeup event action in DT
arm64: dts: rockchip: Avoid wakeup when inserting the pen
.../devicetree/bindings/input/gpio-keys.txt | 8 +++++
arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 2 ++
drivers/input/keyboard/gpio_keys.c | 39 ++++++++++++++++++++++
include/linux/gpio_keys.h | 2 ++
include/uapi/linux/input-event-codes.h | 9 +++++
5 files changed, 60 insertions(+)
--
2.11.0
^ permalink raw reply
* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
From: Baolin Wang @ 2018-03-02 3:10 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, gregkh, stephen lu,
Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
Thomas Gleixner, open list:HID CORE LAYER, DTML,
Linux Kernel Mailing List, Mark Brown, Linus Walleij
In-Reply-To: <20180302004404.GA242226@dtor-ws>
On 2 March 2018 at 08:44, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Thu, Mar 01, 2018 at 03:35:23PM +0800, Baolin Wang wrote:
>> On 28 February 2018 at 22:44, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> >> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>> >> be triggered by level type. So this patch introduces one property to
>> >> indicate if the GPIO trigger type is level trigger or edge trigger.
>> >>
>> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> >> ---
>> >> Changes since v2:
>> >> - Use 'interrupt' property to indicate the irq type.
>> >>
>> >> Changes since v1:
>> >> - Diable the GPIO irq until reversing the GPIO level type.
>> >
>> > I've looked at your patch in more detail now, and given it a bit more thought.
>> >
>> > I wonder if you could move that logic into your gpiochip/irqchip driver instead.
>> > It seems that what you do in the gpio-keys driver is to emulate edge triggered
>> > behavior on a level triggered irqchip.
>> >
>> > If you put the same logic into the gpio driver, you could simply make it
>> > pretend to support an edge trigger on both edges and call into the interrupt
>> > handler whenever the state changes.
>> >
>>
>> That is really a good suggestion, which can avoid duplicate level
>> reverse logic in different drivers. So this patch can be simplified
>> just adding one trigger_type to indicate the interrupt type (not
>> always edge trigger). Thanks for your suggestion.
>
> No, there is no need to add trigger type. The gpio-keys driver expects
> trigger with both edges, falling and rising. If your GPIO chip does not
> support it natively, you need to emulate edge trigger via level
> interrupts by reprogramming trigger from active low to active high and
> back on the fly. This should be done in the gpiochip/irqchip driver.
> There is no need to change gpio-keys driver.
Yes, you are right, It can work on my platform after modifying my GPIO
driver like you and Arnd's suggestion. Thanks for all your help.
--
Baolin.wang
Best Regards
^ permalink raw reply
* Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
From: Brian Norris @ 2018-03-02 2:32 UTC (permalink / raw)
To: JeffyChen
Cc: linux-kernel, briannorris, dtor, dianders, Enric Balletbo i Serra,
Thomas Gleixner, Joseph Lo, stephen lu, Dmitry Torokhov,
Kate Stewart, linux-input, Greg Kroah-Hartman,
Philippe Ombredanne, Arvind Yadav
In-Reply-To: <5A8FE726.6080903@rock-chips.com>
Hi,
On Fri, Feb 23, 2018 at 06:04:22PM +0800, Jeffy Chen wrote:
> On 02/13/2018 06:13 AM, Brian Norris wrote:
> > > >
> > > > if (bdata->gpiod) {
> > > >+ int active_low = gpiod_is_active_low(bdata->gpiod);
> > > >+
> > > > if (button->debounce_interval) {
> > > > error = gpiod_set_debounce(bdata->gpiod,
> > > > button->debounce_interval * 1000);
> > > >@@ -568,6 +572,16 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> > > > isr = gpio_keys_gpio_isr;
> > > > irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
> > > >
> > > >+ switch (button->wakeup_event_action) {
> > > >+ case EV_ACT_ASSERTED:
> > > >+ bdata->wakeup_trigger_type = active_low ?
> > > >+ IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
> > > >+ break;
> > > >+ case EV_ACT_DEASSERTED:
> > > >+ bdata->wakeup_trigger_type = active_low ?
> > > >+ IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
> > > >+ break;
> > What about EV_ACT_ANY? And default case? I think for ANY, we're OK
> > letting suspend/resume not reconfigure the trigger type, but maybe a
> > comment here to note that?
> right, will add comment in the next version.
> >
> > > >+ }
> > > > } else {
> > What about the 'else' case? Shouldn't we try to handle that?
> i think the else case is for irq key, which would generate down and up
> events in one irq, so it would use the same trigger type for all these 3
> cases.
Not necessarily. It uses whatever trigger was provided in
platform/DT/etc. data. You could retrieve that with
irq_get_trigger_type() and try to interpret that. Or you could just
outlaw using that combination (e.g., in the binding documentation).
Brian
> i'll add comment in the next version too.
> >
> > Brian
> >
>
>
^ permalink raw reply
* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
From: Dmitry Torokhov @ 2018-03-02 0:44 UTC (permalink / raw)
To: Baolin Wang
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, gregkh, stephen lu,
Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
Thomas Gleixner, open list:HID CORE LAYER, DTML,
Linux Kernel Mailing List, Mark Brown, Linus Walleij
In-Reply-To: <CAMz4kuLNbsZjDPW19V96yL3DzY-bJsjRR=o+w0x4PGyxZFZ0dg@mail.gmail.com>
On Thu, Mar 01, 2018 at 03:35:23PM +0800, Baolin Wang wrote:
> On 28 February 2018 at 22:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
> >> On some platforms (such as Spreadtrum platform), the GPIO keys can only
> >> be triggered by level type. So this patch introduces one property to
> >> indicate if the GPIO trigger type is level trigger or edge trigger.
> >>
> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >> ---
> >> Changes since v2:
> >> - Use 'interrupt' property to indicate the irq type.
> >>
> >> Changes since v1:
> >> - Diable the GPIO irq until reversing the GPIO level type.
> >
> > I've looked at your patch in more detail now, and given it a bit more thought.
> >
> > I wonder if you could move that logic into your gpiochip/irqchip driver instead.
> > It seems that what you do in the gpio-keys driver is to emulate edge triggered
> > behavior on a level triggered irqchip.
> >
> > If you put the same logic into the gpio driver, you could simply make it
> > pretend to support an edge trigger on both edges and call into the interrupt
> > handler whenever the state changes.
> >
>
> That is really a good suggestion, which can avoid duplicate level
> reverse logic in different drivers. So this patch can be simplified
> just adding one trigger_type to indicate the interrupt type (not
> always edge trigger). Thanks for your suggestion.
No, there is no need to add trigger type. The gpio-keys driver expects
trigger with both edges, falling and rising. If your GPIO chip does not
support it natively, you need to emulate edge trigger via level
interrupts by reprogramming trigger from active low to active high and
back on the fly. This should be done in the gpiochip/irqchip driver.
There is no need to change gpio-keys driver.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/3] Input: gpio-keys - allow setting wakeup event action in DT
From: Rob Herring @ 2018-03-01 21:26 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, briannorris, dtor, dianders, devicetree,
Dmitry Torokhov, linux-input, Mark Rutland
In-Reply-To: <20180210110907.5504-3-jeffy.chen@rock-chips.com>
On Sat, Feb 10, 2018 at 07:09:06PM +0800, Jeffy Chen wrote:
> Allow specifying event actions to trigger wakeup when using the
> gpio-keys input device as a wakeup source.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v2:
> Specify wakeup event action instead of irq trigger type as Brian
> suggested.
>
> Documentation/devicetree/bindings/input/gpio-keys.txt | 8 ++++++++
> 1 file changed, 8 insertions(+)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller
From: Andy Shevchenko @ 2018-03-01 19:57 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <20180301191348.GA9281@casa>
On Thu, Mar 1, 2018 at 9:13 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
>> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
>> <rodrigorivascosta@gmail.com> wrote:
>> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>> >> <rodrigorivascosta@gmail.com> wrote:
> My bad, I didn't mean a literal free() call. More like:
>
> static void steam_unregister(struct steam_device *steam)
> {
> struct input_dev *input;
> struct power_supply *battery;
>
> rcu_read_lock();
> input = rcu_dereference(steam->input);
> battery = rcu_dereference(steam->battery);
> input_gyro = rcu_dereference(steam->input_gyro);
> rcu_read_unlock();
>
> if (input) {
> RCU_INIT_POINTER(steam->input, NULL);
> synchronize_rcu();
> hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> steam->serial_no);
> input_unregister_device(input);
Candidate for a helper
static void steam_unregister_input(...)
{
...
}
steam_unregister_input(input);
> }
> if (battery) {
> RCU_INIT_POINTER(steam->battery, NULL);
> synchronize_rcu();
> power_supply_unregister(battery);
> }
Ditto.
> if (input_gyro) {
> RCU_INIT_POINTER(steam->input_gyro, NULL);
> synchronize_rcu();
> input_unregister_device(input_gyro);
> }
Ditto.
> }
>
> Also I think input_unregister_device() does not admit a NULL pointer.
> Having a special `if (!input_gyro)return;` at the end looks just
> wrong to me, it is no different from the other ones.
See above. Hide that detail in a helper function.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller
From: Rodrigo Rivas Costa @ 2018-03-01 19:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <CAHp75Vd9wgX8S=4hGFERN5bEN9q4=DxYcDea9Z=mvSRi2GcxeQ@mail.gmail.com>
On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> >> <rodrigorivascosta@gmail.com> wrote:
>
> >> > + if (input) {
> >>
> >> if (!input)
> >> return;
> >>
> >> ?
> > That was because of symmetry, because further patches add more objects.
> > Then
> > if (input)
> > free(input);
> > if (battery)
> > free(battery);
> > /* in the future *(
> > if (input_gyro)
> > free(input_gyro);
> >
> > Sure, the last one can do an early return, but you break symmetry.
>
> The generic APIs when designed properly are NULL aware at freeing resources.
>
> So, it should look like
>
> free(input);
> free(battery);
> free(input_gyro);
My bad, I didn't mean a literal free() call. More like:
static void steam_unregister(struct steam_device *steam)
{
struct input_dev *input;
struct power_supply *battery;
rcu_read_lock();
input = rcu_dereference(steam->input);
battery = rcu_dereference(steam->battery);
input_gyro = rcu_dereference(steam->input_gyro);
rcu_read_unlock();
if (input) {
RCU_INIT_POINTER(steam->input, NULL);
synchronize_rcu();
hid_info(steam->hdev, "Steam Controller '%s' disconnected",
steam->serial_no);
input_unregister_device(input);
}
if (battery) {
RCU_INIT_POINTER(steam->battery, NULL);
synchronize_rcu();
power_supply_unregister(battery);
}
if (input_gyro) {
RCU_INIT_POINTER(steam->input_gyro, NULL);
synchronize_rcu();
input_unregister_device(input_gyro);
}
}
Also I think input_unregister_device() does not admit a NULL pointer.
Having a special `if (!input_gyro)return;` at the end looks just
wrong to me, it is no different from the other ones.
>
> >> > + if (steam) {
> >> > + cancel_work_sync(&steam->work_connect);
> >> > + steam_unregister(steam);
> >>
> >> > + hid_set_drvdata(hdev, NULL);
> >>
> >> Hmm.. Doesn't HID do this?
> >
> > Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> > that on hid-core.c or hid-generic.c. And a call like this is done in
> > many modules... so I did the same, just to be sure.
>
> What you are looking for is, AFAIU:
>
> drivers/base/dd.c:902: dev_set_drvdata(dev, NULL);
Ah, yes. I usually feel more comfortable setting the pointer to NULL
just after freeing the object. But currently I'm not freeing the steam
anymore, it is devm'ed, and the actual free happens from dd.c:900.
You convinced me, I'll remove that line.
>
> >>
> >> > + }
> >>
> >> if (steam) {
> >> ...
> >> hid_hw_stop(hdev);
> >> ...
> >> } else {
> >> hid_hw_stop(hdev);
> >> }
> >>
> >> ?
> >
> > I have no real preference. Your version has two 'if', mine has two 'if'.
> > What about:
> >
> > static void steam_remove(struct hid_device *hdev)
> > {
> > struct steam_device *steam = hid_get_drvdata(hdev);
> >
> > if (!steam) {
> > hid_hw_stop(hdev);
> > return;
> > }
> >
> > if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > hid_info(hdev, "Steam wireless receiver disconnected");
> > hid_hw_close(hdev);
> > }
> > hid_hw_stop(hdev);
> > cancel_work_sync(&steam->work_connect);
> > steam_unregister(steam);
>
> > hid_set_drvdata(hdev, NULL);
>
> w/o this one.
>
> > }
>
> For me my version looks more compact to read, but at the end it's up to you.
> Another option split if (steam) variant into helper, and thus
>
> if (steam)
> steam_non_null_device_remove();
> else
> hid_hw_stop();
>
> But again, up to you (that's why question mark in the first place above).
Well, I don't like code such as:
if (cond) {
/* a lot of code */
} else {
/* one line */
}
because at the time I get to the else I don't remember what the
condition was about.
Regards.
Rodrigo
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox