On Thu, 2011-08-25 at 12:39 -0400, J, KEERTHY wrote:
> On Thu, Aug 25, 2011 at 9:49 PM, Guenter Roeck
> <guenter.roeck@xxxxxxxxxxxx> wrote:
> > On Thu, 2011-08-25 at 12:04 -0400, J, KEERTHY wrote:
> >> On Thu, Aug 25, 2011 at 7:36 PM, Guenter Roeck
> >> <guenter.roeck@xxxxxxxxxxxx> wrote:
> >> > On Thu, Aug 25, 2011 at 06:30:07AM -0400, J, KEERTHY wrote:
> >> >> On Wed, Aug 24, 2011 at 10:46 PM, Guenter Roeck
> >> >> <guenter.roeck@xxxxxxxxxxxx> wrote:
> >> >> > On Wed, Aug 24, 2011 at 10:37:12AM -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
> >> >> >
> >> >> > Partial review only. Too many concerns, and after a while I tend to loose track.
> >> >> >
> >> >> >> ---
> >> >> >> Documentation/hwmon/omap_temp_sensor | 27 +
> >> >> >> drivers/hwmon/Kconfig | 11 +
> >> >> >> drivers/hwmon/Makefile | 1 +
> >> >> >> drivers/hwmon/omap_temp_sensor.c | 933 ++++++++++++++++++++++++++++++++++
> >> >> >> 4 files changed, 972 insertions(+), 0 deletions(-)
> >> >> >> create mode 100644 Documentation/hwmon/omap_temp_sensor
> >> >> >> create mode 100644 drivers/hwmon/omap_temp_sensor.c
> >> >> >>
> >> >
> >> > [ ... ]
> >> >
> >> >> >> +/* Sysfs hook functions */
> >> >> >> +
> >> >> >> +static ssize_t show_temp_max(struct device *dev,
> >> >> >> + struct device_attribute *devattr, char *buf)
> >> >> >> +{
> >> >> >> + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev);
> >> >> >> + int temp;
> >> >> >> +
> >> >> >> + temp = omap_temp_sensor_readl(temp_sensor,
> >> >> >> + temp_sensor->registers->bgap_threshold);
> >> >> >> + temp = (temp & temp_sensor->registers->threshold_thot_mask)
> >> >> >> + >> __ffs(temp_sensor->registers->threshold_thot_mask);
> >> >> >> +
> >> >> >> + if (temp < OMAP_ADC_START_VALUE || temp > OMAP_ADC_END_VALUE) {
> >> >> >> + dev_err(dev, "invalid value\n");
> >> >> >> + return -EDOM;
> >> >> >
> >> >> > Please don't use EDOM, and drop the error message.
> >> >>
> >> >> The temperature is out of range. Should i use EIO?
> >> >>
> >> > It is out of range, but not due to a math error, but due to a bad reading from the chip
> >> > or due to a chip failure. EIO is ok.
> >>
> >> Ok
> >>
> >> >
> >> >> >
> >> >> >> + }
> >> >> >> +
> >> >> >> + temp = adc_to_temp_conversion(temp);
> >> >> >> +
> >> >> >> + return snprintf(buf, 16, "%d\n", temp);
> >> >> >> +}
> >> >> >> +
> >> >> >> +static ssize_t set_temp_max(struct device *dev,
> >> >> >> + struct device_attribute *devattr,
> >> >> >> + const char *buf, size_t count)
> >> >> >> +{
> >> >> >> + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev);
> >> >> >> + long val;
> >> >> >> + u32 reg_val, t_cold, t_hot, temp;
> >> >> >> +
> >> >> >> + if (strict_strtol(buf, 10, &val)) {
> >> >> >> + count = -EINVAL;
> >> >> >> + goto out;
> >> >> >
> >> >> > This will try to release the not-acquired lock.
> >> >>
> >> >> I will correct this.
> >> >>
> >> >> >
> >> >> >> + }
> >> >> >> +
> >> >> >> + t_hot = temp_to_adc_conversion(val);
> >> >> >
> >> >> > Bad error return check. Negative on error. Should be
> >> >> > if (t_hot < 0)
> >> >> > return t_hot;
> >> >>
> >> >> Ok.
> >> >>
> >> >> >
> >> >> >> + if ((t_hot < OMAP_ADC_START_VALUE || t_hot > OMAP_ADC_END_VALUE)) {
> >> >> >
> >> >> > Unnecessary (( ))
> >> >>
> >> >> This check is unnecessary with the negative check. I will remove.
> >> >>
> >> >> >
> >> >> >> + count = -EINVAL;
> >> >> >> + goto out;
> >> >> >
> >> >> > This will try to release the not-acquired lock.
> >> >>
> >> >> I will correct this.
> >> >>
> >> >> >
> >> >> >> + }
> >> >> >> +
> >> >> >> + mutex_lock(&temp_sensor->sensor_mutex);
> >> >> >> + /* obtain the T cold value */
> >> >> >> + t_cold = omap_temp_sensor_readl(temp_sensor,
> >> >> >> + temp_sensor->registers->bgap_threshold);
> >> >> >> + t_cold = (t_cold & temp_sensor->registers->threshold_tcold_mask) >>
> >> >> >> + __ffs(temp_sensor->registers->threshold_tcold_mask);
> >> >> >> +
> >> >> >> + if (t_hot < t_cold) {
> >> >> >> + count = -EINVAL;
> >> >> >> + goto out;
> >> >> >
> >> >> > Context specific limitations like this one are not a good idea, since it assumes an order of changing max
> >> >> > and max_hysteresis which is not necessarily guaranteed by the application making the change, nor is a
> >> >> > change order order well defined or even possible. Applications should not have to bother about this.
> >> >>
> >> >> The hardware behavior is like this. As long as the temperature is below
> >> >> t_hot no interrupts are fired. Once the temperature increases above
> >> >> t_hot we get an
> >> >> interrupt. In order to avoid repeated interrupts we mask the t_hot interrupt
> >> >> in the interrupt handler and enable the t_cold interrupts. So we get a t_cold
> >> >> interrupt only when the temperature drops below the t_cold value. Hence
> >> >> setting the t_cold higher than t_hot will mess up the state machine.
> >> >>
> >> >> t_hot value should be greater than t_cold value.
> >> >>
> >> > One option would be to update t_cold (max_hyst) automatically in that case.
> >>
> >> updating the t_cold without application's knowledge?
> >>
> > Why not ? Other drivers do the same. Happens all the time. Better than
> > returning an error and leaving the application wondering what is wrong.
> >
> >> >
> >> > Problem here is that you expect the aplication to know, depending on the current
> >> > values of (max, max_hyst), how to update both limits in order. That is not a reasonable
> >> > expectation.
> >>
> >> The thermal application which is configuring the thresholds must be
> >> intelligent enough to configure the thresholds in the right order. Configuring
> >> the thresholds wrongly and missing the interrupt may even result in burning
> >> the chip.
> >>
> >
> > Applications are generic, and _can not_ be expected to know chip
> > specific requirements such as order of initialization. What is needed
> > for your chip may be completely different for another chip, so you have
> > to make sure that _your driver_ prevents critical situations from
> > happening. You can not blame applications for not knowing the oddities
> > of your chips.
> >
> >> >
> >> >> >
> >> >> >> + }
> >> >> >> +
> >> >> >> + /* write the new t_hot value */
> >> >> >> + reg_val = omap_temp_sensor_readl(temp_sensor,
> >> >> >> + temp_sensor->registers->bgap_threshold);
> >> >> >> + reg_val &= ~temp_sensor->registers->threshold_thot_mask;
> >> >> >> + reg_val |= (t_hot <<
> >> >> >> + __ffs(temp_sensor->registers->threshold_thot_mask));
> >> >> >> + omap_temp_sensor_writel(temp_sensor, reg_val,
> >> >> >> + temp_sensor->registers->bgap_threshold);
> >> >> >> +
> >> >> >> + /* Read the current temperature */
> >> >> >> + temp = omap_temp_sensor_readl(temp_sensor,
> >> >> >> + temp_sensor->registers->temp_sensor_ctrl);
> >> >> >> + temp &= temp_sensor->registers->bgap_dtemp_mask;
> >> >> >> +
> >> >> >> + /*
> >> >> >> + * If user sets the HIGH threshold(t_hot) greater than the current
> >> >> >> + * temperature(temp) unmask the HOT interrupts
> >> >> >> + */
> >> >> >> + if (t_hot > temp) {
> >> >> >> + reg_val = omap_temp_sensor_readl(temp_sensor,
> >> >> >> + temp_sensor->registers->bgap_mask_ctrl);
> >> >> >> + reg_val &= ~temp_sensor->registers->mask_cold_mask;
> >> >> >> + reg_val |= temp_sensor->registers->mask_hot_mask;
> >> >> >> + omap_temp_sensor_writel(temp_sensor, reg_val,
> >> >> >> + temp_sensor->registers->bgap_mask_ctrl);
> >> >> >> + }
> >> >> >> +
> >> >> >> + /*
> >> >> >> + * If current temperature is in-between the hot and cold thresholds,
> >> >> >> + * Enable both masks.
> >> >> >> + */
> >> >> >> + if (temp > t_cold && temp < t_hot) {
> >> >> >> + reg_val = omap_temp_sensor_readl(temp_sensor,
> >> >> >> + temp_sensor->registers->bgap_mask_ctrl);
> >> >> >> + reg_val |= temp_sensor->registers->mask_cold_mask;
> >> >> >> + reg_val |= temp_sensor->registers->mask_hot_mask;
> >> >> >> + omap_temp_sensor_writel(temp_sensor, reg_val,
> >> >> >> + OMAP4460_BGAP_CTRL_OFFSET);
> >> >> >
> >> >> > Why use OMAP4460_BGAP_CTRL_OFFSET here but bgap_mask_ctrl elsewhere ?
> >> >>
> >> >> Ok. I will change this.
> >> >>
> >> >> >
> >> >> >> + }
> >> >> >> + /*
> >> >> >> + * else no need to do anything since HW will immediately compare
> >> >> >> + * the new threshold and generate interrupt accordingly
> >> >> >> + */
> >> >> >> +out:
> >> >> >> + mutex_unlock(&temp_sensor->sensor_mutex);
> >> >> >> +
> >> >> >> + return count;
> >> >> >> +}
> >> >> >> +
> >> >> >> +static ssize_t show_temp_max_hyst(struct device *dev,
> >> >> >> + struct device_attribute *devattr, char *buf)
> >> >> >> +{
> >> >> >> + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev);
> >> >> >> + u32 temp;
> >> >> >> +
> >> >> >> + mutex_lock(&temp_sensor->sensor_mutex);
> >> >> >
> >> >> > This lock would only make sense if threshold_tcold_mask can change, and if that change
> >> >> > would be protected by the mutex. I don't see that to be the case, thus the lock is unnecessary.
> >> >> > Besides, the code in show_temp_max() is almost the same and does not use the lock.
> >> >>
> >> >> The masks are getting changed in show_temp_max() show_temp_max_hyst() and the
> >> >> ISR function. It is better to protect with mutex. I will add that in
> >> >> required places.
> >> >>
> >> > Reeally ? I seem to be blind. I searched for threshold_tcold_mask in the patch but
> >> > did not see where it is changed. Can you point me to the code ?
> >>
> >> I misread it. Yes threshold_tcold_mask is used only in show_temp_max_hyst(),
> >> set_temp_max(), set_temp_max_hyst() functions. It is set only in the
> >> set_temp_max_hyst()
> >> function.
> >>
> > I am still lost. Where exactly ? I don't see it being changed in
> > set_temp_max_hyst() either. Outside initialization code, I only see
> > writes to
> > bgap_threshold
>
> bgap_threshold is the register and "threshold_tcold_mask" is a bit field
> in that register which is written in the set_temp_max_hyst() function to
> configure temp_max_hyst.
> Similarly "threshold_thot_mask" is a bit field in the bgap_threshold
> register which is written in the set_temp_max() function to configure
> temp_max.
Sure, but the mask itself is not changed, which would be the only reason
for needing the mutex. Just writing/reading a single register does not
require a mutex, since a single read is atomic, and it doesn't matter if
the reader gets the new or the old value.
It would be different if the mask was changed, or if you would need to
read a sequence of registers depending on each other.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
