[PATCH] i2c: designware: add reset interface

[PATCH] i2c: designware: add reset interface

[Zhangfei Gao]

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

[PATCH] i2c: designware: add reset interface

[Zhangfei Gao]

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

Re: [PATCH] i2c: designware: add reset interface

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] i2c: designware: add reset interface

[Wolfram Sang]

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>

Re: [PATCH] i2c: designware: add reset interface

[Andy Shevchenko]

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

[PATCH] i2c: designware: add reset interface

[Andy Shevchenko]

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

Re: [PATCH] i2c: designware: add reset interface

[zhangfei]

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

[PATCH] i2c: designware: add reset interface

[zhangfei]

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

[PATCH v2] i2c: designware: add reset interface

[Zhangfei Gao]

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

[PATCH v2] i2c: designware: add reset interface

[Zhangfei Gao]

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

Re: [PATCH v2] i2c: designware: add reset interface

[Andy Shevchenko]

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

[PATCH v2] i2c: designware: add reset interface

[Andy Shevchenko]

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

Re: [PATCH v2] i2c: designware: add reset interface

[Phil Reid]

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

[PATCH v2] i2c: designware: add reset interface

[Phil Reid]

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

Re: [PATCH v2] i2c: designware: add reset interface

[Ramiro Oliveira]

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>

[PATCH v2] i2c: designware: add reset interface

[Ramiro Oliveira]

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>

Re: [PATCH v2] i2c: designware: add reset interface

[Jarkko Nikula]

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

[PATCH v2] i2c: designware: add reset interface

[Jarkko Nikula]

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

Re: [PATCH v2] i2c: designware: add reset interface

[zhangfei]

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

[PATCH v2] i2c: designware: add reset interface

[zhangfei]

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

Re: [PATCH v2] i2c: designware: add reset interface

[zhangfei]

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

[PATCH v2] i2c: designware: add reset interface

[zhangfei]

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

Re: [PATCH v2] i2c: designware: add reset interface

[Philipp Zabel]

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

[PATCH v2] i2c: designware: add reset interface

[Philipp Zabel]

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

Re: [PATCH v2] i2c: designware: add reset interface

[zhangfei]

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

[PATCH v2] i2c: designware: add reset interface

[zhangfei]

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