Date:	Thu, 25 Oct 2012 17:33:08 +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 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.

> 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