On 5 July 2012 07:20, Linus Walleij linus.walleij@linaro.org wrote:
On Wed, Jun 20, 2012 at 3:04 PM, hongbo.zhang hongbo.zhang@linaro.org wrote:
/*
- Thermal Sensor
- */
+#ifdef CONFIG_DB8500_THERMAL
Please don't #ifdef the platform data like this, Documentation/CodingStyle dislikes #ifdefs unless needed and this just saves a very marginal piece of code in footprint, and will compile just as well even if the driver is not selected.
Yes I will remove #ifdef
+static struct resource db8500_thsens_resources[] = {
+#ifdef CONFIG_DB8500_CPUFREQ_COOLING
Don't #ifdef this either.
Then you will need to add device tree bindings for all this platform data sooner or later. Please check with Lee Jones if uncertain.
OK I will check device tree codes and check with him if any doubt.
@@ -607,6 +676,8 @@ static struct platform_device
*snowball_platform_devs[] __initdata = {
&snowball_key_dev, &snowball_sbnet_dev, &ab8500_device,
&u8500_thsens_device,&u8500_cpufreq_cooling_device,};
As you can see the above will break compilation when the drivers are not selected if you don't remove the #ifdefs.
This is a silly mistake I made :(
diff --git a/drivers/thermal/cpu_cooling.c
b/drivers/thermal/cpu_cooling.c
index c40d9a0..a8aa10f 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -399,8 +399,7 @@ struct thermal_cooling_device
*cpufreq_cooling_register(
/*Verify that all the entries of freq_clip_table are present*/ for (i = 0; i < tab_size; i++) { clip_tab = ((struct freq_clip_table *)&tab_ptr[i]);
if (!clip_tab->freq_clip_max || !clip_tab->mask_val|| !clip_tab->temp_level) {
if (!clip_tab->freq_clip_max || !clip_tab->mask_val) { kfree(cpufreq_dev); return ERR_PTR(-EINVAL); }This looks like a generic patch to cpu_cooling, please break it out and send this separately to the thermal maintainer and discuss it already today.
Yes this should be a separate patch.
+#include <linux/compiler.h>
Why is this include needed? (Just curious.)
This is due to some obsolete debuging codes, I forgot to remove this header file.
+/* Bind callback functions for thermal zone */ +static int db8500_cdev_bind(struct thermal_zone_device *thermal,
struct thermal_cooling_device *cdev)+{
struct db8500_thermal_zone *pzone;struct db8500_thsens_platform_data *ptrips;int i, j, ret;pzone = (struct db8500_thermal_zone *)thermal->devdata;ptrips = pzone->thsens_pdev->dev.platform_data;for (i = 0; i < ptrips->num_trips; i++)for (j = 0; j < COOLING_DEV_MAX; j++)if (ptrips->trip_points[i].cooling_dev_name[j]&& cdev->type)
if(strcmp(ptrips->trip_points[i].cooling_dev_name[j], cdev->type) == 0) {
ret =thermal_zone_bind_cooling_device(thermal, i, cdev);
if (ret)pr_err("Error bindingcooling device.\n");
elsepr_info("Cooling device%s binded to trip point %d.\n", cdev->type, i);
}schedule_work(&pzone->therm_work);So what happens if this work is scheduled and you suspend before it executes?
I think you need a suspend/resume hook to clean that up? Probably also in remove().
Yes I will consider suspend/resume functions.
+static irqreturn_t prcmu_low_irq_handler(int irq, void *irq_data) +{
struct db8500_thermal_zone *pzone = irq_data;struct db8500_thsens_platform_data *ptrips;unsigned long next_low, next_high;int i;ptrips = pzone->thsens_pdev->dev.platform_data;for(i = 0; i < ptrips->num_trips; i++) {if (pzone->cur_high == ptrips->trip_points[i].temp)break;}if (i <= 1) {next_high = ptrips->trip_points[0].temp;next_low = PRCMU_DEFAULT_LOW_TEMP;} else {next_high = ptrips->trip_points[i-1].temp;next_low = ptrips->trip_points[i-2].temp;}(void) prcmu_stop_temp_sense();(void) prcmu_config_hotmon((u8)(next_low/1000),(u8)(next_high/1000));
(void) prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);pzone->cur_high = next_high;pzone->cur_low = next_low;pr_debug("PRCMU set max %ld, set min %ld\n", next_high,next_low);
schedule_work(&pzone->therm_work);return IRQ_HANDLED;+}
This also triggers a work that can be inhibited by suspend/resume or remove.
schedule_work(&pzone->therm_work);etc.
+static int __devinit db8500_thermal_probe(struct platform_device *pdev) +{
struct db8500_thermal_zone *pzone = NULL;struct db8500_thsens_platform_data *ptrips;int low_irq, high_irq, ret = 0;unsigned long dft_low, dft_high;pr_info("Function db8500_thermal_probe.\n");pzone = kzalloc(sizeof(struct db8500_thermal_zone), GFP_KERNEL);Use devm_kzalloc().
if (!pzone)return ENOMEM;pzone->thsens_pdev = pdev;low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");if (low_irq < 0) {pr_err("Get IRQ_HOTMON_LOW failed.\n");goto exit_irq;}ret = request_threaded_irq(low_irq, NULL, prcmu_low_irq_handler,IRQF_NO_SUSPEND, "dbx500_temp_low",pzone);
Use devm_request_threaded_irq().
if (ret < 0) {pr_err("Failed to allocate temp low irq.\n");goto exit_irq;}high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");if (high_irq < 0) {pr_err("Get IRQ_HOTMON_HIGH failed.\n");goto exit_irq;}ret = request_threaded_irq(high_irq, NULL,prcmu_high_irq_handler,
IRQF_NO_SUSPEND, "dbx500_temp_high",pzone);
Use devm_request_threaded_irq()
if (ret < 0) {pr_err("Failed to allocate temp high irq.\n");goto exit_irq;}(...)
+exit_irq:
if (pzone->low_irq > 0)free_irq(pzone->low_irq, pzone);if (pzone->low_irq > 0)free_irq(pzone->high_irq, pzone);kfree(pzone);None of this is needed if you use the devm_* functions, they are garbage collecting.
Good suggestion of using devm_* functions.
+static int __devexit db8500_thermal_remove(struct platform_device *pdev) +{
struct db8500_thermal_zone *pzone = th_zone;if (pzone && pzone->therm_dev)thermal_zone_device_unregister(pzone->therm_dev);And...
if (pzone && pzone->low_irq)free_irq(pzone->low_irq, pzone);if (pzone && pzone->high_irq)free_irq(pzone->high_irq, pzone);if (pzone)kfree(pzone);None of this either.
diff --git a/include/linux/platform_data/db8500_thermal.h
b/include/linux/platform_data/db8500_thermal.h
Thanks for using the proper platform data dir!
Thanks for all of you comments.
Yours, Linus Walleij