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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>> ---
>>
>> 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@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
