Re: [PATCH 3/6] OMAP4460: Temperature sensor data
On Sat, Sep 24, 2011 at 1:18 PM, Paul Walmsley p...@pwsan.com wrote: On Fri, 23 Sep 2011, J, KEERTHY wrote: On Fri, Sep 23, 2011 at 11:33 AM, Paul Walmsley p...@pwsan.com wrote: On Thu, 22 Sep 2011, Keerthy wrote: @@ -0,0 +1,175 @@ +/* + * OMAP system control module header file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@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 + * + */ + +#ifndef __ARCH_ARM_PLAT_OMAP_INCLUDE_PLAT_TEMPERATURE_SENSOR_H +#define __ARCH_ARM_PLAT_OMAP_INCLUDE_PLAT_TEMPERATURE_SENSOR_H You're also missing important #includes here for things like mutexes and kernel types that you use later on in the file. Those header files are included in c files. And how does that affect my comment? +#define OMAP_ADC_START_VALUE 530 +#define OMAP_ADC_END_VALUE 923 Are these OMAP4460, OMAP4xxx, or OMAP2+ specific? OMAP4460. I will pass even these values through pdata since they differ from platform to platform. So then the macro names need to include OMAP4460 or whatever SoC they are first valid for. + +/** + * struct omap4460plus_scm_dev_attr - device attributes for scm There are loads of references to 'omap4460plus' when it seems to me that much of this driver should also apply to OMAP4430 also. Shouldn't this driver be named something like 'omap4430plus_scm' or even better 'omap4_scm' ? This is used by hwmod. Hence keeping it in the header file. Did you even read my comment before responding? Sorry about this. OMAP4430 and OMAP4460 temperature sensors are different. The register layout and the functionalities differ. The OMAP4430 temperature sensor is not accurate. The SCM driver can be generic but the temperature sensor driver should be OMAP4460 onwards. Please let me know if this is fine? - Paul -- 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: [PATCH 3/6] OMAP4460: Temperature sensor data
On Fri, 23 Sep 2011, J, KEERTHY wrote: On Fri, Sep 23, 2011 at 11:33 AM, Paul Walmsley p...@pwsan.com wrote: On Thu, 22 Sep 2011, Keerthy wrote: @@ -0,0 +1,175 @@ +/* + * OMAP system control module header file + * + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ + * Author: J Keerthy j-keer...@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 + * + */ + +#ifndef __ARCH_ARM_PLAT_OMAP_INCLUDE_PLAT_TEMPERATURE_SENSOR_H +#define __ARCH_ARM_PLAT_OMAP_INCLUDE_PLAT_TEMPERATURE_SENSOR_H You're also missing important #includes here for things like mutexes and kernel types that you use later on in the file. Those header files are included in c files. And how does that affect my comment? +#define OMAP_ADC_START_VALUE 530 +#define OMAP_ADC_END_VALUE 923 Are these OMAP4460, OMAP4xxx, or OMAP2+ specific? OMAP4460. I will pass even these values through pdata since they differ from platform to platform. So then the macro names need to include OMAP4460 or whatever SoC they are first valid for. + +/** + * struct omap4460plus_scm_dev_attr - device attributes for scm There are loads of references to 'omap4460plus' when it seems to me that much of this driver should also apply to OMAP4430 also. Shouldn't this driver be named something like 'omap4430plus_scm' or even better 'omap4_scm' ? This is used by hwmod. Hence keeping it in the header file. Did you even read my comment before responding? - Paul
Re: [PATCH 3/6] OMAP4460: Temperature sensor data
On Fri, 23 Sep 2011, J, KEERTHY wrote: On Fri, Sep 23, 2011 at 11:33 AM, Paul Walmsley p...@pwsan.com wrote: On Thu, 22 Sep 2011, Keerthy wrote: diff --git a/arch/arm/mach-omap2/temp_sensor4460_data.c b/arch/arm/mach-omap2/temp_sensor4460_data.c new file mode 100644 index 000..2804615 --- /dev/null +++ b/arch/arm/mach-omap2/temp_sensor4460_data.c Is there some reason why this shouldn't go into drivers/ in some form? This is used by mach-omap2. Why does something in mach-omap2 need this data? diff --git a/arch/arm/plat-omap/include/plat/scm.h b/arch/arm/plat-omap/include/plat/scm.h new file mode 100644 index 000..47aa38f --- /dev/null +++ b/arch/arm/plat-omap/include/plat/scm.h If this is being used by a driver, then this header file should go into the appropriate drivers/ subdirectory. If it is being used by code in arch/arm/mach-omap2, then please use the existing arch/arm/mach-omap2/control.h instead. The header file has structures used both by drivers/ and mach-omap. So kept it in plat-omap. The point is, if there are structure definitions and macros that are only needed by code in drivers/, then those should be split off into a separate file and placed in drivers/. Similarly, if there are elements of this file that are only used in mach-omap2/, then those should go into mach-omap2/control.h. About the only part off the top of my head that should go into a plat-omap header file should be the dev_attr structure. And it's debatable whether this driver even needs a dev_attr, or whether all this data should just go into an omap4460_scm.c MFD driver that uses a bunch of common code for the parts that are shared with 4430, etc. Do you have any views on this issue? - Paul
Re: [PATCH 3/6] OMAP4460: Temperature sensor data
On Sat, Sep 24, 2011 at 1:29 PM, Paul Walmsley p...@pwsan.com wrote: On Fri, 23 Sep 2011, J, KEERTHY wrote: On Fri, Sep 23, 2011 at 11:33 AM, Paul Walmsley p...@pwsan.com wrote: On Thu, 22 Sep 2011, Keerthy wrote: diff --git a/arch/arm/mach-omap2/temp_sensor4460_data.c b/arch/arm/mach-omap2/temp_sensor4460_data.c new file mode 100644 index 000..2804615 --- /dev/null +++ b/arch/arm/mach-omap2/temp_sensor4460_data.c Is there some reason why this shouldn't go into drivers/ in some form? This is used by mach-omap2. Why does something in mach-omap2 need this data? The scm hwmod is populating the pointer to the register set which is specific to OMAP4460. So i have kept the OMAP4460 specific data file in mach-omap2. diff --git a/arch/arm/plat-omap/include/plat/scm.h b/arch/arm/plat-omap/include/plat/scm.h new file mode 100644 index 000..47aa38f --- /dev/null +++ b/arch/arm/plat-omap/include/plat/scm.h If this is being used by a driver, then this header file should go into the appropriate drivers/ subdirectory. If it is being used by code in arch/arm/mach-omap2, then please use the existing arch/arm/mach-omap2/control.h instead. The header file has structures used both by drivers/ and mach-omap. So kept it in plat-omap. The point is, if there are structure definitions and macros that are only needed by code in drivers/, then those should be split off into a separate file and placed in drivers/. Similarly, if there are elements of this file that are only used in mach-omap2/, then those should go into mach-omap2/control.h. About the only part off the top of my head that should go into a plat-omap header file should be the dev_attr structure. And it's debatable whether this driver even needs a dev_attr, or whether all this data should just go into an omap4460_scm.c MFD driver that uses a bunch of common code for the parts that are shared with 4430, etc. Do you have any views on this issue? There can be a common omap4_scm.c driver. The temperature sensor is different from OMAP4430 and OMAP4460. So keeping the temperature sensor as omap4460plus. Coming to structure definitions. pdata structure is needed both by mach-omap device file to populate it and also by the driver t extract it. So keeping all of the structure definitions in one header file in plat_omap. My question is which is the ideal place to keep the common structure definition like pdata? Since the temperature sensor does not have a separate hwmod of its own i feel there is a necessity of dev_attr. - Paul -- 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: [PATCH 3/6] OMAP4460: Temperature sensor data
On Fri, Sep 23, 2011 at 11:33 AM, Paul Walmsley p...@pwsan.com wrote:
Hi
some comments
On Thu, 22 Sep 2011, Keerthy wrote:
The register set and the
bit fields might vary across OMAP versions. Hence
creating a structure comprising of all the registers
and bit fields to make the driver uniform for all the
versions with different register sets. The data file
contains the structure populated with register offsets
and bit fields corresponding to OMAP4460 on die sensor.
Signed-off-by: Keerthy j-keer...@ti.com
Cc: t...@atomide.com
---
arch/arm/mach-omap2/Makefile | 2 +-
arch/arm/mach-omap2/temp_sensor4460_data.c | 115 ++
arch/arm/plat-omap/include/plat/scm.h | 175
3 files changed, 291 insertions(+), 1 deletions(-)
create mode 100644 arch/arm/mach-omap2/temp_sensor4460_data.c
create mode 100644 arch/arm/plat-omap/include/plat/scm.h
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 46a3497..e6f8d36 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -86,7 +86,7 @@ obj-$(CONFIG_ARCH_OMAP3) += prcm.o
cm2xxx_3xxx.o prm2xxx_3xxx.o \
obj-$(CONFIG_ARCH_OMAP4) += prcm.o cm2xxx_3xxx.o cminst44xx.o \
cm44xx.o prcm_mpu44xx.o \
prminst44xx.o vc44xx_data.o \
- vp44xx_data.o
+ vp44xx_data.o temp_sensor4460_data.o
This is not part of the PRCM, so it should not be added here.
It also should be conditional on CONFIG_SOC_OMAP4460. If that Kconfig
entry doesn't exist, it should be added.
Ok. I will add it.
# OMAP voltage domains
ifeq ($(CONFIG_PM),y)
diff --git a/arch/arm/mach-omap2/temp_sensor4460_data.c
b/arch/arm/mach-omap2/temp_sensor4460_data.c
new file mode 100644
index 000..2804615
--- /dev/null
+++ b/arch/arm/mach-omap2/temp_sensor4460_data.c
Is there some reason why this shouldn't go into drivers/ in some form?
This is used by mach-omap2.
@@ -0,0 +1,115 @@
+/*
+ * OMAP4460 on die Temperature sensor data file
+ *
+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
+ * Author: J Keerthy j-keer...@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/slab.h
+#include control.h
+#include plat/scm.h
+
+/*
+ * OMAP4460 has one instance of thermal sensor for MPU
+ * need to describe the individual bit fields
+ */
+struct omap_temp_sensor_registers omap_mpu_temp_sensor_registers = {
This is going to break if we want to compile a kernel with support
for, say, the 4430 and 4460 temperature sensors.
Ok. I will rename this as omap4460_temp_sensor_registers
+ .temp_sensor_ctrl = OMAP4460_TEMP_SENSOR_CTRL_OFFSET,
+ .bgap_tempsoff_mask = OMAP4460_BGAP_TEMPSOFF_MASK,
+ .bgap_soc_mask = OMAP4460_BGAP_TEMP_SENSOR_SOC_MASK,
+ .bgap_eocz_mask = OMAP4460_BGAP_TEMP_SENSOR_EOCZ_MASK,
+ .bgap_dtemp_mask = OMAP4460_BGAP_TEMP_SENSOR_DTEMP_MASK,
+
+ .bgap_mask_ctrl = OMAP4460_BGAP_CTRL_OFFSET,
+ .mask_hot_mask = OMAP4460_MASK_HOT_MASK,
+ .mask_cold_mask = OMAP4460_MASK_COLD_MASK,
+
+ .bgap_mode_ctrl = OMAP4460_BGAP_CTRL_OFFSET,
+ .mode_ctrl_mask = OMAP4460_SINGLE_MODE_MASK,
+
+ .bgap_counter = OMAP4460_BGAP_COUNTER_OFFSET,
+ .counter_mask = OMAP4460_COUNTER_MASK,
+
+ .bgap_threshold = OMAP4460_BGAP_THRESHOLD_OFFSET,
+ .threshold_thot_mask = OMAP4460_T_HOT_MASK,
+ .threshold_tcold_mask = OMAP4460_T_COLD_MASK,
+
+ .thsut_threshold = OMAP4460_BGAP_TSHUT_OFFSET,
tshut is misspelled.
I will correct this.
+ .tshut_hot_mask = OMAP4460_TSHUT_HOT_MASK,
+ .tshut_cold_mask = OMAP4460_TSHUT_COLD_MASK,
+
+ .bgap_status = OMAP4460_BGAP_STATUS_OFFSET,
+ .status_clean_stop_mask = OMAP4460_CLEAN_STOP_MASK,
+ .status_bgap_alert_mask = OMAP4460_BGAP_ALERT_MASK,
+
