[PATCH 00/18] i2c: make creation of 8 bit addresses consistent

[PATCH 00/18] i2c: make creation of 8 bit addresses consistent

[Wolfram Sang]

This series addresses two things which have come up in the past:

* document that I2C_M_RD is bit 0. It is not good coding style but some
  drivers (and userspace) silently assume so anyhow. To avoid regressions,
  let's make that explicit.

* use one function to generate an 8 bit address (7 bit address + RW bit)
  from a struct msg. People did this in various ways, sometimes overly
  brave and sometimes overly anxious with the operator precedence. That
  was partly not nice to read and partly triggering code checkers.

The new function was tested using a Renesas Lager board with the i2c-sh_mobile
driver. Buildbot was happy with the series, too. A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/8bit_macro

I'd really appreciate more tests on real HW. Also happy for other comments,
reviews...

Thanks,

   Wolfram


Wolfram Sang (18):
  i2c: guarantee that I2C_M_RD will be 0x0001 forever
  i2c: introduce helper function to get 8 bit address from a message
  i2c: core: use new 8 bit address helper function
  i2c: i2c-bcm-iproc: use new 8 bit address helper function
  i2c: i2c-bcm-kona: use new 8 bit address helper function
  i2c: i2c-brcmstb: use new 8 bit address helper function
  i2c: i2c-cpm: use new 8 bit address helper function
  i2c: i2c-ibm_iic: use new 8 bit address helper function
  i2c: i2c-img-scb: use new 8 bit address helper function
  i2c: i2c-iop3xx: use new 8 bit address helper function
  i2c: i2c-lpc2k: use new 8 bit address helper function
  i2c: i2c-mt65xx: use new 8 bit address helper function
  i2c: i2c-ocores: use new 8 bit address helper function
  i2c: i2c-powermac: use new 8 bit address helper function
  i2c: i2c-qup: use new 8 bit address helper function
  i2c: i2c-sh_mobile: use new 8 bit address helper function
  i2c: i2c-sirf: use new 8 bit address helper function
  i2c: i2c-st: use new 8 bit address helper function

 drivers/i2c/busses/i2c-bcm-iproc.c |  2 +-
 drivers/i2c/busses/i2c-bcm-kona.c  |  5 +----
 drivers/i2c/busses/i2c-brcmstb.c   |  4 +---
 drivers/i2c/busses/i2c-cpm.c       |  4 +---
 drivers/i2c/busses/i2c-ibm_iic.c   |  2 +-
 drivers/i2c/busses/i2c-img-scb.c   |  4 +---
 drivers/i2c/busses/i2c-iop3xx.c    |  5 +----
 drivers/i2c/busses/i2c-lpc2k.c     |  4 +---
 drivers/i2c/busses/i2c-mt65xx.c    |  5 +----
 drivers/i2c/busses/i2c-ocores.c    |  5 +----
 drivers/i2c/busses/i2c-powermac.c  |  4 +---
 drivers/i2c/busses/i2c-qup.c       |  2 +-
 drivers/i2c/busses/i2c-sh_mobile.c |  3 +--
 drivers/i2c/busses/i2c-sirf.c      |  4 +---
 drivers/i2c/busses/i2c-st.c        |  3 +--
 drivers/i2c/i2c-core.c             |  2 +-
 include/linux/i2c.h                |  5 +++++
 include/uapi/linux/i2c.h           | 13 +++++++------
 18 files changed, 28 insertions(+), 48 deletions(-)

-- 
2.7.0

[PATCH 01/18] i2c: guarantee that I2C_M_RD will be 0x0001 forever

[Wolfram Sang]

There is code out there in user space and kernel space which relies on
I2C_M_RD being bit 0 to simplify their bit operations. Add a comment to
make sure this will never break. Do proper sorting of the defines while
we are here.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 include/uapi/linux/i2c.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index b0a7dd61eb353a..adcbef4bff610a 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -68,14 +68,15 @@
 struct i2c_msg {
 	__u16 addr;	/* slave address			*/
 	__u16 flags;
-#define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
 #define I2C_M_RD		0x0001	/* read data, from slave to master */
-#define I2C_M_STOP		0x8000	/* if I2C_FUNC_PROTOCOL_MANGLING */
-#define I2C_M_NOSTART		0x4000	/* if I2C_FUNC_NOSTART */
-#define I2C_M_REV_DIR_ADDR	0x2000	/* if I2C_FUNC_PROTOCOL_MANGLING */
-#define I2C_M_IGNORE_NAK	0x1000	/* if I2C_FUNC_PROTOCOL_MANGLING */
-#define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
+					/* I2C_M_RD is guaranteed to be 0x0001! */
+#define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
+#define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
+#define I2C_M_IGNORE_NAK	0x1000	/* if I2C_FUNC_PROTOCOL_MANGLING */
+#define I2C_M_REV_DIR_ADDR	0x2000	/* if I2C_FUNC_PROTOCOL_MANGLING */
+#define I2C_M_NOSTART		0x4000	/* if I2C_FUNC_NOSTART */
+#define I2C_M_STOP		0x8000	/* if I2C_FUNC_PROTOCOL_MANGLING */
 	__u16 len;		/* msg length				*/
 	__u8 *buf;		/* pointer to msg data			*/
 };
-- 
2.7.0

[PATCH 02/18] i2c: introduce helper function to get 8 bit address from a message

[Wolfram Sang]

Drivers do this in various ways, let's use one standard way of doing it.
Note: I2C_M_RD is bit 0, so the code could be simplified. To be extremly
robust and to advertise good coding practices, I still use the ternary
operator and let the compilers do the optimizing job.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 include/linux/i2c.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 200cf13b00f620..c30833b7b0730d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -654,6 +654,11 @@ static inline int i2c_adapter_id(struct i2c_adapter *adap)
 	return adap->nr;
 }
 
+static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
+{
+	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
+}
+
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
  * @__i2c_driver: i2c_driver struct
-- 
2.7.0

[PATCH 03/18] i2c: core: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 0f2f8484e8ec1f..778d316de9f8ca 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2652,7 +2652,7 @@ static u8 i2c_smbus_pec(u8 crc, u8 *p, size_t count)
 static u8 i2c_smbus_msg_pec(u8 pec, struct i2c_msg *msg)
 {
 	/* The address will be sent first */
-	u8 addr = (msg->addr << 1) | !!(msg->flags & I2C_M_RD);
+	u8 addr = i2c_8bit_addr_from_msg(msg);
 	pec = i2c_smbus_pec(pec, &addr, 1);
 
 	/* The data buffer follows */
-- 
2.7.0

[PATCH 04/18] i2c: i2c-bcm-iproc: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index b9f0fff4e723a0..19c843828fe2ca 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -267,7 +267,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	iproc_i2c->msg = msg;
 
 	/* format and load slave address into the TX FIFO */
-	addr = msg->addr << 1 | (msg->flags & I2C_M_RD ? 1 : 0);
+	addr = i2c_8bit_addr_from_msg(msg);
 	writel(addr, iproc_i2c->base + M_TX_OFFSET);
 
 	/*
-- 
2.7.0

[PATCH 05/18] i2c: i2c-bcm-kona: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-bcm-kona.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-kona.c b/drivers/i2c/busses/i2c-bcm-kona.c
index 2c9d9b1c8e6441..ac9f47679c3a4b 100644
--- a/drivers/i2c/busses/i2c-bcm-kona.c
+++ b/drivers/i2c/busses/i2c-bcm-kona.c
@@ -501,10 +501,7 @@ static int bcm_kona_i2c_do_addr(struct bcm_kona_i2c_dev *dev,
 				return -EREMOTEIO;
 		}
 	} else {
-		addr = msg->addr << 1;
-
-		if (msg->flags & I2C_M_RD)
-			addr |= 1;
+		addr = i2c_8bit_addr_from_msg(msg);
 
 		if (bcm_kona_i2c_write_byte(dev, addr, 0) < 0)
 			return -EREMOTEIO;
-- 
2.7.0

[PATCH 06/18] i2c: i2c-brcmstb: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-brcmstb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 4a45408dd82060..6a8cfc1344b2ed 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -446,9 +446,7 @@ static int brcmstb_i2c_do_addr(struct brcmstb_i2c_dev *dev,
 
 		}
 	} else {
-		addr = msg->addr << 1;
-		if (msg->flags & I2C_M_RD)
-			addr |= 1;
+		addr = i2c_8bit_addr_from_msg(msg);
 
 		bsc_writel(dev, addr, chip_address);
 	}
-- 
2.7.0

[PATCH 07/18] i2c: i2c-cpm: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-cpm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
index 714bdc837769fd..b6d2821aaabd5e 100644
--- a/drivers/i2c/busses/i2c-cpm.c
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -197,9 +197,7 @@ static void cpm_i2c_parse_message(struct i2c_adapter *adap,
 	tbdf = cpm->tbase + tx;
 	rbdf = cpm->rbase + rx;
 
-	addr = pmsg->addr << 1;
-	if (pmsg->flags & I2C_M_RD)
-		addr |= 1;
+	addr = i2c_8bit_addr_from_msg(pmsg);
 
 	tb = cpm->txbuf[tx];
 	rb = cpm->rxbuf[rx];
-- 
2.7.0

[PATCH 08/18] i2c: i2c-ibm_iic: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-ibm_iic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index b6c08033429734..cdaa7be2cd1b94 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -269,7 +269,7 @@ static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p)
 	ndelay(t->hd_sta);
 
 	/* Send address */
-	v = (u8)((p->addr << 1) | ((p->flags & I2C_M_RD) ? 1 : 0));
+	v = i2c_8bit_addr_from_msg(p);
 	for (i = 0, mask = 0x80; i < 8; ++i, mask >>= 1){
 		out_8(&iic->directcntl, sda);
 		ndelay(t->low / 2);
-- 
2.7.0

[PATCH 09/18] i2c: i2c-img-scb: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-img-scb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 379ef9c316645a..ea20425b697217 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -751,9 +751,7 @@ static unsigned int img_i2c_atomic(struct img_i2c *i2c,
 	switch (i2c->at_cur_cmd) {
 	case CMD_GEN_START:
 		next_cmd = CMD_GEN_DATA;
-		next_data = (i2c->msg.addr << 1);
-		if (i2c->msg.flags & I2C_M_RD)
-			next_data |= 0x1;
+		next_data = i2c_8bit_addr_from_msg(&i2c->msg);
 		break;
 	case CMD_GEN_DATA:
 		if (i2c->line_status & LINESTAT_INPUT_HELD_V)
-- 
2.7.0

[PATCH 10/18] i2c: i2c-iop3xx: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-iop3xx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-iop3xx.c b/drivers/i2c/busses/i2c-iop3xx.c
index 72d6161cf77c0d..85cbe4b5557861 100644
--- a/drivers/i2c/busses/i2c-iop3xx.c
+++ b/drivers/i2c/busses/i2c-iop3xx.c
@@ -50,10 +50,7 @@ iic_cook_addr(struct i2c_msg *msg)
 {
 	unsigned char addr;
 
-	addr = (msg->addr << 1);
-
-	if (msg->flags & I2C_M_RD)
-		addr |= 1;
+	addr = i2c_8bit_addr_from_msg(msg);
 
 	return addr;
 }
-- 
2.7.0

[PATCH 11/18] i2c: i2c-lpc2k: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-lpc2k.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-lpc2k.c b/drivers/i2c/busses/i2c-lpc2k.c
index 8560a13bf1b308..586a15205e61de 100644
--- a/drivers/i2c/busses/i2c-lpc2k.c
+++ b/drivers/i2c/busses/i2c-lpc2k.c
@@ -133,9 +133,7 @@ static void i2c_lpc2k_pump_msg(struct lpc2k_i2c *i2c)
 	case M_START:
 	case M_REPSTART:
 		/* Start bit was just sent out, send out addr and dir */
-		data = i2c->msg->addr << 1;
-		if (i2c->msg->flags & I2C_M_RD)
-			data |= 1;
+		data = i2c_8bit_addr_from_msg(i2c->msg);
 
 		writel(data, i2c->base + LPC24XX_I2DAT);
 		writel(LPC24XX_STA, i2c->base + LPC24XX_I2CONCLR);
-- 
2.7.0

[PATCH 12/18] i2c: i2c-mt65xx: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mt65xx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 453358b4d9ca7a..d9373e60be8ae3 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -413,10 +413,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	else
 		writew(I2C_FS_START_CON, i2c->base + OFFSET_EXT_CONF);
 
-	addr_reg = msgs->addr << 1;
-	if (i2c->op == I2C_MASTER_RD)
-		addr_reg |= 0x1;
-
+	addr_reg = i2c_8bit_addr_from_msg(msgs);
 	writew(addr_reg, i2c->base + OFFSET_SLAVE_ADDR);
 
 	/* Clear interrupt status */
-- 
2.7.0

[PATCH 13/18] i2c: i2c-ocores: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-ocores.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 11b7b87311ed2f..dfa7a4b4a91d78 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -178,10 +178,7 @@ static void ocores_process(struct ocores_i2c *i2c)
 		if (i2c->nmsgs) {	/* end? */
 			/* send start? */
 			if (!(msg->flags & I2C_M_NOSTART)) {
-				u8 addr = (msg->addr << 1);
-
-				if (msg->flags & I2C_M_RD)
-					addr |= 1;
+				u8 addr = i2c_8bit_addr_from_msg(msg);
 
 				i2c->state = STATE_START;
 
-- 
2.7.0

[PATCH 14/18] i2c: i2c-powermac: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-powermac.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-powermac.c b/drivers/i2c/busses/i2c-powermac.c
index 6abcf696e3594b..b0d9dee14a7e0b 100644
--- a/drivers/i2c/busses/i2c-powermac.c
+++ b/drivers/i2c/busses/i2c-powermac.c
@@ -150,13 +150,11 @@ static int i2c_powermac_master_xfer(	struct i2c_adapter *adap,
 {
 	struct pmac_i2c_bus	*bus = i2c_get_adapdata(adap);
 	int			rc = 0;
-	int			read;
 	int			addrdir;
 
 	if (msgs->flags & I2C_M_TEN)
 		return -EINVAL;
-	read = (msgs->flags & I2C_M_RD) != 0;
-	addrdir = (msgs->addr << 1) | read;
+	addrdir = i2c_8bit_addr_from_msg(msgs);
 
 	rc = pmac_i2c_open(bus, 0);
 	if (rc) {
-- 
2.7.0

[PATCH 15/18] i2c: i2c-qup: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-qup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 23eaabb19f961a..cc6439ab3f7141 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -515,7 +515,7 @@ static int qup_i2c_get_data_len(struct qup_i2c_dev *qup)
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
 			    struct i2c_msg *msg,  int is_dma)
 {
-	u16 addr = (msg->addr << 1) | ((msg->flags & I2C_M_RD) == I2C_M_RD);
+	u16 addr = i2c_8bit_addr_from_msg(msg);
 	int len = 0;
 	int data_len;
 
-- 
2.7.0

[PATCH 16/18] i2c: i2c-sh_mobile: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 7d2bd3ec2d2d25..6fb3e264599229 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -398,8 +398,7 @@ static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
 {
 	switch (pd->pos) {
 	case -1:
-		*buf = (pd->msg->addr & 0x7f) << 1;
-		*buf |= (pd->msg->flags & I2C_M_RD) ? 1 : 0;
+		*buf = i2c_8bit_addr_from_msg(pd->msg);
 		break;
 	default:
 		*buf = pd->msg->buf[pd->pos];
-- 
2.7.0

[PATCH 17/18] i2c: i2c-sirf: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-sirf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index 13e51ef6af73df..792a42bdd335b9 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -190,9 +190,7 @@ static void i2c_sirfsoc_set_address(struct sirfsoc_i2c *siic,
 
 	writel(regval, siic->base + SIRFSOC_I2C_CMD(siic->cmd_ptr++));
 
-	addr = msg->addr << 1;	/* Generate address */
-	if (msg->flags & I2C_M_RD)
-		addr |= 1;
+	addr = i2c_8bit_addr_from_msg(msg);
 
 	/* Reverse direction bit */
 	if (msg->flags & I2C_M_REV_DIR_ADDR)
-- 
2.7.0

[PATCH 18/18] i2c: i2c-st: use new 8 bit address helper function

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-st.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-st.c b/drivers/i2c/busses/i2c-st.c
index 6ee77159ac57a3..b3c182fea165e9 100644
--- a/drivers/i2c/busses/i2c-st.c
+++ b/drivers/i2c/busses/i2c-st.c
@@ -614,8 +614,7 @@ static int st_i2c_xfer_msg(struct st_i2c_dev *i2c_dev, struct i2c_msg *msg,
 	unsigned long timeout;
 	int ret;
 
-	c->addr		= (u8)(msg->addr << 1);
-	c->addr		|= (msg->flags & I2C_M_RD);
+	c->addr		= i2c_8bit_addr_from_msg(msg);
 	c->buf		= msg->buf;
 	c->count	= msg->len;
 	c->xfered	= 0;
-- 
2.7.0

Re: [PATCH 01/18] i2c: guarantee that I2C_M_RD will be 0x0001 forever

[Andy Shevchenko]

On Sun, 2016-04-03 at 20:44 +0200, Wolfram Sang wrote:
> There is code out there in user space and kernel space which relies
> on
> I2C_M_RD being bit 0 to simplify their bit operations. Add a comment
> to
> make sure this will never break. Do proper sorting of the defines
> while
> we are here.

Didn't see a comment as stated though.

> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  include/uapi/linux/i2c.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index b0a7dd61eb353a..adcbef4bff610a 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -68,14 +68,15 @@
>  struct i2c_msg {
>  	__u16 addr;	/* slave address			*
> /
>  	__u16 flags;
> -#define I2C_M_TEN		0x0010	/* this is a ten bit
> chip address */
>  #define I2C_M_RD		0x0001	/* read data, from
> slave to master */
> -#define I2C_M_STOP		0x8000	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> -#define I2C_M_NOSTART		0x4000	/* if
> I2C_FUNC_NOSTART */
> -#define I2C_M_REV_DIR_ADDR	0x2000	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> -#define I2C_M_IGNORE_NAK	0x1000	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> -#define I2C_M_NO_RD_ACK		0x0800	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> +					/* I2C_M_RD is guaranteed to
> be 0x0001! */
> +#define I2C_M_TEN		0x0010	/* this is a ten bit
> chip address */
>  #define I2C_M_RECV_LEN		0x0400	/* length will
> be first received byte */
> +#define I2C_M_NO_RD_ACK		0x0800	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> +#define I2C_M_IGNORE_NAK	0x1000	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> +#define I2C_M_REV_DIR_ADDR	0x2000	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
> +#define I2C_M_NOSTART		0x4000	/* if
> I2C_FUNC_NOSTART */
> +#define I2C_M_STOP		0x8000	/* if
> I2C_FUNC_PROTOCOL_MANGLING */
>  	__u16 len;		/* msg length			
> 	*/
>  	__u8 *buf;		/* pointer to msg data		
> 	*/
>  };

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

Re: [PATCH 03/18] i2c: core: use new 8 bit address helper function

[Andy Shevchenko]

On Sun, 2016-04-03 at 20:44 +0200, Wolfram Sang wrote:

Even simple short commit message? Same to the rest.

> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/i2c-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 0f2f8484e8ec1f..778d316de9f8ca 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -2652,7 +2652,7 @@ static u8 i2c_smbus_pec(u8 crc, u8 *p, size_t
> count)
>  static u8 i2c_smbus_msg_pec(u8 pec, struct i2c_msg *msg)
>  {
>  	/* The address will be sent first */
> -	u8 addr = (msg->addr << 1) | !!(msg->flags & I2C_M_RD);
> +	u8 addr = i2c_8bit_addr_from_msg(msg);
>  	pec = i2c_smbus_pec(pec, &addr, 1);
>  
>  	/* The data buffer follows */

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

Re: [PATCH 01/18] i2c: guarantee that I2C_M_RD will be 0x0001 forever

[Shevchenko, Andriy]

On Mon, 2016-04-04 at 13:26 +0300, Andy Shevchenko wrote:
> On Sun, 2016-04-03 at 20:44 +0200, Wolfram Sang wrote:
> > 
> > There is code out there in user space and kernel space which relies
> > on
> > I2C_M_RD being bit 0 to simplify their bit operations. Add a
> > comment
> > to
> > make sure this will never break. Do proper sorting of the defines
> > while
> > we are here.
> Didn't see a comment as stated though.

Ah, overlooked it. Anyway, perhaps worth to mention under
Documentation. What do you think?

> 
> > 
> > 
> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> > ---
> >  include/uapi/linux/i2c.h | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > index b0a7dd61eb353a..adcbef4bff610a 100644
> > --- a/include/uapi/linux/i2c.h
> > +++ b/include/uapi/linux/i2c.h
> > @@ -68,14 +68,15 @@
> >  struct i2c_msg {
> >  	__u16 addr;	/* slave address			
> > *
> > /
> >  	__u16 flags;
> > -#define I2C_M_TEN		0x0010	/* this is a ten
> > bit
> > chip address */
> >  #define I2C_M_RD		0x0001	/* read data, from
> > slave to master */
> > -#define I2C_M_STOP		0x8000	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > -#define I2C_M_NOSTART		0x4000	/* if
> > I2C_FUNC_NOSTART */
> > -#define I2C_M_REV_DIR_ADDR	0x2000	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > -#define I2C_M_IGNORE_NAK	0x1000	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > -#define I2C_M_NO_RD_ACK		0x0800	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > +					/* I2C_M_RD is guaranteed
> > to
> > be 0x0001! */
> > +#define I2C_M_TEN		0x0010	/* this is a ten
> > bit
> > chip address */
> >  #define I2C_M_RECV_LEN		0x0400	/* length will
> > be first received byte */
> > +#define I2C_M_NO_RD_ACK		0x0800	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_IGNORE_NAK	0x1000	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_REV_DIR_ADDR	0x2000	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_NOSTART		0x4000	/* if
> > I2C_FUNC_NOSTART */
> > +#define I2C_M_STOP		0x8000	/* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> >  	__u16 len;		/* msg length			
> > 	*/
> >  	__u8 *buf;		/* pointer to msg data		
> > 	*/
> >  };

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 00/18] i2c: make creation of 8 bit addresses consistent

[Andy Shevchenko]

On Sun, 2016-04-03 at 20:44 +0200, Wolfram Sang wrote:
> This series addresses two things which have come up in the past:
> 
> * document that I2C_M_RD is bit 0. It is not good coding style but
> some
>   drivers (and userspace) silently assume so anyhow. To avoid
> regressions,
>   let's make that explicit.
> 
> * use one function to generate an 8 bit address (7 bit address + RW
> bit)
>   from a struct msg. People did this in various ways, sometimes
> overly
>   brave and sometimes overly anxious with the operator precedence.
> That
>   was partly not nice to read and partly triggering code checkers.
> 
> The new function was tested using a Renesas Lager board with the i2c-
> sh_mobile
> driver. Buildbot was happy with the series, too. A branch can be
> found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> i2c/8bit_macro
> 
> I'd really appreciate more tests on real HW. Also happy for other
> comments,
> reviews...

FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

P.S. Couple of minor question, though I'm okay with either option.

> 
> Thanks,
> 
>    Wolfram
> 
> 
> Wolfram Sang (18):
>   i2c: guarantee that I2C_M_RD will be 0x0001 forever
>   i2c: introduce helper function to get 8 bit address from a message
>   i2c: core: use new 8 bit address helper function
>   i2c: i2c-bcm-iproc: use new 8 bit address helper function
>   i2c: i2c-bcm-kona: use new 8 bit address helper function
>   i2c: i2c-brcmstb: use new 8 bit address helper function
>   i2c: i2c-cpm: use new 8 bit address helper function
>   i2c: i2c-ibm_iic: use new 8 bit address helper function
>   i2c: i2c-img-scb: use new 8 bit address helper function
>   i2c: i2c-iop3xx: use new 8 bit address helper function
>   i2c: i2c-lpc2k: use new 8 bit address helper function
>   i2c: i2c-mt65xx: use new 8 bit address helper function
>   i2c: i2c-ocores: use new 8 bit address helper function
>   i2c: i2c-powermac: use new 8 bit address helper function
>   i2c: i2c-qup: use new 8 bit address helper function
>   i2c: i2c-sh_mobile: use new 8 bit address helper function
>   i2c: i2c-sirf: use new 8 bit address helper function
>   i2c: i2c-st: use new 8 bit address helper function
> 
>  drivers/i2c/busses/i2c-bcm-iproc.c |  2 +-
>  drivers/i2c/busses/i2c-bcm-kona.c  |  5 +----
>  drivers/i2c/busses/i2c-brcmstb.c   |  4 +---
>  drivers/i2c/busses/i2c-cpm.c       |  4 +---
>  drivers/i2c/busses/i2c-ibm_iic.c   |  2 +-
>  drivers/i2c/busses/i2c-img-scb.c   |  4 +---
>  drivers/i2c/busses/i2c-iop3xx.c    |  5 +----
>  drivers/i2c/busses/i2c-lpc2k.c     |  4 +---
>  drivers/i2c/busses/i2c-mt65xx.c    |  5 +----
>  drivers/i2c/busses/i2c-ocores.c    |  5 +----
>  drivers/i2c/busses/i2c-powermac.c  |  4 +---
>  drivers/i2c/busses/i2c-qup.c       |  2 +-
>  drivers/i2c/busses/i2c-sh_mobile.c |  3 +--
>  drivers/i2c/busses/i2c-sirf.c      |  4 +---
>  drivers/i2c/busses/i2c-st.c        |  3 +--
>  drivers/i2c/i2c-core.c             |  2 +-
>  include/linux/i2c.h                |  5 +++++
>  include/uapi/linux/i2c.h           | 13 +++++++------
>  18 files changed, 28 insertions(+), 48 deletions(-)
> 

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

Re: [PATCH 01/18] i2c: guarantee that I2C_M_RD will be 0x0001 forever

[Wolfram Sang]

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

> Didn't see a comment as stated though.

Yeah, it is hidden in the refactoring :/

 > +					/* I2C_M_RD is guaranteed to  be 0x0001! */

   Wolfram


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

Re: [PATCH 01/18] i2c: guarantee that I2C_M_RD will be 0x0001 forever

[Wolfram Sang]

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


> Ah, overlooked it. Anyway, perhaps worth to mention under
> Documentation. What do you think?

Unsure. We can add it later, I'd say.


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

Re: [PATCH 00/18] i2c: make creation of 8 bit addresses consistent

[Wolfram Sang]

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

On Sun, Apr 03, 2016 at 08:44:43PM +0200, Wolfram Sang wrote:
> This series addresses two things which have come up in the past:
> 
> * document that I2C_M_RD is bit 0. It is not good coding style but some
>   drivers (and userspace) silently assume so anyhow. To avoid regressions,
>   let's make that explicit.
> 
> * use one function to generate an 8 bit address (7 bit address + RW bit)
>   from a struct msg. People did this in various ways, sometimes overly
>   brave and sometimes overly anxious with the operator precedence. That
>   was partly not nice to read and partly triggering code checkers.
> 
> The new function was tested using a Renesas Lager board with the i2c-sh_mobile
> driver. Buildbot was happy with the series, too. A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/8bit_macro
> 
> I'd really appreciate more tests on real HW. Also happy for other comments,
> reviews...

Applied to for-next, thanks!


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

Re: [15/18] i2c: i2c-qup: use new 8 bit address helper function

[Naveen Kaje]

On 4/3/2016 12:44 PM, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>   drivers/i2c/busses/i2c-qup.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index 23eaabb19f961a..cc6439ab3f7141 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -515,7 +515,7 @@ static int qup_i2c_get_data_len(struct qup_i2c_dev *qup)
>   static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
>   			    struct i2c_msg *msg,  int is_dma)
>   {
> -	u16 addr = (msg->addr << 1) | ((msg->flags & I2C_M_RD) == I2C_M_RD);
> +	u16 addr = i2c_8bit_addr_from_msg(msg);
>   	int len = 0;
>   	int data_len;
Looks good to me. Tested ok on QDF2432.
There is another place in issue_read that we can use this helper 
function. I will send a follow up patch.

Reviewed by: Naveen Kaje <nkaje@codeaurora.org>
Tested by: Naveen Kaje <nkaje@codeaurora.org>
>