i2c: designware: add reset interface
Subsystems (3)
PATCH v3 1/1 i2c: designware: add reset interface
Some platforms like hi3660 need do reset first to allow accessing registers
--- 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
@@ -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
@@ -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
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
-0return 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
Andy Shevchenko
reviewerPATCH 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)
+???????????????gotoexit_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->rstpointer, so it's better to only checkIS_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
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
-0return 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