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.

> +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.

> @@ -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.

> 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.

> +#include <linux/compiler.h>

Why is this include needed?
(Just curious.)

> +/* 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 binding cooling device.\n");
> +                                       else
> +                                               pr_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().

> +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.

> +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!

Yours,
Linus Walleij