| Message ID | 1482500451-30137-1-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, December 23, 2016 9:40:51 PM CET Zhangfei Gao wrote: > @@ -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. > @@ -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; > 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
On Wed, 2017-01-04 at 15:55 +0100, Arnd Bergmann wrote: > On Friday, December 23, 2016 9:40:51 PM CET Zhangfei Gao wrote: > > @@ -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. 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. > > > @@ -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; > > > > 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. > 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
On Wednesday, January 4, 2017 5:35:19 PM CET Andy Shevchenko wrote: > > > @@ -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; > > > > > > > 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. 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
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 0d44d2a..94b14fa 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -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; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 0b42a12..1fbe66f 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -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; }