Re: [RFC PATCH 6/6 V2] hwmon: OMAP4: On die temperature sensor driver

[Guenter Roeck <guenter.roeck@ericsson.com>]

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@ericsson.com> 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@ericsson.com> 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@ti.com>
> >> >> Cc: Jean Delvare <khali@linux-fr.org>
> >> >> Cc: Guenter Roeck <guenter.roeck@ericsson.com>
> >> >> Cc: lm-sensors@lm-sensors.org
> >> >
> >> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html