* Re: Reported key not released
From: Pali Rohár @ 2014-11-24 20:58 UTC (permalink / raw)
To: Matthew Garrett
Cc: linux-input, platform-driver-x86, Darren Hart, Gabriele Mazzotta,
Alex Hung
In-Reply-To: <20141124200433.GA3459@srcf.ucam.org>
[-- Attachment #1: Type: Text/Plain, Size: 590 bytes --]
On Monday 24 November 2014 21:04:33 Matthew Garrett wrote:
> On Sun, Nov 23, 2014 at 02:13:30PM +0100, Pali Rohár wrote:
> > When I enable or disable this switch it correctly enable or
> > disable devices (and also hard rfkill status is set
> > correctly).
>
> If the switch changes the rfkill state, you shouldn't be
> emitting any presses - the rfkill state change will notify
> userspace. Consume the event in the filter and don't pass it
> through.
So kernel should filter i8042 atk key press events which
represent HW switch?
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: Reported key not released
From: Matthew Garrett @ 2014-11-24 21:51 UTC (permalink / raw)
To: Pali Rohár
Cc: linux-input, platform-driver-x86, Darren Hart, Gabriele Mazzotta,
Alex Hung
In-Reply-To: <201411242158.33119@pali>
On Mon, Nov 24, 2014 at 09:58:32PM +0100, Pali Rohár wrote:
> So kernel should filter i8042 atk key press events which
> represent HW switch?
Yeah - the firmware is sending an "Update rfkill state" message, not a
real keypress. If you're already listening for those events through a
filter then you should just drop them once you've done the state update.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: Reported key not released
From: Pali Rohár @ 2014-11-24 22:04 UTC (permalink / raw)
To: Matthew Garrett
Cc: linux-input, platform-driver-x86, Darren Hart, Gabriele Mazzotta,
Alex Hung
In-Reply-To: <20141124215147.GB14361@srcf.ucam.org>
[-- Attachment #1: Type: Text/Plain, Size: 836 bytes --]
On Monday 24 November 2014 22:51:47 Matthew Garrett wrote:
> On Mon, Nov 24, 2014 at 09:58:32PM +0100, Pali Rohár wrote:
> > So kernel should filter i8042 atk key press events which
> > represent HW switch?
>
> Yeah - the firmware is sending an "Update rfkill state"
> message, not a real keypress. If you're already listening for
> those events through a filter then you should just drop them
> once you've done the state update.
Now I wanted to do not use i8042 hook function in dell-laptop.c
which was used only for receiving key event (hw switch) and
propagating it to dell-laptop rfkill device. See my patch:
dell-laptop: Use dell-rbtn instead i8042 filter when possible
But if we really should drop these atk key press, then another
patch for it will be needed.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH 0/3] HID: wacom: Fix generic multitouch device handling
From: Jason Gerecke @ 2014-11-24 23:32 UTC (permalink / raw)
To: jkosina; +Cc: linux-input, benjamin.tissoires, pinglinux
This set of patches addresses the problems that I had getting Benjamin's
"HID generic handling" patches. Each of the patches addresses a seprate
issue I encountered:
* Patch 1/3: The devices that I'm using have some usages outside of
a logical or physical collection container which prevents the
reports from being sent. I'm not sure if keying on the application
collection is ideal, but it does do the job.
* Patch 2/3: The devices that I'm using put their X and Y usages after
the finger count, which prevents the (compatibility) ABS_X and
ABS_Y axes from being initialized. This patch just guarantees that
they'll be initialized.
* Patch 3/3: Benjamin's original code seemed to only report data about
the last finger in a multitouch report since information about "earlier"
fingers would be overwritten. This patch changes the logic so that
we immediately report field values whenever we can (there are some
cases that we can't -- the tip/range bits appear before the contact
identifier, so we have to store their state for a short time).
^ permalink raw reply
* [PATCH 1/3] HID: wacom: Consult the application usage when determining field type
From: Jason Gerecke @ 2014-11-24 23:32 UTC (permalink / raw)
To: jkosina; +Cc: linux-input, benjamin.tissoires, pinglinux, Jason Gerecke
In-Reply-To: <1416871934-14133-1-git-send-email-killertofu@gmail.com>
It is not necessarily sufficient to look only at the physical and logical
usages when determining if a field is for the pen or touch. Some fields
are not contained in a sub-collection and thus only have an application
usage. Not checking the application usage in such cases causes us to
ignore the field entirely, which may lead to incorrect behavior.
Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
drivers/hid/wacom_wac.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 9565d31..1468f00 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1484,9 +1484,11 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
}
#define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
- ((f)->physical == HID_DG_STYLUS))
+ ((f)->physical == HID_DG_STYLUS) || \
+ ((f)->application == HID_DG_PEN))
#define WACOM_FINGER_FIELD(f) (((f)->logical == HID_DG_FINGER) || \
- ((f)->physical == HID_DG_FINGER))
+ ((f)->physical == HID_DG_FINGER) || \
+ ((f)->application == HID_DG_TOUCHSCREEN))
void wacom_wac_usage_mapping(struct hid_device *hdev,
struct hid_field *field, struct hid_usage *usage)
--
2.1.3
^ permalink raw reply related
* [PATCH 2/3] HID: wacom: Manually declare ABS_{X,Y} for pointer emulation
From: Jason Gerecke @ 2014-11-24 23:32 UTC (permalink / raw)
To: jkosina; +Cc: linux-input, benjamin.tissoires, pinglinux, Jason Gerecke
In-Reply-To: <1416871934-14133-1-git-send-email-killertofu@gmail.com>
If a HID descriptor places HID_DG_CONTACTID before HID_DG_X and HID_DG_Y then
the ABS_X and ABS_Y will not be automatically initialized by the call to
input_mt_init_slots. Here we move the setup of those axes outside of the
'if' statement so that they're always present if the associated HID field
is.
Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
drivers/hid/wacom_wac.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 1468f00..a55e0ed 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1382,16 +1382,14 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
switch (usage->hid) {
case HID_GD_X:
- if (touch_max == 1)
- wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
- else
+ wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
+ if (touch_max > 1)
wacom_map_usage(wacom, usage, field, EV_ABS,
ABS_MT_POSITION_X, 4);
break;
case HID_GD_Y:
- if (touch_max == 1)
- wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
- else
+ wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
+ if (touch_max > 1)
wacom_map_usage(wacom, usage, field, EV_ABS,
ABS_MT_POSITION_Y, 4);
break;
--
2.1.3
^ permalink raw reply related
* [PATCH 3/3] HID: wacom: Report input events immediately upon receipt
From: Jason Gerecke @ 2014-11-24 23:32 UTC (permalink / raw)
To: jkosina; +Cc: linux-input, benjamin.tissoires, pinglinux, Jason Gerecke
In-Reply-To: <1416871934-14133-1-git-send-email-killertofu@gmail.com>
Multitouch tablets cannot work properly if wacom_wac_finger_event simply
stores the event details, since details about former fingers will be
overwritten by later ones (causing wacom_wac_finger_report to effectively
only report the state of the *last* finger in the packet).
This patch modifies the logic so that events are generated as soon as
possible in response to events. We do temporarily store HID_DG_TIPSWITCH
value since the value of HID_DG_CONTACTID is (at least in for the tablets
I've tested) not known until shortly afterwards.
Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
drivers/hid/wacom_wac.c | 77 ++++++++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 33 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index a55e0ed..3eaa98a 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1412,19 +1412,40 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
{
struct wacom *wacom = hid_get_drvdata(hdev);
struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+ struct hid_data *data = &wacom_wac->hid_data;
+ unsigned mt = wacom_wac->features.touch_max > 1;
+ struct input_dev *input = wacom_wac->input;
+ bool prox = data->tipswitch && !wacom_wac->shared->stylus_in_proximity;
+ int slot;
switch (usage->hid) {
case HID_GD_X:
- wacom_wac->hid_data.x = value;
+ if (prox) {
+ input_report_abs(input,
+ mt ? ABS_MT_POSITION_X : ABS_X,
+ value);
+ }
break;
case HID_GD_Y:
- wacom_wac->hid_data.y = value;
+ if (prox) {
+ input_report_abs(input,
+ mt ? ABS_MT_POSITION_Y : ABS_Y,
+ value);
+ }
break;
case HID_DG_CONTACTID:
- wacom_wac->hid_data.id = value;
+ slot = input_mt_get_slot_by_key(input, value);
+
+ input_mt_slot(input, slot);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
break;
case HID_DG_TIPSWITCH:
- wacom_wac->hid_data.tipswitch = value;
+ data->tipswitch = value;
+ if (!mt) {
+ prox = data->tipswitch &&
+ !wacom_wac->shared->stylus_in_proximity;
+ input_report_key(input, BTN_TOUCH, prox);
+ }
break;
}
@@ -1432,33 +1453,26 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
return 0;
}
-static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
- struct input_dev *input, bool touch)
+static int wacom_wac_finger_touches(struct hid_device *hdev)
{
- int slot;
- struct hid_data *hid_data = &wacom_wac->hid_data;
-
- slot = input_mt_get_slot_by_key(input, hid_data->id);
-
- input_mt_slot(input, slot);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
- input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
- input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
- }
- input_mt_sync_frame(input);
-}
+ struct wacom *wacom = hid_get_drvdata(hdev);
+ struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+ struct input_dev *input = wacom_wac->input;
+ unsigned touch_max = wacom_wac->features.touch_max;
+ int count = 0;
+ int i;
-static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
- struct input_dev *input, bool touch)
-{
- struct hid_data *hid_data = &wacom_wac->hid_data;
+ if (touch_max == 1)
+ return (wacom_wac->hid_data.tipswitch &&
+ !wacom_wac->shared->stylus_in_proximity);
- if (touch) {
- input_report_abs(input, ABS_X, hid_data->x);
- input_report_abs(input, ABS_Y, hid_data->y);
+ for (i = 0; i < input->mt->num_slots; i++) {
+ struct input_mt_slot *ps = &input->mt->slots[i];
+ int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
+ if (id >= 0)
+ count++;
}
- input_report_key(input, BTN_TOUCH, touch);
+ return count;
}
static void wacom_wac_finger_report(struct hid_device *hdev,
@@ -1467,18 +1481,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
struct wacom *wacom = hid_get_drvdata(hdev);
struct wacom_wac *wacom_wac = &wacom->wacom_wac;
struct input_dev *input = wacom_wac->input;
- bool touch = wacom_wac->hid_data.tipswitch &&
- !wacom_wac->shared->stylus_in_proximity;
unsigned touch_max = wacom_wac->features.touch_max;
if (touch_max > 1)
- wacom_wac_finger_mt_report(wacom_wac, input, touch);
- else
- wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
+ input_mt_sync_frame(input);
+
input_sync(input);
/* keep touch state for pen event */
- wacom_wac->shared->touch_down = touch;
+ wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
}
#define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
--
2.1.3
^ permalink raw reply related
* [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver
From: Andrew Duggan @ 2014-11-24 23:37 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Andrew Duggan, Jiri Kosina, Benjamin Tissoires
On composite HID devices there may be multiple HID devices on separate interfaces, but hid-rmi
should only bind to the touchpad. The previous version simply checked that the interface protocol
was set to mouse. Unfortuately, it is not always the case that the touchpad has the mouse interface
protocol set. This patch takes a different approach and scans the report descriptor looking for
the Generic Desktop Pointer usage and the Vendor Specific Top Level Collection needed by the
hid-rmi driver to interface with the device.
Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
This is a second attempt at the patch I submitted back in October. Instead of looking for specific
HID reports this patch looks for the Generic Desktop Pointer usage. Having that usage along with
the vendor specific collection seems to be a distinctive enough to id the Synaptics touchpad in
the composite USB device.
drivers/hid/hid-core.c | 21 ++++++++++++++++-----
include/linux/hid.h | 4 +++-
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 12b6e67..ba9dc59 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -698,10 +698,20 @@ static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
static void hid_scan_collection(struct hid_parser *parser, unsigned type)
{
struct hid_device *hid = parser->device;
+ int i;
if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
type == HID_COLLECTION_PHYSICAL)
hid->group = HID_GROUP_SENSOR_HUB;
+
+ if ((parser->global.usage_page << 16) == HID_UP_GENDESK)
+ for (i = 0; i < parser->local.usage_index; i++)
+ if (parser->local.usage[i] == HID_GD_POINTER)
+ parser->scan_flags
+ |= HID_SCAN_FLAG_GENDESK_POINTER;
+
+ if ((parser->global.usage_page << 16) == HID_UP_MSVENDOR)
+ parser->scan_flags |= HID_SCAN_FLAG_VENDOR_SPECIFIC;
}
static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
@@ -783,11 +793,12 @@ static int hid_scan_report(struct hid_device *hid)
* Vendor specific handlings
*/
if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
- (hid->group == HID_GROUP_GENERIC) &&
- /* only bind to the mouse interface of composite USB devices */
- (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
- /* hid-rmi should take care of them, not hid-generic */
- hid->group = HID_GROUP_RMI;
+ (hid->group == HID_GROUP_GENERIC)) {
+ if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
+ && (parser->scan_flags & HID_SCAN_FLAG_GENDESK_POINTER))
+ /* hid-rmi should take care of them, not hid-generic */
+ hid->group = HID_GROUP_RMI;
+ }
/*
* Vendor specific handlings
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f53c4a9..b019f15 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -547,7 +547,9 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
#define HID_GLOBAL_STACK_SIZE 4
#define HID_COLLECTION_STACK_SIZE 4
-#define HID_SCAN_FLAG_MT_WIN_8 0x00000001
+#define HID_SCAN_FLAG_MT_WIN_8 BIT(0)
+#define HID_SCAN_FLAG_VENDOR_SPECIFIC BIT(1)
+#define HID_SCAN_FLAG_GENDESK_POINTER BIT(2)
struct hid_parser {
struct hid_global global;
--
2.1.0
^ permalink raw reply related
* [PATCH] HID: rmi: Add support for the touchpad in the Razer Blade 14 laptop
From: Andrew Duggan @ 2014-11-25 0:20 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Andrew Duggan, Jiri Kosina, Benjamin Tissoires
The Razer Blade 14 has a Synaptic's TouchPad on one of the interfaces of
a composite USB device. This patch allows the hid-rmi driver to bind
to that interface. It also adds support for the external click buttons
on the Razer's touchpad. External buttons are reported using generic
mouse reports instead of through the F30 like it is on ClickPads.
Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
This patch depends on the "HID: rmi: Scan the report descriptor to determine if the device is
suitable for the hid-rmi driver" I submitted earlier today to correctly bind to the touchpad HID
device in the composite USB device.
drivers/hid/hid-core.c | 4 +++-
drivers/hid/hid-ids.h | 3 +++
drivers/hid/hid-rmi.c | 15 ++++++++++++++-
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index ba9dc59..d69ea16 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -792,7 +792,9 @@ static int hid_scan_report(struct hid_device *hid)
/*
* Vendor specific handlings
*/
- if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
+ if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS
+ || (hid->vendor == USB_VENDOR_ID_RAZER
+ && hid->product == USB_DEVICE_ID_RAZER_BLADE_14)) &&
(hid->group == HID_GROUP_GENERIC)) {
if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
&& (parser->scan_flags & HID_SCAN_FLAG_GENDESK_POINTER))
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 25cd674..c677aad 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -751,6 +751,9 @@
#define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001 0x3001
#define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
+#define USB_VENDOR_ID_RAZER 0x1532
+#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
+
#define USB_VENDOR_ID_REALTEK 0x0bda
#define USB_DEVICE_ID_REALTEK_READER 0x0152
diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 3cccff7..1f131df 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -453,7 +453,15 @@ static int rmi_raw_event(struct hid_device *hdev,
case RMI_ATTN_REPORT_ID:
return rmi_input_event(hdev, data, size);
case RMI_MOUSE_REPORT_ID:
- rmi_schedule_reset(hdev);
+ /*
+ * touchpads with physical mouse buttons will report those
+ * buttons in mouse reports even in RMI mode. Only reset
+ * the device if we see reports which contain X or Y data.
+ */
+ if (data[2] != 0 || data[3] != 0)
+ rmi_schedule_reset(hdev);
+ else
+ return 1;
break;
}
@@ -871,6 +879,11 @@ static int rmi_input_mapping(struct hid_device *hdev,
struct hid_input *hi, struct hid_field *field,
struct hid_usage *usage, unsigned long **bit, int *max)
{
+ if (field->application == HID_GD_POINTER
+ && (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
+ /* Pass mouse button reports to generic code for processing */
+ return 0;
+
/* we want to make HID ignore the advertised HID collection */
return -1;
}
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup
From: Benson Leung @ 2014-11-25 0:56 UTC (permalink / raw)
To: Oliver Neukum
Cc: johan-DgEjT+Ai2ygdnm+yROfE0A, Jiri Kosina,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Sameer Nanda
In-Reply-To: <1416820395.19925.4.camel-AfvqVibwNMkMNNZnWhT/Jw@public.gmane.org>
Hi Oliver,
On Mon, Nov 24, 2014 at 1:13 AM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
> But there is very little to be gained by switching off remote wakeup.
> The additional energy consumption devices with remote wakeup enabled
> will be dwarfed by the energy needed for an additional wakeup.
>
That makes sense to me. Does this mean we should be moving toward a
solution that doesn't wake suspended devices on close for other usb
devices, not just hid?
This particular pattern of get()/needs_remote_wakeup=0/put() on
close() appears in several other drivers, for example :
62ecae0 Input: wacom - properly enable runtime PM
5d9efc5 Input: usbtouchscreen - implement runtime power management
--
Benson Leung
Software Engineer, Chrom* OS
bleung-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] ARM: dts: Add regulator-haptic device node for exynos3250-rinato
From: Chanwoo Choi @ 2014-11-25 1:02 UTC (permalink / raw)
To: Jaewon Kim
Cc: Kukjin Kim, Dmitry Torokhov, Pankaj Dubey, Dan Murphy,
Hyunhee Kim, linux-kernel, linux-input, linux-samsung-soc
In-Reply-To: <1416840651-17141-3-git-send-email-jaewon02.kim@samsung.com>
On 11/24/2014 11:50 PM, Jaewon Kim wrote:
> This patch adds regulator-haptic device node controlled by regulator.
>
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
> arch/arm/boot/dts/exynos3250-rinato.dts | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos3250-rinato.dts b/arch/arm/boot/dts/exynos3250-rinato.dts
> index 84380fa..da03005 100644
> --- a/arch/arm/boot/dts/exynos3250-rinato.dts
> +++ b/arch/arm/boot/dts/exynos3250-rinato.dts
> @@ -104,6 +104,13 @@
> };
> };
> };
> +
> + haptics {
> + compatible = "regulator-haptic";
> + haptic-supply = <&motor_reg>;
> + min-microvolt = <1100000>;
> + max-microvolt = <2700000>;
> + };
> };
>
> &adc {
>
Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Thanks,
Chanwoo Choi
^ permalink raw reply
* Re: [PATCH v2 1/2] Input: add regulator haptic driver
From: Chanwoo Choi @ 2014-11-25 1:04 UTC (permalink / raw)
To: Jaewon Kim
Cc: Kukjin Kim, Dmitry Torokhov, Pankaj Dubey, Dan Murphy,
Hyunhee Kim, linux-kernel, linux-input, linux-samsung-soc
In-Reply-To: <1416840651-17141-2-git-send-email-jaewon02.kim@samsung.com>
On 11/24/2014 11:50 PM, Jaewon Kim wrote:
> This patch adds support for haptic driver controlled by
> voltage of regulator. And this driver support for
> Force Feedback interface from input framework
>
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> Signed-off-by: Hyunhee Kim <hyunhee.kim@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Looks good to me.
I tested this patch on Exynos3250-based Rinato (Samsung Gear2) board.
Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Thanks,
Chanwoo Choi
> ---
> .../devicetree/bindings/input/regulator-haptic.txt | 24 ++
> drivers/input/misc/Kconfig | 11 +
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/regulator-haptic.c | 247 ++++++++++++++++++++
> include/linux/input/regulator-haptic.h | 30 +++
> 5 files changed, 313 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/regulator-haptic.txt
> create mode 100644 drivers/input/misc/regulator-haptic.c
> create mode 100644 include/linux/input/regulator-haptic.h
>
> diff --git a/Documentation/devicetree/bindings/input/regulator-haptic.txt b/Documentation/devicetree/bindings/input/regulator-haptic.txt
> new file mode 100644
> index 0000000..5a44e8f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/regulator-haptic.txt
> @@ -0,0 +1,24 @@
> +* Requlator Haptic Device Tree Bindings
> +
> +The regulator haptic driver controlled by voltage of regulator.
> +This driver implemented via Force Feedback interface.
> +
> +Required Properties:
> + - compatible : Should be "regulator-haptic"
> + - haptic-supply : Power supply to the haptic motor.
> + [*] refer Documentation/devicetree/bindings/regulator/regulator.txt
> +
> + - max-microvolt : The maximum voltage value supplied to the haptic motor.
> + [The unit of the voltage is a micro]
> +
> + - min-microvolt : The minimum voltage value supplied to the haptic motor.
> + [The unit of the voltage is a micro]
> +
> +Example:
> +
> + haptics {
> + compatible = "regulator-haptic";
> + haptic-supply = <&motor_regulator>;
> + max-microvolt = <2700000>;
> + min-microvolt = <1100000>;
> + };
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 23297ab..e5e556d 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -394,6 +394,17 @@ config INPUT_CM109
> To compile this driver as a module, choose M here: the module will be
> called cm109.
>
> +config INPUT_REGULATOR_HAPTIC
> + tristate "regulator haptics support"
> + select INPUT_FF_MEMLESS
> + help
> + This option enables device driver support for the haptic controlled
> + by regulator. This driver supports ff-memless interface
> + from input framework.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called regulator-haptic.
> +
> config INPUT_RETU_PWRBUTTON
> tristate "Retu Power button Driver"
> depends on MFD_RETU
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 19c7603..1f135af 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY) += pmic8xxx-pwrkey.o
> obj-$(CONFIG_INPUT_POWERMATE) += powermate.o
> obj-$(CONFIG_INPUT_PWM_BEEPER) += pwm-beeper.o
> obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
> +obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
> obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
> obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
> obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o
> diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
> new file mode 100644
> index 0000000..c61dd99
> --- /dev/null
> +++ b/drivers/input/misc/regulator-haptic.c
> @@ -0,0 +1,247 @@
> +/*
> + * Regulator haptic driver
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Author: Jaewon Kim <jaewon02.kim@samsung.com>
> + * Author: Hyunhee Kim <hyunhee.kim@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/input.h>
> +#include <linux/input/regulator-haptic.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#define MAX_MAGNITUDE_SHIFT 16
> +
> +struct regulator_haptic {
> + struct device *dev;
> + struct input_dev *input_dev;
> + struct regulator *regulator;
> + struct work_struct work;
> +
> + bool enabled;
> + bool suspend_state;
> + unsigned int max_volt;
> + unsigned int min_volt;
> + unsigned int intensity;
> + unsigned int magnitude;
> +};
> +
> +static void regulator_haptic_enable(struct regulator_haptic *haptic, bool state)
> +{
> + int error;
> +
> + if (haptic->enabled == state)
> + return;
> +
> + if (state)
> + error = regulator_enable(haptic->regulator);
> + else
> + error = regulator_disable(haptic->regulator);
> + if (error) {
> + dev_err(haptic->dev, "cannot enable regulator\n");
> + return;
> + }
> +
> + haptic->enabled = state;
> +}
> +
> +static void regulator_haptic_work(struct work_struct *work)
> +{
> + struct regulator_haptic *haptic = container_of(work,
> + struct regulator_haptic, work);
> + int error;
> +
> + error = regulator_set_voltage(haptic->regulator,
> + haptic->intensity + haptic->min_volt, haptic->max_volt);
> + if (error) {
> + dev_err(haptic->dev, "cannot set regulator voltage\n");
> + return;
> + }
> +
> + if (haptic->magnitude)
> + regulator_haptic_enable(haptic, true);
> + else
> + regulator_haptic_enable(haptic, false);
> +}
> +
> +static int regulator_haptic_play_effect(struct input_dev *input, void *data,
> + struct ff_effect *effect)
> +{
> + struct regulator_haptic *haptic = input_get_drvdata(input);
> + u64 volt_mag_multi;
> +
> + haptic->magnitude = effect->u.rumble.strong_magnitude;
> + if (!haptic->magnitude)
> + haptic->magnitude = effect->u.rumble.weak_magnitude;
> +
> +
> + volt_mag_multi = (u64)(haptic->max_volt - haptic->min_volt) *
> + haptic->magnitude;
> + haptic->intensity = (unsigned int)(volt_mag_multi >>
> + MAX_MAGNITUDE_SHIFT);
> +
> + schedule_work(&haptic->work);
> +
> + return 0;
> +}
> +
> +static void regulator_haptic_close(struct input_dev *input)
> +{
> + struct regulator_haptic *haptic = input_get_drvdata(input);
> +
> + cancel_work_sync(&haptic->work);
> + regulator_haptic_enable(haptic, false);
> +}
> +
> +#ifdef CONFIG_OF
> +static int regulator_haptic_parse_dt(struct regulator_haptic *haptic)
> +{
> + struct device_node *node = haptic->dev->of_node;
> + int error;
> +
> + error = of_property_read_u32(node, "max-microvolt", &haptic->max_volt);
> + if (error) {
> + dev_err(haptic->dev, "cannot parse max-microvolt\n");
> + return error;
> + }
> +
> + error = of_property_read_u32(node, "min-microvolt", &haptic->min_volt);
> + if (error) {
> + dev_err(haptic->dev, "cannot parse min-microvolt\n");
> + return error;
> + }
> +
> + return 0;
> +}
> +#else
> +static int regulator_haptic_parse_dt(struct regulator_haptic *haptic)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_OF */
> +
> +static int regulator_haptic_probe(struct platform_device *pdev)
> +{
> + struct regulator_haptic *haptic;
> + struct regulator_haptic_data *data;
> + struct input_dev *input_dev;
> + int error;
> +
> + haptic = devm_kzalloc(&pdev->dev, sizeof(*haptic), GFP_KERNEL);
> + if (!haptic)
> + return -ENOMEM;
> +
> + haptic->dev = &pdev->dev;
> + haptic->enabled = false;
> + haptic->suspend_state = false;
> + INIT_WORK(&haptic->work, regulator_haptic_work);
> +
> + if (pdev->dev.of_node) {
> + error = regulator_haptic_parse_dt(haptic);
> + if (error) {
> + dev_err(&pdev->dev, "failed to parse device tree\n");
> + return error;
> + }
> + } else {
> + data = dev_get_platdata(&pdev->dev);
> + if (data) {
> + dev_err(&pdev->dev, "failed to get platdata\n");
> + return -EINVAL;
> + }
> +
> + haptic->regulator = data->regulator;
> + haptic->max_volt = data->max_volt;
> + haptic->min_volt = data->min_volt;
> + }
> +
> + haptic->regulator = devm_regulator_get(&pdev->dev, "haptic");
> + if (IS_ERR(haptic->regulator)) {
> + dev_err(&pdev->dev, "failed to get regulator\n");
> + return PTR_ERR(haptic->regulator);
> + }
> +
> + input_dev = devm_input_allocate_device(&pdev->dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + haptic->input_dev = input_dev;
> + haptic->input_dev->name = "regulator-haptic";
> + haptic->input_dev->dev.parent = &pdev->dev;
> + haptic->input_dev->close = regulator_haptic_close;
> + input_set_drvdata(haptic->input_dev, haptic);
> + input_set_capability(haptic->input_dev, EV_FF, FF_RUMBLE);
> +
> + error = input_ff_create_memless(input_dev, NULL,
> + regulator_haptic_play_effect);
> + if (error) {
> + dev_err(&pdev->dev, "failed to create force-feedback\n");
> + return error;
> + }
> +
> + error = input_register_device(haptic->input_dev);
> + if (error) {
> + dev_err(&pdev->dev, "failed to register input device\n");
> + return error;
> + }
> +
> + platform_set_drvdata(pdev, haptic);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused regulator_haptic_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
> +
> + if (haptic->enabled) {
> + regulator_haptic_enable(haptic, false);
> + haptic->suspend_state = true;
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused regulator_haptic_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
> +
> + if (haptic->suspend_state) {
> + regulator_haptic_enable(haptic, true);
> + haptic->suspend_state = false;
> + }
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(regulator_haptic_pm_ops,
> + regulator_haptic_suspend, regulator_haptic_resume);
> +
> +static struct of_device_id regulator_haptic_dt_match[] = {
> + { .compatible = "regulator-haptic" },
> + {},
> +};
> +
> +static struct platform_driver regulator_haptic_driver = {
> + .probe = regulator_haptic_probe,
> + .driver = {
> + .name = "regulator-haptic",
> + .of_match_table = regulator_haptic_dt_match,
> + .pm = ®ulator_haptic_pm_ops,
> + },
> +};
> +module_platform_driver(regulator_haptic_driver);
> +
> +MODULE_AUTHOR("Jaewon Kim <jaewon02.kim@samsung.com>");
> +MODULE_AUTHOR("Hyunhee Kim <hyunhee.kim@samsung.com>");
> +MODULE_DESCRIPTION("Regulator haptic driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/regulator-haptic.h b/include/linux/input/regulator-haptic.h
> new file mode 100644
> index 0000000..15a629c
> --- /dev/null
> +++ b/include/linux/input/regulator-haptic.h
> @@ -0,0 +1,30 @@
> +/*
> + * Regulator Haptic Platform Data
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Author: Jaewon Kim <jaewon02.kim@samsung.com>
> + * Author: Hyunhee Kim <hyunhee.kim@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _REGULATOR_HAPTIC_H
> +
> +/*
> + * struct regulator_haptic_data - Platform device data
> + *
> + * @regulator: Power supply to the haptic motor
> + * @max_volt: maximum voltage value supplied to the haptic motor.
> + * <The unit of the voltage is a micro>
> + * @min_volt: minimum voltage value supplied to the haptic motor.
> + * <The unit of the voltage is a micro>
> + */
> +struct regulator_haptic_data {
> + struct regulator *regulator;
> + unsigned int max_volt;
> + unsigned int min_volt;
> +};
> +
> +#endif /* _REGULATOR_HAPTIC_H */
>
^ permalink raw reply
* Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup
From: Benson Leung @ 2014-11-25 1:29 UTC (permalink / raw)
To: Alan Stern
Cc: johan, Jiri Kosina, linux-usb, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Sameer Nanda
In-Reply-To: <Pine.LNX.4.44L0.1411221043060.23393-100000@netrider.rowland.org>
Hi Alan,
On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> There is no USB wrapper for pm_runtime_idle calls, but one could be
> added. Still, in the meantime can you check to see what happens if you
> add
>
> pm_runtime_idle(&usbhid->intf->dev);
>
> in usbhid_close() just after needs_remote_wakeup is set to 0? You can
> do the same thing in usbhid_stop() if you want.
I tried using this in lieu of usb_autopm_get/put_interface:
usbhid->intf->needs_remote_wakeup = 0;
pm_runtime_idle(&usbhid->intf->dev);
pm_runtime_idle(usbhid->intf->dev.parent);
It did not work. I see the autosuspend_check() that was kicked off as
a result of hid_hw_power, which falls into the "remote wakeup needed
for autosuspend" branch, but I don't see another autosuspend_check()
that picks up the updated value of needs_remote_wakeup.
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply
* Re: [PATCH] Input: xpad: use proper endpoint type
From: Dmitry Torokhov @ 2014-11-25 8:38 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-input, linux-kernel, Pierre-Loup A. Griffais
In-Reply-To: <20141124020323.GA5651@kroah.com>
On Sun, Nov 23, 2014 at 06:03:23PM -0800, Greg Kroah-Hartman wrote:
> The xpad wireless endpoint is not a bulk endpoint on my devices, but
> rather an interrupt one, so the USB core complains when it is submitted.
> I'm guessing that the author really did mean that this should be an
> interrupt urb, but as there are a zillion different xpad devices out
> there, let's cover out bases and handle both bulk and interrupt
> endpoints just as easily.
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable <stable@vger.kernel.org>
Applied, thank you.
> ---
>
> This has been in my local tree since January, don't know why it never
> got pushed out, sorry for the delay. Valve has been using a version of
> this patch for a year now.
>
> drivers/input/joystick/xpad.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -1179,9 +1179,19 @@ static int xpad_probe(struct usb_interfa
> }
>
> ep_irq_in = &intf->cur_altsetting->endpoint[1].desc;
> - usb_fill_bulk_urb(xpad->bulk_out, udev,
> - usb_sndbulkpipe(udev, ep_irq_in->bEndpointAddress),
> - xpad->bdata, XPAD_PKT_LEN, xpad_bulk_out, xpad);
> + if (usb_endpoint_is_bulk_out(ep_irq_in)) {
> + usb_fill_bulk_urb(xpad->bulk_out, udev,
> + usb_sndbulkpipe(udev,
> + ep_irq_in->bEndpointAddress),
> + xpad->bdata, XPAD_PKT_LEN,
> + xpad_bulk_out, xpad);
> + } else {
> + usb_fill_int_urb(xpad->bulk_out, udev,
> + usb_sndintpipe(udev,
> + ep_irq_in->bEndpointAddress),
> + xpad->bdata, XPAD_PKT_LEN,
> + xpad_bulk_out, xpad, 0);
> + }
>
> /*
> * Submit the int URB immediately rather than waiting for open
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: Initialize input_no by -1
From: Dmitry Torokhov @ 2014-11-25 8:40 UTC (permalink / raw)
To: Aniroop Mathur; +Cc: linux-input@vger.kernel.org
In-Reply-To: <CADYu30-dyHvidn6z6NW0w8hEwFd6Nctf8_5Yb5LKU4MFKvwLHw@mail.gmail.com>
Hi Aniroop,
On Sat, Nov 22, 2014 at 12:18:39AM +0530, Aniroop Mathur wrote:
> This patch initializes input_no by -1 in order to avoid extra subtraction
> operation performed everytime for allocation of an input device.
>
> Signed-off-by: Aniroop Mathur <aniroop.mathur@gmail.com>
It looks like you rmail client converted all tabs to spaces so the patch
can't be applied as is. Could you please fix and resend.
Thanks.
> ---
> drivers/input/input.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 29ca0bb..01fe49e 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1774,7 +1774,7 @@ EXPORT_SYMBOL_GPL(input_class);
> */
> struct input_dev *input_allocate_device(void)
> {
> - static atomic_t input_no = ATOMIC_INIT(0);
> + static atomic_t input_no = ATOMIC_INIT(-1);
> struct input_dev *dev;
>
> dev = kzalloc(sizeof(struct input_dev), GFP_KERNEL);
> @@ -1789,7 +1789,7 @@ struct input_dev *input_allocate_device(void)
> INIT_LIST_HEAD(&dev->node);
>
> dev_set_name(&dev->dev, "input%ld",
> - (unsigned long) atomic_inc_return(&input_no) - 1);
> + (unsigned long) atomic_inc_return(&input_no));
>
> __module_get(THIS_MODULE);
> }
> --
> 1.9.1
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup
From: Oliver Neukum @ 2014-11-25 9:53 UTC (permalink / raw)
To: Benson Leung
Cc: johan, Jiri Kosina, linux-usb, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Sameer Nanda
In-Reply-To: <CANLzEkv5MM8sT3WsPyqYvn=H_MHkuuELsfZQfJtJA0dwzdruKg@mail.gmail.com>
On Mon, 2014-11-24 at 16:56 -0800, Benson Leung wrote:
> Hi Oliver,
>
> On Mon, Nov 24, 2014 at 1:13 AM, Oliver Neukum <oneukum@suse.de> wrote:
> >
> > But there is very little to be gained by switching off remote wakeup.
> > The additional energy consumption devices with remote wakeup enabled
> > will be dwarfed by the energy needed for an additional wakeup.
> >
>
> That makes sense to me. Does this mean we should be moving toward a
> solution that doesn't wake suspended devices on close for other usb
> devices, not just hid?
>
> This particular pattern of get()/needs_remote_wakeup=0/put() on
> close() appears in several other drivers, for example :
> 62ecae0 Input: wacom - properly enable runtime PM
> 5d9efc5 Input: usbtouchscreen - implement runtime power management
Yes, we should never wake up a device just to unset remote
wakeup for runtime PM. In hindsight those patches were clumsy.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 0/7] Fixes for ALPS trackstick
From: Pali Rohár @ 2014-11-25 11:08 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Yunkang Tang, Vadim Klishko, linux-input,
linux-kernel
In-Reply-To: <201411200029.49547@pali>
[-- Attachment #1: Type: Text/Plain, Size: 1223 bytes --]
On Thursday 20 November 2014 00:29:49 Pali Rohár wrote:
> On Monday 17 November 2014 08:39:14 Pali Rohár wrote:
> > On Friday 14 November 2014 21:59:31 Dmitry Torokhov wrote:
> > > Hi Pali,
> > >
> > > On Friday, November 14, 2014 08:38:19 PM Pali Rohár wrote:
> > > > This patch series fix detection and identifying
> > > > trackstick on machines with ALPS devices. Last patch
> > > > split trackstick and bare PS/2 mouse packets between
> > > > dev2 and dev3 input devices which make sure that driver
> > > > will send only trackstick data to trackstick input
> > > > device.
> > >
> > > Thank you for splitting the change, unfortunately it is
> > > now quite big to apply to 3.18. Any chance you could try
> > > implementing what I suggested in
> > > http://www.spinics.net/lists/linux-input/msg34029.html and
> > > then we can do the more comprehensive solution in 3.19.
> > >
> > > Thanks.
> >
> > Hello, I think that patches 1/7 and 5/7 could do that job. I
> > did not tested them alone (without other patches), but if
> > you think that two patches are ok for 3.18 & stable I can
> > test them...
>
> Dmitry, ping.
Dmitry: ping again.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH V2] HID: i2c-hid: fix race condition reading reports
From: Jiri Kosina @ 2014-11-25 14:26 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Antonio Borneo, linux-input, Benjamin Tissoires,
linux-kernel@vger.kernel.org, Jean-Baptiste Maneyrol
In-Reply-To: <CAN+gG=HttchOAYcumPkZHKqfGb5FN83P3P8c3aKod25NXxZX1Q@mail.gmail.com>
On Mon, 24 Nov 2014, Benjamin Tissoires wrote:
> > Current driver uses a common buffer for reading reports either
> > synchronously in i2c_hid_get_raw_report() and asynchronously in
> > the interrupt handler.
> > There is race condition if an interrupt arrives immediately after
> > the report is received in i2c_hid_get_raw_report(); the common
> > buffer is modified by the interrupt handler with the new report
> > and then i2c_hid_get_raw_report() proceed using wrong data.
> >
> > Fix it by using a separate buffers for synchronous reports.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> > [Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
> > Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> > V1 -> V2
> > rename the synchronous buffer as rawbuf (instead of the
> > asynchronous one)
>
> Sorry for the lag and thanks for resubmitting.
>
> This one is reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] hid-multitouch: Add quirk for VTL touch panels
From: Jiri Kosina @ 2014-11-25 14:29 UTC (permalink / raw)
To: Mathieu Magnaudet; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <1416654127-2433-1-git-send-email-mathieu.magnaudet@enac.fr>
On Sat, 22 Nov 2014, Mathieu Magnaudet wrote:
> VTL panels do not switch to the multitouch mode until the input mode
> feature is read by the host. This should normally be done by
> usbhid, but it looks like an other bug prevents usbhid to properly
> retrieve the feature state. As a workaround, we force the reading of
> the feature in mt_set_input_mode for such devices.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/1] HID: add BETOP game controller force feedback support
From: Jiri Kosina @ 2014-11-25 14:38 UTC (permalink / raw)
To: 黄波; +Cc: linux-input, linux-kernel
In-Reply-To: <3e9f782f.a7fa.149df95fef7.Coremail.huangbobupt@163.com>
On Mon, 24 Nov 2014, 黄波 wrote:
> From: Huang Bo <huangbobupt@163.com>
>
> Adds force feedback support for BETOP USB game controllers.
> These devices are mass produced in China.
Thanks a lot for the patch. A few minor things below.
First, the whole driver formatting doesn't comply with our coding style
(it has 4 spaces instead of tab). Please reformat that for v2.
[ ... snip ... ]
> diff --git a/drivers/hid/hid-betopff.c b/drivers/hid/hid-betopff.c
> new file mode 100644
> index 0000000..d97301a
> --- /dev/null
> +++ b/drivers/hid/hid-betopff.c
> @@ -0,0 +1,170 @@
> +/*
> + * Force feedback support for Betop based devices
> + *
> + * The devices are distributed under various names and the same USB device ID
> + * can be used in both adapters and actual game controllers.
> + *
> + * 0x11C0:0x5506 "BTP2185 PC mode Joystick"
> + * - tested with BTP2185 V2 PC Mode.
> + *
> + * 0x8380:0x1850 "BTP2185 V2 PC mode USB Gamepad"
> + * - tested with BTP2185 PC Mode with another version.
> + *
> + * 0x20bc:0x5500 "BTP2185 V2 BFM mode Joystick"
> + * - tested with BTP2185 BFM Mode.
> + * - tested with BTP2171s.
> + *
> + * Copyright (c) 2014 Huang Bo <huangbobupt@163.com>
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +/* #define DEBUG */
> +
> +#define debug(format, arg...) pr_debug("hid-betopff: " format "\n" , ## arg)
> +
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/hid.h>
> +
> +#include "hid-ids.h"
> +
> +struct betopff_device {
> + struct hid_report *report;
> +};
> +
> +static int hid_betopff_play(struct input_dev *dev, void *data,
> + struct ff_effect *effect)
> +{
> + struct hid_device *hid = input_get_drvdata(dev);
> + struct betopff_device *betopff = data;
> + __u16 left, right;
> +
> + left = effect->u.rumble.strong_magnitude;
> + right = effect->u.rumble.weak_magnitude;
> +
> + betopff->report->field[2]->value[0] = left / 256;
> + betopff->report->field[3]->value[0] = right / 256;
> + debug("running with 0x%02x 0x%02x", left, right);
Is there really a need for keeping this debug() in the "production"
version?
[ ... snip ... ]
> +static const struct hid_device_id betop_devices[] = {
> + { HID_USB_DEVICE(0x11C0, 0x5506), }, /* BTP2185 PC mode */
> + { HID_USB_DEVICE(0x8380, 0x1850), }, /* BTP2185 V2 PC mode */
> + { HID_USB_DEVICE(0x20bc, 0x5500), }, /* BTP2185 V2 BFM mode */
> + { }
You also need an entry for these devices in hid_have_special_driver[]
array in drivers/hid/hid-core.c, otherwise correct device <-> driver
binding can't be guaranteed.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup
From: Alan Stern @ 2014-11-25 15:24 UTC (permalink / raw)
To: Benson Leung
Cc: johan, Jiri Kosina, linux-usb, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Sameer Nanda
In-Reply-To: <CANLzEkuZYrF_tLq5k3xBBkSmtszL851ZK7VW8Jav3vKdw9JLMA@mail.gmail.com>
On Mon, 24 Nov 2014, Benson Leung wrote:
> Hi Alan,
>
>
> On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > There is no USB wrapper for pm_runtime_idle calls, but one could be
> > added. Still, in the meantime can you check to see what happens if you
> > add
> >
> > pm_runtime_idle(&usbhid->intf->dev);
> >
> > in usbhid_close() just after needs_remote_wakeup is set to 0? You can
> > do the same thing in usbhid_stop() if you want.
>
> I tried using this in lieu of usb_autopm_get/put_interface:
>
> usbhid->intf->needs_remote_wakeup = 0;
> pm_runtime_idle(&usbhid->intf->dev);
> pm_runtime_idle(usbhid->intf->dev.parent);
>
> It did not work. I see the autosuspend_check() that was kicked off as
> a result of hid_hw_power, which falls into the "remote wakeup needed
> for autosuspend" branch, but I don't see another autosuspend_check()
> that picks up the updated value of needs_remote_wakeup.
Well, why not?
In order to work on the kernel effectively, you need the right
mind-set. Don't just tell people when something goes wrong -- figure
out why the problem occurred and propose a way to fix it.
Alan Stern
^ permalink raw reply
* Re: [PATCH 1/3] HID: wacom: Consult the application usage when determining field type
From: Benjamin Tissoires @ 2014-11-25 15:29 UTC (permalink / raw)
To: Jason Gerecke; +Cc: Jiri Kosina, linux-input, Ping Cheng
In-Reply-To: <1416871934-14133-2-git-send-email-killertofu@gmail.com>
On Mon, Nov 24, 2014 at 6:32 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> It is not necessarily sufficient to look only at the physical and logical
> usages when determining if a field is for the pen or touch. Some fields
> are not contained in a sub-collection and thus only have an application
> usage. Not checking the application usage in such cases causes us to
> ignore the field entirely, which may lead to incorrect behavior.
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
This one is Rev-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> drivers/hid/wacom_wac.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 9565d31..1468f00 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1484,9 +1484,11 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
> }
>
> #define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
> - ((f)->physical == HID_DG_STYLUS))
> + ((f)->physical == HID_DG_STYLUS) || \
> + ((f)->application == HID_DG_PEN))
> #define WACOM_FINGER_FIELD(f) (((f)->logical == HID_DG_FINGER) || \
> - ((f)->physical == HID_DG_FINGER))
> + ((f)->physical == HID_DG_FINGER) || \
> + ((f)->application == HID_DG_TOUCHSCREEN))
>
> void wacom_wac_usage_mapping(struct hid_device *hdev,
> struct hid_field *field, struct hid_usage *usage)
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup
From: Benson Leung @ 2014-11-25 15:29 UTC (permalink / raw)
To: Alan Stern
Cc: johan, Jiri Kosina, linux-usb, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Sameer Nanda
In-Reply-To: <Pine.LNX.4.44L0.1411251022141.976-100000@iolanthe.rowland.org>
On Tue, Nov 25, 2014 at 7:24 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 24 Nov 2014, Benson Leung wrote:
>
>> Hi Alan,
>>
>>
>> On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > There is no USB wrapper for pm_runtime_idle calls, but one could be
>> > added. Still, in the meantime can you check to see what happens if you
>> > add
>> >
>> > pm_runtime_idle(&usbhid->intf->dev);
>> >
>> > in usbhid_close() just after needs_remote_wakeup is set to 0? You can
>> > do the same thing in usbhid_stop() if you want.
>>
>> I tried using this in lieu of usb_autopm_get/put_interface:
>>
>> usbhid->intf->needs_remote_wakeup = 0;
>> pm_runtime_idle(&usbhid->intf->dev);
>> pm_runtime_idle(usbhid->intf->dev.parent);
>>
>> It did not work. I see the autosuspend_check() that was kicked off as
>> a result of hid_hw_power, which falls into the "remote wakeup needed
>> for autosuspend" branch, but I don't see another autosuspend_check()
>> that picks up the updated value of needs_remote_wakeup.
>
> Well, why not?
>
> In order to work on the kernel effectively, you need the right
> mind-set. Don't just tell people when something goes wrong -- figure
> out why the problem occurred and propose a way to fix it.
Sure. I'll dig into this deeper today. I got to this late yesterday
and I ran out of time before I could find out what was not behaving
correctly.
>
> Alan Stern
>
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply
* Re: [PATCH 2/3] HID: wacom: Manually declare ABS_{X,Y} for pointer emulation
From: Benjamin Tissoires @ 2014-11-25 15:34 UTC (permalink / raw)
To: Jason Gerecke; +Cc: Jiri Kosina, linux-input, Ping Cheng
In-Reply-To: <1416871934-14133-3-git-send-email-killertofu@gmail.com>
On Mon, Nov 24, 2014 at 6:32 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> If a HID descriptor places HID_DG_CONTACTID before HID_DG_X and HID_DG_Y then
> the ABS_X and ABS_Y will not be automatically initialized by the call to
> input_mt_init_slots. Here we move the setup of those axes outside of the
> 'if' statement so that they're always present if the associated HID field
> is.
No, I am afraid, that this does not work. The solution is to call
input_mt_init_slot once the mapping has been done.
I am complaining here because input_mt_init_slot will set the fuzz to
0 for ABS_X and 4 for ABS_MT_POSITION_X. If both have a fuzz of 4,
there will be a double de-fuzzing while sending out the single touch
emulation.
Likewise, the pressure should be initialized with ABS_MT_PRESSURE only
(not implemented right now, I know).
At the end of wacom_parse_hid in wacom_sys, there should be a call to
a function which would finalize the initialization. And then,
input_mt_init_slot will be called after ABS_MT_*, no matter what the
order of the fields is.
Cheers,
Benjamin
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
> drivers/hid/wacom_wac.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 1468f00..a55e0ed 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1382,16 +1382,14 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>
> switch (usage->hid) {
> case HID_GD_X:
> - if (touch_max == 1)
> - wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
> - else
> + wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
> + if (touch_max > 1)
> wacom_map_usage(wacom, usage, field, EV_ABS,
> ABS_MT_POSITION_X, 4);
> break;
> case HID_GD_Y:
> - if (touch_max == 1)
> - wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
> - else
> + wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
> + if (touch_max > 1)
> wacom_map_usage(wacom, usage, field, EV_ABS,
> ABS_MT_POSITION_Y, 4);
> break;
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH 3/3] HID: wacom: Report input events immediately upon receipt
From: Benjamin Tissoires @ 2014-11-25 15:52 UTC (permalink / raw)
To: Jason Gerecke; +Cc: Jiri Kosina, linux-input, Ping Cheng
In-Reply-To: <1416871934-14133-4-git-send-email-killertofu@gmail.com>
Hi Jason,
On Mon, Nov 24, 2014 at 6:32 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> Multitouch tablets cannot work properly if wacom_wac_finger_event simply
> stores the event details, since details about former fingers will be
> overwritten by later ones (causing wacom_wac_finger_report to effectively
> only report the state of the *last* finger in the packet).
>
> This patch modifies the logic so that events are generated as soon as
> possible in response to events. We do temporarily store HID_DG_TIPSWITCH
> value since the value of HID_DG_CONTACTID is (at least in for the tablets
> I've tested) not known until shortly afterwards.
>
OK, so, I don't like this one either :-(
I perfectly understand that the previous code worked in the only case
where there is only one contact per report. But this patch assumes
that the contact ID will come right after the tip switch if I
understood correctly. This assumption may strike back in the future
(like mine did), so we should be smarter.
In hid-multitouch, we count the total number of mt fields, then divide
by the number of time we see ContactID. This gives us the number of
fields per touch, and we then have a hook called when we reach this
number of fields while processing the report.
It's not perfect, but it's generic.
If you have a better idea, I am all ears, but unless the FW guys can
give us some guarantees regarding the HID descriptor*, I think we
should go for the most generic possible solution.
BTW, what I dislike here is that I am pretty sure that if we take this
one, we will have to revert part of it to have a 2-steps processing.
Relying on the order of the hid fields is simply not possible in the
long term.
Cheers,
Benjamin
* even if they give some guarantees, I am not sure I will trust them :)
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
> drivers/hid/wacom_wac.c | 77 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index a55e0ed..3eaa98a 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1412,19 +1412,40 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
> {
> struct wacom *wacom = hid_get_drvdata(hdev);
> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> + struct hid_data *data = &wacom_wac->hid_data;
> + unsigned mt = wacom_wac->features.touch_max > 1;
> + struct input_dev *input = wacom_wac->input;
> + bool prox = data->tipswitch && !wacom_wac->shared->stylus_in_proximity;
> + int slot;
>
> switch (usage->hid) {
> case HID_GD_X:
> - wacom_wac->hid_data.x = value;
> + if (prox) {
> + input_report_abs(input,
> + mt ? ABS_MT_POSITION_X : ABS_X,
> + value);
> + }
> break;
> case HID_GD_Y:
> - wacom_wac->hid_data.y = value;
> + if (prox) {
> + input_report_abs(input,
> + mt ? ABS_MT_POSITION_Y : ABS_Y,
> + value);
> + }
> break;
> case HID_DG_CONTACTID:
> - wacom_wac->hid_data.id = value;
> + slot = input_mt_get_slot_by_key(input, value);
> +
> + input_mt_slot(input, slot);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
> break;
> case HID_DG_TIPSWITCH:
> - wacom_wac->hid_data.tipswitch = value;
> + data->tipswitch = value;
> + if (!mt) {
> + prox = data->tipswitch &&
> + !wacom_wac->shared->stylus_in_proximity;
> + input_report_key(input, BTN_TOUCH, prox);
> + }
> break;
> }
>
> @@ -1432,33 +1453,26 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
> return 0;
> }
>
> -static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
> - struct input_dev *input, bool touch)
> +static int wacom_wac_finger_touches(struct hid_device *hdev)
> {
> - int slot;
> - struct hid_data *hid_data = &wacom_wac->hid_data;
> -
> - slot = input_mt_get_slot_by_key(input, hid_data->id);
> -
> - input_mt_slot(input, slot);
> - input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
> - if (touch) {
> - input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
> - input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
> - }
> - input_mt_sync_frame(input);
> -}
> + struct wacom *wacom = hid_get_drvdata(hdev);
> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> + struct input_dev *input = wacom_wac->input;
> + unsigned touch_max = wacom_wac->features.touch_max;
> + int count = 0;
> + int i;
>
> -static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
> - struct input_dev *input, bool touch)
> -{
> - struct hid_data *hid_data = &wacom_wac->hid_data;
> + if (touch_max == 1)
> + return (wacom_wac->hid_data.tipswitch &&
> + !wacom_wac->shared->stylus_in_proximity);
>
> - if (touch) {
> - input_report_abs(input, ABS_X, hid_data->x);
> - input_report_abs(input, ABS_Y, hid_data->y);
> + for (i = 0; i < input->mt->num_slots; i++) {
> + struct input_mt_slot *ps = &input->mt->slots[i];
> + int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
> + if (id >= 0)
> + count++;
> }
> - input_report_key(input, BTN_TOUCH, touch);
> + return count;
> }
>
> static void wacom_wac_finger_report(struct hid_device *hdev,
> @@ -1467,18 +1481,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
> struct wacom *wacom = hid_get_drvdata(hdev);
> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> struct input_dev *input = wacom_wac->input;
> - bool touch = wacom_wac->hid_data.tipswitch &&
> - !wacom_wac->shared->stylus_in_proximity;
> unsigned touch_max = wacom_wac->features.touch_max;
>
> if (touch_max > 1)
> - wacom_wac_finger_mt_report(wacom_wac, input, touch);
> - else
> - wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
> + input_mt_sync_frame(input);
> +
> input_sync(input);
>
> /* keep touch state for pen event */
> - wacom_wac->shared->touch_down = touch;
> + wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
> }
>
> #define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
> --
> 2.1.3
>
^ 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