diff mbox

[05/12] i2c: pxa: Add bus reset functionality

Message ID 1432818224-17070-6-git-send-email-vaibhav.hiremath@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Vaibhav Hiremath May 28, 2015, 1:03 p.m. UTC
From: Rob Herring <robh@kernel.org>

Since there is some problematic i2c slave devices on some
platforms such as dkb (sometimes), it will drop down sda
and make i2c bus hang, at that time, it need to config
scl/sda into gpio to simulate "stop" sequence to recover
i2c bus, so add this interface.

Signed-off-by: Leilei Shang <shangll@marvell.com>
Signed-off-by: Rob Herring <robh@kernel.org>
[vaibhav.hiremath@linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

Comments

Rob Herring (Arm) May 29, 2015, 1:59 p.m. UTC | #1
On Thu, May 28, 2015 at 8:03 AM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:
> From: Rob Herring <robh@kernel.org>

This probably should still be Leilei, but...

> Since there is some problematic i2c slave devices on some
> platforms such as dkb (sometimes), it will drop down sda
> and make i2c bus hang, at that time, it need to config
> scl/sda into gpio to simulate "stop" sequence to recover
> i2c bus, so add this interface.
>
> Signed-off-by: Leilei Shang <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath@linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 8ca5552..eb09071 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -37,6 +37,8 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/i2c/pxa-i2c.h>
> +#include <linux/of_gpio.h>
> +#include <linux/pinctrl/consumer.h>
>
>  #include <asm/irq.h>
>
> @@ -177,6 +179,9 @@ struct pxa_i2c {
>         bool                    highmode_enter;
>         unsigned int            ilcr;
>         unsigned int            iwcr;
> +       struct pinctrl          *pinctrl;
> +       struct pinctrl_state    *pin_i2c;
> +       struct pinctrl_state    *pin_gpio;
>  };
>
>  #define _IBMR(i2c)     ((i2c)->reg_ibmr)
> @@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
>
>  #define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
>
> +static void i2c_bus_reset(struct pxa_i2c *i2c)

There's a generic mechanism in i2c_generic_gpio_recovery we should use
here. It appears to be similar, but not exactly the same. The pinctrl
part should probably be done by gpio driver.

Rob
Vaibhav Hiremath May 29, 2015, 3:40 p.m. UTC | #2
On Friday 29 May 2015 07:29 PM, Rob Herring wrote:
> On Thu, May 28, 2015 at 8:03 AM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>> From: Rob Herring <robh@kernel.org>
>
> This probably should still be Leilei, but...
>

Ok, Since I am taking forward from your sign-off, did not change it.
Anyway will fix in next version.

>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>>
>> Signed-off-by: Leilei Shang <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath@linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index 8ca5552..eb09071 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -37,6 +37,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/io.h>
>>   #include <linux/i2c/pxa-i2c.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/pinctrl/consumer.h>
>>
>>   #include <asm/irq.h>
>>
>> @@ -177,6 +179,9 @@ struct pxa_i2c {
>>          bool                    highmode_enter;
>>          unsigned int            ilcr;
>>          unsigned int            iwcr;
>> +       struct pinctrl          *pinctrl;
>> +       struct pinctrl_state    *pin_i2c;
>> +       struct pinctrl_state    *pin_gpio;
>>   };
>>
>>   #define _IBMR(i2c)     ((i2c)->reg_ibmr)
>> @@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
>>
>>   #define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
>>
>> +static void i2c_bus_reset(struct pxa_i2c *i2c)
>
> There's a generic mechanism in i2c_generic_gpio_recovery we should use
> here. It appears to be similar, but not exactly the same. The pinctrl
> part should probably be done by gpio driver.
>


Good point.

As you mentioned, they are not exactly same.

But we can achieve exactly what we wanted using prepare & unprepare
callbacks.
Generate stop signal in unprepare() callback should suffice our need.


But I am not quite sure about pinctrl handling by gpio driver.
I was tracing the calls from gpio_request_one()anf gpio_free()
but it seems they are not bringing back pins to default state.

correct me if I am wrong.

Thanks,
Vaibhav
Robert Jarzmik May 29, 2015, 8:31 p.m. UTC | #3
Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> From: Rob Herring <robh@kernel.org>
>
> Since there is some problematic i2c slave devices on some
> platforms such as dkb (sometimes), it will drop down sda
> and make i2c bus hang, at that time, it need to config
> scl/sda into gpio to simulate "stop" sequence to recover
> i2c bus, so add this interface.
>
> Signed-off-by: Leilei Shang <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath@linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
I'm not enthusiastic about this patch.
Linus, would you please place a comment on this patch ?

My personal feeling :
 - all the udelays are just ugly
 - the pinctrl binding into i2c-pxa doesn't look good to me
   Linus will probably comment on that

 - all of this patch looks like a workaround for a single platform "poor"
   support. If this is dkb specific (what is dkb BTW ?), can't it be hooked
   into this driver, but placed in dkb platform code ?

Cheers.

--
Robert

> ---
>  drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 8ca5552..eb09071 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -37,6 +37,8 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/i2c/pxa-i2c.h>
> +#include <linux/of_gpio.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <asm/irq.h>
>  
> @@ -177,6 +179,9 @@ struct pxa_i2c {
>  	bool			highmode_enter;
>  	unsigned int		ilcr;
>  	unsigned int		iwcr;
> +	struct pinctrl		*pinctrl;
> +	struct pinctrl_state	*pin_i2c;
> +	struct pinctrl_state	*pin_gpio;
>  };
>  
>  #define _IBMR(i2c)	((i2c)->reg_ibmr)
> @@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
>  
>  #define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
>  
> +static void i2c_bus_reset(struct pxa_i2c *i2c)
> +{
> +	int ret, ccnt, pins_scl, pins_sda;
> +	struct device *dev = i2c->adap.dev.parent;
> +	struct device_node *np = dev->of_node;
> +
> +	if (!i2c->pinctrl) {
> +		dev_warn(dev, "could not do i2c bus reset\n");
> +		return;
> +	}
> +
> +	ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
> +	if (ret) {
> +		dev_err(dev, "could not set gpio pins\n");
> +		return;
> +	}
> +
> +	pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
> +	if (!gpio_is_valid(pins_scl)) {
> +		dev_err(dev, "i2c scl gpio not set\n");
> +		goto err_out;
> +	}
> +	pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
> +	if (!gpio_is_valid(pins_sda)) {
> +		dev_err(dev, "i2c sda gpio not set\n");
> +		goto err_out;
> +	}
> +
> +	gpio_request(pins_scl, NULL);
> +	gpio_request(pins_sda, NULL);
> +
> +	gpio_direction_input(pins_sda);
> +	for (ccnt = 20; ccnt; ccnt--) {
> +		gpio_direction_output(pins_scl, ccnt & 0x01);
> +		udelay(5);
> +	}
> +	gpio_direction_output(pins_scl, 0);
> +	udelay(5);
> +	gpio_direction_output(pins_sda, 0);
> +	udelay(5);
> +	/* stop signal */
> +	gpio_direction_output(pins_scl, 1);
> +	udelay(5);
> +	gpio_direction_output(pins_sda, 1);
> +	udelay(5);
> +
> +	gpio_free(pins_scl);
> +	gpio_free(pins_sda);
> +
> +err_out:
> +	ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
> +	if (ret)
> +		dev_err(dev, "could not set default(i2c) pins\n");
> +	return;
> +}
> +
>  static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
>  {
>  	unsigned int i;
> @@ -281,6 +342,11 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
>  	for (i = 0; i < i2c->irqlogidx; i++)
>  		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
>  	printk("\n");
> +	if (strcmp(why, "exhausted retries") != 0) {
> +		i2c_bus_reset(i2c);
> +		/* reset i2c contorler when it's fail */
> +		i2c_pxa_reset(i2c);
> +	}
>  }
>  
>  #else /* ifdef DEBUG */
> @@ -1301,6 +1367,30 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  
>  	platform_set_drvdata(dev, i2c);
>  
> +	i2c->pinctrl = devm_pinctrl_get(&dev->dev);
> +	if (IS_ERR(i2c->pinctrl)) {
> +		i2c->pinctrl = NULL;
> +		dev_warn(&dev->dev, "could not get pinctrl\n");
> +	} else {
> +		i2c->pin_i2c = pinctrl_lookup_state(i2c->pinctrl, "default");
> +		if (IS_ERR(i2c->pin_i2c)) {
> +			dev_err(&dev->dev, "could not get default(i2c) pinstate\n");
> +			ret = IS_ERR(i2c->pin_i2c);
> +		}
> +
> +		i2c->pin_gpio = pinctrl_lookup_state(i2c->pinctrl, "gpio");
> +		if (IS_ERR(i2c->pin_gpio)) {
> +			dev_err(&dev->dev, "could not get gpio pinstate\n");
> +			ret = IS_ERR(i2c->pin_gpio);
> +		}
> +
> +		if (ret) {
> +			i2c->pin_i2c = NULL;
> +			i2c->pin_gpio = NULL;
> +			i2c->pinctrl = NULL;
> +			ret = 0;
> +		}
> +	}
>  #ifdef CONFIG_I2C_PXA_SLAVE
>  	printk(KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n",
>  	       dev_name(&i2c->adap.dev), i2c->slave_addr);
Linus Walleij June 2, 2015, 1:12 p.m. UTC | #4
On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:

> From: Rob Herring <robh@kernel.org>
>
> Since there is some problematic i2c slave devices on some
> platforms such as dkb (sometimes), it will drop down sda
> and make i2c bus hang, at that time, it need to config
> scl/sda into gpio to simulate "stop" sequence to recover
> i2c bus, so add this interface.
>
> Signed-off-by: Leilei Shang <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath@linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Double signed-off?

(...)
+#include <linux/of_gpio.h>

You should use <linux/gpio/consumer.h> and then use
GPIO descriptors instead.

> @@ -177,6 +179,9 @@ struct pxa_i2c {
>         bool                    highmode_enter;
>         unsigned int            ilcr;
>         unsigned int            iwcr;
> +       struct pinctrl          *pinctrl;
> +       struct pinctrl_state    *pin_i2c;
> +       struct pinctrl_state    *pin_gpio;

Yup that's the right way. I see this is the "default"
state for pin_i2c.

> +static void i2c_bus_reset(struct pxa_i2c *i2c)
> +{
> +       int ret, ccnt, pins_scl, pins_sda;

Use GPIO descriptors.

struct gpio_desc *scl, *sda;

> +       struct device *dev = i2c->adap.dev.parent;
> +       struct device_node *np = dev->of_node;
> +
> +       if (!i2c->pinctrl) {
> +               dev_warn(dev, "could not do i2c bus reset\n");
> +               return;
> +       }
> +
> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
> +       if (ret) {
> +               dev_err(dev, "could not set gpio pins\n");
> +               return;
> +       }

Exactly like that yes.

> +       pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
> +       if (!gpio_is_valid(pins_scl)) {
> +               dev_err(dev, "i2c scl gpio not set\n");
> +               goto err_out;
> +       }
> +       pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
> +       if (!gpio_is_valid(pins_sda)) {
> +               dev_err(dev, "i2c sda gpio not set\n");
> +               goto err_out;
> +       }

I would suggest just using two GPIOs in the node relying
on index order. With GPIO descriptors:

scl = gpiod_get_index(dev, "i2c-gpio", 0, GPIOD_ASIS);
sda = gpiod_get_index(dev, "i2c-gpio", 1, GPIOD_ASIS);

Then use gpiod* accessors below and...

> +
> +       gpio_request(pins_scl, NULL);
> +       gpio_request(pins_sda, NULL);
> +
> +       gpio_direction_input(pins_sda);
> +       for (ccnt = 20; ccnt; ccnt--) {
> +               gpio_direction_output(pins_scl, ccnt & 0x01);
> +               udelay(5);
> +       }
> +       gpio_direction_output(pins_scl, 0);
> +       udelay(5);
> +       gpio_direction_output(pins_sda, 0);
> +       udelay(5);
> +       /* stop signal */
> +       gpio_direction_output(pins_scl, 1);
> +       udelay(5);
> +       gpio_direction_output(pins_sda, 1);
> +       udelay(5);
> +
> +       gpio_free(pins_scl);
> +       gpio_free(pins_sda);

gpiod_put(scl);
gpiod_put(sda);

> +err_out:
> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
> +       if (ret)
> +               dev_err(dev, "could not set default(i2c) pins\n");
> +       return;

Nice.

Overall it looks like a real nice structured workaround using
the API exactly as intended, just need to catch up with
using GPIO descriptors.

Yours,
Linus Walleij
Vaibhav Hiremath June 2, 2015, 4:40 p.m. UTC | #5
On Tuesday 02 June 2015 06:42 PM, Linus Walleij wrote:
> On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>>
>> Signed-off-by: Leilei Shang <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath@linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Double signed-off?
>
> (...)
> +#include <linux/of_gpio.h>
>
> You should use <linux/gpio/consumer.h> and then use
> GPIO descriptors instead.
>
>> @@ -177,6 +179,9 @@ struct pxa_i2c {
>>          bool                    highmode_enter;
>>          unsigned int            ilcr;
>>          unsigned int            iwcr;
>> +       struct pinctrl          *pinctrl;
>> +       struct pinctrl_state    *pin_i2c;
>> +       struct pinctrl_state    *pin_gpio;
>
> Yup that's the right way. I see this is the "default"
> state for pin_i2c.
>
>> +static void i2c_bus_reset(struct pxa_i2c *i2c)
>> +{
>> +       int ret, ccnt, pins_scl, pins_sda;
>
> Use GPIO descriptors.
>
> struct gpio_desc *scl, *sda;
>
>> +       struct device *dev = i2c->adap.dev.parent;
>> +       struct device_node *np = dev->of_node;
>> +
>> +       if (!i2c->pinctrl) {
>> +               dev_warn(dev, "could not do i2c bus reset\n");
>> +               return;
>> +       }
>> +
>> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
>> +       if (ret) {
>> +               dev_err(dev, "could not set gpio pins\n");
>> +               return;
>> +       }
>
> Exactly like that yes.
>
>> +       pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
>> +       if (!gpio_is_valid(pins_scl)) {
>> +               dev_err(dev, "i2c scl gpio not set\n");
>> +               goto err_out;
>> +       }
>> +       pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
>> +       if (!gpio_is_valid(pins_sda)) {
>> +               dev_err(dev, "i2c sda gpio not set\n");
>> +               goto err_out;
>> +       }
>
> I would suggest just using two GPIOs in the node relying
> on index order. With GPIO descriptors:
>
> scl = gpiod_get_index(dev, "i2c-gpio", 0, GPIOD_ASIS);
> sda = gpiod_get_index(dev, "i2c-gpio", 1, GPIOD_ASIS);
>
> Then use gpiod* accessors below and...
>
>> +
>> +       gpio_request(pins_scl, NULL);
>> +       gpio_request(pins_sda, NULL);
>> +
>> +       gpio_direction_input(pins_sda);
>> +       for (ccnt = 20; ccnt; ccnt--) {
>> +               gpio_direction_output(pins_scl, ccnt & 0x01);
>> +               udelay(5);
>> +       }
>> +       gpio_direction_output(pins_scl, 0);
>> +       udelay(5);
>> +       gpio_direction_output(pins_sda, 0);
>> +       udelay(5);
>> +       /* stop signal */
>> +       gpio_direction_output(pins_scl, 1);
>> +       udelay(5);
>> +       gpio_direction_output(pins_sda, 1);
>> +       udelay(5);
>> +
>> +       gpio_free(pins_scl);
>> +       gpio_free(pins_sda);
>
> gpiod_put(scl);
> gpiod_put(sda);
>
>> +err_out:
>> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
>> +       if (ret)
>> +               dev_err(dev, "could not set default(i2c) pins\n");
>> +       return;
>
> Nice.
>
> Overall it looks like a real nice structured workaround using
> the API exactly as intended, just need to catch up with
> using GPIO descriptors.
>


Thanks Linus,

I will work on this and submit V2 shortly.

Thanks,
Vaibhav
Wolfram Sang June 2, 2015, 5:33 p.m. UTC | #6
> Since there is some problematic i2c slave devices on some
> platforms such as dkb (sometimes), it will drop down sda
> and make i2c bus hang, at that time, it need to config
> scl/sda into gpio to simulate "stop" sequence to recover
> i2c bus, so add this interface.

Please note that the i2c core has a bus recovery infrastructure.
Search i2c.h for struct i2c_bus_recovery_info.
Vaibhav Hiremath June 2, 2015, 5:40 p.m. UTC | #7
On Tuesday 02 June 2015 11:03 PM, Wolfram Sang wrote:
>
>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>
> Please note that the i2c core has a bus recovery infrastructure.
> Search i2c.h for struct i2c_bus_recovery_info.
>


Yeah,
I assume you are referring to "i2c_generic_gpio_recovery", right?

Rob had suggested this, and will be using it in my next version.

Thanks,
Vaibhav
Wolfram Sang June 2, 2015, 5:48 p.m. UTC | #8
> I assume you are referring to "i2c_generic_gpio_recovery", right?
> 
> Rob had suggested this, and will be using it in my next version.

Great, thanks!
Vaibhav Hiremath June 2, 2015, 5:57 p.m. UTC | #9
On Tuesday 02 June 2015 11:18 PM, Wolfram Sang wrote:
>> I assume you are referring to "i2c_generic_gpio_recovery", right?
>>
>> Rob had suggested this, and will be using it in my next version.
>
> Great, thanks!
>


can you please also review the RFC which I submitted few days back?

http://www.spinics.net/lists/linux-i2c/msg19719.html


And also,

http://www.spinics.net/lists/arm-kernel/msg422034.html

Thanks,
Vaibhav
Wolfram Sang June 2, 2015, 6:02 p.m. UTC | #10
> can you please also review the RFC which I submitted few days back?

They are added to the queue and will be reviewed when the time comes. I
won't make any promises.
Vaibhav Hiremath June 2, 2015, 6:06 p.m. UTC | #11
On Tuesday 02 June 2015 11:32 PM, Wolfram Sang wrote:
>
>> can you please also review the RFC which I submitted few days back?
>
> They are added to the queue and will be reviewed when the time comes. I
> won't make any promises.
>


Ok, take your time.
Just wanted to make sure that they don't get buried under all email
traffic.

Anyway, thanks for your review.


Thanks,
Vaibhav
Wolfram Sang June 2, 2015, 6:24 p.m. UTC | #12
Can you resend the hwlock patches? I use patchwork for tracking and
those were sent when the server had a little downtime. Thanks!
Vaibhav Hiremath June 2, 2015, 6:46 p.m. UTC | #13
On Tuesday 02 June 2015 11:54 PM, Wolfram Sang wrote:
> Can you resend the hwlock patches? I use patchwork for tracking and
> those were sent when the server had a little downtime. Thanks!
>


Just resent it. please check.

Thanks,
Vaibhav
Vaibhav Hiremath June 3, 2015, 7:16 p.m. UTC | #14
On Tuesday 02 June 2015 06:42 PM, Linus Walleij wrote:
> On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>>
>> Signed-off-by: Leilei Shang <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath@linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Double signed-off?
>

Thanks for your review.

Based on review comments on the list, i will have to change this patch
to use generic gpio reset implementation available in i2c-core.

Having said that, I am still sure whether GPIO lib (request/free)
would handle pinctrl configuration.

Thanks,
vaibhav
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 8ca5552..eb09071 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -37,6 +37,8 @@ 
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/i2c/pxa-i2c.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <asm/irq.h>
 
@@ -177,6 +179,9 @@  struct pxa_i2c {
 	bool			highmode_enter;
 	unsigned int		ilcr;
 	unsigned int		iwcr;
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pin_i2c;
+	struct pinctrl_state	*pin_gpio;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -269,6 +274,62 @@  static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
 
 #define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
 
+static void i2c_bus_reset(struct pxa_i2c *i2c)
+{
+	int ret, ccnt, pins_scl, pins_sda;
+	struct device *dev = i2c->adap.dev.parent;
+	struct device_node *np = dev->of_node;
+
+	if (!i2c->pinctrl) {
+		dev_warn(dev, "could not do i2c bus reset\n");
+		return;
+	}
+
+	ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
+	if (ret) {
+		dev_err(dev, "could not set gpio pins\n");
+		return;
+	}
+
+	pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
+	if (!gpio_is_valid(pins_scl)) {
+		dev_err(dev, "i2c scl gpio not set\n");
+		goto err_out;
+	}
+	pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
+	if (!gpio_is_valid(pins_sda)) {
+		dev_err(dev, "i2c sda gpio not set\n");
+		goto err_out;
+	}
+
+	gpio_request(pins_scl, NULL);
+	gpio_request(pins_sda, NULL);
+
+	gpio_direction_input(pins_sda);
+	for (ccnt = 20; ccnt; ccnt--) {
+		gpio_direction_output(pins_scl, ccnt & 0x01);
+		udelay(5);
+	}
+	gpio_direction_output(pins_scl, 0);
+	udelay(5);
+	gpio_direction_output(pins_sda, 0);
+	udelay(5);
+	/* stop signal */
+	gpio_direction_output(pins_scl, 1);
+	udelay(5);
+	gpio_direction_output(pins_sda, 1);
+	udelay(5);
+
+	gpio_free(pins_scl);
+	gpio_free(pins_sda);
+
+err_out:
+	ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
+	if (ret)
+		dev_err(dev, "could not set default(i2c) pins\n");
+	return;
+}
+
 static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 {
 	unsigned int i;
@@ -281,6 +342,11 @@  static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 	for (i = 0; i < i2c->irqlogidx; i++)
 		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
 	printk("\n");
+	if (strcmp(why, "exhausted retries") != 0) {
+		i2c_bus_reset(i2c);
+		/* reset i2c contorler when it's fail */
+		i2c_pxa_reset(i2c);
+	}
 }
 
 #else /* ifdef DEBUG */
@@ -1301,6 +1367,30 @@  static int i2c_pxa_probe(struct platform_device *dev)
 
 	platform_set_drvdata(dev, i2c);
 
+	i2c->pinctrl = devm_pinctrl_get(&dev->dev);
+	if (IS_ERR(i2c->pinctrl)) {
+		i2c->pinctrl = NULL;
+		dev_warn(&dev->dev, "could not get pinctrl\n");
+	} else {
+		i2c->pin_i2c = pinctrl_lookup_state(i2c->pinctrl, "default");
+		if (IS_ERR(i2c->pin_i2c)) {
+			dev_err(&dev->dev, "could not get default(i2c) pinstate\n");
+			ret = IS_ERR(i2c->pin_i2c);
+		}
+
+		i2c->pin_gpio = pinctrl_lookup_state(i2c->pinctrl, "gpio");
+		if (IS_ERR(i2c->pin_gpio)) {
+			dev_err(&dev->dev, "could not get gpio pinstate\n");
+			ret = IS_ERR(i2c->pin_gpio);
+		}
+
+		if (ret) {
+			i2c->pin_i2c = NULL;
+			i2c->pin_gpio = NULL;
+			i2c->pinctrl = NULL;
+			ret = 0;
+		}
+	}
 #ifdef CONFIG_I2C_PXA_SLAVE
 	printk(KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n",
 	       dev_name(&i2c->adap.dev), i2c->slave_addr);