Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On Tue, Aug 23, 2011 at 9:48 AM, Rajendra Nayak rna...@ti.com wrote: On 8/23/2011 5:28 AM, Kevin Hilman wrote: Rajendra Nayakrna...@ti.com writes: [...] FWIK, its a one time requirement to set the clock rate to the right rate the device can operate in based on what a platform supports. Except $SUBJECT patch hard-codes the clock rate for all platforms in the driver. The device has a requirement to operate in a 1Mhz to 2Mhz range. So the driver is using a clk_round_rate() to get the closest rate supported and sets it using a clk_set_rate(). If the clock rate is to be platform-specific, it should be done in platform-specific code. I am fine if this needs to be moved to platform-specific code, but I wasn't quite sure this needs to be done in clock framework as was suggested. The range can be (ex: 1MHz to 2Mhz) can be sent via pdata as max and min frequencies and the driver can round the rate to get the appropriate rate inside the specified range? Is this approach Fine? Kevin -- Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Rajendra Nayak rna...@ti.com writes: On 8/23/2011 5:28 AM, Kevin Hilman wrote: Rajendra Nayakrna...@ti.com writes: [...] FWIK, its a one time requirement to set the clock rate to the right rate the device can operate in based on what a platform supports. Except $SUBJECT patch hard-codes the clock rate for all platforms in the driver. The device has a requirement to operate in a 1Mhz to 2Mhz range. You mean the device on OMAP4 has this requirement. Will this requirement exist on every platform that uses any version of this IP (or future similar IPs)? I suspect not. So the driver is using a clk_round_rate() to get the closest rate supported and sets it using a clk_set_rate(). If the clock rate is to be platform-specific, it should be done in platform-specific code. I am fine if this needs to be moved to platform-specific code, but I wasn't quite sure this needs to be done in clock framework as was suggested. No, it should't be in clock framework. But since the frequency range is platform-specific, it should be initialized in platform-specific code. Having min/max parameters passed by platform data as suggested by J. Keerthy is fine. Probably even better is to have the driver have a default min/max rang which can be overridden by platform_data only if needed. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On 8/23/2011 10:45 PM, Kevin Hilman wrote: Rajendra Nayakrna...@ti.com writes: On 8/23/2011 5:28 AM, Kevin Hilman wrote: Rajendra Nayakrna...@ti.com writes: [...] FWIK, its a one time requirement to set the clock rate to the right rate the device can operate in based on what a platform supports. Except $SUBJECT patch hard-codes the clock rate for all platforms in the driver. The device has a requirement to operate in a 1Mhz to 2Mhz range. You mean the device on OMAP4 has this requirement. Will this requirement exist on every platform that uses any version of this IP (or future similar IPs)? I suspect not. I agree, the current limitation is specific to the IP rev used, and might change for a never version of the IP. So the driver is using a clk_round_rate() to get the closest rate supported and sets it using a clk_set_rate(). If the clock rate is to be platform-specific, it should be done in platform-specific code. I am fine if this needs to be moved to platform-specific code, but I wasn't quite sure this needs to be done in clock framework as was suggested. No, it should't be in clock framework. But since the frequency range is platform-specific, it should be initialized in platform-specific code. Having min/max parameters passed by platform data as suggested by J. Keerthy is fine. Probably even better is to have the driver have a default min/max rang which can be overridden by platform_data only if needed. This sounds better since if the same IP rev is used on other platforms which does not change the min/max, the driver can do with the default and that would avoid duplications of same min/max having to be passed for multiple platforms. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Rajendra Nayak rna...@ti.com writes: [...] FWIK, its a one time requirement to set the clock rate to the right rate the device can operate in based on what a platform supports. Except $SUBJECT patch hard-codes the clock rate for all platforms in the driver. If the clock rate is to be platform-specific, it should be done in platform-specific code. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On 8/23/2011 5:28 AM, Kevin Hilman wrote: Rajendra Nayakrna...@ti.com writes: [...] FWIK, its a one time requirement to set the clock rate to the right rate the device can operate in based on what a platform supports. Except $SUBJECT patch hard-codes the clock rate for all platforms in the driver. The device has a requirement to operate in a 1Mhz to 2Mhz range. So the driver is using a clk_round_rate() to get the closest rate supported and sets it using a clk_set_rate(). If the clock rate is to be platform-specific, it should be done in platform-specific code. I am fine if this needs to be moved to platform-specific code, but I wasn't quite sure this needs to be done in clock framework as was suggested. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Hi,
On Fri, Aug 12, 2011 at 08:56:29AM +0530, Rajendra Nayak wrote:
On 8/12/2011 3:07 AM, Roger Quadros wrote:
On 08/11/2011 01:55 PM, Felipe Balbi wrote:
Hi,
On Thu, Aug 11, 2011 at 09:54:09PM +0300, Felipe Balbi wrote:
you need some other way to handle this. Why do you need to manually set
the rate rather than having hwmod handle this for you ?
your argument that it's a one time setting is not enough to have this
in the driver. Drivers should not care about clocks anymore, this should
have been done on another layer.
Hwmod will have no idea on the rate required.
does the rate need to change ? Also, I have not mentioned hwmod anytime
i did mention hwmod, nevermind that part. Still I'm not sure where is
the right place to handle this.
Aren't the omap_device_pm_latency callbacks the right place to do it?
e.g. in the following snippet from mach-omap2/temp_sensor_device.c
+static struct omap_device_pm_latency omap_temp_sensor_latency[] = {
+{
+ .deactivate_func = omap_device_idle_hwmods,
+ .activate_func = omap_device_enable_hwmods,
+ .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+}
+};
instead of directly pointing activate_func to omap_device_enable_hwmods,
it could point to a function that sets the required clock rate and then
enables the hwmod.
FWIK, its a one time requirement to set the clock rate to the
right rate the device can operate in based on what a platform
supports. What you are suggesting would add the overhead of doing
this every time the device is runtime enabled/idled.
if it's a one time setting, why don't you just change the clock fwk to
handle this nicely ? Maybe provide a different -enable() function which
would already set the correct rate ?
Russell, what would be the best way here ? driver needs clock to be at a
particular rate for the device to work, but it's a one time setting and
I don't think driver should be doing clk_get() - clk_enable() -
clk_set_rate(), where should that functionality be put ?
--
balbi
signature.asc
Description: Digital signature
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On Wed, Aug 10, 2011 at 6:16 PM, Felipe Balbi ba...@ti.com wrote: Hi, (why aren't below in Cc ? I will add them. HARDWARE MONITORING M: Jean Delvare kh...@linux-fr.org M: Guenter Roeck guenter.ro...@ericsson.com L: lm-sens...@lm-sensors.org W: http://www.lm-sensors.org/ T: quilt kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git S: Maintained F: Documentation/hwmon/ F: drivers/hwmon/ F: include/linux/hwmon*.h) On Wed, Aug 10, 2011 at 05:55:22PM +0530, Keerthy wrote: On chip temperature sensor driver. The driver monitors the temperature of the MPU subsystem of the OMAP4. It sends notifications to the user space if the temperature crosses user defined thresholds via kobject_uevent interface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface. Signed-off-by: Keerthy j-keer...@ti.com --- drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 + drivers/hwmon/omap_temp_sensor.c | 950 ++ 3 files changed, 962 insertions(+), 0 deletions(-) create mode 100644 drivers/hwmon/omap_temp_sensor.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5f888f7..9c9cd8b 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -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 help diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 28061cf..d0f89f5 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -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.o diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? Clock rate setting functions. +#include linux/io.h +#include linux/debugfs.h why ?? It will be removed. +#include linux/delay.h +#include linux/slab.h +#include linux/platform_device.h +#include linux/init.h +#include plat/omap_device.h why ?? Context loss count +#include linux/module.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 plat/common.h why ? usleep_range function. +#include plat/temperature_sensor.h It is the header file with the structure definitions used in the driver. why ? +#include mach/ctrl_module_core_44xx.h why ? It will be removed +#include mach/gpio.h linux/gpio.h for crying out loud... how many times Russell has to say the exact same thing ?? It will be removed +#define TSHUT_THRESHOLD_HOT 122000 /* 122 deg C */ +#define TSHUT_THRESHOLD_COLD 10 /* 100 deg C */ +#define
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Hi, On Thu, Aug 11, 2011 at 03:27:26PM +0530, J, KEERTHY wrote: On Wed, Aug 10, 2011 at 6:16 PM, Felipe Balbi ba...@ti.com wrote: Hi, (why aren't below in Cc ? I will add them. HARDWARE MONITORING M: Jean Delvare kh...@linux-fr.org M: Guenter Roeck guenter.ro...@ericsson.com L: lm-sens...@lm-sensors.org W: http://www.lm-sensors.org/ T: quilt kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git S: Maintained F: Documentation/hwmon/ F: drivers/hwmon/ F: include/linux/hwmon*.h) On Wed, Aug 10, 2011 at 05:55:22PM +0530, Keerthy wrote: On chip temperature sensor driver. The driver monitors the temperature of the MPU subsystem of the OMAP4. It sends notifications to the user space if the temperature crosses user defined thresholds via kobject_uevent interface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface. Signed-off-by: Keerthy j-keer...@ti.com --- drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 + drivers/hwmon/omap_temp_sensor.c | 950 ++ 3 files changed, 962 insertions(+), 0 deletions(-) create mode 100644 drivers/hwmon/omap_temp_sensor.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5f888f7..9c9cd8b 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -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 help diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 28061cf..d0f89f5 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -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.o diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? Clock rate setting functions. you shouldn't need in drivers. +#include linux/io.h +#include linux/debugfs.h why ?? It will be removed. +#include linux/delay.h +#include linux/slab.h +#include linux/platform_device.h +#include linux/init.h +#include plat/omap_device.h why ?? Context loss count shouldn't use in drivers. +#include linux/module.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 plat/common.h why ? usleep_range function. plat/common for usleep_range ?? usleep_range is defined in linux/delay.h +#include plat/temperature_sensor.h It is the header file with the structure definitions used in the driver. why
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On Thu, Aug 11, 2011 at 4:06 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Thu, Aug 11, 2011 at 03:27:26PM +0530, J, KEERTHY wrote: On Wed, Aug 10, 2011 at 6:16 PM, Felipe Balbi ba...@ti.com wrote: Hi, (why aren't below in Cc ? I will add them. HARDWARE MONITORING M: Jean Delvare kh...@linux-fr.org M: Guenter Roeck guenter.ro...@ericsson.com L: lm-sens...@lm-sensors.org W: http://www.lm-sensors.org/ T: quilt kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git S: Maintained F: Documentation/hwmon/ F: drivers/hwmon/ F: include/linux/hwmon*.h) On Wed, Aug 10, 2011 at 05:55:22PM +0530, Keerthy wrote: On chip temperature sensor driver. The driver monitors the temperature of the MPU subsystem of the OMAP4. It sends notifications to the user space if the temperature crosses user defined thresholds via kobject_uevent interface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface. Signed-off-by: Keerthy j-keer...@ti.com --- drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 + drivers/hwmon/omap_temp_sensor.c | 950 ++ 3 files changed, 962 insertions(+), 0 deletions(-) create mode 100644 drivers/hwmon/omap_temp_sensor.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5f888f7..9c9cd8b 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -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 help diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 28061cf..d0f89f5 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -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.o diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? Clock rate setting functions. you shouldn't need in drivers. It is a one time setting of the rate so keeping it in drivers. +#include linux/io.h +#include linux/debugfs.h why ?? It will be removed. +#include linux/delay.h +#include linux/slab.h +#include linux/platform_device.h +#include linux/init.h +#include plat/omap_device.h why ?? Context loss count shouldn't use in drivers. I guess already mmc and display are using omap_pm_get_dev_context_loss_count but are populating this function in pdata as a function pointer. I will add that code. +#include linux/module.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
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Hi, On Thu, Aug 11, 2011 at 06:30:04PM +0530, J, KEERTHY wrote: diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? Clock rate setting functions. you shouldn't need in drivers. It is a one time setting of the rate so keeping it in drivers. you need some other way to handle this. Why do you need to manually set the rate rather than having hwmod handle this for you ? your argument that it's a one time setting is not enough to have this in the driver. Drivers should not care about clocks anymore, this should have been done on another layer. +#include linux/delay.h +#include linux/slab.h +#include linux/platform_device.h +#include linux/init.h +#include plat/omap_device.h why ?? Context loss count shouldn't use in drivers. I guess already mmc and display are using omap_pm_get_dev_context_loss_count but are populating this function in pdata as a function pointer. I will add that code. at least ;-) better than accessing that function directly. But think carefully how you will make this work when we move to DT. +#include plat/temperature_sensor.h It is the header file with the structure definitions used in the driver. why don't you put in linux/platform_data/ ?? I saw many omap specific header files in plat-omap/include/plat. Any specific reason behind placing the header in linux/platform_data? because it's not a good idea to perpetuate the failure. +#include mach/ctrl_module_core_44xx.h why ? It will be removed +#include mach/gpio.h linux/gpio.h for crying out loud... how many times Russell has to say the exact same thing ?? It will be removed oh, you don't even use any gpio ? Why do you blindly add so many headers if you don't need them ??? It is not required. this is my point, be careful when adding new drivers... You're consuming review bandwidth when you send code/patch/new-drivers and reviewers are part of an endangered species. If you keep on making such silly mistakes, you consume time from reviewers to let you know about things which are obvious, to say the least. So be careful when sending code, nobody is perfect, for sure, but by being careful you can help your code being accepted earlier. -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On 8/11/2011 7:42 PM, Felipe Balbi wrote: Hi, On Thu, Aug 11, 2011 at 06:30:04PM +0530, J, KEERTHY wrote: diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated -http://www.ti.com/ + * Author: J Keerthyj-keer...@ti.com + * Author: Moiz Sonasathm-sonas...@ti.com + * + * 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 + * + */ + +#includelinux/interrupt.h +#includelinux/clk.h why ?? Clock rate setting functions. you shouldn't need in drivers. It is a one time setting of the rate so keeping it in drivers. you need some other way to handle this. Why do you need to manually set the rate rather than having hwmod handle this for you ? Because hwmod has not idea about what rate a device needs to operate in any point. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Thu, Aug 11, 2011 at 06:30:04PM +0530, J, KEERTHY wrote: diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? Clock rate setting functions. you shouldn't need in drivers. It is a one time setting of the rate so keeping it in drivers. you need some other way to handle this. Why do you need to manually set the rate rather than having hwmod handle this for you ? your argument that it's a one time setting is not enough to have this in the driver. Drivers should not care about clocks anymore, this should have been done on another layer. Hwmod will have no idea on the rate required. +#include linux/delay.h +#include linux/slab.h +#include linux/platform_device.h +#include linux/init.h +#include plat/omap_device.h why ?? Context loss count shouldn't use in drivers. I guess already mmc and display are using omap_pm_get_dev_context_loss_count but are populating this function in pdata as a function pointer. I will add that code. at least ;-) better than accessing that function directly. But think carefully how you will make this work when we move to DT. Ok +#include plat/temperature_sensor.h It is the header file with the structure definitions used in the driver. why don't you put in linux/platform_data/ ?? I saw many omap specific header files in plat-omap/include/plat. Any specific reason behind placing the header in linux/platform_data? because it's not a good idea to perpetuate the failure. May be Tony can give some inputs on where the OMAP specific header should be placed. +#include mach/ctrl_module_core_44xx.h why ? It will be removed +#include mach/gpio.h linux/gpio.h for crying out loud... how many times Russell has to say the exact same thing ?? It will be removed oh, you don't even use any gpio ? Why do you blindly add so many headers if you don't need them ??? It is not required. this is my point, be careful when adding new drivers... You're consuming review bandwidth when you send code/patch/new-drivers and reviewers are part of an endangered species. If you keep on making such silly mistakes, you consume time from reviewers to let you know about things which are obvious, to say the least. So be careful when sending code, nobody is perfect, for sure, but by being careful you can help your code being accepted earlier. My Bad. I will take care of this. -- balbi -- Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On Thu, 2011-08-11 at 09:00 -0400, J, KEERTHY wrote: On Thu, Aug 11, 2011 at 4:06 PM, Felipe Balbi ba...@ti.com wrote: [ ... ] + 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); temp = ?? + temp = temp * 1000 / (temp_sensor-clk_rate); temp *= ?? Need to multiply the temp with 1000 before dividing. temp *= evaluates the RHS first and then multiplies LHS. temp *= 1000; temp /= clk_rate; Different coding style. I preferred to do it in a single line. Me too. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Hi, On Thu, Aug 11, 2011 at 08:02:55PM +0530, J, KEERTHY wrote: On Thu, Aug 11, 2011 at 7:42 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Thu, Aug 11, 2011 at 06:30:04PM +0530, J, KEERTHY wrote: diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? Clock rate setting functions. you shouldn't need in drivers. It is a one time setting of the rate so keeping it in drivers. you need some other way to handle this. Why do you need to manually set the rate rather than having hwmod handle this for you ? your argument that it's a one time setting is not enough to have this in the driver. Drivers should not care about clocks anymore, this should have been done on another layer. Hwmod will have no idea on the rate required. does the rate need to change ? Also, I have not mentioned hwmod anytime simply because I'm not sure where this should be placed, but hopefully not in the driver. If the clock doesn't need to change after you set the correct rate, then there's really no point in adding complexity to the driver. Tony, would you have any comment here ? +#include plat/temperature_sensor.h It is the header file with the structure definitions used in the driver. why don't you put in linux/platform_data/ ?? I saw many omap specific header files in plat-omap/include/plat. Any specific reason behind placing the header in linux/platform_data? because it's not a good idea to perpetuate the failure. May be Tony can give some inputs on where the OMAP specific header should be placed. it's not omap-specific. It's specific to your driver and considering it's platform_data, it should sit under include/linux/platform_data.h. That directory was created (fairly recently) to hold platform_data... -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Hi, On Thu, Aug 11, 2011 at 09:54:09PM +0300, Felipe Balbi wrote: you need some other way to handle this. Why do you need to manually set the rate rather than having hwmod handle this for you ? your argument that it's a one time setting is not enough to have this in the driver. Drivers should not care about clocks anymore, this should have been done on another layer. Hwmod will have no idea on the rate required. does the rate need to change ? Also, I have not mentioned hwmod anytime i did mention hwmod, nevermind that part. Still I'm not sure where is the right place to handle this. -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On 08/11/2011 01:55 PM, Felipe Balbi wrote:
Hi,
On Thu, Aug 11, 2011 at 09:54:09PM +0300, Felipe Balbi wrote:
you need some other way to handle this. Why do you need to manually set
the rate rather than having hwmod handle this for you ?
your argument that it's a one time setting is not enough to have this
in the driver. Drivers should not care about clocks anymore, this should
have been done on another layer.
Hwmod will have no idea on the rate required.
does the rate need to change ? Also, I have not mentioned hwmod anytime
i did mention hwmod, nevermind that part. Still I'm not sure where is
the right place to handle this.
Aren't the omap_device_pm_latency callbacks the right place to do it?
e.g. in the following snippet from mach-omap2/temp_sensor_device.c
+static struct omap_device_pm_latency omap_temp_sensor_latency[] = {
+ {
+.deactivate_func = omap_device_idle_hwmods,
+.activate_func = omap_device_enable_hwmods,
+.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+ }
+};
instead of directly pointing activate_func to omap_device_enable_hwmods,
it could point to a function that sets the required clock rate and then
enables the hwmod.
regards,
-roger
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On Fri, Aug 12, 2011 at 3:07 AM, Roger Quadros rog...@ti.com wrote:
On 08/11/2011 01:55 PM, Felipe Balbi wrote:
Hi,
On Thu, Aug 11, 2011 at 09:54:09PM +0300, Felipe Balbi wrote:
you need some other way to handle this. Why do you need to manually set
the rate rather than having hwmod handle this for you ?
your argument that it's a one time setting is not enough to have this
in the driver. Drivers should not care about clocks anymore, this should
have been done on another layer.
Hwmod will have no idea on the rate required.
does the rate need to change ? Also, I have not mentioned hwmod anytime
i did mention hwmod, nevermind that part. Still I'm not sure where is
the right place to handle this.
Aren't the omap_device_pm_latency callbacks the right place to do it?
It is a one time setting. These callbacks get called everytime
pm_runtime_get_sync and pm_runtime_put_sync are called.
IMHO this is not the right place.
e.g. in the following snippet from mach-omap2/temp_sensor_device.c
+static struct omap_device_pm_latency omap_temp_sensor_latency[] = {
+ {
+ .deactivate_func = omap_device_idle_hwmods,
+ .activate_func = omap_device_enable_hwmods,
+ .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+ }
+};
instead of directly pointing activate_func to omap_device_enable_hwmods,
it could point to a function that sets the required clock rate and then
enables the hwmod.
regards,
-roger
--
Regards and Thanks,
Keerthy
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
On 8/12/2011 3:07 AM, Roger Quadros wrote:
On 08/11/2011 01:55 PM, Felipe Balbi wrote:
Hi,
On Thu, Aug 11, 2011 at 09:54:09PM +0300, Felipe Balbi wrote:
you need some other way to handle this. Why do you need to manually set
the rate rather than having hwmod handle this for you ?
your argument that it's a one time setting is not enough to have this
in the driver. Drivers should not care about clocks anymore, this should
have been done on another layer.
Hwmod will have no idea on the rate required.
does the rate need to change ? Also, I have not mentioned hwmod anytime
i did mention hwmod, nevermind that part. Still I'm not sure where is
the right place to handle this.
Aren't the omap_device_pm_latency callbacks the right place to do it?
e.g. in the following snippet from mach-omap2/temp_sensor_device.c
+static struct omap_device_pm_latency omap_temp_sensor_latency[] = {
+ {
+.deactivate_func = omap_device_idle_hwmods,
+.activate_func = omap_device_enable_hwmods,
+.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+ }
+};
instead of directly pointing activate_func to omap_device_enable_hwmods,
it could point to a function that sets the required clock rate and then
enables the hwmod.
FWIK, its a one time requirement to set the clock rate to the
right rate the device can operate in based on what a platform
supports. What you are suggesting would add the overhead of doing
this every time the device is runtime enabled/idled.
regards,
-roger
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver
Hi, (why aren't below in Cc ? HARDWARE MONITORING M: Jean Delvare kh...@linux-fr.org M: Guenter Roeck guenter.ro...@ericsson.com L: lm-sens...@lm-sensors.org W: http://www.lm-sensors.org/ T: quilt kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git S: Maintained F: Documentation/hwmon/ F: drivers/hwmon/ F: include/linux/hwmon*.h) On Wed, Aug 10, 2011 at 05:55:22PM +0530, Keerthy wrote: On chip temperature sensor driver. The driver monitors the temperature of the MPU subsystem of the OMAP4. It sends notifications to the user space if the temperature crosses user defined thresholds via kobject_uevent interface. The user is allowed to configure the temperature thresholds vis sysfs nodes exposed using hwmon interface. Signed-off-by: Keerthy j-keer...@ti.com --- drivers/hwmon/Kconfig| 11 + drivers/hwmon/Makefile |1 + drivers/hwmon/omap_temp_sensor.c | 950 ++ 3 files changed, 962 insertions(+), 0 deletions(-) create mode 100644 drivers/hwmon/omap_temp_sensor.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5f888f7..9c9cd8b 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -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 help diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 28061cf..d0f89f5 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -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.o diff --git a/drivers/hwmon/omap_temp_sensor.c b/drivers/hwmon/omap_temp_sensor.c new file mode 100644 index 000..15e2559 --- /dev/null +++ b/drivers/hwmon/omap_temp_sensor.c @@ -0,0 +1,950 @@ +/* + * OMAP4 Temperature sensor driver file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@ti.com + * Author: Moiz Sonasath m-sonas...@ti.com + * + * 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 why ?? +#include linux/io.h +#include linux/debugfs.h why ?? +#include linux/delay.h +#include linux/slab.h +#include linux/platform_device.h +#include linux/init.h +#include plat/omap_device.h why ?? +#include linux/module.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 plat/common.h why ? +#include plat/temperature_sensor.h why ? +#include mach/ctrl_module_core_44xx.h why ? +#include mach/gpio.h linux/gpio.h for crying out loud... how many times Russell has to say the exact same thing ?? +#define TSHUT_THRESHOLD_HOT 122000 /* 122 deg C */ +#define TSHUT_THRESHOLD_COLD 10 /* 100 deg C */ +#define BGAP_THRESHOLD_T_HOT 73000 /* 73 deg C */ +#define BGAP_THRESHOLD_T_COLD71000 /* 71 deg C */ +#define OMAP_ADC_START_VALUE 530 +#define OMAP_ADC_END_VALUE 923 + +/* + * omap_temp_sensor structure + * @hwmon_dev - device pointer + * @clock - Clock pointer + * @registers - Pointer to structure with register
