* Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Andy Shevchenko @ 2013-08-02 14:49 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Jiri Kosina, Mika Westerberg, Benjamin Tissoires
In-Reply-To: <CAN+gG=Ec=jJy7HxRW08up+=Z8C0Lz4B8bhPcP+7__zuONnCAgw@mail.gmail.com>
On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote:
> On Fri, Aug 2, 2013 at 1:07 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > We have nice dynamic debug framework to enable or disable debug messaging at
> > run time. So, instead of an additional module parameter let's use that framework
> > and call dev_dbg() unconditionally in the driver.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
>
> Well, when I introduced those debug variables, I had in mind the fact
> that the driver was not widely tested, and that I may need to ask for
> traces from users. I'm afraid that relying on dev_dbg will create a
> lot more noise when we will want to understand the HID/i2c problems.
You have only those messages on the debug level (frankly, only one is
outside of if (debug) condition).
> Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
> set.
Yes, and what is the difference between previously used
if (debug)
dev_dbg(...)
?
> So, if we ever have to change this debug variable, I would prefer
> using the hid debug environment which would at least limit the number
> of debug outputs to the HID subsystem.
Usually when I see such code I understood it was written in
pre-dynamic-debug epoch. So, my point is to switch to dynamic debug and
use it efficiently.
>
> Cheers,
> Benjamin
>
> > drivers/hid/i2c-hid/i2c-hid.c | 59 ++++++++++++++++++-------------------------
> > 1 file changed, 24 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 05d4f96..5f50fc7 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -47,17 +47,6 @@
> > #define I2C_HID_PWR_ON 0x00
> > #define I2C_HID_PWR_SLEEP 0x01
> >
> > -/* debug option */
> > -static bool debug;
> > -module_param(debug, bool, 0444);
> > -MODULE_PARM_DESC(debug, "print a lot of debug information");
> > -
> > -#define i2c_hid_dbg(ihid, fmt, arg...) \
> > -do { \
> > - if (debug) \
> > - dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
> > -} while (0)
> > -
> > struct i2c_hid_desc {
> > __le16 wHIDDescLength;
> > __le16 bcdVersion;
> > @@ -177,7 +166,7 @@ static int __i2c_hid_command(struct i2c_client *client,
> > memcpy(cmd->data + length, args, args_len);
> > length += args_len;
> >
> > - i2c_hid_dbg(ihid, "%s: cmd=%*ph\n", __func__, length, cmd->data);
> > + dev_dbg(&client->dev, "%s: cmd=%*ph\n", __func__, length, cmd->data);
> >
> > msg[0].addr = client->addr;
> > msg[0].flags = client->flags & I2C_M_TEN;
> > @@ -207,12 +196,12 @@ static int __i2c_hid_command(struct i2c_client *client,
> > ret = 0;
> >
> > if (wait) {
> > - i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
> > + dev_dbg(&client->dev, "%s: waiting...\n", __func__);
> > if (!wait_event_timeout(ihid->wait,
> > !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
> > msecs_to_jiffies(5000)))
> > ret = -ENODATA;
> > - i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
> > + dev_dbg(&client->dev, "%s: finished.\n", __func__);
> > }
> >
> > return ret;
> > @@ -235,7 +224,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
> > int args_len = 0;
> > u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > if (reportID >= 0x0F) {
> > args[args_len++] = reportID;
> > @@ -276,7 +265,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> > size /* args */;
> > int index = 0;
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > if (reportID >= 0x0F) {
> > args[index++] = reportID;
> > @@ -316,10 +305,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> >
> > static int i2c_hid_set_power(struct i2c_client *client, int power_state)
> > {
> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
> > int ret;
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> > 0, NULL, 0, NULL, 0);
> > @@ -331,16 +319,15 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
> >
> > static int i2c_hid_hwreset(struct i2c_client *client)
> > {
> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
> > int ret;
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> > if (ret)
> > return ret;
> >
> > - i2c_hid_dbg(ihid, "resetting...\n");
> > + dev_dbg(&client->dev, "resetting...\n");
> >
> > ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> > if (ret) {
> > @@ -354,15 +341,16 @@ static int i2c_hid_hwreset(struct i2c_client *client)
> >
> > static void i2c_hid_get_input(struct i2c_hid *ihid)
> > {
> > + struct i2c_client *client = ihid->client;
> > int ret, ret_size;
> > int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
> >
> > - ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> > + ret = i2c_master_recv(client, ihid->inbuf, size);
> > if (ret != size) {
> > if (ret < 0)
> > return;
> >
> > - dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
> > + dev_err(&client->dev, "%s: got %d data instead of %d\n",
> > __func__, ret, size);
> > return;
> > }
> > @@ -377,12 +365,12 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> > }
> >
> > if (ret_size > size) {
> > - dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
> > + dev_err(&client->dev, "%s: incomplete report (%d/%d)\n",
> > __func__, size, ret_size);
> > return;
> > }
> >
> > - i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> > + dev_dbg(&client->dev, "input: %*ph\n", ret_size, ihid->inbuf);
> >
> > if (test_bit(I2C_HID_STARTED, &ihid->flags))
> > hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
> > @@ -423,7 +411,8 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
> > report->id, buffer, size))
> > return;
> >
> > - i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
> > + dev_dbg(&client->dev, "report (len=%d): %*ph\n", size,
> > + size, ihid->inbuf);
> >
> > ret_size = buffer[0] | (buffer[1] << 8);
> >
> > @@ -618,7 +607,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> > int ret;
> > int tries = 3;
> >
> > - i2c_hid_dbg(ihid, "entering %s\n", __func__);
> > + dev_dbg(&client->dev, "entering %s\n", __func__);
> >
> > rsize = le16_to_cpu(hdesc->wReportDescLength);
> > if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> > @@ -642,7 +631,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> > return -ENOMEM;
> > }
> >
> > - i2c_hid_dbg(ihid, "asking HID report descriptor\n");
> > + dev_dbg(&client->dev, "asking HID report descriptor\n");
> >
> > ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
> > if (ret) {
> > @@ -651,7 +640,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> > return -EIO;
> > }
> >
> > - i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
> > + dev_dbg(&client->dev, "Report Descriptor: %*ph\n", rsize, rdesc);
> >
> > ret = hid_parse_report(hid, rdesc, rsize);
> > kfree(rdesc);
> > @@ -741,10 +730,9 @@ static void i2c_hid_close(struct hid_device *hid)
> > static int i2c_hid_power(struct hid_device *hid, int lvl)
> > {
> > struct i2c_client *client = hid->driver_data;
> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
> > int ret = 0;
> >
> > - i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
> > + dev_dbg(&client->dev, "%s lvl:%d\n", __func__, lvl);
> >
> > switch (lvl) {
> > case PM_HINT_FULLON:
> > @@ -801,8 +789,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> > * bytes 2-3 -> bcdVersion (has to be 1.00) */
> > ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
> >
> > - i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
> > - ihid->hdesc_buffer);
> > + dev_dbg(&client->dev, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
> > + ihid->hdesc_buffer);
> >
> > if (ret) {
> > dev_err(&client->dev,
> > @@ -832,7 +820,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> > return -ENODEV;
> > }
> >
> > - i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
> > + dev_dbg(&client->dev, "Fetching the HID descriptor\n");
> >
> > ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
> > dsize);
> > @@ -841,7 +829,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> > return -ENODEV;
> > }
> >
> > - i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
> > + dev_dbg(&client->dev, "HID Descriptor: %*ph\n", dsize,
> > + ihid->hdesc_buffer);
> >
> > return 0;
> > }
> > --
> > 1.8.4.rc0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Benjamin Tissoires @ 2013-08-02 14:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-input, Jiri Kosina, Mika Westerberg, Benjamin Tissoires
In-Reply-To: <1375441636-12921-2-git-send-email-andriy.shevchenko@linux.intel.com>
On Fri, Aug 2, 2013 at 1:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> We have nice dynamic debug framework to enable or disable debug messaging at
> run time. So, instead of an additional module parameter let's use that framework
> and call dev_dbg() unconditionally in the driver.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
Well, when I introduced those debug variables, I had in mind the fact
that the driver was not widely tested, and that I may need to ask for
traces from users. I'm afraid that relying on dev_dbg will create a
lot more noise when we will want to understand the HID/i2c problems.
Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
set.
So, if we ever have to change this debug variable, I would prefer
using the hid debug environment which would at least limit the number
of debug outputs to the HID subsystem.
Cheers,
Benjamin
> drivers/hid/i2c-hid/i2c-hid.c | 59 ++++++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 05d4f96..5f50fc7 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -47,17 +47,6 @@
> #define I2C_HID_PWR_ON 0x00
> #define I2C_HID_PWR_SLEEP 0x01
>
> -/* debug option */
> -static bool debug;
> -module_param(debug, bool, 0444);
> -MODULE_PARM_DESC(debug, "print a lot of debug information");
> -
> -#define i2c_hid_dbg(ihid, fmt, arg...) \
> -do { \
> - if (debug) \
> - dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
> -} while (0)
> -
> struct i2c_hid_desc {
> __le16 wHIDDescLength;
> __le16 bcdVersion;
> @@ -177,7 +166,7 @@ static int __i2c_hid_command(struct i2c_client *client,
> memcpy(cmd->data + length, args, args_len);
> length += args_len;
>
> - i2c_hid_dbg(ihid, "%s: cmd=%*ph\n", __func__, length, cmd->data);
> + dev_dbg(&client->dev, "%s: cmd=%*ph\n", __func__, length, cmd->data);
>
> msg[0].addr = client->addr;
> msg[0].flags = client->flags & I2C_M_TEN;
> @@ -207,12 +196,12 @@ static int __i2c_hid_command(struct i2c_client *client,
> ret = 0;
>
> if (wait) {
> - i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
> + dev_dbg(&client->dev, "%s: waiting...\n", __func__);
> if (!wait_event_timeout(ihid->wait,
> !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
> msecs_to_jiffies(5000)))
> ret = -ENODATA;
> - i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
> + dev_dbg(&client->dev, "%s: finished.\n", __func__);
> }
>
> return ret;
> @@ -235,7 +224,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
> int args_len = 0;
> u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
>
> - i2c_hid_dbg(ihid, "%s\n", __func__);
> + dev_dbg(&client->dev, "%s\n", __func__);
>
> if (reportID >= 0x0F) {
> args[args_len++] = reportID;
> @@ -276,7 +265,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> size /* args */;
> int index = 0;
>
> - i2c_hid_dbg(ihid, "%s\n", __func__);
> + dev_dbg(&client->dev, "%s\n", __func__);
>
> if (reportID >= 0x0F) {
> args[index++] = reportID;
> @@ -316,10 +305,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>
> static int i2c_hid_set_power(struct i2c_client *client, int power_state)
> {
> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> int ret;
>
> - i2c_hid_dbg(ihid, "%s\n", __func__);
> + dev_dbg(&client->dev, "%s\n", __func__);
>
> ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> 0, NULL, 0, NULL, 0);
> @@ -331,16 +319,15 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>
> static int i2c_hid_hwreset(struct i2c_client *client)
> {
> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> int ret;
>
> - i2c_hid_dbg(ihid, "%s\n", __func__);
> + dev_dbg(&client->dev, "%s\n", __func__);
>
> ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> if (ret)
> return ret;
>
> - i2c_hid_dbg(ihid, "resetting...\n");
> + dev_dbg(&client->dev, "resetting...\n");
>
> ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> if (ret) {
> @@ -354,15 +341,16 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>
> static void i2c_hid_get_input(struct i2c_hid *ihid)
> {
> + struct i2c_client *client = ihid->client;
> int ret, ret_size;
> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
>
> - ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> + ret = i2c_master_recv(client, ihid->inbuf, size);
> if (ret != size) {
> if (ret < 0)
> return;
>
> - dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
> + dev_err(&client->dev, "%s: got %d data instead of %d\n",
> __func__, ret, size);
> return;
> }
> @@ -377,12 +365,12 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> }
>
> if (ret_size > size) {
> - dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
> + dev_err(&client->dev, "%s: incomplete report (%d/%d)\n",
> __func__, size, ret_size);
> return;
> }
>
> - i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> + dev_dbg(&client->dev, "input: %*ph\n", ret_size, ihid->inbuf);
>
> if (test_bit(I2C_HID_STARTED, &ihid->flags))
> hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
> @@ -423,7 +411,8 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
> report->id, buffer, size))
> return;
>
> - i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
> + dev_dbg(&client->dev, "report (len=%d): %*ph\n", size,
> + size, ihid->inbuf);
>
> ret_size = buffer[0] | (buffer[1] << 8);
>
> @@ -618,7 +607,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> int ret;
> int tries = 3;
>
> - i2c_hid_dbg(ihid, "entering %s\n", __func__);
> + dev_dbg(&client->dev, "entering %s\n", __func__);
>
> rsize = le16_to_cpu(hdesc->wReportDescLength);
> if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> @@ -642,7 +631,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> return -ENOMEM;
> }
>
> - i2c_hid_dbg(ihid, "asking HID report descriptor\n");
> + dev_dbg(&client->dev, "asking HID report descriptor\n");
>
> ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
> if (ret) {
> @@ -651,7 +640,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> return -EIO;
> }
>
> - i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
> + dev_dbg(&client->dev, "Report Descriptor: %*ph\n", rsize, rdesc);
>
> ret = hid_parse_report(hid, rdesc, rsize);
> kfree(rdesc);
> @@ -741,10 +730,9 @@ static void i2c_hid_close(struct hid_device *hid)
> static int i2c_hid_power(struct hid_device *hid, int lvl)
> {
> struct i2c_client *client = hid->driver_data;
> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> int ret = 0;
>
> - i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
> + dev_dbg(&client->dev, "%s lvl:%d\n", __func__, lvl);
>
> switch (lvl) {
> case PM_HINT_FULLON:
> @@ -801,8 +789,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> * bytes 2-3 -> bcdVersion (has to be 1.00) */
> ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
>
> - i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
> - ihid->hdesc_buffer);
> + dev_dbg(&client->dev, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
> + ihid->hdesc_buffer);
>
> if (ret) {
> dev_err(&client->dev,
> @@ -832,7 +820,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> return -ENODEV;
> }
>
> - i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
> + dev_dbg(&client->dev, "Fetching the HID descriptor\n");
>
> ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
> dsize);
> @@ -841,7 +829,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> return -ENODEV;
> }
>
> - i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
> + dev_dbg(&client->dev, "HID Descriptor: %*ph\n", dsize,
> + ihid->hdesc_buffer);
>
> return 0;
> }
> --
> 1.8.4.rc0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph
From: Benjamin Tissoires @ 2013-08-02 12:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-input, Jiri Kosina, Mika Westerberg, Benjamin Tissoires
In-Reply-To: <1375441636-12921-1-git-send-email-andriy.shevchenko@linux.intel.com>
On Fri, Aug 2, 2013 at 1:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> There is no need to pass constants via stack. The width may be explicitly
> specified in the format.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Thanks Andy,
Benjamin
^ permalink raw reply
* [PATCH] hid-holtekff: don't push static constants on stack for %*ph
From: Andy Shevchenko @ 2013-08-02 11:08 UTC (permalink / raw)
To: linux-input, Jiri Kosina; +Cc: Andy Shevchenko
There is no need to pass constants via stack. The width may be explicitly
specified in the format.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/hid/hid-holtekff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-holtekff.c b/drivers/hid/hid-holtekff.c
index 9a8f051..9325545 100644
--- a/drivers/hid/hid-holtekff.c
+++ b/drivers/hid/hid-holtekff.c
@@ -98,7 +98,7 @@ static void holtekff_send(struct holtekff_device *holtekff,
holtekff->field->value[i] = data[i];
}
- dbg_hid("sending %*ph\n", 7, data);
+ dbg_hid("sending %7ph\n", data);
hid_hw_request(hid, holtekff->field->report, HID_REQ_SET_REPORT);
}
--
1.8.4.rc0
^ permalink raw reply related
* [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Andy Shevchenko @ 2013-08-02 11:07 UTC (permalink / raw)
To: linux-input, Jiri Kosina, Mika Westerberg, Benjamin Tissoires
Cc: Andy Shevchenko
In-Reply-To: <1375441636-12921-1-git-send-email-andriy.shevchenko@linux.intel.com>
We have nice dynamic debug framework to enable or disable debug messaging at
run time. So, instead of an additional module parameter let's use that framework
and call dev_dbg() unconditionally in the driver.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/hid/i2c-hid/i2c-hid.c | 59 ++++++++++++++++++-------------------------
1 file changed, 24 insertions(+), 35 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 05d4f96..5f50fc7 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -47,17 +47,6 @@
#define I2C_HID_PWR_ON 0x00
#define I2C_HID_PWR_SLEEP 0x01
-/* debug option */
-static bool debug;
-module_param(debug, bool, 0444);
-MODULE_PARM_DESC(debug, "print a lot of debug information");
-
-#define i2c_hid_dbg(ihid, fmt, arg...) \
-do { \
- if (debug) \
- dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
-} while (0)
-
struct i2c_hid_desc {
__le16 wHIDDescLength;
__le16 bcdVersion;
@@ -177,7 +166,7 @@ static int __i2c_hid_command(struct i2c_client *client,
memcpy(cmd->data + length, args, args_len);
length += args_len;
- i2c_hid_dbg(ihid, "%s: cmd=%*ph\n", __func__, length, cmd->data);
+ dev_dbg(&client->dev, "%s: cmd=%*ph\n", __func__, length, cmd->data);
msg[0].addr = client->addr;
msg[0].flags = client->flags & I2C_M_TEN;
@@ -207,12 +196,12 @@ static int __i2c_hid_command(struct i2c_client *client,
ret = 0;
if (wait) {
- i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
+ dev_dbg(&client->dev, "%s: waiting...\n", __func__);
if (!wait_event_timeout(ihid->wait,
!test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
msecs_to_jiffies(5000)))
ret = -ENODATA;
- i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
+ dev_dbg(&client->dev, "%s: finished.\n", __func__);
}
return ret;
@@ -235,7 +224,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
int args_len = 0;
u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
- i2c_hid_dbg(ihid, "%s\n", __func__);
+ dev_dbg(&client->dev, "%s\n", __func__);
if (reportID >= 0x0F) {
args[args_len++] = reportID;
@@ -276,7 +265,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
size /* args */;
int index = 0;
- i2c_hid_dbg(ihid, "%s\n", __func__);
+ dev_dbg(&client->dev, "%s\n", __func__);
if (reportID >= 0x0F) {
args[index++] = reportID;
@@ -316,10 +305,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
static int i2c_hid_set_power(struct i2c_client *client, int power_state)
{
- struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret;
- i2c_hid_dbg(ihid, "%s\n", __func__);
+ dev_dbg(&client->dev, "%s\n", __func__);
ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
0, NULL, 0, NULL, 0);
@@ -331,16 +319,15 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
static int i2c_hid_hwreset(struct i2c_client *client)
{
- struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret;
- i2c_hid_dbg(ihid, "%s\n", __func__);
+ dev_dbg(&client->dev, "%s\n", __func__);
ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
if (ret)
return ret;
- i2c_hid_dbg(ihid, "resetting...\n");
+ dev_dbg(&client->dev, "resetting...\n");
ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
if (ret) {
@@ -354,15 +341,16 @@ static int i2c_hid_hwreset(struct i2c_client *client)
static void i2c_hid_get_input(struct i2c_hid *ihid)
{
+ struct i2c_client *client = ihid->client;
int ret, ret_size;
int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
- ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+ ret = i2c_master_recv(client, ihid->inbuf, size);
if (ret != size) {
if (ret < 0)
return;
- dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
+ dev_err(&client->dev, "%s: got %d data instead of %d\n",
__func__, ret, size);
return;
}
@@ -377,12 +365,12 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
}
if (ret_size > size) {
- dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
+ dev_err(&client->dev, "%s: incomplete report (%d/%d)\n",
__func__, size, ret_size);
return;
}
- i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
+ dev_dbg(&client->dev, "input: %*ph\n", ret_size, ihid->inbuf);
if (test_bit(I2C_HID_STARTED, &ihid->flags))
hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
@@ -423,7 +411,8 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
report->id, buffer, size))
return;
- i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
+ dev_dbg(&client->dev, "report (len=%d): %*ph\n", size,
+ size, ihid->inbuf);
ret_size = buffer[0] | (buffer[1] << 8);
@@ -618,7 +607,7 @@ static int i2c_hid_parse(struct hid_device *hid)
int ret;
int tries = 3;
- i2c_hid_dbg(ihid, "entering %s\n", __func__);
+ dev_dbg(&client->dev, "entering %s\n", __func__);
rsize = le16_to_cpu(hdesc->wReportDescLength);
if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
@@ -642,7 +631,7 @@ static int i2c_hid_parse(struct hid_device *hid)
return -ENOMEM;
}
- i2c_hid_dbg(ihid, "asking HID report descriptor\n");
+ dev_dbg(&client->dev, "asking HID report descriptor\n");
ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
if (ret) {
@@ -651,7 +640,7 @@ static int i2c_hid_parse(struct hid_device *hid)
return -EIO;
}
- i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
+ dev_dbg(&client->dev, "Report Descriptor: %*ph\n", rsize, rdesc);
ret = hid_parse_report(hid, rdesc, rsize);
kfree(rdesc);
@@ -741,10 +730,9 @@ static void i2c_hid_close(struct hid_device *hid)
static int i2c_hid_power(struct hid_device *hid, int lvl)
{
struct i2c_client *client = hid->driver_data;
- struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret = 0;
- i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
+ dev_dbg(&client->dev, "%s lvl:%d\n", __func__, lvl);
switch (lvl) {
case PM_HINT_FULLON:
@@ -801,8 +789,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
* bytes 2-3 -> bcdVersion (has to be 1.00) */
ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
- i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
- ihid->hdesc_buffer);
+ dev_dbg(&client->dev, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
+ ihid->hdesc_buffer);
if (ret) {
dev_err(&client->dev,
@@ -832,7 +820,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
return -ENODEV;
}
- i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
+ dev_dbg(&client->dev, "Fetching the HID descriptor\n");
ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
dsize);
@@ -841,7 +829,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
return -ENODEV;
}
- i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
+ dev_dbg(&client->dev, "HID Descriptor: %*ph\n", dsize,
+ ihid->hdesc_buffer);
return 0;
}
--
1.8.4.rc0
^ permalink raw reply related
* [PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph
From: Andy Shevchenko @ 2013-08-02 11:07 UTC (permalink / raw)
To: linux-input, Jiri Kosina, Mika Westerberg, Benjamin Tissoires
Cc: Andy Shevchenko
There is no need to pass constants via stack. The width may be explicitly
specified in the format.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/hid/i2c-hid/i2c-hid.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index c21e692..05d4f96 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -801,8 +801,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
* bytes 2-3 -> bcdVersion (has to be 1.00) */
ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
- i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %*ph\n",
- __func__, 4, ihid->hdesc_buffer);
+ i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
+ ihid->hdesc_buffer);
if (ret) {
dev_err(&client->dev,
--
1.8.4.rc0
^ permalink raw reply related
* Re: [PATCH 2/2] HID: hid-logitech-dj, querying_devices was never set
From: Jiri Kosina @ 2013-08-02 1:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nestor Lopez Casado, Andrew de los Reyes, joseph.salisbury,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=GcS7pfK9K5FuW9TyX+PJWNkXZWLk8UX9PzLXom2PSpgQ@mail.gmail.com>
On Thu, 1 Aug 2013, Benjamin Tissoires wrote:
> > Set querying_devices flag to true when we start the enumeration
> > process.
> >
> > This was missing from the original patch. It never produced
> > undesirable effects as it is highly improbable to have a second
> > enumeration triggered while a first one was still in progress.
> >
> > Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
> > ---
> > drivers/hid/hid-logitech-dj.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index 0d13389..d4657a5 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
> > if (djrcv_dev->querying_devices)
> > return 0;
> >
> > + djrcv_dev->querying_devices = true;
> > +
>
> Unfortunately, this breaks the fallback mechanism :(
> We tried to add the two patches in Fedora [1], but this doesn't fix
> the bug because the driver actually things that it already asked for
> the enumeration, but as we get the -EPIPE error, the request was never
> sent.
>
> So, Jiri, if you were to submit that series to Linus (or Greg) for
> fixing the bug, please just drop this second patch.
It's already on its way to Linus (he hasn't pulled yet though) ... which
is not a big deal per se, I can always push a revert, but I have to admit
I don't understand the breakage it is causing at all.
Could you please elaborate? (and put an elaborate description to revert
commit log perhaps?)
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/2] HID: hid-logitech-dj, querying_devices was never set
From: Benjamin Tissoires @ 2013-08-01 10:09 UTC (permalink / raw)
To: Nestor Lopez Casado
Cc: Jiri Kosina, Andrew de los Reyes, joseph.salisbury, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <1374153691-25100-2-git-send-email-nlopezcasad@logitech.com>
On Thu, Jul 18, 2013 at 3:21 PM, Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
> Set querying_devices flag to true when we start the enumeration
> process.
>
> This was missing from the original patch. It never produced
> undesirable effects as it is highly improbable to have a second
> enumeration triggered while a first one was still in progress.
>
> Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
> ---
> drivers/hid/hid-logitech-dj.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 0d13389..d4657a5 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
> if (djrcv_dev->querying_devices)
> return 0;
>
> + djrcv_dev->querying_devices = true;
> +
Unfortunately, this breaks the fallback mechanism :(
We tried to add the two patches in Fedora [1], but this doesn't fix
the bug because the driver actually things that it already asked for
the enumeration, but as we get the -EPIPE error, the request was never
sent.
So, Jiri, if you were to submit that series to Linus (or Greg) for
fixing the bug, please just drop this second patch.
Cheers,
Benjamin
[1] https://bugzilla.redhat.com/show_bug.cgi?id=989138
> dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL);
> if (!dj_report)
> return -ENOMEM;
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH linux-next] hid/logitech-dj: Fix non-atomic kmalloc in logi_dj_ll_input_event()
From: Jiri Kosina @ 2013-07-31 22:47 UTC (permalink / raw)
To: Peter Hurley; +Cc: linux-input, linux-kernel
In-Reply-To: <1375305478-26754-1-git-send-email-peter@hurleysoftware.com>
On Wed, 31 Jul 2013, Peter Hurley wrote:
> The ll_driver's .hidinput_input_event() method is called from
> atomic context [1]. Use GFP_ATOMIC for allocation of the
> synthesized hid report.
>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Applied, thanks Peter.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH linux-next] hid/logitech-dj: Fix non-atomic kmalloc in logi_dj_ll_input_event()
From: Peter Hurley @ 2013-07-31 21:17 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, Peter Hurley
The ll_driver's .hidinput_input_event() method is called from
atomic context [1]. Use GFP_ATOMIC for allocation of the
synthesized hid report.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
[1]
BUG: sleeping function called from invalid context at /home/peter/src/kernels/next/mm/slub.c:941
in_atomic(): 1, irqs_disabled(): 1, pid: 2095, name: Xorg
INFO: lockdep is turned off.
irq event stamp: 1502178
hardirqs last enabled at (1502177): [<ffffffff81785e55>] _raw_spin_unlock_irqrestore+0x65/0x80
hardirqs last disabled at (1502178): [<ffffffff8178632a>] common_interrupt+0x6a/0x6f
softirqs last enabled at (1501802): [<ffffffff81051ed3>] __do_softirq+0x183/0x420
softirqs last disabled at (1501799): [<ffffffff81052315>] irq_exit+0xb5/0xc0
CPU: 3 PID: 2095 Comm: Xorg Not tainted 3.11-next-20130725-xeon+lockdep #20130725
Hardware name: Dell Inc. Precision WorkStation T5400 /0RW203, BIOS A11 04/30/2012
ffffffff81a662e0 ffff8802adcf9ca8 ffffffff8177c330 0000000000000000
ffff8802a76d2440 ffff8802adcf9cd8 ffffffff810867d0 ffff8802a7ac8000
0000000000000010 00000000ffffffff 00000000000000d0 ffff8802adcf9d38
Call Trace:
[<ffffffff8177c330>] dump_stack+0x4f/0x84
[<ffffffff810867d0>] __might_sleep+0x140/0x1f0
[<ffffffff811ad93b>] __kmalloc+0x6b/0x2e0
[<ffffffffa026cb08>] ? hid_alloc_report_buf+0x28/0x30 [hid]
[<ffffffffa026cb08>] hid_alloc_report_buf+0x28/0x30 [hid]
[<ffffffffa00700b0>] logi_dj_ll_input_event+0xb0/0x1b0 [hid_logitech_dj]
[<ffffffff815a559e>] input_handle_event+0x8e/0x540
[<ffffffff815a5aad>] ? input_inject_event+0x5d/0x220
[<ffffffff815a5c10>] input_inject_event+0x1c0/0x220
[<ffffffff815a5a94>] ? input_inject_event+0x44/0x220
[<ffffffff81181660>] ? might_fault+0xa0/0xb0
[<ffffffff81181617>] ? might_fault+0x57/0xb0
[<ffffffff815a909e>] evdev_write+0xde/0x160
[<ffffffff811c0ad8>] vfs_write+0xc8/0x1f0
[<ffffffff811c0fe5>] SyS_write+0x55/0xa0
[<ffffffff8178e682>] system_call_fastpath+0x16/0x1b
---
drivers/hid/hid-logitech-dj.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 510fa8f..e7ad88e 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -591,7 +591,7 @@ static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
}
hid_set_field(field, offset, value);
- data = hid_alloc_report_buf(field->report, GFP_KERNEL);
+ data = hid_alloc_report_buf(field->report, GFP_ATOMIC);
if (!data) {
dev_warn(&dev->dev, "failed to allocate report buf memory\n");
return -1;
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] HID: i2c-hid: add DT bindings
From: Jiri Kosina @ 2013-07-31 10:12 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Grant Likely, Rob Herring, devicetree-discuss,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=HA0wM=O-w_GRUtJJmGjwufmL52wTwhvLO6VLV9oPLmiA@mail.gmail.com>
On Wed, 24 Jul 2013, Benjamin Tissoires wrote:
> >> Add device tree based support for HID over I2C devices.
> >>
> >> Tested on an Odroid-X board with a Synaptics touchpad.
> >>
> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >> ---
> >>
> >> Hi guys,
> >>
> >> well, as the commit message says, this is the DT binding for HID over I2C.
> >> I honestly don't know if it will be used besides me, but it may help others
> >> with a DT based board.
> >> As the spec is for ACPI only, I had no specifications regarding the DT names. So
> >> these names can be changed if you think they are bad.
> >>
> >> I also created a new bindings directory in the devicetree doc to reflect the
> >> split we have between input and hid. However, if the DT experts prefer having
> >> it under input, I'm fine with that.
> >>
> >> Cheers,
> >> Benjamin
> >>
> >> .../devicetree/bindings/hid/hid-over-i2c.txt | 28 ++++++++++++++
> >> drivers/hid/i2c-hid/i2c-hid.c | 44 +++++++++++++++++++++-
> >> include/linux/i2c/i2c-hid.h | 3 +-
> >> 3 files changed, 73 insertions(+), 2 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> >
> > DT folks, any objections to this, please?
> >
> > If not, I'd like to apply this for 3.11 into my tree.
> >
>
> DT folks, ping???
As there have been no objections for more than one month, I am now queuing
this up.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.
From: Felipe Balbi @ 2013-07-31 9:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: balbi, Illia Smyrnov, linux-input, linux-kernel, linux-omap,
Greg KH
In-Reply-To: <1861803.h9dhBdaB3X@dtor-d630.eng.vmware.com>
[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]
On Mon, Jul 29, 2013 at 01:40:28PM -0700, Dmitry Torokhov wrote:
> On Monday, July 29, 2013 11:36:05 PM Felipe Balbi wrote:
> > Hi,
> >
> > On Mon, Jul 29, 2013 at 12:59:23PM -0700, Dmitry Torokhov wrote:
> > > > > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > > > > omap_keypad_dt_match[] = {>
> > > > > > >
> > > > > > > MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > > > > > #endif
> > > > > > >
> > > > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > > > > +{
> > > > > > > + struct platform_device *pdev = to_platform_device(dev);
> > > > > >
> > > > > > you don't need to access the platform_device...
> > > > > >
> > > > > > > + struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > > >
> > > > > > ... since this can become:
> > > > > > struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > > > >
> > > > > No, please use correct accessors for the objects. Platform drivers
> > > > > deal
> > > > > with platform devices and I prefer using platform_get_drvdata() on
> > > > > them.
> > > >
> > > > The argument to this function is a struct device, you prefer to do some
> > > > pointer math to find the containing pdev, then deref that back to dev,
> > > > then to struct device_private and further to driver_data ?
> > > >
> > > > Sounds like a waste of time IMHO. You already have the device pointer
> > > > anyway, why would you go through the trouble of calculating the
> > > > offsets for the containing struct platform_device ?
> > >
> > > This assumes knowledge of dev_get_drvdata() implementation and assumption
> > > that it will stay the same. Unless I hear from device core guys that
> > > <bus>_{get|set}_drvdata() methods are obsolete and will be eventually
> > > removed I will require proper accessors being used.
> >
> > they're not obsolete and will never be removed. They're nothing but
> > helpers though. Instead of calling:
> >
> > dev_set_drvdata(&pdev->dev);
> >
> > you call:
> >
> > platform_set_drvdata(pdev);
> >
> > same is valid for every single bus, but in the end they all just wrap a
> > call dev_{set,get}_drvdata() internally. If you already have a struct
> > device pointer as argument, why waste cycles doing pointer math just to
> > go back to the same struct device pointer on the next call ?
>
> Because I do not want to rely on the fact that what my driver set up
> with platform_set_drvdata(pdev, XXX) is the same as what dev_get_drvdata()
> will return *in the current implementation*. Software layers and all
> that...
fair enough, your call. It's a waste of CPU anyway.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 0/8] HID: Transport Driver Cleanup
From: Jiri Kosina @ 2013-07-31 9:03 UTC (permalink / raw)
To: David Herrmann
Cc: open list:HID CORE LAYER, Benjamin Tissoires, Henrik Rydberg,
Oliver Neukum
In-Reply-To: <CANq1E4TTKgVtCp0t3h0SKhHqyfGePBxz_B3NZpD+FDjLgmbpEQ@mail.gmail.com>
On Wed, 31 Jul 2013, David Herrmann wrote:
> >> This series provides some cleanups for HID transport drivers:
> >
> > Hi David,
> >
> > thanks a lot for your work, again.
> >
> > I have now applied all the patches from the series, except for:
> >
> > - 3/8, waiting for v2
>
> Ugh, patches 4-6 use the generic helper from 3 implicitly. So with the
> current tree, they will not be able to send any LED events. I think my
> conclusion in the discussion on #3 was wrong. I cannot rebase it on
> the end of the series. Sorry, I missed that.
>
> For v2, as mentioned in the thread, I cannot write a generic fallback
> for hid_hw_request() without patch #8. That's because the transport
> drivers handle the raw reports differently. #8 fixes that.
>
> So imho we should apply #3 as it is now. If we ever have a generic
> hid_hw_request(), we can just move the code from the LED handler to
> hid_hw_request_generic().
Bah, brainfart on my side as well. You are right. I am now pulling 3/8
into the tree as well and pushing it out.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC 0/8] HID: Transport Driver Cleanup
From: David Herrmann @ 2013-07-31 8:57 UTC (permalink / raw)
To: Jiri Kosina
Cc: open list:HID CORE LAYER, Benjamin Tissoires, Henrik Rydberg,
Oliver Neukum
In-Reply-To: <alpine.LNX.2.00.1307311035320.18809@pobox.suse.cz>
Hi Jiri
On Wed, Jul 31, 2013 at 10:38 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 15 Jul 2013, David Herrmann wrote:
>
>> This series provides some cleanups for HID transport drivers:
>
> Hi David,
>
> thanks a lot for your work, again.
>
> I have now applied all the patches from the series, except for:
>
> - 3/8, waiting for v2
Ugh, patches 4-6 use the generic helper from 3 implicitly. So with the
current tree, they will not be able to send any LED events. I think my
conclusion in the discussion on #3 was wrong. I cannot rebase it on
the end of the series. Sorry, I missed that.
For v2, as mentioned in the thread, I cannot write a generic fallback
for hid_hw_request() without patch #8. That's because the transport
drivers handle the raw reports differently. #8 fixes that.
So imho we should apply #3 as it is now. If we ever have a generic
hid_hw_request(), we can just move the code from the LED handler to
hid_hw_request_generic().
> - 7/8, waiting for v2 as well, plus haven't finished reviewing it fully
> - 8/8, still thinking about it and reviewing
They're both RFC so no hurry. Especially Patch #8 depends highly on
the ideas in #7 which will change according to Benjamin's comments in
v2.
Thanks for applying the first few fixes. Sorry for the confusion in
#3. I'd vote for applying it directly, Benjamin, what do you think?
I was busy the last few weeks, but I will have time to work on this
again starting next week.
Regards
David
^ permalink raw reply
* Re: [RFC 0/8] HID: Transport Driver Cleanup
From: Jiri Kosina @ 2013-07-31 8:38 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
On Mon, 15 Jul 2013, David Herrmann wrote:
> This series provides some cleanups for HID transport drivers:
Hi David,
thanks a lot for your work, again.
I have now applied all the patches from the series, except for:
- 3/8, waiting for v2
- 7/8, waiting for v2 as well, plus haven't finished reviewing it fully
- 8/8, still thinking about it and reviewing
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC 3/8] HID: input: generic hidinput_input_event handler
From: Jiri Kosina @ 2013-07-31 8:30 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, linux-input, Henrik Rydberg, Oliver Neukum
In-Reply-To: <CANq1E4QqBrM7AwO5b_T9eQRURTtNeq5b0DafXcbAVOucqdkD1g@mail.gmail.com>
On Wed, 17 Jul 2013, David Herrmann wrote:
> >> +
> >> + /* led_work is spawned by input_dev callbacks, but doesn't access the
> >> + * parent input_dev at all. Once all input devices are removed, we
> >> + * know that led_work will never get restarted, so we can cancel it
> >> + * synchronously and are safe. */
> >> + cancel_work_sync(&hid->led_work);
> >
> > You missed the multi-lines comment formatting style on this one :)
>
> The ./net/ subsystem uses these comments quite a lot and there was a
> discussion between davem and linus with the conclusion that these
> comments are ok. But I actually don't care, so I can change to normal
> CodingStyle.
I once got grilled by Dave for submitting patch to netdev with such
comment, but that didn't change my opinion, and I don't care for my
subsystem :)
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC 2/8] HID: usbhid: update LED fields unlocked
From: Jiri Kosina @ 2013-07-31 8:28 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: David Herrmann, linux-input, Henrik Rydberg, Oliver Neukum
In-Reply-To: <CAN+gG=GzgSKXarfDfL9pWVaR8Ayb7rr-my030M7iKu_TaDOPCg@mail.gmail.com>
On Tue, 16 Jul 2013, Benjamin Tissoires wrote:
> > Report fields can be updated from HID drivers unlocked via
> > hid_set_field(). It is protected by input_lock in HID core so only a
> > single input event is handled at a time. USBHID can thus update the field
> > unlocked and doesn't conflict with any HID vendor/device drivers. Note,
> > many HID drivers make heavy use of hid_set_field() in that way.
> >
> > But usbhid also schedules a work to gather multiple LED changes in a
> > single report. Hence, we used to lock the LED field update so the work can
> > read a consistent state. However, hid_set_field() only writes a single
> > integer field, which is guaranteed to be allocated all the time. So the
> > worst possible race-condition is a garbage read on the LED field.
> >
> > Therefore, there is no need to protect the update. In fact, the only thing
> > that is prevented by locking hid_set_field(), is an LED update while the
> > scheduled work currently writes an older LED update out. However, this
> > means, a new work is scheduled directly when the old one is done writing
> > the new state to the device. So we actually _win_ by not protecting the
> > write and allowing the write to be combined with the current write. A new
> > worker is still scheduled, but will not write any new state. So the LED
> > will not blink unnecessarily on the device.
> >
> > Assume we have the LED set to 0. Two request come in which enable the LED
> > and immediately disable it. The current situation with two CPUs would be:
> >
> > usb_hidinput_input_event() | hid_led()
> > ---------------------------------+----------------------------------
> > spin_lock(&usbhid->lock);
> > hid_set_field(1);
> > spin_unlock(&usbhid->lock);
> > schedule_work(...);
> > spin_lock(&usbhid->lock);
> > __usbhid_submit_report(..1..);
> > spin_unlock(&usbhid->lock);
> > spin_lock(&usbhid->lock);
> > hid_set_field(0);
> > spin_unlock(&usbhid->lock);
> > schedule_work(...);
> > spin_lock(&usbhid->lock);
> > __usbhid_submit_report(..0..);
> > spin_unlock(&usbhid->lock);
> >
> > With the locking removed, we _might_ end up with (look at the changed
> > __usbhid_submit_report() parameters in the first try!):
> >
> > usb_hidinput_input_event() | hid_led()
> > ---------------------------------+----------------------------------
> > hid_set_field(1);
> > schedule_work(...);
> > spin_lock(&usbhid->lock);
> > hid_set_field(0);
> > schedule_work(...);
> > __usbhid_submit_report(..0..);
> > spin_unlock(&usbhid->lock);
> >
> > ... next work ...
> >
> > spin_lock(&usbhid->lock);
> > __usbhid_submit_report(..0..);
> > spin_unlock(&usbhid->lock);
> >
> > As one can see, we no longer send the "LED ON" signal as it is disabled
> > immediately afterwards and the following "LED OFF" request overwrites the
> > pending "LED ON".
> >
> > It is important to note that hid_set_field() is not atomic, so we might
> > also end up with any other value. But that doesn't matter either as we
> > _always_ schedule the next work with a correct value and schedule_work()
> > acts as memory barrier, anyways. So in the worst case, we run
> > __usbhid_submit_report(..<garbage>..) in the first case and the following
> > __usbhid_submit_report() will write the correct value. But LED states are
> > booleans so any garbage will be converted to either 0 or 1 and the remote
> > device will never see invalid requests.
> >
> > Why all this? It avoids any custom locking around hid_set_field() in
> > usbhid and finally allows us to provide a generic hidinput_input_event()
> > handler for all HID transport drivers.
> >
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> > ---
>
> Hi David,
>
> that was a very good commit message!
>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I love this patch :) Thanks David, thanks Benjamin.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 0/2] HID special drivers converted to devres API
From: Jiri Kosina @ 2013-07-31 8:15 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Henrik Rydberg, Andy Shevchenko,
Stephane Chatty, linux-input, linux-kernel
In-Reply-To: <1374687485-31440-1-git-send-email-benjamin.tissoires@redhat.com>
On Wed, 24 Jul 2013, Benjamin Tissoires wrote:
> Hi guys,
>
> this is the v2 of this series.
Good stuff, thanks a lot. Applied now.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] hidraw: fix unproper mutex release
From: Jiri Kosina @ 2013-07-30 21:52 UTC (permalink / raw)
To: yonghua zheng; +Cc: linux-input
In-Reply-To: <CAMyfujd=SvO_Fod-rj6nztdreYJJmO9vnN8R9M6XKytvMQTpnw@mail.gmail.com>
On Tue, 30 Jul 2013, yonghua zheng wrote:
> Hi Jiri,
>
> We were seeing kernel crash while connect and disconnect a BT remote
> controller on Android system and kernel was crashing because of null
> pointer access in hidraw_open() in line 271 as follows:
Hi,
thanks a lot for the fix. I'd like to have it pushed to Linus for 3.11
still.
However you patch is missing Signed-off-by: line, as documented in
Documentation/SubmittingPatches.
Could you please provide that, so that I can apply your patch?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] drivers/input/mouse/elantech elantech driver for Lenovo L530 trackpoint
From: Ulrik De Bie @ 2013-07-30 21:44 UTC (permalink / raw)
To: linux-input@vger.kernel.org; +Cc: dmitry.torokhov@gmail.com
Hi,
I've a new work laptop Lenovo L530. For 3.2 and 3.9 kernel, the trackpoint
does not work out of the box. It gives sync errors as shown below when the trackpoint
or trackpoint mouse buttons are pressed and no input is received by userspace:
[ 29.010641] psmouse serio1: Touchpad at isa0060/serio1/input0 lost sync at byte 6
The touchpad does work.
The alternative is to do a downgrade to generic ps/2 mouse (modprobe psmouse proto=bare)
but this has the disadvantage that touchpad can't be disabled (I want trackpoint, not touchpad).
I did some analysis of the psmouse packets generated, and it became apparent that the
generated packets are according to a very strict format as described in the elantech_report_trackpoint
function in the patch below.
With this patch, the trackpoint is provided as another input device; currently called 'My stick'
The trackpoint now succesfully works and I can disable the touchpad with synclient TouchPadOff=1
The patch will also output messages that do not follow the expected pattern.
In the mean time I've seen 2 unknown packets occasionally:
0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
I don't know what those are for, but they can be safely ignored.
Currently all packets that are not known to v3 touchpad and where packet[3] (the fourth byte) lowest
nibble is 6 are now recognized as PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.
The first feedback I would appreciate on the patch:
1a) Do you think that the creation of the extra input device is the correct way to go ? I saw also a synaptics-pt but I was not able to figure it out and the extra input gave me a desirable result fast.
1b) What would be the requirements for the name and the phys parameter of the device ?
2) Is the patch correct with regards to ps/2 protocol semantics ? Should it be more restrictive; maybe limited to the 4 patterns used to dump unexpected packets ?
3) Would a 'trackpoint' detection be required ? I have no idea how to do this because I have a lack of elantech version/capabilities samples, I have just the one on my laptop:
psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.
4) Is there anyone else with different hardware/firmwareversion/synaptics capabilities where this patch also works ?
The patch below was ported to 3.10.4 kernel (originally made for 3.2 kernel which is the default kernel on my laptop)
Thanks for all feedback and your time,
Ulrik
diff -uprN -X linux-3.10.4-vanilla/Documentation/dontdiff linux-3.10.4-vanilla/drivers/input/mouse/elantech.c linux-3.10.4/drivers/input/mouse/elantech.c
--- linux-3.10.4-vanilla/drivers/input/mouse/elantech.c 2013-07-29 01:30:49.000000000 +0200
+++ linux-3.10.4/drivers/input/mouse/elantech.c 2013-07-30 23:06:12.000000000 +0200
@@ -402,6 +402,54 @@ static void elantech_report_absolute_v2(
input_sync(dev);
}
+static void elantech_report_trackpoint(struct psmouse *psmouse,
+ int packet_type)
+{
+ /* byte 0: 0 0 ~sx ~sy 0 M R L */
+ /* byte 1: sx 0 0 0 0 0 0 0 */
+ /* byte 2: sy 0 0 0 0 0 0 0 */
+ /* byte 3: 0 0 sy sx 0 1 1 0 */
+ /* byte 4: x7 x6 x5 x4 x3 x2 x1 x0 */
+ /* byte 5: y7 y6 y5 y4 y3 y2 y1 y0 */
+
+ /*
+ * x and y are written in two's complement spread
+ * over 9 bits with sx/sy the relative top bit and
+ * x7..x0 and y7..y0 the lower bits.
+ * The sign of y is opposite to what the input driver
+ * expects for a relative movement
+ */
+
+ struct elantech_data *etd = psmouse->private;
+ struct input_dev *dev2 = etd->dev2;
+ unsigned char *packet = psmouse->packet;
+ int x, y;
+ input_report_key(dev2, BTN_LEFT, packet[0] & 0x01);
+ input_report_key(dev2, BTN_RIGHT, packet[0] & 0x02);
+ input_report_key(dev2, BTN_MIDDLE, packet[0] & 0x04);
+ x = (s32) ((u32) ((packet[1] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+ packet[4]);
+ y = -(s32) ((u32) ((packet[2] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+ packet[5]);
+ input_report_rel(dev2, REL_X, x);
+ input_report_rel(dev2, REL_Y, y);
+ switch ((((u32) packet[0] & 0xF8) << 24) | ((u32) packet[1] << 16)
+ | (u32) packet[2] << 8 | (u32) packet[3]) {
+ case 0x00808036UL:
+ case 0x10008026UL:
+ case 0x20800016UL:
+ case 0x30000006UL:
+ break;
+ default:
+ /* Dump unexpected packet sequences if debug=1 (default) */
+ if (etd->debug == 1)
+ elantech_packet_dump(psmouse);
+ break;
+ }
+
+ input_sync(dev2);
+}
+
/*
* Interpret complete data packets and report absolute mode input events for
* hardware version 3. (12 byte packets for two fingers)
@@ -688,6 +736,8 @@ static int elantech_packet_check_v3(stru
if ((packet[0] & 0x0c) == 0x0c && (packet[3] & 0xce) == 0x0c)
return PACKET_V3_TAIL;
+ if ((packet[3]&0x0f) == 0x06)
+ return PACKET_TRACKPOINT;
return PACKET_UNKNOWN;
}
@@ -752,7 +802,10 @@ static psmouse_ret_t elantech_process_by
if (packet_type == PACKET_UNKNOWN)
return PSMOUSE_BAD_DATA;
- elantech_report_absolute_v3(psmouse, packet_type);
+ if (packet_type == PACKET_TRACKPOINT)
+ elantech_report_trackpoint(psmouse, packet_type);
+ else
+ elantech_report_absolute_v3(psmouse, packet_type);
break;
case 4:
@@ -1236,8 +1289,10 @@ int elantech_detect(struct psmouse *psmo
*/
static void elantech_disconnect(struct psmouse *psmouse)
{
+ struct elantech_data *etd = psmouse->private;
sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
&elantech_attr_group);
+ input_unregister_device(etd->dev2);
kfree(psmouse->private);
psmouse->private = NULL;
}
@@ -1323,10 +1378,15 @@ int elantech_init(struct psmouse *psmous
struct elantech_data *etd;
int i, error;
unsigned char param[3];
+ struct input_dev *dev2;
psmouse->private = etd = kzalloc(sizeof(struct elantech_data), GFP_KERNEL);
if (!etd)
return -ENOMEM;
+ dev2 = input_allocate_device();
+ if (!dev2)
+ goto init_fail;
+ etd->dev2 = dev2;
psmouse_reset(psmouse);
@@ -1386,9 +1446,26 @@ int elantech_init(struct psmouse *psmous
psmouse->reconnect = elantech_reconnect;
psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
+ snprintf(etd->phys, sizeof(etd->phys), "%s/input1",
+ psmouse->ps2dev.serio->phys);
+ dev2->phys = etd->phys;
+ dev2->name = "My stick";
+ dev2->id.bustype = BUS_I8042;
+ dev2->id.vendor = 0x0002;
+ dev2->id.product = PSMOUSE_ELANTECH;
+ dev2->id.version = 0x0000;
+ dev2->dev.parent = &psmouse->ps2dev.serio->dev;
+ dev2->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
+ dev2->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
+ dev2->keybit[BIT_WORD(BTN_LEFT)] =
+ BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
+
+ if (input_register_device(etd->dev2))
+ goto init_fail;
return 0;
init_fail:
+ input_free_device(dev2);
kfree(etd);
return -1;
}
diff -uprN -X linux-3.10.4-vanilla/Documentation/dontdiff linux-3.10.4-vanilla/drivers/input/mouse/elantech.h linux-3.10.4/drivers/input/mouse/elantech.h
--- linux-3.10.4-vanilla/drivers/input/mouse/elantech.h 2013-07-29 01:30:49.000000000 +0200
+++ linux-3.10.4/drivers/input/mouse/elantech.h 2013-07-30 21:14:09.000000000 +0200
@@ -94,6 +94,7 @@
#define PACKET_V4_HEAD 0x05
#define PACKET_V4_MOTION 0x06
#define PACKET_V4_STATUS 0x07
+#define PACKET_TRACKPOINT 0x08
/*
* track up to 5 fingers for v4 hardware
@@ -114,6 +115,8 @@ struct finger_pos {
};
struct elantech_data {
+ struct input_dev *dev2; /* Relative device */
+ char phys[32];
unsigned char reg_07;
unsigned char reg_10;
unsigned char reg_11;
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] hid: Fix Speedlink VAD Cezanne support for some devices
From: Jiri Kosina @ 2013-07-30 20:59 UTC (permalink / raw)
To: Stefan Kriwanek; +Cc: Dmitry Torokhov, linux-input
In-Reply-To: <51F7979C.4030306@stefankriwanek.de>
On Tue, 30 Jul 2013, Stefan Kriwanek wrote:
> I obtained a mouse device "Speedlink VAD Cezanne" that needs more
> aggressive fixing than already done in the driver. It, however, reports
> itself the same as the model hid-speedlink.c was written for.
>
> The patch applies to any kernel from 3.1 to current 3.11-rc3, since the
> file hasn't ever been touched since then.
>
> Please also note that I made sure through testing that this patch would
> not interfere with the proper working of a device that is bug-free: The
> driver drops EV_REL events with abs(val) >= 256, which are not
> achievable even on the highest laser resolution hardware setting. Hence
> I believe it is safe to also apply to the older kernels (that is, if
> your policy does allow that).
Hi Stefan,
thanks for the debugging effort and the patch. One minor thing ...
>
> Signed-off-by: Stefan Kriwanek <mail@stefankriwanek.de>
>
> ---
>
>
> --- linux-source-3.5.0/drivers/hid/hid-speedlink.c 2012-07-21 22:58:29.000000000 +0200
> +++ linux-source-3.5.0-speedlink/drivers/hid/hid-speedlink.c 2013-07-30 12:24:22.113302655 +0200
> @@ -3,7 +3,7 @@
> * Fixes "jumpy" cursor and removes nonexistent keyboard LEDS from
> * the HID descriptor.
> *
> - * Copyright (c) 2011 Stefan Kriwanek <mail@stefankriwanek.de>
> + * Copyright (c) 2011, 2013 Stefan Kriwanek <dev@stefankriwanek.de>
> */
>
> /*
> @@ -49,7 +49,7 @@ static int speedlink_event(struct hid_de
> {
> /* No other conditions due to usage_table. */
> /* Fix "jumpy" cursor (invalid events sent by device). */
> - if (value == 256)
> + if (abs(value) >= 256)
I think it'd be valuable to have brief description of why the more strict
condition is necessary (and correct at the same time) also in the code
comment preceeding the if(), not just in the patch changelog.
Could you please refresh your patch and resend?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] hid: Fix Speedlink VAD Cezanne support for some devices
From: Stefan Kriwanek @ 2013-07-30 10:38 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, linux-input
Hello,
I obtained a mouse device "Speedlink VAD Cezanne" that needs more aggressive fixing than already done in the driver. It, however, reports itself the same as the model hid-speedlink.c was written for.
The patch applies to any kernel from 3.1 to current 3.11-rc3, since the file hasn't ever been touched since then.
Please also note that I made sure through testing that this patch would not interfere with the proper working of a device that is bug-free: The driver drops EV_REL events with abs(val) >= 256, which are not achievable even on the highest laser resolution hardware setting. Hence I believe it is safe to also apply to the older kernels (that is, if your policy does allow that).
Signed-off-by: Stefan Kriwanek <mail@stefankriwanek.de>
---
--- linux-source-3.5.0/drivers/hid/hid-speedlink.c 2012-07-21 22:58:29.000000000 +0200
+++ linux-source-3.5.0-speedlink/drivers/hid/hid-speedlink.c 2013-07-30 12:24:22.113302655 +0200
@@ -3,7 +3,7 @@
* Fixes "jumpy" cursor and removes nonexistent keyboard LEDS from
* the HID descriptor.
*
- * Copyright (c) 2011 Stefan Kriwanek <mail@stefankriwanek.de>
+ * Copyright (c) 2011, 2013 Stefan Kriwanek <dev@stefankriwanek.de>
*/
/*
@@ -49,7 +49,7 @@ static int speedlink_event(struct hid_de
{
/* No other conditions due to usage_table. */
/* Fix "jumpy" cursor (invalid events sent by device). */
- if (value == 256)
+ if (abs(value) >= 256)
return 1;
/* Drop useless distance 0 events (on button clicks etc.) as well */
if (value == 0)
^ permalink raw reply
* Re: [PATCH 3/5] input.h: add keycodes for Fn Lock
From: Mauro Carvalho Chehab @ 2013-07-30 9:56 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <20130730071413.GA19291@core.coreip.homeip.net>
Em Tue, 30 Jul 2013 00:14:13 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
> On Mon, Jul 29, 2013 at 07:03:46AM -0300, Mauro Carvalho Chehab wrote:
> > Hi Dmitry,
> >
> > Em Sun, 28 Jul 2013 21:53:58 -0700
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
> >
> > > Hi Mauro,
> > >
> > > On Mon, Jul 29, 2013 at 12:59:37AM -0300, Mauro Carvalho Chehab wrote:
> > > > Samsung notebooks have a FN LOCK key. It works like CAPS LOCK or NUM
> > > > LOCK keys.
> > > >
> > > > When FN LOCK key is pressed, any further press to a key with a blue label
> > > > on it (Fn keys) will produce the alternate code.
> > > >
> > > > Another press makes the keyboard to return to its normal state.
> > > >
> > > > On the notebooks where such feature were found, a FN LOCK on event
> > > > produces scancode 0xa8, and a FN LOCK off event produces scancode 0xa9.
> > > >
> > > > Yet, it is better to reserve some space at the keymap to allow some
> > > > different implementation of this feature where the same keycode might
> > > > be used.
> > > >
> > > > Also, as this is actually a switch, add a switch indicator to report
> > > > when this switch is set/reset.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > > > ---
> > > > include/uapi/linux/input.h | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> > > > index d584047..4622c34 100644
> > > > --- a/include/uapi/linux/input.h
> > > > +++ b/include/uapi/linux/input.h
> > > > @@ -716,6 +716,10 @@ struct input_keymap_entry {
> > > > #define BTN_DPAD_LEFT 0x222
> > > > #define BTN_DPAD_RIGHT 0x223
> > > >
> > > > +#define KEY_FNLOCK_TOGGLE 0x224 /* Request switch Fn on or off */
> > > > +#define KEY_FNLOCK_ON 0x225
> > > > +#define KEY_FNLOCK_OFF 0x226
> > > > +
> > > > #define BTN_TRIGGER_HAPPY 0x2c0
> > > > #define BTN_TRIGGER_HAPPY1 0x2c0
> > > > #define BTN_TRIGGER_HAPPY2 0x2c1
> > > > @@ -853,6 +857,7 @@ struct input_keymap_entry {
> > > > #define SW_FRONT_PROXIMITY 0x0b /* set = front proximity sensor active */
> > > > #define SW_ROTATE_LOCK 0x0c /* set = rotate locked/disabled */
> > > > #define SW_LINEIN_INSERT 0x0d /* set = inserted */
> > > > +#define SW_FNLOCK 0x0e /* set = Fn locked */
> > >
> > > I am not sure if we need both the keys and the switch, so I would
> > > probably simply go with the keys, and not bother with switch. Then we do
> > > not need to touch the atkbd driver at all and rely on udev to set up the
> > > keymap and force release keys.
> >
> > The hole idea of doing those patches is to have an userspace tool that will
> > be able to show software LEDs, like mate-applet-lockeys, that will query
> > the input driver to know the current status. So, the better is to keep
> > control of it as soon as kernel starts controlling the Keyboard, as,
> > otherwise, the software LED indicators may be wrong.
> >
> > If you think that having both keycodes and a switch is an overkill, then
> > the better is to just keep the switch.
>
> Yes, we should have either the keys or the switch.
OK.
> I am a bit concerned with the behavior of this FN key and whether the state
> can be reported reliably. Have you tested the behavior of keyboard when you
> press the FN key before atkbd driver had a chance to bind to it?
If I press FN LOCK at grub2, or before that, it simply doesn't work.
I suspect that something at the atkbd initialization (or, more likely, at ACPI
initialization) makes the BIOS to enable it.
I did a quick inspection at ACPI DSDT table, but I wasn't able to discover
anything there. The BIOS don't have explicit support for Linux.
> What
> about suspending with FN engaged and then resuming? Suspend-to-disk
> behavior?
I tested calling both pm-suspend and pm-hibernate here: Fn Lock state was
recovered properly. On a normal reboot, Fn Lock behavior resets.
So, it seems that there's something already in resume code that restores
Fn Lock to the state before suspend.
Regards,
Mauro
^ permalink raw reply
* [PATCH 25/35] Input: touchscreen: use dev_get_platdata()
From: Jingoo Han @ 2013-07-30 8:15 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, 'Jingoo Han'
Use the wrapper function for retrieving the platform data instead of
accessing dev->platform_data directly.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/joystick/as5011.c | 2 +-
drivers/input/joystick/maplecontrol.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/joystick/as5011.c b/drivers/input/joystick/as5011.c
index 121cd63..13eba2a 100644
--- a/drivers/input/joystick/as5011.c
+++ b/drivers/input/joystick/as5011.c
@@ -234,7 +234,7 @@ static int as5011_probe(struct i2c_client *client,
int irq;
int error;
- plat_data = client->dev.platform_data;
+ plat_data = dev_get_platdata(&client->dev);
if (!plat_data)
return -EINVAL;
diff --git a/drivers/input/joystick/maplecontrol.c b/drivers/input/joystick/maplecontrol.c
index 59c10ec..8aa6e4c 100644
--- a/drivers/input/joystick/maplecontrol.c
+++ b/drivers/input/joystick/maplecontrol.c
@@ -61,7 +61,7 @@ static void dc_pad_callback(struct mapleq *mq)
static int dc_pad_open(struct input_dev *dev)
{
- struct dc_pad *pad = dev->dev.platform_data;
+ struct dc_pad *pad = dev_get_platdata(&dev->dev);
maple_getcond_callback(pad->mdev, dc_pad_callback, HZ/20,
MAPLE_FUNC_CONTROLLER);
@@ -71,7 +71,7 @@ static int dc_pad_open(struct input_dev *dev)
static void dc_pad_close(struct input_dev *dev)
{
- struct dc_pad *pad = dev->dev.platform_data;
+ struct dc_pad *pad = dev_get_platdata(&dev->dev);
maple_getcond_callback(pad->mdev, dc_pad_callback, 0,
MAPLE_FUNC_CONTROLLER);
--
1.7.10.4
^ permalink raw reply related
* [PATCH 24/35] Input: mouse: use dev_get_platdata()
From: Jingoo Han @ 2013-07-30 8:15 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, 'Jingoo Han'
Use the wrapper function for retrieving the platform data instead of
accessing dev->platform_data directly.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/mouse/gpio_mouse.c | 2 +-
drivers/input/mouse/pxa930_trkball.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/gpio_mouse.c b/drivers/input/mouse/gpio_mouse.c
index 6b44413..6810a46 100644
--- a/drivers/input/mouse/gpio_mouse.c
+++ b/drivers/input/mouse/gpio_mouse.c
@@ -48,7 +48,7 @@ static void gpio_mouse_scan(struct input_polled_dev *dev)
static int gpio_mouse_probe(struct platform_device *pdev)
{
- struct gpio_mouse_platform_data *pdata = pdev->dev.platform_data;
+ struct gpio_mouse_platform_data *pdata = dev_get_platdata(&pdev->dev);
struct input_polled_dev *input_poll;
struct input_dev *input;
int pin, i;
diff --git a/drivers/input/mouse/pxa930_trkball.c b/drivers/input/mouse/pxa930_trkball.c
index 0ecb9e7..d20d2ae 100644
--- a/drivers/input/mouse/pxa930_trkball.c
+++ b/drivers/input/mouse/pxa930_trkball.c
@@ -166,7 +166,7 @@ static int pxa930_trkball_probe(struct platform_device *pdev)
if (!trkball)
return -ENOMEM;
- trkball->pdata = pdev->dev.platform_data;
+ trkball->pdata = dev_get_platdata(&pdev->dev);
if (!trkball->pdata) {
dev_err(&pdev->dev, "no platform data defined\n");
error = -EINVAL;
--
1.7.10.4
^ permalink raw reply related
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