* [PATCH] i2c: designware: add reset interface @ 2016-11-22 4:41 Zhangfei Gao 2016-12-13 20:34 ` Wolfram Sang 2016-12-15 8:59 ` [PATCH v2] " Zhangfei Gao 0 siblings, 2 replies; 13+ messages in thread From: Zhangfei Gao @ 2016-11-22 4:41 UTC (permalink / raw) To: linux-arm-kernel Some platforms like hi3660 need do reset first to allow accessing registers Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++ 2 files changed, 6 insertions(+) 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..fd80e58 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,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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] i2c: designware: add reset interface 2016-11-22 4:41 [PATCH] i2c: designware: add reset interface Zhangfei Gao @ 2016-12-13 20:34 ` Wolfram Sang 2016-12-14 19:00 ` Andy Shevchenko 2016-12-15 8:59 ` [PATCH v2] " Zhangfei Gao 1 sibling, 1 reply; 13+ messages in thread From: Wolfram Sang @ 2016-12-13 20:34 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 22, 2016 at 12:41:40PM +0800, Zhangfei Gao wrote: > Some platforms like hi3660 need do reset first to allow accessing registers > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> Adding designware maintainers to CC... > --- > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++ > 2 files changed, 6 insertions(+) > > 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..fd80e58 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,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; > > -- > 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] i2c: designware: add reset interface 2016-12-13 20:34 ` Wolfram Sang @ 2016-12-14 19:00 ` Andy Shevchenko 2016-12-15 8:56 ` zhangfei 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2016-12-14 19:00 UTC (permalink / raw) To: linux-arm-kernel On Tue, 2016-12-13 at 21:34 +0100, Wolfram Sang wrote: > On Tue, Nov 22, 2016 at 12:41:40PM +0800, Zhangfei Gao wrote: > > Some platforms like hi3660 need do reset first to allow accessing > > registers > > > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > > Adding designware maintainers to CC... > > > --- > > ?drivers/i2c/busses/i2c-designware-core.h????| 1 + > > ?drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++ > > ?2 files changed, 6 insertions(+) > > > > 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..fd80e58 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,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); 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(). > > + > > ? /* fast mode by default because of legacy reasons */ > > ? dev->clk_freq = 400000; > > ? > > --? > > 2.7.4 > > -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] i2c: designware: add reset interface 2016-12-14 19:00 ` Andy Shevchenko @ 2016-12-15 8:56 ` zhangfei 0 siblings, 0 replies; 13+ messages in thread From: zhangfei @ 2016-12-15 8:56 UTC (permalink / raw) To: linux-arm-kernel On 2016?12?15? 03:00, Andy Shevchenko wrote: > On Tue, 2016-12-13 at 21:34 +0100, Wolfram Sang wrote: >> On Tue, Nov 22, 2016 at 12:41:40PM +0800, Zhangfei Gao wrote: >>> Some platforms like hi3660 need do reset first to allow accessing >>> registers >>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> Adding designware maintainers to CC... >> >>> --- >>> drivers/i2c/busses/i2c-designware-core.h | 1 + >>> drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++ >>> 2 files changed, 6 insertions(+) >>> >>> 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..fd80e58 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,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); > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-11-22 4:41 [PATCH] i2c: designware: add reset interface Zhangfei Gao 2016-12-13 20:34 ` Wolfram Sang @ 2016-12-15 8:59 ` Zhangfei Gao 2016-12-15 12:33 ` Andy Shevchenko 1 sibling, 1 reply; 13+ messages in thread From: Zhangfei Gao @ 2016-12-15 8:59 UTC (permalink / raw) To: linux-arm-kernel Some platforms like hi3660 need do reset first to allow accessing registers Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++++++++++++++++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) 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..e9016ae 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(&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; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-15 8:59 ` [PATCH v2] " Zhangfei Gao @ 2016-12-15 12:33 ` Andy Shevchenko 2016-12-15 14:11 ` Phil Reid 2016-12-15 15:30 ` Ramiro Oliveira 0 siblings, 2 replies; 13+ messages in thread From: Andy Shevchenko @ 2016-12-15 12:33 UTC (permalink / raw) To: linux-arm-kernel On Thu, 2016-12-15 at 16:59 +0800, Zhangfei Gao wrote: > 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 <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > --- > ?drivers/i2c/busses/i2c-designware-core.h????|??1 + > ?drivers/i2c/busses/i2c-designware-platdrv.c | 28 > ++++++++++++++++++++++++---- > ?2 files changed, 25 insertions(+), 4 deletions(-) > > 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..e9016ae 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(&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; > ?} -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-15 12:33 ` Andy Shevchenko @ 2016-12-15 14:11 ` Phil Reid 2016-12-15 15:40 ` Jarkko Nikula 2016-12-15 15:30 ` Ramiro Oliveira 1 sibling, 1 reply; 13+ messages in thread From: Phil Reid @ 2016-12-15 14:11 UTC (permalink / raw) To: linux-arm-kernel On 15/12/2016 20:33, Andy Shevchenko wrote: > On Thu, 2016-12-15 at 16:59 +0800, Zhangfei Gao wrote: >> 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 <andriy.shevchenko@linux.intel.com> > >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/i2c/busses/i2c-designware-core.h | 1 + >> drivers/i2c/busses/i2c-designware-platdrv.c | 28 >> ++++++++++++++++++++++++---- >> 2 files changed, 25 insertions(+), 4 deletions(-) >> >> 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..e9016ae 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(&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"); 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; } } if (hsotg->reset) reset_control_deassert(hsotg->reset); Regards Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-15 14:11 ` Phil Reid @ 2016-12-15 15:40 ` Jarkko Nikula 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Nikula @ 2016-12-15 15:40 UTC (permalink / raw) To: linux-arm-kernel On 12/15/2016 04:11 PM, Phil Reid wrote: > On 15/12/2016 20:33, Andy Shevchenko wrote: >> On Thu, 2016-12-15 at 16:59 +0800, Zhangfei Gao wrote: >>> 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 <andriy.shevchenko@linux.intel.com> >> >>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>> --- >>> drivers/i2c/busses/i2c-designware-core.h | 1 + >>> drivers/i2c/busses/i2c-designware-platdrv.c | 28 >>> ++++++++++++++++++++++++---- >>> 2 files changed, 25 insertions(+), 4 deletions(-) >>> >>> 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..e9016ae 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(&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"); > 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. -- Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-15 12:33 ` Andy Shevchenko 2016-12-15 14:11 ` Phil Reid @ 2016-12-15 15:30 ` Ramiro Oliveira 2016-12-16 2:45 ` zhangfei 1 sibling, 1 reply; 13+ messages in thread From: Ramiro Oliveira @ 2016-12-15 15:30 UTC (permalink / raw) To: linux-arm-kernel Hi Andy and Zhangfei On 12/15/2016 12:33 PM, Andy Shevchenko wrote: > On Thu, 2016-12-15 at 16:59 +0800, Zhangfei Gao wrote: >> 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 <andriy.shevchenko@linux.intel.com> > I tested the patch and it's working for the ARC architecture. >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/i2c/busses/i2c-designware-core.h | 1 + >> drivers/i2c/busses/i2c-designware-platdrv.c | 28 >> ++++++++++++++++++++++++---- >> 2 files changed, 25 insertions(+), 4 deletions(-) >> >> 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..e9016ae 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(&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. >> + 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; >> } > Tested-by: Ramiro Oliveira <ramiro.oliveira@synopsys.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-15 15:30 ` Ramiro Oliveira @ 2016-12-16 2:45 ` zhangfei 2016-12-16 3:01 ` zhangfei 0 siblings, 1 reply; 13+ messages in thread From: zhangfei @ 2016-12-16 2:45 UTC (permalink / raw) To: linux-arm-kernel On 2016?12?15? 23:30, Ramiro Oliveira wrote: > Hi Andy and Zhangfei > > On 12/15/2016 12:33 PM, Andy Shevchenko wrote: >> On Thu, 2016-12-15 at 16:59 +0800, Zhangfei Gao wrote: >>> 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 <andriy.shevchenko@linux.intel.com> >> > I tested the patch and it's working for the ARC architecture. > >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>> --- >>> drivers/i2c/busses/i2c-designware-core.h | 1 + >>> drivers/i2c/busses/i2c-designware-platdrv.c | 28 >>> ++++++++++++++++++++++++---- >>> 2 files changed, 25 insertions(+), 4 deletions(-) >>> >>> 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..e9016ae 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(&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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-16 2:45 ` zhangfei @ 2016-12-16 3:01 ` zhangfei 2016-12-23 10:26 ` Philipp Zabel 0 siblings, 1 reply; 13+ messages in thread From: zhangfei @ 2016-12-16 3:01 UTC (permalink / raw) To: linux-arm-kernel Hi, Philipp On 2016?12?16? 10:45, zhangfei wrote: > > > On 2016?12?15? 23:30, Ramiro Oliveira wrote: >> Hi Andy and Zhangfei >> >> On 12/15/2016 12:33 PM, Andy Shevchenko wrote: >>> On Thu, 2016-12-15 at 16:59 +0800, Zhangfei Gao wrote: >>>> 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 <andriy.shevchenko@linux.intel.com> >>> >> I tested the patch and it's working for the ARC architecture. >> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>>> --- >>>> drivers/i2c/busses/i2c-designware-core.h | 1 + >>>> drivers/i2c/busses/i2c-designware-platdrv.c | 28 >>>> ++++++++++++++++++++++++---- >>>> 2 files changed, 25 insertions(+), 4 deletions(-) >>>> >>>> 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..e9016ae 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(&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 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-16 3:01 ` zhangfei @ 2016-12-23 10:26 ` Philipp Zabel 2016-12-23 13:39 ` zhangfei 0 siblings, 1 reply; 13+ messages in thread From: Philipp Zabel @ 2016-12-23 10:26 UTC (permalink / raw) To: linux-arm-kernel Hi Zhangfei, Am Freitag, den 16.12.2016, 11:01 +0800 schrieb zhangfei: > Hi, Philipp > > On 2016?12?16? 10:45, zhangfei wrote: [...] > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: designware: add reset interface 2016-12-23 10:26 ` Philipp Zabel @ 2016-12-23 13:39 ` zhangfei 0 siblings, 0 replies; 13+ messages in thread From: zhangfei @ 2016-12-23 13:39 UTC (permalink / raw) To: linux-arm-kernel On 2016?12?23? 18:26, Philipp Zabel wrote: > Hi Zhangfei, > > Am Freitag, den 16.12.2016, 11:01 +0800 schrieb zhangfei: >> Hi, Philipp >> >> On 2016?12?16? 10:45, zhangfei wrote: > [...] >> 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. Thanks Philipp for the kind explanation. Will use devm_reset_control_get_optional_exclusive here. Thanks ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-12-23 13:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-22 4:41 [PATCH] i2c: designware: add reset interface Zhangfei Gao 2016-12-13 20:34 ` Wolfram Sang 2016-12-14 19:00 ` Andy Shevchenko 2016-12-15 8:56 ` zhangfei 2016-12-15 8:59 ` [PATCH v2] " Zhangfei Gao 2016-12-15 12:33 ` Andy Shevchenko 2016-12-15 14:11 ` Phil Reid 2016-12-15 15:40 ` Jarkko Nikula 2016-12-15 15:30 ` Ramiro Oliveira 2016-12-16 2:45 ` zhangfei 2016-12-16 3:01 ` zhangfei 2016-12-23 10:26 ` Philipp Zabel 2016-12-23 13:39 ` zhangfei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).