Skip to contentSkip to conversation

i2c: designware: add reset interface

PATCH Superseded Zhangfei Gao 3 participants 4 messages Dec 23, 2016 – Jan 6, 2017

Subsystems (3)
I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver
lore
Share & tools
lore

Zhangfei Gao

author
#0 lore raw

PATCH v3 1/1 i2c: designware: add reset interface

Some platforms like hi3660 need do reset first to allow accessing registers

Signed-off-by Zhangfei Gao <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
Tested-by Ramiro Oliveira <[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.h    |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware-core.h +1 -0
index 0d44d2a..94b14fa 100644
@@ -80,6 +80,7 @@ struct dw_i2c_dev {
 	void __iomem		*base;
 	struct completion	cmd_complete;
 	struct clk		*clk;
+	struct reset_control	*rst;
 	u32			(*get_clk_rate_khz) (struct dw_i2c_dev *dev);
 	struct dw_pci_controller *controller;
 	int			cmd_err;
drivers/i2c/busses/i2c-designware-platdrv.c +24 -4
index 0b42a12..1fbe66f 100644
@@ -38,6 +38,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/io.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/platform_data/i2c-designware.h>

@@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
+	dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(dev->rst)) {
+		if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	} else {
+		reset_control_deassert(dev->rst);
+	}
+
 	/* fast mode by default because of legacy reasons */
 	dev->clk_freq = 400000;
 

@@ -207,12 +216,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	    && dev->clk_freq != 1000000 && dev->clk_freq != 3400000) {
 		dev_err(&pdev->dev,
 			"Only 100kHz, 400kHz, 1MHz and 3.4MHz supported");
-		return -EINVAL;
+		r = -EINVAL;
+		goto exit_reset;
 	}
 
 	r = i2c_dw_eval_lock_support(dev);
 	if (r)
-		return r;
+		goto exit_reset;
 
 	dev->functionality =
 		I2C_FUNC_I2C |

@@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	if (r)
+		goto exit_probe;
 
 	return r;
+
+exit_probe:
+	if (!dev->pm_runtime_disabled)
+		pm_runtime_disable(&pdev->dev);
+exit_reset:
+	if (!IS_ERR_OR_NULL(dev->rst))
+		reset_control_assert(dev->rst);
+	return r;
 }
 
 static int dw_i2c_plat_remove(struct platform_device *pdev)

@@ -290,6 +308,8 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 	pm_runtime_put_sync(&pdev->dev);
 	if (!dev->pm_runtime_disabled)
 		pm_runtime_disable(&pdev->dev);
+	if (!IS_ERR_OR_NULL(dev->rst))
+		reset_control_assert(dev->rst);
 
 	return 0;
 }
Signature · 1 line
2.7.4
#1 lore raw

PATCH v3 1/1 i2c: designware: add reset interface

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 187

Quoted 12 lines · CET Zhangfei Gao
drivers/i2c/busses/i2c-designware-platdrv.c +8 -0
@@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
        dev->irq = irq;
        platform_set_drvdata(pdev, dev);
 
+       dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+       if (IS_ERR(dev->rst)) {
+               if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
+                       return -EPROBE_DEFER;
+       } else {
+               reset_control_deassert(dev->rst);
+       }
+

Sorry for the late reply, I only now stumbled over this. I think it's generally wrong to ignore any error aside from -EPROBE_DEFER. It's better to single-out the error conditions you want to ignore (e.g. no reset specified) and ignore those but return an error for all other problems.

Quoted 9 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2 -2
@@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
        }
 
        r = i2c_dw_probe(dev);
-       if (r && !dev->pm_runtime_disabled)
-               pm_runtime_disable(&pdev->dev);
+       if (r)
+               goto exit_probe;

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 295

Quoted 10 lines
drivers/i2c/busses/i2c-designware-platdrv.c +8 -0
        return r;
+
+exit_probe:
+       if (!dev->pm_runtime_disabled)
+               pm_runtime_disable(&pdev->dev);
+exit_reset:
+       if (!IS_ERR_OR_NULL(dev->rst))
+               reset_control_assert(dev->rst);
+       return r;

try to avoid the IS_ERR_OR_NULL() check, it usually indicates either a bad interface, or that the interface is used wrong.

In this case, I think we can't get here with a NULL dev->rst pointer, so it's better to only check IS_ERR, or to explicitly set the pointer to NULL in case there is no reset line.

Arnd

reviewer
#2 lore raw

PATCH v3 1/1 i2c: designware: add reset interface

Quoted 16 lines · Arnd Bergmann
Quoted 6 lines · CET Zhangfei Gao
(quoted diff) +0 -0
@@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
 ????????dev->irq = irq;
 ????????platform_set_drvdata(pdev, dev);

?
+???????dev->rst = devm_reset_control_get_optional_exclusive(&pdev-

Quoted 8 lines
Quoted 1 line

dev, NULL);

+???????if (IS_ERR(dev->rst)) { +???????????????if (PTR_ERR(dev->rst) == -EPROBE_DEFER) +???????????????????????return -EPROBE_DEFER; +???????} else { +???????????????reset_control_deassert(dev->rst); +???????}
+

Quoted 5 lines

Sorry for the late reply, I only now stumbled over this. I think it's generally wrong to ignore any error aside from -EPROBE_DEFER. It's better to single-out the error conditions you want to ignore (e.g. no reset specified) and ignore those but return an error for all other problems.

Which means that reset framework whenever work _optional is used should return error iff (mind two f:s) there is a problem with existing control.

Quoted 16 lines
Quoted 16 lines
(quoted diff) +0 -0
@@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
 ????????}

?
????????r = i2c_dw_probe(dev);
-???????if (r && !dev->pm_runtime_disabled)
-???????????????pm_runtime_disable(&pdev->dev);
+???????if (r)
+???????????????goto exit_probe;
?
????????return r;

drivers/i2c/busses/i2c-designware-platdrv.c +4 -0
+
+exit_probe:
+???????if (!dev->pm_runtime_disabled)
+???????????????pm_runtime_disable(&pdev->dev);
Quoted 8 lines
Quoted 5 lines

+exit_reset:
+???????if (!IS_ERR_OR_NULL(dev->rst))
+???????????????reset_control_assert(dev->rst);
+???????return r;

try to avoid the IS_ERR_OR_NULL() check, it usually indicates either a bad interface, or that the interface is used wrong.

Please, fix reset framework first than.

For my understanding:
It should return NULL for optional reset control.
It should not fail on NULL argument.

Quoted 5 lines

In this case, I think we can't get here with a NULL dev->rst pointer, so it's better to only check IS_ERR, or to explicitly set the pointer to NULL in case there is no reset line.

Arnd

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
#3 lore raw

PATCH v3 1/1 i2c: designware: add reset interface

Quoted 24 lines · CET Andy Shevchenko
Quoted 10 lines
Quoted 10 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2 -2
@@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
        }
 
        r = i2c_dw_probe(dev);
-       if (r && !dev->pm_runtime_disabled)
-               pm_runtime_disable(&pdev->dev);
+       if (r)
+               goto exit_probe;
Quoted 13 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 296

Quoted 10 lines
drivers/i2c/busses/i2c-designware-platdrv.c +8 -0
        return r;
+
+exit_probe:
+       if (!dev->pm_runtime_disabled)
+               pm_runtime_disable(&pdev->dev);
+exit_reset:
+       if (!IS_ERR_OR_NULL(dev->rst))
+               reset_control_assert(dev->rst);
+       return r;

try to avoid the IS_ERR_OR_NULL() check, it usually indicates either a bad interface, or that the interface is used wrong.

Quoted 5 lines

Please, fix reset framework first than.

For my understanding:
It should return NULL for optional reset control.
It should not fail on NULL argument.

I think we discussed that a few times. Your suggestion makes sense to me, but I don't know why we don't already do that, maybe there is a good reason.

Arnd