i2c: designware: add reset interface

PATCH New Zhangfei Gao 7 participants 13 messages Nov 22, 2016 – Dec 23, 2016

Subsystems (3)

I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver

lore

Share & tools

lore

Zhangfei Gao

author

2016-11-22 04:41 UTC #0 lore raw

PATCH 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]>

---
 drivers/i2c/busses/i2c-designware-core.h    | 1 +
 drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++
 2 files changed, 6 insertions(+)

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 +5 -0

index 0b42a12..fd80e58 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,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
+	dev->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (!IS_ERR(dev->rst))
+		reset_control_reset(dev->rst);
+
 	/* fast mode by default because of legacy reasons */
 	dev->clk_freq = 400000;

Signature · 1 line

2.7.4

Wolfram Sang

2016-12-13 20:34 UTC #1 lore raw

PATCH 1/1 i2c: designware: add reset interface

Quoted 3 lines · Zhangfei Gao

Some platforms like hi3660 need do reset first to allow accessing registers
Signed-off-by Zhangfei Gao <[email protected]>

Adding designware maintainers to CC...

Quoted 43 lines

---
 drivers/i2c/busses/i2c-designware-core.h    | 1 +
 drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++
 2 files changed, 6 insertions(+)

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 +5 -0

index 0b42a12..fd80e58 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,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
+	dev->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (!IS_ERR(dev->rst))
+		reset_control_reset(dev->rst);
+
 	/* fast mode by default because of legacy reasons */
 	dev->clk_freq = 400000;

Signature · 1 line

2.7.4

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161213/c10a0a77/attachment.sig>

Andy Shevchenko

reviewer

2016-12-14 19:00 UTC #2 lore raw

PATCH 1/1 i2c: designware: add reset interface

Quoted 40 lines · Wolfram Sang

Quoted 4 lines · Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Signed-off-by Zhangfei Gao <[email protected]>
Adding designware maintainers to CC...
Quoted 32 lines
---
?drivers/i2c/busses/i2c-designware-core.h????| 1 + ?drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++ ?2 files changed, 6 insertions(+)
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 +0 -0
index 0b42a12..fd80e58 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>

Quoted 8 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 182
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
@@ -176,6 +177,10 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
 ?	dev->irq = irq;
 ?	platform_set_drvdata(pdev, dev);
?
drivers/i2c/busses/i2c-designware-platdrv.c +3 -0
+	dev->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (!IS_ERR(dev->rst))
+		reset_control_reset(dev->rst);

Do we care about EPROBE_DEFER?

Perhaps on error path we need to assert it.

And I guess it should be devm_reset_control_get_optional().

Quoted 7 lines

Quoted 7 lines
+
? /* fast mode by default because of legacy reasons */
? dev->clk_freq = 400000;
?
--?
2.7.4

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

zhangfei

author

2016-12-15 08:56 UTC #3 lore raw

PATCH 1/1 i2c: designware: add reset interface

Quoted 45 lines · Andy Shevchenko

Quoted 44 lines · Wolfram Sang

Quoted 4 lines · Zhangfei Gao

Some platforms like hi3660 need do reset first to allow accessing registers
Signed-off-by Zhangfei Gao <[email protected]>

Adding designware maintainers to CC...

Quoted 37 lines

---
  drivers/i2c/busses/i2c-designware-core.h    | 1 +
  drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++
  2 files changed, 6 insertions(+)

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 +1 -0

index 0b42a12..fd80e58 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,10 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
  	dev->irq = irq;
  	platform_set_drvdata(pdev, dev);

Quoted 8 lines

Quoted 3 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 182
Quoted 3 lines
drivers/i2c/busses/i2c-designware-platdrv.c +3 -0
+	dev->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (!IS_ERR(dev->rst))
+		reset_control_reset(dev->rst);
Do we care about EPROBE_DEFER?
Perhaps on error path we need to assert it.
And I guess it should be devm_reset_control_get_optional().

Thanks Andy
Good suggestion, will update accordingly.

Thanks

Zhangfei Gao

author

2016-12-15 08:59 UTC #4 lore raw

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

In reply to Zhangfei Gao

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

Signed-off-by Zhangfei Gao <[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..e9016ae 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(&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

Andy Shevchenko

reviewer

2016-12-15 12:33 UTC #5 lore raw

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

Quoted 2 lines · Zhangfei Gao

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

Patch itself looks good, but would be nice to have it tested.

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 102 lines

Signed-off-by Zhangfei Gao <[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 +0 -0
index 0b42a12..e9016ae 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>
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
@@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
 ?	dev->irq = irq;
 ?	platform_set_drvdata(pdev, dev);
?
drivers/i2c/busses/i2c-designware-platdrv.c +8 -0
+	dev->rst = devm_reset_control_get_optional(&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;
?
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
@@ -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");
drivers/i2c/busses/i2c-designware-platdrv.c +2 -1
-		return -EINVAL;
+		r = -EINVAL;
+		goto exit_reset;
? }
?
? r = i2c_dw_eval_lock_support(dev);
? if (r)
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
-		return r;
+		goto exit_reset;
?
? dev->functionality =
? I2C_FUNC_I2C |
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
@@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
 ?	}
?
? r = i2c_dw_probe(dev);
drivers/i2c/busses/i2c-designware-platdrv.c +2 -2
-	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 +8 -0
+
+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)
drivers/i2c/busses/i2c-designware-platdrv.c +2 -0
@@ -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 · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Jarkko Nikula

2016-12-15 15:40 UTC #7 lore raw

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

Quoted 66 lines · Phil Reid

Quoted 48 lines · Andy Shevchenko
Quoted 2 lines · Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Patch itself looks good, but would be nice to have it tested.
Reviewed-by Andy Shevchenko <[email protected]>
Quoted 40 lines
Signed-off-by Zhangfei Gao <[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 +1 -0
index 0b42a12..e9016ae 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);
 
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 187
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +8 -0
+    dev->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
+    if (IS_ERR(dev->rst)) {
+        if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+    } else {
+        reset_control_deassert(dev->rst);
+    }
+
More for my education. But some drivers seem to handle the error codes a little more explicitly.
Whats the best approach?
eg: From usb/dwc2 driver it continues only if ENOENT / ENOTSUPP errors are return
and ENOMEM / EINVAL etc is a fatal error.
hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");

Quoted 12 lines

if (IS_ERR(hsotg->reset)) {
        ret = PTR_ERR(hsotg->reset);
        switch (ret) {
        case -ENOENT:
        case -ENOTSUPP:
                hsotg->reset = NULL;
                break;
        default:
                dev_err(hsotg->dev, "error getting reset control %d\n",
                        ret);
                return ret;
        }

This looks a bit extreme. At least we shouldn't spam log on machines that don't need reset control. I kind of think it's good enough to do like the patch does. I.e. handle only EPROBE_DEFER and let all other errors fall through and keep the controller in reset and let the dev->rst carry an error code.

I guess EINVAL is likely seen by developer only. ENOMEM is so fatal that things are already falling apart somewhere else too and I don't think it needs special handling here.

Signature · 1 line

Jarkko

Ramiro Oliveira

2016-12-15 15:30 UTC #8 lore raw

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

In reply to Andy Shevchenko

Hi Andy and Zhangfei

Quoted 8 lines · Andy Shevchenko

Quoted 2 lines · Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Patch itself looks good, but would be nice to have it tested.
Reviewed-by Andy Shevchenko <[email protected]>

I tested the patch and it's working for the ARC architecture.

Quoted 40 lines

Signed-off-by Zhangfei Gao <[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;

drivers/i2c/busses/i2c-designware-core.h +0 -0

 	int			cmd_err;

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

index 0b42a12..e9016ae 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);

Quoted 1 line

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 180
Quoted 1 line
drivers/i2c/busses/i2c-designware-platdrv.c +1 -0
+	dev->rst = devm_reset_control_get_optional(&pdev->dev, NULL);

devm_reset_control_get_optional() is deprecated as explained in linux/reset.h, you should use devm_reset_control_get_optional_exclusive() or devm_reset_control_get_optional_shared() instead, as applicable.

I submitted a similar patch earlier today and I made the same mistake.

Quoted 59 lines

drivers/i2c/busses/i2c-designware-platdrv.c +10 -2

+	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;

drivers/i2c/busses/i2c-designware-platdrv.c +10 -2

 	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)

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

@@ -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);

Quoted 3 lines

Quoted 2 lines
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
 	return 0;
}

Tested-by Ramiro Oliveira <[email protected]>

zhangfei

author

2016-12-16 02:45 UTC #9 lore raw

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

Quoted 51 lines · Ramiro Oliveira

Hi Andy and Zhangfei
Quoted 7 lines · Andy Shevchenko
Quoted 2 lines · Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Patch itself looks good, but would be nice to have it tested.
Reviewed-by Andy Shevchenko <[email protected]>
I tested the patch and it's working for the ARC architecture.
Quoted 39 lines
Quoted 39 lines
Signed-off-by Zhangfei Gao <[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 +1 -0
index 0b42a12..e9016ae 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);
 

Quoted 6 lines

Quoted 1 line
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 180
Quoted 1 line
drivers/i2c/busses/i2c-designware-platdrv.c +1 -0
+	dev->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
devm_reset_control_get_optional() is deprecated as explained in linux/reset.h, you should use devm_reset_control_get_optional_exclusive() or devm_reset_control_get_optional_shared() instead, as applicable.
I submitted a similar patch earlier today and I made the same mistake.

Thanks Ramiro for the info
Will use devm_reset_control_get_optional_exclusive instead. But should the interface be as simple as possible?

Thanks

zhangfei

author

2016-12-16 03:01 UTC #10 lore raw

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

Hi, Philipp

Quoted 59 lines · zhangfei

Quoted 56 lines · Ramiro Oliveira
Hi Andy and Zhangfei
Quoted 7 lines · Andy Shevchenko
Quoted 2 lines · Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Patch itself looks good, but would be nice to have it tested.
Reviewed-by Andy Shevchenko <[email protected]>
I tested the patch and it's working for the ARC architecture.
Quoted 31 lines
Quoted 31 lines
Signed-off-by Zhangfei Gao <[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 +1 -0
index 0b42a12..e9016ae 100644
@@ -38,6 +38,7 @@
  #include <linux/pm_runtime.h>
  #include <linux/property.h>
  #include <linux/io.h>
+#include <linux/reset.h>
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct · line 180
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
  #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(&pdev->dev, NULL);
devm_reset_control_get_optional() is deprecated as explained in linux/reset.h,
you should use devm_reset_control_get_optional_exclusive() or devm_reset_control_get_optional_shared() instead, as applicable.

Quoted 7 lines

Quoted 1 line
I submitted a similar patch earlier today and I made the same mistake.
Thanks Ramiro for the info
Will use devm_reset_control_get_optional_exclusive instead. But should the interface be as simple as possible?
Thanks

Sorry, a bit confused.
Is that mean we should not use devm_reset_control_get_optional() While driver should decide whether use devm_reset_control_get_optional_exclusive() or devm_reset_control_get_optional_shared()
What if different platform has different requirement?

Looks the difference between _exclusive and _shared is refcount, How about handle this inside, and not decided by interface?

Thanks

Philipp Zabel

2016-12-23 10:26 UTC #11 lore raw

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

Hi Zhangfei,

Am Freitag, den 16.12.2016, 11:01 +0800 schrieb zhangfei:

Quoted 3 lines

Hi, Philipp
On 2016?12?16? 10:45, zhangfei wrote:

[...]

Quoted 9 lines

Sorry, a bit confused.
Is that mean we should not use devm_reset_control_get_optional() While driver should decide whether use devm_reset_control_get_optional_exclusive() or devm_reset_control_get_optional_shared()
What if different platform has different requirement?
Looks the difference between _exclusive and _shared is refcount, How about handle this inside, and not decided by interface?

Because there is a significant difference in how the actual reset line behaves. The shared resets are clock-like, and should only be used if the device needs them to be deasserted to be enabled, but is fine if they have been deasserted earlier or if they are not immediately asserted after the device is disabled, but some time later. For the shared / non-exclusive resets calling reset_control_assert and then reset_control_deassert is not guaranteed to do anything at all, because another user of the reset line could keep it deasserted.

If the device needs a reset to be issued at a certain point in time on the other hand, for example to reset its state machine or registers to a known good state, calling assert must guarantee to actually assert the reset. This can only be done if the reset is exclusive.

Whether guaranteed asserts (exclusive reset lines) are necessary depends on the device, so this decision has to be made in the driver.

regards
Philipp

zhangfei

author

2016-12-23 13:39 UTC #12 lore raw

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

Quoted 25 lines · Philipp Zabel

Hi Zhangfei,
Am Freitag, den 16.12.2016, 11:01 +0800 schrieb zhangfei:
Quoted 3 lines
Hi, Philipp
On 2016?12?16? 10:45, zhangfei wrote:
[...]
Quoted 9 lines
Sorry, a bit confused.
Is that mean we should not use devm_reset_control_get_optional() While driver should decide whether use devm_reset_control_get_optional_exclusive() or devm_reset_control_get_optional_shared()
What if different platform has different requirement?
Looks the difference between _exclusive and _shared is refcount, How about handle this inside, and not decided by interface?
Because there is a significant difference in how the actual reset line behaves. The shared resets are clock-like, and should only be used if the device needs them to be deasserted to be enabled, but is fine if they have been deasserted earlier or if they are not immediately asserted after the device is disabled, but some time later. For the shared / non-exclusive resets calling reset_control_assert and then reset_control_deassert is not guaranteed to do anything at all, because another user of the reset line could keep it deasserted.

Quoted 7 lines

If the device needs a reset to be issued at a certain point in time on the other hand, for example to reset its state machine or registers to a known good state, calling assert must guarantee to actually assert the reset. This can only be done if the reset is exclusive.
Whether guaranteed asserts (exclusive reset lines) are necessary depends on the device, so this decision has to be made in the driver.

Thanks Philipp for the kind explanation.
Will use devm_reset_control_get_optional_exclusive here.

Thanks