On Fri, Aug 19, 2011 at 09:01:53AM -0400, J, KEERTHY wrote: > On Fri, Aug 19, 2011 at 11:47 AM, Guenter Roeck > <guenter.roeck@xxxxxxxxxxxx> wrote: > > On Fri, Aug 19, 2011 at 02:04:58AM -0400, J, KEERTHY wrote: > >> On Fri, Aug 19, 2011 at 7:43 AM, Guenter Roeck > >> <guenter.roeck@xxxxxxxxxxxx> wrote: > >> > On Thu, Aug 18, 2011 at 06:52:15AM -0400, Keerthy wrote: > >> >> On chip temperature sensor driver. The driver monitors the temperature of > >> >> the MPU subsystem of the OMAP4. It sends notifications to the user space if > >> >> the temperature crosses user defined thresholds via kobject_uevent interface. > >> >> The user is allowed to configure the temperature thresholds vis sysfs nodes > >> >> exposed using hwmon interface. > >> >> > >> >> Signed-off-by: Keerthy <j-keerthy@xxxxxx> > >> >> Cc: Jean Delvare <khali@xxxxxxxxxxxx> > >> >> Cc: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> > >> >> Cc: lm-sensors@xxxxxxxxxxxxxx > >> > > >> > High level review: > >> > > >> > - too much and too broad mutex locking. show functions should not need locks at all, > >> > set functions only while data is written into registers and into platform data. > >> > >> Ok. I will clean this. > >> > >> > - driver is quite noisy. There should definitely not be any log messages > >> > if a set parameter is wrong. Show functions already return an error value > >> > to the user; a log message indicating the error again just creates noise. > >> > For one boolean set during probe (is_efuse_valid), each subsequent show results > >> > in a log message if it is false. Some errors result in multiple log messages. > >> > >> A user tries to set an invalid temperature threshold. The user should > >> be notified about this. The invalid temperature will not be set. The user > >> should not be allowed to set an invalid temperature. It is to inform > >> the user about precisely the problem with the parameter. > >> > > User is notified with -EINVAL. Unless on the console, which is unlikely, > > the user will likely not notice a message in the kernel log. > > These messages on console are useful for debug purpose > so can i place it under dev_dbg? Only on explicit enabling > these messages can be seen. > Not really for user errors. The user is already alerted with EINVAL that the parameter was invalid. _Why_ it was invalid should be well defined. Are you really sure you want to have something like "user entered 'Hello' as temperature" even in a debug log ? The debug log should be to debug driver problems and behavior, not to debug wrong parameters entered by users. > > > >> In some of the samples the bandgap is not trimmed and hence > >> temperature reported will be wrong. So every time a user tries to read > >> he is alerted that the temperatures are not accurate. > >> > > In the kernel log ? Sorry, that doesn't make sense. You alert the system administrator, > > not the user. > > Ok. Can this be a dev_dbg message? Only for debug purpose > this can be useful. > Yes, but even then you need the message only once. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
