On 02/08/13 20:31, Andy Shevchenko wrote:

On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote: 

quoted

On 02/08/13 16:49, Andy Shevchenko wrote:

quoted

On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote: 

[]

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

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