[PATCH v2 0/9] Use dev_err_probe in i2c probe function

[PATCH v2 0/9] Use dev_err_probe in i2c probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

v2:
1. Convert all dev_err() in lpi2c_imx_probe(), synquacer_i2c_probe(),
   mlxbf_i2c_probe() to dev_err_probe().
2. Add Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
3. Add Reviewed-by: Yicong Yang <yangyicong@hisilicon.com>
4. Add Reviewed-by: Andi Shyti <andi.shyti@kernel.com>

Liao Chang (9):
  i2c: bcm2835: Use dev_err_probe in probe function
  i2c: mlxbf: Use dev_err_probe in probe function
  i2c: xlp9xx: Use dev_err_probe in probe function
  i2c: hisi: Use dev_err_probe in probe function
  i2c: qcom-cci: Use dev_err_probe in probe function
  i2c: pxa: Use dev_err_probe in probe function
  i2c: dln2: Use dev_err_probe in probe function
  i2c: imx-lpi2c: Use dev_err_probe in probe function
  i2c: synquacer: Use dev_err_probe in probe function

 drivers/i2c/busses/i2c-bcm2835.c   | 14 ++++-----
 drivers/i2c/busses/i2c-dln2.c      |  6 ++--
 drivers/i2c/busses/i2c-hisi.c      | 12 +++----
 drivers/i2c/busses/i2c-imx-lpi2c.c | 12 +++----
 drivers/i2c/busses/i2c-mlxbf.c     | 50 ++++++++++--------------------
 drivers/i2c/busses/i2c-pxa.c       |  7 ++---
 drivers/i2c/busses/i2c-qcom-cci.c  |  6 ++--
 drivers/i2c/busses/i2c-synquacer.c | 19 ++++--------
 drivers/i2c/busses/i2c-xlp9xx.c    |  6 ++--
 9 files changed, 46 insertions(+), 86 deletions(-)

-- 
2.25.1

[PATCH 1/9] i2c: bcm2835: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-bcm2835.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 8ce6d3f49551..9af1a68269ab 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -430,10 +430,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 
 	i2c_dev->bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk, i2c_dev);
 
-	if (IS_ERR(i2c_dev->bus_clk)) {
-		dev_err(&pdev->dev, "Could not register clock\n");
-		return PTR_ERR(i2c_dev->bus_clk);
-	}
+	if (IS_ERR(i2c_dev->bus_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(i2c_dev->bus_clk),
+				     "Could not register clock\n");
 
 	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
 				   &bus_clk_rate);
@@ -444,10 +443,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	}
 
 	ret = clk_set_rate_exclusive(i2c_dev->bus_clk, bus_clk_rate);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Could not set clock frequency\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Could not set clock frequency\n");
 
 	ret = clk_prepare_enable(i2c_dev->bus_clk);
 	if (ret) {
-- 
2.25.1

[PATCH 2/9] i2c: mlxbf: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-mlxbf.c | 50 ++++++++++++----------------------
 1 file changed, 17 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index ae66bdd1b737..5ee82016c805 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -2323,10 +2323,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
 
 		ret = mlxbf_i2c_init_resource(pdev, &priv->smbus,
 					      MLXBF_I2C_SMBUS_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch smbus resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch smbus resource info");
 
 		priv->timer->io = priv->smbus->io;
 		priv->mst->io = priv->smbus->io + MLXBF_I2C_MST_ADDR_OFFSET;
@@ -2334,39 +2332,29 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
 	} else {
 		ret = mlxbf_i2c_init_resource(pdev, &priv->timer,
 					      MLXBF_I2C_SMBUS_TIMER_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch timer resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch timer resource info");
 
 		ret = mlxbf_i2c_init_resource(pdev, &priv->mst,
 					      MLXBF_I2C_SMBUS_MST_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch master resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch master resource info");
 
 		ret = mlxbf_i2c_init_resource(pdev, &priv->slv,
 					      MLXBF_I2C_SMBUS_SLV_RES);
-		if (ret < 0) {
-			dev_err(dev, "Cannot fetch slave resource info");
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Cannot fetch slave resource info");
 	}
 
 	ret = mlxbf_i2c_init_resource(pdev, &priv->mst_cause,
 				      MLXBF_I2C_MST_CAUSE_RES);
-	if (ret < 0) {
-		dev_err(dev, "Cannot fetch cause master resource info");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot fetch cause master resource info");
 
 	ret = mlxbf_i2c_init_resource(pdev, &priv->slv_cause,
 				      MLXBF_I2C_SLV_CAUSE_RES);
-	if (ret < 0) {
-		dev_err(dev, "Cannot fetch cause slave resource info");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot fetch cause slave resource info");
 
 	adap = &priv->adap;
 	adap->owner = THIS_MODULE;
@@ -2397,11 +2385,9 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
 	 * does not really hurt, then keep the code as is.
 	 */
 	ret = mlxbf_i2c_init_master(pdev, priv);
-	if (ret < 0) {
-		dev_err(dev, "failed to initialize smbus master %d",
-			priv->bus);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to initialize smbus master %d",
+				     priv->bus);
 
 	mlxbf_i2c_init_timings(pdev, priv);
 
@@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
 	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
 			       IRQF_SHARED | IRQF_PROBE_SHARED,
 			       dev_name(dev), priv);
-	if (ret < 0) {
-		dev_err(dev, "Cannot get irq %d\n", irq);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
 
 	priv->irq = irq;
 
-- 
2.25.1

[PATCH 3/9] i2c: xlp9xx: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-xlp9xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index f59e8c544f36..08a59a920929 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -529,10 +529,8 @@ static int xlp9xx_i2c_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, priv->irq, xlp9xx_i2c_isr, 0,
 			       pdev->name, priv);
-	if (err) {
-		dev_err(&pdev->dev, "IRQ request failed!\n");
-		return err;
-	}
+	if (err)
+		return dev_err_probe(&pdev->dev, err, "IRQ request failed!\n");
 
 	init_completion(&priv->msg_complete);
 	priv->adapter.dev.parent = &pdev->dev;
-- 
2.25.1

[PATCH 4/9] i2c: hisi: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Yicong Yang <yangyicong@hisilicon.com>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-hisi.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c
index e067671b3ce2..6fc8d6fa43b6 100644
--- a/drivers/i2c/busses/i2c-hisi.c
+++ b/drivers/i2c/busses/i2c-hisi.c
@@ -462,18 +462,14 @@ static int hisi_i2c_probe(struct platform_device *pdev)
 	hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL);
 
 	ret = devm_request_irq(dev, ctlr->irq, hisi_i2c_irq, 0, "hisi-i2c", ctlr);
-	if (ret) {
-		dev_err(dev, "failed to request irq handler, ret = %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request irq handler\n");
 
 	ctlr->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
 	if (IS_ERR_OR_NULL(ctlr->clk)) {
 		ret = device_property_read_u64(dev, "clk_rate", &clk_rate_hz);
-		if (ret) {
-			dev_err(dev, "failed to get clock frequency, ret = %d\n", ret);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to get clock frequency\n");
 	} else {
 		clk_rate_hz = clk_get_rate(ctlr->clk);
 	}
-- 
2.25.1

[PATCH 5/9] i2c: qcom-cci: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 622dc14add9d..cf13abec05f1 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -588,10 +588,8 @@ static int cci_probe(struct platform_device *pdev)
 	/* Clocks */
 
 	ret = devm_clk_bulk_get_all(dev, &cci->clocks);
-	if (ret < 1) {
-		dev_err(dev, "failed to get clocks %d\n", ret);
-		return ret;
-	}
+	if (ret < 1)
+		return dev_err_probe(dev, ret, "failed to get clocks\n");
 	cci->nclocks = ret;
 
 	/* Retrieve CCI clock rate */
-- 
2.25.1

[PATCH 6/9] i2c: pxa: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-pxa.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 937f7eebe906..79df4da0166e 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1404,10 +1404,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	strscpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
 
 	i2c->clk = devm_clk_get(&dev->dev, NULL);
-	if (IS_ERR(i2c->clk)) {
-		dev_err(&dev->dev, "failed to get the clk: %ld\n", PTR_ERR(i2c->clk));
-		return PTR_ERR(i2c->clk);
-	}
+	if (IS_ERR(i2c->clk))
+		return dev_err_probe(&dev->dev, PTR_ERR(i2c->clk),
+				     "failed to get the clk\n");
 
 	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
 	i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
-- 
2.25.1

[PATCH 7/9] i2c: dln2: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-dln2.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
index 4f02cc2fb567..631109c7a098 100644
--- a/drivers/i2c/busses/i2c-dln2.c
+++ b/drivers/i2c/busses/i2c-dln2.c
@@ -218,10 +218,8 @@ static int dln2_i2c_probe(struct platform_device *pdev)
 
 	/* initialize the i2c interface */
 	ret = dln2_i2c_enable(dln2, true);
-	if (ret < 0) {
-		dev_err(dev, "failed to initialize adapter: %d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to initialize adapter\n");
 
 	/* and finally attach to i2c layer */
 	ret = i2c_add_adapter(&dln2->adapter);
-- 
2.25.1

[PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c3287c887c6f..bfa788b3775b 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 		sizeof(lpi2c_imx->adapter.name));
 
 	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
 	lpi2c_imx->num_clks = ret;
 
 	ret = of_property_read_u32(pdev->dev.of_node,
@@ -582,10 +580,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
 			       pdev->name, lpi2c_imx);
-	if (ret) {
-		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
 
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
-- 
2.25.1

[PATCH 9/9] i2c: synquacer: Use dev_err_probe in probe function

[Liao Chang]

Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-synquacer.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
index 4cc196ca8f6d..8ada613598eb 100644
--- a/drivers/i2c/busses/i2c-synquacer.c
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -557,11 +557,8 @@ static int synquacer_i2c_probe(struct platform_device *pdev)
 		dev_dbg(&pdev->dev, "clock source %p\n", i2c->pclk);
 
 		ret = clk_prepare_enable(i2c->pclk);
-		if (ret) {
-			dev_err(&pdev->dev, "failed to enable clock (%d)\n",
-				ret);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
 		i2c->pclkrate = clk_get_rate(i2c->pclk);
 	}
 
@@ -582,10 +579,8 @@ static int synquacer_i2c_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, i2c->irq, synquacer_i2c_isr,
 			       0, dev_name(&pdev->dev), i2c);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "cannot claim IRQ %d\n", i2c->irq);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "cannot claim IRQ %d\n", i2c->irq);
 
 	i2c->state = STATE_IDLE;
 	i2c->dev = &pdev->dev;
@@ -605,10 +600,8 @@ static int synquacer_i2c_probe(struct platform_device *pdev)
 	synquacer_i2c_hw_init(i2c);
 
 	ret = i2c_add_numbered_adapter(&i2c->adapter);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to add bus to i2c core\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to add bus to i2c core\n");
 
 	platform_set_drvdata(pdev, i2c);
 
-- 
2.25.1

Re: [PATCH 2/9] i2c: mlxbf: Use dev_err_probe in probe function

[Andi Shyti]

Hi Liao,

On Wed, Aug 02, 2023 at 05:57:30PM +0800, Liao Chang wrote:
> Use the dev_err_probe function instead of dev_err in the probe function
> so that the printed messge includes the return value and also handles
> -EPROBE_DEFER nicely.
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Andi Shyti]

On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
> Use the dev_err_probe function instead of dev_err in the probe function
> so that the printed messge includes the return value and also handles
> -EPROBE_DEFER nicely.
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index c3287c887c6f..bfa788b3775b 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  		sizeof(lpi2c_imx->adapter.name));
>  
>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");

you cut on this because the line was going over 100 characters? :)

In theory you shouldn't change the print message when doing such
changes and you can still split it as:

		return dev_err_probe(&pdev->dev, ret,
				     "can't get I2C peripheral clock, ret=%d\n",
				     ret);

and you're even within the 80 characters.

Sorry, I missed it in the previous version, mind resending it?

Andi

Re: [PATCH 9/9] i2c: synquacer: Use dev_err_probe in probe function

[Andi Shyti]

On Wed, Aug 02, 2023 at 05:57:37PM +0800, Liao Chang wrote:
> Use the dev_err_probe function instead of dev_err in the probe function
> so that the printed messge includes the return value and also handles
> -EPROBE_DEFER nicely.
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
>  drivers/i2c/busses/i2c-synquacer.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
> index 4cc196ca8f6d..8ada613598eb 100644
> --- a/drivers/i2c/busses/i2c-synquacer.c
> +++ b/drivers/i2c/busses/i2c-synquacer.c
> @@ -557,11 +557,8 @@ static int synquacer_i2c_probe(struct platform_device *pdev)
>  		dev_dbg(&pdev->dev, "clock source %p\n", i2c->pclk);
>  
>  		ret = clk_prepare_enable(i2c->pclk);
> -		if (ret) {
> -			dev_err(&pdev->dev, "failed to enable clock (%d)\n",
> -				ret);
> -			return ret;
> -		}
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
>  		i2c->pclkrate = clk_get_rate(i2c->pclk);
>  	}
>  

you forgot

		dev_err(&pdev->dev, "PCLK missing or out of range (%d)\n",
			i2c->pclkrate);
		return -EINVAL;

Thanks,
Andi

Re: [PATCH v2 0/9] Use dev_err_probe in i2c probe function

[Andi Shyti]

Hi Liao,

On Wed, Aug 02, 2023 at 05:57:28PM +0800, Liao Chang wrote:
> Use the dev_err_probe function instead of dev_err in the probe function
> so that the printed messge includes the return value and also handles
> -EPROBE_DEFER nicely.

first of all thanks for this cleanup which is very much
appreciated!

I need to ask you to add the version to all the patches, not just
patch 0/9. It's trivial if you do "git format-patch -v 2...".

Thanks,
Andi

Re: [PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Liao, Chang]

Hi, Andi

在 2023/8/5 6:16, Andi Shyti 写道:
> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
>> Use the dev_err_probe function instead of dev_err in the probe function
>> so that the printed messge includes the return value and also handles
>> -EPROBE_DEFER nicely.
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>> ---
>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>> index c3287c887c6f..bfa788b3775b 100644
>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>>  		sizeof(lpi2c_imx->adapter.name));
>>  
>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
>> -	if (ret < 0) {
>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
>> -		return ret;
>> -	}
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
> 
> you cut on this because the line was going over 100 characters? :)
> 
> In theory you shouldn't change the print message when doing such
> changes and you can still split it as:
> 
> 		return dev_err_probe(&pdev->dev, ret,
> 				     "can't get I2C peripheral clock, ret=%d\n",
> 				     ret);
> 
> and you're even within the 80 characters.

Since dev_err_probe always print the second parameter that happens to be the return value,
I remove the "ret=%d" from the original message to avoid a redundant error message.

So is it better to keep the original message unchanged, even though dev_err_probe also prints
the return error value? Or is it better to make this change so that all error messages printed
in the probe function include the return value in a consistent style?

> 
> Sorry, I missed it in the previous version, mind resending it?

Sure, I will resend it in v3.

Thanks.

> 
> Andi

-- 
BR
Liao, Chang

Re: [PATCH v2 0/9] Use dev_err_probe in i2c probe function

[Liao, Chang]

在 2023/8/5 6:19, Andi Shyti 写道:
> Hi Liao,
> 
> On Wed, Aug 02, 2023 at 05:57:28PM +0800, Liao Chang wrote:
>> Use the dev_err_probe function instead of dev_err in the probe function
>> so that the printed messge includes the return value and also handles
>> -EPROBE_DEFER nicely.
> 
> first of all thanks for this cleanup which is very much
> appreciated!
> 
> I need to ask you to add the version to all the patches, not just
> patch 0/9. It's trivial if you do "git format-patch -v 2...".

Hi Andi,

Trully sorry for the misleading, I will add the version to all the patches
and send them out in next revision.

Thanks for taking the time to review these patches.

> 
> Thanks,
> Andi

-- 
BR
Liao, Chang

Re: [PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Andi Shyti]

On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
> Hi, Andi
> 
> 在 2023/8/5 6:16, Andi Shyti 写道:
> > On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
> >> Use the dev_err_probe function instead of dev_err in the probe function
> >> so that the printed messge includes the return value and also handles
> >> -EPROBE_DEFER nicely.
> >>
> >> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >> ---
> >>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
> >>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >> index c3287c887c6f..bfa788b3775b 100644
> >> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> >> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> >>  		sizeof(lpi2c_imx->adapter.name));
> >>  
> >>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
> >> -	if (ret < 0) {
> >> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
> >> -		return ret;
> >> -	}
> >> +	if (ret < 0)
> >> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
> > 
> > you cut on this because the line was going over 100 characters? :)
> > 
> > In theory you shouldn't change the print message when doing such
> > changes and you can still split it as:
> > 
> > 		return dev_err_probe(&pdev->dev, ret,
> > 				     "can't get I2C peripheral clock, ret=%d\n",
> > 				     ret);
> > 
> > and you're even within the 80 characters.
> 
> Since dev_err_probe always print the second parameter that happens to be the return value,
> I remove the "ret=%d" from the original message to avoid a redundant error message.
> 
> So is it better to keep the original message unchanged, even though dev_err_probe also prints
> the return error value? Or is it better to make this change so that all error messages printed
> in the probe function include the return value in a consistent style?

yes, you are right! Then please ignore this comment, but...

> >   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> >   		pdev->name, lpi2c_imx);
> > - 	if (ret) {
> > - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> > - 		return ret;
> > - 	}
> > + 	if (ret)
> > + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);

please make it coherent to this second part, as well, where the
error number is printed.

Thank you,
Andi

Re: [PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Liao, Chang]

Hi, Andi

在 2023/8/7 16:17, Andi Shyti 写道:
> On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
>> Hi, Andi
>>
>> 在 2023/8/5 6:16, Andi Shyti 写道:
>>> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
>>>> Use the dev_err_probe function instead of dev_err in the probe function
>>>> so that the printed messge includes the return value and also handles
>>>> -EPROBE_DEFER nicely.
>>>>
>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>> index c3287c887c6f..bfa788b3775b 100644
>>>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>>>>  		sizeof(lpi2c_imx->adapter.name));
>>>>  
>>>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
>>>> -	if (ret < 0) {
>>>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
>>>> -		return ret;
>>>> -	}
>>>> +	if (ret < 0)
>>>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
>>>
>>> you cut on this because the line was going over 100 characters? :)
>>>
>>> In theory you shouldn't change the print message when doing such
>>> changes and you can still split it as:
>>>
>>> 		return dev_err_probe(&pdev->dev, ret,
>>> 				     "can't get I2C peripheral clock, ret=%d\n",
>>> 				     ret);
>>>
>>> and you're even within the 80 characters.
>>
>> Since dev_err_probe always print the second parameter that happens to be the return value,
>> I remove the "ret=%d" from the original message to avoid a redundant error message.
>>
>> So is it better to keep the original message unchanged, even though dev_err_probe also prints
>> the return error value? Or is it better to make this change so that all error messages printed
>> in the probe function include the return value in a consistent style?
> 
> yes, you are right! Then please ignore this comment, but...
> 
>>>   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
>>>   		pdev->name, lpi2c_imx);
>>> - 	if (ret) {
>>> - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
>>> - 		return ret;
>>> - 	}
>>> + 	if (ret)
>>> + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
> 
> please make it coherent to this second part, as well, where the
> error number is printed.

Do you mean to convert it to the following?

    if (ret)
        return dev_err_probe(&pdev->dev, ret, "can't claim irq\n");

I understand that the style of error message printed by dev_err_probe is like
"error [ERRNO]: [customized message]", the [ERRNO] comes from 2nd parameter,
[customized message] comes from 3rd paramter, if the original [customized message]it
also print ERRNO, i intend to remove it in this patch, otherwise, I will just keep it.
In the above code, [customized message] intend to print irq but return value, so it is
better to keep the original message, right?

Thanks.

> 
> Thank you,
> Andi

-- 
BR
Liao, Chang

Re: [PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Andi Shyti]

On Mon, Aug 07, 2023 at 06:44:23PM +0800, Liao, Chang wrote:
> Hi, Andi
> 
> 在 2023/8/7 16:17, Andi Shyti 写道:
> > On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
> >> Hi, Andi
> >>
> >> 在 2023/8/5 6:16, Andi Shyti 写道:
> >>> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
> >>>> Use the dev_err_probe function instead of dev_err in the probe function
> >>>> so that the printed messge includes the return value and also handles
> >>>> -EPROBE_DEFER nicely.
> >>>>
> >>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >>>> ---
> >>>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
> >>>>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >>>> index c3287c887c6f..bfa788b3775b 100644
> >>>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> >>>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >>>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> >>>>  		sizeof(lpi2c_imx->adapter.name));
> >>>>  
> >>>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
> >>>> -	if (ret < 0) {
> >>>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
> >>>> -		return ret;
> >>>> -	}
> >>>> +	if (ret < 0)
> >>>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
> >>>
> >>> you cut on this because the line was going over 100 characters? :)
> >>>
> >>> In theory you shouldn't change the print message when doing such
> >>> changes and you can still split it as:
> >>>
> >>> 		return dev_err_probe(&pdev->dev, ret,
> >>> 				     "can't get I2C peripheral clock, ret=%d\n",
> >>> 				     ret);
> >>>
> >>> and you're even within the 80 characters.
> >>
> >> Since dev_err_probe always print the second parameter that happens to be the return value,
> >> I remove the "ret=%d" from the original message to avoid a redundant error message.
> >>
> >> So is it better to keep the original message unchanged, even though dev_err_probe also prints
> >> the return error value? Or is it better to make this change so that all error messages printed
> >> in the probe function include the return value in a consistent style?
> > 
> > yes, you are right! Then please ignore this comment, but...
> > 
> >>>   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> >>>   		pdev->name, lpi2c_imx);
> >>> - 	if (ret) {
> >>> - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> >>> - 		return ret;
> >>> - 	}
> >>> + 	if (ret)
> >>> + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
> > 
> > please make it coherent to this second part, as well, where the
> > error number is printed.
> 
> Do you mean to convert it to the following?
> 
>     if (ret)
>         return dev_err_probe(&pdev->dev, ret, "can't claim irq\n");
> 
> I understand that the style of error message printed by dev_err_probe is like
> "error [ERRNO]: [customized message]", the [ERRNO] comes from 2nd parameter,
> [customized message] comes from 3rd paramter, if the original [customized message]it
> also print ERRNO, i intend to remove it in this patch, otherwise, I will just keep it.
> In the above code, [customized message] intend to print irq but return value, so it is
> better to keep the original message, right?

sorry... I just got confused and read wrong the code. Please
ignore my comments on this patch, you are right here. Feel free
to add.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 8/9] i2c: imx-lpi2c: Use dev_err_probe in probe function

[Liao, Chang]

在 2023/8/7 19:55, Andi Shyti 写道:
> On Mon, Aug 07, 2023 at 06:44:23PM +0800, Liao, Chang wrote:
>> Hi, Andi
>>
>> 在 2023/8/7 16:17, Andi Shyti 写道:
>>> On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
>>>> Hi, Andi
>>>>
>>>> 在 2023/8/5 6:16, Andi Shyti 写道:
>>>>> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
>>>>>> Use the dev_err_probe function instead of dev_err in the probe function
>>>>>> so that the printed messge includes the return value and also handles
>>>>>> -EPROBE_DEFER nicely.
>>>>>>
>>>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>>>>> ---
>>>>>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>>>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>>>> index c3287c887c6f..bfa788b3775b 100644
>>>>>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>>>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>>>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>>>>>>  		sizeof(lpi2c_imx->adapter.name));
>>>>>>  
>>>>>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
>>>>>> -	if (ret < 0) {
>>>>>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
>>>>>> -		return ret;
>>>>>> -	}
>>>>>> +	if (ret < 0)
>>>>>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
>>>>>
>>>>> you cut on this because the line was going over 100 characters? :)
>>>>>
>>>>> In theory you shouldn't change the print message when doing such
>>>>> changes and you can still split it as:
>>>>>
>>>>> 		return dev_err_probe(&pdev->dev, ret,
>>>>> 				     "can't get I2C peripheral clock, ret=%d\n",
>>>>> 				     ret);
>>>>>
>>>>> and you're even within the 80 characters.
>>>>
>>>> Since dev_err_probe always print the second parameter that happens to be the return value,
>>>> I remove the "ret=%d" from the original message to avoid a redundant error message.
>>>>
>>>> So is it better to keep the original message unchanged, even though dev_err_probe also prints
>>>> the return error value? Or is it better to make this change so that all error messages printed
>>>> in the probe function include the return value in a consistent style?
>>>
>>> yes, you are right! Then please ignore this comment, but...
>>>
>>>>>   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
>>>>>   		pdev->name, lpi2c_imx);
>>>>> - 	if (ret) {
>>>>> - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
>>>>> - 		return ret;
>>>>> - 	}
>>>>> + 	if (ret)
>>>>> + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
>>>
>>> please make it coherent to this second part, as well, where the
>>> error number is printed.
>>
>> Do you mean to convert it to the following?
>>
>>     if (ret)
>>         return dev_err_probe(&pdev->dev, ret, "can't claim irq\n");
>>
>> I understand that the style of error message printed by dev_err_probe is like
>> "error [ERRNO]: [customized message]", the [ERRNO] comes from 2nd parameter,
>> [customized message] comes from 3rd paramter, if the original [customized message]it
>> also print ERRNO, i intend to remove it in this patch, otherwise, I will just keep it.
>> In the above code, [customized message] intend to print irq but return value, so it is
>> better to keep the original message, right?
> 
> sorry... I just got confused and read wrong the code. Please
> ignore my comments on this patch, you are right here. Feel free
> to add.

Thanks, I will add a bit more information to explain the changes made in these patches in v3.

> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 
> 
> Andi

-- 
BR
Liao, Chang