From: Lee Jones <lee.jones@linaro.org> To: Henry Chen <henryc.chen@mediatek.com> Cc: John Crispin <blogic@openwrt.org>, Yingjoe Chen <yingjoe.chen@mediatek.com>, Sascha Hauer <s.hauer@pengutronix.de>, linux-kernel@vger.kernel.org, Flora Fu <flora.fu@mediatek.com>, linux-mediatek@lists.infradead.org, linux@roeck-us.net, Matthias Brugger <matthias.bgg@gmail.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] mfd: mt6397: irq domain should initialize before mfd_add_devices() Date: Thu, 7 Apr 2016 08:52:14 +0100 [thread overview] Message-ID: <20160407075214.GX3323@x1> (raw) In-Reply-To: <1459910866.18284.27.camel@mtksdaap41> On Wed, 06 Apr 2016, Henry Chen wrote: > On Thu, 2016-03-31 at 16:07 +0200, John Crispin wrote: > > > > On 31/03/2016 15:41, Yingjoe Chen wrote: > > > On Thu, 2016-03-31 at 11:08 +0200, John Crispin wrote: > > >> > > >> On 31/03/2016 04:32, Yingjoe Chen wrote: > > >>> On Thu, 2016-03-31 at 09:40 +0800, Henry Chen wrote: > > >>>> On Wed, 2016-03-30 at 11:18 +0200, John Crispin wrote: > > >>>>> Hi, > > >>>>> > > >>>>> small nitpick inline > > >>>>> > > >>>>> On 30/03/2016 09:25, Henry Chen wrote: > > >>>>>> Some sub driver like RTC module need irq domain from parent to create > > >>>>>> irq mapping when driver initialize. so move mt6397_irq_init() before > > >>>>>> mfd_add_devices(). > > >>>>>> > > >>>>>> Signed-off-by: Henry Chen <henryc.chen@mediatek.com> > > >>>>>> --- > > >>>>>> This patch fixed the below warning based on "Linux kernel v4.6-rc1" > > >>>>>> WARNING: CPU: 1 PID: 132 at kernel/mediatek/kernel/irq/irqdomain.c:471 > > >>>>>> irq_create_mapping+0xc4/0xd0 > > >>>>>> --- > > >>>>>> drivers/mfd/mt6397-core.c | 37 ++++++++++++++++++++++--------------- > > >>>>>> 1 file changed, 22 insertions(+), 15 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c > > >>>>>> index 8e8d932..a879223 100644 > > >>>>>> --- a/drivers/mfd/mt6397-core.c > > >>>>>> +++ b/drivers/mfd/mt6397-core.c > > >>>>>> @@ -270,22 +270,36 @@ static int mt6397_probe(struct platform_device *pdev) > > >>>>>> goto fail_irq; > > >>>>>> } > > >>>>>> > > >>>>>> + pmic->irq = platform_get_irq(pdev, 0); > > >>>>>> + > > >>>>>> switch (id & 0xff) { > > >>>>>> case MT6323_CID_CODE: > > >>>>>> - pmic->int_con[0] = MT6323_INT_CON0; > > >>>>>> - pmic->int_con[1] = MT6323_INT_CON1; > > >>>>>> - pmic->int_status[0] = MT6323_INT_STATUS0; > > >>>>>> - pmic->int_status[1] = MT6323_INT_STATUS1; > > >>>>>> + if (pmic->irq > 0) { > > >>>>> > > >>>>> should this not be > > >>>>> > > >>>>> if (pmic->irq >= 0) { > > >>>>> > > >>>>> i think the code before your patch was wrong as linux irqs start with 0. > > >>>>> > > >>>>> John > > >>>> Hi John, > > >>>> > > >>>> Thanks, I will modify this. > > >>> > > >>> Linux irq start from 1, 0 is invalid. I can't find the document saying > > >>> this now, but you could see this from irq_create_mapping() in > > >>> kernel/irq/irqdomain.c > > >>> > > >>> I think the code should have check return from platform_get_irq and > > >>> handle -EPROBE_DEFER, but maybe it should be another patch? > > >>> > > >>> BTW, in this function, it is possible that pmic->irq_domain will be NULL > > >>> in fail_irq error handling. We should check before calling > > >>> irq_domain_remove. > > >>> > > >>> Joe.C > > >>> > > >> > > >> Hi, > > >> > > >> looking at > > >> http://lxr.free-electrons.com/source/drivers/base/platform.c#L87 there > > >> is a check in line #100 ret >= 0 > > >> > > >> checking the return value of pmic->irq = platform_get_irq(pdev, 0); > > >> should follow the same pattern i think .. unless i have a thinko and am > > >> reading the code wrong. > > > > > > > > > I'm not sure why platform_get_irq() check for 0, but I think the code > > > logic is differnet. > > > > > > When platform_get_irq() return 0 to our code, it means we don't have > > > valid irq to use. In this case it doesn't make any sense to continue > > > init irq. > > > > > > > > > Joe.C > > > > > > > > > --> http://lwn.net/Articles/470820/ > > > > indeed ARM has changed this is seems. was not aware of this change, > > sorry for the noise > > > > John > > Hi Lee/Joe/John, > > Thanks for comment, that means we can keep this patch for 4.6-rc1 > regression (RTC get NULL irq issue), right? Just re-send everything with any Acks that you collected. > I will send another patch to fixed above error handling problem. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
WARNING: multiple messages have this Message-ID (diff)
From: lee.jones@linaro.org (Lee Jones) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] mfd: mt6397: irq domain should initialize before mfd_add_devices() Date: Thu, 7 Apr 2016 08:52:14 +0100 [thread overview] Message-ID: <20160407075214.GX3323@x1> (raw) In-Reply-To: <1459910866.18284.27.camel@mtksdaap41> On Wed, 06 Apr 2016, Henry Chen wrote: > On Thu, 2016-03-31 at 16:07 +0200, John Crispin wrote: > > > > On 31/03/2016 15:41, Yingjoe Chen wrote: > > > On Thu, 2016-03-31 at 11:08 +0200, John Crispin wrote: > > >> > > >> On 31/03/2016 04:32, Yingjoe Chen wrote: > > >>> On Thu, 2016-03-31 at 09:40 +0800, Henry Chen wrote: > > >>>> On Wed, 2016-03-30 at 11:18 +0200, John Crispin wrote: > > >>>>> Hi, > > >>>>> > > >>>>> small nitpick inline > > >>>>> > > >>>>> On 30/03/2016 09:25, Henry Chen wrote: > > >>>>>> Some sub driver like RTC module need irq domain from parent to create > > >>>>>> irq mapping when driver initialize. so move mt6397_irq_init() before > > >>>>>> mfd_add_devices(). > > >>>>>> > > >>>>>> Signed-off-by: Henry Chen <henryc.chen@mediatek.com> > > >>>>>> --- > > >>>>>> This patch fixed the below warning based on "Linux kernel v4.6-rc1" > > >>>>>> WARNING: CPU: 1 PID: 132 at kernel/mediatek/kernel/irq/irqdomain.c:471 > > >>>>>> irq_create_mapping+0xc4/0xd0 > > >>>>>> --- > > >>>>>> drivers/mfd/mt6397-core.c | 37 ++++++++++++++++++++++--------------- > > >>>>>> 1 file changed, 22 insertions(+), 15 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c > > >>>>>> index 8e8d932..a879223 100644 > > >>>>>> --- a/drivers/mfd/mt6397-core.c > > >>>>>> +++ b/drivers/mfd/mt6397-core.c > > >>>>>> @@ -270,22 +270,36 @@ static int mt6397_probe(struct platform_device *pdev) > > >>>>>> goto fail_irq; > > >>>>>> } > > >>>>>> > > >>>>>> + pmic->irq = platform_get_irq(pdev, 0); > > >>>>>> + > > >>>>>> switch (id & 0xff) { > > >>>>>> case MT6323_CID_CODE: > > >>>>>> - pmic->int_con[0] = MT6323_INT_CON0; > > >>>>>> - pmic->int_con[1] = MT6323_INT_CON1; > > >>>>>> - pmic->int_status[0] = MT6323_INT_STATUS0; > > >>>>>> - pmic->int_status[1] = MT6323_INT_STATUS1; > > >>>>>> + if (pmic->irq > 0) { > > >>>>> > > >>>>> should this not be > > >>>>> > > >>>>> if (pmic->irq >= 0) { > > >>>>> > > >>>>> i think the code before your patch was wrong as linux irqs start with 0. > > >>>>> > > >>>>> John > > >>>> Hi John, > > >>>> > > >>>> Thanks, I will modify this. > > >>> > > >>> Linux irq start from 1, 0 is invalid. I can't find the document saying > > >>> this now, but you could see this from irq_create_mapping() in > > >>> kernel/irq/irqdomain.c > > >>> > > >>> I think the code should have check return from platform_get_irq and > > >>> handle -EPROBE_DEFER, but maybe it should be another patch? > > >>> > > >>> BTW, in this function, it is possible that pmic->irq_domain will be NULL > > >>> in fail_irq error handling. We should check before calling > > >>> irq_domain_remove. > > >>> > > >>> Joe.C > > >>> > > >> > > >> Hi, > > >> > > >> looking at > > >> http://lxr.free-electrons.com/source/drivers/base/platform.c#L87 there > > >> is a check in line #100 ret >= 0 > > >> > > >> checking the return value of pmic->irq = platform_get_irq(pdev, 0); > > >> should follow the same pattern i think .. unless i have a thinko and am > > >> reading the code wrong. > > > > > > > > > I'm not sure why platform_get_irq() check for 0, but I think the code > > > logic is differnet. > > > > > > When platform_get_irq() return 0 to our code, it means we don't have > > > valid irq to use. In this case it doesn't make any sense to continue > > > init irq. > > > > > > > > > Joe.C > > > > > > > > > --> http://lwn.net/Articles/470820/ > > > > indeed ARM has changed this is seems. was not aware of this change, > > sorry for the noise > > > > John > > Hi Lee/Joe/John, > > Thanks for comment, that means we can keep this patch for 4.6-rc1 > regression (RTC get NULL irq issue), right? Just re-send everything with any Acks that you collected. > I will send another patch to fixed above error handling problem. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
next prev parent reply other threads:[~2016-04-07 7:52 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-03-30 7:25 [PATCH] mfd: mt6397: irq domain should initialize before mfd_add_devices() Henry Chen 2016-03-30 7:25 ` Henry Chen 2016-03-30 7:25 ` Henry Chen 2016-03-30 9:18 ` John Crispin 2016-03-30 9:18 ` John Crispin 2016-03-31 1:40 ` Henry Chen 2016-03-31 1:40 ` Henry Chen 2016-03-31 1:40 ` Henry Chen 2016-03-31 2:32 ` Yingjoe Chen 2016-03-31 2:32 ` Yingjoe Chen 2016-03-31 2:32 ` Yingjoe Chen 2016-03-31 9:08 ` John Crispin 2016-03-31 9:08 ` John Crispin 2016-03-31 13:41 ` Yingjoe Chen 2016-03-31 13:41 ` Yingjoe Chen 2016-03-31 13:41 ` Yingjoe Chen 2016-03-31 14:07 ` John Crispin 2016-03-31 14:07 ` John Crispin 2016-04-06 2:47 ` Henry Chen 2016-04-06 2:47 ` Henry Chen 2016-04-06 2:47 ` Henry Chen 2016-04-07 7:52 ` Lee Jones [this message] 2016-04-07 7:52 ` Lee Jones 2016-04-07 8:51 ` John Crispin 2016-04-07 8:51 ` John Crispin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160407075214.GX3323@x1 \ --to=lee.jones@linaro.org \ --cc=blogic@openwrt.org \ --cc=flora.fu@mediatek.com \ --cc=henryc.chen@mediatek.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux@roeck-us.net \ --cc=matthias.bgg@gmail.com \ --cc=s.hauer@pengutronix.de \ --cc=yingjoe.chen@mediatek.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.