[RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Wolfram Sang]

As promised here is my RFC to improve address spaces for I2C. This should give
i2c seperate address spaces for standard clients, 10 bit clients, and our own
slave clients. So, you can now have a 7 bit slave at 0x50 and a 10 bit slave at
0x050. Or, you can have a slave driver listening at some address and at the
same time have a client driver talking to this address. Note that this is only
the core support for that separation, I am still not sure if there is hardware
being able talking to its own slave address, but we will see.

This RFC and while I did some quick tests, it is not thoroughly tested. But I
wanted to push it out before I leave the computer for the weekend. It still
shows what path I chose to solve the problem. So, comments on that and further
testing are more than welcome!

BTW Andrey, I did not modify your patch and couldn't get the i2c-slave-eeprom driver
to work with my Jetson TK1. Does this work for you?

Thanks,

   Wolfram


Andrey Danin (1):
  i2c: tegra: implement slave mode

Wolfram Sang (8):
  dt-bindings: add header for generic I2C flags in bindings
  i2c: add a flag to mark clients as slaves
  i2c: apply address offset for slaves, too
  i2c: rename address check functions
  i2c: make address check indpendent from client struct
  i2c: apply DT flags when probing
  i2c: take address space into account when checking for used addresses
  dts: tegra: WIP: hack dts to test new dt flags for i2c

 arch/arm/boot/dts/tegra124-jetson-tk1.dts |   7 ++
 drivers/i2c/busses/Kconfig                |   1 +
 drivers/i2c/busses/i2c-tegra.c            | 119 ++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c                    |  69 +++++++++++------
 include/dt-bindings/i2c/i2c.h             |  18 +++++
 include/linux/i2c.h                       |   9 ++-
 6 files changed, 197 insertions(+), 26 deletions(-)
 create mode 100644 include/dt-bindings/i2c/i2c.h

-- 
2.1.4

[RFC 4/9] i2c: apply address offset for slaves, too

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

We want a separate address range for being an I2C slave. Add an offset
of 0x1000, so it can be combined with ten bit addresses as well. Add a
separate function to create the address value, we will need it later in
other places.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e6d4935161e490..a807e272e1fc2e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -776,6 +776,21 @@ struct i2c_client *i2c_verify_client(struct device *dev)
 EXPORT_SYMBOL(i2c_verify_client);
 
 
+/* Return a unique address which takes the flags of the client into account */
+static unsigned short i2c_encode_flags_to_addr(struct i2c_client *client)
+{
+	unsigned short addr = client->addr;
+
+	/* For some client flags, add an arbitrary offset to avoid collisions */
+	if (client->flags & I2C_CLIENT_TEN)
+		addr |= 0xa000;
+
+	if (client->flags & I2C_CLIENT_SLAVE)
+		addr |= 0x1000;
+
+	return addr;
+}
+
 /* This is a permissive address validity check, I2C address map constraints
  * are purposely not enforced, except for the general call address. */
 static int i2c_check_client_addr_validity(const struct i2c_client *client)
@@ -921,10 +936,8 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
 		return;
 	}
 
-	/* For 10-bit clients, add an arbitrary offset to avoid collisions */
 	dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap),
-		     client->addr | ((client->flags & I2C_CLIENT_TEN)
-				     ? 0xa000 : 0));
+		     i2c_encode_flags_to_addr(client));
 }
 
 /**
-- 
2.1.4

[RFC 5/9] i2c: rename address check functions

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

The current naming is based on the arguments of the functions and not on
what they do. Even I as the maintainer find this confusing, so let's
rename them to something more descriptive.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a807e272e1fc2e..b54a0f092d53f6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -793,7 +793,7 @@ static unsigned short i2c_encode_flags_to_addr(struct i2c_client *client)
 
 /* This is a permissive address validity check, I2C address map constraints
  * are purposely not enforced, except for the general call address. */
-static int i2c_check_client_addr_validity(const struct i2c_client *client)
+static int i2c_check_addr_validity(const struct i2c_client *client)
 {
 	if (client->flags & I2C_CLIENT_TEN) {
 		/* 10-bit address, all values are valid */
@@ -811,7 +811,7 @@ static int i2c_check_client_addr_validity(const struct i2c_client *client)
  * device uses a reserved address, then it shouldn't be probed. 7-bit
  * addressing is assumed, 10-bit address devices are rare and should be
  * explicitly enumerated. */
-static int i2c_check_addr_validity(unsigned short addr)
+static int i2c_check_7bit_addr_validity_strict(unsigned short addr)
 {
 	/*
 	 * Reserved addresses per I2C specification:
@@ -980,7 +980,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	strlcpy(client->name, info->type, sizeof(client->name));
 
 	/* Check for address validity */
-	status = i2c_check_client_addr_validity(client);
+	status = i2c_check_addr_validity(client);
 	if (status) {
 		dev_err(&adap->dev, "Invalid %d-bit I2C address 0x%02hx\n",
 			client->flags & I2C_CLIENT_TEN ? 10 : 7, client->addr);
@@ -2265,7 +2265,7 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 	int err;
 
 	/* Make sure the address is valid */
-	err = i2c_check_addr_validity(addr);
+	err = i2c_check_7bit_addr_validity_strict(addr);
 	if (err) {
 		dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
 			 addr);
@@ -2382,7 +2382,7 @@ i2c_new_probed_device(struct i2c_adapter *adap,
 
 	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
 		/* Check address validity */
-		if (i2c_check_addr_validity(addr_list[i]) < 0) {
+		if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) {
 			dev_warn(&adap->dev, "Invalid 7-bit address "
 				 "0x%02x\n", addr_list[i]);
 			continue;
@@ -2957,7 +2957,7 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
 
 	if (!(client->flags & I2C_CLIENT_TEN)) {
 		/* Enforce stricter address checking */
-		ret = i2c_check_addr_validity(client->addr);
+		ret = i2c_check_7bit_addr_validity_strict(client->addr);
 		if (ret) {
 			dev_err(&client->dev, "%s: invalid address\n", __func__);
 			return ret;
-- 
2.1.4

[RFC 7/9] i2c: apply DT flags when probing

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Check for slave and 10-bit flags when probing and mark the client when
found. Improve the address validity check, too

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b5719a2cff97bf..4a92eded4ece9c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -27,6 +27,7 @@
    I2C slave support (c) 2014 by Wolfram Sang <wsa@sang-engineering.com>
  */
 
+#include <dt-bindings/i2c/i2c.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
@@ -1283,7 +1284,8 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 	struct i2c_client *result;
 	struct i2c_board_info info = {};
 	struct dev_archdata dev_ad = {};
-	const __be32 *addr;
+	const __be32 *addr_be;
+	u32 addr;
 	int len;
 
 	dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
@@ -1294,20 +1296,31 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 		return ERR_PTR(-EINVAL);
 	}
 
-	addr = of_get_property(node, "reg", &len);
-	if (!addr || (len < sizeof(*addr))) {
+	addr_be = of_get_property(node, "reg", &len);
+	if (!addr_be || (len < sizeof(*addr_be))) {
 		dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
 			node->full_name);
 		return ERR_PTR(-EINVAL);
 	}
 
-	info.addr = be32_to_cpup(addr);
-	if (info.addr > (1 << 10) - 1) {
+	addr = be32_to_cpup(addr_be);
+	if (addr & I2C_TEN_BIT_ADDRESS) {
+		addr &= ~I2C_TEN_BIT_ADDRESS;
+		info.flags |= I2C_CLIENT_TEN;
+	}
+
+	if (addr & I2C_OWN_SLAVE_ADDRESS) {
+		addr &= ~I2C_OWN_SLAVE_ADDRESS;
+		info.flags |= I2C_CLIENT_SLAVE;
+	}
+
+	if (i2c_check_addr_validity(addr, info.flags)) {
 		dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
 			info.addr, node->full_name);
 		return ERR_PTR(-EINVAL);
 	}
 
+	info.addr = addr;
 	info.of_node = of_node_get(node);
 	info.archdata = &dev_ad;
 
-- 
2.1.4

[RFC 1/9] i2c: tegra: implement slave mode

[Wolfram Sang]

From: Andrey Danin <danindrey-JGs/UdohzUI@public.gmane.org>

Initialization code is based on NVEC driver.

There is a HW bug in AP20 that was also mentioned in kernel sources
for Toshiba AC100.

Signed-off-by: Andrey Danin <danindrey-JGs/UdohzUI@public.gmane.org>
Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/Kconfig     |   1 +
 drivers/i2c/busses/i2c-tegra.c | 119 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 577d58d1f1a198..6026271f6d1af5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -881,6 +881,7 @@ config I2C_SUN6I_P2WI
 config I2C_TEGRA
 	tristate "NVIDIA Tegra internal I2C controller"
 	depends on ARCH_TEGRA
+	select I2C_SLAVE
 	help
 	  If you say yes to this option, support will be included for the
 	  I2C controller embedded in NVIDIA Tegra SOCs
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 78a36681469674..dc080ca952960c 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -42,8 +42,17 @@
 #define I2C_SL_CNFG				0x020
 #define I2C_SL_CNFG_NACK			(1<<1)
 #define I2C_SL_CNFG_NEWSL			(1<<2)
+#define I2C_SL_RCVD				0x024
+#define I2C_SL_STATUS				0x028
+#define I2C_SL_ST_IRQ				(1<<3)
+#define I2C_SL_ST_END_TRANS			(1<<4)
+#define I2C_SL_ST_RCVD				(1<<2)
+#define I2C_SL_ST_RNW				(1<<1)
 #define I2C_SL_ADDR1				0x02c
 #define I2C_SL_ADDR2				0x030
+#define I2C_SL_ADDR2_TEN_BIT_MODE		1
+#define I2C_SL_DELAY_COUNT			0x03c
+#define I2C_SL_DELAY_COUNT_DEFAULT		0x1E
 #define I2C_TX_FIFO				0x050
 #define I2C_RX_FIFO				0x054
 #define I2C_PACKET_TRANSFER_STATUS		0x058
@@ -125,6 +134,8 @@ enum msg_end_type {
  * @clk_divisor_std_fast_mode: Clock divisor in standard/fast mode. It is
  *		applicable if there is no fast clock source i.e. single clock
  *		source.
+ * @slave_read_start_delay: Workaround for AP20 I2C Slave Controller bug. Delay
+ *              before writing data byte into register I2C_SL_RCVD.
  */
 
 struct tegra_i2c_hw_feature {
@@ -133,6 +144,7 @@ struct tegra_i2c_hw_feature {
 	bool has_single_clk_source;
 	int clk_divisor_hs_mode;
 	int clk_divisor_std_fast_mode;
+	int slave_read_start_delay;
 };
 
 /**
@@ -173,6 +185,7 @@ struct tegra_i2c_dev {
 	int msg_read;
 	u32 bus_clk_rate;
 	bool is_suspended;
+	struct i2c_client *slave;
 };
 
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -461,12 +474,78 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	return err;
 }
 
+static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
+{
+	i2c_writel(i2c_dev, val, I2C_SL_RCVD);
+
+	/*
+	 * TODO: A correct fix needs to be found for this.
+	 *
+	 * We experience less incomplete messages with this delay than without
+	 * it, but we don't know why. Help is appreciated.
+	 */
+	udelay(100);
+}
+
+static int tegra_i2c_slave_isr(int irq, struct tegra_i2c_dev *i2c_dev)
+{
+	u32 status;
+	u8 value;
+	u8 dummy;
+	u32 is_slave_irq, is_read, is_trans_start, is_trans_end;
+
+	if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
+		return -EINVAL;
+
+	status = i2c_readl(i2c_dev, I2C_SL_STATUS);
+
+	is_slave_irq = (status & I2C_SL_ST_IRQ);
+	is_read = (status & I2C_SL_ST_RNW);
+	is_trans_start = (status & I2C_SL_ST_RCVD);
+	is_trans_end = (status & I2C_SL_ST_END_TRANS);
+
+	if (!is_slave_irq)
+		return -EINVAL;
+
+	/* master sent stop */
+	if (is_trans_end) {
+		i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
+		if (!is_trans_start)
+			return 0;
+	}
+
+	if (is_read) {
+		/* i2c master reads data from us */
+		i2c_slave_event(i2c_dev->slave,
+				is_trans_start ? I2C_SLAVE_READ_REQUESTED
+					       : I2C_SLAVE_READ_PROCESSED,
+				&value);
+		if (is_trans_start && i2c_dev->hw->slave_read_start_delay)
+			udelay(i2c_dev->hw->slave_read_start_delay);
+		tegra_i2c_slave_write(i2c_dev, value);
+	} else {
+		/* i2c master sends data to us */
+		value = i2c_readl(i2c_dev, I2C_SL_RCVD);
+		if (is_trans_start)
+			tegra_i2c_slave_write(i2c_dev, 0);
+		i2c_slave_event(i2c_dev->slave,
+				is_trans_start ? I2C_SLAVE_WRITE_REQUESTED
+					       : I2C_SLAVE_WRITE_RECEIVED,
+				&value);
+	}
+
+	return 0;
+}
+
 static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 {
 	u32 status;
 	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
 	struct tegra_i2c_dev *i2c_dev = dev_id;
 
+	if (!tegra_i2c_slave_isr(irq, i2c_dev))
+		return IRQ_HANDLED;
+
 	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
 
 	if (status == 0) {
@@ -664,9 +743,48 @@ static u32 tegra_i2c_func(struct i2c_adapter *adap)
 	return ret;
 }
 
+static int tegra_reg_slave(struct i2c_client *slave)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+	int addr2 = 0;
+
+	if (i2c_dev->slave)
+		return -EBUSY;
+
+	i2c_dev->slave = slave;
+
+	tegra_i2c_clock_enable(i2c_dev);
+
+	i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
+	i2c_writel(i2c_dev, I2C_SL_DELAY_COUNT_DEFAULT, I2C_SL_DELAY_COUNT);
+
+	if (slave->addr > 0x7F)
+		addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
+
+	i2c_writel(i2c_dev, slave->addr, I2C_SL_ADDR1);
+	i2c_writel(i2c_dev, addr2, I2C_SL_ADDR2);
+
+	return 0;
+}
+
+static int tegra_unreg_slave(struct i2c_client *slave)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+
+	WARN_ON(!i2c_dev->slave);
+
+	i2c_writel(i2c_dev, 0, I2C_SL_CNFG);
+
+	i2c_dev->slave = NULL;
+
+	return 0;
+}
+
 static const struct i2c_algorithm tegra_i2c_algo = {
 	.master_xfer	= tegra_i2c_xfer,
 	.functionality	= tegra_i2c_func,
+	.reg_slave	= tegra_reg_slave,
+	.unreg_slave	= tegra_unreg_slave,
 };
 
 /* payload size is only 12 bit */
@@ -681,6 +799,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_single_clk_source = false,
 	.clk_divisor_hs_mode = 3,
 	.clk_divisor_std_fast_mode = 0,
+	.slave_read_start_delay = 8,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
-- 
2.1.4

[RFC 2/9] dt-bindings: add header for generic I2C flags in bindings

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 include/dt-bindings/i2c/i2c.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 include/dt-bindings/i2c/i2c.h

diff --git a/include/dt-bindings/i2c/i2c.h b/include/dt-bindings/i2c/i2c.h
new file mode 100644
index 00000000000000..1d5da81d90f144
--- /dev/null
+++ b/include/dt-bindings/i2c/i2c.h
@@ -0,0 +1,18 @@
+/*
+ * This header provides constants for I2C bindings
+ *
+ * Copyright (C) 2015 by Sang Engineering
+ * Copyright (C) 2015 by Renesas Electronics Corporation
+ *
+ * Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
+ *
+ * GPLv2 only
+ */
+
+#ifndef _DT_BINDINGS_I2C_I2C_H
+#define _DT_BINDINGS_I2C_I2C_H
+
+#define I2C_TEN_BIT_ADDRESS	(1 << 31)
+#define I2C_OWN_SLAVE_ADDRESS	(1 << 30)
+
+#endif
-- 
2.1.4

[RFC 3/9] i2c: add a flag to mark clients as slaves

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

And update indentation with one more tab, sigh...

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 include/linux/i2c.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e83a738a3b8741..51ed2e40932f3d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -550,11 +550,12 @@ void i2c_lock_adapter(struct i2c_adapter *);
 void i2c_unlock_adapter(struct i2c_adapter *);
 
 /*flags for the client struct: */
-#define I2C_CLIENT_PEC	0x04		/* Use Packet Error Checking */
-#define I2C_CLIENT_TEN	0x10		/* we have a ten bit chip address */
+#define I2C_CLIENT_PEC		0x04	/* Use Packet Error Checking */
+#define I2C_CLIENT_TEN		0x10	/* we have a ten bit chip address */
 					/* Must equal I2C_M_TEN below */
-#define I2C_CLIENT_WAKE	0x80		/* for board_info; true iff can wake */
-#define I2C_CLIENT_SCCB	0x9000		/* Use Omnivision SCCB protocol */
+#define I2C_CLIENT_SLAVE	0x20	/* we are the slave */
+#define I2C_CLIENT_WAKE		0x80	/* for board_info; true iff can wake */
+#define I2C_CLIENT_SCCB		0x9000	/* Use Omnivision SCCB protocol */
 					/* Must match I2C_M_STOP|IGNORE_NAK */
 
 /* i2c adapter classes (bitmask) */
-- 
2.1.4

[RFC 6/9] i2c: make address check indpendent from client struct

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

We want to use this function with struct boardinfo soon, so let's just
pass the parameters really needed. We also extend the type of addr, so
more types can be input. Remove a superfluous dangling comment while
here.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b54a0f092d53f6..b5719a2cff97bf 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -793,15 +793,15 @@ static unsigned short i2c_encode_flags_to_addr(struct i2c_client *client)
 
 /* This is a permissive address validity check, I2C address map constraints
  * are purposely not enforced, except for the general call address. */
-static int i2c_check_addr_validity(const struct i2c_client *client)
+static int i2c_check_addr_validity(unsigned addr, unsigned short flags)
 {
-	if (client->flags & I2C_CLIENT_TEN) {
+	if (flags & I2C_CLIENT_TEN) {
 		/* 10-bit address, all values are valid */
-		if (client->addr > 0x3ff)
+		if (addr > 0x3ff)
 			return -EINVAL;
 	} else {
 		/* 7-bit address, reject the general call address */
-		if (client->addr == 0x00 || client->addr > 0x7f)
+		if (addr == 0x00 || addr > 0x7f)
 			return -EINVAL;
 	}
 	return 0;
@@ -979,8 +979,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 
 	strlcpy(client->name, info->type, sizeof(client->name));
 
-	/* Check for address validity */
-	status = i2c_check_addr_validity(client);
+	status = i2c_check_addr_validity(client->addr, client->flags);
 	if (status) {
 		dev_err(&adap->dev, "Invalid %d-bit I2C address 0x%02hx\n",
 			client->flags & I2C_CLIENT_TEN ? 10 : 7, client->addr);
-- 
2.1.4

[RFC 8/9] i2c: take address space into account when checking for used addresses

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

It is not enough to compare the plain address value, we also need to
check the flags enabling a different address space. E.g. it is valid to
have address 0x50 as a 7-bit address and 0x050 as 10-bit address on the
same bus. Same for addresses when we are the slave.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 4a92eded4ece9c..021ff232beaae3 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -834,7 +834,7 @@ static int __i2c_check_addr_busy(struct device *dev, void *addrp)
 	struct i2c_client	*client = i2c_verify_client(dev);
 	int			addr = *(int *)addrp;
 
-	if (client && client->addr == addr)
+	if (client && i2c_encode_flags_to_addr(client) == addr)
 		return -EBUSY;
 	return 0;
 }
@@ -988,7 +988,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	}
 
 	/* Check for address business */
-	status = i2c_check_addr_busy(adap, client->addr);
+	status = i2c_check_addr_busy(adap, i2c_encode_flags_to_addr(client));
 	if (status)
 		goto out_err;
 
@@ -2284,7 +2284,7 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 		return err;
 	}
 
-	/* Skip if already in use */
+	/* Skip if already in use (7 bit, no need to encode flags) */
 	if (i2c_check_addr_busy(adapter, addr))
 		return 0;
 
@@ -2400,7 +2400,7 @@ i2c_new_probed_device(struct i2c_adapter *adap,
 			continue;
 		}
 
-		/* Check address availability */
+		/* Check address availability (7 bit, no need to encode flags) */
 		if (i2c_check_addr_busy(adap, addr_list[i])) {
 			dev_dbg(&adap->dev, "Address 0x%02x already in "
 				"use, not probing\n", addr_list[i]);
-- 
2.1.4

[RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Wolfram Sang]

Not-Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index bd43ed6d6ec7c0..4d5f2a4c4da1ce 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1,5 +1,6 @@
 /dts-v1/;
 
+#include <dt-bindings/i2c/i2c.h>
 #include <dt-bindings/input/input.h>
 #include "tegra124.dtsi"
 
@@ -1390,6 +1391,12 @@
 			reg = <0x56>;
 			pagesize = <8>;
 		};
+
+		eeprom@42 {
+			compatible = "linux,slave-24c02";
+			//FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
+			reg = <0xc0000042>;
+		};
 	};
 
 	/* Expansion GEN2_I2C_* */
-- 
2.1.4

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Wolfram Sang]

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

> +			//FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
> +			reg = <0xc0000042>;

That should have been 0x40000042, sorry!


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

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Andrey Danin]

On 17.07.2015 17:08, Wolfram Sang wrote:
> +
> +		eeprom@42 {
> +			compatible = "linux,slave-24c02";
> +			//FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
> +			reg = <0xc0000042>;

I used it in this way:
reg = <(I2C_OWN_SLAVE_ADDRESS | 0x42)>;

> +		};
>   	};
>
>   	/* Expansion GEN2_I2C_* */
>

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Wolfram Sang]

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


> >+			//FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
> >+			reg = <0xc0000042>;
> 
> I used it in this way:
> reg = <(I2C_OWN_SLAVE_ADDRESS | 0x42)>;

Ah, nice, it was that easy :) Thanks!


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

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 17 July 2015 16:08:29 Wolfram Sang wrote:
> Not-Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra124-jetson-tk1.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts index
> bd43ed6d6ec7c0..4d5f2a4c4da1ce 100644
> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> @@ -1,5 +1,6 @@
>  /dts-v1/;
> 
> +#include <dt-bindings/i2c/i2c.h>
>  #include <dt-bindings/input/input.h>
>  #include "tegra124.dtsi"
> 
> @@ -1390,6 +1391,12 @@
>  			reg = <0x56>;
>  			pagesize = <8>;
>  		};
> +
> +		eeprom@42 {
> +			compatible = "linux,slave-24c02";
> +			//FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
> +			reg = <0xc0000042>;

The node name doesn't match the reg property anymore. Isn't that considered as 
a problem ?

> +		};
>  	};
> 
>  	/* Expansion GEN2_I2C_* */

-- 
Regards,

Laurent Pinchart

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Wolfram Sang]

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


> > +
> > +		eeprom@42 {
> > +			compatible = "linux,slave-24c02";
> > +			//FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
> > +			reg = <0xc0000042>;
> 
> The node name doesn't match the reg property anymore. Isn't that considered as 
> a problem ?

Hmm, true. So far, Rob (CCed) was fine with this approach:
http://www.spinics.net/lists/linux-tegra/msg22760.html

@Rob: If we introduce flag bits in the MSBs of an I2C address, the reg
property is different from the node name. Is this a problem?

Thanks,

   Wolfram


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

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Rob Herring]

On Mon, Jul 20, 2015 at 3:45 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> > +
>> > +           eeprom@42 {
>> > +                   compatible = "linux,slave-24c02";
>> > +                   //FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
>> > +                   reg = <0xc0000042>;
>>
>> The node name doesn't match the reg property anymore. Isn't that considered as
>> a problem ?
>
> Hmm, true. So far, Rob (CCed) was fine with this approach:
> http://www.spinics.net/lists/linux-tegra/msg22760.html
>
> @Rob: If we introduce flag bits in the MSBs of an I2C address, the reg
> property is different from the node name. Is this a problem?

No, I don't it is a problem.

Rob

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Stephen Warren]

On 07/20/2015 10:10 AM, Rob Herring wrote:
> On Mon, Jul 20, 2015 at 3:45 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>>
>>>> +
>>>> +           eeprom@42 {
>>>> +                   compatible = "linux,slave-24c02";
>>>> +                   //FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
>>>> +                   reg = <0xc0000042>;
>>>
>>> The node name doesn't match the reg property anymore. Isn't that considered as
>>> a problem ?
>>
>> Hmm, true. So far, Rob (CCed) was fine with this approach:
>> http://www.spinics.net/lists/linux-tegra/msg22760.html
>>
>> @Rob: If we introduce flag bits in the MSBs of an I2C address, the reg
>> property is different from the node name. Is this a problem?
>
> No, I don't it is a problem.

The rule so far has been that the unit address (the value in the node 
name) must match the first value in the reg property. I don't see why 
this rule should change. To solve this, just name the node 
eeprom@c0000042 (or eeprom@40000042 with the correction pointed out 
earlier in the thread).

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Wolfram Sang]

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


> >>@Rob: If we introduce flag bits in the MSBs of an I2C address, the reg
> >>property is different from the node name. Is this a problem?
> >
> >No, I don't it is a problem.
> 
> The rule so far has been that the unit address (the value in the node name)
> must match the first value in the reg property. I don't see why this rule
> should change. To solve this, just name the node eeprom@c0000042 (or
> eeprom@40000042 with the correction pointed out earlier in the thread).

We can do that; that would mean that people need to find out the values
of the #define which will be used in the reg property. It works, but
will be cumbersome IMO.


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

Re: [RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Stephen Warren]

On 07/17/2015 08:08 AM, Wolfram Sang wrote:
> As promised here is my RFC to improve address spaces for I2C. This should give
> i2c seperate address spaces for standard clients, 10 bit clients, and our own
> slave clients. So, you can now have a 7 bit slave at 0x50 and a 10 bit slave at
> 0x050. Or, you can have a slave driver listening at some address and at the
> same time have a client driver talking to this address. Note that this is only
> the core support for that separation, I am still not sure if there is hardware
> being able talking to its own slave address, but we will see.
>
> This RFC and while I did some quick tests, it is not thoroughly tested. But I
> wanted to push it out before I leave the computer for the weekend. It still
> shows what path I chose to solve the problem. So, comments on that and further
> testing are more than welcome!
>
> BTW Andrey, I did not modify your patch and couldn't get the i2c-slave-eeprom driver
> to work with my Jetson TK1. Does this work for you?

This approach makes sense to me.

I'd expect patch 2/9 "dt-bindings: add header for generic I2C flags in 
bindings" to document the flags (or at least mention their existence, 
and point at the new header file) in the core I2C bindings document.

Please consider the series,
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

(ack rather than review since I didn't review patch 1, and mostly 
concentrated on reviewing the concepts of how slaves were represented 
rather than the coding details).

Re: [RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Wolfram Sang]

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

On Mon, Jul 20, 2015 at 04:13:13PM -0600, Stephen Warren wrote:
> On 07/17/2015 08:08 AM, Wolfram Sang wrote:
> >As promised here is my RFC to improve address spaces for I2C. This should give
> >i2c seperate address spaces for standard clients, 10 bit clients, and our own
> >slave clients. So, you can now have a 7 bit slave at 0x50 and a 10 bit slave at
> >0x050. Or, you can have a slave driver listening at some address and at the
> >same time have a client driver talking to this address. Note that this is only
> >the core support for that separation, I am still not sure if there is hardware
> >being able talking to its own slave address, but we will see.
> >
> >This RFC and while I did some quick tests, it is not thoroughly tested. But I
> >wanted to push it out before I leave the computer for the weekend. It still
> >shows what path I chose to solve the problem. So, comments on that and further
> >testing are more than welcome!
> >
> >BTW Andrey, I did not modify your patch and couldn't get the i2c-slave-eeprom driver
> >to work with my Jetson TK1. Does this work for you?
> 
> This approach makes sense to me.

\o/

> I'd expect patch 2/9 "dt-bindings: add header for generic I2C flags in
> bindings" to document the flags (or at least mention their existence, and
> point at the new header file) in the core I2C bindings document.

Yes. I forgot to say that docs are missing. I wanted to get some
feedback first. This series is a good reason to finally start the core
I2C binding document.

> Please consider the series,
> Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> (ack rather than review since I didn't review patch 1, and mostly
> concentrated on reviewing the concepts of how slaves were represented rather
> than the coding details).

Thanks, exactly that I was looking for this RFC. Will try to send out
proper patches this week.


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

Re: [RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Andrey Danin]

Hello Wolfram,

On 17.07.2015 17:08, Wolfram Sang wrote:
> As promised here is my RFC to improve address spaces for I2C. This should give
> i2c seperate address spaces for standard clients, 10 bit clients, and our own
> slave clients. So, you can now have a 7 bit slave at 0x50 and a 10 bit slave at
> 0x050. Or, you can have a slave driver listening at some address and at the
> same time have a client driver talking to this address. Note that this is only
> the core support for that separation, I am still not sure if there is hardware
> being able talking to its own slave address, but we will see.
>
> This RFC and while I did some quick tests, it is not thoroughly tested. But I
> wanted to push it out before I leave the computer for the weekend. It still
> shows what path I chose to solve the problem. So, comments on that and further
> testing are more than welcome!
>
> BTW Andrey, I did not modify your patch and couldn't get the i2c-slave-eeprom driver
> to work with my Jetson TK1. Does this work for you?
>
> Thanks,
>
>     Wolfram
>

Thanks for the patches. Slave mode works for me.

The series looks good. Only hardcoded values in patch 4 confuse me a little.

Re: [RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Wolfram Sang]

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


> Thanks for the patches. Slave mode works for me.

Okay, need to have a closer look this week. I combined two I2C busses on
my Jetson TK1. While the initialization of the slave works fine, reading
from the slave via the second I2C controller does not work (yet).

> The series looks good.

Great. I'd be happy for Acked-by or Tested-by tags if possible.

> Only hardcoded values in patch 4 confuse me a little.

Yes, they will most likely be replaced by defines in the next series. We
will need them in another place, too. However, since userspace relies on
them, they should never be changed.


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

Re: [RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Andrey Danin]

On 17.07.2015 17:08, Wolfram Sang wrote:
> As promised here is my RFC to improve address spaces for I2C. This should give
> i2c seperate address spaces for standard clients, 10 bit clients, and our own
> slave clients. So, you can now have a 7 bit slave at 0x50 and a 10 bit slave at
> 0x050. Or, you can have a slave driver listening at some address and at the
> same time have a client driver talking to this address. Note that this is only
> the core support for that separation, I am still not sure if there is hardware
> being able talking to its own slave address, but we will see.
>
> This RFC and while I did some quick tests, it is not thoroughly tested. But I
> wanted to push it out before I leave the computer for the weekend. It still
> shows what path I chose to solve the problem. So, comments on that and further
> testing are more than welcome!
>
> BTW Andrey, I did not modify your patch and couldn't get the i2c-slave-eeprom driver
> to work with my Jetson TK1. Does this work for you?
>
> Thanks,
>
>     Wolfram
>
>
> Andrey Danin (1):
>    i2c: tegra: implement slave mode
>
> Wolfram Sang (8):
>    dt-bindings: add header for generic I2C flags in bindings
>    i2c: add a flag to mark clients as slaves
>    i2c: apply address offset for slaves, too
>    i2c: rename address check functions
>    i2c: make address check indpendent from client struct
>    i2c: apply DT flags when probing
>    i2c: take address space into account when checking for used addresses
>    dts: tegra: WIP: hack dts to test new dt flags for i2c
>
>   arch/arm/boot/dts/tegra124-jetson-tk1.dts |   7 ++
>   drivers/i2c/busses/Kconfig                |   1 +
>   drivers/i2c/busses/i2c-tegra.c            | 119 ++++++++++++++++++++++++++++++
>   drivers/i2c/i2c-core.c                    |  69 +++++++++++------
>   include/dt-bindings/i2c/i2c.h             |  18 +++++
>   include/linux/i2c.h                       |   9 ++-
>   6 files changed, 197 insertions(+), 26 deletions(-)
>   create mode 100644 include/dt-bindings/i2c/i2c.h
>

The series is
Tested-by: Andrey Danin <danindrey@mail.ru>