Re: [Patch v2] i2c: imx: implement bus recovery

2015-09-08 Thread Uwe Kleine-König 
Hello,

On Tue, Sep 08, 2015 at 04:18:41PM +0200, Linus Walleij wrote:
> On Mon, Sep 7, 2015 at 10:00 AM, Uwe Kleine-König
>  wrote:
> > [Me]
> 
> >> If the use case is around the i2c traffic, it is a mode related to I2C,
> >> and if this mode is called "GPIO mode" in the data sheet
> >> is irrelevant, because it is obviously not used for the generic
> >> input/output but the specific I2C. The terminology should be
> >> made familiar to whoever needs to go in and read the code later.
> >
> > The background info that was obviously missing from the part of the
> > thread I sent to you is that pinctrl_pm_select_sleep_state is used to
> > prepare bitbanging on the i2c bus to do bus recovery. (The controller
> > doesn't implement this, so we have to resort to manually drive the
> > pins.)
> 
> I don't understand. What does "manually" mean in this context?
> Code examples for "manual" and "automatic"?

Normally i2c operations are done using the i2c core in the SoC and using
the i2c function of the respective pins. Recovery
however must be done (manually) by configuring the lines as GPIO and
using a sequence of calls to gpio_[gs]et_value.

The sleep state of the i2c bus device (usually?) configures the i2c
lines as GPIO. Is it ok to (ab)use the sleep state to prepare for the
recovery procedure or should we better introduce a dedicated gpio mode
pinctrl?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2] i2c: imx: implement bus recovery

2015-09-08 Thread Linus Walleij 
On Mon, Sep 7, 2015 at 10:00 AM, Uwe Kleine-König
 wrote:
> [Me]

>> If the use case is around the i2c traffic, it is a mode related to I2C,
>> and if this mode is called "GPIO mode" in the data sheet
>> is irrelevant, because it is obviously not used for the generic
>> input/output but the specific I2C. The terminology should be
>> made familiar to whoever needs to go in and read the code later.
>
> The background info that was obviously missing from the part of the
> thread I sent to you is that pinctrl_pm_select_sleep_state is used to
> prepare bitbanging on the i2c bus to do bus recovery. (The controller
> doesn't implement this, so we have to resort to manually drive the
> pins.)

I don't understand. What does "manually" mean in this context?
Code examples for "manual" and "automatic"?

Generally the word "manual" is so ambigous that we should
refrain from using it, to me that means something like
hammering on a keyboard to alter things through sysfs
or debugfs but I hardly believe this is what you mean.

> Is this good enough? I'd like to see implemented a separate pinctrl set
> for bitbanging instead of relying on the sleep state being the right
> thing to enable gpio functionality.

This sounds right but I need to see the two different code sets
to understand. Now my head is all fuzzy...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2] i2c: imx: implement bus recovery

2015-09-07 Thread Uwe Kleine-König 
Hallo Linus,

On Tue, Aug 25, 2015 at 04:18:16PM +0200, Linus Walleij wrote:
> On Wed, Aug 19, 2015 at 9:02 AM, Uwe Kleine-König
>  wrote:
> > On Wed, Aug 19, 2015 at 03:44:49AM +, Gao Pandy wrote:
> >> From: Uwe Kleine-König  Sent: 
> >> Thursday, August 13, 2015 4:15 PM
> >> > > +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
> >> > > + struct imx_i2c_struct *i2c_imx;
> >> > > +
> >> > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> >> > > + if (i2c_imx->pins.sda && i2c_imx->pins.scl) {
> >> > > + pinctrl_pm_select_sleep_state(>dev);
> >> >
> >> > Your requirement that the sleep state should configure the pins as gpio
> >> > is strange. Maybe better introduce a dedicated state for recovery? At
> >> > least you should document this requirement.
> >>
> >> In general, pinctrl sleep mode is gpio function. I will add this in 
> >> binding doc. Thanks.
> >
> > Linus, do you have to say something here? It might be right to have the
> > gpio function as configuration for sleep mode, but it doesn't look right
> > for me to use this for recovery purposes.
> 
> What it usually means is that the pin has a function mode such
> that an asynchronous edge detector is connected to it on the
> outer pad ring, maybe in tristate or some pull-up/down configuration.
> This makes is possible for the system to power
> off completely until an event occurs there with only some
> very peripheral electronics powered up.
> 
> I think the terminology depend on the use case. See the section
> "GPIO mode pitfalls" in Documentation/pinctrl.txt
> 
> If the use case is around the i2c traffic, it is a mode related to I2C,
> and if this mode is called "GPIO mode" in the data sheet
> is irrelevant, because it is obviously not used for the generic
> input/output but the specific I2C. The terminology should be
> made familiar to whoever needs to go in and read the code later.
The background info that was obviously missing from the part of the
thread I sent to you is that pinctrl_pm_select_sleep_state is used to
prepare bitbanging on the i2c bus to do bus recovery. (The controller
doesn't implement this, so we have to resort to manually drive the
pins.)

Is this good enough? I'd like to see implemented a separate pinctrl set
for bitbanging instead of relying on the sleep state being the right
thing to enable gpio functionality.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2] i2c: imx: implement bus recovery

2015-08-25 Thread Linus Walleij 
On Wed, Aug 19, 2015 at 9:02 AM, Uwe Kleine-König
u.kleine-koe...@pengutronix.de wrote:
 On Wed, Aug 19, 2015 at 03:44:49AM +, Gao Pandy wrote:
 From: Uwe Kleine-König mailto:u.kleine-koe...@pengutronix.de Sent: 
 Thursday, August 13, 2015 4:15 PM
   +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
   + struct imx_i2c_struct *i2c_imx;
   +
   + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
   + if (i2c_imx-pins.sda  i2c_imx-pins.scl) {
   + pinctrl_pm_select_sleep_state(adap-dev);
 
  Your requirement that the sleep state should configure the pins as gpio
  is strange. Maybe better introduce a dedicated state for recovery? At
  least you should document this requirement.

 In general, pinctrl sleep mode is gpio function. I will add this in binding 
 doc. Thanks.

 Linus, do you have to say something here? It might be right to have the
 gpio function as configuration for sleep mode, but it doesn't look right
 for me to use this for recovery purposes.

What it usually means is that the pin has a function mode such
that an asynchronous edge detector is connected to it on the
outer pad ring, maybe in tristate or some pull-up/down configuration.
This makes is possible for the system to power
off completely until an event occurs there with only some
very peripheral electronics powered up.

I think the terminology depend on the use case. See the section
GPIO mode pitfalls in Documentation/pinctrl.txt

If the use case is around the i2c traffic, it is a mode related to I2C,
and if this mode is called GPIO mode in the data sheet
is irrelevant, because it is obviously not used for the generic
input/output but the specific I2C. The terminology should be
made familiar to whoever needs to go in and read the code later.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2] i2c: imx: implement bus recovery

2015-08-19 Thread Uwe Kleine-König 
Hello,

Cc += Linus Walleij

On Wed, Aug 19, 2015 at 03:44:49AM +, Gao Pandy wrote:
 From: Uwe Kleine-König mailto:u.kleine-koe...@pengutronix.de Sent: 
 Thursday, August 13, 2015 4:15 PM
   +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
   + struct imx_i2c_struct *i2c_imx;
   +
   + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
   + if (i2c_imx-pins.sda  i2c_imx-pins.scl) {
   + pinctrl_pm_select_sleep_state(adap-dev);
  
  Your requirement that the sleep state should configure the pins as gpio
  is strange. Maybe better introduce a dedicated state for recovery? At
  least you should document this requirement.
 
 In general, pinctrl sleep mode is gpio function. I will add this in binding 
 doc. Thanks.

Linus, do you have to say something here? It might be right to have the
gpio function as configuration for sleep mode, but it doesn't look right
for me to use this for recovery purposes.

 
  
   @@ -1004,12 +1073,13 @@ static int i2c_imx_probe(struct
   platform_device *pdev)
  
 /* Setup i2c_imx driver structure */
 strlcpy(i2c_imx-adapter.name, pdev-name, sizeof(i2c_imx-
  adapter.name));
   - i2c_imx-adapter.owner  = THIS_MODULE;
   - i2c_imx-adapter.algo   = i2c_imx_algo;
   - i2c_imx-adapter.dev.parent = pdev-dev;
   - i2c_imx-adapter.nr = pdev-id;
   - i2c_imx-adapter.dev.of_node= pdev-dev.of_node;
   - i2c_imx-base   = base;
   + i2c_imx-adapter.owner = THIS_MODULE;
   + i2c_imx-adapter.algo = i2c_imx_algo;
   + i2c_imx-adapter.dev.parent = pdev-dev;
   + i2c_imx-adapter.nr = pdev-id;
   + i2c_imx-adapter.dev.of_node = pdev-dev.of_node;
   + i2c_imx-adapter.bus_recovery_info = i2c_imx_bus_recovery_info;
  
  What happens if the device tree only specifies a gpio for sda but not scl?
  The recovery stuff is still hooked up, right? What about only doing that
  if both properties are valid? This would also get rid of the tests for
  validity in the recovery callbacks.
 
 It requires device tree to specify gpio for sda and scl in both. Once only 
 sda is defined, 
 the driver doesn't carry out the recovery process.

Right, and still it is convinced it is able to recover the bus, right?
Can you please change this?

  
   @@ -1037,6 +1107,24 @@ static int i2c_imx_probe(struct platform_device
  *pdev)
 /* Set up adapter data */
 i2c_set_adapdata(i2c_imx-adapter, i2c_imx);
  
   + /* Init recover pins */
   + i2c_imx-pins.sda =
   + devm_gpiod_get_optional(pdev-dev, sda-gpio, 0);
   + i2c_imx-pins.scl =
   + devm_gpiod_get_optional(pdev-dev, scl-gpio, 0);
  
  0 is wrong here, better use GPIOD_ASIS. Is there a reason not to
  configure the direction as input already here?
  
 Thanks, 0 is inappropriate here. Actually, we should use GPIOD_OUT_HIGH
 to set the gpios high. 

Are you sure? Consider the bus is blocked and you switch to gpio mode to
do recovery you have a device pulling SDA to 0 and the imx pulling it to
1. That's wrong, isn't it?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Patch v2] i2c: imx: implement bus recovery

2015-08-19 Thread Gao Pandy 
From: linux-i2c-ow...@vger.kernel.org mailto:linux-i2c-ow...@vger.kernel.org 
Sent: Wednesday, August 19, 2015 3:02 PM
 To: Gao Pan-B54642
 Cc: w...@the-dreams.de; linux-i2c@vger.kernel.org; Li Frank-B20596; Duan
 Fugang-B38611; Linus Walleij
 Subject: Re: [Patch v2] i2c: imx: implement bus recovery
 
 Hello,
 
 Cc += Linus Walleij
 
 On Wed, Aug 19, 2015 at 03:44:49AM +, Gao Pandy wrote:
  From: Uwe Kleine-König mailto:u.kleine-koe...@pengutronix.de Sent:
  Thursday, August 13, 2015 4:15 PM
+static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
+   struct imx_i2c_struct *i2c_imx;
+
+   i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
+   if (i2c_imx-pins.sda  i2c_imx-pins.scl) {
+   pinctrl_pm_select_sleep_state(adap-dev);
  
   Your requirement that the sleep state should configure the pins as
   gpio is strange. Maybe better introduce a dedicated state for
   recovery? At least you should document this requirement.
 
  In general, pinctrl sleep mode is gpio function. I will add this in
 binding doc. Thanks.
 
 Linus, do you have to say something here? It might be right to have the
 gpio function as configuration for sleep mode, but it doesn't look right
 for me to use this for recovery purposes.

Thanks for your precise review.

  
@@ -1004,12 +1073,13 @@ static int i2c_imx_probe(struct
platform_device *pdev)
   
/* Setup i2c_imx driver structure */
strlcpy(i2c_imx-adapter.name, pdev-name, sizeof(i2c_imx-
   adapter.name));
-   i2c_imx-adapter.owner  = THIS_MODULE;
-   i2c_imx-adapter.algo   = i2c_imx_algo;
-   i2c_imx-adapter.dev.parent = pdev-dev;
-   i2c_imx-adapter.nr = pdev-id;
-   i2c_imx-adapter.dev.of_node= pdev-dev.of_node;
-   i2c_imx-base   = base;
+   i2c_imx-adapter.owner = THIS_MODULE;
+   i2c_imx-adapter.algo = i2c_imx_algo;
+   i2c_imx-adapter.dev.parent = pdev-dev;
+   i2c_imx-adapter.nr = pdev-id;
+   i2c_imx-adapter.dev.of_node = pdev-dev.of_node;
+   i2c_imx-adapter.bus_recovery_info =
 i2c_imx_bus_recovery_info;
  
   What happens if the device tree only specifies a gpio for sda but not
 scl?
   The recovery stuff is still hooked up, right? What about only doing
   that if both properties are valid? This would also get rid of the
   tests for validity in the recovery callbacks.
 
  It requires device tree to specify gpio for sda and scl in both. Once
  only sda is defined, the driver doesn't carry out the recovery process.
 
 Right, and still it is convinced it is able to recover the bus, right?
 Can you please change this?
 
Yes, you are right, will change it in the next version.
 
  
@@ -1037,6 +1107,24 @@ static int i2c_imx_probe(struct
platform_device
   *pdev)
/* Set up adapter data */
i2c_set_adapdata(i2c_imx-adapter, i2c_imx);
   
+   /* Init recover pins */
+   i2c_imx-pins.sda =
+   devm_gpiod_get_optional(pdev-dev, sda-gpio, 0);
+   i2c_imx-pins.scl =
+   devm_gpiod_get_optional(pdev-dev, scl-gpio, 0);
  
   0 is wrong here, better use GPIOD_ASIS. Is there a reason not to
   configure the direction as input already here?
 
  Thanks, 0 is inappropriate here. Actually, we should use
  GPIOD_OUT_HIGH to set the gpios high.
 
 Are you sure? Consider the bus is blocked and you switch to gpio mode to
 do recovery you have a device pulling SDA to 0 and the imx pulling it to
 1. That's wrong, isn't it?

Yes, you are right. The gpios should be set low. Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2] i2c: imx: implement bus recovery

2015-08-19 Thread Uwe Kleine-König 
Hello,

On Wed, Aug 19, 2015 at 07:43:51AM +, Gao Pandy wrote:
  On Wed, Aug 19, 2015 at 03:44:49AM +, Gao Pandy wrote:
   /* Set up adapter data */
   i2c_set_adapdata(i2c_imx-adapter, i2c_imx);

 + /* Init recover pins */
 + i2c_imx-pins.sda =
 + devm_gpiod_get_optional(pdev-dev, sda-gpio, 0);
 + i2c_imx-pins.scl =
 + devm_gpiod_get_optional(pdev-dev, scl-gpio, 0);
   
0 is wrong here, better use GPIOD_ASIS. Is there a reason not to
configure the direction as input already here?
  
   Thanks, 0 is inappropriate here. Actually, we should use
   GPIOD_OUT_HIGH to set the gpios high.
  
  Are you sure? Consider the bus is blocked and you switch to gpio mode to
  do recovery you have a device pulling SDA to 0 and the imx pulling it to
  1. That's wrong, isn't it?
 
 Yes, you are right. The gpios should be set low. Thanks.
Then on switching to the gpio function from an idle bus (which should be
the normal case) you pull both lines low which depending on the timing
might even be recognised as a start condition. I'm convinced the right
thing to select here is GPIOD_IN.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Patch v2] i2c: imx: implement bus recovery

2015-08-18 Thread Gao Pandy 
From: Uwe Kleine-König mailto:u.kleine-koe...@pengutronix.de Sent: Thursday, 
August 13, 2015 4:15 PM
 To: Gao Pan-B54642
 Cc: w...@the-dreams.de; linux-i2c@vger.kernel.org; Li Frank-B20596; Duan 
 Fugang-B38611
 Subject: Re: [Patch v2] i2c: imx: implement bus recovery
 
 Hello,
 
 On Fri, Jul 17, 2015 at 10:36:45AM +0800, Gao Pan wrote:
  Implement bus recovery methods for i2c-imx so we can recover from
  situations where SCL/SDA are stuck low.
 
  Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
  pinctrl to gpio mode by calling pinctrl sleep set function, and then
  use GPIO to emulate the i2c protocol to send nine dummy clock to
  recover i2c device. After recovery, set i2c pinctrl to default group
 setting.
 
  V2:
  As Uwe Kleine-König's suggestion, the version do below changes:
   -replace of_get_named_gpio() with devm_gpiod_get_optional()  -move
  gpio_direction_input() and gpio_direction_output() call to the
prepare callback
   -use 0 and 1 as return value for the get_scl and get_sda callbacks
 
 this section usally goes after the triple-dash below. Otherwise this is
 included in the commit log which it shouldn't.

Thanks, will change it in the next version.
 
  diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
  b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
  index ce4311d..a9fe525 100644
  --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
  +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
  @@ -14,6 +14,8 @@ Optional properties:
 The absence of the propoerty indicates the default frequency 100 kHz.
   - dmas: A list of two dma specifiers, one for each entry in dma-names.
   - dma-names: should contain tx and rx.
  +- scl-gpio: specify the gpio related to SCL pin
  +- sda-gpio: specify the gpio related to SDA pin
 
 In the meantime I learned that ...-gpios is the recommended name for
 such properties, even if conceptually there can only be a single gpio
 here. (You might want to help to change that?!)

Thanks, will change it in the next version.
 
  @@ -436,7 +444,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
 *i2c_imx, int for_busy)
  if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500)))
 {
  dev_dbg(i2c_imx-adapter.dev,
  %s I2C bus is busy\n, __func__);
  -   return -ETIMEDOUT;
  +   return i2c_recover_bus(i2c_imx-adapter);
  }
  schedule();
  }
  @@ -450,7 +458,7 @@ static int i2c_imx_trx_complete(struct
  imx_i2c_struct *i2c_imx)
 
  if (unlikely(!(i2c_imx-i2csr  I2SR_IIF))) {
  dev_dbg(i2c_imx-adapter.dev, %s Timeout\n, __func__);
  -   return -ETIMEDOUT;
  +   return i2c_recover_bus(i2c_imx-adapter);
 
 I'm still not convinced that doing that here unconditionally is a good
 idea. Wolfram will know for sure.
 
Thanks, I will remove this change in next version.
 
  +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
  +   struct imx_i2c_struct *i2c_imx;
  +
  +   i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
  +   if (i2c_imx-pins.sda  i2c_imx-pins.scl) {
  +   pinctrl_pm_select_sleep_state(adap-dev);
 
 Your requirement that the sleep state should configure the pins as gpio
 is strange. Maybe better introduce a dedicated state for recovery? At
 least you should document this requirement.

In general, pinctrl sleep mode is gpio function. I will add this in binding 
doc. Thanks.

 
  @@ -1004,12 +1073,13 @@ static int i2c_imx_probe(struct
  platform_device *pdev)
 
  /* Setup i2c_imx driver structure */
  strlcpy(i2c_imx-adapter.name, pdev-name, sizeof(i2c_imx-
 adapter.name));
  -   i2c_imx-adapter.owner  = THIS_MODULE;
  -   i2c_imx-adapter.algo   = i2c_imx_algo;
  -   i2c_imx-adapter.dev.parent = pdev-dev;
  -   i2c_imx-adapter.nr = pdev-id;
  -   i2c_imx-adapter.dev.of_node= pdev-dev.of_node;
  -   i2c_imx-base   = base;
  +   i2c_imx-adapter.owner = THIS_MODULE;
  +   i2c_imx-adapter.algo = i2c_imx_algo;
  +   i2c_imx-adapter.dev.parent = pdev-dev;
  +   i2c_imx-adapter.nr = pdev-id;
  +   i2c_imx-adapter.dev.of_node = pdev-dev.of_node;
  +   i2c_imx-adapter.bus_recovery_info = i2c_imx_bus_recovery_info;
 
 What happens if the device tree only specifies a gpio for sda but not scl?
 The recovery stuff is still hooked up, right? What about only doing that
 if both properties are valid? This would also get rid of the tests for
 validity in the recovery callbacks.

It requires device tree to specify gpio for sda and scl in both. Once only sda 
is defined, 
the driver doesn't carry out the recovery process.

 
  @@ -1037,6 +1107,24 @@ static int i2c_imx_probe(struct platform_device
 *pdev)
  /* Set up adapter data */
  i2c_set_adapdata(i2c_imx-adapter, i2c_imx);
 
  +   /* Init recover pins */
  +   i2c_imx-pins.sda =
  +   devm_gpiod_get_optional(pdev-dev

Re: [Patch v2] i2c: imx: implement bus recovery

2015-08-13 Thread Uwe Kleine-König 
Hello,

On Fri, Jul 17, 2015 at 10:36:45AM +0800, Gao Pan wrote:
 Implement bus recovery methods for i2c-imx so we can recover from
 situations where SCL/SDA are stuck low.
 
 Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
 pinctrl to gpio mode by calling pinctrl sleep set function, and then
 use GPIO to emulate the i2c protocol to send nine dummy clock to recover
 i2c device. After recovery, set i2c pinctrl to default group setting.
 
 V2:
 As Uwe Kleine-König's suggestion, the version do below changes:
  -replace of_get_named_gpio() with devm_gpiod_get_optional()
  -move gpio_direction_input() and gpio_direction_output() call to the
   prepare callback
  -use 0 and 1 as return value for the get_scl and get_sda callbacks

this section usally goes after the triple-dash below. Otherwise this is
included in the commit log which it shouldn't.

 diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt 
 b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 index ce4311d..a9fe525 100644
 --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 @@ -14,6 +14,8 @@ Optional properties:
The absence of the propoerty indicates the default frequency 100 kHz.
  - dmas: A list of two dma specifiers, one for each entry in dma-names.
  - dma-names: should contain tx and rx.
 +- scl-gpio: specify the gpio related to SCL pin
 +- sda-gpio: specify the gpio related to SDA pin

In the meantime I learned that ...-gpios is the recommended name for
such properties, even if conceptually there can only be a single gpio
here. (You might want to help to change that?!)

 @@ -436,7 +444,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct 
 *i2c_imx, int for_busy)
   if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
   dev_dbg(i2c_imx-adapter.dev,
   %s I2C bus is busy\n, __func__);
 - return -ETIMEDOUT;
 + return i2c_recover_bus(i2c_imx-adapter);
   }
   schedule();
   }
 @@ -450,7 +458,7 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct 
 *i2c_imx)
  
   if (unlikely(!(i2c_imx-i2csr  I2SR_IIF))) {
   dev_dbg(i2c_imx-adapter.dev, %s Timeout\n, __func__);
 - return -ETIMEDOUT;
 + return i2c_recover_bus(i2c_imx-adapter);

I'm still not convinced that doing that here unconditionally is a good
idea. Wolfram will know for sure.

 +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
 +{
 + struct imx_i2c_struct *i2c_imx;
 +
 + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
 + if (i2c_imx-pins.sda  i2c_imx-pins.scl) {
 + pinctrl_pm_select_sleep_state(adap-dev);

Your requirement that the sleep state should configure the pins as gpio
is strange. Maybe better introduce a dedicated state for recovery? At
least you should document this requirement.

 @@ -1004,12 +1073,13 @@ static int i2c_imx_probe(struct platform_device *pdev)
  
   /* Setup i2c_imx driver structure */
   strlcpy(i2c_imx-adapter.name, pdev-name, 
 sizeof(i2c_imx-adapter.name));
 - i2c_imx-adapter.owner  = THIS_MODULE;
 - i2c_imx-adapter.algo   = i2c_imx_algo;
 - i2c_imx-adapter.dev.parent = pdev-dev;
 - i2c_imx-adapter.nr = pdev-id;
 - i2c_imx-adapter.dev.of_node= pdev-dev.of_node;
 - i2c_imx-base   = base;
 + i2c_imx-adapter.owner = THIS_MODULE;
 + i2c_imx-adapter.algo = i2c_imx_algo;
 + i2c_imx-adapter.dev.parent = pdev-dev;
 + i2c_imx-adapter.nr = pdev-id;
 + i2c_imx-adapter.dev.of_node = pdev-dev.of_node;
 + i2c_imx-adapter.bus_recovery_info = i2c_imx_bus_recovery_info;

What happens if the device tree only specifies a gpio for sda but not
scl? The recovery stuff is still hooked up, right? What about only doing
that if both properties are valid? This would also get rid of the tests
for validity in the recovery callbacks.

 @@ -1037,6 +1107,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
   /* Set up adapter data */
   i2c_set_adapdata(i2c_imx-adapter, i2c_imx);
  
 + /* Init recover pins */
 + i2c_imx-pins.sda =
 + devm_gpiod_get_optional(pdev-dev, sda-gpio, 0);
 + i2c_imx-pins.scl =
 + devm_gpiod_get_optional(pdev-dev, scl-gpio, 0);

0 is wrong here, better use GPIOD_ASIS. Is there a reason not to
configure the direction as input already here?

 +
 + if (IS_ERR(i2c_imx-pins.sda)) {
 + ret = PTR_ERR(i2c_imx-pins.sda);
 + dev_err(pdev-dev, Failed to get sda gpio: %d\n, ret);
 + i2c_imx-pins.sda = NULL;
 + }

As you're using devm_gpiod_get_optional there is no justification to
ignore the error code.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions |

RE: [Patch v2] i2c: imx: implement bus recovery

2015-08-12 Thread Gao Pandy 
Ping...

 -Original Message-
 From: Gao Pan [mailto:b54...@freescale.com]
 Sent: Friday, July 17, 2015 10:37 AM
 To: w...@the-dreams.de
 Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; Gao
 Pan-B54642; u.kleine-koe...@pengutronix.de
 Subject: [Patch v2] i2c: imx: implement bus recovery
 
 Implement bus recovery methods for i2c-imx so we can recover from
 situations where SCL/SDA are stuck low.
 
 Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
 pinctrl to gpio mode by calling pinctrl sleep set function, and then use
 GPIO to emulate the i2c protocol to send nine dummy clock to recover i2c
 device. After recovery, set i2c pinctrl to default group setting.
 
 V2:
 As Uwe Kleine-König's suggestion, the version do below changes:
  -replace of_get_named_gpio() with devm_gpiod_get_optional()  -move
 gpio_direction_input() and gpio_direction_output() call to the
   prepare callback
  -use 0 and 1 as return value for the get_scl and get_sda callbacks
 
 Signed-off-by: Fugang Duan b38...@freescale.com
 Signed-off-by: Gao Pan b54...@freescale.com
 ---
  Documentation/devicetree/bindings/i2c/i2c-imx.txt |   4 +
  drivers/i2c/busses/i2c-imx.c  | 104
 --
  2 files changed, 100 insertions(+), 8 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 index ce4311d..a9fe525 100644
 --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
 @@ -14,6 +14,8 @@ Optional properties:
The absence of the propoerty indicates the default frequency 100 kHz.
  - dmas: A list of two dma specifiers, one for each entry in dma-names.
  - dma-names: should contain tx and rx.
 +- scl-gpio: specify the gpio related to SCL pin
 +- sda-gpio: specify the gpio related to SDA pin
 
  Examples:
 
 @@ -37,4 +39,6 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
   dmas = edma0 0 50,
   edma0 0 51;
   dma-names = rx,tx;
 + scl-gpio = gpio5 26 0;
 + sda-gpio = gpio5 27 0;
  };
 diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
 index 785aa67..4f75370 100644
 --- a/drivers/i2c/busses/i2c-imx.c
 +++ b/drivers/i2c/busses/i2c-imx.c
 @@ -40,6 +40,7 @@
  #include linux/dmapool.h
  #include linux/err.h
  #include linux/errno.h
 +#include linux/gpio.h
  #include linux/i2c.h
  #include linux/init.h
  #include linux/interrupt.h
 @@ -48,6 +49,7 @@
  #include linux/module.h
  #include linux/of.h
  #include linux/of_device.h
 +#include linux/of_gpio.h
  #include linux/of_dma.h
  #include linux/platform_data/i2c-imx.h  #include
 linux/platform_device.h @@ -175,6 +177,11 @@ enum imx_i2c_type {
   VF610_I2C,
  };
 
 +struct imx_i2c_pinctrl {
 + struct gpio_desc *sda;
 + struct gpio_desc *scl;
 +};
 +
  struct imx_i2c_hwdata {
   enum imx_i2c_type   devtype;
   unsignedregshift;
 @@ -206,6 +213,7 @@ struct imx_i2c_struct {
   unsigned intifdr; /* IMX_I2C_IFDR */
   unsigned intcur_clk;
   unsigned intbitrate;
 + struct imx_i2c_pinctrl  pins;
   const struct imx_i2c_hwdata *hwdata;
 
   struct imx_i2c_dma  *dma;
 @@ -436,7 +444,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
 *i2c_imx, int for_busy)
   if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500)))
 {
   dev_dbg(i2c_imx-adapter.dev,
   %s I2C bus is busy\n, __func__);
 - return -ETIMEDOUT;
 + return i2c_recover_bus(i2c_imx-adapter);
   }
   schedule();
   }
 @@ -450,7 +458,7 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct
 *i2c_imx)
 
   if (unlikely(!(i2c_imx-i2csr  I2SR_IIF))) {
   dev_dbg(i2c_imx-adapter.dev, %s Timeout\n, __func__);
 - return -ETIMEDOUT;
 + return i2c_recover_bus(i2c_imx-adapter);
   }
   dev_dbg(i2c_imx-adapter.dev, %s TRX complete\n, __func__);
   i2c_imx-i2csr = 0;
 @@ -956,6 +964,67 @@ fail0:
   return (result  0) ? result : num;
  }
 
 +static int i2c_imx_get_scl(struct i2c_adapter *adap) {
 + struct imx_i2c_struct *i2c_imx;
 +
 + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
 + if (i2c_imx-pins.scl)
 + return gpiod_get_value(i2c_imx-pins.scl);
 +
 + return 1;
 +}
 +
 +static int i2c_imx_get_sda(struct i2c_adapter *adap) {
 + struct imx_i2c_struct *i2c_imx;
 +
 + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
 + if (i2c_imx-pins.sda)
 + return gpiod_get_value(i2c_imx-pins.sda);
 +
 + return 1;
 +}
 +
 +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val) {
 + struct imx_i2c_struct *i2c_imx;
 +
 + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
 + if (i2c_imx-pins.scl)
 +