On Wed, Jun 20, 2012 at 3:04 PM, hongbo.zhang <hongbo.zh...@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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
