hwmon: OMAP4: On die temperature sensor driver
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>
Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 15 lines
--- 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.cDocumentation/hwmon/omap_temp_sensor new file +0
-0
Documentation/hwmon/omap_temp_sensor line 8
Quoted 8 lines
Documentation/hwmon/omap_temp_sensor new file +8
-0@@ -0,0 +1,27 @@ +Kernel driver omap_temp_sensor +=============== + +Supported chips: + * Texas Instruments OMAP4460 + Prefix: 'omap_temp_sensor' + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp102.html +
I am a bit lost here. What does the TMP102 have to do with the OMAP4460 ?
Quoted 6 lines
Documentation/hwmon/omap_temp_sensor new file +6
-0+Author: + J Keerthy <[email protected]> + +Description +----------- +
Documentation/hwmon/omap_temp_sensor line 24
Quoted 10 lines
Documentation/hwmon/omap_temp_sensor new file +10
-0+The Texas Instruments OMAP4 family of chips have a bandgap temperature sensor. +The temperature sensor feature is used to convert the temperature of the device +into a decimal value coded on 10 bits. An internal ADC is used for conversion. +The recommended operating temperatures must be in the range -40 degree Celsius +to 123 degree celsius for standard conversion. +The thresholds are programmable and upon crossing the thresholds an interrupt +is generated. The OMAP temperature sensor has a programmable update rate in +milli seconds. +(Currently the driver programs a default of 2000 milli seconds). +
s/milli seconds/milliseconds/
Quoted 125 lines
Documentation/hwmon/omap_temp_sensor new file +3
-0+The driver provides the common sysfs-interface for temperatures (see +Documentation/hwmon/sysfs-interface under Temperatures). +drivers/hwmon/Kconfig +11
-0@@ -323,6 +323,17 @@ config SENSORS_F71805F This driver can also be built as a module. If so, the module will be called f71805f. +config SENSORS_OMAP_BANDGAP_TEMP_SENSOR + bool "OMAP on-die temperature sensor hwmon driver" + depends on HWMON && ARCH_OMAP && OMAP_TEMP_SENSOR + help + If you say yes here you get support for hardware + monitoring features of the OMAP on die temperature + sensor. + + Continuous conversion programmable delay + mode is used for temperature conversion. + config SENSORS_F71882FG tristate "Fintek F71882FG and compatibles" helpdrivers/hwmon/Makefile +1
-0@@ -91,6 +91,7 @@ obj-$(CONFIG_SENSORS_MAX6639) += max6639.o obj-$(CONFIG_SENSORS_MAX6642) += max6642.o obj-$(CONFIG_SENSORS_MAX6650) += max6650.o obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o +obj-$(CONFIG_SENSORS_OMAP_BANDGAP_TEMP_SENSOR) += omap_temp_sensor.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.odrivers/hwmon/omap_temp_sensor.c new file +81
-0@@ -0,0 +1,933 @@ +/* +* OMAP4 Temperature sensor driver file +* +* Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ +* Author: J Keerthy <[email protected]> +* Author: Moiz Sonasath <[email protected]> +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License +* version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA +* 02110-1301 USA +* +*/ + +#include <linux/interrupt.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/init.h> +#include <plat/omap_device.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <linux/jiffies.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/stddef.h> +#include <linux/sysfs.h> +#include <linux/err.h> +#include <linux/types.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/delay.h> +#include <plat/temperature_sensor.h> + +#define TSHUT_HOT 920/* 122 deg C */ +#define TSHUT_COLD 866/* 100 deg C */ +#define T_HOT 800/* 73 deg C */ +#define T_COLD 795/* 71 deg C */ +#define OMAP_ADC_START_VALUE 530 +#define OMAP_ADC_END_VALUE 923 +#define MAX_FREQ 2000000 +#define MIN_FREQ 1000000 + +/* +* omap_temp_sensor structure +* @hwmon_dev - device pointer +* @clock - Clock pointer +* @registers - Pointer to structure with register offsets and bitfields +* @sensor_mutex - Mutex for sysfs, irq and PM +* @irq - MPU Irq number for thermal alert +* @phy_base - Physical base of the temp I/O +* @clk_rate - Holds current clock rate +* @temp_sensor_ctrl - temp sensor control register value +* @bg_ctrl - bandgap ctrl register value +* @bg_counter - bandgap counter value +* @bg_threshold - bandgap threshold register value +* @temp_sensor_tshut_threshold - bandgap tshut register value +* @is_efuse_valid - Flag to determine if efuse is valid or not +* @clk_on - Manages the current clock state +*/ +struct omap_temp_sensor { +struct device *hwmon_dev; +struct clk *clock; +struct omap_temp_sensor_registers *registers; +struct mutex sensor_mutex;/* Mutex for sysfs, irq and PM */ +unsigned int irq; +void __iomem *phy_base; +u32 clk_rate; +u32 temp_sensor_ctrl; +u32 bg_ctrl; +u32 bg_counter; +u32 bg_threshold;
drivers/hwmon/omap_temp_sensor.c line 91
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+u32 temp_sensor_tshut_threshold; +bool is_efuse_valid; +bool clk_on; +}; + +/* +* Temperature values in milli degree celsius +* ADC code values from 530 to 923 +*/ +static int adc_to_temp[] = {
This array should have an explicit size.
Quoted 48 lines
drivers/hwmon/omap_temp_sensor.c new file +47
-0+-40000 ,-40000 ,-40000 ,-40000 ,-39800 ,-39400 ,-39000 ,-38600 ,-38200 , +-37800 ,-37300 ,-36800 ,-36400 ,-36000 ,-35600 ,-35200 ,-34800 , +-34300 ,-33800 ,-33400 ,-33000 ,-32600 ,-32200 ,-31800 ,-31300 , +-30800 ,-30400 ,-30000 ,-29600 ,-29200 ,-28700 ,-28200 ,-27800 , +-27400 ,-27000 ,-26600 ,-26200 ,-25700 ,-25200 ,-24800 ,-24400 , +-24000 ,-23600 ,-23200 ,-22700 ,-22200 ,-21800 ,-21400 ,-21000 , +-20600 ,-20200 ,-19700 ,-19200 ,-18800 ,-18400 ,-18000 ,-17600 , +-17200 ,-16700 ,-16200 ,-15800 ,-15400 ,-15000 ,-14600 ,-14200 , +-13700 ,-13200 ,-12800 ,-12400 ,-12000 ,-11600 ,-11200 ,-10700 , +-10200 ,-9800 ,-9400 ,-9000 ,-8600 ,-8200 ,-7700 ,-7200 ,-6800 , +-6400 ,-6000 ,-5600 ,-5200 ,-4800 ,-4300 ,-3800 ,-3400 ,-3000 , +-2600 ,-2200 ,-1800 ,-1300 ,-800 ,-400 ,0 ,400 ,800 ,1200 ,1600 , +2100 ,2600 ,3000 ,3400 ,3800 ,4200 ,4600 ,5100 ,5600 ,6000 ,6400 , +6800 ,7200 ,7600 ,8000 ,8500 ,9000 ,9400 ,9800 ,10200 ,10600 ,11000 , +11400 ,11900 ,12400 ,12800 ,13200 ,13600 ,14000 ,14400 ,14800 , +15300 ,15800 ,16200 ,16600 ,17000 ,17400 ,17800 ,18200 ,18700 , +19200 ,19600 ,20000 ,20400 ,20800 ,21200 ,21600 ,22100 ,22600 , +23000 ,23400 ,23800 ,24200 ,24600 ,25000 ,25400 ,25900 ,26400 , +26800 ,27200 ,27600 ,28000 ,28400 ,28800 ,29300 ,29800 ,30200 , +30600 ,31000 ,31400 ,31800 ,32200 ,32600 ,33100 ,33600 ,34000 , +34400 ,34800 ,35200 ,35600 ,36000 ,36400 ,36800 ,37300 ,37800 , +38200 ,38600 ,39000 ,39400 ,39800 ,40200 ,40600 ,41100 ,41600 , +42000 ,42400 ,42800 ,43200 ,43600 ,44000 ,44400 ,44800 ,45300 , +45800 ,46200 ,46600 ,47000 ,47400 ,47800 ,48200 ,48600 ,49000 , +49500 ,50000 ,50400 ,50800 ,51200 ,51600 ,52000 ,52400 ,52800 , +53200 ,53700 ,54200 ,54600 ,55000 ,55400 ,55800 ,56200 ,56600 , +57000 ,57400 ,57800 ,58200 ,58700 ,59200 ,59600 ,60000 ,60400 , +60800 ,61200 ,61600 ,62000 ,62400 ,62800 ,63300 ,63800 ,64200 , +64600 ,65000 ,65400 ,65800 ,66200 ,66600 ,67000 ,67400 ,67800 , +68200 ,68700 ,69200 ,69600 ,70000 ,70400 ,70800 ,71200 ,71600 , +72000 ,72400 ,72800 ,73200 ,73600 ,74100 ,74600 ,75000 ,75400 , +75800 ,76200 ,76600 ,77000 ,77400 ,77800 ,78200 ,78600 ,79000 , +79400 ,79800 ,80300 ,80800 ,81200 ,81600 ,82000 ,82400 ,82800 , +83200 ,83600 ,84000 ,84400 ,84800 ,85200 ,85600 ,86000 ,86400 , +86800 ,87300 ,87800 ,88200 ,88600 ,89000 ,89400 ,89800 ,90200 , +90600 ,91000 ,91400 ,91800 ,92200 ,92600 ,93000 ,93400 ,93800 , +94200 ,94600 ,95000 ,95500 ,96000 ,96400 ,96800 ,97200 ,97600 , +98000 ,98400 ,98800 ,99200 ,99600 ,100000 ,100400 ,100800 ,101200 , +101600 ,102000 ,102400 ,102800 ,103200 ,103600 ,104000 ,104400 , +104800 ,105200 ,105600 ,106100 ,106600 ,107000 ,107400 ,107800 , +108200 ,108600 ,109000 ,109400 ,109800 ,110200 ,110600 ,111000 , +111400 ,111800 ,112200 ,112600 ,113000 ,113400 ,113800 ,114200 , +114600 ,115000 ,115400 ,115800 ,116200 ,116600 ,117000 ,117400 , +117800 ,118200 ,118600 ,119000 ,119400 ,119800 ,120200 ,120600 , +121000 ,121400 ,121800 ,122200 ,122600 ,123000 +}; +
drivers/hwmon/omap_temp_sensor.c line 148
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+static unsigned long omap_temp_sensor_readl (struct omap_temp_sensor + *temp_sensor,u32 reg) +{ +return __raw_readl (temp_sensor->phy_base + reg); +} + +static void omap_temp_sensor_writel (struct omap_temp_sensor *temp_sensor, +u32 val,u32 reg) +{ +__raw_writel (val, (temp_sensor->phy_base + reg));
Unnecessary ( )
Quoted 57 lines
drivers/hwmon/omap_temp_sensor.c new file +57
-0+} + +static int adc_to_temp_conversion (int adc_val) +{ +return adc_to_temp[adc_val -OMAP_ADC_START_VALUE ]; +} + +static int temp_to_adc_conversion (long temp) +{ +int high, low, mid; + +if (temp < adc_to_temp[0 ] || + temp > adc_to_temp[OMAP_ADC_END_VALUE -OMAP_ADC_START_VALUE ]) +return -EINVAL ; + + high =OMAP_ADC_END_VALUE -OMAP_ADC_START_VALUE ; + low =0 ; + mid = (high + low) /2 ; + +while (low < high) { +if (temp < adc_to_temp[mid]) + high = mid -1 ; +else + low = mid +1 ; + mid = (low + high) /2 ; + } + +return OMAP_ADC_START_VALUE + low; +} + +static void omap_configure_temp_sensor_thresholds (struct omap_temp_sensor + *temp_sensor) +{ +u32 temp; + +/* Configure the TALERT thresholds */ + temp =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_threshold); + temp |= (T_HOT <<__ffs (temp_sensor->registers->threshold_thot_mask)) | + (T_COLD <<__ffs (temp_sensor->registers->threshold_tcold_mask)); +omap_temp_sensor_writel (temp_sensor, temp, + temp_sensor->registers->bgap_threshold); + +/* Configure the TSHUT thresholds */ + temp =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->thsut_threshold); + temp |= (TSHUT_HOT <<__ffs (temp_sensor->registers->tshut_hot_mask)) + | (TSHUT_COLD <<__ffs (temp_sensor->registers->tshut_hot_mask)); +omap_temp_sensor_writel (temp_sensor, temp, + temp_sensor->registers->thsut_threshold); +} + +static void omap_configure_temp_sensor_counter (struct omap_temp_sensor + *temp_sensor,u32 counter) +{ +u32 val; +
drivers/hwmon/omap_temp_sensor.c line 215
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_counter); + val &= ~temp_sensor->registers->counter_mask; + val |= counter <<__ffs (temp_sensor->registers->counter_mask); +omap_temp_sensor_writel (temp_sensor, val, + temp_sensor->registers->bgap_counter); +} + +static void omap_enable_continuous_mode (struct omap_temp_sensor *temp_sensor, +bool enable)
The enable parameter is always 1 when called, and thus unnecessary.
Quoted 21 lines
drivers/hwmon/omap_temp_sensor.c new file +21
-0+{ +u32 val; + + val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_mode_ctrl); + +if (enable) + val |=1 <<__ffs (temp_sensor->registers->mode_ctrl_mask); +else + val &= ~temp_sensor->registers->mode_ctrl_mask; + +omap_temp_sensor_writel (temp_sensor, val, + temp_sensor->registers->bgap_mode_ctrl); +} + +/* 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);
drivers/hwmon/omap_temp_sensor.c line 246
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+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.
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c new file +8
-0+ } + + temp =adc_to_temp_conversion (temp); + +return snprintf (buf,16 ,"%d\n" , temp); +} + +static ssize_t set_temp_max(struct device *dev,
drivers/hwmon/omap_temp_sensor.c line 264
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+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.
drivers/hwmon/omap_temp_sensor.c line 267
Quoted 3 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ } + + t_hot =temp_to_adc_conversion (val);
Bad error return check. Negative on error. Should be
if (t_hot < 0) return t_hot;
drivers/hwmon/omap_temp_sensor.c line 268
Quoted 1 line
drivers/hwmon/omap_temp_sensor.c new file +1
-0+if ((t_hot <OMAP_ADC_START_VALUE || t_hot >OMAP_ADC_END_VALUE )) {
Unnecessary (( ))
drivers/hwmon/omap_temp_sensor.c line 270
Quoted 2 lines
drivers/hwmon/omap_temp_sensor.c new file +2
-0+ count = -EINVAL ; + goto out;
This will try to release the not-acquired lock.
drivers/hwmon/omap_temp_sensor.c line 282
Quoted 12 lines
drivers/hwmon/omap_temp_sensor.c new file +12
-0+ } + +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.
Quoted 30 lines
drivers/hwmon/omap_temp_sensor.c new file +30
-0+ } + +/* 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); + } + +/*
drivers/hwmon/omap_temp_sensor.c line 322
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+* 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 ?
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c new file +8
-0+ } +/* +* 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); +
drivers/hwmon/omap_temp_sensor.c line 340
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+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.
drivers/hwmon/omap_temp_sensor.c line 349
Quoted 9 lines
drivers/hwmon/omap_temp_sensor.c new file +9
-0+ temp =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_threshold); + temp = (temp & temp_sensor->registers->threshold_tcold_mask) >> +__ffs (temp_sensor->registers->threshold_tcold_mask); +mutex_unlock (&temp_sensor->sensor_mutex); + +if (temp <OMAP_ADC_START_VALUE || temp >OMAP_ADC_END_VALUE ) { +dev_err (dev,"invalid value\n" ); +return -EDOM ;
"Math argument out of domain of func" does not make sense.
Unless you don't know anything better, use EIO (I/O Error). Not very specific, but at least not misleading.
Besides, the log message is unnecessary. The user is informed with -EIO, and the admin won't be able to do anything useful with "invalid value".
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c new file +8
-0+ } + + temp =adc_to_temp_conversion (temp); + +return snprintf (buf,16 ,"%d\n" , temp); +} + +static ssize_t set_temp_max_hyst(struct device *dev,
drivers/hwmon/omap_temp_sensor.c line 367
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); +u32 reg_val, t_hot, t_cold, temp; +long val; + +if (strict_strtol (buf,10 , &val)) { + count = -EINVAL ; + goto out;
This will try to release the not acquired lock.
drivers/hwmon/omap_temp_sensor.c line 370
Quoted 3 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ } + + t_cold =temp_to_adc_conversion (val);
Bad error return check; same as above.
drivers/hwmon/omap_temp_sensor.c line 383
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c new file +13
-0+if (t_cold <OMAP_ADC_START_VALUE || t_cold >OMAP_ADC_END_VALUE ) { + count = -EINVAL ; + goto out; + } + +mutex_lock (&temp_sensor->sensor_mutex); +/* obtain the T HOT value */ + t_hot =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_threshold); + t_hot = (t_hot & temp_sensor->registers->threshold_thot_mask) >> +__ffs (temp_sensor->registers->threshold_thot_mask); + +if (t_cold > t_hot) {
Same concern as above. Change order is left to application which should not be bothered.
Quoted 38 lines
drivers/hwmon/omap_temp_sensor.c new file +38
-0+ count = -EINVAL ; + goto out; + } + +/* write the new t_cold value */ + reg_val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_threshold); + reg_val &= ~temp_sensor->registers->threshold_tcold_mask; + reg_val |= t_cold << +__ffs (temp_sensor->registers->threshold_tcold_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 LOW threshold(t_cold) lower than the current +* temperature(temp) unmask the COLD interrupts +*/ +if (t_cold < temp) { + reg_val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + reg_val &= ~temp_sensor->registers->mask_hot_mask; + reg_val |= temp_sensor->registers->mask_cold_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_hot && temp > t_cold) { + reg_val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_mask_ctrl);
drivers/hwmon/omap_temp_sensor.c line 431
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ 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); + } + +/* +* else no need to do anything since HW will immediately compare +* the new threshold and generate interrupt accordingly +*/
Would be nice if you could unify the interrupt enable code in a single function.
drivers/hwmon/omap_temp_sensor.c line 444
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c new file +13
-0+ +out: +mutex_unlock (&temp_sensor->sensor_mutex); + +return count; +} + +static ssize_t show_update_rate (struct device *dev, +struct device_attribute *devattr,char *buf) +{ +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); +u32 temp =0 , ret =0 ; +
No need to initialize temp.
ret is u32, which doesn't seem to be a good type to use for negative values.
drivers/hwmon/omap_temp_sensor.c line 448
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ +if (!temp_sensor->clk_rate) { +dev_err (dev,"clk_rate is NULL\n" ); + ret = -EDOM ;
Same comment as above (this is not a math error). And you don't have to check clk_rate anyway, since it is set in probe, and probe bails out if it is not set.
drivers/hwmon/omap_temp_sensor.c line 452
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ goto out; + } + +mutex_lock (&temp_sensor->sensor_mutex);
As with other show locks, this one would only make sense if counter_mask can change during runtime, and if that change would be protected by the mutex. This is not the case, thus the lock is unnecessary.
Quoted 24 lines
drivers/hwmon/omap_temp_sensor.c new file +24
-0+ temp =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_counter); + temp = (temp & temp_sensor->registers->counter_mask) >> +__ffs (temp_sensor->registers->counter_mask); +mutex_unlock (&temp_sensor->sensor_mutex); + temp = temp *1000 / temp_sensor->clk_rate; + +out: +if (!ret) +return sprintf (buf,"%d\n" , temp); + +return ret; +} + +static ssize_t set_update_rate (struct device *dev, +struct device_attribute *devattr, +const char *buf,size_t count) +{ +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); +u32 reg_val; +long val; + +if (strict_strtol (buf,10 , &val)) { + count = -EINVAL ;
drivers/hwmon/omap_temp_sensor.c line 486
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ goto out; + } + + val *= temp_sensor->clk_rate /1000 ; +mutex_lock (&temp_sensor->sensor_mutex); + reg_val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_counter); + + reg_val &= ~temp_sensor->registers->counter_mask; + reg_val |= val;
Sure you want to accept any value (including negative ones) here ? val is not shifted, and not masked against counter_mask, meaning it can write all bits of the register. That doesn't sound right.
It would make sense to call omap_configure_temp_sensor_counter() instead of duplicating the set functionality.
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+omap_temp_sensor_writel (temp_sensor, reg_val, + temp_sensor->registers->bgap_counter); +mutex_unlock (&temp_sensor->sensor_mutex); + +out: +return count;
drivers/hwmon/omap_temp_sensor.c line 502
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+} + +static int omap_temp_sensor_read_temp (struct device *dev, +struct device_attribute *devattr, +char *buf) +{ +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); +int temp, ret =0 ; + +mutex_lock (&temp_sensor->sensor_mutex);
Unless bgap_dtemp_mask can change during runtime, and that change is protected by the mutex, this lock is unnecessary.
drivers/hwmon/omap_temp_sensor.c line 510
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c new file +8
-0+ temp =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + temp &= temp_sensor->registers->bgap_dtemp_mask; + +/* look up for temperature in the table and return the temperature */ +if (temp <OMAP_ADC_START_VALUE || temp >OMAP_ADC_END_VALUE ) { +dev_err (dev,"invalid adc code reported %d" , temp); + ret = -EDOM ;
Another wrong use of EDOM, and another value-free error message.
Quoted 12 lines
drivers/hwmon/omap_temp_sensor.c new file +12
-0+ goto out; + } + + temp = adc_to_temp[temp -OMAP_ADC_START_VALUE ]; + +out: +mutex_unlock (&temp_sensor->sensor_mutex); +if (!ret) +return sprintf (buf,"%d\n" , temp); + +return ret; +}
drivers/hwmon/omap_temp_sensor.c line 532
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, omap_temp_sensor_read_temp, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max, + set_temp_max, 0); +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, show_temp_max_hyst, + set_temp_max_hyst, 0); +static SENSOR_DEVICE_ATTR(update_rate, S_IWUSR | S_IRUGO, show_update_rate, + set_update_rate, 0); +
ABI is update_interval. Please use it.
Quoted 21 lines
drivers/hwmon/omap_temp_sensor.c new file +21
-0+static struct attribute *omap_temp_sensor_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_temp1_max.dev_attr.attr, + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr, + &sensor_dev_attr_update_rate.dev_attr.attr, +NULL +}; + +static const struct attribute_group omap_temp_sensor_group = { + .attrs = omap_temp_sensor_attributes, +}; + +static int omap_temp_sensor_clk_enable (struct omap_temp_sensor *temp_sensor) +{ +u32 ret =0 ; + +if (temp_sensor->clk_on) { +dev_err (temp_sensor->hwmon_dev,"clock already on\n" ); + goto out; + } +
drivers/hwmon/omap_temp_sensor.c line 563
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ ret =pm_runtime_get_sync (temp_sensor->hwmon_dev); +if (ret <0 ) { +dev_err (temp_sensor->hwmon_dev,"get sync failed\n" ); + goto out; + } + +mutex_lock (&temp_sensor->sensor_mutex); + temp_sensor->clk_on =1 ; +mutex_unlock (&temp_sensor->sensor_mutex); +
Completely unnecessary lock.
drivers/hwmon/omap_temp_sensor.c line 568
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c new file +5
-0+out: +return ret; +} + +static int omap_temp_sensor_clk_disable (struct omap_temp_sensor *temp_sensor)
Return code is not used anywhere, why bother providing one ?
drivers/hwmon/omap_temp_sensor.c line 574
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+{ +u32 ret =0 ; + +if (!temp_sensor->clk_on) { +dev_err (temp_sensor->hwmon_dev,"clock already off\n" ); + goto out;
... and since you ignore the error anyway, why log it ?
drivers/hwmon/omap_temp_sensor.c line 587
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c new file +13
-0+ } + +/* Gate the clock */ + ret =pm_runtime_put_sync (temp_sensor->hwmon_dev); +if (ret <0 ) { +dev_err (temp_sensor->hwmon_dev,"put sync failed\n" ); + goto out; + } + +mutex_lock (&temp_sensor->sensor_mutex); + temp_sensor->clk_on =0 ; +mutex_unlock (&temp_sensor->sensor_mutex); +
Completely unnecessary lock.
Quoted 56 lines
drivers/hwmon/omap_temp_sensor.c new file +56
-0+out: +return ret; +} + +static irqreturn_t omap_talert_irq_handler (int irq,void *data) +{ +struct omap_temp_sensor *temp_sensor; +int t_hot, t_cold, temp; + + temp_sensor = data; +mutex_lock (&temp_sensor->sensor_mutex); + +/* Read the status of t_hot */ + t_hot =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_hot_mask; + +/* Read the status of t_cold */ + t_cold =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_cold_mask; + + temp =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); +/* +* One TALERT interrupt: Two sources +* If the interrupt is due to t_hot then mask t_hot and +* and unmask t_cold else mask t_cold and unmask t_hot +*/ +if (t_hot) { + temp &= ~temp_sensor->registers->mask_hot_mask; + temp |= temp_sensor->registers->mask_cold_mask; + }else if (t_cold) { + temp &= ~temp_sensor->registers->mask_cold_mask; + temp |= temp_sensor->registers->mask_hot_mask; + } + +omap_temp_sensor_writel (temp_sensor, temp, + temp_sensor->registers->bgap_mask_ctrl); + +/* kobject_uvent to user space telling thermal threshold crossed */ +kobject_uevent (&temp_sensor->hwmon_dev->kobj,KOBJ_CHANGE ); + +mutex_unlock (&temp_sensor->sensor_mutex); + +return IRQ_HANDLED ; +} + +static int __devinit omap_temp_sensor_probe (struct platform_device *pdev) +{ +struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; +struct omap_temp_sensor *temp_sensor; +struct resource *mem; +int ret =0 ; +int val, clk_rate; +u32 max_freq, min_freq;
drivers/hwmon/omap_temp_sensor.c line 653
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ +if (!pdata) { +dev_err (&pdev->dev,"platform data missing\n" ); +return -EINVAL ; + } + + temp_sensor =kzalloc (sizeof (*temp_sensor),GFP_KERNEL ); +if (!temp_sensor) +return -ENOMEM ; +
You have error messages all over the place, Why not here ?
drivers/hwmon/omap_temp_sensor.c line 662
Quoted 9 lines
drivers/hwmon/omap_temp_sensor.c new file +9
-0+mutex_init (&temp_sensor->sensor_mutex); + + mem =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!mem) { +dev_err (&pdev->dev,"no mem resource\n" ); + ret = -ENOMEM ; + goto plat_res_err; + } +
If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Quoted 22 lines
drivers/hwmon/omap_temp_sensor.c new file +22
-0+ temp_sensor->irq =platform_get_irq_byname (pdev,"thermal_alert" ); +if (temp_sensor->irq <0 ) { +dev_err (&pdev->dev,"get_irq_byname failed\n" ); + ret = temp_sensor->irq; + goto plat_res_err; + } + + temp_sensor->phy_base =ioremap (mem->start,resource_size (mem)); + temp_sensor->clock =NULL ; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + +if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + }else { + max_freq =MAX_FREQ ; + min_freq =MIN_FREQ ; + } + +pm_runtime_enable (&pdev->dev); +pm_runtime_irq_safe (&pdev->dev);
drivers/hwmon/omap_temp_sensor.c line 694
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ +/* +* check if the efuse has a non-zero value if not +* it is an untrimmed sample and the temperatures +* may not be accurate +*/ + +if (omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_efuse)) { + temp_sensor->is_efuse_valid =1 ;
is_efuse_valid is not used anywhere. Might as well drop it.
drivers/hwmon/omap_temp_sensor.c line 696
Quoted 2 lines
drivers/hwmon/omap_temp_sensor.c new file +2
-0+dev_dbg (temp_sensor->hwmon_dev, +"Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n" );
dev_info might be better here.
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ } + +dev_set_drvdata (&pdev->dev, temp_sensor); + temp_sensor->clock =clk_get (temp_sensor->hwmon_dev,"fck" );
drivers/hwmon/omap_temp_sensor.c line 710
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+if (IS_ERR (temp_sensor->clock)) { + ret =PTR_ERR (temp_sensor->clock); +dev_err (temp_sensor->hwmon_dev, +"unable to get fclk: %d\n" , ret); + goto plat_res_err; + } + + ret =omap_temp_sensor_clk_enable (temp_sensor); +if (ret) { +dev_err (&pdev->dev,"Cannot enable temp sensor\n" );
omap_temp_sensor_clk_enable() already generates error messages.
drivers/hwmon/omap_temp_sensor.c line 717
Quoted 7 lines
drivers/hwmon/omap_temp_sensor.c new file +7
-0+ goto clken_err; + } + + clk_rate =clk_round_rate (temp_sensor->clock, max_freq); +if (clk_rate < min_freq || clk_rate =0xffffffff ) { +dev_err (&pdev->dev,"Error round rate\n" ); + ret = -EDOM ;
EDOM is wrong. Please use ENODEV.
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ goto clken_err; + } + + ret =clk_set_rate (temp_sensor->clock, clk_rate);
drivers/hwmon/omap_temp_sensor.c line 731
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+if (ret) { +dev_err (&pdev->dev,"Cannot set clock rate\n" ); + goto clken_err; + } + + temp_sensor->clk_rate = clk_rate; +omap_enable_continuous_mode (temp_sensor,1 ); +omap_configure_temp_sensor_thresholds (temp_sensor); +/* 1 ms */ +omap_configure_temp_sensor_counter (temp_sensor,1 );
If temp_sensor->clk_rate * 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would be temp_sensor->clk_rate / 1000.
Quoted 11 lines
drivers/hwmon/omap_temp_sensor.c new file +11
-0+ +/* Wait till the first conversion is done wait for at least 1ms */ +usleep_range (1000 ,2000 ); + +/* Read the temperature once due to hw issue*/ +omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + +/* Set 2 seconds time as default counter */ +omap_configure_temp_sensor_counter (temp_sensor, + temp_sensor->clk_rate *2 );
drivers/hwmon/omap_temp_sensor.c line 752
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ + temp_sensor->hwmon_dev =hwmon_device_register (&pdev->dev); +if (IS_ERR (temp_sensor->hwmon_dev)) { +dev_err (&pdev->dev,"hwmon_device_register failed.\n" ); + ret =PTR_ERR (temp_sensor->hwmon_dev); + goto hwmon_reg_err; + } + + ret =sysfs_create_group (&pdev->dev.kobj, + &omap_temp_sensor_group);
one line is sufficient here.
drivers/hwmon/omap_temp_sensor.c line 759
Quoted 7 lines
drivers/hwmon/omap_temp_sensor.c new file +7
-0+if (ret) { +dev_err (&pdev->dev,"could not create sysfs files\n" ); + goto sysfs_create_err; + } + +kobject_uevent (&temp_sensor->hwmon_dev->kobj,KOBJ_ADD ); +
No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Quoted 7 lines
drivers/hwmon/omap_temp_sensor.c new file +7
-0+ ret =request_threaded_irq (temp_sensor->irq,NULL , + omap_talert_irq_handler,IRQF_TRIGGER_RISING |IRQF_ONESHOT , +"temp_sensor" , temp_sensor); +if (ret) { +dev_err (&pdev->dev,"Request threaded irq failed.\n" ); + goto req_irq_err; + }
drivers/hwmon/omap_temp_sensor.c line 776
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ +/* unmask the T_COLD and unmask T_HOT at init */ + val =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + +omap_temp_sensor_writel (temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +
This is racy. From Documentation/hwmon/submitting-patches:
* Make sure there are no race conditions in the probe function. Specifically, completely initialize your chip first, then create sysfs entries and register with the hwmon subsystem.
drivers/hwmon/omap_temp_sensor.c line 780
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+return 0 ; + +req_irq_err: +kobject_uevent (&temp_sensor->hwmon_dev->kobj,KOBJ_REMOVE );
Same as before; why is this needed here but not in any other hwmon driver ?
Quoted 11 lines
drivers/hwmon/omap_temp_sensor.c new file +11
-0+sysfs_remove_group (&temp_sensor->hwmon_dev->kobj, + &omap_temp_sensor_group); +sysfs_create_err: +hwmon_device_unregister (&pdev->dev); +hwmon_reg_err: +omap_temp_sensor_clk_disable (temp_sensor); +clken_err: +clk_put (temp_sensor->clock); +plat_res_err: +dev_set_drvdata (&pdev->dev,NULL ); +mutex_destroy (&temp_sensor->sensor_mutex);
drivers/hwmon/omap_temp_sensor.c line 801
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+kfree (temp_sensor); +return ret; +} + +static int __devexit omap_temp_sensor_remove(struct platform_device *pdev) +{ +struct omap_temp_sensor *temp_sensor =platform_get_drvdata (pdev); + +hwmon_device_unregister (&pdev->dev); +kobject_uevent (&temp_sensor->hwmon_dev->kobj,KOBJ_REMOVE );
... and again.
Quoted 132 lines
drivers/hwmon/omap_temp_sensor.c new file +131
-1+sysfs_remove_group (&temp_sensor->hwmon_dev->kobj, + &omap_temp_sensor_group); +omap_temp_sensor_clk_disable (temp_sensor); +free_irq (temp_sensor->irq, temp_sensor); +clk_put (temp_sensor->clock); +dev_set_drvdata (&pdev->dev,NULL ); +mutex_destroy (&temp_sensor->sensor_mutex); +kfree (temp_sensor); + +return 0 ; +} + +#ifdef CONFIG_PM +static void omap_temp_sensor_save_ctxt (struct omap_temp_sensor *temp_sensor) +{ + temp_sensor->temp_sensor_ctrl =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + temp_sensor->bg_ctrl =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + temp_sensor->bg_counter =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_counter); + temp_sensor->bg_threshold =omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->bgap_threshold); + temp_sensor->temp_sensor_tshut_threshold > +omap_temp_sensor_readl (temp_sensor, + temp_sensor->registers->thsut_threshold); +} + +static void omap_temp_sensor_restore_ctxt (struct omap_temp_sensor *temp_sensor) +{ +omap_temp_sensor_writel (temp_sensor, + temp_sensor->temp_sensor_ctrl, + temp_sensor->registers->temp_sensor_ctrl); +omap_temp_sensor_writel (temp_sensor, + temp_sensor->bg_ctrl, + temp_sensor->registers->bgap_mask_ctrl); +omap_temp_sensor_writel (temp_sensor, + temp_sensor->bg_counter, + temp_sensor->registers->bgap_counter); +omap_temp_sensor_writel (temp_sensor, + temp_sensor->bg_threshold, + temp_sensor->registers->bgap_threshold); +omap_temp_sensor_writel (temp_sensor, + temp_sensor->temp_sensor_tshut_threshold, + temp_sensor->registers->thsut_threshold); +} + +static int omap_temp_sensor_suspend (struct device *dev) +{ +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); + +omap_temp_sensor_save_ctxt (temp_sensor); + +return 0 ; +} + +static int omap_temp_sensor_resume (struct device *dev) +{ +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); + +omap_temp_sensor_restore_ctxt (temp_sensor); + +return 0 ; +} + +static int omap_temp_sensor_runtime_suspend (struct device *dev) +{ +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); + +omap_temp_sensor_save_ctxt (temp_sensor); + +return 0 ; +} + +static int omap_temp_sensor_runtime_resume (struct device *dev) +{ +static int context_loss_count; +int temp; +struct omap_temp_sensor *temp_sensor =dev_get_drvdata (dev); + + temp =omap_device_get_context_loss_count (to_platform_device (dev)); + +if (temp != context_loss_count && context_loss_count !=0 ) +omap_temp_sensor_restore_ctxt (temp_sensor); + + context_loss_count = temp; + +return 0 ; +} + +static int omap_temp_sensor_idle (struct device *dev) +{ +return 0 ; +} + +static const struct dev_pm_ops omap_temp_sensor_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(omap_temp_sensor_suspend, + omap_temp_sensor_resume) +SET_RUNTIME_PM_OPS (omap_temp_sensor_runtime_suspend, + omap_temp_sensor_runtime_resume, omap_temp_sensor_idle) +}; + +#define DEV_PM_OPS (&omap_temp_sensor_dev_pm_ops) +#else +#define DEV_PM_OPS NULL +#endif + +static struct platform_driver omap_temp_sensor_driver = { + .probe = omap_temp_sensor_probe, + .remove = omap_temp_sensor_remove, + .driver = { + .name ="omap_temp_sensor" , + .pm =DEV_PM_OPS , + }, +}; + +int __init omap_temp_sensor_init (void ) +{ +return platform_driver_register (&omap_temp_sensor_driver); +} +module_init (omap_temp_sensor_init); + +static void __exit omap_temp_sensor_exit(void) +{ +platform_driver_unregister (&omap_temp_sensor_driver); +} +module_exit (omap_temp_sensor_exit); + +MODULE_DESCRIPTION ("OMAP446X temperature sensor Driver" ); +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("platform:" DRIVER_NAME ); +MODULE_AUTHOR ("J Keerthy <[email protected]>" ); --
Quoted 2 lines
1.7.0.4
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 23 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 9 lines
---
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_sensorcreate mode 100644drivers/hwmon/omap_temp_sensor.c
Quoted 16 lines
Quoted 6 lines
Documentation/hwmon/omap_temp_sensor new file +0
-0Documentation/hwmon/omap_temp_sensor line 8
Quoted 8 lines
Documentation/hwmon/omap_temp_sensor new file +8
-0@@ -0,0 +1,27 @@ +Kernel driver omap_temp_sensor +============================== + +Supported chips: + * Texas Instruments OMAP4460 + Prefix: 'omap_temp_sensor' + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp102.html +I am a bit lost here. What does the TMP102 have to do with the OMAP4460 ?
My mistake. I will point to the correct data sheet.
Quoted 11 lines
Quoted 11 lines
Documentation/hwmon/omap_temp_sensor new file +10
-0+Author: + J Keerthy <[email protected]> + +Description +----------- + +The Texas Instruments OMAP4 family of chips have a bandgap temperature sensor. +The temperature sensor feature is used to convert the temperature of the device +into a decimal value coded on 10 bits. An internal ADC is used for conversion. +The recommended operating temperatures must be in the range -40 degree Celsius
Quoted 8 lines
Documentation/hwmon/omap_temp_sensor line 24
Quoted 6 lines
Documentation/hwmon/omap_temp_sensor new file +6
-0+to 123 degree celsius for standard conversion. +The thresholds are programmable and upon crossing the thresholds an interrupt +is generated. The OMAP temperature sensor has a programmable update rate in +milli seconds. +(Currently the driver programs a default of 2000 milli seconds). +s/milli seconds/milliseconds/
Ok
Quoted 129 lines
Quoted 129 lines
Documentation/hwmon/omap_temp_sensor new file +3
-0+The driver provides the common sysfs-interface for temperatures (see +Documentation/hwmon/sysfs-interface under Temperatures). +drivers/hwmon/Kconfig +0
-0@@ -323,6 +323,17 @@ config SENSORS_F71805FThis driver can also be built as a module. If so, the module will be called f71805f.
drivers/hwmon/Kconfig +11
-0+config SENSORS_OMAP_BANDGAP_TEMP_SENSOR + bool "OMAP on-die temperature sensor hwmon driver" + depends on HWMON && ARCH_OMAP && OMAP_TEMP_SENSOR + help + If you say yes here you get support for hardware + monitoring features of the OMAP on die temperature + sensor. + + Continuous conversion programmable delay + mode is used for temperature conversion. +config
SENSORS_F71882FG
tristate "Fintek F71882FG and compatibles" helpdrivers/hwmon/Makefile +0
-0@@ -91,6 +91,7 @@ obj-$(CONFIG_SENSORS_MAX6639) += max6639.oobj-$(CONFIG_SENSORS_MAX6642) += max6642.o obj-$(CONFIG_SENSORS_MAX6650) += max6650.o obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o +obj-$(CONFIG_SENSORS_OMAP_BANDGAP_TEMP_SENSOR) += omap_temp_sensor.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
drivers/hwmon/omap_temp_sensor.c new file +85
-0@@ -0,0 +1,933 @@ +/* +* OMAP4 Temperature sensor driver file +* +* Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ +* Author: J Keerthy <[email protected]> +* Author: Moiz Sonasath <[email protected]> +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License +* version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA +* 02110-1301 USA +* +*/ + +#include <linux/interrupt.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/init.h> +#include <plat/omap_device.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <linux/jiffies.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/stddef.h> +#include <linux/sysfs.h> +#include <linux/err.h> +#include <linux/types.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/delay.h> +#include <plat/temperature_sensor.h> + +#define TSHUT_HOT 920/* 122 deg C */ +#define TSHUT_COLD 866/* 100 deg C */ +#define T_HOT 800/* 73 deg C */ +#define T_COLD 795/* 71 deg C */ +#define OMAP_ADC_START_VALUE 530 +#define OMAP_ADC_END_VALUE 923 +#define MAX_FREQ 2000000 +#define MIN_FREQ 1000000 + +/* +* omap_temp_sensor structure +* @hwmon_dev - device pointer +* @clock - Clock pointer +* @registers - Pointer to structure with register offsets and bitfields +* @sensor_mutex - Mutex for sysfs, irq and PM +* @irq - MPU Irq number for thermal alert +* @phy_base - Physical base of the temp I/O +* @clk_rate - Holds current clock rate +* @temp_sensor_ctrl - temp sensor control register value +* @bg_ctrl - bandgap ctrl register value +* @bg_counter - bandgap counter value +* @bg_threshold - bandgap threshold register value +* @temp_sensor_tshut_threshold - bandgap tshut register value +* @is_efuse_valid - Flag to determine if efuse is valid or not +* @clk_on - Manages the current clock state +*/ +struct omap_temp_sensor { + struct device *hwmon_dev; + struct clk *clock; + struct omap_temp_sensor_registers *registers; + struct mutex sensor_mutex; /* Mutex for sysfs, irq and PM */ + unsigned int irq; + void __iomem *phy_base; + u32 clk_rate; + u32 temp_sensor_ctrl; + u32 bg_ctrl; + u32 bg_counter; + u32 bg_threshold; + u32 temp_sensor_tshut_threshold; + bool is_efuse_valid; + bool clk_on; +};
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 91
Quoted 7 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ +/* +* Temperature values in milli degree celsius +* ADC code values from 530 to 923 +*/ +static int adc_to_temp[] = {This array should have an explicit size.
Ok
Quoted 53 lines
Quoted 53 lines
drivers/hwmon/omap_temp_sensor.c new file +51
-0+ -40000, -40000, -40000, -40000, -39800, -39400, -39000, -38600, -38200, + -37800, -37300, -36800, -36400, -36000, -35600, -35200, -34800, + -34300, -33800, -33400, -33000, -32600, -32200, -31800, -31300, + -30800, -30400, -30000, -29600, -29200, -28700, -28200, -27800, + -27400, -27000, -26600, -26200, -25700, -25200, -24800, -24400, + -24000, -23600, -23200, -22700, -22200, -21800, -21400, -21000, + -20600, -20200, -19700, -19200, -18800, -18400, -18000, -17600, + -17200, -16700, -16200, -15800, -15400, -15000, -14600, -14200, + -13700, -13200, -12800, -12400, -12000, -11600, -11200, -10700, + -10200, -9800, -9400, -9000, -8600, -8200, -7700, -7200, -6800, + -6400, -6000, -5600, -5200, -4800, -4300, -3800, -3400, -3000, + -2600, -2200, -1800, -1300, -800, -400, 0, 400, 800, 1200, 1600, + 2100, 2600, 3000, 3400, 3800, 4200, 4600, 5100, 5600, 6000, 6400, + 6800, 7200, 7600, 8000, 8500, 9000, 9400, 9800, 10200, 10600, 11000, + 11400, 11900, 12400, 12800, 13200, 13600, 14000, 14400, 14800, + 15300, 15800, 16200, 16600, 17000, 17400, 17800, 18200, 18700, + 19200, 19600, 20000, 20400, 20800, 21200, 21600, 22100, 22600, + 23000, 23400, 23800, 24200, 24600, 25000, 25400, 25900, 26400, + 26800, 27200, 27600, 28000, 28400, 28800, 29300, 29800, 30200, + 30600, 31000, 31400, 31800, 32200, 32600, 33100, 33600, 34000, + 34400, 34800, 35200, 35600, 36000, 36400, 36800, 37300, 37800, + 38200, 38600, 39000, 39400, 39800, 40200, 40600, 41100, 41600, + 42000, 42400, 42800, 43200, 43600, 44000, 44400, 44800, 45300, + 45800, 46200, 46600, 47000, 47400, 47800, 48200, 48600, 49000, + 49500, 50000, 50400, 50800, 51200, 51600, 52000, 52400, 52800, + 53200, 53700, 54200, 54600, 55000, 55400, 55800, 56200, 56600, + 57000, 57400, 57800, 58200, 58700, 59200, 59600, 60000, 60400, + 60800, 61200, 61600, 62000, 62400, 62800, 63300, 63800, 64200, + 64600, 65000, 65400, 65800, 66200, 66600, 67000, 67400, 67800, + 68200, 68700, 69200, 69600, 70000, 70400, 70800, 71200, 71600, + 72000, 72400, 72800, 73200, 73600, 74100, 74600, 75000, 75400, + 75800, 76200, 76600, 77000, 77400, 77800, 78200, 78600, 79000, + 79400, 79800, 80300, 80800, 81200, 81600, 82000, 82400, 82800, + 83200, 83600, 84000, 84400, 84800, 85200, 85600, 86000, 86400, + 86800, 87300, 87800, 88200, 88600, 89000, 89400, 89800, 90200, + 90600, 91000, 91400, 91800, 92200, 92600, 93000, 93400, 93800, + 94200, 94600, 95000, 95500, 96000, 96400, 96800, 97200, 97600, + 98000, 98400, 98800, 99200, 99600, 100000, 100400, 100800, 101200, + 101600, 102000, 102400, 102800, 103200, 103600, 104000, 104400, + 104800, 105200, 105600, 106100, 106600, 107000, 107400, 107800, + 108200, 108600, 109000, 109400, 109800, 110200, 110600, 111000, + 111400, 111800, 112200, 112600, 113000, 113400, 113800, 114200, + 114600, 115000, 115400, 115800, 116200, 116600, 117000, 117400, + 117800, 118200, 118600, 119000, 119400, 119800, 120200, 120600, + 121000, 121400, 121800, 122200, 122600, 123000 +}; + +static unsigned long omap_temp_sensor_readl (struct omap_temp_sensor + *temp_sensor, u32 reg) +{ + return __raw_readl(temp_sensor->phy_base + reg);
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 148
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+} + +static void omap_temp_sensor_writel (struct omap_temp_sensor *temp_sensor, + u32 val, u32 reg) +{ + __raw_writel(val, (temp_sensor->phy_base + reg));Unnecessary ( )
I will remove them.
Quoted 62 lines
Quoted 62 lines
drivers/hwmon/omap_temp_sensor.c new file +61
-0+} + +static int adc_to_temp_conversion (int adc_val) +{ + return adc_to_temp[adc_val - OMAP_ADC_START_VALUE]; +} + +static int temp_to_adc_conversion (long temp) +{ + int high, low, mid; + + if (temp < adc_to_temp[0] || + temp > adc_to_temp[OMAP_ADC_END_VALUE - OMAP_ADC_START_VALUE]) + return -EINVAL; + + high = OMAP_ADC_END_VALUE - OMAP_ADC_START_VALUE; + low = 0; + mid = (high + low) / 2; + + while (low < high) { + if (temp < adc_to_temp[mid]) + high = mid - 1; + else + low = mid + 1; + mid = (low + high) / 2; + } + + return OMAP_ADC_START_VALUE + low; +} + +static void omap_configure_temp_sensor_thresholds (struct omap_temp_sensor + *temp_sensor) +{ + u32 temp; + + /* Configure the TALERT thresholds */ + temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + temp |= (T_HOT << __ffs(temp_sensor->registers->threshold_thot_mask)) | + (T_COLD << __ffs(temp_sensor->registers->threshold_tcold_mask)); + omap_temp_sensor_writel(temp_sensor, temp, + temp_sensor->registers->bgap_threshold); + + /* Configure the TSHUT thresholds */ + temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->thsut_threshold); + temp |= (TSHUT_HOT << __ffs(temp_sensor->registers->tshut_hot_mask)) + | (TSHUT_COLD << __ffs(temp_sensor->registers->tshut_hot_mask)); + omap_temp_sensor_writel(temp_sensor, temp, + temp_sensor->registers->thsut_threshold); +} + +static void omap_configure_temp_sensor_counter (struct omap_temp_sensor + *temp_sensor, u32 counter) +{ + u32 val; + + val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_counter); + val &= ~temp_sensor->registers->counter_mask; + val |= counter << __ffs(temp_sensor->registers->counter_mask);
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 215
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_counter); +} + +static void omap_enable_continuous_mode (struct omap_temp_sensor *temp_sensor, + bool enable)The enable parameter is always 1 when called, and thus unnecessary.
Ok.
Quoted 26 lines
Quoted 26 lines
drivers/hwmon/omap_temp_sensor.c new file +25
-0+{ + u32 val; + + val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mode_ctrl); + + if (enable) + val |= 1 << __ffs(temp_sensor->registers->mode_ctrl_mask); + else + val &= ~temp_sensor->registers->mode_ctrl_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mode_ctrl); +} + +/* 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);
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 246
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ 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?
Quoted 13 lines
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c new file +12
-0+ } + + 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);
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 264
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ 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.
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 267
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ } + + t_hot = temp_to_adc_conversion(val);Bad error return check. Negative on error. Should be if (
t_hot< 0)
returnt_hot;
Ok.
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c line 268
Quoted 2 lines
drivers/hwmon/omap_temp_sensor.c new file +1
-0+ 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.
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c line 270
Quoted 3 lines
drivers/hwmon/omap_temp_sensor.c new file +2
-0+ count = -EINVAL; + goto out;This will try to release the not-acquired lock.
I will correct this.
Quoted 17 lines
drivers/hwmon/omap_temp_sensor.c line 282
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c new file +12
-0+ } + + 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_hysteresiswhich 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.
Quoted 35 lines
Quoted 35 lines
drivers/hwmon/omap_temp_sensor.c new file +34
-0+ } + + /* 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) {
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 322
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ 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_OFFSEThere butbgap_mask_ctrlelsewhere ?
Ok. I will change this.
Quoted 15 lines
Quoted 15 lines
drivers/hwmon/omap_temp_sensor.c new file +14
-0+ } + /* +* 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) +{
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 340
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ 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_maskcan 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 inshow_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.
Quoted 11 lines
drivers/hwmon/omap_temp_sensor.c line 349
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +9
-0+ temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + temp = (temp & temp_sensor->registers->threshold_tcold_mask) >> + __ffs(temp_sensor->registers->threshold_tcold_mask); + mutex_unlock(&temp_sensor->sensor_mutex); + + if (temp < OMAP_ADC_START_VALUE || temp > OMAP_ADC_END_VALUE) { + dev_err(dev, "invalid value\n"); + return -EDOM;
Quoted 3 lines
"Math argument out of domain of func" does not make sense.
Unless you don't know anything better, use EIO (I/O Error). Not very specific, but at least not misleading.
Out of range i assumed this was the closest error value. I will use EIO.
Quoted 3 lines
Besides, the log message is unnecessary. The user is informed with
-EIO, and the admin won't be able to do anything useful with "invalid value".
Ok. I will return -EIO and eliminate the log message.
Quoted 13 lines
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c new file +12
-0+ } + + temp = adc_to_temp_conversion(temp); + + return snprintf(buf, 16, "%d\n", temp); +} + +static ssize_t set_temp_max_hyst (struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev);
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 367
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ u32 reg_val, t_hot, t_cold, temp; + long val; + + if (strict_strtol(buf, 10, &val)) { + count = -EINVAL; + goto out;This will try to release the not acquired lock.
I will correct this.
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c line 370
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ } + + t_cold = temp_to_adc_conversion(val);Bad error return check; same as above.
Ok. I will correct this.
Quoted 59 lines
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ if (t_cold < OMAP_ADC_START_VALUE || t_cold > OMAP_ADC_END_VALUE) { + count = -EINVAL; + goto out;drivers/hwmon/omap_temp_sensor.c line 383
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ } + + mutex_lock(&temp_sensor->sensor_mutex); + /* obtain the T HOT value */ + t_hot = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + t_hot = (t_hot & temp_sensor->registers->threshold_thot_mask) >> + __ffs(temp_sensor->registers->threshold_thot_mask); + + if (t_cold > t_hot) {Same concern as above. Change order is left to application which should not be bothered.
Quoted 42 lines
drivers/hwmon/omap_temp_sensor.c new file +42
-0+ count = -EINVAL; + goto out; + } + + /* write the new t_cold value */ + reg_val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + reg_val &= ~temp_sensor->registers->threshold_tcold_mask; + reg_val |= t_cold << + __ffs(temp_sensor->registers->threshold_tcold_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 LOW threshold(t_cold) lower than the current + * temperature(temp) unmask the COLD interrupts + */ + if (t_cold < temp) { + reg_val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + reg_val &= ~temp_sensor->registers->mask_hot_mask; + reg_val |= temp_sensor->registers->mask_cold_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_hot && temp > t_cold) { + 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);
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 431
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ } + + /* +* else no need to do anything since HW will immediately compare +* the new threshold and generate interrupt accordingly +*/ Would be nice if you could unify the interrupt enable code in a single function.
Ok. I will have a function for this.
Quoted 16 lines
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ +out: + mutex_unlock(&temp_sensor->sensor_mutex);drivers/hwmon/omap_temp_sensor.c line 444
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ + return count; +} + +static ssize_t show_update_rate (struct device *dev, + struct device_attribute *devattr, char *buf) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + u32 temp = 0, ret = 0; +No need to initialize temp.
ret is u32, which doesn't seem to be a good type to use for negative values.
Oops yes. I will correct this.
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 448
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ + if (!temp_sensor->clk_rate) { + dev_err(dev, "clk_rate is NULL\n"); + ret = -EDOM;Same comment as above (this is not a math error). And you don't have to check
clk_rateanyway, since it is set in probe, and probe bails out if it is not set.
Ok
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 452
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ goto out; + } + + mutex_lock(&temp_sensor->sensor_mutex);As with other show locks, this one would only make sense if
counter_maskcan change during runtime, and if that change would be protected by the mutex. This is not the case, thus the lock is unnecessary.
Counter value can be altered runtime. A lock is needed in both the cases. I will add it.
Quoted 31 lines
Quoted 31 lines
drivers/hwmon/omap_temp_sensor.c new file +30
-0+ temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_counter); + temp = (temp & temp_sensor->registers->counter_mask) >> + __ffs(temp_sensor->registers->counter_mask); + mutex_unlock(&temp_sensor->sensor_mutex); + temp = temp * 1000 / temp_sensor->clk_rate; + +out: + if (!ret) + return sprintf(buf, "%d\n", temp); + + return ret; +} + +static ssize_t set_update_rate (struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + u32 reg_val; + long val; + + if (strict_strtol(buf, 10, &val)) { + count = -EINVAL; + goto out; + } + + val *= temp_sensor->clk_rate / 1000; + mutex_lock(&temp_sensor->sensor_mutex); + reg_val = omap_temp_sensor_readl(temp_sensor,
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 486
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ temp_sensor->registers->bgap_counter); + + reg_val &= ~temp_sensor->registers->counter_mask; + reg_val |= val;Sure you want to accept any value (including negative ones) here ? val is not shifted, and not masked against
counter_mask, meaning it can write all bits of the register. That doesn't sound right.
I will have a check.
Quoted 3 lines
It would make sense to call
omap_configure_temp_sensor_counter()instead of duplicating the set functionality.
Yes I will make use of that.
Quoted 12 lines
Quoted 12 lines
drivers/hwmon/omap_temp_sensor.c new file +11
-0+ omap_temp_sensor_writel(temp_sensor, reg_val, + temp_sensor->registers->bgap_counter); + mutex_unlock(&temp_sensor->sensor_mutex); + +out: + return count; +} + +static int omap_temp_sensor_read_temp (struct device *dev, + struct device_attribute *devattr, + char *buf)
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 502
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c new file +5
-0+{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + int temp, ret = 0; + + mutex_lock(&temp_sensor->sensor_mutex);Unless
bgap_dtemp_maskcan change during runtime, and that change is protected by the mutex, this lock is unnecessary.
It is read only and is updated by hardware. Change cannot be protected by mutex.
Quoted 11 lines
drivers/hwmon/omap_temp_sensor.c line 510
Quoted 9 lines
drivers/hwmon/omap_temp_sensor.c new file +8
-0+ temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + temp &= temp_sensor->registers->bgap_dtemp_mask; + + /* look up for temperature in the table and return the temperature */ + if (temp < OMAP_ADC_START_VALUE || temp > OMAP_ADC_END_VALUE) { + dev_err(dev, "invalid adc code reported %d", temp); + ret = -EDOM;Another wrong use of EDOM, and another value-free error message.
Ok. I will use -EIO and drop the error message.
Quoted 17 lines
Quoted 17 lines
drivers/hwmon/omap_temp_sensor.c new file +16
-0+ goto out; + } + + temp = adc_to_temp[temp - OMAP_ADC_START_VALUE]; + +out: + mutex_unlock(&temp_sensor->sensor_mutex); + if (!ret) + return sprintf(buf, "%d\n", temp); + + return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, omap_temp_sensor_read_temp, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max,
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 532
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ set_temp_max, 0); +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, show_temp_max_hyst, + set_temp_max_hyst, 0); +static SENSOR_DEVICE_ATTR(update_rate, S_IWUSR | S_IRUGO, show_update_rate, + set_update_rate, 0); +ABI is
update_interval. Please use it.
Ok.
Quoted 26 lines
Quoted 26 lines
drivers/hwmon/omap_temp_sensor.c new file +25
-0+static struct attribute *omap_temp_sensor_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_temp1_max.dev_attr.attr, + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr, + &sensor_dev_attr_update_rate.dev_attr.attr, + NULL +}; + +static const struct attribute_group omap_temp_sensor_group = { + .attrs = omap_temp_sensor_attributes, +}; + +static int omap_temp_sensor_clk_enable (struct omap_temp_sensor *temp_sensor) +{ + u32 ret = 0; + + if (temp_sensor->clk_on) { + dev_err(temp_sensor->hwmon_dev, "clock already on\n"); + goto out; + } + + ret = pm_runtime_get_sync(temp_sensor->hwmon_dev); + if (ret < 0) { + dev_err(temp_sensor->hwmon_dev, "get sync failed\n"); + goto out;
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 563
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ } + + mutex_lock(&temp_sensor->sensor_mutex); + temp_sensor->clk_on = 1; + mutex_unlock(&temp_sensor->sensor_mutex); +Completely unnecessary lock.
Ok
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 568
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +5
-0+out: + return ret; +} + +static int omap_temp_sensor_clk_disable (struct omap_temp_sensor *temp_sensor)Return code is not used anywhere, why bother providing one ?
It is not checked in the error path. I can make it void.
Quoted 9 lines
drivers/hwmon/omap_temp_sensor.c line 574
Quoted 7 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+{ + u32 ret = 0; + + if (!temp_sensor->clk_on) { + dev_err(temp_sensor->hwmon_dev, "clock already off\n"); + goto out;... and since you ignore the error anyway, why log it ?
Ok. I will remove the log message.
Quoted 16 lines
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +3
-0+ } + + /* Gate the clock */drivers/hwmon/omap_temp_sensor.c line 587
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ ret = pm_runtime_put_sync(temp_sensor->hwmon_dev); + if (ret < 0) { + dev_err(temp_sensor->hwmon_dev, "put sync failed\n"); + goto out; + } + + mutex_lock(&temp_sensor->sensor_mutex); + temp_sensor->clk_on = 0; + mutex_unlock(&temp_sensor->sensor_mutex); +Completely unnecessary lock.
Ok
Quoted 61 lines
Quoted 61 lines
drivers/hwmon/omap_temp_sensor.c new file +60
-0+out: + return ret; +} + +static irqreturn_t omap_talert_irq_handler (int irq,void *data) +{ + struct omap_temp_sensor *temp_sensor; + int t_hot, t_cold, temp; + + temp_sensor = data; + mutex_lock(&temp_sensor->sensor_mutex); + + /* Read the status of t_hot */ + t_hot = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_hot_mask; + + /* Read the status of t_cold */ + t_cold = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_cold_mask; + + temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + /* + * One TALERT interrupt: Two sources + * If the interrupt is due to t_hot then mask t_hot and + * and unmask t_cold else mask t_cold and unmask t_hot + */ + if (t_hot) { + temp &= ~temp_sensor->registers->mask_hot_mask; + temp |= temp_sensor->registers->mask_cold_mask; + } else if (t_cold) { + temp &= ~temp_sensor->registers->mask_cold_mask; + temp |= temp_sensor->registers->mask_hot_mask; + } + + omap_temp_sensor_writel(temp_sensor, temp, + temp_sensor->registers->bgap_mask_ctrl); + + /* kobject_uvent to user space telling thermal threshold crossed */ + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_CHANGE); + + mutex_unlock(&temp_sensor->sensor_mutex); + + return IRQ_HANDLED; +} + +static int __devinit omap_temp_sensor_probe (struct platform_device *pdev) +{ + struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; + struct omap_temp_sensor *temp_sensor; + struct resource *mem; + int ret = 0; + int val, clk_rate; + u32 max_freq, min_freq; + + if (!pdata) { + dev_err(&pdev->dev, "platform data missing\n"); + return -EINVAL;
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 653
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ } + + temp_sensor = kzalloc(sizeof(*temp_sensor), GFP_KERNEL); + if (!temp_sensor) + return -ENOMEM; +You have error messages all over the place, Why not here ?
Ok. I will add an error message here.
Quoted 13 lines
drivers/hwmon/omap_temp_sensor.c line 662
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +9
-0+ mutex_init(&temp_sensor->sensor_mutex); + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource\n"); + ret = -ENOMEM; + goto plat_res_err; + } +If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Ok. Here if i fail i am releasing temp_sensor memory which has been allocated.
Quoted 27 lines
Quoted 27 lines
drivers/hwmon/omap_temp_sensor.c new file +26
-0+ temp_sensor->irq = platform_get_irq_byname(pdev, "thermal_alert"); + if (temp_sensor->irq < 0) { + dev_err(&pdev->dev, "get_irq_byname failed\n"); + ret = temp_sensor->irq; + goto plat_res_err; + } + + temp_sensor->phy_base = ioremap(mem->start, resource_size(mem)); + temp_sensor->clock = NULL; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + + if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + } else { + max_freq = MAX_FREQ; + min_freq = MIN_FREQ; + } + + pm_runtime_enable(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + + /* +* check if the efuse has a non-zero value if not +* it is an untrimmed sample and the temperatures
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 694
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+* may not be accurate +*/ + + if (omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_efuse)) { + temp_sensor->is_efuse_valid = 1;
is_efuse_validis not used anywhere. Might as well drop it.
Ok
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c line 696
Quoted 3 lines
drivers/hwmon/omap_temp_sensor.c new file +2
-0+ dev_dbg(temp_sensor->hwmon_dev, + "Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n");
dev_infomight be better here.
Ok.
Quoted 17 lines
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ } + + dev_set_drvdata(&pdev->dev, temp_sensor); + temp_sensor->clock = clk_get(temp_sensor->hwmon_dev, "fck");drivers/hwmon/omap_temp_sensor.c line 710
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ if (IS_ERR(temp_sensor->clock)) { + ret = PTR_ERR(temp_sensor->clock); + dev_err(temp_sensor->hwmon_dev, + "unable to get fclk: %d\n", ret); + goto plat_res_err; + } + + ret = omap_temp_sensor_clk_enable(temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Cannot enable temp sensor\n");
omap_temp_sensor_clk_enable()already generates error messages.
Ok. I will remove this.
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c line 717
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c new file +7
-0+ goto clken_err; + } + + clk_rate = clk_round_rate(temp_sensor->clock, max_freq); + if (clk_rate < min_freq || clk_rate == 0xffffffff) { + dev_err(&pdev->dev, "Error round rate\n"); + ret = -EDOM;EDOM is wrong. Please use ENODEV.
Ok
Quoted 3 lines
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Ok
Quoted 10 lines
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c new file +9
-0+ goto clken_err; + } + + ret = clk_set_rate(temp_sensor->clock, clk_rate); + if (ret) { + dev_err(&pdev->dev, "Cannot set clock rate\n"); + goto clken_err; + } +
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 731
Quoted 5 lines
drivers/hwmon/omap_temp_sensor.c new file +5
-0+ temp_sensor->clk_rate = clk_rate; + omap_enable_continuous_mode(temp_sensor, 1); + omap_configure_temp_sensor_thresholds(temp_sensor); + /* 1 ms */ + omap_configure_temp_sensor_counter(temp_sensor, 1);If
temp_sensor->clk_rate* 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would betemp_sensor->clk_rate/ 1000.
Yes! Wrong comment. It is configured to start the temperature conversion. It represents 1 clock cycle of the temperature sensor fclk. I will correct this.
Quoted 16 lines
Quoted 16 lines
drivers/hwmon/omap_temp_sensor.c new file +15
-0+ + /* Wait till the first conversion is done wait for at least 1ms */ + usleep_range(1000, 2000); + + /* Read the temperature once due to hw issue*/ + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + + /* Set 2 seconds time as default counter */ + omap_configure_temp_sensor_counter(temp_sensor, + temp_sensor->clk_rate * 2); + + temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev); + if (IS_ERR(temp_sensor->hwmon_dev)) { + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 752
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+ ret = PTR_ERR(temp_sensor->hwmon_dev); + goto hwmon_reg_err; + } + + ret = sysfs_create_group(&pdev->dev.kobj, + &omap_temp_sensor_group);one line is sufficient here.
Ok.
Quoted 10 lines
drivers/hwmon/omap_temp_sensor.c line 759
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c new file +7
-0+ if (ret) { + dev_err(&pdev->dev, "could not create sysfs files\n"); + goto sysfs_create_err; + } + + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_ADD); +No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Yes i will remove this.
Quoted 11 lines
Quoted 11 lines
drivers/hwmon/omap_temp_sensor.c new file +10
-0+ ret = request_threaded_irq(temp_sensor->irq, NULL, + omap_talert_irq_handler, IRQF_TRIGGER_RISING | IRQF_ONESHOT, + "temp_sensor", temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Request threaded irq failed.\n"); + goto req_irq_err; + } + + /* unmask the T_COLD and unmask T_HOT at init */ + val = omap_temp_sensor_readl(temp_sensor,
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 776
Quoted 7 lines
drivers/hwmon/omap_temp_sensor.c new file +7
-0+ temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +This is racy. From
Documentation/hwmon/submitting-patches:
I read it. If i enable the interrupts before i have the sysfs infrastructure ready.
I am likely to miss out on the uevent notification if the interrupt occurs as soon as
i unmask. Correct me if my understanding is wrong.
Quoted 11 lines
* Make sure there are no race conditions in the probe function. Specifically, completely initialize your chip first, then create sysfs entries and register with the hwmon subsystem.
drivers/hwmon/omap_temp_sensor.c line 780
Quoted 4 lines
drivers/hwmon/omap_temp_sensor.c new file +4
-0+ return 0; + +req_irq_err: + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_REMOVE);Same as before; why is this needed here but not in any other hwmon driver ?
Ok. I will remove this.
Quoted 16 lines
Quoted 16 lines
drivers/hwmon/omap_temp_sensor.c new file +15
-0+ sysfs_remove_group(&temp_sensor->hwmon_dev->kobj, + &omap_temp_sensor_group); +sysfs_create_err: + hwmon_device_unregister(&pdev->dev); +hwmon_reg_err: + omap_temp_sensor_clk_disable(temp_sensor); +clken_err: + clk_put(temp_sensor->clock); +plat_res_err: + dev_set_drvdata(&pdev->dev, NULL); + mutex_destroy(&temp_sensor->sensor_mutex); + kfree(temp_sensor); + return ret; +} +
Quoted 8 lines
drivers/hwmon/omap_temp_sensor.c line 801
Quoted 6 lines
drivers/hwmon/omap_temp_sensor.c new file +6
-0+static int __devexit omap_temp_sensor_remove(struct platform_device *pdev) +{ + struct omap_temp_sensor *temp_sensor = platform_get_drvdata(pdev); + + hwmon_device_unregister(&pdev->dev); + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_REMOVE);... and again.
Ok. I will remove this.
Quoted 137 lines
Quoted 136 lines
drivers/hwmon/omap_temp_sensor.c new file +132
-1+ sysfs_remove_group(&temp_sensor->hwmon_dev->kobj, + &omap_temp_sensor_group); + omap_temp_sensor_clk_disable(temp_sensor); + free_irq(temp_sensor->irq, temp_sensor); + clk_put(temp_sensor->clock); + dev_set_drvdata(&pdev->dev, NULL); + mutex_destroy(&temp_sensor->sensor_mutex); + kfree(temp_sensor); + + return 0; +} + +#ifdef CONFIG_PM +static void omap_temp_sensor_save_ctxt (struct omap_temp_sensor *temp_sensor) +{ + temp_sensor->temp_sensor_ctrl = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + temp_sensor->bg_ctrl = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + temp_sensor->bg_counter = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_counter); + temp_sensor->bg_threshold = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + temp_sensor->temp_sensor_tshut_threshold = + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->thsut_threshold); +} + +static void omap_temp_sensor_restore_ctxt (struct omap_temp_sensor *temp_sensor) +{ + omap_temp_sensor_writel(temp_sensor, + temp_sensor->temp_sensor_ctrl, + temp_sensor->registers->temp_sensor_ctrl); + omap_temp_sensor_writel(temp_sensor, + temp_sensor->bg_ctrl, + temp_sensor->registers->bgap_mask_ctrl); + omap_temp_sensor_writel(temp_sensor, + temp_sensor->bg_counter, + temp_sensor->registers->bgap_counter); + omap_temp_sensor_writel(temp_sensor, + temp_sensor->bg_threshold, + temp_sensor->registers->bgap_threshold); + omap_temp_sensor_writel(temp_sensor, + temp_sensor->temp_sensor_tshut_threshold, + temp_sensor->registers->thsut_threshold); +} + +static int omap_temp_sensor_suspend (struct device *dev) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + + omap_temp_sensor_save_ctxt(temp_sensor); + + return 0; +} + +static int omap_temp_sensor_resume (struct device *dev) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + + omap_temp_sensor_restore_ctxt(temp_sensor); + + return 0; +} + +static int omap_temp_sensor_runtime_suspend (struct device *dev) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + + omap_temp_sensor_save_ctxt(temp_sensor); + + return 0; +} + +static int omap_temp_sensor_runtime_resume (struct device *dev) +{ + static int context_loss_count; + int temp; + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + + temp = omap_device_get_context_loss_count(to_platform_device(dev)); + + if (temp != context_loss_count && context_loss_count != 0) + omap_temp_sensor_restore_ctxt(temp_sensor); + + context_loss_count = temp; + + return 0; +} + +static int omap_temp_sensor_idle (struct device *dev) +{ + return 0; +} + +static const struct dev_pm_ops omap_temp_sensor_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(omap_temp_sensor_suspend, + omap_temp_sensor_resume) + SET_RUNTIME_PM_OPS(omap_temp_sensor_runtime_suspend, + omap_temp_sensor_runtime_resume, omap_temp_sensor_idle) +}; + +#define DEV_PM_OPS (&omap_temp_sensor_dev_pm_ops) +#else +#define DEV_PM_OPS NULL +#endif + +static struct platform_driver omap_temp_sensor_driver = { + .probe = omap_temp_sensor_probe, + .remove = omap_temp_sensor_remove, + .driver = { + .name = "omap_temp_sensor", + .pm = DEV_PM_OPS, + }, +}; + +int __init omap_temp_sensor_init (void ) +{ + return platform_driver_register(&omap_temp_sensor_driver); +} +module_init (omap_temp_sensor_init); + +static void __exit omap_temp_sensor_exit(void) +{ + platform_driver_unregister(&omap_temp_sensor_driver); +} +module_exit (omap_temp_sensor_exit); + +MODULE_DESCRIPTION ("OMAP446X temperature sensor Driver" ); +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("platform:" DRIVER_NAME ); +MODULE_AUTHOR ("J Keerthy <[email protected]>" ); --1.7.0.4
Signature · 6 lines
Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 25 lines · KEERTHY
Quoted 23 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 9 lines
--- 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
[ ... ]
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
(quoted diff) +10
-0+/* 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 2 lines
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.
Quoted 76 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +12
-0+ } + + 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ } + + t_hot = temp_to_adc_conversion(val);Bad error return check. Negative on error. Should be
if (t_hot < 0) return t_hot;Ok.
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ count = -EINVAL; + goto out;This will try to release the not-acquired lock.
I will correct this.
Quoted 17 lines
Review
Quoted 13 lines
(quoted diff) +12
-0+ } + + 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.
One option would be to update t_cold (max_hyst) automatically in that case.
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.
Quoted 70 lines
Quoted 35 lines
Quoted 35 lines
(quoted diff) +34
-0+ } + + /* 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) {Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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_OFFSEThere butbgap_mask_ctrlelsewhere ?Ok. I will change this.
Quoted 15 lines
Quoted 15 lines
(quoted diff) +14
-0+ } + /* + * 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) +{Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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_maskcan 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 inshow_temp_max()is almost the same and does not use the lock.
Quoted 4 lines
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 ?
If it is changed outside the driver, you have a real problem, since there is no guarantee that the mask is not changed from outside the driver, by code not protected by the mutex. And if read functions have the side effect of changing such values, there should really be a note somewhere to make people aware of it. I'd call that side effect programming, and would argue it is a really bad idea.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
(quoted diff) +9
-0+ temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + temp = (temp & temp_sensor->registers->threshold_tcold_mask) >> + __ffs(temp_sensor->registers->threshold_tcold_mask); + mutex_unlock(&temp_sensor->sensor_mutex); + + if (temp < OMAP_ADC_START_VALUE || temp > OMAP_ADC_END_VALUE) { + dev_err(dev, "invalid value\n"); + return -EDOM;
Quoted 6 lines
Quoted 3 lines
"Math argument out of domain of func" does not make sense.
Unless you don't know anything better, use EIO (I/O Error). Not very specific, but at least not misleading.Out of range i assumed this was the closest error value. I will use EIO.
It is not a math error, though.
Quoted 145 lines
Quoted 3 lines
Besides, the log message is unnecessary. The user is informed with
-EIO, and the admin won't be able to do anything useful with "invalid value".Ok. I will return
-EIOand eliminate the log message.Quoted 13 lines
Quoted 13 lines
(quoted diff) +12
-0+ } + + temp = adc_to_temp_conversion(temp); + + return snprintf(buf, 16, "%d\n", temp); +} + +static ssize_t set_temp_max_hyst(struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ u32 reg_val, t_hot, t_cold, temp; + long val; + + if (strict_strtol(buf, 10, &val)) { + count = -EINVAL; + goto out;This will try to release the not acquired lock.
I will correct this.
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ } + + t_cold = temp_to_adc_conversion(val);Bad error return check; same as above.
Ok. I will correct this.
Quoted 59 lines
Quoted 4 lines
(quoted diff) +3
-0+ if (t_cold < OMAP_ADC_START_VALUE || t_cold > OMAP_ADC_END_VALUE) { + count = -EINVAL; + goto out;Review
Quoted 10 lines
(quoted diff) +10
-0+ } + + mutex_lock(&temp_sensor->sensor_mutex); + /* obtain the T HOT value */ + t_hot = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + t_hot = (t_hot & temp_sensor->registers->threshold_thot_mask) >> + __ffs(temp_sensor->registers->threshold_thot_mask); + + if (t_cold > t_hot) {Same concern as above. Change order is left to application which should not be bothered.
Quoted 42 lines
(quoted diff) +42
-0+ count = -EINVAL; + goto out; + } + + /* write the new t_cold value */ + reg_val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + reg_val &= ~temp_sensor->registers->threshold_tcold_mask; + reg_val |= t_cold << + __ffs(temp_sensor->registers->threshold_tcold_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 LOW threshold(t_cold) lower than the current + * temperature(temp) unmask the COLD interrupts + */ + if (t_cold < temp) { + reg_val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + reg_val &= ~temp_sensor->registers->mask_hot_mask; + reg_val |= temp_sensor->registers->mask_cold_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_hot && temp > t_cold) { + 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + /* + * else no need to do anything since HW will immediately compare + * the new threshold and generate interrupt accordingly + */Would be nice if you could unify the interrupt enable code in a single function.
Ok. I will have a function for this.
Quoted 16 lines
Quoted 4 lines
(quoted diff) +3
-0+ +out: + mutex_unlock(&temp_sensor->sensor_mutex);Review
Quoted 10 lines
(quoted diff) +10
-0+ + return count; +} + +static ssize_t show_update_rate(struct device *dev, + struct device_attribute *devattr, char *buf) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + u32 temp = 0, ret = 0; +No need to initialize temp.
ret is u32, which doesn't seem to be a good type to use for negative values.Oops yes. I will correct this.
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ + if (!temp_sensor->clk_rate) { + dev_err(dev, "clk_rate is NULL\n"); + ret = -EDOM;Same comment as above (this is not a math error). And you don't have to check
clk_rateanyway, since it is set in probe, and probe bails out if it is not set.Ok
Quoted 6 lines
Quoted 5 lines
(quoted diff) +4
-0+ goto out; + } + + mutex_lock(&temp_sensor->sensor_mutex);
Quoted 6 lines
Quoted 2 lines
As with other show locks, this one would only make sense if
counter_maskcan change during runtime, and if that change would be protected by the mutex. This is not the case, thus the lock is unnecessary.Counter value can be altered runtime. A lock is needed in both the cases. I will add it.
clk_rate does not change, counter_mask does not change, and reading a single value doesn't need a mutex. Am I missing something ?
Quoted 448 lines
Quoted 31 lines
Quoted 31 lines
(quoted diff) +30
-0+ temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_counter); + temp = (temp & temp_sensor->registers->counter_mask) >> + __ffs(temp_sensor->registers->counter_mask); + mutex_unlock(&temp_sensor->sensor_mutex); + temp = temp * 1000 / temp_sensor->clk_rate; + +out: + if (!ret) + return sprintf(buf, "%d\n", temp); + + return ret; +} + +static ssize_t set_update_rate(struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + u32 reg_val; + long val; + + if (strict_strtol(buf, 10, &val)) { + count = -EINVAL; + goto out; + } + + val *= temp_sensor->clk_rate / 1000; + mutex_lock(&temp_sensor->sensor_mutex); + reg_val = omap_temp_sensor_readl(temp_sensor,Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ temp_sensor->registers->bgap_counter); + + reg_val &= ~temp_sensor->registers->counter_mask; + reg_val |= val;Sure you want to accept any value (including negative ones) here ? val is not shifted, and not masked against
counter_mask, meaning it can write all bits of the register. That doesn't sound right.I will have a check.
Quoted 3 lines
It would make sense to call
omap_configure_temp_sensor_counter()instead of duplicating the set functionality.Yes I will make use of that.
Quoted 12 lines
Quoted 12 lines
(quoted diff) +11
-0+ omap_temp_sensor_writel(temp_sensor, reg_val, + temp_sensor->registers->bgap_counter); + mutex_unlock(&temp_sensor->sensor_mutex); + +out: + return count; +} + +static int omap_temp_sensor_read_temp(struct device *dev, + struct device_attribute *devattr, + char *buf)Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + int temp, ret = 0; + + mutex_lock(&temp_sensor->sensor_mutex);Unless
bgap_dtemp_maskcan change during runtime, and that change is protected by the mutex, this lock is unnecessary.It is read only and is updated by hardware. Change cannot be protected by mutex.
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + temp &= temp_sensor->registers->bgap_dtemp_mask; + + /* look up for temperature in the table and return the temperature */ + if (temp < OMAP_ADC_START_VALUE || temp > OMAP_ADC_END_VALUE) { + dev_err(dev, "invalid adc code reported %d", temp); + ret = -EDOM;Another wrong use of EDOM, and another value-free error message.
Ok. I will use
-EIOand drop the error message.Quoted 17 lines
Quoted 17 lines
(quoted diff) +16
-0+ goto out; + } + + temp = adc_to_temp[temp - OMAP_ADC_START_VALUE]; + +out: + mutex_unlock(&temp_sensor->sensor_mutex); + if (!ret) + return sprintf(buf, "%d\n", temp); + + return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, omap_temp_sensor_read_temp, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max,Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ set_temp_max, 0); +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, show_temp_max_hyst, + set_temp_max_hyst, 0); +static SENSOR_DEVICE_ATTR(update_rate, S_IWUSR | S_IRUGO, show_update_rate, + set_update_rate, 0); +ABI is
update_interval. Please use it.Ok.
Quoted 26 lines
Quoted 26 lines
(quoted diff) +25
-0+static struct attribute *omap_temp_sensor_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_temp1_max.dev_attr.attr, + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr, + &sensor_dev_attr_update_rate.dev_attr.attr, + NULL +}; + +static const struct attribute_group omap_temp_sensor_group = { + .attrs = omap_temp_sensor_attributes, +}; + +static int omap_temp_sensor_clk_enable(struct omap_temp_sensor *temp_sensor) +{ + u32 ret = 0; + + if (temp_sensor->clk_on) { + dev_err(temp_sensor->hwmon_dev, "clock already on\n"); + goto out; + } + + ret = pm_runtime_get_sync(temp_sensor->hwmon_dev); + if (ret < 0) { + dev_err(temp_sensor->hwmon_dev, "get sync failed\n"); + goto out;Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + mutex_lock(&temp_sensor->sensor_mutex); + temp_sensor->clk_on = 1; + mutex_unlock(&temp_sensor->sensor_mutex); +Completely unnecessary lock.
Ok
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+out: + return ret; +} + +static int omap_temp_sensor_clk_disable(struct omap_temp_sensor *temp_sensor)Return code is not used anywhere, why bother providing one ?
It is not checked in the error path. I can make it void.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+{ + u32 ret = 0; + + if (!temp_sensor->clk_on) { + dev_err(temp_sensor->hwmon_dev, "clock already off\n"); + goto out;... and since you ignore the error anyway, why log it ?
Ok. I will remove the log message.
Quoted 16 lines
Quoted 4 lines
(quoted diff) +3
-0+ } + + /* Gate the clock */Review
Quoted 10 lines
(quoted diff) +10
-0+ ret = pm_runtime_put_sync(temp_sensor->hwmon_dev); + if (ret < 0) { + dev_err(temp_sensor->hwmon_dev, "put sync failed\n"); + goto out; + } + + mutex_lock(&temp_sensor->sensor_mutex); + temp_sensor->clk_on = 0; + mutex_unlock(&temp_sensor->sensor_mutex); +Completely unnecessary lock.
Ok
Quoted 61 lines
Quoted 61 lines
(quoted diff) +60
-0+out: + return ret; +} + +static irqreturn_t omap_talert_irq_handler(int irq, void *data) +{ + struct omap_temp_sensor *temp_sensor; + int t_hot, t_cold, temp; + + temp_sensor = data; + mutex_lock(&temp_sensor->sensor_mutex); + + /* Read the status of t_hot */ + t_hot = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_hot_mask; + + /* Read the status of t_cold */ + t_cold = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_cold_mask; + + temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + /* + * One TALERT interrupt: Two sources + * If the interrupt is due to t_hot then mask t_hot and + * and unmask t_cold else mask t_cold and unmask t_hot + */ + if (t_hot) { + temp &= ~temp_sensor->registers->mask_hot_mask; + temp |= temp_sensor->registers->mask_cold_mask; + } else if (t_cold) { + temp &= ~temp_sensor->registers->mask_cold_mask; + temp |= temp_sensor->registers->mask_hot_mask; + } + + omap_temp_sensor_writel(temp_sensor, temp, + temp_sensor->registers->bgap_mask_ctrl); + + /* kobject_uvent to user space telling thermal threshold crossed */ + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_CHANGE); + + mutex_unlock(&temp_sensor->sensor_mutex); + + return IRQ_HANDLED; +} + +static int __devinit omap_temp_sensor_probe(struct platform_device *pdev) +{ + struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; + struct omap_temp_sensor *temp_sensor; + struct resource *mem; + int ret = 0; + int val, clk_rate; + u32 max_freq, min_freq; + + if (!pdata) { + dev_err(&pdev->dev, "platform data missing\n"); + return -EINVAL;Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + temp_sensor = kzalloc(sizeof(*temp_sensor), GFP_KERNEL); + if (!temp_sensor) + return -ENOMEM; +You have error messages all over the place, Why not here ?
Ok. I will add an error message here.
Quoted 13 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+ mutex_init(&temp_sensor->sensor_mutex); + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource\n"); + ret = -ENOMEM; + goto plat_res_err; + } +If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Ok. Here if i fail i am releasing
temp_sensormemory which has been allocated.Quoted 27 lines
Quoted 27 lines
(quoted diff) +26
-0+ temp_sensor->irq = platform_get_irq_byname(pdev, "thermal_alert"); + if (temp_sensor->irq < 0) { + dev_err(&pdev->dev, "get_irq_byname failed\n"); + ret = temp_sensor->irq; + goto plat_res_err; + } + + temp_sensor->phy_base = ioremap(mem->start, resource_size(mem)); + temp_sensor->clock = NULL; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + + if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + } else { + max_freq = MAX_FREQ; + min_freq = MIN_FREQ; + } + + pm_runtime_enable(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + + /* + * check if the efuse has a non-zero value if not + * it is an untrimmed sample and the temperaturesQuoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ * may not be accurate + */ + + if (omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_efuse)) { + temp_sensor->is_efuse_valid = 1;
is_efuse_validis not used anywhere. Might as well drop it.Ok
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ dev_dbg(temp_sensor->hwmon_dev, + "Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n");
dev_infomight be better here.Ok.
Quoted 17 lines
Quoted 5 lines
(quoted diff) +4
-0+ } + + dev_set_drvdata(&pdev->dev, temp_sensor); + temp_sensor->clock = clk_get(temp_sensor->hwmon_dev, "fck");Review
Quoted 10 lines
(quoted diff) +10
-0+ if (IS_ERR(temp_sensor->clock)) { + ret = PTR_ERR(temp_sensor->clock); + dev_err(temp_sensor->hwmon_dev, + "unable to get fclk: %d\n", ret); + goto plat_res_err; + } + + ret = omap_temp_sensor_clk_enable(temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Cannot enable temp sensor\n");
omap_temp_sensor_clk_enable()already generates error messages.Ok. I will remove this.
Quoted 10 lines
Review
Quoted 8 lines
(quoted diff) +7
-0+ goto clken_err; + } + + clk_rate = clk_round_rate(temp_sensor->clock, max_freq); + if (clk_rate < min_freq || clk_rate = 0xffffffff) { + dev_err(&pdev->dev, "Error round rate\n"); + ret = -EDOM;EDOM is wrong. Please use ENODEV.
Ok
Quoted 3 lines
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Ok
Quoted 10 lines
Quoted 10 lines
(quoted diff) +9
-0+ goto clken_err; + } + + ret = clk_set_rate(temp_sensor->clock, clk_rate); + if (ret) { + dev_err(&pdev->dev, "Cannot set clock rate\n"); + goto clken_err; + } +Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ temp_sensor->clk_rate = clk_rate; + omap_enable_continuous_mode(temp_sensor, 1); + omap_configure_temp_sensor_thresholds(temp_sensor); + /* 1 ms */ + omap_configure_temp_sensor_counter(temp_sensor, 1);If
temp_sensor->clk_rate* 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would betemp_sensor->clk_rate/ 1000.Yes! Wrong comment. It is configured to start the temperature conversion. It represents 1 clock cycle of the temperature sensor fclk. I will correct this.
Quoted 16 lines
Quoted 16 lines
(quoted diff) +15
-0+ + /* Wait till the first conversion is done wait for at least 1ms */ + usleep_range(1000, 2000); + + /* Read the temperature once due to hw issue*/ + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + + /* Set 2 seconds time as default counter */ + omap_configure_temp_sensor_counter(temp_sensor, + temp_sensor->clk_rate * 2); + + temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev); + if (IS_ERR(temp_sensor->hwmon_dev)) { + dev_err(&pdev->dev, "hwmon_device_register failed.\n");Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ ret = PTR_ERR(temp_sensor->hwmon_dev); + goto hwmon_reg_err; + } + + ret = sysfs_create_group(&pdev->dev.kobj, + &omap_temp_sensor_group);one line is sufficient here.
Ok.
Quoted 10 lines
Review
Quoted 8 lines
(quoted diff) +7
-0+ if (ret) { + dev_err(&pdev->dev, "could not create sysfs files\n"); + goto sysfs_create_err; + } + + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_ADD); +No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Yes i will remove this.
Quoted 11 lines
Quoted 11 lines
(quoted diff) +10
-0+ ret = request_threaded_irq(temp_sensor->irq, NULL, + omap_talert_irq_handler, IRQF_TRIGGER_RISING | IRQF_ONESHOT, + "temp_sensor", temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Request threaded irq failed.\n"); + goto req_irq_err; + } + + /* unmask the T_COLD and unmask T_HOT at init */ + val = omap_temp_sensor_readl(temp_sensor,Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +This is racy. From
Documentation/hwmon/submitting-patches:
Quoted 6 lines
I read it. If i enable the interrupts before i have the sysfs infrastructure ready.
I am likely to miss out on the uevent notification if the interrupt occurs as soon as
i unmask. Correct me if my understanding is wrong.
See other email on sequence subject. I'll have to look into it in more detail. later - no time right now ...
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 15 lines · Guenter Roeck
Quoted 14 lines · KEERTHY
Quoted 12 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>
[ ... ]
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 4 lines
(quoted diff) +2
-0+ } +Review
Quoted 10 lines
(quoted diff) +10
-0+ 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.
Quoted 8 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.Problem here is that you expect the application to know, depending on the current values of (max,
max_hyst), how to update both limits in order. That is not a reasonable expectation.
One possible solution would be to have a single function to set both t_cold and t_hot, and call it from both initialization code and from the set functions. That would unify all the register setting and interrupt enable code.
Regarding the actual values to set, you can (as an example) use the following approach:
- If max is set, and t_hot < t_cold, adjust t_cold to the new value of t_hot.
- if max_hyst is set, and t_cold > t_hot, set t_cold to the new value of t_hot.
So, in other words, your new unified function would only need the following simple validation:
if (t_cold > t_hot) t_cold = t_hot;
[ ... ]
Quoted 208 lines
Quoted 208 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +13
-0+ +static int __devinit omap_temp_sensor_probe(struct platform_device *pdev) +{ + struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; + struct omap_temp_sensor *temp_sensor; + struct resource *mem; + int ret = 0; + int val, clk_rate; + u32 max_freq, min_freq; + + if (!pdata) { + dev_err(&pdev->dev, "platform data missing\n"); + return -EINVAL;Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + temp_sensor = kzalloc(sizeof(*temp_sensor), GFP_KERNEL); + if (!temp_sensor) + return -ENOMEM; +You have error messages all over the place, Why not here ?
Ok. I will add an error message here.
Quoted 13 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+ mutex_init(&temp_sensor->sensor_mutex); + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource\n"); + ret = -ENOMEM; + goto plat_res_err; + } +If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Ok. Here if i fail i am releasing
temp_sensormemory which has been allocated.Quoted 27 lines
Quoted 27 lines
(quoted diff) +26
-0+ temp_sensor->irq = platform_get_irq_byname(pdev, "thermal_alert"); + if (temp_sensor->irq < 0) { + dev_err(&pdev->dev, "get_irq_byname failed\n"); + ret = temp_sensor->irq; + goto plat_res_err; + } + + temp_sensor->phy_base = ioremap(mem->start, resource_size(mem)); + temp_sensor->clock = NULL; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + + if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + } else { + max_freq = MAX_FREQ; + min_freq = MIN_FREQ; + } + + pm_runtime_enable(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + + /* + * check if the efuse has a non-zero value if not + * it is an untrimmed sample and the temperaturesQuoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ * may not be accurate + */ + + if (omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_efuse)) { + temp_sensor->is_efuse_valid = 1;
is_efuse_validis not used anywhere. Might as well drop it.Ok
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ dev_dbg(temp_sensor->hwmon_dev, + "Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n");
dev_infomight be better here.Ok.
Quoted 17 lines
Quoted 5 lines
(quoted diff) +4
-0+ } + + dev_set_drvdata(&pdev->dev, temp_sensor); + temp_sensor->clock = clk_get(temp_sensor->hwmon_dev, "fck");Review
Quoted 10 lines
(quoted diff) +10
-0+ if (IS_ERR(temp_sensor->clock)) { + ret = PTR_ERR(temp_sensor->clock); + dev_err(temp_sensor->hwmon_dev, + "unable to get fclk: %d\n", ret); + goto plat_res_err; + } + + ret = omap_temp_sensor_clk_enable(temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Cannot enable temp sensor\n");
omap_temp_sensor_clk_enable()already generates error messages.Ok. I will remove this.
Quoted 10 lines
Review
Quoted 8 lines
(quoted diff) +7
-0+ goto clken_err; + } + + clk_rate = clk_round_rate(temp_sensor->clock, max_freq); + if (clk_rate < min_freq || clk_rate = 0xffffffff) { + dev_err(&pdev->dev, "Error round rate\n"); + ret = -EDOM;EDOM is wrong. Please use ENODEV.
Ok
Quoted 3 lines
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Ok
Quoted 10 lines
Quoted 10 lines
(quoted diff) +9
-0+ goto clken_err; + } + + ret = clk_set_rate(temp_sensor->clock, clk_rate); + if (ret) { + dev_err(&pdev->dev, "Cannot set clock rate\n"); + goto clken_err; + } +Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ temp_sensor->clk_rate = clk_rate; + omap_enable_continuous_mode(temp_sensor, 1); + omap_configure_temp_sensor_thresholds(temp_sensor); + /* 1 ms */ + omap_configure_temp_sensor_counter(temp_sensor, 1);If
temp_sensor->clk_rate* 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would betemp_sensor->clk_rate/ 1000.Yes! Wrong comment. It is configured to start the temperature conversion. It represents 1 clock cycle of the temperature sensor fclk. I will correct this.
Quoted 16 lines
Quoted 16 lines
(quoted diff) +15
-0+ + /* Wait till the first conversion is done wait for at least 1ms */ + usleep_range(1000, 2000); + + /* Read the temperature once due to hw issue*/ + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + + /* Set 2 seconds time as default counter */ + omap_configure_temp_sensor_counter(temp_sensor, + temp_sensor->clk_rate * 2); + + temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev); + if (IS_ERR(temp_sensor->hwmon_dev)) { + dev_err(&pdev->dev, "hwmon_device_register failed.\n");Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ ret = PTR_ERR(temp_sensor->hwmon_dev); + goto hwmon_reg_err; + } + + ret = sysfs_create_group(&pdev->dev.kobj, + &omap_temp_sensor_group);one line is sufficient here.
Ok.
Quoted 10 lines
Review
Quoted 8 lines
(quoted diff) +7
-0+ if (ret) { + dev_err(&pdev->dev, "could not create sysfs files\n"); + goto sysfs_create_err; + } + + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_ADD); +No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Yes i will remove this.
Quoted 19 lines
Quoted 8 lines
(quoted diff) +7
-0+ ret = request_threaded_irq(temp_sensor->irq, NULL, + omap_talert_irq_handler, IRQF_TRIGGER_RISING | IRQF_ONESHOT, + "temp_sensor", temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Request threaded irq failed.\n"); + goto req_irq_err; + }Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* unmask the T_COLD and unmask T_HOT at init */ + val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +This is racy. From
Documentation/hwmon/submitting-patches:
Quoted 8 lines
Quoted 6 lines
I read it. If i enable the interrupts before i have the sysfs infrastructure ready.
I am likely to miss out on the uevent notification if the interrupt occurs as soon as
i unmask. Correct me if my understanding is wrong.See other email on sequence subject. I'll have to look into it in more detail. later - no time right now ...
Concerns:
- create sysfs entries first, then register with hwmon. hwmon_device_register is usually the last call, made only after everything is known to work.
- You can definitely request the interrupt before registering sysfs entries
- temp_sensor->hwmon_dev is initially set to
temp_sensor->hwmon_dev = &pdev->dev; then overwritten with temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev);
This is, at the very least, odd and doesn't seem to make sense.
- Interrupt enable code (setting bgap_mask_ctrl) in probe function occurs after sysfs files have been enabled, and the access is not mutex protected. Thus, sysfs access can already happen, causing the main race I was concerned about.
My suggestion is
- request interrupt prior to sysfs creation and hwmon registration.
- swap sysfs file creation and hwmon registration.
- Use the unified function I suggested above to set t_cold, t_hot, and to enable interrupts as needed. Since that function will be mutex protected, you can call it after hwmon registration, and avoid the race.
Also look into use of temp_sensor->hwmon_dev vs. &pdev->dev. That code just doesn't look right.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 318 lines · Guenter Roeck
Quoted 15 lines · Guenter Roeck
Quoted 14 lines · KEERTHY
Quoted 12 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>[ ... ]
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 4 lines
+ }
(quoted diff) +1
-0+Review
Quoted 10 lines
(quoted diff) +10
-0+ 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.Quoted 8 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.Problem here is that you expect the application to know, depending on the current values of (max,
max_hyst), how to update both limits in order. That is not a reasonable expectation.One possible solution would be to have a single function to set both
t_coldandt_hot, and call it from both initialization code and from the set functions. That would unify all the register setting and interrupt enable code.Regarding the actual values to set, you can (as an example) use the following approach:
- If max is set, and
t_hot<t_cold, adjustt_coldto the new value oft_hot.- if
max_hystis set, andt_cold>t_hot, sett_coldto the new value oft_hot.So, in other words, your new unified function would only need the following simple validation:
if (
t_cold>t_hot)
t_cold =t_hot;[ ... ]
Quoted 208 lines
Quoted 208 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +13
-0+ +static int __devinit omap_temp_sensor_probe(struct platform_device *pdev) +{ + struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; + struct omap_temp_sensor *temp_sensor; + struct resource *mem; + int ret = 0; + int val, clk_rate; + u32 max_freq, min_freq; + + if (!pdata) { + dev_err(&pdev->dev, "platform data missing\n"); + return -EINVAL;Quoted 8 lines
Review
Quoted 6 lines
+ }
(quoted diff) +5
-0+ + temp_sensor = kzalloc(sizeof(*temp_sensor), GFP_KERNEL); + if (!temp_sensor) + return -ENOMEM; +You have error messages all over the place, Why not here ?
Ok. I will add an error message here.
Quoted 13 lines
Review
Quoted 10 lines
+
mutex_init(&temp_sensor->sensor_mutex);(quoted diff) +8
-0+ + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource\n"); + ret = -ENOMEM; + goto plat_res_err; + } +If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Ok. Here if i fail i am releasing
temp_sensormemory which has been allocated.Quoted 27 lines
Quoted 27 lines
+
temp_sensor->irq=platform_get_irq_byname(pdev, "thermal_alert");
+ if (temp_sensor->irq< 0) {
+dev_err(&pdev->dev, "get_irq_byname failed\n");
+ ret =temp_sensor->irq;
+ gotoplat_res_err;
+ }(quoted diff) +20
-0+ + temp_sensor->phy_base = ioremap(mem->start, resource_size(mem)); + temp_sensor->clock = NULL; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + + if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + } else { + max_freq = MAX_FREQ; + min_freq = MIN_FREQ; + } + + pm_runtime_enable(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + + /* + * check if the efuse has a non-zero value if not + * it is an untrimmed sample and the temperaturesQuoted 8 lines
Quoted 6 lines
+ * may not be accurate
+ */
+
+ if (omap_temp_sensor_readl(temp_sensor,
+temp_sensor->registers->bgap_efuse)) {
+temp_sensor->is_efuse_valid= 1;
is_efuse_validis not used anywhere. Might as well drop it.Ok
Quoted 5 lines
Quoted 3 lines
+ dev_dbg(temp_sensor->hwmon_dev,
+ "Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n");
dev_infomight be better here.Ok.
Quoted 17 lines
Quoted 5 lines
+ }
(quoted diff) +3
-0+ + dev_set_drvdata(&pdev->dev, temp_sensor); + temp_sensor->clock = clk_get(temp_sensor->hwmon_dev, "fck");Review
Quoted 10 lines
(quoted diff) +10
-0+ if (IS_ERR(temp_sensor->clock)) { + ret = PTR_ERR(temp_sensor->clock); + dev_err(temp_sensor->hwmon_dev, + "unable to get fclk: %d\n", ret); + goto plat_res_err; + } + + ret = omap_temp_sensor_clk_enable(temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Cannot enable temp sensor\n");
omap_temp_sensor_clk_enable()already generates error messages.Ok. I will remove this.
Quoted 10 lines
Quoted 8 lines
+ goto
clken_err;
+ }
+
+clk_rate=clk_round_rate(temp_sensor->clock, max_freq);
+ if (clk_rate<min_freq||clk_rate==0xffffffff) {
+dev_err(&pdev->dev, "Error round rate\n");
+ ret =-EDOM;EDOM is wrong. Please use ENODEV.
Ok
Quoted 3 lines
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Ok
Quoted 10 lines
Quoted 10 lines
+ goto
clken_err;
+ }(quoted diff) +7
-0+ + ret = clk_set_rate(temp_sensor->clock, clk_rate); + if (ret) { + dev_err(&pdev->dev, "Cannot set clock rate\n"); + goto clken_err; + } +Quoted 8 lines
Quoted 5 lines
+
temp_sensor->clk_rate=clk_rate;
+omap_enable_continuous_mode(temp_sensor, 1);
+omap_configure_temp_sensor_thresholds(temp_sensor);
+ /* 1 ms */
+omap_configure_temp_sensor_counter(temp_sensor, 1);If
temp_sensor->clk_rate* 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would betemp_sensor->clk_rate/ 1000.Yes! Wrong comment. It is configured to start the temperature conversion. It represents 1 clock cycle of the temperature sensor fclk. I will correct this.
Quoted 16 lines
Quoted 16 lines
(quoted diff) +15
-0+ + /* Wait till the first conversion is done wait for at least 1ms */ + usleep_range(1000, 2000); + + /* Read the temperature once due to hw issue*/ + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + + /* Set 2 seconds time as default counter */ + omap_configure_temp_sensor_counter(temp_sensor, + temp_sensor->clk_rate * 2); + + temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev); + if (IS_ERR(temp_sensor->hwmon_dev)) { + dev_err(&pdev->dev, "hwmon_device_register failed.\n");Quoted 8 lines
Quoted 6 lines
+ ret =
PTR_ERR(temp_sensor->hwmon_dev);
+ gotohwmon_reg_err;
+ }
+
+ ret = sysfs_create_group(&pdev->dev.kobj,
+ &omap_temp_sensor_group);one line is sufficient here.
Ok.
Quoted 10 lines
Review
Quoted 8 lines
+ if (ret) {
+dev_err(&pdev->dev, "could not create sysfs files\n");
+ gotosysfs_create_err;
+ }(quoted diff) +3
-0+ + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_ADD); +No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Yes i will remove this.
Quoted 19 lines
Quoted 8 lines
+ ret = request_threaded_irq(temp_sensor->irq, NULL,
+omap_talert_irq_handler,IRQF_TRIGGER_RISING|IRQF_ONESHOT,
+ "temp_sensor",temp_sensor);
+ if (ret) {
+dev_err(&pdev->dev, "Request threaded irq failed.\n");
+ gotoreq_irq_err;
+ }Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* unmask the T_COLD and unmask T_HOT at init */ + val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +This is racy. From
Documentation/hwmon/submitting-patches:Quoted 8 lines
Quoted 6 lines
I read it. If i enable the interrupts before i have the sysfs infrastructure ready.
I am likely to miss out on the uevent notification if the interrupt occurs as soon as
i unmask. Correct me if my understanding is wrong.See other email on sequence subject. I'll have to look into it in more detail. later - no time right now ...
Concerns:
- create sysfs entries first, then register with hwmon.
hwmon_device_registeris usually the last call, made only after everything is known to work.
- You can definitely request the interrupt before registering sysfs entries
-temp_sensor->hwmon_devis initially set totemp_sensor->hwmon_dev=&pdev->dev;
then overwritten with
temp_sensor->hwmon_dev =hwmon_device_register(&pdev->dev); This is, at the very least, odd and doesn't seem to make sense.
- Interrupt enable code (settingbgap_mask_ctrl) in probe function occurs after sysfs files have been enabled, and the access is not mutex protected. Thus, sysfs access can already happen, causing the main race I was concerned about.My suggestion is
- request interrupt prior to sysfs creation and hwmon registration.
- swap sysfs file creation and hwmon registration.
- Use the unified function I suggested above to sett_cold,t_hot, and to enable interrupts as needed. Since that function will be mutex protected, you can call it after hwmon registration, and avoid the race.
Quoted 2 lines
Also look into use of
temp_sensor->hwmon_devvs.&pdev->dev. That code just doesn't look right.
Thanks for the inputs. I will work on them and get back.
Quoted 6 lines
Thanks,
Guenter
Signature · 6 lines
Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 66 lines · Guenter Roeck
Quoted 15 lines · Guenter Roeck
Quoted 14 lines · KEERTHY
Quoted 12 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>[ ... ]
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 4 lines
+ }
(quoted diff) +1
-0+Review
Quoted 10 lines
(quoted diff) +10
-0+ 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.Quoted 8 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.Problem here is that you expect the application to know, depending on the current values of (max,
max_hyst), how to update both limits in order. That is not a reasonable expectation.One possible solution would be to have a single function to set both
t_coldandt_hot, and call it from both initialization code and from the set functions. That would unify all the register setting and interrupt enable code.Regarding the actual values to set, you can (as an example) use the following approach:
- If max is set, and
t_hot<t_cold, adjustt_coldto the new value oft_hot.
Quoted 8 lines
- if
max_hystis set, andt_cold>t_hot, sett_coldto the new value oft_hot.So, in other words, your new unified function would only need the following simple validation:
if (
t_cold>t_hot)
t_cold =t_hot;
One concern here. There should be a hysteresis
difference between the two and can not be equal.
I need to add a hysteresis value to t_hot so as
to maintain t_hot > t_cold condition.
Quoted 252 lines
[ ... ]
Quoted 208 lines
Quoted 208 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +13
-0+ +static int __devinit omap_temp_sensor_probe(struct platform_device *pdev) +{ + struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; + struct omap_temp_sensor *temp_sensor; + struct resource *mem; + int ret = 0; + int val, clk_rate; + u32 max_freq, min_freq; + + if (!pdata) { + dev_err(&pdev->dev, "platform data missing\n"); + return -EINVAL;Quoted 8 lines
Review
Quoted 6 lines
+ }
(quoted diff) +5
-0+ + temp_sensor = kzalloc(sizeof(*temp_sensor), GFP_KERNEL); + if (!temp_sensor) + return -ENOMEM; +You have error messages all over the place, Why not here ?
Ok. I will add an error message here.
Quoted 13 lines
Review
Quoted 10 lines
+
mutex_init(&temp_sensor->sensor_mutex);(quoted diff) +8
-0+ + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource\n"); + ret = -ENOMEM; + goto plat_res_err; + } +If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Ok. Here if i fail i am releasing
temp_sensormemory which has been allocated.Quoted 27 lines
Quoted 27 lines
+
temp_sensor->irq=platform_get_irq_byname(pdev, "thermal_alert");
+ if (temp_sensor->irq< 0) {
+dev_err(&pdev->dev, "get_irq_byname failed\n");
+ ret =temp_sensor->irq;
+ gotoplat_res_err;
+ }(quoted diff) +20
-0+ + temp_sensor->phy_base = ioremap(mem->start, resource_size(mem)); + temp_sensor->clock = NULL; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + + if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + } else { + max_freq = MAX_FREQ; + min_freq = MIN_FREQ; + } + + pm_runtime_enable(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + + /* + * check if the efuse has a non-zero value if not + * it is an untrimmed sample and the temperaturesQuoted 8 lines
Quoted 6 lines
+ * may not be accurate
+ */
+
+ if (omap_temp_sensor_readl(temp_sensor,
+temp_sensor->registers->bgap_efuse)) {
+temp_sensor->is_efuse_valid= 1;
is_efuse_validis not used anywhere. Might as well drop it.Ok
Quoted 5 lines
Quoted 3 lines
+ dev_dbg(temp_sensor->hwmon_dev,
+ "Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n");
dev_infomight be better here.Ok.
Quoted 17 lines
Quoted 5 lines
+ }
(quoted diff) +3
-0+ + dev_set_drvdata(&pdev->dev, temp_sensor); + temp_sensor->clock = clk_get(temp_sensor->hwmon_dev, "fck");Review
Quoted 10 lines
(quoted diff) +10
-0+ if (IS_ERR(temp_sensor->clock)) { + ret = PTR_ERR(temp_sensor->clock); + dev_err(temp_sensor->hwmon_dev, + "unable to get fclk: %d\n", ret); + goto plat_res_err; + } + + ret = omap_temp_sensor_clk_enable(temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Cannot enable temp sensor\n");
omap_temp_sensor_clk_enable()already generates error messages.Ok. I will remove this.
Quoted 10 lines
Quoted 8 lines
+ goto
clken_err;
+ }
+
+clk_rate=clk_round_rate(temp_sensor->clock, max_freq);
+ if (clk_rate<min_freq||clk_rate==0xffffffff) {
+dev_err(&pdev->dev, "Error round rate\n");
+ ret =-EDOM;EDOM is wrong. Please use ENODEV.
Ok
Quoted 3 lines
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Ok
Quoted 10 lines
Quoted 10 lines
+ goto
clken_err;
+ }(quoted diff) +7
-0+ + ret = clk_set_rate(temp_sensor->clock, clk_rate); + if (ret) { + dev_err(&pdev->dev, "Cannot set clock rate\n"); + goto clken_err; + } +Quoted 8 lines
Quoted 5 lines
+
temp_sensor->clk_rate=clk_rate;
+omap_enable_continuous_mode(temp_sensor, 1);
+omap_configure_temp_sensor_thresholds(temp_sensor);
+ /* 1 ms */
+omap_configure_temp_sensor_counter(temp_sensor, 1);If
temp_sensor->clk_rate* 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would betemp_sensor->clk_rate/ 1000.Yes! Wrong comment. It is configured to start the temperature conversion. It represents 1 clock cycle of the temperature sensor fclk. I will correct this.
Quoted 16 lines
Quoted 16 lines
(quoted diff) +15
-0+ + /* Wait till the first conversion is done wait for at least 1ms */ + usleep_range(1000, 2000); + + /* Read the temperature once due to hw issue*/ + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + + /* Set 2 seconds time as default counter */ + omap_configure_temp_sensor_counter(temp_sensor, + temp_sensor->clk_rate * 2); + + temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev); + if (IS_ERR(temp_sensor->hwmon_dev)) { + dev_err(&pdev->dev, "hwmon_device_register failed.\n");Quoted 8 lines
Quoted 6 lines
+ ret =
PTR_ERR(temp_sensor->hwmon_dev);
+ gotohwmon_reg_err;
+ }
+
+ ret = sysfs_create_group(&pdev->dev.kobj,
+ &omap_temp_sensor_group);one line is sufficient here.
Ok.
Quoted 10 lines
Review
Quoted 8 lines
+ if (ret) {
+dev_err(&pdev->dev, "could not create sysfs files\n");
+ gotosysfs_create_err;
+ }(quoted diff) +3
-0+ + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_ADD); +No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Yes i will remove this.
Quoted 19 lines
Quoted 8 lines
+ ret = request_threaded_irq(temp_sensor->irq, NULL,
+omap_talert_irq_handler,IRQF_TRIGGER_RISING|IRQF_ONESHOT,
+ "temp_sensor",temp_sensor);
+ if (ret) {
+dev_err(&pdev->dev, "Request threaded irq failed.\n");
+ gotoreq_irq_err;
+ }Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* unmask the T_COLD and unmask T_HOT at init */ + val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +This is racy. From
Documentation/hwmon/submitting-patches:Quoted 8 lines
Quoted 6 lines
I read it. If i enable the interrupts before i have the sysfs infrastructure ready.
I am likely to miss out on the uevent notification if the interrupt occurs as soon as
i unmask. Correct me if my understanding is wrong.See other email on sequence subject. I'll have to look into it in more detail. later - no time right now ...
Concerns:
- create sysfs entries first, then register with hwmon.
hwmon_device_registeris usually the last call, made only after everything is known to work.
- You can definitely request the interrupt before registering sysfs entries
-temp_sensor->hwmon_devis initially set totemp_sensor->hwmon_dev=&pdev->dev;
then overwritten with
temp_sensor->hwmon_dev =hwmon_device_register(&pdev->dev); This is, at the very least, odd and doesn't seem to make sense.
- Interrupt enable code (settingbgap_mask_ctrl) in probe function occurs after sysfs files have been enabled, and the access is not mutex protected. Thus, sysfs access can already happen, causing the main race I was concerned about.My suggestion is
- request interrupt prior to sysfs creation and hwmon registration.
- swap sysfs file creation and hwmon registration.
- Use the unified function I suggested above to sett_cold,t_hot, and to enable interrupts as needed. Since that function will be mutex protected, you can call it after hwmon registration, and avoid the race.Also look into use of
temp_sensor->hwmon_devvs.&pdev->dev. That code just doesn't look right.Thanks,
Guenter
Signature · 6 lines
Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 74 lines · KEERTHY
Quoted 72 lines · Guenter Roeck
Quoted 15 lines · Guenter Roeck
Quoted 14 lines · KEERTHY
Quoted 12 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>[ ... ]
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 4 lines
(quoted diff) +2
-0+ } +Review
Quoted 10 lines
(quoted diff) +10
-0+ 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.Quoted 8 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.Problem here is that you expect the application to know, depending on the current values of (max,
max_hyst), how to update both limits in order. That is not a reasonable expectation.One possible solution would be to have a single function to set both
t_coldandt_hot, and call it from both initialization code and from the set functions. That would unify all the register setting and interrupt enable code.Regarding the actual values to set, you can (as an example) use the following approach:
- If max is set, and
t_hot<t_cold, adjustt_coldto the new value oft_hot.- if
max_hystis set, andt_cold>t_hot, sett_coldto the new value oft_hot.So, in other words, your new unified function would only need the following simple validation:
Quoted 8 lines
Quoted 2 lines
if (t_cold > t_hot) t_cold = t_hot;One concern here. There should be a hysteresis difference between the two and can not be equal. I need to add a hysteresis value to
t_hotso as to maintaint_hot>t_coldcondition.
Sure, makes sense. Was that in your original patch ? Just wondering.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 78 lines · Guenter Roeck
Quoted 77 lines · KEERTHY
Quoted 74 lines · Guenter Roeck
Quoted 15 lines · Guenter Roeck
Quoted 14 lines · KEERTHY
Quoted 12 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>[ ... ]
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 4 lines
+ }
(quoted diff) +1
-0+Review
Quoted 10 lines
(quoted diff) +10
-0+ 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.Quoted 8 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.Problem here is that you expect the application to know, depending on the current values of (max,
max_hyst), how to update both limits in order. That is not a reasonable expectation.One possible solution would be to have a single function to set both
t_coldandt_hot, and call it from both initialization code and from the set functions. That would unify all the register setting and interrupt enable code.Regarding the actual values to set, you can (as an example) use the following approach:
- If max is set, and
t_hot<t_cold, adjustt_coldto the new value oft_hot.- if
max_hystis set, andt_cold>t_hot, sett_coldto the new value oft_hot.So, in other words, your new unified function would only need the following simple validation:
if (
t_cold>t_hot)
t_cold =t_hot;
Quoted 6 lines
Quoted 5 lines
One concern here. There should be a hysteresis difference between the two and can not be equal. I need to add a hysteresis value to
t_hotso as to maintaint_hot>t_coldcondition.Sure, makes sense. Was that in your original patch ? Just wondering.
Original patch never let the user set thresholds which failed to satisfy the condition t_hot > t_cold. The original patch threw up error if this condition was violated.
Quoted 6 lines
Thanks,
Guenter
Signature · 6 lines
Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 48 lines · Guenter Roeck
Quoted 25 lines · KEERTHY
Quoted 23 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 9 lines
---
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_sensorcreate mode 100644drivers/hwmon/omap_temp_sensor.c[ ... ]
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
(quoted diff) +6
-0+/* 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);Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 4 lines
Quoted 2 lines
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
Quoted 77 lines
Quoted 77 lines
Quoted 14 lines
Quoted 14 lines
+ }
(quoted diff) +11
-0+ + 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);Quoted 8 lines
Quoted 6 lines
+ long val;
+ u32reg_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.
Quoted 8 lines
Quoted 4 lines
+ }
+
+t_hot=temp_to_adc_conversion(val);Bad error return check. Negative on error. Should be if (
t_hot< 0)
returnt_hot;Ok.
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 5 lines
Quoted 3 lines
+ count =
-EINVAL;
+ goto out;This will try to release the not-acquired lock.
I will correct this.
Quoted 17 lines
Review
Quoted 13 lines
+ }
(quoted diff) +11
-0+ + 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.
Quoted 3 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.
updating the t_cold without application's knowledge?
Quoted 4 lines
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.
Quoted 71 lines
Quoted 71 lines
Quoted 36 lines
Quoted 36 lines
+ }
(quoted diff) +33
-0+ + /* 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) {Quoted 8 lines
Quoted 6 lines
+
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_OFFSEThere butbgap_mask_ctrlelsewhere ?Ok. I will change this.
Quoted 23 lines
Quoted 9 lines
+ }
+ /*
+ * else no need to do anything since HW will immediately compare
+ * the new threshold and generate interrupt accordingly
+ */(quoted diff) +3
-0+out: + mutex_unlock(&temp_sensor->sensor_mutex); +Review
Quoted 10 lines
(quoted diff) +10
-0+ 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_maskcan 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 inshow_temp_max()is almost the same and does not use the lock.
Quoted 6 lines
Quoted 4 lines
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_maskin 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.
Quoted 6 lines
If it is changed outside the driver, you have a real problem, since there is no guarantee that the mask is not changed from outside the driver, by code not protected by the mutex. And if read functions have the side effect of changing such values, there should really be a note somewhere to make people aware of it. I'd call that side effect programming, and would argue it is a really bad idea.
It is not changed anywhere outside the driver.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
+ temp = omap_temp_sensor_readl(temp_sensor,
+temp_sensor->registers->bgap_threshold);
+ temp = (temp &temp_sensor->registers->threshold_tcold_mask) >>
+__ffs(temp_sensor->registers->threshold_tcold_mask);
+mutex_unlock(&temp_sensor->sensor_mutex);
+
+ if (temp <OMAP_ADC_START_VALUE|| temp >OMAP_ADC_END_VALUE) {
+dev_err(dev, "invalid value\n");
+ return-EDOM;
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
"Math argument out of domain of func" does not make sense.
Unless you don't know anything better, use EIO (I/O Error). Not very specific, but at least not misleading.Out of range i assumed this was the closest error value. I will use EIO.
It is not a math error, though.
Ok
Quoted 146 lines
Quoted 146 lines
Quoted 4 lines
Besides, the log message is unnecessary. The user is informed with
-EIO, and the admin won't be able to do anything useful with "invalid value".Ok. I will return
-EIOand eliminate the log message.Quoted 13 lines
Quoted 13 lines
+ }
(quoted diff) +11
-0+ + temp = adc_to_temp_conversion(temp); + + return snprintf(buf, 16, "%d\n", temp); +} + +static ssize_t set_temp_max_hyst(struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev);Quoted 8 lines
Quoted 6 lines
+ u32
reg_val,t_hot,t_cold, temp;
+ long val;
+
+ if (strict_strtol(buf, 10, &val)) {
+ count =-EINVAL;
+ goto out;This will try to release the not acquired lock.
I will correct this.
Quoted 6 lines
Quoted 4 lines
+ }
+
+t_cold=temp_to_adc_conversion(val);Bad error return check; same as above.
Ok. I will correct this.
Quoted 59 lines
Quoted 5 lines
+ if (
t_cold<OMAP_ADC_START_VALUE||t_cold>OMAP_ADC_END_VALUE) {
+ count =-EINVAL;
+ goto out;
+ }Review
Quoted 9 lines
(quoted diff) +9
-0+ + mutex_lock(&temp_sensor->sensor_mutex); + /* obtain the T HOT value */ + t_hot = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + t_hot = (t_hot & temp_sensor->registers->threshold_thot_mask) >> + __ffs(temp_sensor->registers->threshold_thot_mask); + + if (t_cold > t_hot) {Same concern as above. Change order is left to application which should not be bothered.
Quoted 42 lines
+ count =
-EINVAL;
+ goto out;
+ }(quoted diff) +39
-0+ + /* write the new t_cold value */ + reg_val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_threshold); + reg_val &= ~temp_sensor->registers->threshold_tcold_mask; + reg_val |= t_cold << + __ffs(temp_sensor->registers->threshold_tcold_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 LOW threshold(t_cold) lower than the current + * temperature(temp) unmask the COLD interrupts + */ + if (t_cold < temp) { + reg_val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + reg_val &= ~temp_sensor->registers->mask_hot_mask; + reg_val |= temp_sensor->registers->mask_cold_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_hot && temp > t_cold) { + 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);Quoted 8 lines
Quoted 6 lines
+ }
+
+ /*
+ * else no need to do anything since HW will immediately compare
+ * the new threshold and generate interrupt accordingly
+ */Would be nice if you could unify the interrupt enable code in a single function.
Ok. I will have a function for this.
Quoted 16 lines
Quoted 4 lines
(quoted diff) +3
-0+ +out: + mutex_unlock(&temp_sensor->sensor_mutex);Review
Quoted 10 lines
(quoted diff) +10
-0+ + return count; +} + +static ssize_t show_update_rate(struct device *dev, + struct device_attribute *devattr, char *buf) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + u32 temp = 0, ret = 0; +No need to initialize temp.
ret is u32, which doesn't seem to be a good type to use for negative values.Oops yes. I will correct this.
Quoted 8 lines
Quoted 5 lines
+
+ if (!temp_sensor->clk_rate) {
+dev_err(dev, "clk_rate is NULL\n");
+ ret =-EDOM;Same comment as above (this is not a math error). And you don't have to check
clk_rateanyway, since it is set in probe, and probe bails out if it is not set.Ok
Quoted 6 lines
Quoted 5 lines
+ goto out;
+ }
+
+mutex_lock(&temp_sensor->sensor_mutex);
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
As with other show locks, this one would only make sense if
counter_maskcan change during runtime, and if that change would be protected by the mutex. This is not the case, thus the lock is unnecessary.Counter value can be altered runtime. A lock is needed in both the cases. I will add it.
clk_ratedoes not change,counter_maskdoes not change, and reading a single value doesn't need a mutex. Am I missing something ?
Ok. I will remove the lock from show function.
Quoted 449 lines
Quoted 449 lines
Quoted 32 lines
Quoted 32 lines
+ temp = omap_temp_sensor_readl(temp_sensor,
+temp_sensor->registers->bgap_counter);
+ temp = (temp &temp_sensor->registers->counter_mask) >>
+__ffs(temp_sensor->registers->counter_mask);
+mutex_unlock(&temp_sensor->sensor_mutex);
+ temp = temp * 1000 /temp_sensor->clk_rate;(quoted diff) +24
-0+ +out: + if (!ret) + return sprintf(buf, "%d\n", temp); + + return ret; +} + +static ssize_t set_update_rate(struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + u32 reg_val; + long val; + + if (strict_strtol(buf, 10, &val)) { + count = -EINVAL; + goto out; + } + + val *= temp_sensor->clk_rate / 1000; + mutex_lock(&temp_sensor->sensor_mutex); + reg_val = omap_temp_sensor_readl(temp_sensor,Quoted 8 lines
Quoted 4 lines
+
temp_sensor->registers->bgap_counter); +
+reg_val&= ~temp_sensor->registers->counter_mask;
+reg_val|= val;Sure you want to accept any value (including negative ones) here ? val is not shifted, and not masked against
counter_mask, meaning it can write all bits of the register. That doesn't sound right.I will have a check.
Quoted 3 lines
It would make sense to call
omap_configure_temp_sensor_counter()instead of duplicating the set functionality.Yes I will make use of that.
Quoted 12 lines
Quoted 12 lines
+ omap_temp_sensor_writel(temp_sensor,
reg_val,
+temp_sensor->registers->bgap_counter);
+mutex_unlock(&temp_sensor->sensor_mutex);(quoted diff) +8
-0+ +out: + return count; +} + +static int omap_temp_sensor_read_temp(struct device *dev, + struct device_attribute *devattr, + char *buf)Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+{ + struct omap_temp_sensor *temp_sensor = dev_get_drvdata(dev); + int temp, ret = 0; + + mutex_lock(&temp_sensor->sensor_mutex);Unless
bgap_dtemp_maskcan change during runtime, and that change is protected by the mutex, this lock is unnecessary.It is read only and is updated by hardware. Change cannot be protected by mutex.
Quoted 11 lines
Quoted 9 lines
+ temp = omap_temp_sensor_readl(temp_sensor,
+temp_sensor->registers->temp_sensor_ctrl);
+ temp &=temp_sensor->registers->bgap_dtemp_mask;
+
+ /* look up for temperature in the table and return the temperature */
+ if (temp <OMAP_ADC_START_VALUE|| temp >OMAP_ADC_END_VALUE) {
+dev_err(dev, "invalid adc code reported %d", temp);
+ ret =-EDOM;Another wrong use of EDOM, and another value-free error message.
Ok. I will use
-EIOand drop the error message.Quoted 17 lines
Quoted 17 lines
+ goto out;
+ }(quoted diff) +14
-0+ + temp = adc_to_temp[temp - OMAP_ADC_START_VALUE]; + +out: + mutex_unlock(&temp_sensor->sensor_mutex); + if (!ret) + return sprintf(buf, "%d\n", temp); + + return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, omap_temp_sensor_read_temp, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max,Quoted 8 lines
Review
Quoted 6 lines
+
set_temp_max, 0);(quoted diff) +5
-0+static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, show_temp_max_hyst, + set_temp_max_hyst, 0); +static SENSOR_DEVICE_ATTR(update_rate, S_IWUSR | S_IRUGO, show_update_rate, + set_update_rate, 0); +ABI is
update_interval. Please use it.Ok.
Quoted 26 lines
Quoted 26 lines
(quoted diff) +25
-0+static struct attribute *omap_temp_sensor_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_temp1_max.dev_attr.attr, + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr, + &sensor_dev_attr_update_rate.dev_attr.attr, + NULL +}; + +static const struct attribute_group omap_temp_sensor_group = { + .attrs = omap_temp_sensor_attributes, +}; + +static int omap_temp_sensor_clk_enable(struct omap_temp_sensor *temp_sensor) +{ + u32 ret = 0; + + if (temp_sensor->clk_on) { + dev_err(temp_sensor->hwmon_dev, "clock already on\n"); + goto out; + } + + ret = pm_runtime_get_sync(temp_sensor->hwmon_dev); + if (ret < 0) { + dev_err(temp_sensor->hwmon_dev, "get sync failed\n"); + goto out;Quoted 8 lines
Review
Quoted 6 lines
+ }
(quoted diff) +5
-0+ + mutex_lock(&temp_sensor->sensor_mutex); + temp_sensor->clk_on = 1; + mutex_unlock(&temp_sensor->sensor_mutex); +Completely unnecessary lock.
Ok
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+out: + return ret; +} + +static int omap_temp_sensor_clk_disable(struct omap_temp_sensor *temp_sensor)Return code is not used anywhere, why bother providing one ?
It is not checked in the error path. I can make it void.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+{ + u32 ret = 0; + + if (!temp_sensor->clk_on) { + dev_err(temp_sensor->hwmon_dev, "clock already off\n"); + goto out;... and since you ignore the error anyway, why log it ?
Ok. I will remove the log message.
Quoted 16 lines
Quoted 4 lines
+ }
(quoted diff) +2
-0+ + /* Gate the clock */Review
Quoted 10 lines
(quoted diff) +10
-0+ ret = pm_runtime_put_sync(temp_sensor->hwmon_dev); + if (ret < 0) { + dev_err(temp_sensor->hwmon_dev, "put sync failed\n"); + goto out; + } + + mutex_lock(&temp_sensor->sensor_mutex); + temp_sensor->clk_on = 0; + mutex_unlock(&temp_sensor->sensor_mutex); +Completely unnecessary lock.
Ok
Quoted 61 lines
Quoted 61 lines
(quoted diff) +60
-0+out: + return ret; +} + +static irqreturn_t omap_talert_irq_handler(int irq, void *data) +{ + struct omap_temp_sensor *temp_sensor; + int t_hot, t_cold, temp; + + temp_sensor = data; + mutex_lock(&temp_sensor->sensor_mutex); + + /* Read the status of t_hot */ + t_hot = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_hot_mask; + + /* Read the status of t_cold */ + t_cold = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_status) + & temp_sensor->registers->status_cold_mask; + + temp = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + /* + * One TALERT interrupt: Two sources + * If the interrupt is due to t_hot then mask t_hot and + * and unmask t_cold else mask t_cold and unmask t_hot + */ + if (t_hot) { + temp &= ~temp_sensor->registers->mask_hot_mask; + temp |= temp_sensor->registers->mask_cold_mask; + } else if (t_cold) { + temp &= ~temp_sensor->registers->mask_cold_mask; + temp |= temp_sensor->registers->mask_hot_mask; + } + + omap_temp_sensor_writel(temp_sensor, temp, + temp_sensor->registers->bgap_mask_ctrl); + + /* kobject_uvent to user space telling thermal threshold crossed */ + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_CHANGE); + + mutex_unlock(&temp_sensor->sensor_mutex); + + return IRQ_HANDLED; +} + +static int __devinit omap_temp_sensor_probe(struct platform_device *pdev) +{ + struct omap_temp_sensor_pdata *pdata = pdev->dev.platform_data; + struct omap_temp_sensor *temp_sensor; + struct resource *mem; + int ret = 0; + int val, clk_rate; + u32 max_freq, min_freq; + + if (!pdata) { + dev_err(&pdev->dev, "platform data missing\n"); + return -EINVAL;Quoted 8 lines
Review
Quoted 6 lines
+ }
(quoted diff) +5
-0+ + temp_sensor = kzalloc(sizeof(*temp_sensor), GFP_KERNEL); + if (!temp_sensor) + return -ENOMEM; +You have error messages all over the place, Why not here ?
Ok. I will add an error message here.
Quoted 13 lines
Review
Quoted 10 lines
+
mutex_init(&temp_sensor->sensor_mutex);(quoted diff) +8
-0+ + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource\n"); + ret = -ENOMEM; + goto plat_res_err; + } +If you try to get the resource before allocating the memory, you don't have to release the memory if the platform resource is missing.
Ok. Here if i fail i am releasing
temp_sensormemory which has been allocated.Quoted 27 lines
Quoted 27 lines
+
temp_sensor->irq=platform_get_irq_byname(pdev, "thermal_alert");
+ if (temp_sensor->irq< 0) {
+dev_err(&pdev->dev, "get_irq_byname failed\n");
+ ret =temp_sensor->irq;
+ gotoplat_res_err;
+ }(quoted diff) +20
-0+ + temp_sensor->phy_base = ioremap(mem->start, resource_size(mem)); + temp_sensor->clock = NULL; + temp_sensor->registers = pdata->registers; + temp_sensor->hwmon_dev = &pdev->dev; + + if (pdata->max_freq && pdata->min_freq) { + max_freq = pdata->max_freq; + min_freq = pdata->min_freq; + } else { + max_freq = MAX_FREQ; + min_freq = MIN_FREQ; + } + + pm_runtime_enable(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + + /* + * check if the efuse has a non-zero value if not + * it is an untrimmed sample and the temperaturesQuoted 8 lines
Quoted 6 lines
+ * may not be accurate
+ */
+
+ if (omap_temp_sensor_readl(temp_sensor,
+temp_sensor->registers->bgap_efuse)) {
+temp_sensor->is_efuse_valid= 1;
is_efuse_validis not used anywhere. Might as well drop it.Ok
Quoted 5 lines
Quoted 3 lines
+ dev_dbg(temp_sensor->hwmon_dev,
+ "Invalid EFUSE, Non-trimmed BGAP, Temp not accurate\n");
dev_infomight be better here.Ok.
Quoted 17 lines
Quoted 5 lines
+ }
(quoted diff) +3
-0+ + dev_set_drvdata(&pdev->dev, temp_sensor); + temp_sensor->clock = clk_get(temp_sensor->hwmon_dev, "fck");Review
Quoted 10 lines
(quoted diff) +10
-0+ if (IS_ERR(temp_sensor->clock)) { + ret = PTR_ERR(temp_sensor->clock); + dev_err(temp_sensor->hwmon_dev, + "unable to get fclk: %d\n", ret); + goto plat_res_err; + } + + ret = omap_temp_sensor_clk_enable(temp_sensor); + if (ret) { + dev_err(&pdev->dev, "Cannot enable temp sensor\n");
omap_temp_sensor_clk_enable()already generates error messages.Ok. I will remove this.
Quoted 10 lines
Quoted 8 lines
+ goto
clken_err;
+ }
+
+clk_rate=clk_round_rate(temp_sensor->clock, max_freq);
+ if (clk_rate<min_freq||clk_rate==0xffffffff) {
+dev_err(&pdev->dev, "Error round rate\n");
+ ret =-EDOM;EDOM is wrong. Please use ENODEV.
Ok
Quoted 3 lines
The error log doesn't really provide anything useful to the administrator. Either add parameters, or drop it.
Ok
Quoted 10 lines
Quoted 10 lines
+ goto
clken_err;
+ }(quoted diff) +7
-0+ + ret = clk_set_rate(temp_sensor->clock, clk_rate); + if (ret) { + dev_err(&pdev->dev, "Cannot set clock rate\n"); + goto clken_err; + } +Quoted 8 lines
Quoted 5 lines
+
temp_sensor->clk_rate=clk_rate;
+omap_enable_continuous_mode(temp_sensor, 1);
+omap_configure_temp_sensor_thresholds(temp_sensor);
+ /* 1 ms */
+omap_configure_temp_sensor_counter(temp_sensor, 1);If
temp_sensor->clk_rate* 2 is two seconds, as suggested below, 1 is not 1ms. 1ms would betemp_sensor->clk_rate/ 1000.Yes! Wrong comment. It is configured to start the temperature conversion. It represents 1 clock cycle of the temperature sensor fclk. I will correct this.
Quoted 16 lines
Quoted 16 lines
(quoted diff) +15
-0+ + /* Wait till the first conversion is done wait for at least 1ms */ + usleep_range(1000, 2000); + + /* Read the temperature once due to hw issue*/ + omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->temp_sensor_ctrl); + + /* Set 2 seconds time as default counter */ + omap_configure_temp_sensor_counter(temp_sensor, + temp_sensor->clk_rate * 2); + + temp_sensor->hwmon_dev = hwmon_device_register(&pdev->dev); + if (IS_ERR(temp_sensor->hwmon_dev)) { + dev_err(&pdev->dev, "hwmon_device_register failed.\n");Quoted 8 lines
Quoted 6 lines
+ ret =
PTR_ERR(temp_sensor->hwmon_dev);
+ gotohwmon_reg_err;
+ }
+
+ ret = sysfs_create_group(&pdev->dev.kobj,
+ &omap_temp_sensor_group);one line is sufficient here.
Ok.
Quoted 10 lines
Review
Quoted 8 lines
+ if (ret) {
+dev_err(&pdev->dev, "could not create sysfs files\n");
+ gotosysfs_create_err;
+ }(quoted diff) +3
-0+ + kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_ADD); +No other hwmon driver needs this. Doesn't the sysfs code take care of this ?
Yes i will remove this.
Quoted 19 lines
Quoted 8 lines
+ ret = request_threaded_irq(temp_sensor->irq, NULL,
+omap_talert_irq_handler,IRQF_TRIGGER_RISING|IRQF_ONESHOT,
+ "temp_sensor",temp_sensor);
+ if (ret) {
+dev_err(&pdev->dev, "Request threaded irq failed.\n");
+ gotoreq_irq_err;
+ }Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* unmask the T_COLD and unmask T_HOT at init */ + val = omap_temp_sensor_readl(temp_sensor, + temp_sensor->registers->bgap_mask_ctrl); + val |= temp_sensor->registers->mask_cold_mask + | temp_sensor->registers->mask_hot_mask; + + omap_temp_sensor_writel(temp_sensor, val, + temp_sensor->registers->bgap_mask_ctrl); +This is racy. From
Documentation/hwmon/submitting-patches:
Quoted 8 lines
Quoted 6 lines
I read it. If i enable the interrupts before i have the sysfs infrastructure ready.
I am likely to miss out on the uevent notification if the interrupt occurs as soon as
i unmask. Correct me if my understanding is wrong.See other email on sequence subject. I'll have to look into it in more detail. later - no time right now ...
Ok
Quoted 4 lines
Guenter
Signature · 6 lines
Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 134 lines · KEERTHY
Quoted 48 lines · Guenter Roeck
Quoted 25 lines · KEERTHY
Quoted 23 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 9 lines
--- 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[ ... ]
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
(quoted diff) +6
-0+/* 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);Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 4 lines
Quoted 2 lines
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
Quoted 77 lines
Quoted 77 lines
Quoted 14 lines
Quoted 14 lines
(quoted diff) +12
-0+ } + + 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ } + + t_hot = temp_to_adc_conversion(val);Bad error return check. Negative on error. Should be
if (t_hot < 0) return t_hot;Ok.
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ count = -EINVAL; + goto out;This will try to release the not-acquired lock.
I will correct this.
Quoted 17 lines
Review
Quoted 13 lines
(quoted diff) +12
-0+ } + + 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.updating the
t_coldwithout 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.
Quoted 10 lines
Quoted 4 lines
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.
Quoted 75 lines
Quoted 75 lines
Quoted 75 lines
Quoted 36 lines
Quoted 36 lines
(quoted diff) +34
-0+ } + + /* 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) {Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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_OFFSEThere butbgap_mask_ctrlelsewhere ?Ok. I will change this.
Quoted 15 lines
Quoted 15 lines
(quoted diff) +14
-0+ } + /* + * 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) +{Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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_maskcan 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 inshow_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.
Quoted 8 lines
Quoted 2 lines
Reeally ? I seem to be blind. I searched for
threshold_tcold_maskin the patch but did not see where it is changed. Can you point me to the code ?I misread it. Yes
threshold_tcold_maskis used only inshow_temp_max_hyst(),set_temp_max(),set_temp_max_hyst()functions. It is set only in theset_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_counter
bgap_mask_ctrl
but not to any of the masks.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 245 lines · Guenter Roeck
Quoted 134 lines · KEERTHY
Quoted 48 lines · Guenter Roeck
Quoted 25 lines · KEERTHY
Quoted 23 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 9 lines
---
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_sensorcreate mode 100644drivers/hwmon/omap_temp_sensor.c[ ... ]
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
(quoted diff) +6
-0+/* 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);Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 4 lines
Quoted 2 lines
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
Quoted 77 lines
Quoted 77 lines
Quoted 14 lines
Quoted 14 lines
+ }
(quoted diff) +11
-0+ + 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);Quoted 8 lines
Quoted 6 lines
+ long val;
+ u32reg_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.
Quoted 8 lines
Quoted 4 lines
+ }
+
+t_hot=temp_to_adc_conversion(val);Bad error return check. Negative on error. Should be if (
t_hot< 0)
returnt_hot;Ok.
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 5 lines
Quoted 3 lines
+ count =
-EINVAL;
+ goto out;This will try to release the not-acquired lock.
I will correct this.
Quoted 17 lines
Review
Quoted 13 lines
+ }
(quoted diff) +11
-0+ + 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.updating the
t_coldwithout 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.
Quoted 10 lines
Quoted 4 lines
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
_cannot_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_yourdriver_prevents critical situations from happening. You can not blame applications for not knowing the oddities of your chips.Quoted 75 lines
Quoted 75 lines
Quoted 75 lines
Quoted 36 lines
Quoted 36 lines
+ }
(quoted diff) +33
-0+ + /* 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) {Quoted 8 lines
Quoted 6 lines
+
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_OFFSEThere butbgap_mask_ctrlelsewhere ?Ok. I will change this.
Quoted 15 lines
Quoted 15 lines
+ }
+ /*
+ * else no need to do anything since HW will immediately compare
+ * the new threshold and generate interrupt accordingly
+ */(quoted diff) +9
-0+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) +{Quoted 8 lines
Quoted 4 lines
+ 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_maskcan 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 inshow_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.Quoted 8 lines
Quoted 2 lines
Reeally ? I seem to be blind. I searched for
threshold_tcold_maskin the patch but did not see where it is changed. Can you point me to the code ?I misread it. Yes
threshold_tcold_maskis used only inshow_temp_max_hyst(),set_temp_max(),set_temp_max_hyst()functions. It is set only in theset_temp_max_hyst()
function.
Quoted 4 lines
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.
Quoted 9 lines
bgap_counter
bgap_mask_ctrl
but not to any of the masks.Thanks,
Guenter
Signature · 6 lines
Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 252 lines · KEERTHY
Quoted 245 lines · Guenter Roeck
Quoted 134 lines · KEERTHY
Quoted 48 lines · Guenter Roeck
Quoted 25 lines · KEERTHY
Quoted 23 lines · Guenter Roeck
Quoted 10 lines · Keerthy
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_ueventinterface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface.Signed-off-by Keerthy <[email protected]>Cc Jean Delvare <[email protected]>Cc Guenter Roeck <[email protected]>Partial review only. Too many concerns, and after a while I tend to loose track.
Quoted 9 lines
--- 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[ ... ]
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
(quoted diff) +6
-0+/* 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);Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 4 lines
Quoted 2 lines
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
Quoted 77 lines
Quoted 77 lines
Quoted 14 lines
Quoted 14 lines
(quoted diff) +12
-0+ } + + 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ } + + t_hot = temp_to_adc_conversion(val);Bad error return check. Negative on error. Should be
if (t_hot < 0) return t_hot;Ok.
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ count = -EINVAL; + goto out;This will try to release the not-acquired lock.
I will correct this.
Quoted 17 lines
Review
Quoted 13 lines
(quoted diff) +12
-0+ } + + 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_hysteresiswhich 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_hotno interrupts are fired. Once the temperature increases abovet_hotwe get an
interrupt. In order to avoid repeated interrupts we mask thet_hotinterrupt in the interrupt handler and enable thet_coldinterrupts. So we get at_coldinterrupt only when the temperature drops below thet_coldvalue. Hence setting thet_coldhigher thant_hotwill mess up the state machine.Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
t_hotvalue should be greater thant_coldvalue.One option would be to update
t_cold(max_hyst) automatically in that case.updating the
t_coldwithout 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.
Quoted 10 lines
Quoted 4 lines
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
_cannot_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_yourdriver_prevents critical situations from happening. You can not blame applications for not knowing the oddities of your chips.Quoted 75 lines
Quoted 75 lines
Quoted 75 lines
Quoted 36 lines
Quoted 36 lines
(quoted diff) +34
-0+ } + + /* 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) {Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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_OFFSEThere butbgap_mask_ctrlelsewhere ?Ok. I will change this.
Quoted 15 lines
Quoted 15 lines
(quoted diff) +14
-0+ } + /* + * 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) +{Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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_maskcan 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 inshow_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.Quoted 8 lines
Quoted 2 lines
Reeally ? I seem to be blind. I searched for
threshold_tcold_maskin the patch but did not see where it is changed. Can you point me to the code ?I misread it. Yes
threshold_tcold_maskis used only inshow_temp_max_hyst(),set_temp_max(),set_temp_max_hyst()functions. It is set only in theset_temp_max_hyst()
function.Quoted 4 lines
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
Quoted 6 lines
bgap_thresholdis the register and "threshold_tcold_mask" is a bit field in that register which is written in theset_temp_max_hyst()function to configuretemp_max_hyst.
Similarly "threshold_thot_mask" is a bit field in thebgap_thresholdregister which is written in theset_temp_max()function to configuretemp_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
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors