[PATCH v3 0/3] Use void pointers instead of char in I2C transfer APIs

[PATCH v3 0/3] Use void pointers instead of char in I2C transfer APIs

[Dmitry Torokhov]

While we indeed often deal with a stream of bytes when executing a
transfer, at the higher layers we usually work with more structured
data, and there is not really a reason to require casts to u8 * form the
callers. These series change I2C APIs to accept [const] void pointers,
and also adjust SMBUS implementation to use get/put_unaligned_16() and
memcpy() for moving data around.

Changes in v3:
- addressed Luca's comments
- added Jonathan's Acked-by
- split put_unaligned_le16 into a separate patch
- more call sites converted to get/put_unaligned_le16
- new patch using memcpy() for moving data around

Changes in v2:
- adjusted max1363 to the new i2c_master_send/recv signatures

Dmitry Torokhov (3):
  i2c: use void pointers for supplying data for reads and writes
  i2c: smbus: use get/put_unaligned_le16 when working with word data
  i2c: smbus: switch from loops to memcpy

 drivers/i2c/i2c-core-base.c  |  2 +-
 drivers/i2c/i2c-core-smbus.c | 40 +++++++++++++++---------------------
 drivers/iio/adc/max1363.c    | 14 +++++++------
 include/linux/i2c.h          | 28 +++++++++++++------------
 4 files changed, 41 insertions(+), 43 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

[Dmitry Torokhov]

There is no need to force users of i2c_master_send()/i2c_master_recv()
and other i2c read/write bulk data API to cast everything into u8 pointers.
While everything can be considered byte stream, the drivers are usually
work with more structured data.

Let's switch the APIs to accept [const] void pointers to cut amount of
casting needed.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---

Changes in v3:
- addressed Luca's comments
- added Jonathan's Acked-by

Changes in v2:
- adjusted max1363 to the new i2c_master_send/recv signatures

 drivers/i2c/i2c-core-base.c  |  2 +-
 drivers/i2c/i2c-core-smbus.c | 17 +++++++++--------
 drivers/iio/adc/max1363.c    | 14 ++++++++------
 include/linux/i2c.h          | 28 +++++++++++++++-------------
 4 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 6a5183cffdfc3..aeb4201ef55e4 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2065,7 +2065,7 @@ EXPORT_SYMBOL(i2c_transfer);
  *
  * Returns negative errno, or else the number of bytes transferred.
  */
-int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
+int i2c_transfer_buffer_flags(const struct i2c_client *client, void *buf,
 			      int count, u16 flags)
 {
 	int ret;
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 3ac426a8ab5ab..f8708409b4dbc 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -213,7 +213,7 @@ EXPORT_SYMBOL(i2c_smbus_write_word_data);
  * mechanism (I2C_M_RECV_LEN) which may not be implemented.
  */
 s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
-			      u8 *values)
+			      void *values)
 {
 	union i2c_smbus_data data;
 	int status;
@@ -240,7 +240,7 @@ EXPORT_SYMBOL(i2c_smbus_read_block_data);
  * else zero on success.
  */
 s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
-			       u8 length, const u8 *values)
+			       u8 length, const void *values)
 {
 	union i2c_smbus_data data;
 
@@ -256,7 +256,7 @@ EXPORT_SYMBOL(i2c_smbus_write_block_data);
 
 /* Returns the number of read bytes */
 s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
-				  u8 length, u8 *values)
+				  u8 length, void *values)
 {
 	union i2c_smbus_data data;
 	int status;
@@ -276,7 +276,7 @@ s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
 EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data);
 
 s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command,
-				   u8 length, const u8 *values)
+				   u8 length, const void *values)
 {
 	union i2c_smbus_data data;
 
@@ -628,8 +628,9 @@ EXPORT_SYMBOL(__i2c_smbus_xfer);
  * transfer.
  */
 s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
-					      u8 command, u8 length, u8 *values)
+					      u8 command, u8 length, void *values)
 {
+	u8 *bytes = values;
 	u8 i = 0;
 	int status;
 
@@ -647,8 +648,8 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 			status = i2c_smbus_read_word_data(client, command + i);
 			if (status < 0)
 				return status;
-			values[i] = status & 0xff;
-			values[i + 1] = status >> 8;
+			bytes[i] = status & 0xff;
+			bytes[i + 1] = status >> 8;
 			i += 2;
 		}
 	}
@@ -657,7 +658,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 		status = i2c_smbus_read_byte_data(client, command + i);
 		if (status < 0)
 			return status;
-		values[i] = status;
+		bytes[i] = status;
 		i++;
 	}
 
diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 5c2cc61b666e7..48ed76a0e83d4 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -182,9 +182,9 @@ struct max1363_state {
 	struct regulator		*vref;
 	u32				vref_uv;
 	int				(*send)(const struct i2c_client *client,
-						const char *buf, int count);
+						const void *buf, int count);
 	int				(*recv)(const struct i2c_client *client,
-						char *buf, int count);
+						void *buf, int count);
 };
 
 #define MAX1363_MODE_SINGLE(_num, _mask) {				\
@@ -310,27 +310,29 @@ static const struct max1363_mode
 	return NULL;
 }
 
-static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
+static int max1363_smbus_send(const struct i2c_client *client, const void *buf,
 		int count)
 {
+	const u8 *data = buf;
 	int i, err;
 
 	for (i = err = 0; err == 0 && i < count; ++i)
-		err = i2c_smbus_write_byte(client, buf[i]);
+		err = i2c_smbus_write_byte(client, data[i]);
 
 	return err ? err : count;
 }
 
-static int max1363_smbus_recv(const struct i2c_client *client, char *buf,
+static int max1363_smbus_recv(const struct i2c_client *client, void *buf,
 		int count)
 {
+	u8 *data = buf;
 	int i, ret;
 
 	for (i = 0; i < count; ++i) {
 		ret = i2c_smbus_read_byte(client);
 		if (ret < 0)
 			return ret;
-		buf[i] = ret;
+		data[i] = ret;
 	}
 
 	return count;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index aaf57d9b41dbb..64cf92e191aa8 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -51,7 +51,7 @@ struct property_entry;
  * @count must be be less than 64k since msg.len is u16.
  */
 extern int i2c_transfer_buffer_flags(const struct i2c_client *client,
-				     char *buf, int count, u16 flags);
+				     void *buf, int count, u16 flags);
 
 /**
  * i2c_master_recv - issue a single I2C message in master receive mode
@@ -62,7 +62,7 @@ extern int i2c_transfer_buffer_flags(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes read.
  */
 static inline int i2c_master_recv(const struct i2c_client *client,
-				  char *buf, int count)
+				  void *buf, int count)
 {
 	return i2c_transfer_buffer_flags(client, buf, count, I2C_M_RD);
 };
@@ -77,7 +77,7 @@ static inline int i2c_master_recv(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes read.
  */
 static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
-					  char *buf, int count)
+					  void *buf, int count)
 {
 	return i2c_transfer_buffer_flags(client, buf, count,
 					 I2C_M_RD | I2C_M_DMA_SAFE);
@@ -92,9 +92,10 @@ static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes written.
  */
 static inline int i2c_master_send(const struct i2c_client *client,
-				  const char *buf, int count)
+				  const void *buf, int count)
 {
-	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
+	return i2c_transfer_buffer_flags(client, (void *)buf /* const cast */,
+					 count, 0);
 };
 
 /**
@@ -107,10 +108,10 @@ static inline int i2c_master_send(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes written.
  */
 static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
-					  const char *buf, int count)
+					  const void *buf, int count)
 {
-	return i2c_transfer_buffer_flags(client, (char *)buf, count,
-					 I2C_M_DMA_SAFE);
+	return i2c_transfer_buffer_flags(client, (void *)buf /* const cast */,
+					 count, I2C_M_DMA_SAFE);
 };
 
 /* Transfer num messages.
@@ -166,18 +167,19 @@ i2c_smbus_write_word_swapped(const struct i2c_client *client,
 
 /* Returns the number of read bytes */
 extern s32 i2c_smbus_read_block_data(const struct i2c_client *client,
-				     u8 command, u8 *values);
+				     u8 command, void *values);
 extern s32 i2c_smbus_write_block_data(const struct i2c_client *client,
-				      u8 command, u8 length, const u8 *values);
+				      u8 command, u8 length,
+				      const void *values);
 /* Returns the number of read bytes */
 extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client,
-					 u8 command, u8 length, u8 *values);
+					 u8 command, u8 length, void *values);
 extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
 					  u8 command, u8 length,
-					  const u8 *values);
+					  const void *values);
 extern s32
 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
-					  u8 command, u8 length, u8 *values);
+					  u8 command, u8 length, void *values);
 int i2c_get_device_id(const struct i2c_client *client,
 		      struct i2c_device_identity *id);
 #endif /* I2C */
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v3 2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data

[Dmitry Torokhov]

It is potentially more performant, and also shows intent more clearly,
to use get_unaligned_le16() and put_unaligned_le16() when working with
word data.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---

Changes in v3:
- split put_unaligned_le16 into a separate patch
- more call sites converted to get/put_unaligned_le16

 drivers/i2c/i2c-core-smbus.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index f8708409b4dbc..7b4e2270eeda1 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -15,6 +15,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-smbus.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 
 #include "i2c-core.h"
 
@@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			msg[1].len = 2;
 		else {
 			msg[0].len = 3;
-			msgbuf0[1] = data->word & 0xff;
-			msgbuf0[2] = data->word >> 8;
+			put_unaligned_le16(data->word, msgbuf0 + 1);
 		}
 		break;
 	case I2C_SMBUS_PROC_CALL:
@@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 		read_write = I2C_SMBUS_READ;
 		msg[0].len = 3;
 		msg[1].len = 2;
-		msgbuf0[1] = data->word & 0xff;
-		msgbuf0[2] = data->word >> 8;
+		put_unaligned_le16(data->word, msgbuf0 + 1);
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
 		if (read_write == I2C_SMBUS_READ) {
@@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			break;
 		case I2C_SMBUS_WORD_DATA:
 		case I2C_SMBUS_PROC_CALL:
-			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
+			data->word = get_unaligned_le16(msgbuf1);
 			break;
 		case I2C_SMBUS_I2C_BLOCK_DATA:
 			for (i = 0; i < data->block[0]; i++)
@@ -648,8 +647,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 			status = i2c_smbus_read_word_data(client, command + i);
 			if (status < 0)
 				return status;
-			bytes[i] = status & 0xff;
-			bytes[i + 1] = status >> 8;
+			put_unaligned_le16(status, values + i);
 			i += 2;
 		}
 	}
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v3 3/3] i2c: smbus: switch from loops to memcpy

[Dmitry Torokhov]

When copying memory from one buffer to another, instead of open-coding
loops with byte-by-byte copies let's use memcpy() which might be a bit
faster and makes intent more clear.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---

Changes in v3:
- new patch using memcpy() for moving data around

 drivers/i2c/i2c-core-smbus.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 7b4e2270eeda1..bbafdd3b1b114 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -397,8 +397,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			}
 
 			i2c_smbus_try_get_dmabuf(&msg[0], command);
-			for (i = 1; i < msg[0].len; i++)
-				msg[0].buf[i] = data->block[i - 1];
+			memcpy(msg[0].buf + 1, data->block, msg[0].len - 1);
 		}
 		break;
 	case I2C_SMBUS_BLOCK_PROC_CALL:
@@ -413,8 +412,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 
 		msg[0].len = data->block[0] + 2;
 		i2c_smbus_try_get_dmabuf(&msg[0], command);
-		for (i = 1; i < msg[0].len; i++)
-			msg[0].buf[i] = data->block[i - 1];
+		memcpy(msg[0].buf + 1, data->block, msg[0].len - 1);
 
 		msg[1].flags |= I2C_M_RECV_LEN;
 		msg[1].len = 1; /* block length will be added by
@@ -436,8 +434,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			msg[0].len = data->block[0] + 1;
 
 			i2c_smbus_try_get_dmabuf(&msg[0], command);
-			for (i = 1; i <= data->block[0]; i++)
-				msg[0].buf[i] = data->block[i];
+			memcpy(msg[0].buf + 1, data->block + 1, data->block[0]);
 		}
 		break;
 	default:
@@ -489,13 +486,11 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			data->word = get_unaligned_le16(msgbuf1);
 			break;
 		case I2C_SMBUS_I2C_BLOCK_DATA:
-			for (i = 0; i < data->block[0]; i++)
-				data->block[i + 1] = msg[1].buf[i];
+			memcpy(data->block + 1, msg[1].buf, data->block[0]);
 			break;
 		case I2C_SMBUS_BLOCK_DATA:
 		case I2C_SMBUS_BLOCK_PROC_CALL:
-			for (i = 0; i < msg[1].buf[0] + 1; i++)
-				data->block[i] = msg[1].buf[i];
+			memcpy(data->block, msg[1].buf, msg[1].buf[0] + 1);
 			break;
 		}
 
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

[Luca Ceresoli]

Hi Dmitry,

thanks for having taken into account my comments.

On 12/11/19 21:31, Dmitry Torokhov wrote:
> There is no need to force users of i2c_master_send()/i2c_master_recv()
> and other i2c read/write bulk data API to cast everything into u8 pointers.
> While everything can be considered byte stream, the drivers are usually
> work with more structured data.
> 
> Let's switch the APIs to accept [const] void pointers to cut amount of
> casting needed.
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v3 2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data

[Luca Ceresoli]

Hi Dmitry,

On 12/11/19 21:31, Dmitry Torokhov wrote:
> It is potentially more performant, and also shows intent more clearly,
> to use get_unaligned_le16() and put_unaligned_le16() when working with
> word data.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> ---
> 
> Changes in v3:
> - split put_unaligned_le16 into a separate patch
> - more call sites converted to get/put_unaligned_le16
> 
>  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index f8708409b4dbc..7b4e2270eeda1 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -15,6 +15,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-smbus.h>
>  #include <linux/slab.h>
> +#include <asm/unaligned.h>
>  
>  #include "i2c-core.h"
>  
> @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			msg[1].len = 2;
>  		else {
>  			msg[0].len = 3;
> -			msgbuf0[1] = data->word & 0xff;
> -			msgbuf0[2] = data->word >> 8;
> +			put_unaligned_le16(data->word, msgbuf0 + 1);
>  		}
>  		break;
>  	case I2C_SMBUS_PROC_CALL:
> @@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  		read_write = I2C_SMBUS_READ;
>  		msg[0].len = 3;
>  		msg[1].len = 2;
> -		msgbuf0[1] = data->word & 0xff;
> -		msgbuf0[2] = data->word >> 8;
> +		put_unaligned_le16(data->word, msgbuf0 + 1);
>  		break;
>  	case I2C_SMBUS_BLOCK_DATA:
>  		if (read_write == I2C_SMBUS_READ) {
> @@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			break;
>  		case I2C_SMBUS_WORD_DATA:
>  		case I2C_SMBUS_PROC_CALL:
> -			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
> +			data->word = get_unaligned_le16(msgbuf1);

Well, msgbuf1 cannot be unaligned, so it looks like you just need to
convert little endian to host endian. Perhaps __le16_to_cpu(msgbuf1) is
better (and equally or more efficient).

> @@ -648,8 +647,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
>  			status = i2c_smbus_read_word_data(client, command + i);
>  			if (status < 0)
>  				return status;
> -			bytes[i] = status & 0xff;
> -			bytes[i + 1] = status >> 8;
> +			put_unaligned_le16(status, values + i);
>  			i += 2;
>  		}

I've been pondering on this one, because 'i' is always an even number.
But, depending on the caller, 'values' could be unaligned, thus
put_unaligned_le16() is OK here.

-- 
Luca

Re: [PATCH v3 3/3] i2c: smbus: switch from loops to memcpy

[Luca Ceresoli]

Hi Dmitry,

On 12/11/19 21:31, Dmitry Torokhov wrote:
> When copying memory from one buffer to another, instead of open-coding
> loops with byte-by-byte copies let's use memcpy() which might be a bit
> faster and makes intent more clear.

Good idea!

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v3 2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data

[Dmitry Torokhov]

Hi Luca,

On Wed, Nov 13, 2019 at 10:47:42AM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
> 
> On 12/11/19 21:31, Dmitry Torokhov wrote:
> > It is potentially more performant, and also shows intent more clearly,
> > to use get_unaligned_le16() and put_unaligned_le16() when working with
> > word data.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - split put_unaligned_le16 into a separate patch
> > - more call sites converted to get/put_unaligned_le16
> > 
> >  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> > index f8708409b4dbc..7b4e2270eeda1 100644
> > --- a/drivers/i2c/i2c-core-smbus.c
> > +++ b/drivers/i2c/i2c-core-smbus.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/i2c-smbus.h>
> >  #include <linux/slab.h>
> > +#include <asm/unaligned.h>
> >  
> >  #include "i2c-core.h"
> >  
> > @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  			msg[1].len = 2;
> >  		else {
> >  			msg[0].len = 3;
> > -			msgbuf0[1] = data->word & 0xff;
> > -			msgbuf0[2] = data->word >> 8;
> > +			put_unaligned_le16(data->word, msgbuf0 + 1);
> >  		}
> >  		break;
> >  	case I2C_SMBUS_PROC_CALL:
> > @@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  		read_write = I2C_SMBUS_READ;
> >  		msg[0].len = 3;
> >  		msg[1].len = 2;
> > -		msgbuf0[1] = data->word & 0xff;
> > -		msgbuf0[2] = data->word >> 8;
> > +		put_unaligned_le16(data->word, msgbuf0 + 1);
> >  		break;
> >  	case I2C_SMBUS_BLOCK_DATA:
> >  		if (read_write == I2C_SMBUS_READ) {
> > @@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  			break;
> >  		case I2C_SMBUS_WORD_DATA:
> >  		case I2C_SMBUS_PROC_CALL:
> > -			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
> > +			data->word = get_unaligned_le16(msgbuf1);
> 
> Well, msgbuf1 cannot be unaligned, so it looks like you just need to
> convert little endian to host endian. Perhaps __le16_to_cpu(msgbuf1) is
> better (and equally or more efficient).

Well, msgbuf1 (as is any other variable unless adjusted by special
alignment attribute) is naturally aligned for its own data type (which
for u8 means it can start at any address), but that does not mean that
is is aligned for the purpose of storing word data. In fact, the
preceding msgbuf0 variable is 32 + 3 = 35 bytes long, which means that
msgbuf1 is starting on an odd address, and is definitely not aligned for
word access and using __le16_to_cpu to fetch that word will trap on some
architectures.

Thanks.

-- 
Dmitry

Re: [PATCH v3 2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data

[Luca Ceresoli]

Hi Dmitry,

On 13/11/19 20:39, Dmitry Torokhov wrote:
> Hi Luca,
> 
> On Wed, Nov 13, 2019 at 10:47:42AM +0100, Luca Ceresoli wrote:
>> Hi Dmitry,
>>
>> On 12/11/19 21:31, Dmitry Torokhov wrote:
>>> It is potentially more performant, and also shows intent more clearly,
>>> to use get_unaligned_le16() and put_unaligned_le16() when working with
>>> word data.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>
>>> ---
>>>
>>> Changes in v3:
>>> - split put_unaligned_le16 into a separate patch
>>> - more call sites converted to get/put_unaligned_le16
>>>
>>>  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>> index f8708409b4dbc..7b4e2270eeda1 100644
>>> --- a/drivers/i2c/i2c-core-smbus.c
>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>> @@ -15,6 +15,7 @@
>>>  #include <linux/i2c.h>
>>>  #include <linux/i2c-smbus.h>
>>>  #include <linux/slab.h>
>>> +#include <asm/unaligned.h>
>>>  
>>>  #include "i2c-core.h"
>>>  
>>> @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>  			msg[1].len = 2;
>>>  		else {
>>>  			msg[0].len = 3;
>>> -			msgbuf0[1] = data->word & 0xff;
>>> -			msgbuf0[2] = data->word >> 8;
>>> +			put_unaligned_le16(data->word, msgbuf0 + 1);
>>>  		}
>>>  		break;
>>>  	case I2C_SMBUS_PROC_CALL:
>>> @@ -379,8 +379,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>  		read_write = I2C_SMBUS_READ;
>>>  		msg[0].len = 3;
>>>  		msg[1].len = 2;
>>> -		msgbuf0[1] = data->word & 0xff;
>>> -		msgbuf0[2] = data->word >> 8;
>>> +		put_unaligned_le16(data->word, msgbuf0 + 1);
>>>  		break;
>>>  	case I2C_SMBUS_BLOCK_DATA:
>>>  		if (read_write == I2C_SMBUS_READ) {
>>> @@ -487,7 +486,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>>>  			break;
>>>  		case I2C_SMBUS_WORD_DATA:
>>>  		case I2C_SMBUS_PROC_CALL:
>>> -			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
>>> +			data->word = get_unaligned_le16(msgbuf1);
>>
>> Well, msgbuf1 cannot be unaligned, so it looks like you just need to
>> convert little endian to host endian. Perhaps __le16_to_cpu(msgbuf1) is
>> better (and equally or more efficient).
> 
> Well, msgbuf1 (as is any other variable unless adjusted by special
> alignment attribute) is naturally aligned for its own data type (which
> for u8 means it can start at any address), but that does not mean that
> is is aligned for the purpose of storing word data. In fact, the
> preceding msgbuf0 variable is 32 + 3 = 35 bytes long, which means that
> msgbuf1 is starting on an odd address, and is definitely not aligned for
> word access and using __le16_to_cpu to fetch that word will trap on some
> architectures.

Uhm, you're obviously right. And I was probably drunk when I wrote the
opposite! Sorry about that...

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v3 2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data

[Uwe Kleine-König]

Hello Dmitry,

On Tue, Nov 12, 2019 at 12:31:31PM -0800, Dmitry Torokhov wrote:
> It is potentially more performant, and also shows intent more clearly,
> to use get_unaligned_le16() and put_unaligned_le16() when working with
> word data.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> ---
> 
> Changes in v3:
> - split put_unaligned_le16 into a separate patch
> - more call sites converted to get/put_unaligned_le16
> 
>  drivers/i2c/i2c-core-smbus.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index f8708409b4dbc..7b4e2270eeda1 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -15,6 +15,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-smbus.h>
>  #include <linux/slab.h>
> +#include <asm/unaligned.h>
>  
>  #include "i2c-core.h"
>  
> @@ -370,8 +371,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			msg[1].len = 2;
>  		else {
>  			msg[0].len = 3;
> -			msgbuf0[1] = data->word & 0xff;
> -			msgbuf0[2] = data->word >> 8;
> +			put_unaligned_le16(data->word, msgbuf0 + 1);

You claim this was clearer. For me it is not. With the explicit
assignment to msgbuf0[1] and msbbuf0[2] it is immediatly obvious to me
what happens.  Even though the endianness is explicitly mentioned in
put_unaligned_le16, it takes a bit longer for me to understand what it
does and which part of data->word ends up in which byte.

Concerning the "potentially more performant" part: I wonder if this is
backed by numbers and if it is indeed benificial on some platforms if
this is a compiler problem.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

[Uwe Kleine-König]

Hello Dmitry,

On Tue, Nov 12, 2019 at 12:31:30PM -0800, Dmitry Torokhov wrote:
> There is no need to force users of i2c_master_send()/i2c_master_recv()
> and other i2c read/write bulk data API to cast everything into u8 pointers.
> While everything can be considered byte stream, the drivers are usually
> work with more structured data.
> 
> Let's switch the APIs to accept [const] void pointers to cut amount of
> casting needed.
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Can you give an example where you save some casts? Given that i2c is a
byte oriented protocol (as opposed to for example spi) I think it's a
good idea to expose this in the API.

> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 5c2cc61b666e7..48ed76a0e83d4 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c

This change isn't motivated in the commit log. Is this here by mistake?

> @@ -182,9 +182,9 @@ struct max1363_state {
>  	struct regulator		*vref;
>  	u32				vref_uv;
>  	int				(*send)(const struct i2c_client *client,
> -						const char *buf, int count);
> +						const void *buf, int count);
>  	int				(*recv)(const struct i2c_client *client,
> -						char *buf, int count);
> +						void *buf, int count);
>  };
>  
>  #define MAX1363_MODE_SINGLE(_num, _mask) {				\
> @@ -310,27 +310,29 @@ static const struct max1363_mode
>  	return NULL;
>  }
>  
> -static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
> +static int max1363_smbus_send(const struct i2c_client *client, const void *buf,
>  		int count)
>  {
> +	const u8 *data = buf;
>  	int i, err;
>  
>  	for (i = err = 0; err == 0 && i < count; ++i)
> -		err = i2c_smbus_write_byte(client, buf[i]);
> +		err = i2c_smbus_write_byte(client, data[i]);

Isn't this hunk an indicator that keeping char (or u8) as type of the
members of buf is a good idea?
 
>  	return err ? err : count;
>  }
>  
> -static int max1363_smbus_recv(const struct i2c_client *client, char *buf,
> +static int max1363_smbus_recv(const struct i2c_client *client, void *buf,
>  		int count)
>  {
> +	u8 *data = buf;
>  	int i, ret;
>  
>  	for (i = 0; i < count; ++i) {
>  		ret = i2c_smbus_read_byte(client);
>  		if (ret < 0)
>  			return ret;
> -		buf[i] = ret;
> +		data[i] = ret;
>  	}
>  
>  	return count;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 3/3] i2c: smbus: switch from loops to memcpy

[Uwe Kleine-König]

Hello Dmitry,

On Tue, Nov 12, 2019 at 12:31:32PM -0800, Dmitry Torokhov wrote:
> When copying memory from one buffer to another, instead of open-coding
> loops with byte-by-byte copies let's use memcpy() which might be a bit
> faster and makes intent more clear.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> ---
> 
> Changes in v3:
> - new patch using memcpy() for moving data around
> 
>  drivers/i2c/i2c-core-smbus.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 7b4e2270eeda1..bbafdd3b1b114 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -397,8 +397,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			}
>  
>  			i2c_smbus_try_get_dmabuf(&msg[0], command);
> -			for (i = 1; i < msg[0].len; i++)
> -				msg[0].buf[i] = data->block[i - 1];
> +			memcpy(msg[0].buf + 1, data->block, msg[0].len - 1);

Can it happen that msg[0].len is zero?

>  		}
>  		break;
>  	case I2C_SMBUS_BLOCK_PROC_CALL:

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

[Dmitry Torokhov]

Hi Uwe,

On Mon, Nov 18, 2019 at 08:43:49AM +0100, Uwe Kleine-König wrote:
> Hello Dmitry,
> 
> On Tue, Nov 12, 2019 at 12:31:30PM -0800, Dmitry Torokhov wrote:
> > There is no need to force users of i2c_master_send()/i2c_master_recv()
> > and other i2c read/write bulk data API to cast everything into u8 pointers.
> > While everything can be considered byte stream, the drivers are usually
> > work with more structured data.
> > 
> > Let's switch the APIs to accept [const] void pointers to cut amount of
> > casting needed.
> > 
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Can you give an example where you save some casts? Given that i2c is a
> byte oriented protocol (as opposed to for example spi) I think it's a
> good idea to expose this in the API.

I see this at least:

dtor@dtor-ws:~/kernel/work $ git grep "i2c_master.*(u8 \*)"
drivers/crypto/atmel-i2c.c:     ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
drivers/iio/common/ms_sensors/ms_sensors_i2c.c:         ret = i2c_master_recv(client, (u8 *)&buf, 3);
drivers/iio/humidity/hdc100x.c: ret = i2c_master_recv(client, (u8 *)buf, 4);
drivers/iio/pressure/abp060mg.c:        ret = i2c_master_send(client, (u8 *)&buf, state->mreq_len);
drivers/iio/pressure/abp060mg.c:        ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
drivers/input/misc/ad714x-i2c.c:        error = i2c_master_send(client, (u8 *)chip->xfer_buf,
drivers/input/misc/ad714x-i2c.c:        error = i2c_master_send(client, (u8 *)chip->xfer_buf,
drivers/input/misc/ad714x-i2c.c:                error = i2c_master_recv(client, (u8 *)chip->xfer_buf,
drivers/input/touchscreen/sx8654.c:     len = i2c_master_recv(ts->client, (u8 *)data, readlen);
drivers/nfc/nfcmrvl/i2c.c:      ret = i2c_master_recv(drv_data->i2c, (u8 *)&nci_hdr, NCI_CTRL_HDR_SIZE);
drivers/nfc/nxp-nci/i2c.c:      r = i2c_master_recv(client, (u8 *) &header, NXP_NCI_FW_HDR_LEN);
drivers/nfc/nxp-nci/i2c.c:      r = i2c_master_recv(client, (u8 *) &header, NCI_CTRL_HDR_SIZE);
drivers/video/fbdev/ssd1307fb.c:        ret = i2c_master_send(client, (u8 *)array, len);

I am pretty sure there are more that my quick grep did not catch.

And I agree that I2C itself is a byte-oriented protocol, but the data from the
point of the driver (once transfer is done) is often more structured.

> 
> > diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> > index 5c2cc61b666e7..48ed76a0e83d4 100644
> > --- a/drivers/iio/adc/max1363.c
> > +++ b/drivers/iio/adc/max1363.c
> 
> This change isn't motivated in the commit log. Is this here by mistake?

No, it is needed as signature of i2c_master_send/recv has changed.

> 
> > @@ -182,9 +182,9 @@ struct max1363_state {
> >  	struct regulator		*vref;
> >  	u32				vref_uv;
> >  	int				(*send)(const struct i2c_client *client,
> > -						const char *buf, int count);
> > +						const void *buf, int count);
> >  	int				(*recv)(const struct i2c_client *client,
> > -						char *buf, int count);
> > +						void *buf, int count);
> >  };
> >  
> >  #define MAX1363_MODE_SINGLE(_num, _mask) {				\
> > @@ -310,27 +310,29 @@ static const struct max1363_mode
> >  	return NULL;
> >  }
> >  
> > -static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
> > +static int max1363_smbus_send(const struct i2c_client *client, const void *buf,
> >  		int count)
> >  {
> > +	const u8 *data = buf;
> >  	int i, err;
> >  
> >  	for (i = err = 0; err == 0 && i < count; ++i)
> > -		err = i2c_smbus_write_byte(client, buf[i]);
> > +		err = i2c_smbus_write_byte(client, data[i]);
> 
> Isn't this hunk an indicator that keeping char (or u8) as type of the
> members of buf is a good idea?

Not necessarily, if you check the driver sometimes it deals with stream of
bytes, and sometimes it sends structured data, like little-endian words. I
did not make additional changes because I wanted to minimize amount of changes
to this driver in this patch.


>  
> >  	return err ? err : count;
> >  }
> >  
> > -static int max1363_smbus_recv(const struct i2c_client *client, char *buf,
> > +static int max1363_smbus_recv(const struct i2c_client *client, void *buf,
> >  		int count)
> >  {
> > +	u8 *data = buf;
> >  	int i, ret;
> >  
> >  	for (i = 0; i < count; ++i) {
> >  		ret = i2c_smbus_read_byte(client);
> >  		if (ret < 0)
> >  			return ret;
> > -		buf[i] = ret;
> > +		data[i] = ret;
> >  	}
> >  
> >  	return count;
> 

Thanks.

-- 
Dmitry

Re: [PATCH v3 3/3] i2c: smbus: switch from loops to memcpy

[Dmitry Torokhov]

On Mon, Nov 18, 2019 at 08:47:57AM +0100, Uwe Kleine-König wrote:
> Hello Dmitry,
> 
> On Tue, Nov 12, 2019 at 12:31:32PM -0800, Dmitry Torokhov wrote:
> > When copying memory from one buffer to another, instead of open-coding
> > loops with byte-by-byte copies let's use memcpy() which might be a bit
> > faster and makes intent more clear.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - new patch using memcpy() for moving data around
> > 
> >  drivers/i2c/i2c-core-smbus.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> > index 7b4e2270eeda1..bbafdd3b1b114 100644
> > --- a/drivers/i2c/i2c-core-smbus.c
> > +++ b/drivers/i2c/i2c-core-smbus.c
> > @@ -397,8 +397,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> >  			}
> >  
> >  			i2c_smbus_try_get_dmabuf(&msg[0], command);
> > -			for (i = 1; i < msg[0].len; i++)
> > -				msg[0].buf[i] = data->block[i - 1];
> > +			memcpy(msg[0].buf + 1, data->block, msg[0].len - 1);
> 
> Can it happen that msg[0].len is zero?

No, it can not, because of the "msg[0].len = data->block[0] + 2;" line
above.

Thanks.

-- 
Dmitry

Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

[Uwe Kleine-König]

[Fixing the address of the linux-iio list]

Hello Dmitry,

On Mon, Nov 18, 2019 at 12:04:46AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 18, 2019 at 08:43:49AM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 12, 2019 at 12:31:30PM -0800, Dmitry Torokhov wrote:
> > > There is no need to force users of i2c_master_send()/i2c_master_recv()
> > > and other i2c read/write bulk data API to cast everything into u8 pointers.
> > > While everything can be considered byte stream, the drivers are usually
> > > work with more structured data.
> > > 
> > > Let's switch the APIs to accept [const] void pointers to cut amount of
> > > casting needed.
> > > 
> > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > Can you give an example where you save some casts? Given that i2c is a
> > byte oriented protocol (as opposed to for example spi) I think it's a
> > good idea to expose this in the API.
> 
> I see this at least:
> 
> dtor@dtor-ws:~/kernel/work $ git grep "i2c_master.*(u8 \*)"
> drivers/crypto/atmel-i2c.c:     ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
> drivers/iio/common/ms_sensors/ms_sensors_i2c.c:         ret = i2c_master_recv(client, (u8 *)&buf, 3);
> drivers/iio/humidity/hdc100x.c: ret = i2c_master_recv(client, (u8 *)buf, 4);

I think this one has an endianness bug.

> drivers/iio/pressure/abp060mg.c:        ret = i2c_master_send(client, (u8 *)&buf, state->mreq_len);

From a quick look: mreq_len might be 1 in some cases and buf is declared
as __be16[2]. Hmm.

> drivers/iio/pressure/abp060mg.c:        ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
> drivers/input/misc/ad714x-i2c.c:        error = i2c_master_send(client, (u8 *)chip->xfer_buf,
> drivers/input/misc/ad714x-i2c.c:        error = i2c_master_send(client, (u8 *)chip->xfer_buf,
> drivers/input/misc/ad714x-i2c.c:                error = i2c_master_recv(client, (u8 *)chip->xfer_buf,
> drivers/input/touchscreen/sx8654.c:     len = i2c_master_recv(ts->client, (u8 *)data, readlen);
> drivers/nfc/nfcmrvl/i2c.c:      ret = i2c_master_recv(drv_data->i2c, (u8 *)&nci_hdr, NCI_CTRL_HDR_SIZE);
> drivers/nfc/nxp-nci/i2c.c:      r = i2c_master_recv(client, (u8 *) &header, NXP_NCI_FW_HDR_LEN);
> drivers/nfc/nxp-nci/i2c.c:      r = i2c_master_recv(client, (u8 *) &header, NCI_CTRL_HDR_SIZE);
> drivers/video/fbdev/ssd1307fb.c:        ret = i2c_master_send(client, (u8 *)array, len);
> 
> I am pretty sure there are more that my quick grep did not catch.
> 
> And I agree that I2C itself is a byte-oriented protocol, but the data from the
> point of the driver (once transfer is done) is often more structured.

I think it is fine to require from a caller that they are aware that a
byte (or byte array) must be passed to i2c functions. Given the (maybe)
two problems I pointed out above making it a bit harder to pass non-byte
data to these functions doesn't sound like a bad idea to me.

Obviously your mileage varies, but I personally like having an explicit
type in the API. I guess we have to agree to not agree and let Wolfram
decide if he likes your change or not.

> > > diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> > > index 5c2cc61b666e7..48ed76a0e83d4 100644
> > > --- a/drivers/iio/adc/max1363.c
> > > +++ b/drivers/iio/adc/max1363.c
> > 
> > This change isn't motivated in the commit log. Is this here by mistake?
> 
> No, it is needed as signature of i2c_master_send/recv has changed.

Ah, I see, there is

	st->send = i2c_master_send;

in the code. I think this is worth mentioning in the commit log that it
changes to this file don't look like a mistake as I wondered.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 3/3] i2c: smbus: switch from loops to memcpy

[Uwe Kleine-König]

On Mon, Nov 18, 2019 at 12:09:39AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 18, 2019 at 08:47:57AM +0100, Uwe Kleine-König wrote:
> > Hello Dmitry,
> > 
> > On Tue, Nov 12, 2019 at 12:31:32PM -0800, Dmitry Torokhov wrote:
> > > When copying memory from one buffer to another, instead of open-coding
> > > loops with byte-by-byte copies let's use memcpy() which might be a bit
> > > faster and makes intent more clear.
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > 
> > > ---
> > > 
> > > Changes in v3:
> > > - new patch using memcpy() for moving data around
> > > 
> > >  drivers/i2c/i2c-core-smbus.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> > > index 7b4e2270eeda1..bbafdd3b1b114 100644
> > > --- a/drivers/i2c/i2c-core-smbus.c
> > > +++ b/drivers/i2c/i2c-core-smbus.c
> > > @@ -397,8 +397,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> > >  			}
> > >  
> > >  			i2c_smbus_try_get_dmabuf(&msg[0], command);
> > > -			for (i = 1; i < msg[0].len; i++)
> > > -				msg[0].buf[i] = data->block[i - 1];
> > > +			memcpy(msg[0].buf + 1, data->block, msg[0].len - 1);
> > 
> > Can it happen that msg[0].len is zero?
> 
> No, it can not, because of the "msg[0].len = data->block[0] + 2;" line
> above.

OK, and as passing data with data->block[0] = 254 also makes the code do
strange things already without your patch. I now also checked the other
conversions for similar problems and didn't find any. So:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 0/3] Use void pointers instead of char in I2C transfer APIs

[Wolfram Sang]

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

On Tue, Nov 12, 2019 at 12:31:29PM -0800, Dmitry Torokhov wrote:
> While we indeed often deal with a stream of bytes when executing a
> transfer, at the higher layers we usually work with more structured
> data, and there is not really a reason to require casts to u8 * form the
> callers. These series change I2C APIs to accept [const] void pointers,
> and also adjust SMBUS implementation to use get/put_unaligned_16() and
> memcpy() for moving data around.

I just started to work on something SMBus, so I rediscovered this series
and will comment now.


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

Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

[Wolfram Sang]

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


> I think it is fine to require from a caller that they are aware that a
> byte (or byte array) must be passed to i2c functions. Given the (maybe)
> two problems I pointed out above making it a bit harder to pass non-byte
> data to these functions doesn't sound like a bad idea to me.
> 
> Obviously your mileage varies, but I personally like having an explicit
> type in the API. I guess we have to agree to not agree and let Wolfram
> decide if he likes your change or not.

I am on Uwe's side here. I like it being explicit and think the casts as
they are now are the smaller problem.


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

Re: [PATCH v3 2/3] i2c: smbus: use get/put_unaligned_le16 when working with word data

[Wolfram Sang]

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


> You claim this was clearer. For me it is not. With the explicit
> assignment to msgbuf0[1] and msbbuf0[2] it is immediatly obvious to me
> what happens.  Even though the endianness is explicitly mentioned in
> put_unaligned_le16, it takes a bit longer for me to understand what it
> does and which part of data->word ends up in which byte.

Seems like I am on Uwe's side again. For me, the current way is
also more readable.


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

Re: [PATCH v3 3/3] i2c: smbus: switch from loops to memcpy

[Wolfram Sang]

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

On Tue, Nov 12, 2019 at 12:31:32PM -0800, Dmitry Torokhov wrote:
> When copying memory from one buffer to another, instead of open-coding
> loops with byte-by-byte copies let's use memcpy() which might be a bit
> faster and makes intent more clear.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 

This one I like very much! Applied to for-next, thanks!


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