* Re: [PATCH v3] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-18 6:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Xianglong Du, linux-input@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Rongjun Ying, Barry Song,
DL-SHA-WorkGroupLinux
In-Reply-To: <20140217191658.GA19223@core.coreip.homeip.net>
2014-02-18 3:16 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Fri, Feb 14, 2014 at 09:41:06PM +0800, Barry Song wrote:
>> >> Input: sirfsoc-onkey - implement open and close methods
>> >>
>> >> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >>
>> >> We can control whetehr device generates interrupts or not so let's
>> >> implement open and close methods of input device so that we do not do any
>> >> processing until there are users.
>> >>
>> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> Tested-by: Xianglong Du <Xianglong.Du@csr.com>
>>
>> Dmitry, will you push this one to your tree so that i can rebase others?
>
> I rebased and pushed everything out, please take a look to make sure it
> all looks good.
thanks, we have done a update to CSR local tree.
>
> Thanks.
>
> --
> Dmitry
-barry
^ permalink raw reply
* [PATCH] HID: cp2112: can't be used without gpio support
From: Jiri Kosina @ 2014-02-18 8:55 UTC (permalink / raw)
To: David Barksdale; +Cc: linux-input, fengguang.wu
Add Kconfig driver dependency on GPIOLIB.
Reported-by: fengguang.wu@intel.com
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/hid/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 5eaf0db..669cc19 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -177,7 +177,7 @@ config HID_PRODIKEYS
config HID_CP2112
tristate "Silicon Labs CP2112 HID USB-to-SMBus Bridge support"
- depends on USB_HID && I2C
+ depends on USB_HID && I2C && GPIOLIB
---help---
Support for Silicon Labs CP2112 HID USB to SMBus Master Bridge.
This is a HID device driver which registers as an i2c adapter
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH] HID: cp2112: fix incorrect error propagation in cp2112_xfer()
From: Jiri Kosina @ 2014-02-18 8:56 UTC (permalink / raw)
To: David Barksdale; +Cc: linux-input, fengguang.wu
Both cp2112_read_req() and cp2112_write_req() are returning negative
value in cases of error, but cp2112_xfer() is storing the return
value into unsigned size_t-typed 'count'.
Fix this by making 'count' signed type.
Reported-by: fengguang.wu@intel.com
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/hid/hid-cp2112.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 6d679f1..1025982 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -428,7 +428,7 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
struct hid_device *hdev = dev->hdev;
u8 buf[64];
__be16 word;
- size_t count;
+ ssize_t count;
size_t read_length = 0;
unsigned int retries;
int ret;
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* Re: linux-next: Tree for Feb 18 (hid/hid-cp2112.c)
From: Randy Dunlap @ 2014-02-18 16:34 UTC (permalink / raw)
To: Stephen Rothwell, linux-next
Cc: LKML, linux-input@vger.kernel.org, David Barksdale
In-Reply-To: <20140218160612.34d64514ade66fcb21a6cc0f@canb.auug.org.au>
On 02/17/14 21:06, Stephen Rothwell wrote:
> Hi all,
>
> If you see failures in building this tree due to missing declarations of
> k..alloc/free, then it may be caused by commit 2bd59d48ebfb ("cgroup:
> convert to kernfs"). Please send Tejun Heo <tj@kernel.org> a patch
> adding an inclusion of linux/slab.h to the appropriate file(s).
>
> This tree fails (more than usual) the powerpc allyesconfig build.
>
> Changes since 20140217:
>
on x86_64:
when GPIOLIB is not enabled.
Add
depends on GPIOLIB
to drivers/hid/Kconfig?
CC [M] drivers/hid/hid-cp2112.o
drivers/hid/hid-cp2112.c:162:19: error: field 'gc' has incomplete type
drivers/hid/hid-cp2112.c: In function 'cp2112_gpio_direction_input':
drivers/hid/hid-cp2112.c:171:30: warning: initialization from incompatible pointer type [enabled by default]
drivers/hid/hid-cp2112.c:171:30: warning: (near initialization for 'dev') [enabled by default]
drivers/hid/hid-cp2112.c: In function 'cp2112_gpio_set':
drivers/hid/hid-cp2112.c:200:30: warning: initialization from incompatible pointer type [enabled by default]
drivers/hid/hid-cp2112.c:200:30: warning: (near initialization for 'dev') [enabled by default]
drivers/hid/hid-cp2112.c: In function 'cp2112_gpio_get':
drivers/hid/hid-cp2112.c:218:30: warning: initialization from incompatible pointer type [enabled by default]
drivers/hid/hid-cp2112.c:218:30: warning: (near initialization for 'dev') [enabled by default]
drivers/hid/hid-cp2112.c: In function 'cp2112_gpio_direction_output':
drivers/hid/hid-cp2112.c:237:30: warning: initialization from incompatible pointer type [enabled by default]
drivers/hid/hid-cp2112.c:237:30: warning: (near initialization for 'dev') [enabled by default]
drivers/hid/hid-cp2112.c: In function 'cp2112_probe':
drivers/hid/hid-cp2112.c:944:2: error: implicit declaration of function 'gpiochip_add' [-Werror=implicit-function-declaration]
/local/lnx/next/linux-next-20140218/drivers/hid/hid-cp2112.c:962:2: error: implicit declaration of function 'gpiochip_remove' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
make[3]: *** [drivers/hid/hid-cp2112.o] Error 1
--
~Randy
^ permalink raw reply
* Re: [PATCH 03/11] Input: synaptics-rmi4 - do not update configuration in rmi_f01_probe()
From: Christopher Heiny @ 2014-02-18 21:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <20140217192321.GC19223@core.coreip.homeip.net>
On 02/17/2014 11:23 AM, Dmitry Torokhov wrote:
> On Fri, Feb 14, 2014 at 03:00:43PM -0800, Christopher Heiny wrote:
>> On 02/13/2014 01:54 PM, Dmitry Torokhov wrote:
>>> On Thu, Feb 13, 2014 at 11:23:44AM -0800, Christopher Heiny wrote:
>>>>> On 02/12/2014 09:27 PM, Dmitry Torokhov wrote:
>>>>>>> Do not write configuration data in probe(), we have config() for that.
>>>>>
>>>>> Then we should call config() in rmi_function_probe() to ensure that
>>>>> any platform data or device tree configuration settings get written
>>>>> to the device.
>>>
>>> Well, yes, we may elect to update device configuration in probe, but
>>> then we should not be doing that 2nd time in ->config(). We shoudl pick
>>> either one or another.
>>
>> But as the code currently stands, config() is only called when a
>> device reset is detected, not during initialization. So if there's
>> platform specific configuration data that needs to be written to a
>> function, it won't get written until after a device reset occurs,
>> which might never happen. So either we need to write that data (or
>> call config()) in each function's probe(), or in
>> rmi_function_probe().
>
> Ah, I missed the fact that we do not normally call ->config() unless
> device was reset. BTW, if device was reset, shouldn't we tear down
> everything and then reenumerate all functions?
That's only required if the reset is a result of the device being
reflashed. Since we're dropping support for user-space control of
reflash, and will instead use an in-driver reflash, we know which resets
are the result of a reflash and which aren't. The reflash code will do
the following sequence:
- tell the core to tear down the functions
- perform the reflash operation
- reset the device
- tell the core to re-enumerate the functions
Chris
^ permalink raw reply
* [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
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