| Message ID | 1481792388-13781-1-git-send-email-zhangfei.gao@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html