Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422772Ab2JYJ4L (ORCPT ); Thu, 25 Oct 2012 05:56:11 -0400 Received: from mail-ie0-f174.google.com ([209.85.223.174]:65167 "EHLO mail-ie0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934884Ab2JYJ4I (ORCPT ); Thu, 25 Oct 2012 05:56:08 -0400 MIME-Version: 1.0 In-Reply-To: References: <1350387889-15324-1-git-send-email-hongbo.zhang@linaro.com> <1351079900-32236-1-git-send-email-hongbo.zhang@linaro.com> <1351079900-32236-6-git-send-email-hongbo.zhang@linaro.com> Date: Thu, 25 Oct 2012 17:56:07 +0800 Message-ID: Subject: Re: [PATCH V2 5/6] Thermal: Add ST-Ericsson DB8500 thermal dirver. From: Hongbo Zhang To: Viresh Kumar Cc: Amit Kucheria , 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" , patches@linaro.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5333 Lines: 139 On 25 October 2012 17:33, Hongbo Zhang wrote: > On 25 October 2012 16:41, Viresh Kumar wrote: >> On 25 October 2012 13:56, Hongbo Zhang 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/