Message-ID: <508595A0.4040604@gmail.com>
Date: Mon, 22 Oct 2012 20:51:12 +0200
From: Francesco Lavra <francescolavra.fl@gmail.com>
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20110929 Thunderbird/7.0.1
MIME-Version: 1.0
To: Hongbo Zhang <hongbo.zhang@linaro.org>
CC: linaro-dev@lists.linaro.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, STEricsson_nomadik_linux@list.st.com,
        kernel@igloocommunity.org, linaro-kernel@lists.linaro.org,
        "hongbo.zhang" <hongbo.zhang@linaro.com>, patches@linaro.org
Subject: Re: [PATCH 5/5] Thermal: Add ST-Ericsson db8500 thermal dirver.
References: <1350387889-15324-1-git-send-email-hongbo.zhang@linaro.com> <1350387889-15324-6-git-send-email-hongbo.zhang@linaro.com> <50840E3D.1060204@gmail.com> <CAJLyvQxC8eNRmB40o18JR0k7f38+=fpt4UimSYxDCNzNH03Cfw@mail.gmail.com>
In-Reply-To: <CAJLyvQxC8eNRmB40o18JR0k7f38+=fpt4UimSYxDCNzNH03Cfw@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9521
Lines: 270

On 10/22/2012 02:02 PM, Hongbo Zhang wrote:
[...]
>>> +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;
>>> +     }
>>> +
>>> +     pzone->cur_index -= 1;
>>> +     pzone->cur_temp_pseudo = (next_high + next_low)/2;
>>> +
>>> +     prcmu_stop_temp_sense();
>>> +     prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
>>> +     prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>> +
>>> +     pr_debug("PRCMU set max %ld, set min %ld\n", next_high, next_low);
>>> +
>>> +     pzone->trend = THERMAL_TREND_DROPPING;
>>> +     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;
>>> +
>>> +             pzone->cur_index += 1;
>>> +             pzone->cur_temp_pseudo = (next_high + next_low)/2;
>>> +
>>> +             prcmu_stop_temp_sense();
>>> +             prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
>>> +             prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>> +
>>> +             pr_debug("PRCMU set max %ld, min %ld\n", next_high, next_low);
>>> +     }
>>> +
>>> +     if (idx == ptrips->num_trips - 1)
>>
>> } else if ()
> There is no else condition here, because it it the highest critical
> trip point, system will be shut down in thermal_work.
> But I'd like to add some comments here to state this situation.

I didn't mean adding a new else condition, what I meant is that if the
first condition (idx < ptrips->num_trips - 1) is verified, then there is
no need to check for the second condition (idx == ptrips->num_trips -
1). So I was thinking of changing the code to:

if (idx < ptrips->num_trips - 1)
	...
else if (idx == ptrips->num_trips - 1)
	...

Sorry if I wasn't clear.

>>
>>> +             pzone->cur_temp_pseudo = ptrips->trip_points[idx].temp + 1;
>>> +
>>> +     pzone->trend = THERMAL_TREND_RAISING;
>>> +     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) {
>>> +             pr_warn("Warning: thermal function disabled.\n");
>>> +             return;
>>> +     }
>>> +
>>> +     thermal_zone_device_update(pzone->therm_dev);
>>> +     pr_debug("db8500_thermal_work finished.\n");
>>> +}
>>> +
>>> +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;
>>> +
>>> +     pzone = devm_kzalloc(&pdev->dev,
>>> +                     sizeof(struct db8500_thermal_zone), GFP_KERNEL);
>>> +     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");
>>> +             return low_irq;
>>> +     }
>>> +
>>> +     ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
>>> +             prcmu_low_irq_handler,
>>> +             IRQF_NO_SUSPEND | IRQF_ONESHOT, "dbx500_temp_low", pzone);
>>> +     if (ret < 0) {
>>> +             pr_err("Failed to allocate temp low irq.\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
>>> +     if (high_irq < 0) {
>>> +             pr_err("Get IRQ_HOTMON_HIGH failed.\n");
>>> +             return high_irq;
>>> +     }
>>> +
>>> +     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) {
>>> +             pr_err("Failed to allocate temp high irq.\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     pzone->low_irq = low_irq;
>>> +     pzone->high_irq = high_irq;
>>> +
>>> +     pzone->mode = THERMAL_DEVICE_DISABLED;
>>> +
>>> +     mutex_init(&pzone->th_lock);
>>> +
>>> +     INIT_WORK(&pzone->therm_work, db8500_thermal_work);
>>> +
>>> +     ptrips = pdev->dev.platform_data;
>>> +     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(pzone->therm_dev)) {
>>> +             pr_err("Failed to register thermal zone device\n");
>>> +             return PTR_ERR(pzone->therm_dev);
>>> +     }
>>> +
>>> +     dft_low = PRCMU_DEFAULT_LOW_TEMP;
>>> +     dft_high = ptrips->trip_points[0].temp;
>>> +
>>> +     prcmu_stop_temp_sense();
>>> +     prcmu_config_hotmon((u8)(dft_low/1000), (u8)(dft_high/1000));
>>> +     prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>> +
>>> +     pzone->cur_index = 0;
>>> +     pzone->cur_temp_pseudo = (dft_low + dft_high)/2;
>>> +     pzone->trend = THERMAL_TREND_STABLE;
>>
>> All the stuff from prcmu_stop_temp_sense() up to this line can race with
>> the irq handlers, I would suggest doing it before requesting the irqs.
>>
>>> +     pzone->mode = THERMAL_DEVICE_ENABLED;
>>
>> Shouldn't this be protected by pzone->th_lock? Otherwise it should be
>> set before the thermal zone is registered.
>>
>>> +
>>> +     platform_set_drvdata(pdev, pzone);
>>> +
>>> +     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);
>>> +
>>> +     if (pzone->therm_dev)
>>> +             thermal_zone_device_unregister(pzone->therm_dev);
>>> +
>>> +     return 0;
>>> +}
>>
>> mutex_destroy() should be called on pzone->th_lock
>>
>>> +
>>> +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;
>>> +
>>> +     prcmu_config_hotmon((u8)(dft_low/1000), (u8)(dft_high/1000));
>>> +     prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
>>
>> Shouldn't cur_index and cur_temp_pseudo be updated as well? You may want
>> to define a helper function with all the code shared by irq handlers
>> (both high and low), probe and resume.
> No, they cannot be update because we don't know the actual current
> temp[1] after short or long time suspend, everything goes as
> beginning.

That's what I wanted to say, if everything is reset to a default value,
then cur_index and cur_temp should be reset too, as it's done in the
probe function. Otherwise you may have a current pseudo-temp and a
current index unrelated to how the hotmon is configured.

> If a helper function is introduced, it can be only used in probe and
> resume I think, different and a bit complicated algorithm in irq
> handlers.

I was thinking about a function which takes the new index and the new
low and high parameters, and updates cur_index and cur_pseudo_temp and
does the prcmu stuff. It seems to me this is (or should be) common to
all the 4 functions.

> [1] due to lack of corresponding interface, search "TODO" in this file
> to get more explanation.

--
Francesco
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/