Date:	Thu, 25 Oct 2012 17:56:07 +0800
From:	Hongbo Zhang <hongbo.zhang@...aro.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	Amit Kucheria <amit.kucheria@...aro.org>,
	linaro-dev@...ts.linaro.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org, STEricsson_nomadik_linux@...t.st.com,
	kernel@...oocommunity.org, linaro-kernel@...ts.linaro.org,
	"hongbo.zhang" <hongbo.zhang@...aro.com>, patches@...aro.org
Subject: Re: [PATCH V2 5/6] Thermal: Add ST-Ericsson DB8500 thermal dirver.

On 25 October 2012 17:33, Hongbo Zhang <hongbo.zhang@...aro.org> wrote:
> On 25 October 2012 16:41, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>> On 25 October 2012 13:56, Hongbo Zhang <hongbo.zhang@...aro.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists