On 3/17/20 10:28 PM, Alexandru Lazar wrote:
Hi Lars,
Thank you very much for your comments! I'll send a version with the
fixes in a day or two (ar as soon as there's no more feedback, anyways)
-- in the meantime I have a question about this one:
+ if (!adc->shdn)
+ dev_info(&spi->dev, "no shdn pin passed, low-power mode disabled");
+ else
+ dev_info(&spi->dev, "shdn pin passed, low-power mode enabled");
I can see how these message above are useful during development, but I'd
remove them or turn them into dev_dbg() for the "production" version of the
driver. Imagine every driver printed something like this, there would be a
lot of spam in the bootlog.
I thought this should go under info, rather than debug, because it's
(possibly) relevant runtime information. It doesn't require any action,
but it's something that a user of this driver may want to be aware
of. The timing (and power consumption, of course) in low-power mode are
different. It's akin to e.g.:
./at91_adc.c:782: dev_info(&idev->dev, "Resolution used: %u bits\n", st->res);
./at91_adc.c:842: dev_info(dev, "ADC Touch screen is disabled.\n");
./at91_adc.c:955: dev_info(&idev->dev, "not support touchscreen in the adc compatible string.\n");
or:
./ti-ads124s08.c:320: dev_info(&spi->dev, "Reset GPIO not defined\n");
which is why I figured I'd do the same in my code (what can I say, I
wear my code monkey badge with pride!).
Needless to say, since you've seen more IIO subsystem drivers than I've
seen, I totally trust your assessment of whether this is debug or info
more than I trust mine. If this was a false positive on your "looks like
a leftover debug message", let me know (and while I'm at it I might make
the message more useful/friendly...). Otherwise it'll get bumped down to
dev_dbg in my next revision.
If I was to write this driver I would not make it dev_info(). In my
opinion drivers should only print essential information during probe,
like when something goes wrong. Otherwise the boot log gets very noisy
quickly.
- Lars
