* Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c
From: Dmitry Torokhov @ 2014-01-09 8:04 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1389230319-4737-1-git-send-email-cheiny@synaptics.com>
On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
> This is a trivial change to replace the sprintf loop with snprintf using
> up-to-date format capability.
Hmm, how about we do this instead:
Input: synaptics-rmi4 - clean up debug handling in rmi_i2c
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Kernel now has standard facility to format and print binary buffers, let's
use it. By doing so we no longer need to allocate memory for debug buffers
and we can let debugfs code go as well.
We have a new limitation however - output is limited to the first 64 bytes
of the buffer. However if we really need to capture larger messages dumping
them in dmesg is not right interface anyway.
Also clean up extra includes, while we are at it.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_i2c.c | 131 +++++-------------------------------------
1 file changed, 15 insertions(+), 116 deletions(-)
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index b33074c..c2ad1aa 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -8,16 +8,10 @@
*/
#include <linux/kernel.h>
-#include <linux/delay.h>
#include <linux/i2c.h>
-#include <linux/interrupt.h>
-#include <linux/kconfig.h>
-#include <linux/lockdep.h>
#include <linux/module.h>
-#include <linux/pm.h>
#include <linux/rmi.h>
#include <linux/slab.h>
-#include <linux/uaccess.h>
#include "rmi_driver.h"
#define BUFFER_SIZE_INCREMENT 32
@@ -31,11 +25,6 @@
*
* @tx_buf: Buffer used for transmitting data to the sensor over i2c.
* @tx_buf_size: Size of the buffer
- * @debug_buf: Buffer used for exposing buffer contents using dev_dbg
- * @debug_buf_size: Size of the debug buffer.
- *
- * @comms_debug: Latest data read/written for debugging I2C communications
- * @debugfs_comms: Debugfs file for debugging I2C communications
*/
struct rmi_i2c_data {
struct mutex page_mutex;
@@ -44,56 +33,8 @@ struct rmi_i2c_data {
u8 *tx_buf;
int tx_buf_size;
- u8 *debug_buf;
- int debug_buf_size;
-
- u32 comms_debug;
-#ifdef CONFIG_RMI4_DEBUG
- struct dentry *debugfs_comms;
-#endif
};
-#ifdef CONFIG_RMI4_DEBUG
-
-static int setup_debugfs(struct rmi_device *rmi_dev, struct rmi_i2c_data *data)
-{
- if (!rmi_dev->debugfs_root)
- return -ENODEV;
-
- data->debugfs_comms = debugfs_create_bool("comms_debug",
- (S_IRUGO | S_IWUGO), rmi_dev->debugfs_root,
- &data->comms_debug);
- if (!data->debugfs_comms || IS_ERR(data->debugfs_comms)) {
- dev_warn(&rmi_dev->dev,
- "Failed to create debugfs comms_debug.\n");
- data->debugfs_comms = NULL;
- }
-
- return 0;
-}
-
-static void teardown_debugfs(struct rmi_i2c_data *data)
-{
- if (data->debugfs_comms)
- debugfs_remove(data->debugfs_comms);
-}
-
-#else
-
-static inline int setup_debugfs(struct rmi_device *rmi_dev,
- struct rmi_i2c_data *data)
-{
- return 0;
-}
-
-static inline void teardown_debugfs(struct rmi_i2c_data *data)
-{
-}
-
-#endif
-
-#define COMMS_DEBUG(data) (IS_ENABLED(CONFIG_RMI4_DEBUG) && data->comms_debug)
-
#define RMI_PAGE_SELECT_REGISTER 0xff
#define RMI_I2C_PAGE(addr) (((addr) >> 8) & 0xff)
@@ -120,8 +61,7 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
int retval;
- if (COMMS_DEBUG(data))
- dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
+ dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
txbuf[0], txbuf[1]);
xport->info.tx_count++;
xport->info.tx_bytes += sizeof(txbuf);
@@ -136,34 +76,6 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
return 0;
}
-static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
- const u8 *buf, const int len) {
- int i;
- int n = 0;
- char *temp;
- int dbg_size = 3 * len + 1;
-
- if (!data->debug_buf || data->debug_buf_size < dbg_size) {
- if (data->debug_buf)
- devm_kfree(dev, data->debug_buf);
- data->debug_buf_size = dbg_size + BUFFER_SIZE_INCREMENT;
- data->debug_buf = devm_kzalloc(dev, data->debug_buf_size,
- GFP_KERNEL);
- if (!data->debug_buf) {
- data->debug_buf_size = 0;
- return -ENOMEM;
- }
- }
- temp = data->debug_buf;
-
- for (i = 0; i < len; i++) {
- n = sprintf(temp, " %02x", buf[i]);
- temp += n;
- }
-
- return 0;
-}
-
static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
const void *buf, const int len)
{
@@ -195,12 +107,8 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
goto exit;
}
- if (COMMS_DEBUG(data)) {
- int rc = copy_to_debug_buf(&client->dev, data, (u8 *) buf, len);
- if (!rc)
- dev_dbg(&client->dev, "writes %d bytes at %#06x:%s\n",
- len, addr, data->debug_buf);
- }
+ dev_dbg(&client->dev,
+ "writes %d bytes at %#06x: %*ph\n", len, addr, len, buf);
xport->info.tx_count++;
xport->info.tx_bytes += tx_size;
@@ -232,8 +140,7 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
goto exit;
}
- if (COMMS_DEBUG(data))
- dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
+ dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
xport->info.tx_count++;
xport->info.tx_bytes += sizeof(txbuf);
@@ -244,18 +151,16 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
goto exit;
}
- retval = i2c_master_recv(client, (u8 *) buf, len);
+ retval = i2c_master_recv(client, buf, len);
xport->info.rx_count++;
xport->info.rx_bytes += len;
if (retval < 0)
xport->info.rx_errs++;
- else if (COMMS_DEBUG(data)) {
- int rc = copy_to_debug_buf(&client->dev, data, (u8 *) buf, len);
- if (!rc)
- dev_dbg(&client->dev, "read %d bytes at %#06x:%s\n",
- len, addr, data->debug_buf);
- }
+ else
+ dev_dbg(&client->dev,
+ "read %d bytes at %#06x: %*ph\n",
+ len, addr, len, buf);
exit:
mutex_unlock(&data->page_mutex);
@@ -265,9 +170,9 @@ exit:
static int rmi_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
+ const struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
struct rmi_transport_dev *xport;
struct rmi_i2c_data *data;
- struct rmi_device_platform_data *pdata = client->dev.platform_data;
int retval;
if (!pdata) {
@@ -318,7 +223,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
mutex_init(&data->page_mutex);
- /* Setting the page to zero will (a) make sure the PSR is in a
+ /*
+ * Setting the page to zero will (a) make sure the PSR is in a
* known state, and (b) make sure we can talk to the device.
*/
retval = rmi_set_page(xport, 0);
@@ -335,11 +241,6 @@ static int rmi_i2c_probe(struct i2c_client *client,
}
i2c_set_clientdata(client, xport);
- retval = setup_debugfs(xport->rmi_dev, data);
- if (retval < 0)
- dev_warn(&client->dev, "Failed to setup debugfs. Code: %d.\n",
- retval);
-
dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
client->addr);
return 0;
@@ -353,14 +254,12 @@ err_gpio:
static int rmi_i2c_remove(struct i2c_client *client)
{
struct rmi_transport_dev *xport = i2c_get_clientdata(client);
- struct rmi_device_platform_data *pd = client->dev.platform_data;
-
- teardown_debugfs(xport->data);
+ struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
rmi_unregister_transport_device(xport);
- if (pd->gpio_config)
- pd->gpio_config(&pd->gpio_data, false);
+ if (pdata->gpio_config)
+ pdata->gpio_config(&pdata->gpio_data, false);
return 0;
}
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Dmitry Torokhov @ 2014-01-09 6:52 UTC (permalink / raw)
To: Aniroop Mathur
Cc: linux-input, cpgs, a.anurag, naveen.sel, aniroop.mathur,
vikas.kala, narendra.m1, poorva.s
In-Reply-To: <1389243476-11196-1-git-send-email-a.mathur@samsung.com>
Hi Aniroop,
On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
> This patch allows user(driver) to set sysfs node name of input
> devices. To set sysfs node name, user(driver) just needs to set
> node_name_unique variable. If node_name_unique is not set, default
> name is given(as before). So, this patch is completely
> backward-compatible.
>
> Sysfs Input node name format is: input_<name>
> Sysfs Event node name format is: event_<name>
>
> This "name" is given by user and automatically, prefix(input and
> event) is added by input core.
>
> This name must be unique among all input devices and driver(user) has
> the responsibility to ensure it. If same name is used again for other
> input device, registration of that input device will fail because two
> input devices cannot have same name.
>
> Advantages of this patch are:
>
> 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
> Upper-Layer do not need to search input/event number corresponding to
> each input device in /dev/input/... This searching in /dev/input/ was
> taking too much time. (Especially in mobile devices, where there are
> many input devices (many sensors, touchscreen, etc), it reduces a lot
> of booting time)
I am sorry, how much time does it take to scan a directory of what, 20
devices? If it such a factor have udev create nodes that are easier for
you to locate, similarly how we already create nodes by-id and by-path.
For example you can encode major:minor in device name.
>
> 2. Improves Readabilty of input and event sysfs node paths because
> names are used instead of numbers.
I do not see why it is that important. If one wants overview
/proc/bus/input/devices gives nice picture.
>
> 3. Removes Input Devices Dependency. If one input device probe fails,
> other input devices still work. Before this patch, if one input
> device probe fails before input_register_device, then input number of
> other input devices changes and due to this permission settings are
> disturbed and hence HAL or upper layer cannot open the required sysfs
> node because permission denied error comes.
I have only one suggestion here: fix your userspace so that does not
depend on device initialization ordering.
IOW I am totally unconvinced that this facility is needed.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] ims-pcu: Add commands supported by the new version of the FW
From: Dmitry Torokhov @ 2014-01-09 6:43 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: linux-input, linux-kernel
In-Reply-To: <CAHQ1cqFaXCXw8UTwRVw72mmf7e_t3QjWNMshccEchsmR-2DVbw@mail.gmail.com>
On Wed, Jan 08, 2014 at 09:57:20PM -0800, Andrey Smirnov wrote:
> On Mon, Jan 6, 2014 at 11:14 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Andrey, were you able to test the patch?
>
> Hi Dmitry,
>
> I tested the patch and with exception of a trivial to fix typo/bug it
> worked fine.
>
> I see that you moved the OFN attributes to a dedicated directory,
> which I personally like better, but I am not sure if IMS would not
> want to push back on it since they might have started to use the API
> already. I'll send a new version to Chris so that he can do some
> additional testing and see if they can incorporate that naming change.
> I'll e-mail you v3 of the patch as soon as that matter becomes more
> clear.
Andrey,
I am pretty sure that the new sysfs layout with OFN group split makes
most sense. Luckily this is brand new functionality so IMS should be
able to adjust their userspace to accommodate it.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: sony: Add force feedback for the Dualshock 4
From: simon @ 2014-01-09 6:42 UTC (permalink / raw)
Cc: Frank Praznik, linux-input
In-Reply-To: <e14a0771e48b8c1d72ee9182eb7d2593.squirrel@mungewell.org>
>
>> However I am seeing some weird behaviour in whether FF/LED actually
>> functions. It seems that in a 'complete' kernel installation the driver
>> does not present FF or LED.
>>
Found the problem (or at least a solution), there is no entry in 'hid-core.c'
--
static const struct hid_device_id hid_have_special_driver[] = {
...
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY,
USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
--
Adding this makes it work for me, although I don't know why the insmod
trick was working....
Simon
^ permalink raw reply
* Re: [PATCH v2] ims-pcu: Add commands supported by the new version of the FW
From: Andrey Smirnov @ 2014-01-09 5:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20140107071436.GB20074@core.coreip.homeip.net>
On Mon, Jan 6, 2014 at 11:14 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Andrey, were you able to test the patch?
Hi Dmitry,
I tested the patch and with exception of a trivial to fix typo/bug it
worked fine.
I see that you moved the OFN attributes to a dedicated directory,
which I personally like better, but I am not sure if IMS would not
want to push back on it since they might have started to use the API
already. I'll send a new version to Chris so that he can do some
additional testing and see if they can incorporate that naming change.
I'll e-mail you v3 of the patch as soon as that matter becomes more
clear.
Thanks,
Andrey
^ permalink raw reply
* [PATCH] Introduce Naming Convention in Input Subsystem
From: Aniroop Mathur @ 2014-01-09 4:57 UTC (permalink / raw)
To: linux-input, dmitry.torokhov, dtor
Cc: cpgs, a.mathur, a.anurag, naveen.sel, aniroop.mathur, vikas.kala,
narendra.m1, poorva.s
This patch allows user(driver) to set sysfs node name of input devices.
To set sysfs node name, user(driver) just needs to set node_name_unique variable.
If node_name_unique is not set, default name is given(as before).
So, this patch is completely backward-compatible.
Sysfs Input node name format is: input_<name>
Sysfs Event node name format is: event_<name>
This "name" is given by user and automatically, prefix(input and event) is added by input core.
This name must be unique among all input devices and driver(user) has the responsibility to ensure it.
If same name is used again for other input device, registration of that input device will fail because
two input devices cannot have same name.
Advantages of this patch are:
1. Reduces Booting Time of HAL/Upper-Layer because now HAL or Upper-Layer do not need to search input/event number
corresponding to each input device in /dev/input/...
This searching in /dev/input/ was taking too much time.
(Especially in mobile devices, where there are many input devices (many sensors, touchscreen, etc),
it reduces a lot of booting time)
2. Improves Readabilty of input and event sysfs node paths because names are used instead of numbers.
3. Removes Input Devices Dependency. If one input device probe fails, other input devices still work.
Before this patch, if one input device probe fails before input_register_device, then
input number of other input devices changes and due to this permission settings are disturbed
and hence HAL or upper layer cannot open the required sysfs node because permission denied error comes.
This patch is applicable upto kernel version 3.11
Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
drivers/input/evdev.c | 11 ++++++++++-
drivers/input/input.c | 12 +++++++++++-
include/linux/input.h | 4 ++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index b6ded17..b6a5848 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -1131,7 +1131,16 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
/* Normalize device number if it falls into legacy range */
if (dev_no < EVDEV_MINOR_BASE + EVDEV_MINORS)
dev_no -= EVDEV_MINOR_BASE;
- dev_set_name(&evdev->dev, "event%d", dev_no);
+
+ /*
+ * As per user choice (driver),
+ * name of sysfs node is set, as mentioned in node_name_unique variable.
+ * If node_name_unique is not set, default name is given.
+ */
+ if (dev->node_name_unique)
+ dev_set_name(&evdev->dev, "event_%s", dev->node_name_unique);
+ else
+ dev_set_name(&evdev->dev, "event%d", dev_no);
evdev->handle.dev = input_get_device(dev);
evdev->handle.name = dev_name(&evdev->dev);
diff --git a/drivers/input/input.c b/drivers/input/input.c
index c044699..c8126b3 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2073,7 +2073,17 @@ int input_register_device(struct input_dev *dev)
if (!dev->setkeycode)
dev->setkeycode = input_default_setkeycode;
- dev_set_name(&dev->dev, "input%ld",
+ /*
+ * As per user choice (driver),
+ * name of sysfs node is set, as mentioned in node_name_unique variable.
+ * If node_name_unique is not set, default name is given.
+ */
+ if (dev->node_name_unique) {
+ atomic_inc_return(&input_no);
+ dev_set_name(&dev->dev, "input_%s",
+ dev->node_name_unique);
+ } else
+ dev_set_name(&dev->dev, "input%ld",
(unsigned long) atomic_inc_return(&input_no) - 1);
error = device_add(&dev->dev);
diff --git a/include/linux/input.h b/include/linux/input.h
index 82ce323..fe44643 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -43,6 +43,9 @@ struct input_value {
* @uniq: unique identification code for the device (if device has it)
* @id: id of the device (struct input_id)
* @propbit: bitmap of device properties and quirks
+ * @node_name_unique: name of input and event sysfs device node (char *).
+ * This name must be unique among all input devices and driver(user)
+ * has the responsibility to ensure it (if using).
* @evbit: bitmap of types of events supported by the device (EV_KEY,
* EV_REL, etc.)
* @keybit: bitmap of keys/buttons this device has
@@ -123,6 +126,7 @@ struct input_dev {
const char *phys;
const char *uniq;
struct input_id id;
+ char *node_name_unique;
unsigned long propbit[BITS_TO_LONGS(INPUT_PROP_CNT)];
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 1/2] HID: sony: Add force feedback for the Dualshock 4
From: simon @ 2014-01-09 3:46 UTC (permalink / raw)
Cc: Frank Praznik, linux-input
In-Reply-To: <c6470a2c8d6a5ad2e6d3c023ae49e7ef.squirrel@mungewell.org>
> However I am seeing some weird behaviour in whether FF/LED actually
> functions. It seems that in a 'complete' kernel installation the driver
> does not present FF or LED.
>
> But if I use a stock 3.13rc6 kernel, load the 'hid-sony' module from that
> and then remove and install the patched one it does work.
> --
> $ sudo modprobe hid-sony (stock)
> $ sudo rmmod hid-sony
> $ sudo insmod hid-sony.ko (patched)
> --
I note that using the above modprobe/rmmod/insmod trick the message log
shows additional lines
--
Jan 8 20:12:29 atom kernel: [ 144.840112] input: Sony Computer
Entertainment Wireless Controller as
/devices/pci0000:00/0000:00:1d.1/usb3/3-1/3-1:1.0/input/input9
Jan 8 20:12:29 atom kernel: [ 144.841044] sony 0003:054C:05C4.0001:
input,hidraw0: USB HID v1.11 Gamepad [Sony Computer Entertainment Wireless
Controller] on usb-0000:00:1d.1-1/input0
Jan 8 20:12:29 atom kernel: [ 144.841560] usbcore: registered new
interface driver usbhid
Jan 8 20:12:29 atom kernel: [ 144.841563] usbhid: USB HID core driver
--
and the Sony driver is being used
--
root@atom:/sys/bus/usb/devices/3-1:1.0# ls -al 0003\:054C\:05C4.0001/
total 0
drwxr-xr-x 5 root root 0 Jan 8 20:12 .
drwxr-xr-x 7 root root 0 Jan 8 20:12 ..
lrwxrwxrwx 1 root root 0 Jan 8 20:13 driver ->
../../../../../../../bus/hid/drivers/sony
--
Whereas with the fully patched kernel it is not
--
root@atom:/sys/bus/usb/devices/3-1:1.0# ls -al 0003\:054C\:05C4.0001/
total 0
drwxr-xr-x 4 root root 0 Jan 8 20:20 .
drwxr-xr-x 7 root root 0 Jan 8 20:20 ..
lrwxrwxrwx 1 root root 0 Jan 8 20:21 driver ->
../../../../../../../bus/hid/drivers/hid-generic
--
Further more my DS3-SixAxis appears to work OK with the full kernel, where
the DS4 does not.... any suggestions?
Simon
^ permalink raw reply
* [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c
From: Christopher Heiny @ 2014-01-09 1:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
This is a trivial change to replace the sprintf loop with snprintf using
up-to-date format capability.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_i2c.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index d94114b..8a93dad 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -138,9 +138,6 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
const u8 *buf, const int len) {
- int i;
- int n = 0;
- char *temp;
int dbg_size = 3 * len + 1;
if (!data->debug_buf || data->debug_buf_size < dbg_size) {
@@ -154,12 +151,8 @@ static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
return -ENOMEM;
}
}
- temp = data->debug_buf;
- for (i = 0; i < len; i++) {
- n = sprintf(temp, " %02x", buf[i]);
- temp += n;
- }
+ snprintf(data->debug_buf, data->debug_buf_size, "%*ph", len, buf);
return 0;
}
^ permalink raw reply related
* Re: [PATCH] input: synaptics-rmi4 - cleanup rmi_i2c_probe()
From: Christopher Heiny @ 2014-01-08 23:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <20140108224625.GB27279@core.coreip.homeip.net>
On 01/08/2014 02:46 PM, Dmitry Torokhov wrote:
> On Wed, Jan 08, 2014 at 02:11:32PM -0800, Christopher Heiny wrote:
>> Moves i2c_check_functionality to occur before the gpio_config() call. This
>> can catch some issues that would otherwise result in mysterious problems
>> in gpio_config().
>>
>> Reduces debugging output; updates remaining output to be more accurate.
>
> What kind of gpio config is there? Did not we add proper configuration
> of attn_gpio to the core? Do you really need these callbacks? They will
> hurt you when you will try to move to devicetree-based setups.
This is a gpio_config() does the platform specific gpio setup (power
configuration, voltage levels, hardware reset lines, and so on). This
is used on every production platform using the driver that I know of,
and the setup is different on every platform (sometimes even between
revs of that platform). We'd go crazy trying to handle that in
rmi_i2c.c or rmi_driver.c, so it's pushed to the platform.
And yeah, I figure it'll hurt when we move to devicetree. Right now
we're just trying to get the current state of the code in the
synaptics-rmi4 branch from "horribly broken" to "it lives!". We're
willing to put off that devicetree pain in order to get the code on its
feet.
^ permalink raw reply
* Re: [PATCH] input: synaptics-rmi4 - cleanup rmi_i2c_probe()
From: Dmitry Torokhov @ 2014-01-08 22:46 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1389219092-32042-1-git-send-email-cheiny@synaptics.com>
On Wed, Jan 08, 2014 at 02:11:32PM -0800, Christopher Heiny wrote:
> Moves i2c_check_functionality to occur before the gpio_config() call. This
> can catch some issues that would otherwise result in mysterious problems
> in gpio_config().
>
> Reduces debugging output; updates remaining output to be more accurate.
What kind of gpio config is there? Did not we add proper configuration
of attn_gpio to the core? Do you really need these callbacks? They will
hurt you when you will try to move to devicetree-based setups.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c
From: Dmitry Torokhov @ 2014-01-08 22:44 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1389217075-28551-1-git-send-email-cheiny@synaptics.com>
On Wed, Jan 08, 2014 at 01:37:55PM -0800, Christopher Heiny wrote:
> This is a trivial change to use snprintf rather than sprintf while building
> the diagnostic output buffer.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
>
> drivers/input/rmi4/rmi_i2c.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index b33074c..43b0e53 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -157,7 +157,7 @@ static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
> temp = data->debug_buf;
>
> for (i = 0; i < len; i++) {
> - n = sprintf(temp, " %02x", buf[i]);
> + n = snprintf(temp, 3, " %02x", buf[i]);
> temp += n;
> }
>
I do not think it is right, as if there is truncation you'd move too
far. In any case please replace the while loop with:
snprintf(data->debug_buf, data->debug_buf_size, "%*ph", len, buf);
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 5/5] HID: logitech-dj: add .request callback
From: Benjamin Tissoires @ 2014-01-08 22:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, linux-input, linux-kernel
In-Reply-To: <1389219529-29671-1-git-send-email-benjamin.tissoires@redhat.com>
From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
The .request callback allows to send data from the dj device driver to the
device. Because of the DJ protocol, we only authorize HID++ communication
through this request.
The device index has to be overwritten by the receiver. All communication pass
through it, and the receiver is the only one to know which dj device has which
device index.
Furthermore, this allows to use the same calls from the driver point of view:
if a device is connected through a DJ interface, the receiver will overwrite
the device index, if it is connected through another bus (like bluetooth),
then the transport layer will not change the report, and it will be correctly
forwarded to the device.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 3444feb..4335d21 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -624,6 +624,22 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
return 0;
}
+static void logi_dj_ll_request(struct hid_device *hid, struct hid_report *rep,
+ int reqtype)
+{
+ struct dj_device *djdev = hid->driver_data;
+ struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
+
+ if ((rep->id != REPORT_ID_HIDPP_LONG) &&
+ (rep->id != REPORT_ID_HIDPP_SHORT))
+ /* prevent forwarding of non acceptable reports */
+ return;
+
+ hid_set_field(rep->field[0], 0, djdev->device_index);
+
+ hid_hw_request(djrcv_dev->hdev, rep, reqtype);
+}
+
static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
{
memcpy(rdesc + *rsize, data, size);
@@ -759,6 +775,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
.stop = logi_dj_ll_stop,
.open = logi_dj_ll_open,
.close = logi_dj_ll_close,
+ .request = logi_dj_ll_request,
.hidinput_input_event = logi_dj_ll_input_event,
};
--
1.8.4.2
^ permalink raw reply related
* [PATCH 4/5] HID: logitech-dj: forward incoming HID++ reports to the correct dj device
From: Benjamin Tissoires @ 2014-01-08 22:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, linux-input, linux-kernel
In-Reply-To: <1389219529-29671-1-git-send-email-benjamin.tissoires@redhat.com>
From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
HID++ is a Logitech-specific protocol for communicating with HID
devices. DJ devices implement HID++, and so we can add the HID++
collection in the report descriptor and forward the incoming
reports from the receiver to the appropriate DJ device.
Signed-off-by: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 99 ++++++++++++++++++++++++++++++++++++++++---
drivers/hid/hid-logitech-dj.h | 6 +++
2 files changed, 99 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index a4b3cee..3444feb 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -152,6 +152,57 @@ static const char media_descriptor[] = {
0xc0, /* EndCollection */
}; /* */
+/* HIDPP descriptor */
+static const char hidpp_descriptor[] = {
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x09, 0x01, /* Usage (Vendor Usage 1) */
+ 0xa1, 0x01, /* Collection (Application) */
+ 0x85, 0x10, /* Report ID (16) */
+ 0x75, 0x08, /* Report Size (8) */
+ 0x95, 0x06, /* Report Count (6) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x26, 0xff, 0x00, /* Logical Maximum (255) */
+ 0x09, 0x01, /* Usage (Vendor Usage 1) */
+ 0x81, 0x00, /* Input (Data,Arr,Abs) */
+ 0x09, 0x01, /* Usage (Vendor Usage 1) */
+ 0x91, 0x00, /* Output (Data,Arr,Abs) */
+ 0xc0, /* End Collection */
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x09, 0x02, /* Usage (Vendor Usage 2) */
+ 0xa1, 0x01, /* Collection (Application) */
+ 0x85, 0x11, /* Report ID (17) */
+ 0x75, 0x08, /* Report Size (8) */
+ 0x95, 0x13, /* Report Count (19) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x26, 0xff, 0x00, /* Logical Maximum (255) */
+ 0x09, 0x02, /* Usage (Vendor Usage 2) */
+ 0x81, 0x00, /* Input (Data,Arr,Abs) */
+ 0x09, 0x02, /* Usage (Vendor Usage 2) */
+ 0x91, 0x00, /* Output (Data,Arr,Abs) */
+ 0xc0, /* End Collection */
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x09, 0x04, /* Usage (Vendor Usage 0x04) */
+ 0xa1, 0x01, /* Collection (Application) */
+ 0x85, 0x20, /* Report ID (32) */
+ 0x75, 0x08, /* Report Size (8) */
+ 0x95, 0x0e, /* Report Count (14) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x26, 0xff, 0x00, /* Logical Maximum (255) */
+ 0x09, 0x41, /* Usage (Vendor Usage 0x41) */
+ 0x81, 0x00, /* Input (Data,Arr,Abs) */
+ 0x09, 0x41, /* Usage (Vendor Usage 0x41) */
+ 0x91, 0x00, /* Output (Data,Arr,Abs) */
+ 0x85, 0x21, /* Report ID (33) */
+ 0x95, 0x1f, /* Report Count (31) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x26, 0xff, 0x00, /* Logical Maximum (255) */
+ 0x09, 0x42, /* Usage (Vendor Usage 0x42) */
+ 0x81, 0x00, /* Input (Data,Arr,Abs) */
+ 0x09, 0x42, /* Usage (Vendor Usage 0x42) */
+ 0x91, 0x00, /* Output (Data,Arr,Abs) */
+ 0xc0, /* End Collection */
+};
+
/* Maximum size of all defined hid reports in bytes (including report id) */
#define MAX_REPORT_SIZE 8
@@ -161,7 +212,8 @@ static const char media_descriptor[] = {
sizeof(mse_descriptor) + \
sizeof(consumer_descriptor) + \
sizeof(syscontrol_descriptor) + \
- sizeof(media_descriptor))
+ sizeof(media_descriptor) + \
+ sizeof(hidpp_descriptor))
/* Number of possible hid report types that can be created by this driver.
*
@@ -456,6 +508,25 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
}
}
+static void logi_dj_recv_forward_hidpp(struct dj_receiver_dev *djrcv_dev,
+ u8 *data, int size)
+{
+ /* We are called from atomic context (tasklet && djrcv->lock held) */
+
+ struct dj_device *dj_dev = NULL;
+ u8 device_index = data[1];
+
+ if ((device_index < DJ_DEVICE_INDEX_MIN) ||
+ (device_index > DJ_DEVICE_INDEX_MAX))
+ return;
+
+ dj_dev = djrcv_dev->paired_dj_devices[device_index];
+
+ if (!dj_dev)
+ return;
+
+ hid_input_report(dj_dev->hdev, HID_INPUT_REPORT, data, size, 1);
+}
static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
struct dj_report *dj_report)
@@ -610,6 +681,8 @@ static int logi_dj_ll_parse(struct hid_device *hid)
__func__, djdev->reports_supported);
}
+ rdcat(rdesc, &rsize, hidpp_descriptor, sizeof(hidpp_descriptor));
+
retval = hid_parse_report(hid, rdesc, rsize);
kfree(rdesc);
@@ -701,12 +774,12 @@ static int logi_dj_raw_event(struct hid_device *hdev,
dbg_hid("%s, size:%d\n", __func__, size);
- /* Here we receive all data coming from iface 2, there are 4 cases:
+ /* Here we receive all data coming from iface 2, there are 5 cases:
*
* 1) Data should continue its normal processing i.e. data does not
- * come from the DJ collection, in which case we do nothing and
- * return 0, so hid-core can continue normal processing (will forward
- * to associated hidraw device)
+ * come from the DJ or the HID++ collection, in which case we do nothing
+ * and return 0, so hid-core can continue normal processing (will
+ * forward to associated hidraw device)
*
* 2) Data is from DJ collection, and is intended for this driver i. e.
* data contains arrival, departure, etc notifications, in which case
@@ -723,10 +796,17 @@ static int logi_dj_raw_event(struct hid_device *hdev,
* a paired DJ device in which case we forward it to the correct hid
* device (via hid_input_report() ) and return 1 so hid-core does not do
* anything else with it.
+ *
+ * 5) Data is from the HID++ collection, in this case, we forward the
+ * data to the corresponding child dj device and return 0 to hid-core
+ * so he data also goes to the hidraw device of the receiver. This
+ * allows a user space application to implement the full HID++ routing
+ * via the receiver.
*/
spin_lock_irqsave(&djrcv_dev->lock, flags);
- if (dj_report->report_id == REPORT_ID_DJ_SHORT) {
+ switch (data[0]) {
+ case REPORT_ID_DJ_SHORT:
switch (dj_report->report_type) {
case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
@@ -742,6 +822,13 @@ static int logi_dj_raw_event(struct hid_device *hdev,
logi_dj_recv_forward_report(djrcv_dev, dj_report);
}
report_processed = true;
+ break;
+ case REPORT_ID_HIDPP_SHORT:
+ /* intentional fallthrough */
+ case REPORT_ID_HIDPP_LONG:
+ logi_dj_recv_forward_hidpp(djrcv_dev, data, size);
+ report_processed = false;
+ break;
}
spin_unlock_irqrestore(&djrcv_dev->lock, flags);
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 2e52167..a805d44 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -36,6 +36,12 @@
#define REPORT_ID_DJ_SHORT 0x20
#define REPORT_ID_DJ_LONG 0x21
+#define REPORT_ID_HIDPP_SHORT 0x10
+#define REPORT_ID_HIDPP_LONG 0x11
+
+#define HIDPP_REPORT_SHORT_LENGTH 7
+#define HIDPP_REPORT_LONG_LENGTH 20
+
#define REPORT_TYPE_RFREPORT_FIRST 0x01
#define REPORT_TYPE_RFREPORT_LAST 0x1F
--
1.8.4.2
^ permalink raw reply related
* [PATCH 0/5] HID logitech DJ fixes and preps for enabling extended features
From: Benjamin Tissoires @ 2014-01-08 22:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, linux-input, linux-kernel
Hi Jiri,
Well, this work is _not_ for 3.14 (except maybe patch 1), especially since it is
missing the biggest part where we enable the capabilities of Logitech devices.
Long story short:
This work is based on the work I did back in Summer 2011. I worked at Logitech
for a few weeks to show up a demo of a driver for the Logitech Wireless Touchpad.
At that time, a first draft has been done, but due to a lack of resources, noone
upstreamed it.
Since then, the code marinated a little at Logitech and in the ChromeOS kernel
tree, but nobody tried to push it upstream. So here I am, trying to push this stuff
upstream.
I can not send the full series right now because I am lacking most of the
testing hardware (I mean I only have the oldest Wireless Touchpad).
Hopefully, I should receive some other soon, and I'll be able to send the second
part then.
Now, let me roughly explain the patches:
- patch 1 can be applied right now I think, but it's entirely up to you Jiri.
This patch should fix the missing notifications with some USB 3.0 boards.
- patches 2 to 5 allows to forward in both direction the proprietary protocol
used by Logitech (HID++ [1]) between the driver and the hardware.
- later patches will introduce a transport layer for HID++ and also a driver
to support full multitouch on various Logitech touchpads.
Nestor, Andrew, feel free to add your "Signed-off-by" whereever you want, I lost
a little bit the track of who added what.
Cheers,
Benjamin
[1] HID++: Documentation is provided at
https://drive.google.com/a/logitech.com/?tab=mo#folders/0BxbRzx7vEV7eWmgwazJ3NUFfQ28
Benjamin Tisssoires (5):
HID: logitech-dj: Fix USB 3.0 issue
HID: core: do not scan reports if the group is already set
HID: logitech-dj: rely on hid groups to separate receivers from dj
devices
HID: logitech-dj: forward incoming HID++ reports to the correct dj
device
HID: logitech-dj: add .request callback
drivers/hid/hid-core.c | 3 +-
drivers/hid/hid-logitech-dj.c | 161 +++++++++++++++++++++++++++++++++---------
drivers/hid/hid-logitech-dj.h | 16 ++---
include/linux/hid.h | 1 +
4 files changed, 136 insertions(+), 45 deletions(-)
--
1.8.4.2
^ permalink raw reply
* [PATCH 3/5] HID: logitech-dj: rely on hid groups to separate receivers from dj devices
From: Benjamin Tissoires @ 2014-01-08 22:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, linux-input, linux-kernel
In-Reply-To: <1389219529-29671-1-git-send-email-benjamin.tissoires@redhat.com>
From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
Several benefits here:
- we can drop the macro is_dj_device: I never been really conviced by
this macro as we could fall into a null pointer anytime. Anyway time
showed that this never happened.
- we can simplify the hid driver logitech-djdevice, and make it aware
of any new receiver VID/PID.
- we can use the Wireless PID of the DJ device as the product id of the
hid device, this way the sysfs will differentiate between different
DJ devices.
Signed-off-by: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 37 +++++++++----------------------------
drivers/hid/hid-logitech-dj.h | 10 ----------
include/linux/hid.h | 1 +
3 files changed, 10 insertions(+), 38 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index f45279c..a4b3cee 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -263,11 +263,14 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
dj_hiddev->dev.parent = &djrcv_hdev->dev;
dj_hiddev->bus = BUS_USB;
dj_hiddev->vendor = le16_to_cpu(usbdev->descriptor.idVendor);
- dj_hiddev->product = le16_to_cpu(usbdev->descriptor.idProduct);
+ dj_hiddev->product =
+ (dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_MSB] << 8) |
+ dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_LSB];
snprintf(dj_hiddev->name, sizeof(dj_hiddev->name),
- "Logitech Unifying Device. Wireless PID:%02x%02x",
- dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_MSB],
- dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_LSB]);
+ "Logitech Unifying Device. Wireless PID:%04x",
+ dj_hiddev->product);
+
+ dj_hiddev->group = HID_GROUP_LOGITECH_DJ_DEVICE_GENERIC;
usb_make_path(usbdev, dj_hiddev->phys, sizeof(dj_hiddev->phys));
snprintf(tmpstr, sizeof(tmpstr), ":%d", dj_report->device_index);
@@ -752,9 +755,6 @@ static int logi_dj_probe(struct hid_device *hdev,
struct dj_receiver_dev *djrcv_dev;
int retval;
- if (is_dj_device((struct dj_device *)hdev->driver_data))
- return -ENODEV;
-
dbg_hid("%s called for ifnum %d\n", __func__,
intf->cur_altsetting->desc.bInterfaceNumber);
@@ -907,22 +907,6 @@ static void logi_dj_remove(struct hid_device *hdev)
hid_set_drvdata(hdev, NULL);
}
-static int logi_djdevice_probe(struct hid_device *hdev,
- const struct hid_device_id *id)
-{
- int ret;
- struct dj_device *dj_dev = hdev->driver_data;
-
- if (!is_dj_device(dj_dev))
- return -ENODEV;
-
- ret = hid_parse(hdev);
- if (!ret)
- ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
-
- return ret;
-}
-
static const struct hid_device_id logi_dj_receivers[] = {
{HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER)},
@@ -946,17 +930,14 @@ static struct hid_driver logi_djreceiver_driver = {
static const struct hid_device_id logi_dj_devices[] = {
- {HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
- USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER)},
- {HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
- USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2)},
+ { HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE_GENERIC,
+ USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
{}
};
static struct hid_driver logi_djdevice_driver = {
.name = "logitech-djdevice",
.id_table = logi_dj_devices,
- .probe = logi_djdevice_probe,
};
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 4a40003..2e52167 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -111,14 +111,4 @@ struct dj_device {
u8 device_index;
};
-/**
- * is_dj_device - know if the given dj_device is not the receiver.
- * @dj_dev: the dj device to test
- *
- * This macro tests if a struct dj_device pointer is a device created
- * by the bus enumarator.
- */
-#define is_dj_device(dj_dev) \
- (&(dj_dev)->dj_receiver_dev->hdev->dev == (dj_dev)->hdev->dev.parent)
-
#endif
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 31b9d29..eacf556 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -299,6 +299,7 @@ struct hid_item {
#define HID_GROUP_MULTITOUCH 0x0002
#define HID_GROUP_SENSOR_HUB 0x0003
#define HID_GROUP_MULTITOUCH_WIN_8 0x0004
+#define HID_GROUP_LOGITECH_DJ_DEVICE_GENERIC 0x0005
/*
* This is the global environment of the parser. This information is
--
1.8.4.2
^ permalink raw reply related
* [PATCH 2/5] HID: core: do not scan reports if the group is already set
From: Benjamin Tissoires @ 2014-01-08 22:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, linux-input, linux-kernel
In-Reply-To: <1389219529-29671-1-git-send-email-benjamin.tissoires@redhat.com>
From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
This allows the transport layer (I have in mind hid-logitech-dj and uhid)
to set the group before it is added to the hid bus. This way, it can
bypass the hid_scan_report() call, and choose in advance which driver
will handle the newly created hid device.
Signed-off-by: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a5a8c6a..200118a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2438,7 +2438,8 @@ int hid_add_device(struct hid_device *hdev)
* Scan generic devices for group information
*/
if (hid_ignore_special_drivers ||
- !hid_match_id(hdev, hid_have_special_driver)) {
+ (!hdev->group &&
+ !hid_match_id(hdev, hid_have_special_driver))) {
ret = hid_scan_report(hdev);
if (ret)
hid_warn(hdev, "bad device descriptor (%d)\n", ret);
--
1.8.4.2
^ permalink raw reply related
* [PATCH 1/5] HID: logitech-dj: Fix USB 3.0 issue
From: Benjamin Tissoires @ 2014-01-08 22:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, linux-input, linux-kernel
In-Reply-To: <1389219529-29671-1-git-send-email-benjamin.tissoires@redhat.com>
From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
This fix (not very clean though) should fix the long time USB3
issue that was spotted last year. The rational has been given by
Hans de Goede:
----
I think the most likely cause for this is a firmware bug
in the unifying receiver, likely a race condition.
The most prominent difference between having a USB-2 device
plugged into an EHCI (so USB-2 only) port versus an XHCI
port will be inter packet timing. Specifically if you
send packets (ie hid reports) one at a time, then with
the EHCI controller their will be a significant pause
between them, where with XHCI they will be very close
together in time.
The reason for this is the difference in EHCI / XHCI
controller OS <-> driver interfaces.
For non periodic endpoints (control, bulk) the EHCI uses a
circular linked-list of commands in dma-memory, which it
follows to execute commands, if the list is empty, it
will go into an idle state and re-check periodically.
The XHCI uses a ring of commands per endpoint, and if the OS
places anything new on the ring it will do an ioport write,
waking up the XHCI making it send the new packet immediately.
For periodic transfers (isoc, interrupt) the delay between
packets when sending one at a time (rather then queuing them
up) will be even larger, because they need to be inserted into
the EHCI schedule 2 ms in the future so the OS driver can be
sure that the EHCI driver does not try to start executing the
time slot in question before the insertion has completed.
So a possible fix may be to insert a delay between packets
being send to the receiver.
----
I tested this on a buggy Haswell USB 3.0 motherboard, and I always
get the notification after adding the msleep.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index a7947d8..f45279c 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -516,6 +516,14 @@ static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
dj_report->report_params[CMD_SWITCH_PARAM_TIMEOUT_SECONDS] = (u8)timeout;
retval = logi_dj_recv_send_report(djrcv_dev, dj_report);
kfree(dj_report);
+
+ /*
+ * Ugly sleep to work around a USB 3.0 bug when the receiver is still
+ * processing the "switch-to-dj" command while we send an other command.
+ * 50 msec should gives enough time to the receiver to be ready.
+ */
+ msleep(50);
+
return retval;
}
--
1.8.4.2
^ permalink raw reply related
* [PATCH] input: synaptics-rmi4 - cleanup rmi_i2c_probe()
From: Christopher Heiny @ 2014-01-08 22:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
Moves i2c_check_functionality to occur before the gpio_config() call. This
can catch some issues that would otherwise result in mysterious problems
in gpio_config().
Reduces debugging output; updates remaining output to be more accurate.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_i2c.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index 43b0e53..b4bd8ae 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -274,26 +274,24 @@ static int rmi_i2c_probe(struct i2c_client *client,
dev_err(&client->dev, "no platform data\n");
return -EINVAL;
}
- dev_info(&client->dev, "Probing %s at %#02x (IRQ %d).\n",
+ dev_dbg(&client->dev, "Probing %s at %#02x (GPIO %d).\n",
pdata->sensor_name ? pdata->sensor_name : "-no name-",
client->addr, pdata->attn_gpio);
+ retval = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
+ if (!retval) {
+ dev_err(&client->dev, "i2c_check_functionality error %d.\n",
+ retval);
+ return retval;
+ }
+
if (pdata->gpio_config) {
- dev_dbg(&client->dev, "Configuring GPIOs.\n");
retval = pdata->gpio_config(pdata->gpio_data, true);
if (retval < 0) {
dev_err(&client->dev, "Failed to configure GPIOs, code: %d.\n",
retval);
return retval;
}
- dev_info(&client->dev, "Done with GPIO configuration.\n");
- }
-
- retval = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
- if (!retval) {
- dev_err(&client->dev, "i2c_check_functionality error %d.\n",
- retval);
- return retval;
}
xport = devm_kzalloc(&client->dev, sizeof(struct rmi_transport_dev),
^ permalink raw reply related
* [PATCH] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c
From: Christopher Heiny @ 2014-01-08 21:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
This is a trivial change to use snprintf rather than sprintf while building
the diagnostic output buffer.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index b33074c..43b0e53 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -157,7 +157,7 @@ static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
temp = data->debug_buf;
for (i = 0; i < len; i++) {
- n = sprintf(temp, " %02x", buf[i]);
+ n = snprintf(temp, 3, " %02x", buf[i]);
temp += n;
}
^ permalink raw reply related
* I have a very lucrative Business
From: Dr. Musa Hamed @ 2014-01-08 20:47 UTC (permalink / raw)
ATTN: Dear,
I am Dr. Musa Hamed Executive Director of the Bank of Africa
Plc B.O.A,Cotonou Benin Rep. I have a very lucrative Business proposal
worth of 21,500,000.00 USD.
An Iraqi named Hamadi Hashem a business man made a fixed deposit of
21,500,000.00 USD . Upon maturity several notices was sent to him,
even during the war,eleven years ago (2003) up till this date. Again
after the war another notification was sent and still no response came
from him. it has been found out that Hamadi Hashem and his family had
been killed during the war in bomb blast that hit their home at
Mukaradeeb where his personal oil well was. I am prepared to split the
funds with you 50%:50%.By placing you as the NEXT OF KIN to his
deposit.
Should you be interested in doing this deal with me, please send me
the following information:
(1) Your Name
(2) Resident Address
(3) Your Occupation
(4) Your Phone Number
(5) Date of Birth
(6) Resident Country
And I will prefer you to reach me privately and securely on my personal
email address: dr.musahamed30@yahoo.fr
Tel: +229-988-400-97
I will provide you with further information as soon as I hear
positively from you.
Regards,
Dr. Musa Hamed
^ permalink raw reply
* Re: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
From: Dmitry Torokhov @ 2014-01-08 19:12 UTC (permalink / raw)
To: Opensource [Anthony Olech]
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Huqiu Liu, David Dajun Chen
In-Reply-To: <24DF37198A1E704D9811D8F72B87EB51847B1D99@NB-EX-MBX02.diasemi.com>
On Wed, Jan 08, 2014 at 06:12:27PM +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: 08 January 2014 17:26
> > To: Opensource [Anthony Olech]
> > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu;
> > David Dajun Chen
> > Subject: Re: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
> > Anthony Olech <anthony.olech.opensource@diasemi.com> wrote:
> > >The touchscreen component driver for the da9052/3 Dialog PMICs leaks
> > >memory by not freeing the input device in the remove call.
> > >This patch matches the existing call to input_alloc_device() in
> > >da9052_ts_probe() to a new call to input_free_device() in
> > >da9052_ts_remove()
> > >Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn>
> > >Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> > >---
> > >This patch is relative to linux-next repository tag next-20140108
> > >Many thanks to Huqiu Liu who found the bug.
> > No, this is not a bug. Please refer to input API spec in input.h
> > Thanks.
>
> Hi Dmitry,
> the spec *seems* to say that if the allocation is done via devm_input_allocate_device(dev)
> then no explicit freeing must be done.
>
> However that is not the case here, so the bug exists.
*sigh*
/**
* input_free_device - free memory occupied by input_dev structure
* @dev: input device to free
*
* This function should only be used if input_register_device()
* was not called yet or if it failed. Once device was registered
* use input_unregister_device() and memory will be freed once last
* reference to the device is dropped.
THIS ^^^^^^^^^^^^^^^^^^^^
*
* Device should be allocated by input_allocate_device().
*
* NOTE: If there are references to the input device then memory
* will not be freed until last reference is dropped.
*/
/**
* input_allocate_device - allocate memory for new input device
*
* Returns prepared struct input_dev or %NULL.
*
* NOTE: Use input_free_device() to free devices that have not been
* registered; input_unregister_device() should be used for already
* registered devices.
and this ^^^^^^^^^^^^^^^^^^^^^^^^
*/
>
> It does seem that there is an alternative way of resolving the issue, namely to:
> 1) change from allocate_device() to devm_input_allocate_device(dev) and
> 2) remove the exiosting input_free_device() from the error path in the probe() function
There is no issue, but if you want to convert the driver to using
managed resources that would be fine.
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
From: Opensource [Anthony Olech] @ 2014-01-08 18:12 UTC (permalink / raw)
To: Dmitry Torokhov, Opensource [Anthony Olech]
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Huqiu Liu, David Dajun Chen
In-Reply-To: <7aad8e91-f5d9-4527-8a94-bc038a9ccd85@email.android.com>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 08 January 2014 17:26
> To: Opensource [Anthony Olech]
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu;
> David Dajun Chen
> Subject: Re: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
> Anthony Olech <anthony.olech.opensource@diasemi.com> wrote:
> >The touchscreen component driver for the da9052/3 Dialog PMICs leaks
> >memory by not freeing the input device in the remove call.
> >This patch matches the existing call to input_alloc_device() in
> >da9052_ts_probe() to a new call to input_free_device() in
> >da9052_ts_remove()
> >Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn>
> >Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> >---
> >This patch is relative to linux-next repository tag next-20140108
> >Many thanks to Huqiu Liu who found the bug.
> No, this is not a bug. Please refer to input API spec in input.h
> Thanks.
Hi Dmitry,
the spec *seems* to say that if the allocation is done via devm_input_allocate_device(dev)
then no explicit freeing must be done.
However that is not the case here, so the bug exists.
It does seem that there is an alternative way of resolving the issue, namely to:
1) change from allocate_device() to devm_input_allocate_device(dev) and
2) remove the exiosting input_free_device() from the error path in the probe() function
I will update the patch submission to the alternative tomorrow
Tony Olech (Dialog Semiconductor)
> > drivers/input/touchscreen/da9052_tsi.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >diff --git a/drivers/input/touchscreen/da9052_tsi.c
> >b/drivers/input/touchscreen/da9052_tsi.c
> >index ab64d58..43a69d1 100644
> >--- a/drivers/input/touchscreen/da9052_tsi.c
> >+++ b/drivers/input/touchscreen/da9052_tsi.c
> >@@ -320,6 +320,7 @@ err_free_mem:
> > static int da9052_ts_remove(struct platform_device *pdev) {
> > struct da9052_tsi *tsi = platform_get_drvdata(pdev);
> >+ struct input_dev *input_dev = tsi->dev;
> > da9052_reg_write(tsi->da9052, DA9052_LDO9_REG, 0x19);
> >@@ -328,6 +329,7 @@ static int da9052_ts_remove(struct
> platform_device
> >*pdev)
> > input_unregister_device(tsi->dev);
> > kfree(tsi);
> >+ input_free_device(input_dev);
> > return 0;
> > }
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
From: Dmitry Torokhov @ 2014-01-08 17:26 UTC (permalink / raw)
To: Anthony Olech; +Cc: linux-input, linux-kernel, Huqiu Liu, David Dajun Chen
In-Reply-To: <201401081709.s08H9nMr038265@swsrvapps-02.lan>
Anthony Olech <anthony.olech.opensource@diasemi.com> wrote:
>The touchscreen component driver for the da9052/3 Dialog PMICs
>leaks memory by not freeing the input device in the remove call.
>
>This patch matches the existing call to input_alloc_device()
>in da9052_ts_probe() to a new call to input_free_device() in
>da9052_ts_remove()
>
>Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn>
>Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
>---
>This patch is relative to linux-next repository tag next-20140108
>
>Many thanks to Huqiu Liu who found the bug.
No, this is not a bug. Please refer to input API spec in input.h
Thanks.
>
> drivers/input/touchscreen/da9052_tsi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/input/touchscreen/da9052_tsi.c
>b/drivers/input/touchscreen/da9052_tsi.c
>index ab64d58..43a69d1 100644
>--- a/drivers/input/touchscreen/da9052_tsi.c
>+++ b/drivers/input/touchscreen/da9052_tsi.c
>@@ -320,6 +320,7 @@ err_free_mem:
> static int da9052_ts_remove(struct platform_device *pdev)
> {
> struct da9052_tsi *tsi = platform_get_drvdata(pdev);
>+ struct input_dev *input_dev = tsi->dev;
>
> da9052_reg_write(tsi->da9052, DA9052_LDO9_REG, 0x19);
>
>@@ -328,6 +329,7 @@ static int da9052_ts_remove(struct platform_device
>*pdev)
>
> input_unregister_device(tsi->dev);
> kfree(tsi);
>+ input_free_device(input_dev);
>
> return 0;
> }
--
Dmitry
^ permalink raw reply
* [PATCH V1] input: fix memory leak in da9052 touchscreen driver
From: Anthony Olech @ 2014-01-08 16:58 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Huqiu Liu, David Dajun Chen
The touchscreen component driver for the da9052/3 Dialog PMICs
leaks memory by not freeing the input device in the remove call.
This patch matches the existing call to input_alloc_device()
in da9052_ts_probe() to a new call to input_free_device() in
da9052_ts_remove()
Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn>
Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
---
This patch is relative to linux-next repository tag next-20140108
Many thanks to Huqiu Liu who found the bug.
drivers/input/touchscreen/da9052_tsi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/da9052_tsi.c b/drivers/input/touchscreen/da9052_tsi.c
index ab64d58..43a69d1 100644
--- a/drivers/input/touchscreen/da9052_tsi.c
+++ b/drivers/input/touchscreen/da9052_tsi.c
@@ -320,6 +320,7 @@ err_free_mem:
static int da9052_ts_remove(struct platform_device *pdev)
{
struct da9052_tsi *tsi = platform_get_drvdata(pdev);
+ struct input_dev *input_dev = tsi->dev;
da9052_reg_write(tsi->da9052, DA9052_LDO9_REG, 0x19);
@@ -328,6 +329,7 @@ static int da9052_ts_remove(struct platform_device *pdev)
input_unregister_device(tsi->dev);
kfree(tsi);
+ input_free_device(input_dev);
return 0;
}
--
end-of-patch for input: fix memory leak in da9052 touchscreen driver V1
^ permalink raw reply related
* Re: [PATCH] HID: input: fix input sysfs path for hid devices
From: David Herrmann @ 2014-01-08 16:46 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, Benjamin Tissoires, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <alpine.LNX.2.00.1401081724440.31628@pobox.suse.cz>
Hi
On Wed, Jan 8, 2014 at 5:26 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 8 Jan 2014, David Herrmann wrote:
>
>> >> > I think this is valuable because it will give a better understanding of the
>> >> > actual mapping hardware/inputs. Like a touchscreen with pen + touch will have
>> >> > the same hid parent, and I expect X/Wayland to be able to detect this at some
>> >> > point to keep the mapping correctly between the two input devices and the screen.
>> >> >
>> >> > I also need that for hid-replay, so that I can be sure which input is attached
>> >> > to which uhid device, and give up the heuristics I currently use.
>> >>
>> >> I was just wondering where we have multiple HID devices on a single
>> >> parent, but yeah, uhid is a good example. I actually have no
>> >> objections to this patch and it looks fine. But I cannot tell whether
>> >> anyone relies on this.
>> >>
>> >> I'd say we should give it a try.
>> >
>> > Agreed, let's take it for 3.14, and if anyone complains about breakage
>> > caused by this (which I don't expect), we'll revert to the previous state.
>>
>> You dropped the "Reviewed-by:" part in your commit-msg:
>> http://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/commit/?h=for-next&id=bbe3175408cde792fbaa5bd1e41e430ea9e4fb4f
>>
>> Not sure, whether you sometimes rebase your tree. If you do, you might
>> wanna fix it, otherwise, I don't care. Just wanted to point it out.
>
> Gah, sorry for that. No idea what in my process ate the tag.
>
> I am really trying to make the tree non-rebasing. If you don't mind, I
> wouldn't take this as a reson to rebase.
>
> I can put you twice to some patch later to balance this out :p
>
> Thanks for understanding,
Haha, don't worry, just saw it during post-review and wanted to let you know.
Cheers
David
^ 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