[PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph

[PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph

[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

[PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Andy Shevchenko]

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

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Benjamin Tissoires]

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

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Andy Shevchenko]

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

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Benjamin Tissoires]

On 02/08/13 16:49, Andy Shevchenko wrote:
> 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).

Yes, and this is normal:
I want to be able to ask people to boot with i2c_hid.debug=1, and then
retrieve their debug traces.
And the one which is not guarded by the debug condition is a message
that will appear only once at boot (so it will not spam the debug output
-- which is not the case by the others).

> 
> 
>> 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(...)
> 
> ?

That's because the previous code was directly using
dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.

This call does not use dynamic debugging or does not get compiled out if
DEBUG is not set.

Also, to my defence, I can add that I developed this driver on an ARM
board without dynamic debugging support enabled... :)

> 
> 
>> 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.

Ok, so if you can guarantee me that adding the proper kernel parameter
will allow me to retrieve all the i2c-hid logs from the boot, then I'd
be happy to ack this. However, I have no way to test this right now, so
I'll need to trust you (that's why I'm asking you to do proper testing).

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
> 

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Andy Shevchenko]

On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote: 
> On 02/08/13 16:49, Andy Shevchenko wrote:
> > On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote: 

[]

> >> 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(...)
> > 
> > ?
> 
> That's because the previous code was directly using
> dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.
> 
> This call does not use dynamic debugging or does not get compiled out if
> DEBUG is not set.

Yeah, I've checked this just after I sent the message.

> Also, to my defence, I can add that I developed this driver on an ARM
> board without dynamic debugging support enabled... :)


So, the problem is DYNAMIC_DEBUG is not default option in kernel (and I
could imagine why).

I checked few distros for that: Debian/Ubuntu - off,
Fedora/OpenSuse/ArchLinux - on.

Just to understand what kind of user might run this with debug? Is it
developer or person who knows the subject? If it's not a such person, it
would be difficult  to him or her to enable dynamic debug in distros
like Ubuntu, which sucks.

> >> 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.
> 
> Ok, so if you can guarantee me that adding the proper kernel parameter
> will allow me to retrieve all the i2c-hid logs from the boot, then I'd
> be happy to ack this. However, I have no way to test this right now, so
> I'll need to trust you (that's why I'm asking you to do proper testing).

With only one condition if dynamic debug is enabled in kernel.
So, it would be nice to gather opinions, however, the decision is
totally depends on type of user who wants to debug the module.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Benjamin Tissoires]

On 02/08/13 20:31, Andy Shevchenko wrote:
> On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote: 
>> On 02/08/13 16:49, Andy Shevchenko wrote:
>>> On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote: 
> 
> []
> 
>>>> 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(...)
>>>
>>> ?
>>
>> That's because the previous code was directly using
>> dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.
>>
>> This call does not use dynamic debugging or does not get compiled out if
>> DEBUG is not set.
> 
> Yeah, I've checked this just after I sent the message.
> 
>> Also, to my defence, I can add that I developed this driver on an ARM
>> board without dynamic debugging support enabled... :)
> 
> 
> So, the problem is DYNAMIC_DEBUG is not default option in kernel (and I
> could imagine why).
> 
> I checked few distros for that: Debian/Ubuntu - off,
> Fedora/OpenSuse/ArchLinux - on.
> 
> Just to understand what kind of user might run this with debug? Is it
> developer or person who knows the subject? If it's not a such person, it
> would be difficult  to him or her to enable dynamic debug in distros
> like Ubuntu, which sucks.

I'm afraid people reporting this kind of bugs are most of the time
regular users, who just bought a brand new laptop, and who send a bug
report through their distribution's bugzilla... :(
I'm not expecting that much of developers or person who knows the
subject to actually report the problem, because most of the time a
normal user reports "my touchpad is not working". At least, that's what
I have mostly seen in the input subsystem.

> 
>>>> 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.
>>
>> Ok, so if you can guarantee me that adding the proper kernel parameter
>> will allow me to retrieve all the i2c-hid logs from the boot, then I'd
>> be happy to ack this. However, I have no way to test this right now, so
>> I'll need to trust you (that's why I'm asking you to do proper testing).
> 
> With only one condition if dynamic debug is enabled in kernel.
> So, it would be nice to gather opinions, however, the decision is
> totally depends on type of user who wants to debug the module.
> 

Yep, I agree that Jiri's opinion would be helpful.

Meanwhile, we could also wait a little for more i2c-hid to hit the
market and to be widely tested, and then remove the debug flag. We
should also remove the debug events in get_i2c_hid_get_input() which
would pollute systems without dynamic debugging but with the DEBUG
config still enabled.

Cheers,
Benjamin

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Jiri Kosina]

On Fri, 2 Aug 2013, Benjamin Tissoires wrote:

> > With only one condition if dynamic debug is enabled in kernel.
> > So, it would be nice to gather opinions, however, the decision is
> > totally depends on type of user who wants to debug the module.
> > 
> Yep, I agree that Jiri's opinion would be helpful.
> 
> Meanwhile, we could also wait a little for more i2c-hid to hit the
> market and to be widely tested, and then remove the debug flag. We
> should also remove the debug events in get_i2c_hid_get_input() which
> would pollute systems without dynamic debugging but with the DEBUG
> config still enabled.

Well, it doesn't seem to make too much sense to me to have generic/bus use 
hid_dbg() (although it's used very rarely) and a "sub-driver" use 
something else.

Basically the options I see:

- keep i2c-hid as is
- convert the whole drivers/hid to dev_dbg()
- introduce another hid debugfs file in parallel to rdesc and events for 
  these types of messages

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Andy Shevchenko]

On Mon, 2013-08-05 at 11:26 +0200, Jiri Kosina wrote: 
> On Fri, 2 Aug 2013, Benjamin Tissoires wrote:
> 
> > > With only one condition if dynamic debug is enabled in kernel.
> > > So, it would be nice to gather opinions, however, the decision is
> > > totally depends on type of user who wants to debug the module.
> > > 
> > Yep, I agree that Jiri's opinion would be helpful.
> > 
> > Meanwhile, we could also wait a little for more i2c-hid to hit the
> > market and to be widely tested, and then remove the debug flag. We
> > should also remove the debug events in get_i2c_hid_get_input() which
> > would pollute systems without dynamic debugging but with the DEBUG
> > config still enabled.
> 
> Well, it doesn't seem to make too much sense to me to have generic/bus use 
> hid_dbg() (although it's used very rarely) and a "sub-driver" use 
> something else.
> 
> Basically the options I see:
> 
> - keep i2c-hid as is
> - convert the whole drivers/hid to dev_dbg()
> - introduce another hid debugfs file in parallel to rdesc and events for 
>   these types of messages

Debugfs could be off in the kernel configuration. Whatever we choose
there will be a kernel that has a disabled option which prevents to
debug: either debugging is turned off, or driver itself.


There is another possibility as well:
instead of dev_dbg() we may add trace points and subscribe to them from
userspace via perf, for example.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Andy Shevchenko]

On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote: 
> On 02/08/13 16:49, Andy Shevchenko wrote:

> > 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.
> 
> Ok, so if you can guarantee me that adding the proper kernel parameter
> will allow me to retrieve all the i2c-hid logs from the boot, then I'd
> be happy to ack this. However, I have no way to test this right now, so
> I'll need to trust you (that's why I'm asking you to do proper testing).

Just to your information, I noticed that Greg KH did something like I
propose for usb drivers (misc, serial, ...).

So, it would be considered as third party opinion, I guess.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph

[Benjamin Tissoires]

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

Re: [PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph

[Jiri Kosina]

On Fri, 2 Aug 2013, Benjamin Tissoires wrote:

> <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>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs