[PATCH v3 0/4] i2c: designware: add i2c gpio recovery option

[PATCH v3 0/4] i2c: designware: add i2c gpio recovery option

[Phil Reid]

Changes from V1:
- In review Andy suggested change the i2c core to use the gpiod
  I've added a patch that allows the gradual switching of drivers 
  to using gpiod interface. The old interface is preserved so
  that changes can be made incrementally.
- I've update Tim's patch for the designware driver to use the new
  interface. Tweaked a couple of things to his patch and fixed
  up things Andy id in last review. 
  The core changes in p1 don't require the get/set scl/sda functions.
  Hopefully I've done the right thing with preserving authorship and
  signoff.

Changes from V2:
- Rebase on https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/
  i2c/for-next
  No intentional changes, but needed to move i2c_dw_plat_prepare_clk to common
  for the master recovery functions to use. which is included as two additional
  patches.


Phil Reid (4):
  i2c: Switch to using gpiod interface for gpio bus recovery
  i2c: designware: move i2c_dw_plat_prepare_clk to common
  i2c: designware: rename i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk

Tim Sander (1):
  i2c: designware: add i2c gpio recovery option

 drivers/i2c/busses/i2c-designware-common.c  | 24 ++++++++++--
 drivers/i2c/busses/i2c-designware-core.h    |  2 +
 drivers/i2c/busses/i2c-designware-master.c  | 57 +++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++-------
 drivers/i2c/i2c-core-base.c                 | 22 +++++++++--
 include/linux/i2c.h                         |  2 +
 6 files changed, 103 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/4] i2c: Switch to using gpiod interface for gpio bus recovery

[Phil Reid]

Currently the i2c gpio recovery code uses gpio integer interface
instead of the gpiod. This change switch the core code to use
the gpiod while still retaining compatibility with the gpio integer
interface. This will allow individual driver to be updated and tested
individual to switch to using the gpiod interface.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++----
 include/linux/i2c.h         |  2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 56e4658..3f54e25 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -133,17 +133,17 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 /* i2c bus recovery routines */
 static int get_scl_gpio_value(struct i2c_adapter *adap)
 {
-	return gpio_get_value(adap->bus_recovery_info->scl_gpio);
+	return gpiod_get_value_cansleep(adap->bus_recovery_info->scl_gpiod);
 }
 
 static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
 {
-	gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
+	gpiod_set_value_cansleep(adap->bus_recovery_info->scl_gpiod, val);
 }
 
 static int get_sda_gpio_value(struct i2c_adapter *adap)
 {
-	return gpio_get_value(adap->bus_recovery_info->sda_gpio);
+	return gpiod_get_value_cansleep(adap->bus_recovery_info->sda_gpiod);
 }
 
 static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
@@ -158,6 +158,7 @@ static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
 		dev_warn(dev, "Can't get SCL gpio: %d\n", bri->scl_gpio);
 		return ret;
 	}
+	bri->scl_gpiod = gpio_to_desc(bri->scl_gpio);
 
 	if (bri->get_sda) {
 		if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
@@ -167,6 +168,7 @@ static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
 			bri->get_sda = NULL;
 		}
 	}
+	bri->sda_gpiod = gpio_to_desc(bri->sda_gpio);
 
 	return ret;
 }
@@ -175,10 +177,13 @@ static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
 {
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 
-	if (bri->get_sda)
+	if (bri->get_sda) {
 		gpio_free(bri->sda_gpio);
+		bri->sda_gpiod = NULL;
+	}
 
 	gpio_free(bri->scl_gpio);
+	bri->scl_gpiod = NULL;
 }
 
 /*
@@ -272,6 +277,15 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 		goto err;
 	}
 
+	if ((bri->scl_gpiod) &&
+	    (bri->recover_bus == i2c_generic_scl_recovery)) {
+		bri->get_scl = get_scl_gpio_value;
+		bri->set_scl = set_scl_gpio_value;
+		if (bri->sda_gpiod)
+			bri->get_sda = get_sda_gpio_value;
+		return;
+	}
+
 	/* Generic GPIO recovery */
 	if (bri->recover_bus == i2c_generic_gpio_recovery) {
 		if (!gpio_is_valid(bri->scl_gpio)) {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index d501d39..7ad68a1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -511,6 +511,8 @@ struct i2c_bus_recovery_info {
 	/* gpio recovery */
 	int scl_gpio;
 	int sda_gpio;
+	struct gpio_desc *scl_gpiod;
+	struct gpio_desc *sda_gpiod;
 };
 
 int i2c_recover_bus(struct i2c_adapter *adap);
-- 
1.8.3.1

Re: [PATCH v3 1/4] i2c: Switch to using gpiod interface for gpio bus recovery

[Andy Shevchenko]

On Wed, 2017-08-30 at 14:17 +0800, Phil Reid wrote:
> Currently the i2c gpio recovery code uses gpio integer interface
> instead of the gpiod. This change switch the core code to use
> the gpiod while still retaining compatibility with the gpio integer
> interface. This will allow individual driver to be updated and tested
> individual to switch to using the gpiod interface.

>  static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> @@ -158,6 +158,7 @@ static int i2c_get_gpios_for_recovery(struct
> i2c_adapter *adap)
>  		dev_warn(dev, "Can't get SCL gpio: %d\n", bri-
> >scl_gpio);
>  		return ret;
>  	}
> +	bri->scl_gpiod = gpio_to_desc(bri->scl_gpio);
>  
>  	if (bri->get_sda) {
>  		if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-
> sda")) {
> @@ -167,6 +168,7 @@ static int i2c_get_gpios_for_recovery(struct
> i2c_adapter *adap)
>  			bri->get_sda = NULL;
>  		}
>  	}

> +	bri->sda_gpiod = gpio_to_desc(bri->sda_gpio);

Shouldn't it be inside conditional?

>  	return ret;
>  }
> @@ -175,10 +177,13 @@ static void i2c_put_gpios_for_recovery(struct
> i2c_adapter *adap)
>  {
>  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>  
> -	if (bri->get_sda)
> +	if (bri->get_sda) {
>  		gpio_free(bri->sda_gpio);
> +		bri->sda_gpiod = NULL;
> +	}
>  
>  	gpio_free(bri->scl_gpio);
> +	bri->scl_gpiod = NULL;

Can we go other way around, i.e. put descriptors and assign plain
integers to something like -ENOENT?

>  }


> +	if ((bri->scl_gpiod) &&

Redundant parens.

> +	    (bri->recover_bus == i2c_generic_scl_recovery)) {

Ditto, though here it might be slightly better to read.

> +		bri->get_scl = get_scl_gpio_value;
> +		bri->set_scl = set_scl_gpio_value;
> +		if (bri->sda_gpiod)
> +			bri->get_sda = get_sda_gpio_value;
> +		return;
> +	}

>  	int scl_gpio;
>  	int sda_gpio;
> +	struct gpio_desc *scl_gpiod;
> +	struct gpio_desc *sda_gpiod;

I think we even could get rid of plain integers completely.
In case some call needs it we can derive it still from the descriptor.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 1/4] i2c: Switch to using gpiod interface for gpio bus recovery

[Jarkko Nikula]

On 09/28/2017 01:44 PM, Andy Shevchenko wrote:
>> interface. This will allow individual driver to be updated and tested
>> individual to switch to using the gpiod interface.
> 
>>   	int scl_gpio;
>>   	int sda_gpio;
>> +	struct gpio_desc *scl_gpiod;
>> +	struct gpio_desc *sda_gpiod;
> 
> I think we even could get rid of plain integers completely.
> In case some call needs it we can derive it still from the descriptor.
> 
I guess it's still worth to split that into multiple patches and those 
driver conversion and integer removal can be follow up patches somewhere 
in the future?

-- 
Jarkko

Re: [PATCH v3 1/4] i2c: Switch to using gpiod interface for gpio bus recovery

[Andy Shevchenko]

On Thu, 2017-09-28 at 13:54 +0300, Jarkko Nikula wrote:
> On 09/28/2017 01:44 PM, Andy Shevchenko wrote:
> > > interface. This will allow individual driver to be updated and
> > > tested
> > > individual to switch to using the gpiod interface.
> > >   	int scl_gpio;
> > >   	int sda_gpio;
> > > +	struct gpio_desc *scl_gpiod;
> > > +	struct gpio_desc *sda_gpiod;
> > 
> > I think we even could get rid of plain integers completely.
> > In case some call needs it we can derive it still from the
> > descriptor.
> > 
> 
> I guess it's still worth to split that into multiple patches and
> those 
> driver conversion and integer removal can be follow up patches
> somewhere 
> in the future?

I didn't check and my memory is clean about users.
So, if there are users of those integers outside of I2C core, definitely
we need more patches / iterations.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 1/4] i2c: Switch to using gpiod interface for gpio bus recovery

[Phil Reid]

On 28/09/2017 18:58, Andy Shevchenko wrote:
> On Thu, 2017-09-28 at 13:54 +0300, Jarkko Nikula wrote:
>> On 09/28/2017 01:44 PM, Andy Shevchenko wrote:
>>>> interface. This will allow individual driver to be updated and
>>>> tested
>>>> individual to switch to using the gpiod interface.
>>>>    	int scl_gpio;
>>>>    	int sda_gpio;
>>>> +	struct gpio_desc *scl_gpiod;
>>>> +	struct gpio_desc *sda_gpiod;
>>>
>>> I think we even could get rid of plain integers completely.
>>> In case some call needs it we can derive it still from the
>>> descriptor.
>>>
>>
>> I guess it's still worth to split that into multiple patches and
>> those
>> driver conversion and integer removal can be follow up patches
>> somewhere
>> in the future?
> 
> I didn't check and my memory is clean about users.
> So, if there are users of those integers outside of I2C core, definitely
> we need more patches / iterations.
> 
There's a couple of drivers using those.
If my approach looks good then I can try and update those as well.

Question is does it all need to be in the one patch series?


-- 
Regards
Phil Reid

Re: [PATCH v3 1/4] i2c: Switch to using gpiod interface for gpio bus recovery

[Andy Shevchenko]

On Fri, 2017-09-29 at 14:59 +0800, Phil Reid wrote:
> On 28/09/2017 18:58, Andy Shevchenko wrote:
> > On Thu, 2017-09-28 at 13:54 +0300, Jarkko Nikula wrote:

> > I didn't check and my memory is clean about users.
> > So, if there are users of those integers outside of I2C core,
> > definitely
> > we need more patches / iterations.
> > 
> 
> There's a couple of drivers using those.
> If my approach looks good then I can try and update those as well.
> 
> Question is does it all need to be in the one patch series?

Depends on your choice.
Either will work (at least for me).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 2/4] i2c: designware: move i2c_dw_plat_prepare_clk to common

[Phil Reid]

Move the i2c_dw_plat_prepare_clk funciton to common file in preparation
for its use also by the master driver.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/busses/i2c-designware-common.c  | 13 +++++++++++++
 drivers/i2c/busses/i2c-designware-core.h    |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c | 12 ------------
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index d1a6937..b79f342 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -21,6 +21,7 @@
  * ----------------------------------------------------------------------------
  *
  */
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/errno.h>
@@ -185,6 +186,18 @@ unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
 	return dev->get_clk_rate_khz(dev);
 }
 
+int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
+{
+	if (IS_ERR(i_dev->clk))
+		return PTR_ERR(i_dev->clk);
+
+	if (prepare)
+		return clk_prepare_enable(i_dev->clk);
+
+	clk_disable_unprepare(i_dev->clk);
+	return 0;
+}
+
 int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 {
 	int ret;
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9fee4c0..b2a31cb 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -299,6 +299,7 @@ struct dw_i2c_dev {
 void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable);
 void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable);
 unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev);
+int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare);
 int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
 void i2c_dw_release_lock(struct dw_i2c_dev *dev);
 int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 56a17fd..14e7fab 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -214,18 +214,6 @@ static void i2c_dw_configure_slave(struct dw_i2c_dev *dev)
 	}
 }
 
-static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
-{
-	if (IS_ERR(i_dev->clk))
-		return PTR_ERR(i_dev->clk);
-
-	if (prepare)
-		return clk_prepare_enable(i_dev->clk);
-
-	clk_disable_unprepare(i_dev->clk);
-	return 0;
-}
-
 static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
 {
 	u32 param, tx_fifo_depth, rx_fifo_depth;
-- 
1.8.3.1

Re: [PATCH v3 2/4] i2c: designware: move i2c_dw_plat_prepare_clk to common

[Jarkko Nikula]

On 08/30/2017 09:17 AM, Phil Reid wrote:
> Move the i2c_dw_plat_prepare_clk funciton to common file in preparation
> for its use also by the master driver.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>   drivers/i2c/busses/i2c-designware-common.c  | 13 +++++++++++++
>   drivers/i2c/busses/i2c-designware-core.h    |  1 +
>   drivers/i2c/busses/i2c-designware-platdrv.c | 12 ------------
>   3 files changed, 14 insertions(+), 12 deletions(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH v3 3/4] i2c: designware: rename i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk

[Phil Reid]

For consistency with the reset of the file rename function and parameter to
be consistent with the reset of the common file.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/busses/i2c-designware-common.c  | 10 +++++-----
 drivers/i2c/busses/i2c-designware-core.h    |  2 +-
 drivers/i2c/busses/i2c-designware-platdrv.c |  6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index b79f342..3d684c6 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -186,15 +186,15 @@ unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
 	return dev->get_clk_rate_khz(dev);
 }
 
-int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
+int i2c_dw_prepare_clk(struct dw_i2c_dev *dev, bool prepare)
 {
-	if (IS_ERR(i_dev->clk))
-		return PTR_ERR(i_dev->clk);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
 
 	if (prepare)
-		return clk_prepare_enable(i_dev->clk);
+		return clk_prepare_enable(dev->clk);
 
-	clk_disable_unprepare(i_dev->clk);
+	clk_disable_unprepare(dev->clk);
 	return 0;
 }
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index b2a31cb..fef44b5 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -299,7 +299,7 @@ struct dw_i2c_dev {
 void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable);
 void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable);
 unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev);
-int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare);
+int i2c_dw_prepare_clk(struct dw_i2c_dev *dev, bool prepare);
 int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
 void i2c_dw_release_lock(struct dw_i2c_dev *dev);
 int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 14e7fab..58f55a8 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -325,7 +325,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		i2c_dw_configure_master(dev);
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
-	if (!i2c_dw_plat_prepare_clk(dev, true)) {
+	if (!i2c_dw_prepare_clk(dev, true)) {
 		dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 
 		if (!dev->sda_hold_time && ht)
@@ -422,7 +422,7 @@ static int dw_i2c_plat_runtime_suspend(struct device *dev)
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
 	i_dev->disable(i_dev);
-	i2c_dw_plat_prepare_clk(i_dev, false);
+	i2c_dw_prepare_clk(i_dev, false);
 
 	return 0;
 }
@@ -431,7 +431,7 @@ static int dw_i2c_plat_resume(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
-	i2c_dw_plat_prepare_clk(i_dev, true);
+	i2c_dw_prepare_clk(i_dev, true);
 	i_dev->init(i_dev);
 
 	return 0;
-- 
1.8.3.1

Re: [PATCH v3 3/4] i2c: designware: rename i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk

[Jarkko Nikula]

On 08/30/2017 09:17 AM, Phil Reid wrote:
> For consistency with the reset of the file rename function and parameter to
> be consistent with the reset of the common file.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>   drivers/i2c/busses/i2c-designware-common.c  | 10 +++++-----
>   drivers/i2c/busses/i2c-designware-core.h    |  2 +-
>   drivers/i2c/busses/i2c-designware-platdrv.c |  6 +++---
>   3 files changed, 9 insertions(+), 9 deletions(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH v3 4/4] i2c: designware: add i2c gpio recovery option

[Phil Reid]

From: Tim Sander <tim@krieglstein.org>

This patch contains much input from Phil Reid and has been tested
on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
SCL and SDA GPIO's. I am still a little unsure about the recover
in the timeout case (i2c-designware-core.c:770) as i could not
test this codepath.

Signed-off-by: Tim Sander <tim@krieglstein.org>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/busses/i2c-designware-common.c | 11 ++++--
 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 3d684c6..e3120987 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -230,7 +230,11 @@ int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 	while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
 		if (timeout <= 0) {
 			dev_warn(dev->dev, "timeout waiting for bus ready\n");
-			return -ETIMEDOUT;
+			i2c_recover_bus(&dev->adapter);
+
+			if (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY)
+				return -ETIMEDOUT;
+			return 0;
 		}
 		timeout--;
 		usleep_range(1000, 1100);
@@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
 	for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
 		dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
 
-	if (abort_source & DW_IC_TX_ARB_LOST)
+	if (abort_source & DW_IC_TX_ARB_LOST) {
+		i2c_recover_bus(&dev->adapter);
 		return -EAGAIN;
-	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+	} else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
 		return -EINVAL; /* wrong msgs[] data */
 	else
 		return -EIO;
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index fef44b5..8707c76 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -284,6 +284,7 @@ struct dw_i2c_dev {
 	void			(*disable_int)(struct dw_i2c_dev *dev);
 	int			(*init)(struct dw_i2c_dev *dev);
 	int			mode;
+	struct i2c_bus_recovery_info rinfo;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 418c233..07e34fe 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -25,11 +25,13 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/export.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 #include "i2c-designware-core.h"
 
@@ -443,6 +445,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	if (!wait_for_completion_timeout(&dev->cmd_complete, adap->timeout)) {
 		dev_err(dev->dev, "controller timed out\n");
 		/* i2c_dw_init implicitly disables the adapter */
+		i2c_recover_bus(&dev->adapter);
 		i2c_dw_init_master(dev);
 		ret = -ETIMEDOUT;
 		goto done;
@@ -613,6 +616,57 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void i2c_dw_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	i2c_dw_disable(dev);
+	reset_control_assert(dev->rst);
+	i2c_dw_prepare_clk(dev, false);
+}
+
+static void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	i2c_dw_prepare_clk(dev, true);
+	reset_control_deassert(dev->rst);
+	i2c_dw_init_master(dev);
+}
+
+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
+{
+	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+	struct i2c_adapter *adap = &dev->adapter;
+	struct gpio_desc *gpio;
+	int r;
+
+	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio)) {
+		r = PTR_ERR(gpio);
+		if ((r == -ENOENT) || (r == -ENOENT))
+			return 0;
+		return r;
+	}
+	rinfo->scl_gpiod = gpio;
+
+	gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	rinfo->sda_gpiod = gpio;
+
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	rinfo->prepare_recovery = i2c_dw_prepare_recovery;
+	rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
+	adap->bus_recovery_info = rinfo;
+
+	dev_info(dev->dev,
+		"adapter: %s running with gpio recovery mode! scl:%i sda:%i\n",
+		adap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod);
+
+	return 0;
+}
+
 int i2c_dw_probe(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
@@ -664,6 +718,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 		dev_err(dev->dev, "failure adding adapter: %d\n", ret);
 	pm_runtime_put_noidle(dev->dev);
 
+	if (!ret)
+		ret = i2c_dw_init_recovery_info(dev);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(i2c_dw_probe);
-- 
1.8.3.1

Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option

[Andy Shevchenko]

On Wed, 2017-08-30 at 14:17 +0800, Phil Reid wrote:
> From: Tim Sander <tim@krieglstein.org>
> 
> This patch contains much input from Phil Reid and has been tested
> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
> SCL and SDA GPIO's. I am still a little unsure about the recover
> in the timeout case (i2c-designware-core.c:770) as i could not
> test this codepath.

 
> -	if (abort_source & DW_IC_TX_ARB_LOST)
> +	if (abort_source & DW_IC_TX_ARB_LOST) {
> +		i2c_recover_bus(&dev->adapter);
>  		return -EAGAIN;

> -	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
> +	} else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)

else is redundant.

>  		return -EINVAL; /* wrong msgs[] data */
>  	else

Ditto.

>  		return -EIO;

> +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +	struct i2c_adapter *adap = &dev->adapter;
> +	struct gpio_desc *gpio;
> +	int r;
> +
> +	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpio)) {
> +		r = PTR_ERR(gpio);

> +		if ((r == -ENOENT) || (r == -ENOENT))

Copy'n'paste typo?

> +			return 0;
> +		return r;
> +	}
> +	rinfo->scl_gpiod = gpio;
> +
> +	gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
> +	if (IS_ERR(gpio))
> +		return PTR_ERR(gpio);
> +	rinfo->sda_gpiod = gpio;
> +
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	rinfo->prepare_recovery = i2c_dw_prepare_recovery;
> +	rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
> +	adap->bus_recovery_info = rinfo;
> +

> +	dev_info(dev->dev,
> +		"adapter: %s running with gpio recovery mode! scl:%i
> sda:%i\n",
> +		adap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod);

Instead of doing numbers, better just to list available descriptors,
e.g.

...("... %s scl\n", rinfo->sda_gpiod ? "sda,");

No need to explain that scl doesn't need any check here.

And I'm not sure why do you need adap->name here. Can you show an
example of output from your test platform?

> +	if (!ret)
> +		ret = i2c_dw_init_recovery_info(dev);

Better to 
if (ret)
 return ret;

return i2c...();

> +
>  	return ret;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option

[Phil Reid]

Thanks for the review.

On 28/09/2017 18:58, Andy Shevchenko wrote:
> On Wed, 2017-08-30 at 14:17 +0800, Phil Reid wrote:
>> From: Tim Sander <tim@krieglstein.org>
>>
>> This patch contains much input from Phil Reid and has been tested
>> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
>> SCL and SDA GPIO's. I am still a little unsure about the recover
>> in the timeout case (i2c-designware-core.c:770) as i could not
>> test this codepath.
> 
>   
>> -	if (abort_source & DW_IC_TX_ARB_LOST)
>> +	if (abort_source & DW_IC_TX_ARB_LOST) {
>> +		i2c_recover_bus(&dev->adapter);
>>   		return -EAGAIN;
> 
>> -	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
>> +	} else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
> 
> else is redundant.
> 
>>   		return -EINVAL; /* wrong msgs[] data */
>>   	else
> 
> Ditto.
Yep.
> 
>>   		return -EIO;
> 
>> +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
>> +{
>> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>> +	struct i2c_adapter *adap = &dev->adapter;
>> +	struct gpio_desc *gpio;
>> +	int r;
>> +
>> +	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(gpio)) {
>> +		r = PTR_ERR(gpio);
> 
>> +		if ((r == -ENOENT) || (r == -ENOENT))
> 
> Copy'n'paste typo?
Yep.

> 
>> +			return 0;
>> +		return r;
>> +	}
>> +	rinfo->scl_gpiod = gpio;
>> +
>> +	gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
>> +	if (IS_ERR(gpio))
>> +		return PTR_ERR(gpio);
>> +	rinfo->sda_gpiod = gpio;
>> +
>> +	rinfo->recover_bus = i2c_generic_scl_recovery;
>> +	rinfo->prepare_recovery = i2c_dw_prepare_recovery;
>> +	rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
>> +	adap->bus_recovery_info = rinfo;
>> +
> 
>> +	dev_info(dev->dev,
>> +		"adapter: %s running with gpio recovery mode! scl:%i
>> sda:%i\n",
>> +		adap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod);
> 
> Instead of doing numbers, better just to list available descriptors,
> e.g.
> 
> ...("... %s scl\n", rinfo->sda_gpiod ? "sda,");
Ok.

> 
> No need to explain that scl doesn't need any check here.
> 
> And I'm not sure why do you need adap->name here. Can you show an
> example of output from your test platform?

Good question. can't see a need.

> 
>> +	if (!ret)
>> +		ret = i2c_dw_init_recovery_info(dev);
> 
> Better to
> if (ret)
>   return ret;
> 
> return i2c...();
> 
>> +
>>   	return ret;
> 


-- 
Regards
Phil Reid

Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option

[Jarkko Nikula]

On 08/30/2017 09:17 AM, Phil Reid wrote:
> From: Tim Sander <tim@krieglstein.org>
> 
> This patch contains much input from Phil Reid and has been tested
> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
> SCL and SDA GPIO's. I am still a little unsure about the recover
> in the timeout case (i2c-designware-core.c:770) as i could not
> test this codepath.
> 
> Signed-off-by: Tim Sander <tim@krieglstein.org>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>   drivers/i2c/busses/i2c-designware-common.c | 11 ++++--
>   drivers/i2c/busses/i2c-designware-core.h   |  1 +
>   drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++
>   3 files changed, 66 insertions(+), 3 deletions(-)
> 
While taking into account Andy's comments please modify the last 
sentence in the above commit log - i2c-designware-core.c doesn't exist 
anymore. Maybe better is to have the uncertainty documented as a 
"REVISIT:" comment in the code etc.

> @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
>   	for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
>   		dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
>   
> -	if (abort_source & DW_IC_TX_ARB_LOST)
> +	if (abort_source & DW_IC_TX_ARB_LOST) {
> +		i2c_recover_bus(&dev->adapter);

Are you sure about doing recovery for arbitration lost case? To me it 
seems wrong to do it if another master is accessing the bus.

-- 
Jarkko

Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option

[Phil Reid]

On 28/09/2017 21:21, Jarkko Nikula wrote:
> On 08/30/2017 09:17 AM, Phil Reid wrote:
>> From: Tim Sander <tim@krieglstein.org>
>>
>> This patch contains much input from Phil Reid and has been tested
>> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
>> SCL and SDA GPIO's. I am still a little unsure about the recover
>> in the timeout case (i2c-designware-core.c:770) as i could not
>> test this codepath.
>>
>> Signed-off-by: Tim Sander <tim@krieglstein.org>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>   drivers/i2c/busses/i2c-designware-common.c | 11 ++++--
>>   drivers/i2c/busses/i2c-designware-core.h   |  1 +
>>   drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++
>>   3 files changed, 66 insertions(+), 3 deletions(-)
>>
> While taking into account Andy's comments please modify the last sentence in the above commit log - i2c-designware-core.c doesn't exist anymore. Maybe better is 
> to have the uncertainty documented as a "REVISIT:" comment in the code etc.
> 
>> @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
>>       for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
>>           dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
>> -    if (abort_source & DW_IC_TX_ARB_LOST)
>> +    if (abort_source & DW_IC_TX_ARB_LOST) {
>> +        i2c_recover_bus(&dev->adapter);
> 
> Are you sure about doing recovery for arbitration lost case? To me it seems wrong to do it if another master is accessing the bus.
> 
yes I think your right.

-- 
Regards
Phil Reid

Re: [PATCH v3 0/4] i2c: designware: add i2c gpio recovery option

[Phil Reid]

On 30/08/2017 14:17, Phil Reid wrote:
> Changes from V1:
> - In review Andy suggested change the i2c core to use the gpiod
>    I've added a patch that allows the gradual switching of drivers
>    to using gpiod interface. The old interface is preserved so
>    that changes can be made incrementally.
> - I've update Tim's patch for the designware driver to use the new
>    interface. Tweaked a couple of things to his patch and fixed
>    up things Andy id in last review.
>    The core changes in p1 don't require the get/set scl/sda functions.
>    Hopefully I've done the right thing with preserving authorship and
>    signoff.
> 
> Changes from V2:
> - Rebase on https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/
>    i2c/for-next
>    No intentional changes, but needed to move i2c_dw_plat_prepare_clk to common
>    for the master recovery functions to use. which is included as two additional
>    patches.
> 
> 
> Phil Reid (4):
>    i2c: Switch to using gpiod interface for gpio bus recovery
>    i2c: designware: move i2c_dw_plat_prepare_clk to common
>    i2c: designware: rename i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk
> 
> Tim Sander (1):
>    i2c: designware: add i2c gpio recovery option
> 
>   drivers/i2c/busses/i2c-designware-common.c  | 24 ++++++++++--
>   drivers/i2c/busses/i2c-designware-core.h    |  2 +
>   drivers/i2c/busses/i2c-designware-master.c  | 57 +++++++++++++++++++++++++++++
>   drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++-------
>   drivers/i2c/i2c-core-base.c                 | 22 +++++++++--
>   include/linux/i2c.h                         |  2 +
>   6 files changed, 103 insertions(+), 22 deletions(-)
> 

Any comments on this series?

-- 
Regards
Phil Reid

Re: [PATCH v3 0/4] i2c: designware: add i2c gpio recovery option

[Jarkko Nikula]

On 09/28/2017 10:37 AM, Phil Reid wrote:
> On 30/08/2017 14:17, Phil Reid wrote:
>> Changes from V1:
>> - In review Andy suggested change the i2c core to use the gpiod
>>    I've added a patch that allows the gradual switching of drivers
>>    to using gpiod interface. The old interface is preserved so
>>    that changes can be made incrementally.
>> - I've update Tim's patch for the designware driver to use the new
>>    interface. Tweaked a couple of things to his patch and fixed
>>    up things Andy id in last review.
>>    The core changes in p1 don't require the get/set scl/sda functions.
>>    Hopefully I've done the right thing with preserving authorship and
>>    signoff.
>>
>> Changes from V2:
>> - Rebase on 
>> https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/
>>    i2c/for-next
>>    No intentional changes, but needed to move i2c_dw_plat_prepare_clk 
>> to common
>>    for the master recovery functions to use. which is included as two 
>> additional
>>    patches.
>>
>>
>> Phil Reid (4):
>>    i2c: Switch to using gpiod interface for gpio bus recovery
>>    i2c: designware: move i2c_dw_plat_prepare_clk to common
>>    i2c: designware: rename i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk
>>
>> Tim Sander (1):
>>    i2c: designware: add i2c gpio recovery option
>>
>>   drivers/i2c/busses/i2c-designware-common.c  | 24 ++++++++++--
>>   drivers/i2c/busses/i2c-designware-core.h    |  2 +
>>   drivers/i2c/busses/i2c-designware-master.c  | 57 
>> +++++++++++++++++++++++++++++
>>   drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++-------
>>   drivers/i2c/i2c-core-base.c                 | 22 +++++++++--
>>   include/linux/i2c.h                         |  2 +
>>   6 files changed, 103 insertions(+), 22 deletions(-)
>>
> 
> Any comments on this series?
> 
Thanks for reminder. This got buried under other activity... 
i2c-designware changes look ok at quick look.

-- 
Jarkko

Re: [PATCH v3 0/4] i2c: designware: add i2c gpio recovery option

[Andy Shevchenko]

On Thu, 2017-09-28 at 15:37 +0800, Phil Reid wrote:
> On 30/08/2017 14:17, Phil Reid wrote:
> > Changes from V1:
> > - In review Andy suggested change the i2c core to use the gpiod
> >    I've added a patch that allows the gradual switching of drivers
> >    to using gpiod interface. The old interface is preserved so
> >    that changes can be made incrementally.
> > - I've update Tim's patch for the designware driver to use the new
> >    interface. Tweaked a couple of things to his patch and fixed
> >    up things Andy id in last review.
> >    The core changes in p1 don't require the get/set scl/sda
> > functions.
> >    Hopefully I've done the right thing with preserving authorship
> > and
> >    signoff.

+Cc: Ferry.

Ferry, I think you might be interested in this series.
If so, perhaps Phil can include you in Cc for v4 of it.

> > 
> > Changes from V2:
> > - Rebase on https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linu
> > x.git/
> >    i2c/for-next
> >    No intentional changes, but needed to move
> > i2c_dw_plat_prepare_clk to common
> >    for the master recovery functions to use. which is included as
> > two additional
> >    patches.
> > 
> > 
> > Phil Reid (4):
> >    i2c: Switch to using gpiod interface for gpio bus recovery
> >    i2c: designware: move i2c_dw_plat_prepare_clk to common
> >    i2c: designware: rename i2c_dw_plat_prepare_clk to
> > i2c_dw_prepare_clk
> > 
> > Tim Sander (1):
> >    i2c: designware: add i2c gpio recovery option
> > 
> >   drivers/i2c/busses/i2c-designware-common.c  | 24 ++++++++++--
> >   drivers/i2c/busses/i2c-designware-core.h    |  2 +
> >   drivers/i2c/busses/i2c-designware-master.c  | 57
> > +++++++++++++++++++++++++++++
> >   drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++-------
> >   drivers/i2c/i2c-core-base.c                 | 22 +++++++++--
> >   include/linux/i2c.h                         |  2 +
> >   6 files changed, 103 insertions(+), 22 deletions(-)
> > 
> 
> Any comments on this series?
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/4] i2c: designware: add i2c gpio recovery option

[Ferry Toth]

Andy Shevchenko schreef op do 28-09-2017 om 13:55 [+0300]:
> On Thu, 2017-09-28 at 15:37 +0800, Phil Reid wrote:
> > On 30/08/2017 14:17, Phil Reid wrote:
> > > Changes from V1:
> > > - In review Andy suggested change the i2c core to use the gpiod
> > >    I've added a patch that allows the gradual switching of
> > > drivers
> > >    to using gpiod interface. The old interface is preserved so
> > >    that changes can be made incrementally.
> > > - I've update Tim's patch for the designware driver to use the
> > > new
> > >    interface. Tweaked a couple of things to his patch and fixed
> > >    up things Andy id in last review.
> > >    The core changes in p1 don't require the get/set scl/sda
> > > functions.
> > >    Hopefully I've done the right thing with preserving authorship
> > > and
> > >    signoff.
> 
> +Cc: Ferry.
> 
> Ferry, I think you might be interested in this series.
> If so, perhaps Phil can include you in Cc for v4 of it.

Thanks. I am interested in this, though I may be experiencing a
somewhat different issue.

I my case (Intel Edison on Arduino expansion board) there are 4 MUX
attached to the same i2c bus. On warm boot sometimes the first device
is not detected (as shown by i2c-detect). The 2nd - 4th devices are
detected fine.

It would seem that initially the bus is stuck accessing the first
device and recovers after this. But this is probably not the case, as
on successive warm reboots, the same first device remains undetected.

A cold boot fixes this.

To me that looks like the bus is not stuck, but the particular slave is
in the wrong state. 

I have not yet been able to figure out if this patch series only resets
a device when it holds the bus, or if it also resets a device that
doesn't respond or responds with garbled data, but I will look into it.

Up to now I have not found a reliable way to trigger the situation,
other than noting it seems to happen mostly after a kernel or u-boot
reflash. 

> > > 
> > > Changes from V2:
> > > - Rebase on https://git.kernel.org/pub/scm/linux/kernel/git/wsa/l
> > > inu
> > > x.git/
> > >    i2c/for-next
> > >    No intentional changes, but needed to move
> > > i2c_dw_plat_prepare_clk to common
> > >    for the master recovery functions to use. which is included as
> > > two additional
> > >    patches.
> > > 
> > > 
> > > Phil Reid (4):
> > >    i2c: Switch to using gpiod interface for gpio bus recovery
> > >    i2c: designware: move i2c_dw_plat_prepare_clk to common
> > >    i2c: designware: rename i2c_dw_plat_prepare_clk to
> > > i2c_dw_prepare_clk
> > > 
> > > Tim Sander (1):
> > >    i2c: designware: add i2c gpio recovery option
> > > 
> > >   drivers/i2c/busses/i2c-designware-common.c  | 24 ++++++++++--
> > >   drivers/i2c/busses/i2c-designware-core.h    |  2 +
> > >   drivers/i2c/busses/i2c-designware-master.c  | 57
> > > +++++++++++++++++++++++++++++
> > >   drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++-------
> > >   drivers/i2c/i2c-core-base.c                 | 22 +++++++++--
> > >   include/linux/i2c.h                         |  2 +
> > >   6 files changed, 103 insertions(+), 22 deletions(-)
> > > 
> > 
> > Any comments on this series?
> > 
> 
>