[RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[Wolfram Sang]

To keep the discussion about master_xfer_irqless going, I post here a
draft how I envision the changes to the I2C core. They are only build
tested. I am unsure if I can test them on hardware before next week, so
I'll send them around as RFC already, so people can get an idea and
comment. Maybe Stefan has some bandwidth to test his imx driver
implementation on top of this?

A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless

Happy hacking and thanks for all the input to this,

   Wolfram


Wolfram Sang (4):
  i2c: core: remove outdated DEBUG output
  i2c: core: remove level of indentation in i2c_transfer
  i2c: core: use I2C locking behaviour also for SMBUS
  i2c: core: introduce master_xfer_irqless callback

 drivers/i2c/i2c-core-base.c  | 44 +++++++++++++-----------------------
 drivers/i2c/i2c-core-smbus.c |  7 +++++-
 drivers/i2c/i2c-core.h       | 12 ++++++++++
 include/linux/i2c.h          | 10 +++++---
 4 files changed, 41 insertions(+), 32 deletions(-)

-- 
2.18.0

[RFC PATCH 1/4] i2c: core: remove outdated DEBUG output

[Wolfram Sang]

The usefulness of this debug output is questionable. It covers only
direct i2c_transfer calls and no SMBUS calls, neither direct nor
emulated ones. And the latter one is largely used in the kernel, so a
lot of stuff is missed. Also, we have a proper tracing mechanism for all
these kinds of transfers in place for years now. Remove this old one.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9ee9a15e7134..c2b352c46fae 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 */
 
 	if (adap->algo->master_xfer) {
-#ifdef DEBUG
-		for (ret = 0; ret < num; ret++) {
-			dev_dbg(&adap->dev,
-				"master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
-				ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
-				msgs[ret].addr, msgs[ret].len,
-				(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
-		}
-#endif
-
 		if (in_atomic() || irqs_disabled()) {
 			ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
 			if (!ret)
-- 
2.18.0

Re: [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> The usefulness of this debug output is questionable. It covers only
> direct i2c_transfer calls and no SMBUS calls, neither direct nor
> emulated ones. And the latter one is largely used in the kernel, so a
> lot of stuff is missed. Also, we have a proper tracing mechanism for all
> these kinds of transfers in place for years now. Remove this old one.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

This old one fires before locking and even if locking fails for the
atomic/irq case. You might want to mention that in the commit message?
But I certainly agree with the usefulness statement...

Acked-by: Peter Rosin <peda@axentia.se>

> ---
>  drivers/i2c/i2c-core-base.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9ee9a15e7134..c2b352c46fae 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	 */
>  
>  	if (adap->algo->master_xfer) {
> -#ifdef DEBUG
> -		for (ret = 0; ret < num; ret++) {
> -			dev_dbg(&adap->dev,
> -				"master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
> -				ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
> -				msgs[ret].addr, msgs[ret].len,
> -				(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
> -		}
> -#endif
> -
>  		if (in_atomic() || irqs_disabled()) {
>  			ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
>  			if (!ret)
> 

Re: [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output

[Wolfram Sang]

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

On Thu, Sep 20, 2018 at 06:14:20PM +0200, Wolfram Sang wrote:
> The usefulness of this debug output is questionable. It covers only
> direct i2c_transfer calls and no SMBUS calls, neither direct nor
> emulated ones. And the latter one is largely used in the kernel, so a
> lot of stuff is missed. Also, we have a proper tracing mechanism for all
> these kinds of transfers in place for years now. Remove this old one.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I think we can have this clean up already.

Applied to for-next (with updated commit message), thanks!


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

[RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer

[Wolfram Sang]

Using the common kernel pattern to bail out at the beginning if some
conditions are not met, we can save a level of indentation. No
functional change.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c2b352c46fae..799776c6d421 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1922,6 +1922,11 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	int ret;
 
+	if (!adap->algo->master_xfer) {
+		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
+		return -EOPNOTSUPP;
+	}
+
 	/* REVISIT the fault reporting model here is weak:
 	 *
 	 *  - When we get an error after receiving N bytes from a slave,
@@ -1938,25 +1943,19 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 *    one (discarding status on the second message) or errno
 	 *    (discarding status on the first one).
 	 */
-
-	if (adap->algo->master_xfer) {
-		if (in_atomic() || irqs_disabled()) {
-			ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
-			if (!ret)
-				/* I2C activity is ongoing. */
-				return -EAGAIN;
-		} else {
-			i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
-		}
-
-		ret = __i2c_transfer(adap, msgs, num);
-		i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
-
-		return ret;
+	if (in_atomic() || irqs_disabled()) {
+		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
+		if (!ret)
+			/* I2C activity is ongoing. */
+			return -EAGAIN;
 	} else {
-		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
-		return -EOPNOTSUPP;
+		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
 	}
+
+	ret = __i2c_transfer(adap, msgs, num);
+	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
+
+	return ret;
 }
 EXPORT_SYMBOL(i2c_transfer);
 
-- 
2.18.0

Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> Using the common kernel pattern to bail out at the beginning if some
> conditions are not met, we can save a level of indentation. No
> functional change.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Yes please!

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer

[Wolfram Sang]

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

On Thu, Sep 20, 2018 at 07:26:21PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > Using the common kernel pattern to bail out at the beginning if some
> > conditions are not met, we can save a level of indentation. No
> > functional change.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Yes please!

:)


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

Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer

[Wolfram Sang]

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

On Thu, Sep 20, 2018 at 06:14:21PM +0200, Wolfram Sang wrote:
> Using the common kernel pattern to bail out at the beginning if some
> conditions are not met, we can save a level of indentation. No
> functional change.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I think we can have this clean up already.

Applied to for-next, thanks!


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

[RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS

[Wolfram Sang]

If I2C transfers are executed in atomic contexts, trylock is used
instead of lock. This behaviour was missing for SMBUS, although a lot of
transfers are of SMBUS type, either emulated or direct. So, factor out
the locking routine into a helper and use it for I2C and SMBUS.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c  | 11 +++--------
 drivers/i2c/i2c-core-smbus.c |  7 ++++++-
 drivers/i2c/i2c-core.h       | 12 ++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 799776c6d421..904b4d2ebefa 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 *    one (discarding status on the second message) or errno
 	 *    (discarding status on the first one).
 	 */
-	if (in_atomic() || irqs_disabled()) {
-		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
-		if (!ret)
-			/* I2C activity is ongoing. */
-			return -EAGAIN;
-	} else {
-		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
-	}
+	ret = __i2c_lock_bus_helper(adap);
+	if (ret)
+		return ret;
 
 	ret = __i2c_transfer(adap, msgs, num);
 	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 9cd66cabb84f..dbb46edb8e02 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -20,6 +20,8 @@
 #include <linux/i2c-smbus.h>
 #include <linux/slab.h>
 
+#include "i2c-core.h"
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/smbus.h>
 
@@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 {
 	s32 res;
 
-	i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
+	res = __i2c_lock_bus_helper(adapter);
+	if (res)
+		return res;
+
 	res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
 			       command, protocol, data);
 	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 37576f50fe20..6e98aa811980 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -29,6 +29,18 @@ extern int		__i2c_first_dynamic_bus_num;
 
 int i2c_check_7bit_addr_validity_strict(unsigned short addr);
 
+static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
+{
+	int ret = 0;
+
+	if (in_atomic() || irqs_disabled())
+		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
+	else
+		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
+
+	return ret;
+}
+
 #ifdef CONFIG_ACPI
 const struct acpi_device_id *
 i2c_acpi_match_device(const struct acpi_device_id *matches,
-- 
2.18.0

Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> If I2C transfers are executed in atomic contexts, trylock is used
> instead of lock. This behaviour was missing for SMBUS, although a lot of
> transfers are of SMBUS type, either emulated or direct. So, factor out
> the locking routine into a helper and use it for I2C and SMBUS.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Is it ok with static analyzers to "hide" the locking in helpers like
this? Will it not be harder for them to "see" what's going on? But I
don't think we have any annotations anyway, so...

Acked-by: Peter Rosin <peda@axentia.se>


> ---
>  drivers/i2c/i2c-core-base.c  | 11 +++--------
>  drivers/i2c/i2c-core-smbus.c |  7 ++++++-
>  drivers/i2c/i2c-core.h       | 12 ++++++++++++
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 799776c6d421..904b4d2ebefa 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	 *    one (discarding status on the second message) or errno
>  	 *    (discarding status on the first one).
>  	 */
> -	if (in_atomic() || irqs_disabled()) {
> -		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
> -		if (!ret)
> -			/* I2C activity is ongoing. */
> -			return -EAGAIN;
> -	} else {
> -		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> -	}
> +	ret = __i2c_lock_bus_helper(adap);
> +	if (ret)
> +		return ret;
>  
>  	ret = __i2c_transfer(adap, msgs, num);
>  	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 9cd66cabb84f..dbb46edb8e02 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -20,6 +20,8 @@
>  #include <linux/i2c-smbus.h>
>  #include <linux/slab.h>
>  
> +#include "i2c-core.h"
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/smbus.h>
>  
> @@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
>  {
>  	s32 res;
>  
> -	i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
> +	res = __i2c_lock_bus_helper(adapter);
> +	if (res)
> +		return res;
> +
>  	res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
>  			       command, protocol, data);
>  	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 37576f50fe20..6e98aa811980 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,18 @@ extern int		__i2c_first_dynamic_bus_num;
>  
>  int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>  
> +static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> +{
> +	int ret = 0;
> +
> +	if (in_atomic() || irqs_disabled())
> +		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
> +	else
> +		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> +
> +	return ret;
> +}
> +
>  #ifdef CONFIG_ACPI
>  const struct acpi_device_id *
>  i2c_acpi_match_device(const struct acpi_device_id *matches,
> 

Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS

[Wolfram Sang]

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

On Thu, Sep 20, 2018 at 07:31:19PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > If I2C transfers are executed in atomic contexts, trylock is used
> > instead of lock. This behaviour was missing for SMBUS, although a lot of
> > transfers are of SMBUS type, either emulated or direct. So, factor out
> > the locking routine into a helper and use it for I2C and SMBUS.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Is it ok with static analyzers to "hide" the locking in helpers like
> this? Will it not be harder for them to "see" what's going on? But I
> don't think we have any annotations anyway, so...

Yes, you are right. Yet, I prefer this to open coding the same twice and
have the problem to keep them in sync.


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

[RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback

[Wolfram Sang]

We had the request to access devices very late when interrupts are not
available anymore multiple times now. Mostly to prepare shutdown or
reboot. Allow adapters to specify a specific callback for this case.
Note that we fall back to the generic master_xfer callback if this new
irqless one is not present. This is intentional to preserve the previous
behaviour and avoid regressions. Because there are drivers not using
interrupts or because it might have worked "accidently" before.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  6 +++++-
 include/linux/i2c.h         | 10 +++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 904b4d2ebefa..f827446c3089 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	/* Retry automatically on arbitration loss */
 	orig_jiffies = jiffies;
 	for (ret = 0, try = 0; try <= adap->retries; try++) {
-		ret = adap->algo->master_xfer(adap, msgs, num);
+		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
+			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
+		else
+			ret = adap->algo->master_xfer(adap, msgs, num);
+
 		if (ret != -EAGAIN)
 			break;
 		if (time_after(jiffies, orig_jiffies + adap->timeout))
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..11e615123bd0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
  *   defined by the msgs array, with num messages available to transfer via
  *   the adapter specified by adap.
+ * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
+ *   so e.g. PMICs can be accessed very late before shutdown
  * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
  *   is not present, then the bus layer will try and convert the SMBus calls
  *   into I2C transfers instead.
@@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
  * to name two of the most common.
  *
- * The return codes from the @master_xfer field should indicate the type of
- * error code that occurred during the transfer, as documented in the kernel
- * Documentation file Documentation/i2c/fault-codes.
+ * The return codes from the @master_xfer{_irqless} field should indicate the
+ * type of error code that occurred during the transfer, as documented in the
+ * Kernel Documentation file Documentation/i2c/fault-codes.
  */
 struct i2c_algorithm {
 	/* If an adapter algorithm can't do I2C-level access, set master_xfer
@@ -524,6 +526,8 @@ struct i2c_algorithm {
 	   processed, or a negative value on error */
 	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			   int num);
+	int (*master_xfer_irqless)(struct i2c_adapter *adap,
+				   struct i2c_msg *msgs, int num);
 	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
 			   unsigned short flags, char read_write,
 			   u8 command, int size, union i2c_smbus_data *data);
-- 
2.18.0

Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or

The first sentence is a bit backwards, I'd rephrase like so:

Multiple times now we've had the request to access devices very late, when
interrupts are no longer available.

> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

accidentally

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +		else
> +			ret = adap->algo->master_xfer(adap, msgs, num);
> +
>  		if (ret != -EAGAIN)
>  			break;
>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts

"Same" (with capital 'S') to match the other entries. Also, should it
not be @master_xfer to help the tools do the right thing?

> + *   so e.g. PMICs can be accessed very late before shutdown

Trailing period.

I'm fine with this change, but should it not wait until there is a user?
(I think there is one in the wings, so that's a very weak objection...)

Acked-by: Peter Rosin <peda@axentia.se>


>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  	   processed, or a negative value on error */
>  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num);
> +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
>  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  			   unsigned short flags, char read_write,
>  			   u8 command, int size, union i2c_smbus_data *data);
> 

Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback

[Wolfram Sang]

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

On Thu, Sep 20, 2018 at 07:41:05PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > We had the request to access devices very late when interrupts are not
> > available anymore multiple times now. Mostly to prepare shutdown or
> 
> The first sentence is a bit backwards, I'd rephrase like so:
> 
> Multiple times now we've had the request to access devices very late, when
> interrupts are no longer available.

Ok. Don't see much difference, but I don't mind.

> > reboot. Allow adapters to specify a specific callback for this case.
> > Note that we fall back to the generic master_xfer callback if this new
> > irqless one is not present. This is intentional to preserve the previous
> > behaviour and avoid regressions. Because there are drivers not using
> > interrupts or because it might have worked "accidently" before.
> 
> accidentally

Thanks.

> > @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
> >   *   defined by the msgs array, with num messages available to transfer via
> >   *   the adapter specified by adap.
> > + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> 
> "Same" (with capital 'S') to match the other entries. Also, should it
> not be @master_xfer to help the tools do the right thing?

I'll check. You are probably right.

> > + *   so e.g. PMICs can be accessed very late before shutdown
> 
> Trailing period.
> 
> I'm fine with this change, but should it not wait until there is a user?
> (I think there is one in the wings, so that's a very weak objection...)

As I mentioned in the cover-letter, this series is RFC because it is
mainly meant as assistance for Stefan, so he could base his imx patches
on top of it. Or the TI folks for their omap driver.

I somewhen need to implement irqless transfers for the i2c-sh_mobile
driver. But this may take a while, so I hope the others are first. And
yes, I won't apply this series without a user and proper testing.

Thanks for the review, Peter!


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

Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback

[Russell King - ARM Linux]

On Thu, Sep 20, 2018 at 06:14:23PM +0200, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or
> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

It's not really about "irqless", it's about being in atomic context.
irqless makes it sound like you may sleep, but the reality is sleeping
is also out (the scheduler needs IRQs to do it's job.)

So, it may be tempting to use things like msleep() in "irqless" but
that's not permissable.  So surely "atomic" would be a better name for
this?

Also, how does this get around the issue which I pointed out with (eg)
an oops occuring, which leads to a panic followed by an attempt to
reboot if the I2C bus in question is already mid-transaction?  Won't
we deadlock?

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +		else
> +			ret = adap->algo->master_xfer(adap, msgs, num);
> +
>  		if (ret != -EAGAIN)
>  			break;
>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> + *   so e.g. PMICs can be accessed very late before shutdown
>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  	   processed, or a negative value on error */
>  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num);
> +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
>  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  			   unsigned short flags, char read_write,
>  			   u8 command, int size, union i2c_smbus_data *data);
> -- 
> 2.18.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback

[Wolfram Sang]

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

Hi Russell,

I know this has been quite a while. I didn't have further time for this
topic back then. But now I am going to revive and rework it. Still,
thank you for your input.

On Thu, Oct 18, 2018 at 11:44:46AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 20, 2018 at 06:14:23PM +0200, Wolfram Sang wrote:
> > We had the request to access devices very late when interrupts are not
> > available anymore multiple times now. Mostly to prepare shutdown or
> > reboot. Allow adapters to specify a specific callback for this case.
> > Note that we fall back to the generic master_xfer callback if this new
> > irqless one is not present. This is intentional to preserve the previous
> > behaviour and avoid regressions. Because there are drivers not using
> > interrupts or because it might have worked "accidently" before.
> 
> It's not really about "irqless", it's about being in atomic context.
> irqless makes it sound like you may sleep, but the reality is sleeping
> is also out (the scheduler needs IRQs to do it's job.)
> 
> So, it may be tempting to use things like msleep() in "irqless" but
> that's not permissable.  So surely "atomic" would be a better name for
> this?

Full ack. It should be 'master_xfer_atomic'.

> Also, how does this get around the issue which I pointed out with (eg)
> an oops occuring, which leads to a panic followed by an attempt to
> reboot if the I2C bus in question is already mid-transaction?  Won't
> we deadlock?

Interesting question with multiple aspects:

- The above setting will cause problems but this is orthogonal to this
  patch. It works by modifying __i2c_transfer() but all the locking logic
  is one layer above in i2c_transfer(). So, there shouldn't be a
  difference.

- We won't deadlock, because I2C core will use trylock in this case.
  However, even reporting -EAGAIN won't be helpful because the
  interrupted transfer is likely to never finish without irqs. So,
  retrying won't help and we are still stuck.

- Brainstorming: we could decide that if in_atomic() and
  master_xfer_atomic() present and the I2C client is allowed to do
  ATOMIC_IO (and maybe more checks), to ignore the locking and force
  this command on the bus. Also requiring master_xfer_atomic() to reset
  the IP core properly, etc. Probably also enforcing i2c_recover_bus()
  to ensure a free bus. This is probably the best we could do, but
  still no guarantees here. Some I2C devices just lock up when
  interrupted at the wrong time.

Relying on I2C for poweroff/reboot is a somewhat fragile design to begin
with. I will re-start this series with something simple, but keep the
above scenario in mind. Then, we can see how much we can do, hopefully.

And I got the idea for a panic-fault-injector which sounds interesting.
I will try that!

Kind regards,

   Wolfram

> 
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  drivers/i2c/i2c-core-base.c |  6 +++++-
> >  include/linux/i2c.h         | 10 +++++++---
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 904b4d2ebefa..f827446c3089 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> >  	/* Retry automatically on arbitration loss */
> >  	orig_jiffies = jiffies;
> >  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> > -		ret = adap->algo->master_xfer(adap, msgs, num);
> > +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> > +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> > +		else
> > +			ret = adap->algo->master_xfer(adap, msgs, num);
> > +
> >  		if (ret != -EAGAIN)
> >  			break;
> >  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 65b4eaed1d96..11e615123bd0 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
> >   *   defined by the msgs array, with num messages available to transfer via
> >   *   the adapter specified by adap.
> > + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> > + *   so e.g. PMICs can be accessed very late before shutdown
> >   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
> >   *   is not present, then the bus layer will try and convert the SMBus calls
> >   *   into I2C transfers instead.
> > @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
> >   * to name two of the most common.
> >   *
> > - * The return codes from the @master_xfer field should indicate the type of
> > - * error code that occurred during the transfer, as documented in the kernel
> > - * Documentation file Documentation/i2c/fault-codes.
> > + * The return codes from the @master_xfer{_irqless} field should indicate the
> > + * type of error code that occurred during the transfer, as documented in the
> > + * Kernel Documentation file Documentation/i2c/fault-codes.
> >   */
> >  struct i2c_algorithm {
> >  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> > @@ -524,6 +526,8 @@ struct i2c_algorithm {
> >  	   processed, or a negative value on error */
> >  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  			   int num);
> > +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> > +				   struct i2c_msg *msgs, int num);
> >  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
> >  			   unsigned short flags, char read_write,
> >  			   u8 command, int size, union i2c_smbus_data *data);
> > -- 
> > 2.18.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

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

Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[Tony Lindgren]

* Wolfram Sang <wsa+renesas@sang-engineering.com> [180920 09:19]:
> To keep the discussion about master_xfer_irqless going, I post here a
> draft how I envision the changes to the I2C core. They are only build
> tested. I am unsure if I can test them on hardware before next week, so
> I'll send them around as RFC already, so people can get an idea and
> comment. Maybe Stefan has some bandwidth to test his imx driver
> implementation on top of this?
> 
> A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless
> 
> Happy hacking and thanks for all the input to this,

Thanks looks nice to me:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[Wolfram Sang]

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


> Thanks looks nice to me:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

Thanks, Tony!


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

Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[Keerthy]

On Friday 21 September 2018 04:26 AM, Wolfram Sang wrote:
> 
>> Thanks looks nice to me:
>>
>> Acked-by: Tony Lindgren <tony@atomide.com>
> 
> Thanks, Tony!

Hi Wolfram Sang,

Posted: https://lore.kernel.org/patchwork/patch/1001457/

Thanks for the series! I tested for poweroff on am572x-idk.

Tested-by: Keerthy <j-keerthy@ti.com>

Regards,
Keerthy
> 

Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[Wolfram Sang]

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

On Thu, Sep 20, 2018 at 06:14:19PM +0200, Wolfram Sang wrote:
> To keep the discussion about master_xfer_irqless going, I post here a
> draft how I envision the changes to the I2C core. They are only build
> tested. I am unsure if I can test them on hardware before next week, so
> I'll send them around as RFC already, so people can get an idea and
> comment. Maybe Stefan has some bandwidth to test his imx driver
> implementation on top of this?
> 
> A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless

What I forgot to mention:

At some point, I'd like an adiitional check if the device is flagged to
have very late I2C transfers and trigger a fat warning if not. To keep
bugs visible.

I have ideas but not perfect yet, so I'll leave this as the next step.


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

Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless

[Stefan Lengfeld]

Hi Wolfram,

On Thu, Sep 20, 2018 at 06:14:19PM +0200, Wolfram Sang wrote:
> To keep the discussion about master_xfer_irqless going, I post here a
> draft how I envision the changes to the I2C core. They are only build
> tested. I am unsure if I can test them on hardware before next week, so
> I'll send them around as RFC already, so people can get an idea and
> comment. Maybe Stefan has some bandwidth to test his imx driver
> implementation on top of this?

No problem. I have some spare time in the next days and will post my
results to the mailinglist.

Kind Regards,
Stefan Lengfeld

Re: [RFC PATCH 0/3] i2c: core: introduce master_xfer_irqless

[Stefan Lengfeld]

Hi Wolfram,

I have ported my i.MX6 patches on your patch stack and tested the reboot
handler on my i.MX6 Solo Board.  Works without lockdep and non-atomic
warnings so far.

Kind Regards,
    Stefan


Stefan Christ (1):
  ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog

Stefan Lengfeld (2):
  i2c: imx: implement master_xfer_irqless callback
  watchdog: da9062: avoid regmap in restart handler

 arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts |   8 ++
 drivers/i2c/busses/i2c-imx.c                      | 128 ++++++++++++++++------
 drivers/mfd/da9062-core.c                         |   1 +
 drivers/watchdog/da9062_wdt.c                     |  17 ++-
 include/linux/mfd/da9062/core.h                   |   1 +
 5 files changed, 116 insertions(+), 39 deletions(-)

-- 
2.16.4

[RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback

[Stefan Lengfeld]

Rework the read and write code paths in the driver to support operation
in IRQ disabled contexts. The patch is currently tested only on a
phyCORE-i.MX6 Solo board.

The driver supports normal operation, DMA transfers and now the polling
mode or also called sleep-free or IRQ-less operation. It makes the code
not simpler or easier to read, but IRQ less I2C transfers are needed on
some hardware configurations, e.g. to trigger reboots on an external
PMIC chip.

Signed-off-by: Stefan Lengfeld <contact@stefanchrist.eu>
---
 drivers/i2c/busses/i2c-imx.c | 128 +++++++++++++++++++++++++++++++------------
 1 file changed, 93 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index c406700789e1..af72a1cbedbe 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -404,7 +404,7 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
 	dma->chan_using = NULL;
 }
 
-static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
+static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool polling)
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
@@ -434,15 +434,39 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
 				"<%s> I2C bus is busy\n", __func__);
 			return -ETIMEDOUT;
 		}
-		schedule();
+		if (!polling)
+			schedule();
+		else
+			udelay(100);
 	}
 
 	return 0;
 }
 
-static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool polling)
 {
-	wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+	if (!polling) {
+		wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+	} else {
+		int counter = 0;
+
+		while (1) {
+			unsigned int reg;
+
+			reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+			i2c_imx->i2csr = reg;
+			if (reg & I2SR_IIF)
+				break;
+
+			if (counter > 1000) {
+				dev_err(&i2c_imx->adapter.dev, "<%s> TXR timeout\n", __func__);
+				return -EIO;
+			}
+			udelay(100);
+			counter++;
+		}
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+	}
 
 	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
@@ -520,7 +544,7 @@ static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_start(struct imx_i2c_struct *i2c_imx, bool polling)
 {
 	unsigned int temp = 0;
 	int result;
@@ -533,23 +557,32 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 	imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR);
 
 	/* Wait controller to be stable */
-	usleep_range(50, 150);
+	if (!polling)
+		usleep_range(50, 150);
+	else
+		udelay(50);
 
 	/* Start I2C transaction */
 	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 	temp |= I2CR_MSTA;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-	result = i2c_imx_bus_busy(i2c_imx, 1);
+	result = i2c_imx_bus_busy(i2c_imx, 1, polling);
 	if (result)
 		return result;
 
-	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	if (!polling) {
+		temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	} else {
+		temp |= I2CR_MTX | I2CR_TXAK;
+		temp &= ~I2CR_IIEN; /* Disable interrupt */
+	}
+
 	temp &= ~I2CR_DMAEN;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 	return result;
 }
 
-static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool polling)
 {
 	unsigned int temp = 0;
 
@@ -571,7 +604,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 	}
 
 	if (!i2c_imx->stopped)
-		i2c_imx_bus_busy(i2c_imx, 0);
+		i2c_imx_bus_busy(i2c_imx, 0, polling);
 
 	/* Disable I2C controller */
 	temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
@@ -652,7 +685,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	/* The last data byte must be transferred by the CPU. */
 	imx_i2c_write_reg(msgs->buf[msgs->len-1],
 				i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 
@@ -711,7 +744,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 
 	msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
 	/* read n byte data */
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 
@@ -724,7 +757,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 		temp &= ~(I2CR_MSTA | I2CR_MTX);
 		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-		i2c_imx_bus_busy(i2c_imx, 0);
+		i2c_imx_bus_busy(i2c_imx, 0, false);
 	} else {
 		/*
 		 * For i2c master receiver repeat restart operation like:
@@ -742,7 +775,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 	return 0;
 }
 
-static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool polling)
 {
 	int i, result;
 
@@ -751,7 +784,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 
 	/* write slave address */
 	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, polling);
 	if (result)
 		return result;
 	result = i2c_imx_acked(i2c_imx);
@@ -765,7 +798,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 			"<%s> write byte: B%d=0x%X\n",
 			__func__, i, msgs->buf[i]);
 		imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
-		result = i2c_imx_trx_complete(i2c_imx);
+		result = i2c_imx_trx_complete(i2c_imx, polling);
 		if (result)
 			return result;
 		result = i2c_imx_acked(i2c_imx);
@@ -775,7 +808,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 	return 0;
 }
 
-static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool is_lastmsg)
+static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool is_lastmsg, bool polling)
 {
 	int i, result;
 	unsigned int temp;
@@ -788,7 +821,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 
 	/* write slave address */
 	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, polling);
 	if (result)
 		return result;
 	result = i2c_imx_acked(i2c_imx);
@@ -821,7 +854,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	for (i = 0; i < msgs->len; i++) {
 		u8 len = 0;
 
-		result = i2c_imx_trx_complete(i2c_imx);
+		result = i2c_imx_trx_complete(i2c_imx, polling);
 		if (result)
 			return result;
 		/*
@@ -849,7 +882,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 				temp &= ~(I2CR_MSTA | I2CR_MTX);
 				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-				i2c_imx_bus_busy(i2c_imx, 0);
+				i2c_imx_bus_busy(i2c_imx, 0, polling);
 			} else {
 				/*
 				 * For i2c master receiver repeat restart operation like:
@@ -880,8 +913,8 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	return 0;
 }
 
-static int i2c_imx_xfer(struct i2c_adapter *adapter,
-						struct i2c_msg *msgs, int num)
+static int i2c_imx_xfer_impl(struct i2c_adapter *adapter,
+			     struct i2c_msg *msgs, int num, bool polling)
 {
 	unsigned int i, temp;
 	int result;
@@ -895,11 +928,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 		goto out;
 
 	/* Start I2C transfer */
-	result = i2c_imx_start(i2c_imx);
+	result = i2c_imx_start(i2c_imx, polling);
 	if (result) {
 		if (i2c_imx->adapter.bus_recovery_info) {
 			i2c_recover_bus(&i2c_imx->adapter);
-			result = i2c_imx_start(i2c_imx);
+			result = i2c_imx_start(i2c_imx, polling);
 		}
 	}
 
@@ -917,7 +950,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 			temp |= I2CR_RSTA;
 			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-			result = i2c_imx_bus_busy(i2c_imx, 1);
+			result = i2c_imx_bus_busy(i2c_imx, 1, polling);
 			if (result)
 				goto fail0;
 		}
@@ -941,13 +974,17 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			(temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0),
 			(temp & I2SR_RXAK ? 1 : 0));
 #endif
-		if (msgs[i].flags & I2C_M_RD)
-			result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg);
-		else {
-			if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD)
-				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
-			else
-				result = i2c_imx_write(i2c_imx, &msgs[i]);
+		if (msgs[i].flags & I2C_M_RD) {
+			result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg, polling);
+		} else {
+			if (!polling) {
+				if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD)
+					result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
+				else
+					result = i2c_imx_write(i2c_imx, &msgs[i], polling);
+			} else {
+				result = i2c_imx_write(i2c_imx, &msgs[i], polling);
+			}
 		}
 		if (result)
 			goto fail0;
@@ -955,7 +992,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 
 fail0:
 	/* Stop I2C transfer */
-	i2c_imx_stop(i2c_imx);
+	i2c_imx_stop(i2c_imx, polling);
 
 	pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
 	pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
@@ -967,6 +1004,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 	return (result < 0) ? result : num;
 }
 
+static int i2c_imx_xfer(struct i2c_adapter *adapter,
+			struct i2c_msg *msgs, int num)
+{
+	return i2c_imx_xfer_impl(adapter, msgs, num, false);
+}
+
+static int i2c_imx_xfer_irqless(struct i2c_adapter *adapter,
+				struct i2c_msg *msgs, int num)
+{
+	return i2c_imx_xfer_impl(adapter, msgs, num, true);
+}
+
 static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
 {
 	struct imx_i2c_struct *i2c_imx;
@@ -1039,8 +1088,9 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
 }
 
 static const struct i2c_algorithm i2c_imx_algo = {
-	.master_xfer	= i2c_imx_xfer,
-	.functionality	= i2c_imx_func,
+	.master_xfer = i2c_imx_xfer,
+	.master_xfer_irqless = i2c_imx_xfer_irqless,
+	.functionality = i2c_imx_func,
 };
 
 static int i2c_imx_probe(struct platform_device *pdev)
@@ -1117,6 +1167,14 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	/* Set up platform driver data */
 	platform_set_drvdata(pdev, i2c_imx);
 
+	/*
+	 * Driver's PM callbacks are safe to be called in IRQ disabled
+	 * contexts. Providing this information to the PM subsystem is required
+	 * for the 'master_xfer_irqless' implementation that calls PM routines
+	 * in IRQ disabled/atomic contexts, too.
+	 */
+	pm_runtime_irq_safe(&pdev->dev);
+
 	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
-- 
2.16.4

Re: [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback

[Andy Shevchenko]

On Sun, Oct 07, 2018 at 05:39:35PM +0200, Stefan Lengfeld wrote:
> Rework the read and write code paths in the driver to support operation
> in IRQ disabled contexts. The patch is currently tested only on a
> phyCORE-i.MX6 Solo board.
> 
> The driver supports normal operation, DMA transfers and now the polling
> mode or also called sleep-free or IRQ-less operation. It makes the code
> not simpler or easier to read, but IRQ less I2C transfers are needed on
> some hardware configurations, e.g. to trigger reboots on an external
> PMIC chip.

> +		if (!polling)
> +			schedule();
> +		else
> +			udelay(100);

What the point to use negative conditional when it's pretty straightforward and
positive may be used?

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler

[Stefan Lengfeld]

Using i2c_transfer() directly to set the shutdown bit is more reliable
than using regmap in atomic contexts, because calls to 'schedule()' or
'sleep()' must be avoided in call code paths.

Tested on a phyCORE-i.MX6 Solo board.

Signed-off-by: Stefan Lengfeld <contact@stefanchrist.eu>
---
 drivers/mfd/da9062-core.c       |  1 +
 drivers/watchdog/da9062_wdt.c   | 17 +++++++++++++----
 include/linux/mfd/da9062/core.h |  1 +
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
index 9f6105906c09..b6a01054ba0f 100644
--- a/drivers/mfd/da9062-core.c
+++ b/drivers/mfd/da9062-core.c
@@ -609,6 +609,7 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, chip);
 	chip->dev = &i2c->dev;
+	chip->i2c = i2c;
 
 	if (!i2c->irq) {
 		dev_err(chip->dev, "No IRQ configured\n");
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index fe169d8e1fb2..ad6483d25f83 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -11,6 +11,7 @@
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
+#include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/jiffies.h>
 #include <linux/mfd/da9062/registers.h>
@@ -152,12 +153,20 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 			      void *data)
 {
 	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	struct i2c_client *client = wdt->hw->i2c;
+	__u8 buf[3] = {DA9062AA_CONTROL_F, DA9062AA_SHUTDOWN_MASK, 0x0};
+	struct i2c_msg msgs[1] = {
+		{
+			.addr = client->addr,
+			.flags = (client->flags & I2C_M_TEN),
+			.len = sizeof(buf),
+			.buf = buf,
+		}
+	};
 	int ret;
 
-	ret = regmap_write(wdt->hw->regmap,
-			   DA9062AA_CONTROL_F,
-			   DA9062AA_SHUTDOWN_MASK);
-	if (ret)
+	ret = i2c_transfer(client->adapter, msgs, sizeof(msgs));
+	if (ret < 0)
 		dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n",
 			  ret);
 
diff --git a/include/linux/mfd/da9062/core.h b/include/linux/mfd/da9062/core.h
index 74d33a01ddae..c994293b3aef 100644
--- a/include/linux/mfd/da9062/core.h
+++ b/include/linux/mfd/da9062/core.h
@@ -68,6 +68,7 @@ enum da9062_irqs {
 struct da9062 {
 	struct device *dev;
 	struct regmap *regmap;
+	struct i2c_client *i2c;
 	struct regmap_irq_chip_data *regmap_irq;
 	enum da9062_compatible_types chip_type;
 };
-- 
2.16.4

Re: [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler

[Andy Shevchenko]

On Sun, Oct 07, 2018 at 05:39:36PM +0200, Stefan Lengfeld wrote:
> Using i2c_transfer() directly to set the shutdown bit is more reliable
> than using regmap in atomic contexts, because calls to 'schedule()' or
> 'sleep()' must be avoided in call code paths.

>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> +#include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/jiffies.h>
>  #include <linux/mfd/da9062/registers.h>

The rule of thumb when extending a header inclusion block is that try to
squeeze the new inclusion in the longest sorted part of the list.

Rationale behind is easy maintenance.

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 3/3] ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog

[Stefan Lengfeld]

From: Stefan Christ <s.christ@phytec.de>

Rely on PMIC watchdog and reboot support. The i.MX6 internal watchdog
cannot be used, because it does not reset external PMIC voltages on
reset.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
Signed-off-by: Christian Hemp <c.hemp@phytec.de>
---
 arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts b/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts
index 9f7f9f98139d..04e2d5a1b998 100644
--- a/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts
+++ b/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts
@@ -62,3 +62,11 @@
 &usdhc1 {
 	status = "okay";
 };
+
+&wdog1 {
+	/*
+	 * Rely on PMIC reboot handler. Internal i.MX6 watchdog, that is also
+	 * used for reboot, does not reset all external PMIC voltages on reset.
+	 */
+	status = "disabled";
+};
-- 
2.16.4