All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yingjoe Chen <yingjoe.chen@mediatek.com>
To: Henry Chen <henryc.chen@mediatek.com>
Cc: John Crispin <blogic@openwrt.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	<linux-kernel@vger.kernel.org>, Flora Fu <flora.fu@mediatek.com>,
	<linux-mediatek@lists.infradead.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] mfd: mt6397: irq domain should initialize before mfd_add_devices()
Date: Thu, 31 Mar 2016 10:32:13 +0800	[thread overview]
Message-ID: <1459391533.21870.15.camel@mtksdaap41> (raw)
In-Reply-To: <1459388452.18284.2.camel@mtksdaap41>

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

WARNING: multiple messages have this Message-ID (diff)
From: Yingjoe Chen <yingjoe.chen@mediatek.com>
To: Henry Chen <henryc.chen@mediatek.com>
Cc: John Crispin <blogic@openwrt.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	linux-kernel@vger.kernel.org, Flora Fu <flora.fu@mediatek.com>,
	linux-mediatek@lists.infradead.org,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] mfd: mt6397: irq domain should initialize before mfd_add_devices()
Date: Thu, 31 Mar 2016 10:32:13 +0800	[thread overview]
Message-ID: <1459391533.21870.15.camel@mtksdaap41> (raw)
In-Reply-To: <1459388452.18284.2.camel@mtksdaap41>

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

WARNING: multiple messages have this Message-ID (diff)
From: yingjoe.chen@mediatek.com (Yingjoe Chen)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] mfd: mt6397: irq domain should initialize before mfd_add_devices()
Date: Thu, 31 Mar 2016 10:32:13 +0800	[thread overview]
Message-ID: <1459391533.21870.15.camel@mtksdaap41> (raw)
In-Reply-To: <1459388452.18284.2.camel@mtksdaap41>

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

  reply	other threads:[~2016-03-31  2:32 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 [this message]
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
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=1459391533.21870.15.camel@mtksdaap41 \
    --to=yingjoe.chen@mediatek.com \
    --cc=blogic@openwrt.org \
    --cc=flora.fu@mediatek.com \
    --cc=henryc.chen@mediatek.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=s.hauer@pengutronix.de \
    /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: link
Be 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.