[PATCH 0/4] staging: omap-thermal: clean-ups and fixes

[PATCH 0/4] staging: omap-thermal: clean-ups and fixes

[Eduardo Valentin]

Hello Greg,

Here are some patches to clean the omap-thermal driver code a bit.
There are 2 fixes as well related to registration to thermal fw.

These patches are based on staging-next tree.

They are also available here:
git@gitorious.org:thermal-framework/thermal-framework.git thermal_work/omap/omap-thermal-fixes

All best,

Eduardo Valentin (3):
  staging: omap-thermal: remove checkpatch.pl warnings on data files
  staging: omap-thermal: fix polling period settings
  staging: omap-thermal: improve conf data handling and initialization

J Keerthy (1):
  staging: omap-thermal: Correct checkpatch.pl warnings

 drivers/staging/omap-thermal/omap-bandgap.c        |   21 ++++----
 drivers/staging/omap-thermal/omap-thermal-common.c |   42 +++++++++++++--
 drivers/staging/omap-thermal/omap4-thermal.c       |   54 ++++++++++----------
 drivers/staging/omap-thermal/omap5-thermal.c       |   38 +++++++-------
 4 files changed, 93 insertions(+), 62 deletions(-)

-- 
1.7.7.1.488.ge8e1c

[PATCH 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

[Eduardo Valentin]

From: J Keerthy <j-keerthy@ti.com>

Removes checkpatch warnings on omap-bandgap.c.

Signed-off-by: J Keerthy <j-keerthy@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/omap-thermal/omap-bandgap.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/omap-thermal/omap-bandgap.c b/drivers/staging/omap-thermal/omap-bandgap.c
index c556abb..9ef44ea 100644
--- a/drivers/staging/omap-thermal/omap-bandgap.c
+++ b/drivers/staging/omap-thermal/omap-bandgap.c
@@ -1037,20 +1037,20 @@ static int omap_bandgap_save_ctxt(struct omap_bandgap *bg_ptr)
 
 		if (OMAP_BANDGAP_HAS(bg_ptr, MODE_CONFIG))
 			rval->bg_mode_ctrl = omap_bandgap_readl(bg_ptr,
-								tsr->bgap_mode_ctrl);
+							tsr->bgap_mode_ctrl);
 		if (OMAP_BANDGAP_HAS(bg_ptr, COUNTER))
 			rval->bg_counter = omap_bandgap_readl(bg_ptr,
-							      tsr->bgap_counter);
+							tsr->bgap_counter);
 		if (OMAP_BANDGAP_HAS(bg_ptr, TALERT)) {
 			rval->bg_threshold = omap_bandgap_readl(bg_ptr,
-								tsr->bgap_threshold);
+							tsr->bgap_threshold);
 			rval->bg_ctrl = omap_bandgap_readl(bg_ptr,
-							   tsr->bgap_mask_ctrl);
+						   tsr->bgap_mask_ctrl);
 		}
 
 		if (OMAP_BANDGAP_HAS(bg_ptr, TSHUT_CONFIG))
 			rval->tshut_threshold = omap_bandgap_readl(bg_ptr,
-								   tsr->tshut_threshold);
+						   tsr->tshut_threshold);
 	}
 
 	return 0;
@@ -1074,8 +1074,9 @@ static int omap_bandgap_restore_ctxt(struct omap_bandgap *bg_ptr)
 
 		if (val == 0) {
 			if (OMAP_BANDGAP_HAS(bg_ptr, TSHUT_CONFIG))
-				omap_bandgap_writel(bg_ptr, rval->tshut_threshold,
-							   tsr->tshut_threshold);
+				omap_bandgap_writel(bg_ptr,
+					rval->tshut_threshold,
+						   tsr->tshut_threshold);
 			/* Force immediate temperature measurement and update
 			 * of the DTEMP field
 			 */
-- 
1.7.7.1.488.ge8e1c

[PATCH 2/4] staging: omap-thermal: remove checkpatch.pl warnings on data files

[Eduardo Valentin]

Simple checkpatch.pl clean ups.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/omap-thermal/omap4-thermal.c |   54 +++++++++++++-------------
 drivers/staging/omap-thermal/omap5-thermal.c |   38 +++++++++---------
 2 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/omap-thermal/omap4-thermal.c b/drivers/staging/omap-thermal/omap4-thermal.c
index fa9dbcd..04c02b6 100644
--- a/drivers/staging/omap-thermal/omap4-thermal.c
+++ b/drivers/staging/omap-thermal/omap4-thermal.c
@@ -77,15 +77,15 @@ const struct omap_bandgap_data omap4430_data = {
 	.remove_sensor = omap_thermal_remove_sensor,
 	.sensors = {
 		{
-			.registers = &omap4430_mpu_temp_sensor_registers,
-			.ts_data = &omap4430_mpu_temp_sensor_data,
-			.domain = "cpu",
-			.slope = 0,
-			.constant = 20000,
-			.slope_pcb = 0,
-			.constant_pcb = 20000,
-			.register_cooling = omap_thermal_register_cpu_cooling,
-			.unregister_cooling = omap_thermal_unregister_cpu_cooling,
+		.registers = &omap4430_mpu_temp_sensor_registers,
+		.ts_data = &omap4430_mpu_temp_sensor_data,
+		.domain = "cpu",
+		.slope = 0,
+		.constant = 20000,
+		.slope_pcb = 0,
+		.constant_pcb = 20000,
+		.register_cooling = omap_thermal_register_cpu_cooling,
+		.unregister_cooling = omap_thermal_unregister_cpu_cooling,
 		},
 	},
 	.sensor_count = 1,
@@ -215,15 +215,15 @@ const struct omap_bandgap_data omap4460_data = {
 	.remove_sensor = omap_thermal_remove_sensor,
 	.sensors = {
 		{
-			.registers = &omap4460_mpu_temp_sensor_registers,
-			.ts_data = &omap4460_mpu_temp_sensor_data,
-			.domain = "cpu",
-			.slope = OMAP_GRADIENT_SLOPE_4460,
-			.constant = OMAP_GRADIENT_CONST_4460,
-			.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_4460,
-			.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_4460,
-			.register_cooling = omap_thermal_register_cpu_cooling,
-			.unregister_cooling = omap_thermal_unregister_cpu_cooling,
+		.registers = &omap4460_mpu_temp_sensor_registers,
+		.ts_data = &omap4460_mpu_temp_sensor_data,
+		.domain = "cpu",
+		.slope = OMAP_GRADIENT_SLOPE_4460,
+		.constant = OMAP_GRADIENT_CONST_4460,
+		.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_4460,
+		.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_4460,
+		.register_cooling = omap_thermal_register_cpu_cooling,
+		.unregister_cooling = omap_thermal_unregister_cpu_cooling,
 		},
 	},
 	.sensor_count = 1,
@@ -244,15 +244,15 @@ const struct omap_bandgap_data omap4470_data = {
 	.remove_sensor = omap_thermal_remove_sensor,
 	.sensors = {
 		{
-			.registers = &omap4460_mpu_temp_sensor_registers,
-			.ts_data = &omap4460_mpu_temp_sensor_data,
-			.domain = "cpu",
-			.slope = OMAP_GRADIENT_SLOPE_4470,
-			.constant = OMAP_GRADIENT_CONST_4470,
-			.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_4470,
-			.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_4470,
-			.register_cooling = omap_thermal_register_cpu_cooling,
-			.unregister_cooling = omap_thermal_unregister_cpu_cooling,
+		.registers = &omap4460_mpu_temp_sensor_registers,
+		.ts_data = &omap4460_mpu_temp_sensor_data,
+		.domain = "cpu",
+		.slope = OMAP_GRADIENT_SLOPE_4470,
+		.constant = OMAP_GRADIENT_CONST_4470,
+		.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_4470,
+		.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_4470,
+		.register_cooling = omap_thermal_register_cpu_cooling,
+		.unregister_cooling = omap_thermal_unregister_cpu_cooling,
 		},
 	},
 	.sensor_count = 1,
diff --git a/drivers/staging/omap-thermal/omap5-thermal.c b/drivers/staging/omap-thermal/omap5-thermal.c
index 0658af2..2f3a498 100644
--- a/drivers/staging/omap-thermal/omap5-thermal.c
+++ b/drivers/staging/omap-thermal/omap5-thermal.c
@@ -268,29 +268,29 @@ const struct omap_bandgap_data omap5430_data = {
 	.remove_sensor = omap_thermal_remove_sensor,
 	.sensors = {
 		{
-			.registers = &omap5430_mpu_temp_sensor_registers,
-			.ts_data = &omap5430_mpu_temp_sensor_data,
-			.domain = "cpu",
-			.register_cooling = omap_thermal_register_cpu_cooling,
-			.unregister_cooling = omap_thermal_unregister_cpu_cooling,
-			.slope = OMAP_GRADIENT_SLOPE_5430_CPU,
-			.constant = OMAP_GRADIENT_CONST_5430_CPU,
-			.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_5430_CPU,
-			.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_5430_CPU,
+		.registers = &omap5430_mpu_temp_sensor_registers,
+		.ts_data = &omap5430_mpu_temp_sensor_data,
+		.domain = "cpu",
+		.register_cooling = omap_thermal_register_cpu_cooling,
+		.unregister_cooling = omap_thermal_unregister_cpu_cooling,
+		.slope = OMAP_GRADIENT_SLOPE_5430_CPU,
+		.constant = OMAP_GRADIENT_CONST_5430_CPU,
+		.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_5430_CPU,
+		.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_5430_CPU,
 		},
 		{
-			.registers = &omap5430_gpu_temp_sensor_registers,
-			.ts_data = &omap5430_gpu_temp_sensor_data,
-			.domain = "gpu",
-			.slope = OMAP_GRADIENT_SLOPE_5430_GPU,
-			.constant = OMAP_GRADIENT_CONST_5430_GPU,
-			.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_5430_GPU,
-			.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_5430_GPU,
+		.registers = &omap5430_gpu_temp_sensor_registers,
+		.ts_data = &omap5430_gpu_temp_sensor_data,
+		.domain = "gpu",
+		.slope = OMAP_GRADIENT_SLOPE_5430_GPU,
+		.constant = OMAP_GRADIENT_CONST_5430_GPU,
+		.slope_pcb = OMAP_GRADIENT_SLOPE_W_PCB_5430_GPU,
+		.constant_pcb = OMAP_GRADIENT_CONST_W_PCB_5430_GPU,
 		},
 		{
-			.registers = &omap5430_core_temp_sensor_registers,
-			.ts_data = &omap5430_core_temp_sensor_data,
-			.domain = "core",
+		.registers = &omap5430_core_temp_sensor_registers,
+		.ts_data = &omap5430_core_temp_sensor_data,
+		.domain = "core",
 		},
 	},
 	.sensor_count = 3,
-- 
1.7.7.1.488.ge8e1c

[PATCH 3/4] staging: omap-thermal: fix polling period settings

[Eduardo Valentin]

While registering the omap thermal zones we need to
properly specify TC1 and TC2, as long as the proper
passive polling period and monitor period.

This patch fixes the parameters passed while registering
the thermal zone.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/omap-thermal/omap-thermal-common.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/omap-thermal/omap-thermal-common.c b/drivers/staging/omap-thermal/omap-thermal-common.c
index 0675a5e..d156424 100644
--- a/drivers/staging/omap-thermal/omap-thermal-common.c
+++ b/drivers/staging/omap-thermal/omap-thermal-common.c
@@ -246,7 +246,9 @@ int omap_thermal_expose_sensor(struct omap_bandgap *bg_ptr, int id,
 	/* Create thermal zone */
 	data->omap_thermal = thermal_zone_device_register(domain,
 				OMAP_TRIP_NUMBER, 0, data, &omap_thermal_ops,
-				0, FAST_TEMP_MONITORING_RATE, 0, 0);
+				1, 2, /*TODO: remove this when FW allows */
+				FAST_TEMP_MONITORING_RATE,
+				FAST_TEMP_MONITORING_RATE);
 	if (IS_ERR_OR_NULL(data->omap_thermal)) {
 		dev_err(bg_ptr->dev, "thermal zone device is NULL\n");
 		return PTR_ERR(data->omap_thermal);
-- 
1.7.7.1.488.ge8e1c

[PATCH 4/4] staging: omap-thermal: improve conf data handling and initialization

[Eduardo Valentin]

While registering the thermal zone, it is required to have the cooling
devices already setup, so that the .bind callback can succeed.

Due to that, the driver code needs to be reorganized so that we first
setup the cooling devices then the zones. This way we cope with the
right thermal framework initialization sequence.

This patch changes the order of the thermal zone initialization,
so that we create it only when the cooling devices are available.
It also adds some defensive checks for the config data, so that
the callbacks are ready for calls when the data is still not
initialized.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/omap-thermal/omap-bandgap.c        |    6 ++--
 drivers/staging/omap-thermal/omap-thermal-common.c |   38 +++++++++++++++++---
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/omap-thermal/omap-bandgap.c b/drivers/staging/omap-thermal/omap-bandgap.c
index 9ef44ea..ff93c15 100644
--- a/drivers/staging/omap-thermal/omap-bandgap.c
+++ b/drivers/staging/omap-thermal/omap-bandgap.c
@@ -953,12 +953,12 @@ int __devinit omap_bandgap_probe(struct platform_device *pdev)
 	for (i = 0; i < bg_ptr->conf->sensor_count; i++) {
 		char *domain;
 
+		if (bg_ptr->conf->sensors[i].register_cooling)
+			bg_ptr->conf->sensors[i].register_cooling(bg_ptr, i);
+
 		domain = bg_ptr->conf->sensors[i].domain;
 		if (bg_ptr->conf->expose_sensor)
 			bg_ptr->conf->expose_sensor(bg_ptr, i, domain);
-
-		if (bg_ptr->conf->sensors[i].register_cooling)
-			bg_ptr->conf->sensors[i].register_cooling(bg_ptr, i);
 	}
 
 	/*
diff --git a/drivers/staging/omap-thermal/omap-thermal-common.c b/drivers/staging/omap-thermal/omap-thermal-common.c
index d156424..46ee0a9 100644
--- a/drivers/staging/omap-thermal/omap-thermal-common.c
+++ b/drivers/staging/omap-thermal/omap-thermal-common.c
@@ -77,10 +77,16 @@ static inline int omap_thermal_get_temp(struct thermal_zone_device *thermal,
 					 unsigned long *temp)
 {
 	struct omap_thermal_data *data = thermal->devdata;
-	struct omap_bandgap *bg_ptr = data->bg_ptr;
-	struct omap_temp_sensor *s = &bg_ptr->conf->sensors[data->sensor_id];
+	struct omap_bandgap *bg_ptr;
+	struct omap_temp_sensor *s;
 	int ret, tmp, pcb_temp, slope, constant;
 
+	if (!data)
+		return 0;
+
+	bg_ptr = data->bg_ptr;
+	s = &bg_ptr->conf->sensors[data->sensor_id];
+
 	ret = omap_bandgap_read_temperature(bg_ptr, data->sensor_id, &tmp);
 	if (ret)
 		return ret;
@@ -227,21 +233,37 @@ static struct thermal_zone_device_ops omap_thermal_ops = {
 	.get_crit_temp = omap_thermal_get_crit_temp,
 };
 
-int omap_thermal_expose_sensor(struct omap_bandgap *bg_ptr, int id,
-			       char *domain)
+static struct omap_thermal_data
+*omap_thermal_build_data(struct omap_bandgap *bg_ptr, int id)
 {
 	struct omap_thermal_data *data;
 
 	data = devm_kzalloc(bg_ptr->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
 		dev_err(bg_ptr->dev, "kzalloc fail\n");
-		return -ENOMEM;
+		return NULL;
 	}
 	data->sensor_id = id;
 	data->bg_ptr = bg_ptr;
 	data->mode = THERMAL_DEVICE_ENABLED;
 	INIT_WORK(&data->thermal_wq, omap_thermal_work);
 
+	return data;
+}
+
+int omap_thermal_expose_sensor(struct omap_bandgap *bg_ptr, int id,
+			       char *domain)
+{
+	struct omap_thermal_pdata pdata;
+
+	data = omap_bandgap_get_sensor_data(bg_ptr, id);
+
+	if (!data)
+		data = omap_thermal_build_pdata(bg_ptr, id);
+
+	if (!data)
+		return -EINVAL;
+
 	/* TODO: remove TC1 TC2 */
 	/* Create thermal zone */
 	data->omap_thermal = thermal_zone_device_register(domain,
@@ -335,6 +357,11 @@ int omap_thermal_register_cpu_cooling(struct omap_bandgap *bg_ptr, int id)
 	int tab_size, ret;
 
 	data = omap_bandgap_get_sensor_data(bg_ptr, id);
+	if (!data)
+		data = omap_thermal_build_pdata(bg_ptr, id);
+
+	if (!data)
+		return -EINVAL;
 
 	ret = omap_thermal_build_cpufreq_clip(bg_ptr, &tab_ptr, &tab_size);
 	if (ret < 0) {
@@ -351,6 +378,7 @@ int omap_thermal_register_cpu_cooling(struct omap_bandgap *bg_ptr, int id)
 		return PTR_ERR(data->cool_dev);
 	}
 	bg_ptr->conf->sensors[id].cooling_data.freq_clip_count = tab_size;
+	omap_bandgap_set_sensor_data(bg_ptr, id, data);
 
 	return 0;
 }
-- 
1.7.7.1.488.ge8e1c

Re: [PATCH 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

[Dan Carpenter]

On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> From: J Keerthy <j-keerthy@ti.com>
> 
> Removes checkpatch warnings on omap-bandgap.c.
> 

Which checkpatch.pl warnings?

> +				omap_bandgap_writel(bg_ptr,
> +					rval->tshut_threshold,
> +						   tsr->tshut_threshold);

That's just whacky.

Personally, I've never cared much about long lines, so I'd prefer
to leave these as is until someone can break the functions up and
remove them in a proper way instead of just shifting everything
randomly to the left.

regards,
dan carpenter

Re: [PATCH 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

[Dan Carpenter]

On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> > From: J Keerthy <j-keerthy@ti.com>
> > 
> > Removes checkpatch warnings on omap-bandgap.c.
> > 
> 
> Which checkpatch.pl warnings?
> 
> > +				omap_bandgap_writel(bg_ptr,
> > +					rval->tshut_threshold,
> > +						   tsr->tshut_threshold);
> 
> That's just whacky.
> 
> Personally, I've never cared much about long lines, so I'd prefer
> to leave these as is until someone can break the functions up and
> remove them in a proper way instead of just shifting everything
> randomly to the left.
> 

Sorry, that was my default response without looking at the code.

This is already broken up into small functions pretty nicely.  You
might want to consider using shorter names.  For example
omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
could be changed to just "bg" because it's obviously a pointer.

regards,
dan carpenter

Re: [PATCH 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

[Valentin, Eduardo]

Hello Dan,

On Wed, Sep 12, 2012 at 11:26 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
>> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
>> > From: J Keerthy <j-keerthy@ti.com>
>> >
>> > Removes checkpatch warnings on omap-bandgap.c.
>> >
>>
>> Which checkpatch.pl warnings?
>>
>> > +                           omap_bandgap_writel(bg_ptr,
>> > +                                   rval->tshut_threshold,
>> > +                                              tsr->tshut_threshold);
>>
>> That's just whacky.
>>
>> Personally, I've never cared much about long lines, so I'd prefer
>> to leave these as is until someone can break the functions up and
>> remove them in a proper way instead of just shifting everything
>> randomly to the left.
>>
>
> Sorry, that was my default response without looking at the code.
>
> This is already broken up into small functions pretty nicely.  You
> might want to consider using shorter names.  For example
> omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
> could be changed to just "bg" because it's obviously a pointer.

Yeah, that's one option. Of course the deal is to find the proper
balance between cryptic symbol names and code mangled / shifted to the
left.

>
> regards,
> dan carpenter



-- 

Eduardo Valentin

Re: [PATCH 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

[Dan Carpenter]

On Wed, Sep 12, 2012 at 12:19:00PM +0300, Valentin, Eduardo wrote:
> Hello Dan,
> 
> On Wed, Sep 12, 2012 at 11:26 AM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
> > On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
> >> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> >> > From: J Keerthy <j-keerthy@ti.com>
> >> >
> >> > Removes checkpatch warnings on omap-bandgap.c.
> >> >
> >>
> >> Which checkpatch.pl warnings?
> >>
> >> > +                           omap_bandgap_writel(bg_ptr,
> >> > +                                   rval->tshut_threshold,
> >> > +                                              tsr->tshut_threshold);
> >>
> >> That's just whacky.
> >>
> >> Personally, I've never cared much about long lines, so I'd prefer
> >> to leave these as is until someone can break the functions up and
> >> remove them in a proper way instead of just shifting everything
> >> randomly to the left.
> >>
> >
> > Sorry, that was my default response without looking at the code.
> >
> > This is already broken up into small functions pretty nicely.  You
> > might want to consider using shorter names.  For example
> > omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
> > could be changed to just "bg" because it's obviously a pointer.
> 
> Yeah, that's one option. Of course the deal is to find the proper
> balance between cryptic symbol names and code mangled / shifted to the
> left.
> 

Another option would be to just let checkpatch complain.  It's a
perl script, not the king of us.  Do what looks the nicest to human
beings.

regards,
dan carpenter