[V2,5/6] Thermal: Add ST-Ericsson DB8500 thermal dirver.

Message ID	1351079900-32236-6-git-send-email-hongbo.zhang@linaro.com (mailing list archive)
State	Not Applicable, archived
Headers	show Return-Path: <linux-pm-owner@vger.kernel.org> From: "hongbo.zhang" <hongbo.zhang@linaro.org> To: linaro-dev@lists.linaro.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Cc: patches@linaro.org, linaro-kernel@lists.linaro.org, STEricsson_nomadik_linux@list.st.com, kernel@igloocommunity.org, "hongbo.zhang" <hongbo.zhang@linaro.com> Subject: [PATCH V2 5/6] Thermal: Add ST-Ericsson DB8500 thermal dirver. Date: Wed, 24 Oct 2012 19:58:19 +0800 Message-Id: <1351079900-32236-6-git-send-email-hongbo.zhang@linaro.com> In-Reply-To: <1351079900-32236-1-git-send-email-hongbo.zhang@linaro.com> References: <1350387889-15324-1-git-send-email-hongbo.zhang@linaro.com> <1351079900-32236-1-git-send-email-hongbo.zhang@linaro.com> Sender: linux-pm-owner@vger.kernel.org Precedence: bulk

Hongbo Zhang Oct. 24, 2012, 11:58 a.m. UTC

From: "hongbo.zhang" <hongbo.zhang@linaro.com>

This diver is based on the thermal management framework in thermal_sys.c. A
thermal zone device is created with the trip points to which cooling devices
can be bound, the current cooling device is cpufreq, e.g. CPU frequency is
clipped down to cool the CPU, and other cooling devices can be added and bound
to the trip points dynamically.  The platform specific PRCMU interrupts are
used to active thermal update when trip points are reached.

Signed-off-by: hongbo.zhang <hongbo.zhang@linaro.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Francesco Lavra <francescolavra.fl@gmail.com>
---
 arch/arm/configs/u8500_defconfig             |   4 +
 drivers/thermal/Kconfig                      |  20 +
 drivers/thermal/Makefile                     |   2 +
 drivers/thermal/db8500_cpufreq_cooling.c     | 123 ++++++
 drivers/thermal/db8500_thermal.c             | 557 +++++++++++++++++++++++++++
 include/linux/platform_data/db8500_thermal.h |  39 ++
 6 files changed, 745 insertions(+)
 create mode 100644 drivers/thermal/db8500_cpufreq_cooling.c
 create mode 100644 drivers/thermal/db8500_thermal.c
 create mode 100644 include/linux/platform_data/db8500_thermal.h

Viresh Kumar Oct. 24, 2012, 2:38 p.m. UTC | #1

On 24 October 2012 17:28, hongbo.zhang <hongbo.zhang@linaro.org> wrote:
> From: "hongbo.zhang" <hongbo.zhang@linaro.com>
>
> This diver is based on the thermal management framework in thermal_sys.c. A
> thermal zone device is created with the trip points to which cooling devices
> can be bound, the current cooling device is cpufreq, e.g. CPU frequency is
> clipped down to cool the CPU, and other cooling devices can be added and bound
> to the trip points dynamically.  The platform specific PRCMU interrupts are
> used to active thermal update when trip points are reached.
>
> Signed-off-by: hongbo.zhang <hongbo.zhang@linaro.com>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Francesco Lavra <francescolavra.fl@gmail.com>

You can't add these lines, until somebody has replied you with them in
earlier mails.

They don't show that somebody has put effort in reviewing them, but that current
patch looks Ok to these guys.

> ---
>  arch/arm/configs/u8500_defconfig             |   4 +

This is considered as platform part. So it must be part of next patch.

>  drivers/thermal/Kconfig                      |  20 +
>  drivers/thermal/Makefile                     |   2 +
>  drivers/thermal/db8500_cpufreq_cooling.c     | 123 ++++++
>  drivers/thermal/db8500_thermal.c             | 557 +++++++++++++++++++++++++++
>  include/linux/platform_data/db8500_thermal.h |  39 ++
>  6 files changed, 745 insertions(+)
>  create mode 100644 drivers/thermal/db8500_cpufreq_cooling.c
>  create mode 100644 drivers/thermal/db8500_thermal.c
>  create mode 100644 include/linux/platform_data/db8500_thermal.h
>
> diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
> index cc5e7a8..34918c4 100644
> --- a/arch/arm/configs/u8500_defconfig
> +++ b/arch/arm/configs/u8500_defconfig
> @@ -118,3 +118,7 @@ CONFIG_DEBUG_KERNEL=y
>  CONFIG_DEBUG_INFO=y
>  # CONFIG_FTRACE is not set
>  CONFIG_DEBUG_USER=y
> +CONFIG_THERMAL=y
> +CONFIG_CPU_THERMAL=y
> +CONFIG_DB8500_THERMAL=y
> +CONFIG_DB8500_CPUFREQ_COOLING=y
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index e1cb6bd..54c8fd0 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -31,6 +31,26 @@ config CPU_THERMAL
>           and not the ACPI interface.
>           If you want this support, you should say Y here.
>
> +config DB8500_THERMAL
> +       bool "DB8500 thermal management"
> +       depends on THERMAL
> +       default y
> +       help
> +         Adds DB8500 thermal management implementation according to the thermal
> +         management framework. A thermal zone with several trip points will be
> +         created. Cooling devices can be bound to the trip points to cool this
> +         thermal zone if trip points reached.
> +
> +config DB8500_CPUFREQ_COOLING
> +       tristate "DB8500 cpufreq cooling"
> +       depends on CPU_THERMAL
> +       default y
> +       help
> +         Adds DB8500 cpufreq cooling devices, and these cooling devices can be
> +         bound to thermal zone trip points. When a trip point reached, the
> +         bound cpufreq cooling device turns active to set CPU frequency low to
> +         cool down the CPU.
> +
>  config SPEAR_THERMAL
>         bool "SPEAr thermal sensor driver"
>         depends on THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 885550d..c7a8dab 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -7,3 +7,5 @@ obj-$(CONFIG_CPU_THERMAL)               += cpu_cooling.o
>  obj-$(CONFIG_SPEAR_THERMAL)            += spear_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)     += rcar_thermal.o
>  obj-$(CONFIG_EXYNOS_THERMAL)           += exynos_thermal.o
> +obj-$(CONFIG_DB8500_THERMAL)           += db8500_thermal.o
> +obj-$(CONFIG_DB8500_CPUFREQ_COOLING)   += db8500_cpufreq_cooling.o
> diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
> new file mode 100644
> index 0000000..e4eddfd
> --- /dev/null
> +++ b/drivers/thermal/db8500_cpufreq_cooling.c
> @@ -0,0 +1,123 @@
> +/*
> + * db8500_cpufreq_cooling.c - db8500 cpufreq works as cooling device.
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang <hognbo.zhang@linaro.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *

remove extra blank line.

> + */
> +
> +#include <linux/cpu_cooling.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +static LIST_HEAD(db8500_cpufreq_cdev_list);
> +
> +struct db8500_cpufreq_cdev {
> +       struct thermal_cooling_device *cdev;
> +       struct list_head node;
> +};
> +
> +static int __devinit db8500_cpufreq_cooling_probe(struct platform_device *pdev)

As said earlier, don't use __devinit, exit...

> +{
> +       struct db8500_cpufreq_cdev *cooling_dev;
> +       struct cpumask mask_val;
> +
> +       cooling_dev = devm_kzalloc(&pdev->dev,
> +                       sizeof(*cooling_dev), GFP_KERNEL);

Align this too with 'gq'

> +       if (!cooling_dev)
> +               return -ENOMEM;
> +
> +       cpumask_set_cpu(0, &mask_val);
> +       cooling_dev->cdev = cpufreq_cooling_register(&mask_val);
> +
> +       if (IS_ERR_OR_NULL(cooling_dev->cdev)) {
> +               dev_err(&pdev->dev,
> +                       "Failed to register cpufreq cooling device\n");
> +               return PTR_ERR(cooling_dev->cdev);
> +       }
> +
> +       pdev->dev.platform_data = cooling_dev->cdev;

Use platform_set_drvdata() and platform_get_drvdata()

> +       list_add_tail(&cooling_dev->node, &db8500_cpufreq_cdev_list);
> +       dev_info(&pdev->dev, "Cooling device registered: %s\n",
> +               cooling_dev->cdev->type);
> +
> +       return 0;
> +}
> +
> +static int __devexit db8500_cpufreq_cooling_remove(struct platform_device *pdev)
> +{
> +       struct db8500_cpufreq_cdev *cooling_dev;
> +
> +       list_for_each_entry(cooling_dev, &db8500_cpufreq_cdev_list, node)
> +               if (cooling_dev->cdev == pdev->dev.platform_data) {

Use platform_get_drvdata()

> +                       cpufreq_cooling_unregister(cooling_dev->cdev);
> +                       list_del(&cooling_dev->node);
> +               }
> +
> +       return 0;
> +}
> +
> +static int db8500_cpufreq_cooling_suspend(struct platform_device *pdev,
> +               pm_message_t state)
> +{
> +       return -ENOSYS;
> +}
> +
> +static int db8500_cpufreq_cooling_resume(struct platform_device *pdev)
> +{
> +       return -ENOSYS;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id db8500_cpufreq_cooling_match[] = {
> +       { .compatible = "stericsson,db8500-cpufreq-cooling" },
> +       {},
> +};
> +#else
> +#define db8500_cpufreq_cooling_match NULL
> +#endif
> +
> +static struct platform_driver db8500_cpufreq_cooling_driver = {
> +       .driver = {
> +               .owner = THIS_MODULE,
> +               .name = "db8500-cpufreq-cooling",
> +               .of_match_table = db8500_cpufreq_cooling_match,
> +       },
> +       .probe = db8500_cpufreq_cooling_probe,
> +       .suspend = db8500_cpufreq_cooling_suspend,
> +       .resume = db8500_cpufreq_cooling_resume,
> +       .remove = __devexit_p(db8500_cpufreq_cooling_remove),
> +};
> +
> +static int __init db8500_cpufreq_cooling_init(void)
> +{
> +       return platform_driver_register(&db8500_cpufreq_cooling_driver);
> +}
> +
> +static void __exit db8500_cpufreq_cooling_exit(void)
> +{
> +       platform_driver_unregister(&db8500_cpufreq_cooling_driver);
> +}
> +
> +/* Should be later than db8500_cpufreq_register */
> +late_initcall(db8500_cpufreq_cooling_init);
> +module_exit(db8500_cpufreq_cooling_exit);
> +
> +MODULE_AUTHOR("Hongbo Zhang <hongbo.zhang@stericsson.com>");
> +MODULE_DESCRIPTION("DB8500 cpufreq cooling driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
> new file mode 100644
> index 0000000..52b814d
> --- /dev/null
> +++ b/drivers/thermal/db8500_thermal.c
> @@ -0,0 +1,557 @@
> +/*
> + * db8500_thermal.c - db8500 Thermal Management Implementation
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang <hognbo.zhang@linaro.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *

remove extra blank line.

> + */
> +
> +#include <linux/cpu_cooling.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/dbx500-prcmu.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_data/db8500_thermal.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +
> +#define PRCMU_DEFAULT_MEASURE_TIME     0xFFF
> +#define PRCMU_DEFAULT_LOW_TEMP         0
> +
> +struct db8500_thermal_zone {
> +       struct thermal_zone_device *therm_dev;
> +       struct mutex th_lock;
> +       struct work_struct therm_work;
> +       struct db8500_thsens_platform_data *trip_tab;
> +       enum thermal_device_mode mode;
> +       enum thermal_trend trend;
> +       unsigned long cur_temp_pseudo;
> +       unsigned int cur_index;
> +};
> +
> +/* Local function to check if thermal zone matches cooling devices */
> +static int db8500_thermal_match_cdev(struct thermal_cooling_device *cdev,
> +                       struct db8500_trip_point *trip_points)
> +{
> +       int i;
> +       char *cdev_name;
> +
> +       if (!strlen(cdev->type))
> +               return -EINVAL;
> +
> +       for (i = 0; i < COOLING_DEV_MAX; i++) {
> +               cdev_name = trip_points->cdev_name[i];
> +               if (!strlen(cdev_name))
> +                       continue;

Even if you don't have this strlen(), below strcmp will skip null strings.

> +               if (!strcmp(cdev_name, cdev->type))
> +                       return 0;
> +       }
> +
> +       return -ENODEV;
> +}
> +
> +/* Callback to bind cooling device to 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;
> +       unsigned long max_state, upper, lower;
> +       int i, ret = -EINVAL;
> +
> +       pzone = thermal->devdata;
> +       ptrips = pzone->trip_tab;

Do this with definition of these variables.

> +       for (i = 0; i < ptrips->num_trips; i++) {
> +               if (db8500_thermal_match_cdev(cdev, &ptrips->trip_points[i]))
> +                       continue;
> +
> +               cdev->ops->get_max_state(cdev, &max_state);
> +               lower = upper = (i > max_state) ? max_state : i;
> +
> +               ret = thermal_zone_bind_cooling_device(thermal, i,
> +                       cdev, upper, lower);
> +
> +               dev_info(&cdev->device, "%s bind to %d: %d-%s\n",
> +                       cdev->type, i, ret, ret ? "fail" : "succeed");
> +       }
> +
> +       return ret;
> +}
> +
> +/* Callback to unbind cooling device from thermal zone */
> +static int db8500_cdev_unbind(struct thermal_zone_device *thermal,
> +                         struct thermal_cooling_device *cdev)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       struct db8500_thsens_platform_data *ptrips;
> +       int i, ret = -EINVAL;
> +
> +       pzone = thermal->devdata;
> +       ptrips = pzone->trip_tab;

ditto

> +       for (i = 0; i < ptrips->num_trips; i++) {
> +               if (db8500_thermal_match_cdev(cdev, &ptrips->trip_points[i]))
> +                       continue;
> +
> +               ret = thermal_zone_unbind_cooling_device(thermal, i, cdev);
> +
> +               dev_info(&cdev->device, "%s unbind: %s\n",
> +                       cdev->type, ret ? "fail" : "succeed");
> +       }
> +
> +       return ret;
> +}
> +
> +/* Callback to get current temperature */
> +static int db8500_sys_get_temp(struct thermal_zone_device *thermal,
> +                       unsigned long *temp)
> +{
> +       struct db8500_thermal_zone *pzone = thermal->devdata;
> +
> +       /*
> +        * TODO: There is no PRCMU interface to get temperature data currently,
> +        * so a pseudo temperature is returned , it works for thermal framework
> +        * and this will be fixed when the PRCMU interface is available.
> +        */
> +       *temp = pzone->cur_temp_pseudo;
> +
> +       return 0;
> +}
> +
> +/* Callback to get temperature changing trend */
> +static int db8500_sys_get_trend(struct thermal_zone_device *thermal,
> +                       int trip, enum thermal_trend *trend)
> +{
> +       struct db8500_thermal_zone *pzone = thermal->devdata;
> +
> +       *trend = pzone->trend;
> +
> +       return 0;

Can make it return void.

> +}
> +
> +/* Callback to get thermal zone mode */
> +static int db8500_sys_get_mode(struct thermal_zone_device *thermal,
> +                       enum thermal_device_mode *mode)
> +{
> +       struct db8500_thermal_zone *pzone = thermal->devdata;
> +
> +       mutex_lock(&pzone->th_lock);
> +       *mode = pzone->mode;
> +       mutex_unlock(&pzone->th_lock);
> +
> +       return 0;
> +}
> +
> +/* Callback to set thermal zone mode */
> +static int db8500_sys_set_mode(struct thermal_zone_device *thermal,
> +                       enum thermal_device_mode mode)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       struct thermal_zone_device *pthdev;
> +
> +       pzone = thermal->devdata;
> +       pthdev = pzone->therm_dev;

ditto

> +       mutex_lock(&pzone->th_lock);
> +
> +       pzone->mode = mode;
> +       if (mode == THERMAL_DEVICE_ENABLED)
> +               schedule_work(&pzone->therm_work);
> +
> +       mutex_unlock(&pzone->th_lock);
> +
> +       return 0;
> +}
> +
> +/* Callback to get trip point type */
> +static int db8500_sys_get_trip_type(struct thermal_zone_device *thermal,
> +                       int trip, enum thermal_trip_type *type)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       struct db8500_thsens_platform_data *ptrips;
> +
> +       pzone = thermal->devdata;
> +       ptrips = pzone->trip_tab;

ditto

do it everywhere

> +       if (trip >= ptrips->num_trips)
> +               return -EINVAL;
> +
> +       *type = ptrips->trip_points[trip].type;
> +
> +       return 0;
> +}
> +
> +/* Callback to get trip point temperature */
> +static int db8500_sys_get_trip_temp(struct thermal_zone_device *thermal,
> +                       int trip, unsigned long *temp)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       struct db8500_thsens_platform_data *ptrips;
> +
> +       pzone = thermal->devdata;
> +       ptrips = pzone->trip_tab;
> +
> +       if (trip >= ptrips->num_trips)
> +               return -EINVAL;
> +
> +       *temp = ptrips->trip_points[trip].temp;
> +
> +       return 0;
> +}
> +
> +/* Callback to get critical trip point temperature */
> +static int db8500_sys_get_crit_temp(struct thermal_zone_device *thermal,
> +                       unsigned long *temp)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       struct db8500_thsens_platform_data *ptrips;
> +       int i;
> +
> +       pzone = thermal->devdata;
> +       ptrips = pzone->trip_tab;
> +
> +       for (i = (ptrips->num_trips - 1); i > 0; i--) {

don't need extra () here. It would be followed by a ";"

> +               if (ptrips->trip_points[i].type == THERMAL_TRIP_CRITICAL) {
> +                       *temp = ptrips->trip_points[i].temp;
> +                       return 0;
> +               }
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static struct thermal_zone_device_ops thdev_ops = {
> +       .bind = db8500_cdev_bind,
> +       .unbind = db8500_cdev_unbind,
> +       .get_temp = db8500_sys_get_temp,
> +       .get_trend = db8500_sys_get_trend,
> +       .get_mode = db8500_sys_get_mode,
> +       .set_mode = db8500_sys_set_mode,
> +       .get_trip_type = db8500_sys_get_trip_type,
> +       .get_trip_temp = db8500_sys_get_trip_temp,
> +       .get_crit_temp = db8500_sys_get_crit_temp,
> +};
> +
> +static void db8500_thermal_update_config(struct db8500_thermal_zone *pzone,
> +               unsigned int idx, enum thermal_trend trend,
> +               unsigned long next_low, unsigned long next_high)
> +{
> +       pzone->cur_index = idx;
> +       pzone->cur_temp_pseudo = (next_low + next_high)/2;
> +       pzone->trend = trend;
> +
> +       prcmu_stop_temp_sense();
> +       prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
> +       prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
> +}
> +
> +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;
> +       unsigned int idx;
> +
> +       ptrips = pzone->trip_tab;
> +       idx = pzone->cur_index;
> +       if (unlikely(idx == 0))
> +               /* Meaningless for thermal management, ignoring it */
> +               return IRQ_HANDLED;
> +
> +       if (idx == 1) {
> +               next_high = ptrips->trip_points[0].temp;
> +               next_low = PRCMU_DEFAULT_LOW_TEMP;
> +       } else {
> +               next_high = ptrips->trip_points[idx-1].temp;
> +               next_low = ptrips->trip_points[idx-2].temp;
> +       }
> +       idx -= 1;
> +
> +       db8500_thermal_update_config(pzone, idx, THERMAL_TREND_DROPPING,
> +                       next_low, next_high);
> +
> +       dev_dbg(&pzone->therm_dev->device,
> +               "PRCMU set max %ld, min %ld\n", next_high, next_low);
> +
> +       schedule_work(&pzone->therm_work);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t prcmu_high_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;
> +       unsigned int idx;
> +
> +       ptrips = pzone->trip_tab;
> +       idx = pzone->cur_index;
> +
> +       if (idx < ptrips->num_trips - 1) {
> +               next_high = ptrips->trip_points[idx+1].temp;
> +               next_low = ptrips->trip_points[idx].temp;
> +               idx += 1;
> +
> +               db8500_thermal_update_config(pzone, idx, THERMAL_TREND_RAISING,
> +                       next_low, next_high);
> +
> +               dev_dbg(&pzone->therm_dev->device,
> +               "PRCMU set max %ld, min %ld\n", next_high, next_low);
> +       }
> +
> +       else if (idx == ptrips->num_trips - 1)
> +               pzone->cur_temp_pseudo = ptrips->trip_points[idx].temp + 1;
> +
> +       schedule_work(&pzone->therm_work);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void db8500_thermal_work(struct work_struct *work)
> +{
> +       enum thermal_device_mode cur_mode;
> +       struct db8500_thermal_zone *pzone;
> +
> +       pzone = container_of(work, struct db8500_thermal_zone, therm_work);
> +
> +       mutex_lock(&pzone->th_lock);
> +       cur_mode = pzone->mode;
> +       mutex_unlock(&pzone->th_lock);
> +
> +       if (cur_mode == THERMAL_DEVICE_DISABLED)
> +               return;
> +
> +       thermal_zone_device_update(pzone->therm_dev);
> +       dev_dbg(&pzone->therm_dev->device, "thermal work finished.\n");
> +}
> +
> +#ifdef CONFIG_OF
> +static struct db8500_thsens_platform_data*
> +               db8500_thermal_parse_dt(struct platform_device *pdev)
> +{
> +       struct db8500_thsens_platform_data *ptrips;
> +       struct device_node *np = pdev->dev.of_node;
> +       char prop_name[32];
> +       const char *tmp_str;
> +       u32 tmp_data;
> +       int i, j;
> +
> +       if (!np) {
> +               dev_err(&pdev->dev, "Missing device tree data\n");
> +               return NULL;
> +       }
> +
> +       ptrips = devm_kzalloc(&pdev->dev, sizeof(*ptrips), GFP_KERNEL);
> +       if (!ptrips)
> +               return NULL;
> +
> +       if (of_property_read_u32(np, "num-trips", &tmp_data))
> +               goto err_parse_dt;
> +
> +       ptrips->num_trips = tmp_data;
> +
> +       for (i = 0; i < ptrips->num_trips; i++) {
> +               sprintf(prop_name, "trip%d-temp", i);
> +               if (of_property_read_u32(np, prop_name, &tmp_data))
> +                       goto err_parse_dt;
> +
> +               ptrips->trip_points[i].temp = tmp_data;
> +
> +               sprintf(prop_name, "trip%d-type", i);
> +               if (of_property_read_string(np, prop_name, &tmp_str))
> +                       goto err_parse_dt;
> +
> +               if (!strcmp(tmp_str, "active"))
> +                       ptrips->trip_points[i].type = THERMAL_TRIP_ACTIVE;
> +               else if (!strcmp(tmp_str, "passive"))
> +                       ptrips->trip_points[i].type = THERMAL_TRIP_PASSIVE;
> +               else if (!strcmp(tmp_str, "hot"))
> +                       ptrips->trip_points[i].type = THERMAL_TRIP_HOT;
> +               else if (!strcmp(tmp_str, "critical"))
> +                       ptrips->trip_points[i].type = THERMAL_TRIP_CRITICAL;
> +               else
> +                       goto err_parse_dt;
> +
> +               sprintf(prop_name, "trip%d-cdev-num", i);
> +               if (of_property_read_u32(np, prop_name, &tmp_data))
> +                       goto err_parse_dt;
> +
> +               for (j = 0; j < tmp_data; j++) {
> +                       sprintf(prop_name, "trip%d-cdev-name%d", i, j);
> +                       if (of_property_read_string(np, prop_name, &tmp_str))
> +                               goto err_parse_dt;
> +                       strcpy(ptrips->trip_points[i].cdev_name[j], tmp_str);
> +               }
> +       }
> +       return ptrips;
> +
> +err_parse_dt:
> +       dev_err(&pdev->dev, "Parsing device tree data error.\n");
> +       return NULL;
> +}
> +#else
> +static struct db8500_thsens_platform_data*
> +               db8500_thermal_parse_dt(struct platform_device *pdev)

mark it inline here.

> +{
> +       return NULL;
> +}
> +#endif
> +
> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
> +{
> +       struct db8500_thermal_zone *pzone = NULL;
> +       struct db8500_thsens_platform_data *ptrips = NULL;
> +       int low_irq, high_irq, ret = 0;
> +       unsigned long dft_low, dft_high;
> +
> +       pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
> +       if (!pzone)
> +               return -ENOMEM;
> +
> +       ptrips = db8500_thermal_parse_dt(pdev);

This is what u have in this routine at the very first line:

       if (!np) {
               dev_err(&pdev->dev, "Missing device tree data\n");

So, you will end up printing this line for every non-DT case. Not good.
What u can do is, give preference to normal pdata here.

> +       if (!ptrips)
> +               ptrips = dev_get_platdata(&pdev->dev);
> +
> +       if (!ptrips)

In case we have a DT case, you will run this if statement twice :(

> +               return -EINVAL;
> +

move pzone = devm_kzalloc, here after verifying pdata is there. so that
you don't allocate things for error cases.

> +       mutex_init(&pzone->th_lock);
> +       mutex_lock(&pzone->th_lock);
> +
> +       pzone->mode = THERMAL_DEVICE_DISABLED;
> +       pzone->trip_tab = ptrips;
> +
> +       pzone->therm_dev = thermal_zone_device_register("db8500_thermal_zone",
> +               ptrips->num_trips, 0, pzone, &thdev_ops, 0, 0);
> +
> +       if (IS_ERR_OR_NULL(pzone->therm_dev)) {
> +               dev_err(&pdev->dev, "Register thermal zone device failed.\n");
> +               return PTR_ERR(pzone->therm_dev);
> +       }
> +       dev_info(&pdev->dev, "Thermal zone device registered.\n");
> +
> +       dft_low = PRCMU_DEFAULT_LOW_TEMP;
> +       dft_high = ptrips->trip_points[0].temp;
> +
> +       db8500_thermal_update_config(pzone, 0, THERMAL_TREND_STABLE,
> +                       dft_low, dft_high);
> +
> +       INIT_WORK(&pzone->therm_work, db8500_thermal_work);
> +
> +       low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");
> +       if (low_irq < 0) {
> +               dev_err(&pdev->dev, "Get IRQ_HOTMON_LOW failed.\n");
> +               return low_irq;

Don't you want to do thermal_zone_device_unregister here? Please check
the sequence of stuff in this routine.

> +       }
> +
> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,

why threaded irq?

> +               prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> +               "dbx500_temp_low", pzone);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "Failed to allocate temp low irq.\n");
> +               return ret;
> +       }
> +
> +       high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
> +       if (high_irq < 0) {
> +               dev_err(&pdev->dev, "Get IRQ_HOTMON_HIGH failed.\n");
> +               return high_irq;

don't want to free resources?

> +       }
> +
> +       ret = devm_request_threaded_irq(&pdev->dev, high_irq, NULL,
> +               prcmu_high_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> +               "dbx500_temp_high", pzone);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "Failed to allocate temp high irq.\n");
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, pzone);
> +       pzone->mode = THERMAL_DEVICE_ENABLED;
> +       mutex_unlock(&pzone->th_lock);
> +
> +       return 0;
> +}
> +
> +static int __devexit db8500_thermal_remove(struct platform_device *pdev)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       pzone = platform_get_drvdata(pdev);
> +
> +       cancel_work_sync(&pzone->therm_work);
> +       mutex_destroy(&pzone->th_lock);
> +       thermal_zone_device_unregister(pzone->therm_dev);

The first thing you must do here is unregister... Then cancel work and mutex
destroy. It has to be opposite of probe.

> +       return 0;
> +}
> +
> +static int db8500_thermal_suspend(struct platform_device *pdev,
> +               pm_message_t state)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       pzone = platform_get_drvdata(pdev);
> +
> +       flush_work_sync(&pzone->therm_work);
> +       prcmu_stop_temp_sense();
> +
> +       return 0;
> +}
> +
> +static int db8500_thermal_resume(struct platform_device *pdev)
> +{
> +       struct db8500_thermal_zone *pzone;
> +       struct db8500_thsens_platform_data *ptrips;
> +       unsigned long dft_low, dft_high;
> +
> +       pzone = platform_get_drvdata(pdev);
> +       ptrips = pzone->trip_tab;
> +       dft_low = PRCMU_DEFAULT_LOW_TEMP;
> +       dft_high = ptrips->trip_points[0].temp;
> +
> +       db8500_thermal_update_config(pzone, 0, THERMAL_TREND_STABLE,
> +                               dft_low, dft_high);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id db8500_thermal_match[] = {
> +       { .compatible = "stericsson,db8500-thermal" },
> +       {},
> +};
> +#else
> +#define db8500_thermal_match NULL
> +#endif
> +
> +static struct platform_driver db8500_thermal_driver = {
> +       .driver = {
> +               .owner = THIS_MODULE,
> +               .name = "db8500-thermal",
> +               .of_match_table = db8500_thermal_match,
> +       },
> +       .probe = db8500_thermal_probe,
> +       .suspend = db8500_thermal_suspend,
> +       .resume = db8500_thermal_resume,
> +       .remove = __devexit_p(db8500_thermal_remove),
> +};
> +
> +module_platform_driver(db8500_thermal_driver);
> +
> +MODULE_AUTHOR("Hongbo Zhang <hongbo.zhang@stericsson.com>");
> +MODULE_DESCRIPTION("DB8500 thermal driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/db8500_thermal.h b/include/linux/platform_data/db8500_thermal.h
> new file mode 100644
> index 0000000..8825cd9
> --- /dev/null
> +++ b/include/linux/platform_data/db8500_thermal.h
> @@ -0,0 +1,39 @@
> +/*
> + * db8500_thermal.h - db8500 Thermal Management Implementation
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang <hognbo.zhang@linaro.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *

:(

> + */
> +
> +#ifndef _DB8500_THERMAL_H_
> +#define _DB8500_THERMAL_H_
> +
> +#include <linux/thermal.h>
> +
> +#define COOLING_DEV_MAX 8
> +
> +struct db8500_trip_point {
> +       unsigned long temp;
> +       enum thermal_trip_type type;
> +       char cdev_name[COOLING_DEV_MAX][THERMAL_NAME_LENGTH];
> +};
> +
> +struct db8500_thsens_platform_data {
> +       struct db8500_trip_point trip_points[THERMAL_MAX_TRIPS];
> +       int num_trips;
> +};
> +
> +#endif /* _DB8500_THERMAL_H_ */
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hongbo Zhang Oct. 25, 2012, 8:26 a.m. UTC | #2

[...]
>> +/* Callback to get temperature changing trend */
>> +static int db8500_sys_get_trend(struct thermal_zone_device *thermal,
>> +                       int trip, enum thermal_trend *trend)
>> +{
>> +       struct db8500_thermal_zone *pzone = thermal->devdata;
>> +
>> +       *trend = pzone->trend;
>> +
>> +       return 0;
>
> Can make it return void.
No, it is callback of thermal layer, prototype it to return int.

[...]
>> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
>> +{
>> +       struct db8500_thermal_zone *pzone = NULL;
>> +       struct db8500_thsens_platform_data *ptrips = NULL;
>> +       int low_irq, high_irq, ret = 0;
>> +       unsigned long dft_low, dft_high;
>> +
>> +       pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
>> +       if (!pzone)
>> +               return -ENOMEM;
>> +
>> +       ptrips = db8500_thermal_parse_dt(pdev);
>
> This is what u have in this routine at the very first line:
>
>        if (!np) {
>                dev_err(&pdev->dev, "Missing device tree data\n");
>
> So, you will end up printing this line for every non-DT case. Not good.
> What u can do is, give preference to normal pdata here.
I moved this if(!np) into parse_dt function, no problem again.
(in fact have already done this, but it is missed in this sending)
>
>> +       if (!ptrips)
>> +               ptrips = dev_get_platdata(&pdev->dev);
>> +
>> +       if (!ptrips)
>
[...]
>> +       }
>> +
>> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>
> why threaded irq?
In fact PRCMU firmware is polling the thermal sensor, and if it meets
threshold, the PRCMU will write this event into share memory (shared
between PRCMU and ARM) and trigger an interrupt to ARM.
There may be other events passed via share memory, so it is better to
handle this kind of irq as fast as possible(it is always the policy),
and threaded irq satisfies this case better then the traditional one.
>
>> +               prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>> +               "dbx500_temp_low", pzone);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "Failed to allocate temp low irq.\n");
>> +               return ret;
>> +       }
>> +
>> +       high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
>> +       if (high_irq < 0) {
>> +               dev_err(&pdev->dev, "Get IRQ_HOTMON_HIGH failed.\n");
>> +               return high_irq;
>
> don't want to free resources?
devm_* is used to allocate resources, so no need to free them manually.
>
>> +       }
>> +
>> +       ret = devm_request_threaded_irq(&pdev->dev, high_irq, NULL,
>> +               prcmu_high_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>> +               "dbx500_temp_high", pzone);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "Failed to allocate temp high irq.\n");
>> +               return ret;
>> +       }
>> +
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar Oct. 25, 2012, 8:41 a.m. UTC | #3

On 25 October 2012 13:56, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

While replying to mails, don't remove lines like above. They help
identifying who
wrote what.

> [...]
>>> +/* Callback to get temperature changing trend */
>>> +static int db8500_sys_get_trend(struct thermal_zone_device *thermal,

For example, you can't tell who wrote this line...

>>> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
>>> +{
>>> +       struct db8500_thermal_zone *pzone = NULL;
>>> +       struct db8500_thsens_platform_data *ptrips = NULL;
>>> +       int low_irq, high_irq, ret = 0;
>>> +       unsigned long dft_low, dft_high;
>>> +
>>> +       pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
>>> +       if (!pzone)
>>> +               return -ENOMEM;
>>> +
>>> +       ptrips = db8500_thermal_parse_dt(pdev);
>>
>> This is what u have in this routine at the very first line:
>>
>>        if (!np) {
>>                dev_err(&pdev->dev, "Missing device tree data\n");
>>
>> So, you will end up printing this line for every non-DT case. Not good.
>> What u can do is, give preference to normal pdata here.
> I moved this if(!np) into parse_dt function, no problem again.
> (in fact have already done this, but it is missed in this sending)

Sorry couldn't get your point. :(
Can you share diff of latest code in the same mail thread?

>>> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>>
>> why threaded irq?

> In fact PRCMU firmware is polling the thermal sensor, and if it meets
> threshold, the PRCMU will write this event into share memory (shared
> between PRCMU and ARM) and trigger an interrupt to ARM.
>
> There may be other events passed via share memory, so it is better to
> handle this kind of irq as fast as possible(it is always the policy),
> and threaded irq satisfies this case better then the traditional one.

Its been long that i prepared for an interview, but i believe purpose
of threaded
irq is something else.

There can be two use cases of request_irq()
- We don't want to sleep from interrupt handler, because we don't need to sleep
  for reading hardware's register. And so handler must be called from interrupt
  context. We use normal request_irq() here. This is the fastest one.

- We have to sleep from some part of interrupt handler, because we don't have
  peripherals register on AMBA bus. But we have it on SPI or I2C bus,
where read/
  write to SPI/I2C can potentially sleep. So, we want handler to execute from
  process context and so use request_threaded_irq(), i.e. handler will
be called
  from a thread. This will surely be slow.

  Now in threaded irq case, we can give two handlers, one that must be called
  from interrupt context and other that must be called from process context.
  Both will be called one by one.

Sorry if i am wrong in my theory :(
@Amit: Am i correct??

Now, the same question again. Are you sure you want threaded irq here.

>>> +               prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>>> +               "dbx500_temp_low", pzone);
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hongbo Zhang Oct. 25, 2012, 9:33 a.m. UTC | #4

On 25 October 2012 16:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25 October 2012 13:56, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>
> While replying to mails, don't remove lines like above. They help
> identifying who
> wrote what.
>
>> [...]
>>>> +/* Callback to get temperature changing trend */
>>>> +static int db8500_sys_get_trend(struct thermal_zone_device *thermal,
>
> For example, you can't tell who wrote this line...
>
>>>> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct db8500_thermal_zone *pzone = NULL;
>>>> +       struct db8500_thsens_platform_data *ptrips = NULL;
>>>> +       int low_irq, high_irq, ret = 0;
>>>> +       unsigned long dft_low, dft_high;
>>>> +
>>>> +       pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
>>>> +       if (!pzone)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       ptrips = db8500_thermal_parse_dt(pdev);
>>>
>>> This is what u have in this routine at the very first line:
>>>
>>>        if (!np) {
>>>                dev_err(&pdev->dev, "Missing device tree data\n");
>>>
>>> So, you will end up printing this line for every non-DT case. Not good.
>>> What u can do is, give preference to normal pdata here.
>> I moved this if(!np) into parse_dt function, no problem again.
>> (in fact have already done this, but it is missed in this sending)
>
> Sorry couldn't get your point. :(
> Can you share diff of latest code in the same mail thread?
Just paste my current pieces of codes here:

static struct db8500_thsens_platform_data*
		db8500_thermal_parse_dt(struct platform_device *pdev)
{
	struct db8500_thsens_platform_data *ptrips;
	struct device_node *np = pdev->dev.of_node;
	char prop_name[32];
	const char *tmp_str;
	u32 tmp_data;
	int i, j;

	if (!np) {
		dev_err(&pdev->dev, "Missing device tree data\n");
		return NULL;
	}
	......
}

static int db8500_thermal_probe(struct platform_device *pdev)
{
	struct db8500_thermal_zone *pzone = NULL;
	struct db8500_thsens_platform_data *ptrips = NULL;
	int low_irq, high_irq, ret = 0;
	unsigned long dft_low, dft_high;

	ptrips = db8500_thermal_parse_dt(pdev);
	if (!ptrips)
		ptrips = dev_get_platdata(&pdev->dev);

	if (!ptrips)
		return -EINVAL;

	pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
	if (!pzone)
		return -ENOMEM;
	......
}

>
>>>> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>>>
>>> why threaded irq?
>
>> In fact PRCMU firmware is polling the thermal sensor, and if it meets
>> threshold, the PRCMU will write this event into share memory (shared
>> between PRCMU and ARM) and trigger an interrupt to ARM.
>>
>> There may be other events passed via share memory, so it is better to
>> handle this kind of irq as fast as possible(it is always the policy),
>> and threaded irq satisfies this case better then the traditional one.
>
> Its been long that i prepared for an interview, but i believe purpose
> of threaded
> irq is something else.
>
> There can be two use cases of request_irq()
> - We don't want to sleep from interrupt handler, because we don't need to sleep
>   for reading hardware's register. And so handler must be called from interrupt
>   context. We use normal request_irq() here. This is the fastest one.
>
> - We have to sleep from some part of interrupt handler, because we don't have
>   peripherals register on AMBA bus. But we have it on SPI or I2C bus,
> where read/
>   write to SPI/I2C can potentially sleep. So, we want handler to execute from
>   process context and so use request_threaded_irq(), i.e. handler will
> be called
>   from a thread. This will surely be slow.
>
>   Now in threaded irq case, we can give two handlers, one that must be called
>   from interrupt context and other that must be called from process context.
>   Both will be called one by one.
>
Understand your points.

> Sorry if i am wrong in my theory :(
> @Amit: Am i correct??
>
> Now, the same question again. Are you sure you want threaded irq here.
I just saw that all the PRCMU and ab8500 related irqs use request_threaded_irq
only difference is that I use devm_request_threaded_irq

>
>>>> +               prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>>>> +               "dbx500_temp_low", pzone);
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar Oct. 25, 2012, 9:42 a.m. UTC | #5

On 25 October 2012 15:03, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
> On 25 October 2012 16:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> Just paste my current pieces of codes here:
>
> static struct db8500_thsens_platform_data*
>                 db8500_thermal_parse_dt(struct platform_device *pdev)
> {

>         if (!np) {
>                 dev_err(&pdev->dev, "Missing device tree data\n");

> }
>
> static int db8500_thermal_probe(struct platform_device *pdev)
> {

>         ptrips = db8500_thermal_parse_dt(pdev);
>         if (!ptrips)
>                 ptrips = dev_get_platdata(&pdev->dev);

So, the above code still has the flaw i pointed out. It will print
"Missing device tree data", while booting for non-DT case.

What i would suggest you is:

static int db8500_thermal_probe(struct platform_device *pdev)
{
        struct device_node *np = pdev->dev.of_node;

        if (np)
                ptrips = db8500_thermal_parse_dt(pdev);
        else
                ptrips = dev_get_platdata(&pdev->dev);

       if (!ptrips)
              explode!!


>>>>> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,

> I just saw that all the PRCMU and ab8500 related irqs use request_threaded_irq
> only difference is that I use devm_request_threaded_irq

See, i started this threaded_irq thread is to make sure you know
exactly what you
are doing. Others are doing it doesn't mean you should do it too.. :)

You must dig in a bit to see why is it required for your case? If
earlier code related
to PRCMU and db8500 is correct, then i am sure you need to sleep from your
handler.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hongbo Zhang Oct. 25, 2012, 9:56 a.m. UTC | #6

On 25 October 2012 17:33, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
> On 25 October 2012 16:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 25 October 2012 13:56, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>>
>> While replying to mails, don't remove lines like above. They help
>> identifying who
>> wrote what.
>>
>>> [...]
>>>>> +/* Callback to get temperature changing trend */
>>>>> +static int db8500_sys_get_trend(struct thermal_zone_device *thermal,
>>
>> For example, you can't tell who wrote this line...
>>
>>>>> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +       struct db8500_thermal_zone *pzone = NULL;
>>>>> +       struct db8500_thsens_platform_data *ptrips = NULL;
>>>>> +       int low_irq, high_irq, ret = 0;
>>>>> +       unsigned long dft_low, dft_high;
>>>>> +
>>>>> +       pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
>>>>> +       if (!pzone)
>>>>> +               return -ENOMEM;
>>>>> +
>>>>> +       ptrips = db8500_thermal_parse_dt(pdev);
>>>>
>>>> This is what u have in this routine at the very first line:
>>>>
>>>>        if (!np) {
>>>>                dev_err(&pdev->dev, "Missing device tree data\n");
>>>>
>>>> So, you will end up printing this line for every non-DT case. Not good.
>>>> What u can do is, give preference to normal pdata here.
>>> I moved this if(!np) into parse_dt function, no problem again.
>>> (in fact have already done this, but it is missed in this sending)
>>
>> Sorry couldn't get your point. :(
>> Can you share diff of latest code in the same mail thread?
> Just paste my current pieces of codes here:
>
> static struct db8500_thsens_platform_data*
>                 db8500_thermal_parse_dt(struct platform_device *pdev)
> {
>         struct db8500_thsens_platform_data *ptrips;
>         struct device_node *np = pdev->dev.of_node;
>         char prop_name[32];
>         const char *tmp_str;
>         u32 tmp_data;
>         int i, j;
>
>         if (!np) {
>                 dev_err(&pdev->dev, "Missing device tree data\n");
>                 return NULL;
>         }
>         ......
> }
>
> static int db8500_thermal_probe(struct platform_device *pdev)
> {
>         struct db8500_thermal_zone *pzone = NULL;
>         struct db8500_thsens_platform_data *ptrips = NULL;
>         int low_irq, high_irq, ret = 0;
>         unsigned long dft_low, dft_high;
>
>         ptrips = db8500_thermal_parse_dt(pdev);
>         if (!ptrips)
>                 ptrips = dev_get_platdata(&pdev->dev);
>
>         if (!ptrips)
>                 return -EINVAL;
>
>         pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
>         if (!pzone)
>                 return -ENOMEM;
>         ......
> }
>
>>
>>>>> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>>>>
>>>> why threaded irq?
>>
>>> In fact PRCMU firmware is polling the thermal sensor, and if it meets
>>> threshold, the PRCMU will write this event into share memory (shared
>>> between PRCMU and ARM) and trigger an interrupt to ARM.
>>>
>>> There may be other events passed via share memory, so it is better to
>>> handle this kind of irq as fast as possible(it is always the policy),
>>> and threaded irq satisfies this case better then the traditional one.
>>
>> Its been long that i prepared for an interview, but i believe purpose
>> of threaded
>> irq is something else.
>>
>> There can be two use cases of request_irq()
>> - We don't want to sleep from interrupt handler, because we don't need to sleep
>>   for reading hardware's register. And so handler must be called from interrupt
>>   context. We use normal request_irq() here. This is the fastest one.
>>
>> - We have to sleep from some part of interrupt handler, because we don't have
>>   peripherals register on AMBA bus. But we have it on SPI or I2C bus,
>> where read/
>>   write to SPI/I2C can potentially sleep. So, we want handler to execute from
>>   process context and so use request_threaded_irq(), i.e. handler will
>> be called
>>   from a thread. This will surely be slow.
>>
>>   Now in threaded irq case, we can give two handlers, one that must be called
>>   from interrupt context and other that must be called from process context.
>>   Both will be called one by one.
>>
> Understand your points.
Verish, see the codes in include/linux/interrupt.h:
static inline int __must_check
devm_request_irq(struct device *dev, unsigned int irq, irq_handler_t handler,
		 unsigned long irqflags, const char *devname, void *dev_id)
{
	return devm_request_threaded_irq(dev, irq, handler, NULL, irqflags,
					 devname, dev_id);
}
devm_request_irq is devm_request_threaded_irq

>
>> Sorry if i am wrong in my theory :(
>> @Amit: Am i correct??
>>
>> Now, the same question again. Are you sure you want threaded irq here.
> I just saw that all the PRCMU and ab8500 related irqs use request_threaded_irq
> only difference is that I use devm_request_threaded_irq
>
>>
>>>>> +               prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>>>>> +               "dbx500_temp_low", pzone);
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar Oct. 25, 2012, 10:04 a.m. UTC | #7

On 25 October 2012 15:26, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

> Verish, see the codes in include/linux/interrupt.h:

s/verish/viresh :)

> static inline int __must_check
> devm_request_irq(struct device *dev, unsigned int irq, irq_handler_t handler,
>                  unsigned long irqflags, const char *devname, void *dev_id)
> {
>         return devm_request_threaded_irq(dev, irq, handler, NULL, irqflags,
>                                          devname, dev_id);
> }
> devm_request_irq is devm_request_threaded_irq

See carefully what's happening here.

All interrupt types have a common irq_desc type in kernel. This has
few pointers for every
interrupt line:
- List of handlers to call from interrupt context
- handlers to call from process context via a thread.

So, the internal implementation is exactly same... The only difference
is which pointer
should be called in.

the devm_request_threaded_irq() called from devm_request_irq() has
following params
handler: For irq to be called from interrupt context (param 3)
NULL: For irq to be called from process context. (param 4)

So, that means  normal request_irq type only.

In your case, you have passed interrupt context pointer as NULL and
process context
one as handler. So that's an issue.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar Oct. 25, 2012, 10:11 a.m. UTC | #8

On 25 October 2012 15:34, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25 October 2012 15:26, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

This is what your prcmu driver's routines are doing:

int db8500_prcmu_config_hotmon(u8 low, u8 high)
{
...
	wait_for_completion(&mb4_transfer.work);
...
	return 0;
}

This is why others in STE have used threaded_irqs... Because the
routine you guys call from interrupt handlers actually sleeps.

So, they can't be called from interrupt context.

I wanted you to knew this :)

Its okay now, you need to use threaded irq only and you can't use
normal request_irq(). Its not that you want to make things fast that's why
you used threaded irqs... If you try to sleep from interrupt context
(i.e. if you
have registered your handler with request_irq()), you will see a kernel crash.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hongbo Zhang Oct. 25, 2012, 10:43 a.m. UTC | #9

On 25 October 2012 17:42, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25 October 2012 15:03, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>> On 25 October 2012 16:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> Just paste my current pieces of codes here:
>>
>> static struct db8500_thsens_platform_data*
>>                 db8500_thermal_parse_dt(struct platform_device *pdev)
>> {
>
>>         if (!np) {
>>                 dev_err(&pdev->dev, "Missing device tree data\n");
>
>> }
>>
>> static int db8500_thermal_probe(struct platform_device *pdev)
>> {
>
>>         ptrips = db8500_thermal_parse_dt(pdev);
>>         if (!ptrips)
>>                 ptrips = dev_get_platdata(&pdev->dev);
>
> So, the above code still has the flaw i pointed out. It will print
> "Missing device tree data", while booting for non-DT case.
>
> What i would suggest you is:
>
> static int db8500_thermal_probe(struct platform_device *pdev)
> {
>         struct device_node *np = pdev->dev.of_node;
>
>         if (np)
>                 ptrips = db8500_thermal_parse_dt(pdev);
>         else
>                 ptrips = dev_get_platdata(&pdev->dev);
>
>        if (!ptrips)
>               explode!!
>
This seems neat.
>
>>>>>> +       ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>
>> I just saw that all the PRCMU and ab8500 related irqs use request_threaded_irq
>> only difference is that I use devm_request_threaded_irq
>
> See, i started this threaded_irq thread is to make sure you know
> exactly what you
> are doing. Others are doing it doesn't mean you should do it too.. :)
>
> You must dig in a bit to see why is it required for your case? If
> earlier code related
> to PRCMU and db8500 is correct, then i am sure you need to sleep from your
> handler.
>
> --
> viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hongbo Zhang Oct. 25, 2012, 10:45 a.m. UTC | #10

On 25 October 2012 18:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25 October 2012 15:34, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 25 October 2012 15:26, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>
> This is what your prcmu driver's routines are doing:
>
> int db8500_prcmu_config_hotmon(u8 low, u8 high)
> {
> ...
>         wait_for_completion(&mb4_transfer.work);
> ...
>         return 0;
> }
>
> This is why others in STE have used threaded_irqs... Because the
> routine you guys call from interrupt handlers actually sleeps.
>
I should find this for you, but you find it for me :(
Thanks a lot.
> So, they can't be called from interrupt context.
>
> I wanted you to knew this :)
>
> Its okay now, you need to use threaded irq only and you can't use
> normal request_irq(). Its not that you want to make things fast that's why
> you used threaded irqs... If you try to sleep from interrupt context
> (i.e. if you
> have registered your handler with request_irq()), you will see a kernel crash.
>
> --
> viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[V2,5/6] Thermal: Add ST-Ericsson DB8500 thermal dirver.

Commit Message

Comments

Patch