diff mbox series

[v3] i2c: imx: support DMA defer probing

Message ID 20241127074458.2102112-1-carlos.song@nxp.com (mailing list archive)
State Superseded
Headers show
Series [v3] i2c: imx: support DMA defer probing | expand

Commit Message

Carlos Song Nov. 27, 2024, 7:44 a.m. UTC 
From: Carlos Song <carlos.song@nxp.com>

Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver
have not ready yet.

Move i2c_imx_dma_request() before registering I2C adapter to avoid
infinite loop of .probe() calls to the same driver, see "e8c220fac415
Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
and "Documentation/driver-api/driver-model/driver.rst".

Use CPU mode to avoid stuck registering i2c adapter when DMA resources
are unavailable.

Signed-off-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
Change for V3:
- According to Marc's comment, remove error print when defer probe.
  Add info log when DMA has not been enabled and add error log when
  DMA error, both won't stuck the i2c adapter register, just for reminding,
  i2c adapter is working only in PIO mode. 
Change for V2:
- According to Frank's comments, wrap at 75 char and Simplify fix code
  at i2c_imx_dma_request().
- Use strict patch check, fix alignment warning at i2c_imx_dma_request()
---
 drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

Comments

Marc Kleine-Budde Nov. 27, 2024, 7:42 a.m. UTC | #1 
On 27.11.2024 15:44:58, carlos.song@nxp.com wrote:
> From: Carlos Song <carlos.song@nxp.com>
> 
> Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver
> have not ready yet.
> 
> Move i2c_imx_dma_request() before registering I2C adapter to avoid
> infinite loop of .probe() calls to the same driver, see "e8c220fac415
> Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
> and "Documentation/driver-api/driver-model/driver.rst".
> 
> Use CPU mode to avoid stuck registering i2c adapter when DMA resources
> are unavailable.
> 
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
> Change for V3:
> - According to Marc's comment, remove error print when defer probe.
>   Add info log when DMA has not been enabled and add error log when
>   DMA error, both won't stuck the i2c adapter register, just for reminding,
>   i2c adapter is working only in PIO mode. 
> Change for V2:
> - According to Frank's comments, wrap at 75 char and Simplify fix code
>   at i2c_imx_dma_request().
> - Use strict patch check, fix alignment warning at i2c_imx_dma_request()
> ---
>  drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 5ed4cb61e262..47112a38d034 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx)
>  }
>  
>  /* Functions for DMA support */
> -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> -						dma_addr_t phy_addr)
> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr)
>  {
>  	struct imx_i2c_dma *dma;
>  	struct dma_slave_config dma_sconfig;
> -	struct device *dev = &i2c_imx->adapter.dev;
> +	struct device *dev = i2c_imx->adapter.dev.parent;
>  	int ret;
>  
>  	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
>  	if (!dma)
> -		return;
> +		return -ENOMEM;
>  
>  	dma->chan_tx = dma_request_chan(dev, "tx");
>  	if (IS_ERR(dma->chan_tx)) {
> @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>  	dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
>  		dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
>  
> -	return;
> +	return 0;
>  
>  fail_rx:
>  	dma_release_channel(dma->chan_rx);
> @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>  	dma_release_channel(dma->chan_tx);
>  fail_al:
>  	devm_kfree(dev, dma);
> +
> +	return ret;
>  }
>  
>  static void i2c_imx_dma_callback(void *arg)
> @@ -1803,6 +1804,23 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	if (ret == -EPROBE_DEFER)
>  		goto clk_notifier_unregister;
>  
> +	/*
> +	 * Init DMA config if supported, -ENODEV means DMA not enabled at
> +	 * this platform, that is not a real error, so just remind "only
> +	 * PIO mode is used". If DMA is enabled, but meet error when request
> +	 * DMA channel, error should be showed in probe error log. PIO mode
> +	 * should be available regardless of DMA.
> +	 */
> +	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			goto clk_notifier_unregister;
> +		else if (ret == -ENODEV)
> +			dev_info(&pdev->dev, "Only use PIO mode\n");

On a system without DMA configured, with this patch we now get this info
message that wasn't there before. I think this might annoy some people,
so you should remove it.

regards,
Marc

> +		else
> +			dev_err_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n");
> +	}
> +
>  	/* Add I2C adapter */
>  	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
>  	if (ret < 0)
> @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  		i2c_imx->adapter.name);
>  	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>  
> -	/* Init DMA config if supported */
> -	i2c_imx_dma_request(i2c_imx, phy_addr);
> -
>  	return 0;   /* Return OK */
>  
>  clk_notifier_unregister:
> -- 
> 2.34.1
> 
>
Carlos Song Nov. 27, 2024, 7:48 a.m. UTC | #2 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: Wednesday, November 27, 2024 3:42 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; o.rempel@pengutronix.de;
> kernel@pengutronix.de; andi.shyti@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; linux-i2c@vger.kernel.org;
> imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v3] i2c: imx: support DMA defer probing
> 
> On 27.11.2024 15:44:58, carlos.song@nxp.com wrote:
> > From: Carlos Song <carlos.song@nxp.com>
> >
> > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA
> > driver have not ready yet.
> >
> > Move i2c_imx_dma_request() before registering I2C adapter to avoid
> > infinite loop of .probe() calls to the same driver, see "e8c220fac415
> > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
> > and "Documentation/driver-api/driver-model/driver.rst".
> >
> > Use CPU mode to avoid stuck registering i2c adapter when DMA resources
> > are unavailable.
> >
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > ---
> > Change for V3:
> > - According to Marc's comment, remove error print when defer probe.
> >   Add info log when DMA has not been enabled and add error log when
> >   DMA error, both won't stuck the i2c adapter register, just for reminding,
> >   i2c adapter is working only in PIO mode.
> > Change for V2:
> > - According to Frank's comments, wrap at 75 char and Simplify fix code
> >   at i2c_imx_dma_request().
> > - Use strict patch check, fix alignment warning at
> > i2c_imx_dma_request()
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx.c
> > b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..47112a38d034 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct
> > imx_i2c_struct *i2c_imx)  }
> >
> >  /* Functions for DMA support */
> > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > -						dma_addr_t phy_addr)
> > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > +dma_addr_t phy_addr)
> >  {
> >  	struct imx_i2c_dma *dma;
> >  	struct dma_slave_config dma_sconfig;
> > -	struct device *dev = &i2c_imx->adapter.dev;
> > +	struct device *dev = i2c_imx->adapter.dev.parent;
> >  	int ret;
> >
> >  	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
> >  	if (!dma)
> > -		return;
> > +		return -ENOMEM;
> >
> >  	dma->chan_tx = dma_request_chan(dev, "tx");
> >  	if (IS_ERR(dma->chan_tx)) {
> > @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct
> imx_i2c_struct *i2c_imx,
> >  	dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
> >  		dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
> >
> > -	return;
> > +	return 0;
> >
> >  fail_rx:
> >  	dma_release_channel(dma->chan_rx);
> > @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct
> imx_i2c_struct *i2c_imx,
> >  	dma_release_channel(dma->chan_tx);
> >  fail_al:
> >  	devm_kfree(dev, dma);
> > +
> > +	return ret;
> >  }
> >
> >  static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@
> > static int i2c_imx_probe(struct platform_device *pdev)
> >  	if (ret == -EPROBE_DEFER)
> >  		goto clk_notifier_unregister;
> >
> > +	/*
> > +	 * Init DMA config if supported, -ENODEV means DMA not enabled at
> > +	 * this platform, that is not a real error, so just remind "only
> > +	 * PIO mode is used". If DMA is enabled, but meet error when request
> > +	 * DMA channel, error should be showed in probe error log. PIO mode
> > +	 * should be available regardless of DMA.
> > +	 */
> > +	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			goto clk_notifier_unregister;
> > +		else if (ret == -ENODEV)
> > +			dev_info(&pdev->dev, "Only use PIO mode\n");
> 
> On a system without DMA configured, with this patch we now get this info
> message that wasn't there before. I think this might annoy some people, so you
> should remove it.
> 

:-) hhh, get it.

How about change to dev_dbg?
> regards,
> Marc
> 
> > +		else
> > +			dev_err_probe(&pdev->dev, ret, "Failed to setup DMA, only use
> PIO mode\n");
> > +	}
> > +
> >  	/* Add I2C adapter */
> >  	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
> >  	if (ret < 0)
> > @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> >  		i2c_imx->adapter.name);
> >  	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
> >
> > -	/* Init DMA config if supported */
> > -	i2c_imx_dma_request(i2c_imx, phy_addr);
> > -
> >  	return 0;   /* Return OK */
> >
> >  clk_notifier_unregister:
> > --
> > 2.34.1
> >
> >
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Marc Kleine-Budde Nov. 27, 2024, 7:58 a.m. UTC | #3 
On 27.11.2024 07:48:13, Carlos Song wrote:
> > >  static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@
> > > static int i2c_imx_probe(struct platform_device *pdev)
> > >  	if (ret == -EPROBE_DEFER)
> > >  		goto clk_notifier_unregister;
> > >
> > > +	/*
> > > +	 * Init DMA config if supported, -ENODEV means DMA not enabled at
> > > +	 * this platform, that is not a real error, so just remind "only
> > > +	 * PIO mode is used". If DMA is enabled, but meet error when request
> > > +	 * DMA channel, error should be showed in probe error log. PIO mode
> > > +	 * should be available regardless of DMA.
> > > +	 */
> > > +	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > > +	if (ret) {
> > > +		if (ret == -EPROBE_DEFER)
> > > +			goto clk_notifier_unregister;
> > > +		else if (ret == -ENODEV)
> > > +			dev_info(&pdev->dev, "Only use PIO mode\n");
> > 
> > On a system without DMA configured, with this patch we now get this info
> > message that wasn't there before. I think this might annoy some people, so you
> > should remove it.
> > 
> 
> :-) hhh, get it.

Some things look reasonable when discussing the patch, but when you
see the new, cleaned-up version, you immediately realize that this is
going to annoy people :)

> How about change to dev_dbg?

Good idea.

regards,
Marc
Carlos Song Nov. 27, 2024, 8:18 a.m. UTC | #4 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: Wednesday, November 27, 2024 3:59 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; o.rempel@pengutronix.de; 
> kernel@pengutronix.de; andi.shyti@kernel.org; shawnguo@kernel.org; 
> s.hauer@pengutronix.de; festevam@gmail.com; linux-i2c@vger.kernel.org; 
> imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: Re: RE: [EXT] Re: [PATCH v3] i2c: imx: support DMA defer 
> probing
> 
> On 27.11.2024 07:48:13, Carlos Song wrote:
> > > >  static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 
> > > > @@ static int i2c_imx_probe(struct platform_device *pdev)
> > > >  	if (ret == -EPROBE_DEFER)
> > > >  		goto clk_notifier_unregister;
> > > >
> > > > +	/*
> > > > +	 * Init DMA config if supported, -ENODEV means DMA not enabled at
> > > > +	 * this platform, that is not a real error, so just remind "only
> > > > +	 * PIO mode is used". If DMA is enabled, but meet error when
> request
> > > > +	 * DMA channel, error should be showed in probe error log. PIO mode
> > > > +	 * should be available regardless of DMA.
> > > > +	 */
> > > > +	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > > > +	if (ret) {
> > > > +		if (ret == -EPROBE_DEFER)
> > > > +			goto clk_notifier_unregister;
> > > > +		else if (ret == -ENODEV)
> > > > +			dev_info(&pdev->dev, "Only use PIO mode\n");
> > >
> > > On a system without DMA configured, with this patch we now get 
> > > this info message that wasn't there before. I think this might 
> > > annoy some people, so you should remove it.
> > >
> >
> > :-) hhh, get it.
> 
> Some things look reasonable when discussing the patch, but when you 
> see the new, cleaned-up version, you immediately realize that this is 
> going to annoy people :)
> 

Such valuable experience~ Thank you very much, Marc! This is definitely what I haven't thought about before.
I'll pay more attention in the future. :)

> > How about change to dev_dbg?
> 
> Good idea.
> 
Will fix it at V4.

> regards,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 5ed4cb61e262..47112a38d034 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -397,17 +397,16 @@  static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx)
 }
 
 /* Functions for DMA support */
-static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
-						dma_addr_t phy_addr)
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr)
 {
 	struct imx_i2c_dma *dma;
 	struct dma_slave_config dma_sconfig;
-	struct device *dev = &i2c_imx->adapter.dev;
+	struct device *dev = i2c_imx->adapter.dev.parent;
 	int ret;
 
 	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
 	if (!dma)
-		return;
+		return -ENOMEM;
 
 	dma->chan_tx = dma_request_chan(dev, "tx");
 	if (IS_ERR(dma->chan_tx)) {
@@ -452,7 +451,7 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
 		dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
 
-	return;
+	return 0;
 
 fail_rx:
 	dma_release_channel(dma->chan_rx);
@@ -460,6 +459,8 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_release_channel(dma->chan_tx);
 fail_al:
 	devm_kfree(dev, dma);
+
+	return ret;
 }
 
 static void i2c_imx_dma_callback(void *arg)
@@ -1803,6 +1804,23 @@  static int i2c_imx_probe(struct platform_device *pdev)
 	if (ret == -EPROBE_DEFER)
 		goto clk_notifier_unregister;
 
+	/*
+	 * Init DMA config if supported, -ENODEV means DMA not enabled at
+	 * this platform, that is not a real error, so just remind "only
+	 * PIO mode is used". If DMA is enabled, but meet error when request
+	 * DMA channel, error should be showed in probe error log. PIO mode
+	 * should be available regardless of DMA.
+	 */
+	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			goto clk_notifier_unregister;
+		else if (ret == -ENODEV)
+			dev_info(&pdev->dev, "Only use PIO mode\n");
+		else
+			dev_err_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n");
+	}
+
 	/* Add I2C adapter */
 	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
 	if (ret < 0)
@@ -1817,9 +1835,6 @@  static int i2c_imx_probe(struct platform_device *pdev)
 		i2c_imx->adapter.name);
 	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
 
-	/* Init DMA config if supported */
-	i2c_imx_dma_request(i2c_imx, phy_addr);
-
 	return 0;   /* Return OK */
 
 clk_notifier_unregister: