Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 May 2020 21:53:06 +0800
From:   Dejin Zheng <zhengdejin5@gmail.com>
To:     Grygorii Strashko <grygorii.strashko@ti.com>
Cc:     gregkh@linuxfoundation.org, wsa+renesas@sang-engineering.com,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] i2c: busses: convert to
 devm_platform_request_irq()
Message-ID: <20200519135306.GA1647@nuc8i5>
References: <20200518155304.28639-1-zhengdejin5@gmail.com>
 <20200518155304.28639-3-zhengdejin5@gmail.com>
 <c4f33036-1a88-1d75-c4ce-3024d2bf9bb1@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c4f33036-1a88-1d75-c4ce-3024d2bf9bb1@ti.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 18, 2020 at 08:34:01PM +0300, Grygorii Strashko wrote:
> 
> 
> On 18/05/2020 18:53, Dejin Zheng wrote:
> > Use devm_platform_request_irq() to simplify code, and it contains
> > platform_get_irq() and devm_request_irq().
> > 
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> >   drivers/i2c/busses/i2c-bcm-kona.c  | 16 +++-------------
> >   drivers/i2c/busses/i2c-cadence.c   | 10 +++-------
> >   drivers/i2c/busses/i2c-digicolor.c | 10 +++-------
> >   drivers/i2c/busses/i2c-emev2.c     |  5 ++---
> >   drivers/i2c/busses/i2c-jz4780.c    |  5 ++---
> >   drivers/i2c/busses/i2c-meson.c     | 13 ++++---------
> >   drivers/i2c/busses/i2c-mxs.c       |  9 +++------
> >   drivers/i2c/busses/i2c-pnx.c       |  9 ++-------
> >   drivers/i2c/busses/i2c-rcar.c      |  9 +++------
> >   drivers/i2c/busses/i2c-rk3x.c      | 14 +++-----------
> >   drivers/i2c/busses/i2c-sirf.c      | 10 ++--------
> >   drivers/i2c/busses/i2c-stu300.c    |  4 ++--
> >   drivers/i2c/busses/i2c-synquacer.c | 12 +++---------
> >   13 files changed, 35 insertions(+), 91 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-bcm-kona.c b/drivers/i2c/busses/i2c-bcm-kona.c
> > index ed5e1275ae46..f45acb47552a 100644
> > --- a/drivers/i2c/busses/i2c-bcm-kona.c
> > +++ b/drivers/i2c/busses/i2c-bcm-kona.c
> > @@ -818,20 +818,10 @@ static int bcm_kona_i2c_probe(struct platform_device *pdev)
> >   	       ISR_NOACK_MASK,
> >   	       dev->base + ISR_OFFSET);
> > -	/* Get the interrupt number */
> > -	dev->irq = platform_get_irq(pdev, 0);
> > -	if (dev->irq < 0) {
> > -		rc = dev->irq;
> > -		goto probe_disable_clk;
> > -	}
> > -
> > -	/* register the ISR handler */
> > -	rc = devm_request_irq(&pdev->dev, dev->irq, bcm_kona_i2c_isr,
> > -			      IRQF_SHARED, pdev->name, dev);
> > -	if (rc) {
> > -		dev_err(dev->device, "failed to request irq %i\n", dev->irq);
> > +	rc = devm_platform_request_irq(pdev, 0, &dev->irq, bcm_kona_i2c_isr,
> > +					IRQF_SHARED, pdev->name, dev);
> > +	if (rc)
> >   		goto probe_disable_clk;
> > -	}
> >   	/* Enable the controller but leave it idle */
> >   	bcm_kona_i2c_send_cmd_to_ctrl(dev, BCM_CMD_NOACTION);
> > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> > index 4b72398af505..9ffae4d231dc 100644
> > --- a/drivers/i2c/busses/i2c-cadence.c
> > +++ b/drivers/i2c/busses/i2c-cadence.c
> > @@ -1204,8 +1204,6 @@ static int cdns_i2c_probe(struct platform_device *pdev)
> >   	if (IS_ERR(id->membase))
> >   		return PTR_ERR(id->membase);
> > -	id->irq = platform_get_irq(pdev, 0);
> > -
> 
> 
> In many cases It is two strictly different steps
> 1) Request resource, including IRQ mapping and differed probe handling.
>    It should be done as early as possible to avoid unnecessary initialization steps
>    when resource (irq) is not ready,  and so avoid boot time increasing.
> 2) Actually request and enable IRQ, which, in many case, should be done late in probe
>    when driver and HW are actually ready to handle IRQs.
> 
> here, for example, between this point
> 
> >   	id->adap.owner = THIS_MODULE;
> >   	id->adap.dev.of_node = pdev->dev.of_node;
> >   	id->adap.algo = &cdns_i2c_algo;
> > @@ -1256,12 +1254,10 @@ static int cdns_i2c_probe(struct platform_device *pdev)
> >   		goto err_clk_dis;
> >   	}
> > -	ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0,
> > -				 DRIVER_NAME, id);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "cannot get irq %d\n", id->irq);
> 
> and this point the following happens:
>  - devm_clk_get() can fail and cause probe defer
>  - clk_prepare_enable()
>  - pm_runtime_.. - pm init
>  - cdns_i2c_.. - hw int
> 
> > +	ret = devm_platform_request_irq(pdev, 0, &id->irq, cdns_i2c_isr, 0,
> > +					DRIVER_NAME, id);
> 
> and now platform_get_irq(), which can fail due to IRQ controller not ready,
> and all above has to be reverted.
> 
> > +	if (ret)
> >   		goto err_clk_dis;
> > -	}
> [...]
> 
> A bit risky optimization, especially for bulk changes.
>
Grygorii, Thanks for your comments, you are right, abandon these two patches.

BTW, The gmail will prevent me sending messages to many recipients, so
remove some recipients. and still sending to i2c and kernel mail list.

BR,
Dejin

> -- 
> Best regards,
> grygorii