* [PATCH 1/3] HID: sony: Enable LED controls and rumble for the Sixaxis on Bluetooth.
From: Frank Praznik @ 2014-02-18 22:22 UTC (permalink / raw)
To: linux-input; +Cc: dh.herrmann, jkosina, Frank Praznik
In-Reply-To: <1392762123-17725-1-git-send-email-frank.praznik@oh.rr.com>
Add a SIXAXIS_CONTROLLER macro to simplify conditionals where the
connection type is irrelevant.
Enable the LED and force feedback controls for Sixaxis controllers connected via
Bluetooth.
Send Sixaxis Bluetooth output reports on the control channel.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
drivers/hid/hid-sony.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index f88d1ae..81917eb 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -44,12 +44,12 @@
#define DUALSHOCK4_CONTROLLER_USB BIT(5)
#define DUALSHOCK4_CONTROLLER_BT BIT(6)
+#define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
#define DUALSHOCK4_CONTROLLER (DUALSHOCK4_CONTROLLER_USB |\
DUALSHOCK4_CONTROLLER_BT)
-#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER |\
- DUALSHOCK4_CONTROLLER)
-#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT |\
+#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
DUALSHOCK4_CONTROLLER)
+#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
#define MAX_LEDS 4
@@ -935,8 +935,7 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
/* Sixaxis HID report has acclerometers/gyro with MSByte first, this
* has to be BYTE_SWAPPED before passing up to joystick interface
*/
- if ((sc->quirks & (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)) &&
- rd[0] == 0x01 && size == 49) {
+ if ((sc->quirks & SIXAXIS_CONTROLLER) && rd[0] == 0x01 && size == 49) {
swap(rd[41], rd[42]);
swap(rd[43], rd[44]);
swap(rd[45], rd[46]);
@@ -1096,8 +1095,7 @@ static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
buzz_set_leds(hdev, leds);
- } else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) ||
- (drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
+ } else {
for (n = 0; n < count; n++)
drv_data->led_state[n] = leds[n];
schedule_work(&drv_data->state_worker);
@@ -1285,7 +1283,11 @@ static void sixaxis_state_worker(struct work_struct *work)
buf[10] |= sc->led_state[2] << 3;
buf[10] |= sc->led_state[3] << 4;
- hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
+ if (sc->quirks & SIXAXIS_CONTROLLER_USB)
+ hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
+ else
+ hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
+ HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
}
static void dualshock4_state_worker(struct work_struct *work)
@@ -1520,10 +1522,10 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
ret = sixaxis_set_operational_usb(hdev);
INIT_WORK(&sc->state_worker, sixaxis_state_worker);
- }
- else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
+ } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
ret = sixaxis_set_operational_bt(hdev);
- else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+ INIT_WORK(&sc->state_worker, sixaxis_state_worker);
+ } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
ret = dualshock4_set_operational_bt(hdev);
if (ret < 0) {
--
1.8.5.3
^ permalink raw reply related
* [PATCH 0/3] HID: sony: Various cleanups and fixes for the sony module
From: Frank Praznik @ 2014-02-18 22:22 UTC (permalink / raw)
To: linux-input; +Cc: dh.herrmann, jkosina, Frank Praznik
Since the new ll_driver functions allow for reports to be sent on the control
channel the LEDs and force-feedback can be enabled for the Sixaxis/Dualshock 3.
The second patch only sets the force-feedback flags for devices that actually
support force feedback. It also moves the cancel_work_sync function out of the
ff shutdown function since the worker functions are initialized and used for
the LEDs even when force-feedback support is disabled at compile time.
The final patch is an updated version of the duplicate controller detection code
that was originally part of the Bluetooth patch set. It now works on both the
Sixaxis and Dualshock 4. On USB the MAC can be retrieved via feature reports.
Unfortunately, neither controller supports retrieving the MAC address via a
feature report when connected via Bluetooth so it needs to be parsed from the
uniq string where HIDP stores it. Based on previous discussions
(http://www.spinics.net/lists/linux-input/msg29530.html), uniq should provide a
stable way for retrieving a Bluetooth MAC address under normal circumstances,
unless the behavior in HIDP changes for some reason. If a MAC cannot be parsed
from the uniq string the duplicate check will be skipped and the connection will
proceed, so even in the case of a uHID device or the behavior of HIDP changing
in the future, a user's controller will still work.
^ permalink raw reply
* [PATCH 2/3] HID: sony: Force-feedback cleanup
From: Frank Praznik @ 2014-02-18 22:22 UTC (permalink / raw)
To: linux-input; +Cc: dh.herrmann, jkosina, Frank Praznik
In-Reply-To: <1392762123-17725-1-git-send-email-frank.praznik@oh.rr.com>
Only initialize force feedback for devices that actually support it (Sixaxis and
Dualshock 4).
Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
is used for the LEDs even when force-feedback is disabled.
Remove the sony_destroy_ff() function since it is no longer used.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
drivers/hid/hid-sony.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 81917eb..ad1cebd 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -50,6 +50,7 @@
#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
DUALSHOCK4_CONTROLLER)
#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
+#define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
#define MAX_LEDS 4
@@ -1354,22 +1355,12 @@ static int sony_init_ff(struct hid_device *hdev)
return input_ff_create_memless(input_dev, NULL, sony_play_effect);
}
-static void sony_destroy_ff(struct hid_device *hdev)
-{
- struct sony_sc *sc = hid_get_drvdata(hdev);
-
- cancel_work_sync(&sc->state_worker);
-}
-
#else
static int sony_init_ff(struct hid_device *hdev)
{
return 0;
}
-static void sony_destroy_ff(struct hid_device *hdev)
-{
-}
#endif
static int sony_battery_get_property(struct power_supply *psy,
@@ -1567,9 +1558,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
}
}
- ret = sony_init_ff(hdev);
- if (ret < 0)
- goto err_close;
+ if (sc->quirks & SONY_FF_SUPPORT) {
+ ret = sony_init_ff(hdev);
+ if (ret < 0)
+ goto err_close;
+ }
return 0;
err_close:
@@ -1595,7 +1588,7 @@ static void sony_remove(struct hid_device *hdev)
sony_battery_remove(sc);
}
- sony_destroy_ff(hdev);
+ cancel_work_sync(&sc->state_worker);
hid_hw_stop(hdev);
}
--
1.8.5.3
^ permalink raw reply related
* [PATCH 3/3] HID: sony: Prevent duplicate controller connections.
From: Frank Praznik @ 2014-02-18 22:22 UTC (permalink / raw)
To: linux-input; +Cc: dh.herrmann, jkosina, Frank Praznik
In-Reply-To: <1392762123-17725-1-git-send-email-frank.praznik@oh.rr.com>
If a Sixaxis or Dualshock 4 controller is connected via USB while already
connected via Bluetooth it will cause duplicate devices to be added to the
input device list.
To prevent this a global list of controllers and their MAC addresses is
maintained and new controllers are checked against this list. If a duplicate
is found, the probe function with exit with -EEXIST.
On USB the MAC is retrieved via a feature report. On Bluetooth neither
controller reports the MAC address in a feature report so the MAC is parsed from
the uniq string. As uniq cannot be guaranteed to be a MAC address in every case
(uHID or the behavior of HIDP changing) a parsing failure will not prevent the
connection.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
drivers/hid/hid-sony.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 159 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index ad1cebd..c25f0d7 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -713,8 +713,12 @@ static enum power_supply_property sony_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
};
+static spinlock_t sony_dev_list_lock;
+static LIST_HEAD(sony_device_list);
+
struct sony_sc {
spinlock_t lock;
+ struct list_head list_node;
struct hid_device *hdev;
struct led_classdev *leds[MAX_LEDS];
unsigned long quirks;
@@ -726,6 +730,7 @@ struct sony_sc {
__u8 right;
#endif
+ __u8 mac_address[6];
__u8 cable_state;
__u8 battery_charging;
__u8 battery_capacity;
@@ -1473,6 +1478,137 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
return 0;
}
+/* If a controller is plugged in via USB while already connected via Bluetooth
+ * it will show up as two devices. A global list of connected controllers and
+ * their MAC addresses is maintained to ensure that a device is only connected
+ * once.
+ */
+static int sony_check_add_dev_list(struct sony_sc *sc)
+{
+ struct sony_sc *entry;
+ struct list_head *pos;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&sony_dev_list_lock, flags);
+
+ list_for_each(pos, &sony_device_list) {
+ entry = list_entry(pos, struct sony_sc, list_node);
+ ret = memcmp(sc->mac_address, entry->mac_address,
+ sizeof(sc->mac_address));
+ if (!ret) {
+ if (sc->hdev->bus != entry->hdev->bus) {
+ ret = -EEXIST;
+ hid_info(sc->hdev, "controller with MAC address %pMR already connected\n",
+ sc->mac_address);
+ } else {
+ /* Duplicate found, but on the same bus as the
+ * original. Allow the connection in this case.
+ */
+ ret = 0;
+ }
+
+ spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+ return ret;
+ }
+ }
+
+ list_add(&(sc->list_node), &sony_device_list);
+ spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+
+ return 0;
+}
+
+static void sony_remove_dev_list(struct sony_sc *sc)
+{
+ unsigned long flags;
+
+ if (sc->list_node.next) {
+ spin_lock_irqsave(&sony_dev_list_lock, flags);
+ list_del(&(sc->list_node));
+ spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+ }
+}
+
+static int sony_get_bt_devaddr(struct sony_sc *sc)
+{
+ int ret, n;
+ unsigned int mac_addr[6];
+
+ /* HIDP stores the device MAC address as a string in the uniq field. */
+ ret = strlen(sc->hdev->uniq);
+ if (ret != 17)
+ return -EINVAL;
+
+ ret = sscanf(sc->hdev->uniq, "%02x:%02x:%02x:%02x:%02x:%02x",
+ &mac_addr[5], &mac_addr[4], &mac_addr[3], &mac_addr[2],
+ &mac_addr[1], &mac_addr[0]);
+
+ if (ret != 6)
+ return -EINVAL;
+
+ for (n = 0; n < 6; n++)
+ sc->mac_address[n] = (__u8)mac_addr[n];
+
+ return 0;
+}
+
+static int sony_check_add(struct sony_sc *sc)
+{
+ int n, ret;
+
+ if ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) ||
+ (sc->quirks & SIXAXIS_CONTROLLER_BT)) {
+ /* sony_get_bt_devaddr() attempts to parse the Bluetooth MAC
+ * address from the uniq string where HIDP stores it.
+ * As uniq cannot be guaranteed to be a MAC address in all cases
+ * a failure of this function should not prevent the connection.
+ */
+ if (sony_get_bt_devaddr(sc) < 0) {
+ hid_warn(sc->hdev, "UNIQ does not contain a MAC address; duplicate check skipped\n");
+ return 0;
+ }
+ } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
+ __u8 buf[7];
+
+ /* The MAC address of a DS4 controller connected via USB can be
+ * retrieved with feature report 0x81. The address begins at
+ * offset 1.
+ */
+ ret = hid_hw_raw_request(sc->hdev, 0x81, buf, sizeof(buf),
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+
+ if (ret != 7) {
+ hid_err(sc->hdev, "failed to retrieve feature report 0x81 with the DualShock 4 MAC address\n");
+ return ret < 0 ? ret : -EINVAL;
+ }
+
+ memcpy(sc->mac_address, &buf[1], sizeof(sc->mac_address));
+ } else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
+ __u8 buf[18];
+
+ /* The MAC address of a Sixaxis controller connected via USB can
+ * be retrieved with feature report 0xf2. The address begins at
+ * offset 4.
+ */
+ ret = hid_hw_raw_request(sc->hdev, 0xf2, buf, sizeof(buf),
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+
+ if (ret != 18) {
+ hid_err(sc->hdev, "failed to retrieve feature report 0xf2 with the Sixaxis MAC address\n");
+ return ret < 0 ? ret : -EINVAL;
+ }
+
+ /* The Sixaxis device MAC in the report is in reverse order */
+ for (n = 0; n < 6; n++)
+ sc->mac_address[5-n] = buf[4+n];
+ } else {
+ return 0;
+ }
+
+ return sony_check_add_dev_list(sc);
+}
+
static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
int ret;
@@ -1509,12 +1645,22 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
- hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
- ret = sixaxis_set_operational_usb(hdev);
- INIT_WORK(&sc->state_worker, sixaxis_state_worker);
- } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
- ret = sixaxis_set_operational_bt(hdev);
+ if (sc->quirks & SIXAXIS_CONTROLLER) {
+ if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
+ hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
+ ret = sixaxis_set_operational_usb(hdev);
+ } else
+ ret = sixaxis_set_operational_bt(hdev);
+
+ if (ret < 0) {
+ hid_err(hdev, "failed to set the Sixaxis operational mode\n");
+ goto err_stop;
+ }
+
+ ret = sony_check_add(sc);
+ if (ret < 0)
+ goto err_stop;
+
INIT_WORK(&sc->state_worker, sixaxis_state_worker);
} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
@@ -1524,6 +1670,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto err_stop;
}
}
+
+ ret = sony_check_add(sc);
+ if (ret < 0)
+ goto err_stop;
+
/* The Dualshock 4 touchpad supports 2 touches and has a
* resolution of 1920x940.
*/
@@ -1588,6 +1739,8 @@ static void sony_remove(struct hid_device *hdev)
sony_battery_remove(sc);
}
+ sony_remove_dev_list(sc);
+
cancel_work_sync(&sc->state_worker);
hid_hw_stop(hdev);
--
1.8.5.3
^ permalink raw reply related
* [PATCH] Input: ALPS - add support for HP Pavilion 14 TS
From: AceLan Kao @ 2014-02-19 7:08 UTC (permalink / raw)
To: linux-input, Dmitry Torokhov
Without the patch, the touchpad works with no cursor.
[ 137.718584] psmouse serio1: alps: Unknown ALPS touchpad: E7=73 03 14, EC=73 02 36
[ 138.536736] input: ImPS/2 Generic Wheel Mouse as /devices/platform/i8042/serio1/input/input12
After adding its data into the table, it works with cursor.
[ 182.003025] input: DualPoint Stick as /devices/platform/i8042/serio1/input/input13
[ 182.022643] input: AlpsPS/2 ALPS DualPoint TouchPad as /devices/platform/i8042/serio1/input/input14
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
drivers/input/mouse/alps.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index fb15c64..74859a0 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -123,6 +123,7 @@ static const struct alps_model_info alps_model_data[] = {
{ { 0x62, 0x02, 0x14 }, 0x00, ALPS_PROTO_V2, 0xcf, 0xcf,
ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED },
{ { 0x73, 0x00, 0x14 }, 0x00, ALPS_PROTO_V6, 0xff, 0xff, ALPS_DUALPOINT }, /* Dell XT2 */
+ { { 0x73, 0x03, 0x14 }, 0x00, ALPS_PROTO_V6, 0xff, 0xff, ALPS_DUALPOINT }, /* HP Pavilion 14 TS Notebook PC */
{ { 0x73, 0x02, 0x50 }, 0x00, ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS }, /* Dell Vostro 1400 */
{ { 0x52, 0x01, 0x14 }, 0x00, ALPS_PROTO_V2, 0xff, 0xff,
ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED }, /* Toshiba Tecra A11-11L */
--
1.9.rc1
^ permalink raw reply related
* Re: [PATCH 1/3] HID: sony: Enable LED controls and rumble for the Sixaxis on Bluetooth.
From: David Herrmann @ 2014-02-19 10:18 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1392762123-17725-2-git-send-email-frank.praznik@oh.rr.com>
Hi
On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add a SIXAXIS_CONTROLLER macro to simplify conditionals where the
> connection type is irrelevant.
>
> Enable the LED and force feedback controls for Sixaxis controllers connected via
> Bluetooth.
>
> Send Sixaxis Bluetooth output reports on the control channel.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> drivers/hid/hid-sony.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index f88d1ae..81917eb 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -44,12 +44,12 @@
> #define DUALSHOCK4_CONTROLLER_USB BIT(5)
> #define DUALSHOCK4_CONTROLLER_BT BIT(6)
>
> +#define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
> #define DUALSHOCK4_CONTROLLER (DUALSHOCK4_CONTROLLER_USB |\
> DUALSHOCK4_CONTROLLER_BT)
> -#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER |\
> - DUALSHOCK4_CONTROLLER)
> -#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT |\
> +#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
> DUALSHOCK4_CONTROLLER)
> +#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
>
> #define MAX_LEDS 4
>
> @@ -935,8 +935,7 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
> /* Sixaxis HID report has acclerometers/gyro with MSByte first, this
> * has to be BYTE_SWAPPED before passing up to joystick interface
> */
> - if ((sc->quirks & (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)) &&
> - rd[0] == 0x01 && size == 49) {
> + if ((sc->quirks & SIXAXIS_CONTROLLER) && rd[0] == 0x01 && size == 49) {
> swap(rd[41], rd[42]);
> swap(rd[43], rd[44]);
> swap(rd[45], rd[46]);
> @@ -1096,8 +1095,7 @@ static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
>
> if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
> buzz_set_leds(hdev, leds);
> - } else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) ||
> - (drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
> + } else {
> for (n = 0; n < count; n++)
> drv_data->led_state[n] = leds[n];
> schedule_work(&drv_data->state_worker);
> @@ -1285,7 +1283,11 @@ static void sixaxis_state_worker(struct work_struct *work)
> buf[10] |= sc->led_state[2] << 3;
> buf[10] |= sc->led_state[3] << 4;
>
> - hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> + if (sc->quirks & SIXAXIS_CONTROLLER_USB)
> + hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> + else
> + hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
> + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> }
>
> static void dualshock4_state_worker(struct work_struct *work)
> @@ -1520,10 +1522,10 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> ret = sixaxis_set_operational_usb(hdev);
> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> - }
> - else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> + } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
We usually send coding-style cleanups as separate patches so reverts
will not affect them. Anyhow, no need to resend imho, so:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> ret = sixaxis_set_operational_bt(hdev);
> - else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> + INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> + } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
> ret = dualshock4_set_operational_bt(hdev);
> if (ret < 0) {
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: [PATCH 2/3] HID: sony: Force-feedback cleanup
From: David Herrmann @ 2014-02-19 10:23 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1392762123-17725-3-git-send-email-frank.praznik@oh.rr.com>
Hi
On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Only initialize force feedback for devices that actually support it (Sixaxis and
> Dualshock 4).
>
> Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
> is used for the LEDs even when force-feedback is disabled.
>
> Remove the sony_destroy_ff() function since it is no longer used.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> drivers/hid/hid-sony.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 81917eb..ad1cebd 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -50,6 +50,7 @@
> #define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
> DUALSHOCK4_CONTROLLER)
> #define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
> +#define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
>
> #define MAX_LEDS 4
>
> @@ -1354,22 +1355,12 @@ static int sony_init_ff(struct hid_device *hdev)
> return input_ff_create_memless(input_dev, NULL, sony_play_effect);
> }
>
> -static void sony_destroy_ff(struct hid_device *hdev)
> -{
> - struct sony_sc *sc = hid_get_drvdata(hdev);
> -
> - cancel_work_sync(&sc->state_worker);
> -}
> -
> #else
> static int sony_init_ff(struct hid_device *hdev)
> {
> return 0;
> }
>
> -static void sony_destroy_ff(struct hid_device *hdev)
> -{
> -}
> #endif
>
> static int sony_battery_get_property(struct power_supply *psy,
> @@ -1567,9 +1558,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> - ret = sony_init_ff(hdev);
> - if (ret < 0)
> - goto err_close;
> + if (sc->quirks & SONY_FF_SUPPORT) {
> + ret = sony_init_ff(hdev);
> + if (ret < 0)
> + goto err_close;
> + }
>
> return 0;
> err_close:
> @@ -1595,7 +1588,7 @@ static void sony_remove(struct hid_device *hdev)
> sony_battery_remove(sc);
> }
>
> - sony_destroy_ff(hdev);
> + cancel_work_sync(&sc->state_worker);
Ugh, that's a bugfix! This can trigger if the led-worker runs during
unplug. You should mark it for stable next time you send it (adding:
Cc: <stable@vger.kernel.org>)
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
>
> hid_hw_stop(hdev);
> }
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: [PATCH 3/3] HID: sony: Prevent duplicate controller connections.
From: David Herrmann @ 2014-02-19 11:04 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1392762123-17725-4-git-send-email-frank.praznik@oh.rr.com>
Hi
On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> If a Sixaxis or Dualshock 4 controller is connected via USB while already
> connected via Bluetooth it will cause duplicate devices to be added to the
> input device list.
>
> To prevent this a global list of controllers and their MAC addresses is
> maintained and new controllers are checked against this list. If a duplicate
> is found, the probe function with exit with -EEXIST.
>
> On USB the MAC is retrieved via a feature report. On Bluetooth neither
> controller reports the MAC address in a feature report so the MAC is parsed from
> the uniq string. As uniq cannot be guaranteed to be a MAC address in every case
> (uHID or the behavior of HIDP changing) a parsing failure will not prevent the
> connection.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> drivers/hid/hid-sony.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 159 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index ad1cebd..c25f0d7 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -713,8 +713,12 @@ static enum power_supply_property sony_battery_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> };
>
> +static spinlock_t sony_dev_list_lock;
> +static LIST_HEAD(sony_device_list);
Please include <linux/list.h>
> +
> struct sony_sc {
> spinlock_t lock;
> + struct list_head list_node;
> struct hid_device *hdev;
> struct led_classdev *leds[MAX_LEDS];
> unsigned long quirks;
> @@ -726,6 +730,7 @@ struct sony_sc {
> __u8 right;
> #endif
>
> + __u8 mac_address[6];
> __u8 cable_state;
> __u8 battery_charging;
> __u8 battery_capacity;
> @@ -1473,6 +1478,137 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
> return 0;
> }
>
> +/* If a controller is plugged in via USB while already connected via Bluetooth
> + * it will show up as two devices. A global list of connected controllers and
> + * their MAC addresses is maintained to ensure that a device is only connected
> + * once.
> + */
We usually use one of those comment-styles:
/*
* some
* comment
*/
/* some
* comment */
..but I use my own style all the time, so who am I to judge.. You
might get screamed at in other subsystems, though.
> +static int sony_check_add_dev_list(struct sony_sc *sc)
> +{
> + struct sony_sc *entry;
> + struct list_head *pos;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&sony_dev_list_lock, flags);
> +
> + list_for_each(pos, &sony_device_list) {
> + entry = list_entry(pos, struct sony_sc, list_node);
You can use "list_for_each_entry()" here.
> + ret = memcmp(sc->mac_address, entry->mac_address,
> + sizeof(sc->mac_address));
> + if (!ret) {
> + if (sc->hdev->bus != entry->hdev->bus) {
> + ret = -EEXIST;
> + hid_info(sc->hdev, "controller with MAC address %pMR already connected\n",
> + sc->mac_address);
> + } else {
> + /* Duplicate found, but on the same bus as the
> + * original. Allow the connection in this case.
> + */
> + ret = 0;
Please drop that. Imagine some device uses BT-LE at some point and
thus is managed via uHID. If the device is connected via BT *and*
BT-LE, both will have BUS_BLUETOOTH set but are the same device. I
think the mac-comparison is enough. I don't know why a single device
would be connected twice and be managed by hid-sony..
> + }
> +
> + spin_unlock_irqrestore(&sony_dev_list_lock, flags);
> + return ret;
Usually we use:
r = -ESOMETHING;
goto unlock;
...
r = 0;
unlock:
spin_unlock_irqrestore(&sony_dev_list_lock, flags);
return r;
This way the locking is symmetric and easier to review.
> + }
> + }
> +
> + list_add(&(sc->list_node), &sony_device_list);
> + spin_unlock_irqrestore(&sony_dev_list_lock, flags);
> +
> + return 0;
> +}
> +
> +static void sony_remove_dev_list(struct sony_sc *sc)
> +{
> + unsigned long flags;
> +
> + if (sc->list_node.next) {
> + spin_lock_irqsave(&sony_dev_list_lock, flags);
> + list_del(&(sc->list_node));
> + spin_unlock_irqrestore(&sony_dev_list_lock, flags);
> + }
> +}
> +
> +static int sony_get_bt_devaddr(struct sony_sc *sc)
> +{
> + int ret, n;
> + unsigned int mac_addr[6];
> +
> + /* HIDP stores the device MAC address as a string in the uniq field. */
> + ret = strlen(sc->hdev->uniq);
Are you sure "uniq" cannot be NULL?
> + if (ret != 17)
> + return -EINVAL;
> +
> + ret = sscanf(sc->hdev->uniq, "%02x:%02x:%02x:%02x:%02x:%02x",
> + &mac_addr[5], &mac_addr[4], &mac_addr[3], &mac_addr[2],
> + &mac_addr[1], &mac_addr[0]);
%02hhx so you can avoid the temporary copy?
> +
> + if (ret != 6)
> + return -EINVAL;
> +
> + for (n = 0; n < 6; n++)
> + sc->mac_address[n] = (__u8)mac_addr[n];
> +
> + return 0;
> +}
> +
> +static int sony_check_add(struct sony_sc *sc)
> +{
> + int n, ret;
> +
> + if ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) ||
> + (sc->quirks & SIXAXIS_CONTROLLER_BT)) {
> + /* sony_get_bt_devaddr() attempts to parse the Bluetooth MAC
> + * address from the uniq string where HIDP stores it.
> + * As uniq cannot be guaranteed to be a MAC address in all cases
> + * a failure of this function should not prevent the connection.
> + */
> + if (sony_get_bt_devaddr(sc) < 0) {
> + hid_warn(sc->hdev, "UNIQ does not contain a MAC address; duplicate check skipped\n");
> + return 0;
> + }
> + } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> + __u8 buf[7];
> +
> + /* The MAC address of a DS4 controller connected via USB can be
> + * retrieved with feature report 0x81. The address begins at
> + * offset 1.
> + */
> + ret = hid_hw_raw_request(sc->hdev, 0x81, buf, sizeof(buf),
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +
> + if (ret != 7) {
> + hid_err(sc->hdev, "failed to retrieve feature report 0x81 with the DualShock 4 MAC address\n");
> + return ret < 0 ? ret : -EINVAL;
> + }
> +
> + memcpy(sc->mac_address, &buf[1], sizeof(sc->mac_address));
> + } else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> + __u8 buf[18];
> +
> + /* The MAC address of a Sixaxis controller connected via USB can
> + * be retrieved with feature report 0xf2. The address begins at
> + * offset 4.
> + */
> + ret = hid_hw_raw_request(sc->hdev, 0xf2, buf, sizeof(buf),
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +
> + if (ret != 18) {
> + hid_err(sc->hdev, "failed to retrieve feature report 0xf2 with the Sixaxis MAC address\n");
> + return ret < 0 ? ret : -EINVAL;
> + }
> +
> + /* The Sixaxis device MAC in the report is in reverse order */
"reverse" sounds weird here.. addresses are usually stored in
network-byte-order (BigEndian), so lets be clear here and say the
input is little-endian so you have to swap it. Or am I wrong?
> + for (n = 0; n < 6; n++)
> + sc->mac_address[5-n] = buf[4+n];
> + } else {
> + return 0;
> + }
> +
> + return sony_check_add_dev_list(sc);
> +}
> +
> static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> int ret;
> @@ -1509,12 +1645,22 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> - if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> - hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> - ret = sixaxis_set_operational_usb(hdev);
> - INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> - } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
> - ret = sixaxis_set_operational_bt(hdev);
> + if (sc->quirks & SIXAXIS_CONTROLLER) {
> + if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> + hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> + ret = sixaxis_set_operational_usb(hdev);
> + } else
> + ret = sixaxis_set_operational_bt(hdev);
> +
> + if (ret < 0) {
> + hid_err(hdev, "failed to set the Sixaxis operational mode\n");
> + goto err_stop;
> + }
> +
> + ret = sony_check_add(sc);
> + if (ret < 0)
> + goto err_stop;
> +
You check the quirks in sony_check_add() again, so why not leave all
this code as is and call sony_check_add() below for *all* devices?
Could you add a line to net/hidp/core.c where we write UNIQ that some
devices depend on this? Other than that, the patch looks good.
Thanks
David
> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
> @@ -1524,6 +1670,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto err_stop;
> }
> }
> +
> + ret = sony_check_add(sc);
> + if (ret < 0)
> + goto err_stop;
> +
> /* The Dualshock 4 touchpad supports 2 touches and has a
> * resolution of 1920x940.
> */
> @@ -1588,6 +1739,8 @@ static void sony_remove(struct hid_device *hdev)
> sony_battery_remove(sc);
> }
>
> + sony_remove_dev_list(sc);
> +
> cancel_work_sync(&sc->state_worker);
>
> hid_hw_stop(hdev);
> --
> 1.8.5.3
>
^ permalink raw reply
* ff-core effect id handling in case of a failed effect upload
From: Elias Vanderstuyft @ 2014-02-19 11:42 UTC (permalink / raw)
To: Anssi Hannula; +Cc: linux-input, wine-devel
Hi,
In the process of reviewing the Wine DInput translation layer, I
noticed an inconvenience (in the ff-core implementation?) that can
possibly lead to confusing problems to application developers (not
only for Wine), in short:
If a new (id==-1) effect was uploaded (look at
ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
ff-core will have assigned a positive number to the effect id. This
can be confusing because the dev->ff->effects[] array will not contain
an element at the index of that new effect id.
Here is a more elaborated description/discussion:
- This is a bug that is either the responsibility of the Linux kernel,
or of Wine (and possibly other applications that do the same thing as
described below):
It is caused by the following situation:
When uploading an effect, the specific kernel device driver
may return an error,
e.g. EINVAL for example when a periodic's effect period is set to zero.
This error will then be returned by "ioctl(*(This->fd),
EVIOCSFF, &This->effect)".
With or without error, one can find out that
/drivers/input/ff-core.c:input_ff_upload(...) is called,
which will set effect->id >= 0, if it was set to -1 (=> new
effect created) by the user.
But in case of an error:
- Assume effect->id was set to -1 by the user:
The error is reported by ff->upload(...) at
/drivers/input/ff-core.c:167,
the effect->id will also be set >= 0 (*).
The offending effect will not be saved in the
ff->effects[] array (***).
- Assume effect->id was set >= 0 by the user (and
ff->effects[effect->id] is a valid existing effect):
The error is reported by ff->upload(...) at
/drivers/input/ff-core.c:167,
the effect->id will remain unchanged (**).
The offending effect will not overwrite the
ff->effects[effect->id] element (****).
Is this (see *, **, *** and ****) desired behaviour?
- If yes:
Change the following in Wine's dinput/effect_linuxinput.c:90 :
if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
to :
int effectId_old = This->effect.id;
if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
This->effect.id = effectId_old;
- If no for *:
Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
has to be patched to revert "effect->id" back to its original value
set by the user,
which is only needed when the initial (by user) value of
"effect->id" was equal to -1.
- If no for **** (or maybe also ***):
ff->effects[effect->id] could be replaced by an 'empty'
effect (however this can get complex because the effect's type has to
remain unchanged)
This would be a change in the kernel code
/drivers/input/ff-core.c:input_ff_upload(...).
- If no for **:
I don't really know. Discussion is needed.
- In my opinion, **, *** and **** are desired behaviour, while
* should leave the effect->id at -1.
In that case, Wine's dinput implementation does not have
to be patched, and the kernel code only should apply a minor patch.
Best regards,
Elias
^ permalink raw reply
* [PATCH v2 2/3] Input: gpio_keys - convert to use devm_*
From: Andy Shevchenko @ 2014-02-19 13:21 UTC (permalink / raw)
To: Dmitry Torokhov, Linus Walleij, linux-input; +Cc: Andy Shevchenko
In-Reply-To: <1392816071-23839-1-git-send-email-andriy.shevchenko@linux.intel.com>
This makes the error handling much more simpler than open-coding everything and
in addition makes the probe function smaller an tidier.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/input/keyboard/gpio_keys.c | 75 +++++++++++++-------------------------
1 file changed, 25 insertions(+), 50 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 209d4c6..8791d94 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -47,7 +47,7 @@ struct gpio_keys_drvdata {
const struct gpio_keys_platform_data *pdata;
struct input_dev *input;
struct mutex disable_lock;
- struct gpio_button_data data[0];
+ struct gpio_button_data *data;
};
/*
@@ -577,25 +577,22 @@ gpio_keys_get_devtree_pdata(struct device *dev)
int i;
node = dev->of_node;
- if (!node) {
- error = -ENODEV;
- goto err_out;
- }
+ if (!node)
+ return ERR_PTR(-ENODEV);
nbuttons = of_get_child_count(node);
- if (nbuttons == 0) {
- error = -ENODEV;
- goto err_out;
- }
+ if (nbuttons == 0)
+ return ERR_PTR(-ENODEV);
- pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button),
- GFP_KERNEL);
- if (!pdata) {
- error = -ENOMEM;
- goto err_out;
- }
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ pdata->buttons = devm_kcalloc(dev, nbuttons, sizeof (*button),
+ GFP_KERNEL);
+ if (!pdata->buttons)
+ return ERR_PTR(-ENOMEM);
- pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
pdata->nbuttons = nbuttons;
pdata->rep = !!of_get_property(node, "autorepeat", NULL);
@@ -618,7 +615,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
dev_err(dev,
"Failed to get gpio flags, error: %d\n",
error);
- goto err_free_pdata;
+ return ERR_PTR(error);
}
button = &pdata->buttons[i++];
@@ -629,8 +626,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
if (of_property_read_u32(pp, "linux,code", &button->code)) {
dev_err(dev, "Button without keycode: 0x%x\n",
button->gpio);
- error = -EINVAL;
- goto err_free_pdata;
+ return ERR_PTR(-EINVAL);
}
button->desc = of_get_property(pp, "label", NULL);
@@ -645,17 +641,10 @@ gpio_keys_get_devtree_pdata(struct device *dev)
button->debounce_interval = 5;
}
- if (pdata->nbuttons == 0) {
- error = -EINVAL;
- goto err_free_pdata;
- }
+ if (pdata->nbuttons == 0)
+ return ERR_PTR(-EINVAL);
return pdata;
-
-err_free_pdata:
- kfree(pdata);
-err_out:
- return ERR_PTR(error);
}
static struct of_device_id gpio_keys_of_match[] = {
@@ -699,16 +688,18 @@ static int gpio_keys_probe(struct platform_device *pdev)
return PTR_ERR(pdata);
}
- ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
- pdata->nbuttons * sizeof(struct gpio_button_data),
- GFP_KERNEL);
- input = input_allocate_device();
+ ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+ input = devm_input_allocate_device(dev);
if (!ddata || !input) {
dev_err(dev, "failed to allocate state\n");
- error = -ENOMEM;
- goto fail1;
+ return -ENOMEM;
}
+ ddata->data = devm_kcalloc(dev, pdata->nbuttons, sizeof(*ddata->data),
+ GFP_KERNEL);
+ if (!ddata->data)
+ return -ENOMEM;
+
ddata->pdata = pdata;
ddata->input = input;
mutex_init(&ddata->disable_lock);
@@ -767,20 +758,12 @@ static int gpio_keys_probe(struct platform_device *pdev)
while (--i >= 0)
gpio_remove_key(&ddata->data[i]);
- fail1:
- input_free_device(input);
- kfree(ddata);
- /* If we have no platform data, we allocated pdata dynamically. */
- if (!dev_get_platdata(&pdev->dev))
- kfree(pdata);
-
return error;
}
static int gpio_keys_remove(struct platform_device *pdev)
{
struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
- struct input_dev *input = ddata->input;
int i;
sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -790,14 +773,6 @@ static int gpio_keys_remove(struct platform_device *pdev)
for (i = 0; i < ddata->pdata->nbuttons; i++)
gpio_remove_key(&ddata->data[i]);
- input_unregister_device(input);
-
- /* If we have no platform data, we allocated pdata dynamically. */
- if (!dev_get_platdata(&pdev->dev))
- kfree(ddata->pdata);
-
- kfree(ddata);
-
return 0;
}
--
1.9.0
^ permalink raw reply related
* [PATCH v2 3/3] Input: gpio_keys - convert struct descriptions to kernel-doc
From: Andy Shevchenko @ 2014-02-19 13:21 UTC (permalink / raw)
To: Dmitry Torokhov, Linus Walleij, linux-input; +Cc: Andy Shevchenko
In-Reply-To: <1392816071-23839-1-git-send-email-andriy.shevchenko@linux.intel.com>
This patch converts descriptions of the structures defined in linux/gpio_keys.h
to follow kernel-doc format.
There is no functional change.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
include/linux/gpio_keys.h | 45 +++++++++++++++++++++++++++++++++------------
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index a7e977f..997134a 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -3,29 +3,50 @@
struct device;
+/**
+ * struct gpio_keys_button - configuration parameters
+ * @code: input event code (KEY_*, SW_*)
+ * @gpio: %-1 if this key does not support gpio
+ * @active_low:
+ * @desc:
+ * @type: input event type (%EV_KEY, %EV_SW, %EV_ABS)
+ * @wakeup: configure the button as a wake-up source
+ * @debounce_interval: debounce ticks interval in msecs
+ * @can_disable:
+ * @value: axis value for %EV_ABS
+ * @irq: Irq number in case of interrupt keys
+ */
struct gpio_keys_button {
- /* Configuration parameters */
- unsigned int code; /* input event code (KEY_*, SW_*) */
- int gpio; /* -1 if this key does not support gpio */
+ unsigned int code;
+ int gpio;
int active_low;
const char *desc;
- unsigned int type; /* input event type (EV_KEY, EV_SW, EV_ABS) */
- int wakeup; /* configure the button as a wake-up source */
- int debounce_interval; /* debounce ticks interval in msecs */
+ unsigned int type;
+ int wakeup;
+ int debounce_interval;
bool can_disable;
- int value; /* axis value for EV_ABS */
- unsigned int irq; /* Irq number in case of interrupt keys */
+ int value;
+ unsigned int irq;
};
+/**
+ * struct gpio_keys_platform_data - platform data for gpio_keys driver
+ * @buttons:
+ * @nbuttons:
+ * @poll_interval: polling interval in msecs - for polling driver only
+ * @rep: enable input subsystem auto repeat
+ * @enable:
+ * @disable:
+ * @name: input device name
+ */
struct gpio_keys_platform_data {
struct gpio_keys_button *buttons;
int nbuttons;
- unsigned int poll_interval; /* polling interval in msecs -
- for polling driver only */
- unsigned int rep:1; /* enable input subsystem auto repeat */
+ unsigned int poll_interval;
+ unsigned int rep:1;
int (*enable)(struct device *dev);
void (*disable)(struct device *dev);
- const char *name; /* input device name */
+ const char *name;
};
#endif
--
1.9.0
^ permalink raw reply related
* [PATCH v2 1/3] Input: gpio_keys - use dev instead of pdev in gpio_keys_setup_key()
From: Andy Shevchenko @ 2014-02-19 13:21 UTC (permalink / raw)
To: Dmitry Torokhov, Linus Walleij, linux-input; +Cc: Andy Shevchenko
The platform device is not used in gpio_keys_setup_key(). This patch
substitutes it by struct device.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/input/keyboard/gpio_keys.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2db1324..209d4c6 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -424,13 +424,12 @@ out:
return IRQ_HANDLED;
}
-static int gpio_keys_setup_key(struct platform_device *pdev,
+static int gpio_keys_setup_key(struct device *dev,
struct input_dev *input,
struct gpio_button_data *bdata,
const struct gpio_keys_button *button)
{
const char *desc = button->desc ? button->desc : "gpio_keys";
- struct device *dev = &pdev->dev;
irq_handler_t isr;
unsigned long irqflags;
int irq, error;
@@ -719,7 +718,7 @@ static int gpio_keys_probe(struct platform_device *pdev)
input->name = pdata->name ? : pdev->name;
input->phys = "gpio-keys/input0";
- input->dev.parent = &pdev->dev;
+ input->dev.parent = dev;
input->open = gpio_keys_open;
input->close = gpio_keys_close;
@@ -736,7 +735,7 @@ static int gpio_keys_probe(struct platform_device *pdev)
const struct gpio_keys_button *button = &pdata->buttons[i];
struct gpio_button_data *bdata = &ddata->data[i];
- error = gpio_keys_setup_key(pdev, input, bdata, button);
+ error = gpio_keys_setup_key(dev, input, bdata, button);
if (error)
goto fail2;
--
1.9.0
^ permalink raw reply related
* Re: ff-core effect id handling in case of a failed effect upload
From: Anssi Hannula @ 2014-02-19 16:49 UTC (permalink / raw)
To: Elias Vanderstuyft, Dmitry Torokhov; +Cc: linux-input, wine-devel
In-Reply-To: <CADbOyBRX8Zd=WyXpgWo8Kkg5HkAJHjSA0KCiJD_36PLiSKAQJQ@mail.gmail.com>
(added Dmitry to CC)
19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
> Hi,
>
Hi,
> In the process of reviewing the Wine DInput translation layer, I
> noticed an inconvenience (in the ff-core implementation?) that can
> possibly lead to confusing problems to application developers (not
> only for Wine), in short:
> If a new (id==-1) effect was uploaded (look at
> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
> ff-core will have assigned a positive number to the effect id. This
> can be confusing because the dev->ff->effects[] array will not contain
> an element at the index of that new effect id.
I agree that this is a bit confusing, and the kernel code should
probably be modified to not clobber the ioctl-provided effect on failure
(effect->id is set to an "undefined" value, i.e. next free effect slot).
Dmitry, WDYT?
The downside is that if changed, any new userspace code may unknowingly
depend on the fixed non-clobbering behavior (though apparently they
already do, as evidenced by Wine DInput, so might not be much of an
argument...).
> Here is a more elaborated description/discussion:
> - This is a bug that is either the responsibility of the Linux kernel,
> or of Wine (and possibly other applications that do the same thing as
> described below):
> It is caused by the following situation:
> When uploading an effect, the specific kernel device driver
> may return an error,
> e.g. EINVAL for example when a periodic's effect period is set to zero.
> This error will then be returned by "ioctl(*(This->fd),
> EVIOCSFF, &This->effect)".
> With or without error, one can find out that
> /drivers/input/ff-core.c:input_ff_upload(...) is called,
> which will set effect->id >= 0, if it was set to -1 (=> new
> effect created) by the user.
>
> But in case of an error:
> - Assume effect->id was set to -1 by the user:
> The error is reported by ff->upload(...) at
> /drivers/input/ff-core.c:167,
> the effect->id will also be set >= 0 (*).
> The offending effect will not be saved in the
> ff->effects[] array (***).
> - Assume effect->id was set >= 0 by the user (and
> ff->effects[effect->id] is a valid existing effect):
> The error is reported by ff->upload(...) at
> /drivers/input/ff-core.c:167,
> the effect->id will remain unchanged (**).
> The offending effect will not overwrite the
> ff->effects[effect->id] element (****).
>
> Is this (see *, **, *** and ****) desired behaviour?
> - If yes:
> Change the following in Wine's dinput/effect_linuxinput.c:90 :
> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
> to :
> int effectId_old = This->effect.id;
> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
> This->effect.id = effectId_old;
> - If no for *:
> Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
> has to be patched to revert "effect->id" back to its original value
> set by the user,
> which is only needed when the initial (by user) value of
> "effect->id" was equal to -1.
> - If no for **** (or maybe also ***):
> ff->effects[effect->id] could be replaced by an 'empty'
> effect (however this can get complex because the effect's type has to
> remain unchanged)
> This would be a change in the kernel code
> /drivers/input/ff-core.c:input_ff_upload(...).
> - If no for **:
> I don't really know. Discussion is needed.
>
> - In my opinion, **, *** and **** are desired behaviour, while
> * should leave the effect->id at -1.
Right.
> In that case, Wine's dinput implementation does not have
> to be patched, and the kernel code only should apply a minor patch.
Well, maybe better to change dinput anyway so that it can work properly
with current kernel versions (assuming this gets changed in the future)?
Not my call, though.
--
Anssi Hannula
^ permalink raw reply
* Re: ff-core effect id handling in case of a failed effect upload
From: Elias Vanderstuyft @ 2014-02-19 17:32 UTC (permalink / raw)
To: Anssi Hannula; +Cc: Dmitry Torokhov, linux-input, wine-devel
In-Reply-To: <5304E0A0.5070007@iki.fi>
On Wed, Feb 19, 2014 at 5:49 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> (added Dmitry to CC)
>
> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> Hi,
>>
>
> Hi,
>
>> In the process of reviewing the Wine DInput translation layer, I
>> noticed an inconvenience (in the ff-core implementation?) that can
>> possibly lead to confusing problems to application developers (not
>> only for Wine), in short:
>> If a new (id==-1) effect was uploaded (look at
>> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> ff-core will have assigned a positive number to the effect id. This
>> can be confusing because the dev->ff->effects[] array will not contain
>> an element at the index of that new effect id.
>
> I agree that this is a bit confusing, and the kernel code should
> probably be modified to not clobber the ioctl-provided effect on failure
> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>
> Dmitry, WDYT?
>
> The downside is that if changed, any new userspace code may unknowingly
> depend on the fixed non-clobbering behavior (though apparently they
> already do, as evidenced by Wine DInput, so might not be much of an
> argument...).
I don't think that's a problem.
Something that can be more problematic, is that a newer application
assumes the fixed non-clobbering behaviour, but runs on a kernel with
the old clobbering behaviour, could experience problems.
>
>> Here is a more elaborated description/discussion:
>> - This is a bug that is either the responsibility of the Linux kernel,
>> or of Wine (and possibly other applications that do the same thing as
>> described below):
>> It is caused by the following situation:
>> When uploading an effect, the specific kernel device driver
>> may return an error,
>> e.g. EINVAL for example when a periodic's effect period is set to zero.
>> This error will then be returned by "ioctl(*(This->fd),
>> EVIOCSFF, &This->effect)".
>> With or without error, one can find out that
>> /drivers/input/ff-core.c:input_ff_upload(...) is called,
>> which will set effect->id >= 0, if it was set to -1 (=> new
>> effect created) by the user.
>>
>> But in case of an error:
>> - Assume effect->id was set to -1 by the user:
>> The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>> the effect->id will also be set >= 0 (*).
>> The offending effect will not be saved in the
>> ff->effects[] array (***).
>> - Assume effect->id was set >= 0 by the user (and
>> ff->effects[effect->id] is a valid existing effect):
>> The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>> the effect->id will remain unchanged (**).
>> The offending effect will not overwrite the
>> ff->effects[effect->id] element (****).
>>
>> Is this (see *, **, *** and ****) desired behaviour?
>> - If yes:
>> Change the following in Wine's dinput/effect_linuxinput.c:90 :
>> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>> to :
>> int effectId_old = This->effect.id;
>> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>> This->effect.id = effectId_old;
>> - If no for *:
>> Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
>> has to be patched to revert "effect->id" back to its original value
>> set by the user,
>> which is only needed when the initial (by user) value of
>> "effect->id" was equal to -1.
>> - If no for **** (or maybe also ***):
>> ff->effects[effect->id] could be replaced by an 'empty'
>> effect (however this can get complex because the effect's type has to
>> remain unchanged)
>> This would be a change in the kernel code
>> /drivers/input/ff-core.c:input_ff_upload(...).
>> - If no for **:
>> I don't really know. Discussion is needed.
>>
>> - In my opinion, **, *** and **** are desired behaviour, while
>> * should leave the effect->id at -1.
>
> Right.
>
>> In that case, Wine's dinput implementation does not have
>> to be patched, and the kernel code only should apply a minor patch.
>
> Well, maybe better to change dinput anyway so that it can work properly
> with current kernel versions (assuming this gets changed in the future)?
> Not my call, though.
Yes, agreed.
I'll include the code (with effectId_old variable) and this discussion
when I submit my whole patchset for Wine's DInput libs to Wine-devel
mailing list. Right now, I don't know who I should personally mail to.
>
> --
> Anssi Hannula
Elias
^ permalink raw reply
* [PATCH] HID: sony: Fix work queue issues.
From: Frank Praznik @ 2014-02-19 18:09 UTC (permalink / raw)
To: linux-input; +Cc: stable, dh.herrmann, jkosina, Frank Praznik
Don't initialize force-feedback for devices that don't support it to avoid calls
to schedule_work() with an uninitialized work_struct.
Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
is used for the LEDs even when force-feedback is disabled.
Remove sony_destroy_ff() to avoid a compiler warning since it is no longer used.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
This is a bugfix for 3.14.
drivers/hid/hid-sony.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 1235405..2f19b15 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -42,6 +42,7 @@
#define DUALSHOCK4_CONTROLLER_BT BIT(6)
#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER | DUALSHOCK4_CONTROLLER_USB)
+#define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER_USB | DUALSHOCK4_CONTROLLER_USB)
#define MAX_LEDS 4
@@ -499,6 +500,7 @@ struct sony_sc {
__u8 right;
#endif
+ __u8 worker_initialized;
__u8 led_state[MAX_LEDS];
__u8 led_count;
};
@@ -993,22 +995,11 @@ static int sony_init_ff(struct hid_device *hdev)
return input_ff_create_memless(input_dev, NULL, sony_play_effect);
}
-static void sony_destroy_ff(struct hid_device *hdev)
-{
- struct sony_sc *sc = hid_get_drvdata(hdev);
-
- cancel_work_sync(&sc->state_worker);
-}
-
#else
static int sony_init_ff(struct hid_device *hdev)
{
return 0;
}
-
-static void sony_destroy_ff(struct hid_device *hdev)
-{
-}
#endif
static int sony_set_output_report(struct sony_sc *sc, int req_id, int req_size)
@@ -1077,6 +1068,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
ret = sixaxis_set_operational_usb(hdev);
+
+ sc->worker_initialized = 1;
INIT_WORK(&sc->state_worker, sixaxis_state_worker);
}
else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
@@ -1087,6 +1080,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (ret < 0)
goto err_stop;
+ sc->worker_initialized = 1;
INIT_WORK(&sc->state_worker, dualshock4_state_worker);
} else {
ret = 0;
@@ -1101,9 +1095,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto err_stop;
}
- ret = sony_init_ff(hdev);
- if (ret < 0)
- goto err_stop;
+ if (sc->quirks & SONY_FF_SUPPORT) {
+ ret = sony_init_ff(hdev);
+ if (ret < 0)
+ goto err_stop;
+ }
return 0;
err_stop:
@@ -1120,7 +1116,8 @@ static void sony_remove(struct hid_device *hdev)
if (sc->quirks & SONY_LED_SUPPORT)
sony_leds_remove(hdev);
- sony_destroy_ff(hdev);
+ if (sc->worker_initialized)
+ cancel_work_sync(&sc->state_worker);
hid_hw_stop(hdev);
}
--
1.8.5.3
^ permalink raw reply related
* Re: [PATCH] HID: sony: Fix work queue issues.
From: Greg KH @ 2014-02-19 18:27 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input, stable, dh.herrmann, jkosina
In-Reply-To: <1392833362-4218-1-git-send-email-frank.praznik@oh.rr.com>
On Wed, Feb 19, 2014 at 01:09:22PM -0500, Frank Praznik wrote:
> Don't initialize force-feedback for devices that don't support it to avoid calls
> to schedule_work() with an uninitialized work_struct.
>
> Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
> is used for the LEDs even when force-feedback is disabled.
>
> Remove sony_destroy_ff() to avoid a compiler warning since it is no longer used.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>
> This is a bugfix for 3.14.
>
> drivers/hid/hid-sony.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
^ permalink raw reply
* Re: [PATCH] HID: sony: Fix work queue issues.
From: David Herrmann @ 2014-02-19 18:41 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1392833362-4218-1-git-send-email-frank.praznik@oh.rr.com>
Hi Frank
On Wed, Feb 19, 2014 at 7:09 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Don't initialize force-feedback for devices that don't support it to avoid calls
> to schedule_work() with an uninitialized work_struct.
>
> Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
> is used for the LEDs even when force-feedback is disabled.
>
> Remove sony_destroy_ff() to avoid a compiler warning since it is no longer used.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
Sorry, my comment might have been misleading. You have to add the "Cc:
<stable@vger.kernel.org>" line underneath/above your "Signed-off-by".
This will send a notice to the stable guys once the patch is applied
in Linus' tree.
But that's also described in the file Greg posted.
Cheers
David
> ---
>
> This is a bugfix for 3.14.
>
> drivers/hid/hid-sony.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 1235405..2f19b15 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -42,6 +42,7 @@
> #define DUALSHOCK4_CONTROLLER_BT BIT(6)
>
> #define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER | DUALSHOCK4_CONTROLLER_USB)
> +#define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER_USB | DUALSHOCK4_CONTROLLER_USB)
>
> #define MAX_LEDS 4
>
> @@ -499,6 +500,7 @@ struct sony_sc {
> __u8 right;
> #endif
>
> + __u8 worker_initialized;
> __u8 led_state[MAX_LEDS];
> __u8 led_count;
> };
> @@ -993,22 +995,11 @@ static int sony_init_ff(struct hid_device *hdev)
> return input_ff_create_memless(input_dev, NULL, sony_play_effect);
> }
>
> -static void sony_destroy_ff(struct hid_device *hdev)
> -{
> - struct sony_sc *sc = hid_get_drvdata(hdev);
> -
> - cancel_work_sync(&sc->state_worker);
> -}
> -
> #else
> static int sony_init_ff(struct hid_device *hdev)
> {
> return 0;
> }
> -
> -static void sony_destroy_ff(struct hid_device *hdev)
> -{
> -}
> #endif
>
> static int sony_set_output_report(struct sony_sc *sc, int req_id, int req_size)
> @@ -1077,6 +1068,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> ret = sixaxis_set_operational_usb(hdev);
> +
> + sc->worker_initialized = 1;
> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> }
> else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> @@ -1087,6 +1080,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (ret < 0)
> goto err_stop;
>
> + sc->worker_initialized = 1;
> INIT_WORK(&sc->state_worker, dualshock4_state_worker);
> } else {
> ret = 0;
> @@ -1101,9 +1095,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto err_stop;
> }
>
> - ret = sony_init_ff(hdev);
> - if (ret < 0)
> - goto err_stop;
> + if (sc->quirks & SONY_FF_SUPPORT) {
> + ret = sony_init_ff(hdev);
> + if (ret < 0)
> + goto err_stop;
> + }
>
> return 0;
> err_stop:
> @@ -1120,7 +1116,8 @@ static void sony_remove(struct hid_device *hdev)
> if (sc->quirks & SONY_LED_SUPPORT)
> sony_leds_remove(hdev);
>
> - sony_destroy_ff(hdev);
> + if (sc->worker_initialized)
> + cancel_work_sync(&sc->state_worker);
>
> hid_hw_stop(hdev);
> }
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Elias Vanderstuyft @ 2014-02-19 18:54 UTC (permalink / raw)
To: Anssi Hannula
Cc: Michal Malý, Dmitry Torokhov, dtor, Johann Deneux,
linux-input
In-Reply-To: <52FFF276.1060704@iki.fi>
On Sun, Feb 16, 2014 at 12:04 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> 15.02.2014 22:32, Elias Vanderstuyft kirjoitti:
>> On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>>>
>>> However, from what I can see, you can achieve the exact same effect with
>>> these parameters (DInput deadband definition):
>>> sat_left = 0xFFFF
>>> sat_right = 0x0000
>>> coeff_left = 0x4000
>>> coeff_right = 0x0000
>>> center = 0x0000 (50% point)
>>> deadband (DInput/ours) = 0x8000 (50% total area, or 50% of
>>> center-to-end, so it reaches 25%..75%)
>>>
>>> On the left side, the effect is exactly the same as before with same
>>> parameters (sat 0xffff, coeff 0x4000, starts at 25% point).
>>>
>>> On the right side the parameters differ, but the end-result is the same,
>>> there is no effect at all:
>>> - In the first example, the right side is fully in deadband area,
>>> causing the effect to have zero effect.
>>> - In my variant with our definition, the right side has zero saturation,
>>> causing the effect to have zero effect.
>>>
>>>
>>> So the present definitions (and DInput definitions) can achieve the same
>>> effects as your proposed definitions, unless I'm missing something,
>>> making the change unneeded.
Today, I noticed something strange: an explicit saturation of zero in
DInput (or at least using the most recent driver for my Logitech MOMO
wheel), appears to be transformed to max saturation! (This is why I
chose to quote your "sat_right = 0x0000" part, however that's not the
point I want to make in this mail)
E.g:
pos_sat = 10000 -> Max saturation send to device
pos_sat = 5000 -> Half saturation send to device
...
pos_sat = 1 -> Zero saturation send to device (because
it appears to be rounded to zero)
pos_sat = 0 -> Max saturation send to device (?!)
Also interesting is that the FEdit tool (from MS DirectX SDK) sets all
'saturation' values to zero by default. This is not the case for the
'coefficient' values: they are set to maximum. The resulting default
conditional force generates a maxed out conditional effect on my MOMO
wheel.
The weird thing is, that MSDN does not explicitly say anything about
this: http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sdk.reference.dicondition%28v=vs.85%29.aspx
"dwPositiveSaturation:
Maximum force output on the positive side of the offset, in the
range from 0 through 10,000.
If the device does not support force saturation, the value of this
member is ignored.
"
So, my question is:
- Should Wine's translation layer convert a 0 dinput saturation to
max linux saturation?
- or Should the Linux FF drivers send a max saturation command
when the linux saturation equals zero?
- or Should we forget about this? (but that will result Wine's
behaviour to be different from Windows', at least for my MOMO wheel (I
don't have another capable (non-Logitech?) wheel to test it with))
Best regards,
Elias
^ permalink raw reply
* Re: [PATCH] HID: sony: Fix work queue issues.
From: Frank Praznik @ 2014-02-19 19:21 UTC (permalink / raw)
To: David Herrmann; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <CANq1E4TmfDRQLqBEw8JpS6da=96+ZENFy__YJjskPqVaA04jdw@mail.gmail.com>
On Wed, Feb 19, 2014 at 1:41 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi Frank
>
> On Wed, Feb 19, 2014 at 7:09 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
>> Don't initialize force-feedback for devices that don't support it to avoid calls
>> to schedule_work() with an uninitialized work_struct.
>>
>> Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
>> is used for the LEDs even when force-feedback is disabled.
>>
>> Remove sony_destroy_ff() to avoid a compiler warning since it is no longer used.
>>
>> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>
> Sorry, my comment might have been misleading. You have to add the "Cc:
> <stable@vger.kernel.org>" line underneath/above your "Signed-off-by".
> This will send a notice to the stable guys once the patch is applied
> in Linus' tree.
>
> But that's also described in the file Greg posted.
>
> Cheers
> David
>
Actually, does this even need to go into stable? The issue doesn't
exist below kernel 3.14 since the LED system wasn't added until commit
0a286ef278529f2bc4f4bb27c4cf99c05999c818 in December. Could this just
go into for-3.14/upstream-fixes and for-linus with the other pending
fixes?
^ permalink raw reply
* Re: ff-core effect id handling in case of a failed effect upload
From: Andrew Eikum @ 2014-02-19 19:32 UTC (permalink / raw)
To: Elias Vanderstuyft
Cc: Anssi Hannula, Dmitry Torokhov, linux-input, wine-devel
In-Reply-To: <CADbOyBTY8KtRyM6q7UMDAVieeR-X93qCoWO=ttizguKvouUkVw@mail.gmail.com>
On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote:
> I'll include the code (with effectId_old variable) and this discussion
> when I submit my whole patchset for Wine's DInput libs to Wine-devel
> mailing list. Right now, I don't know who I should personally mail to.
>
Patches ready to be included in Wine should go to
wine-patches@winehq.org. I've done a bit of work in dinput myself, so
you can feel free to send them to me and/or wine-devel first if you'd
like a review.
Andrew
^ permalink raw reply
* Problem with Logitech SpaceNavigator
From: Christian Ehrlicher @ 2014-02-19 19:38 UTC (permalink / raw)
To: linux-input
[-- Attachment #1: Type: text/plain, Size: 464 bytes --]
Hi,
I've a problem with some SpaceNavigator devices with a newer firmware
(4.31). The problem is that the fixup for the EV_REL/EV_ABS
(LG_RDESC_REL_ABS) is not applied for the new firmware because the
report_descriptor is different (see attachments). It looks like the
checked and modified bytes are no longer at offset 32/33 and 49/50 but
at 36/37 and 53/54.
Is my assumption correct and is it possible to get this into hid-lg.c ?
Thx,
Christian Ehrlicher
[-- Attachment #2: sn_401_report_descriptor.txt --]
[-- Type: text/plain, Size: 1022 bytes --]
00000000 05 01 09 08 a1 01 a1 00 85 01 16 0c fe 26 f4 01 |.............&..|
00000010 36 00 80 46 ff 7f 09 30 09 31 09 32 75 10 95 03 |6..F...0.1.2u...|
00000020 81 02 c0 a1 00 85 02 09 33 09 34 09 35 75 10 95 |........3.4.5u..|
00000030 03 81 02 c0 a1 02 85 03 05 01 05 09 19 01 29 02 |..............).|
00000040 15 00 25 01 35 00 45 01 75 01 95 02 81 02 95 06 |..%.5.E.u.......|
00000050 81 03 c0 a1 02 85 04 05 08 09 4b 15 00 25 01 95 |..........K..%..|
00000060 01 75 01 91 02 95 01 75 07 91 03 c0 06 00 ff 09 |.u.....u........|
00000070 01 a1 02 15 80 25 7f 75 08 09 3a a1 02 85 05 09 |.....%.u..:.....|
00000080 20 95 01 b1 02 c0 a1 02 85 06 09 21 95 01 b1 02 | ..........!....|
00000090 c0 a1 02 85 07 09 22 95 01 b1 02 c0 a1 02 85 08 |......".........|
000000a0 09 23 95 07 b1 02 c0 a1 02 85 09 09 24 95 07 b1 |.#..........$...|
000000b0 02 c0 a1 02 85 0a 09 25 95 07 b1 02 c0 a1 02 85 |.......%........|
000000c0 0b 09 26 95 01 b1 02 c0 c0 c0 |..&.......|
[-- Attachment #3: sn_431_report_descriptor.txt --]
[-- Type: text/plain, Size: 1100 bytes --]
00000000 05 01 09 08 a1 01 a1 00 85 01 16 a2 fe 26 5e 01 |.............&^.|
00000010 36 88 fa 46 78 05 55 0c 65 11 09 30 09 31 09 32 |6..Fx.U.e..0.1.2|
00000020 75 10 95 03 81 06 c0 a1 00 85 02 09 33 09 34 09 |u...........3.4.|
00000030 35 75 10 95 03 81 06 c0 a1 02 85 03 05 01 05 09 |5u..............|
00000040 19 01 29 02 15 00 25 01 35 00 45 01 75 01 95 02 |..)...%.5.E.u...|
00000050 81 02 95 0e 81 03 c0 a1 02 85 04 05 08 09 4b 15 |..............K.|
00000060 00 25 01 95 01 75 01 91 02 95 01 75 07 91 03 c0 |.%...u.....u....|
00000070 06 00 ff 09 01 a1 02 15 80 25 7f 75 08 09 3a a1 |.........%.u..:.|
00000080 02 85 05 09 20 95 01 b1 02 c0 a1 02 85 06 09 21 |.... ..........!|
00000090 95 01 b1 02 c0 a1 02 85 07 09 22 95 01 b1 02 c0 |..........".....|
000000a0 a1 02 85 08 09 23 95 07 b1 02 c0 a1 02 85 09 09 |.....#..........|
000000b0 24 95 07 b1 02 c0 a1 02 85 0a 09 25 95 07 b1 02 |$..........%....|
000000c0 c0 a1 02 85 0b 09 26 95 01 b1 02 c0 a1 02 85 13 |......&.........|
000000d0 09 2e 95 01 b1 02 c0 c0 c0 |.........|
^ permalink raw reply
* Re: ff-core effect id handling in case of a failed effect upload
From: Elias Vanderstuyft @ 2014-02-19 19:49 UTC (permalink / raw)
To: Andrew Eikum; +Cc: Anssi Hannula, wine-devel, Dmitry Torokhov, linux-input
In-Reply-To: <20140219193215.GM743@foghorn.codeweavers.com>
On Wed, Feb 19, 2014 at 8:32 PM, Andrew Eikum <aeikum@codeweavers.com> wrote:
> On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote:
>> I'll include the code (with effectId_old variable) and this discussion
>> when I submit my whole patchset for Wine's DInput libs to Wine-devel
>> mailing list. Right now, I don't know who I should personally mail to.
>>
>
> Patches ready to be included in Wine should go to
> wine-patches@winehq.org. I've done a bit of work in dinput myself, so
> you can feel free to send them to me and/or wine-devel first if you'd
> like a review.
<offtopic>
Alright, thanks Andrew!
I've also done some work on reverse engineering the FFE file format,
which is needed to complete some missing functionality
(http://wiki.winehq.org/ForceFeedbackSummerOfCode2005Summary) of the
following functions:
IDirectInputDevice8::WriteEffectToFile and
IDirectInputDevice8::EnumEffectsInFile.
Can I send it to you (in a new mail subject, of course) and cc wine-devel?
</offtopic>
>
> Andrew
Elias
^ permalink raw reply
* Re: ff-core effect id handling in case of a failed effect upload
From: Dmitry Torokhov @ 2014-02-19 21:05 UTC (permalink / raw)
To: Anssi Hannula; +Cc: Elias Vanderstuyft, linux-input, wine-devel
In-Reply-To: <5304E0A0.5070007@iki.fi>
On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
> (added Dmitry to CC)
>
> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
> > Hi,
> >
>
> Hi,
>
> > In the process of reviewing the Wine DInput translation layer, I
> > noticed an inconvenience (in the ff-core implementation?) that can
> > possibly lead to confusing problems to application developers (not
> > only for Wine), in short:
> > If a new (id==-1) effect was uploaded (look at
> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
> > ff-core will have assigned a positive number to the effect id. This
> > can be confusing because the dev->ff->effects[] array will not contain
> > an element at the index of that new effect id.
>
> I agree that this is a bit confusing, and the kernel code should
> probably be modified to not clobber the ioctl-provided effect on failure
> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>
> Dmitry, WDYT?
Yeah, it looks like we need to change evdev.c to read:
error = input_ff_upload(dev, &effect, file);
if (error)
return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
return 0;
Unfortunately applications should still expect changed effect ID for
quite some time.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: add i2c/smbus driver for elan touchpad
From: Benson Leung @ 2014-02-19 21:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel@vger.kernel.org, linux-input, agnescheng, phoenix,
Jeff.Chuang, Duson Lin
In-Reply-To: <CANLzEkv6YiY40QnSao7p=17SsZsnCPHT6zOMYNGCpbsa5d_OcA@mail.gmail.com>
On Thu, Feb 6, 2014 at 4:21 PM, Benson Leung <bleung@chromium.org> wrote:
> On Fri, Jan 10, 2014 at 12:30 PM, Benson Leung <bleung@chromium.org> wrote:
>> On Mon, Jan 6, 2014 at 7:08 PM, Duson Lin <dusonlin@emc.com.tw> wrote:
>>> This driver adds support for elan i2c/smbus touchpad found on some laptops PC
>>>
>>> Signed-off-by: Duson Lin <dusonlin@emc.com.tw>
>>
>> Reviewed-by: Benson Leung <bleung@chromium.org>
>>
>
> Hi Dmitry, This should be the most up to date version of this driver.
> Disregard the one I pinged from Dec 17, 2013 . Could you take another
> look at this?
Hi Dmitry,
Could you help take a look at Duson's latest elan_i2c driver submission?
Reviewed-by: Benson Leung <bleung@chromium.org>
Thanks,
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply
* Re: ff-core effect id handling in case of a failed effect upload
From: Elias Vanderstuyft @ 2014-02-19 22:14 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Anssi Hannula, linux-input, wine-devel
In-Reply-To: <20140219210523.GA13951@core.coreip.homeip.net>
On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
>> (added Dmitry to CC)
>>
>> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> > Hi,
>> >
>>
>> Hi,
>>
>> > In the process of reviewing the Wine DInput translation layer, I
>> > noticed an inconvenience (in the ff-core implementation?) that can
>> > possibly lead to confusing problems to application developers (not
>> > only for Wine), in short:
>> > If a new (id==-1) effect was uploaded (look at
>> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> > ff-core will have assigned a positive number to the effect id. This
>> > can be confusing because the dev->ff->effects[] array will not contain
>> > an element at the index of that new effect id.
>>
>> I agree that this is a bit confusing, and the kernel code should
>> probably be modified to not clobber the ioctl-provided effect on failure
>> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>>
>> Dmitry, WDYT?
>
> Yeah, it looks like we need to change evdev.c to read:
>
> error = input_ff_upload(dev, &effect, file);
> if (error)
> return error;
>
> if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
> return -EFAULT;
>
> return 0;
Alright, who will create the patch?
Do I may / have to do it?
>
> Unfortunately applications should still expect changed effect ID for
> quite some time.
>
> Thanks.
>
> --
> Dmitry
Best regards,
Elias
^ 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