input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

This is a trivial change to replace the sprintf loop with snprintf using up-to-date format capability.

Quoted 2 lines · Christopher Heiny

This is a trivial change to replace the sprintf loop with snprintf using up-to-date format capability.

Hmm, how about we do this instead:

Quoted 5 lines · Dmitry Torokhov

Quoted 2 lines · Christopher Heiny

This is a trivial change to replace the sprintf loop with snprintf using up-to-date format capability.

Hmm, how about we do this instead:

And another small one...

Input: synaptics-rmi4 - transport name should be a const pointer

From: Dmitry Torokhov <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---
  drivers/input/rmi4/rmi_bus.h |    2 +-
  drivers/input/rmi4/rmi_i2c.c |    4 +---
  2 files changed, 2 insertions(+), 4 deletions(-)

drivers/input/rmi4/rmi_bus.h +1 -1

index 65dd934..3e8b57a 100644
@@ -148,7 +148,7 @@ struct rmi_driver {
   * @att_count Number of times ATTN assertions have been handled.
   */
  struct rmi_transport_info {
-	char *proto;
+	const char *proto;
  	long tx_count;
  	long tx_bytes;
  	long tx_errs;

drivers/input/rmi4/rmi_i2c.c +1 -3

index ea01823..ebe74ec 100644
@@ -38,8 +38,6 @@ struct rmi_i2c_data {
  #define RMI_PAGE_SELECT_REGISTER 0xff
  #define RMI_I2C_PAGE(addr) (((addr) >> 8) & 0xff)
 
-static char *xport_proto_name = "i2c";
-
  /*
   * rmi_set_page - Set RMI page
   * @xport: The pointer to the rmi_transport_dev struct

@@ -217,7 +215,7 @@ static int rmi_i2c_probe(struct i2c_client *client,
 
  	xport->write_block = rmi_i2c_write_block;
  	xport->read_block = rmi_i2c_read_block;
-	xport->info.proto = xport_proto_name;
+	xport->info.proto = "i2c";
 
  	mutex_init(&data->page_mutex);
 

Quoted 3 lines · Dmitry Torokhov

Quoted 2 lines · Dmitry Torokhov

Quoted 1 line · Christopher Heiny

This is a trivial change to replace the sprintf loop with snprintf

using

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

up-to-date format capability.

Hmm, how about we do this instead:

And another small one...

Input: synaptics-rmi4 - transport name should be a const pointer

Quoted 3 lines

From: Dmitry Torokhov <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

Looks good to me. I think the correct protocol is to Ack the patch, so...

Acked-by Christopher Heiny <[email protected]>

Review

Quoted 8 lines

---
  drivers/input/rmi4/rmi_bus.h |    2 +-
  drivers/input/rmi4/rmi_i2c.c |    4 +---
  2 files changed, 2 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/input/rmi4/rmi_bus.h

Review

Quoted 13 lines

index 65dd934..3e8b57a 100644

drivers/input/rmi4/rmi_bus.h +1 -1

@@ -148,7 +148,7 @@ struct rmi_driver {
   * @att_count Number of times ATTN assertions have been handled.
   */
  struct rmi_transport_info {
-	char *proto;
+	const char *proto;
  	long tx_count;
  	long tx_bytes;
  	long tx_errs;

(quoted diff) +0 -0

b/drivers/input/rmi4/rmi_i2c.c

drivers/input/rmi4/rmi_i2c.c static int rmi_i2c_probe(struct i2c_client

Quoted 13 lines

index ea01823..ebe74ec 100644

drivers/input/rmi4/rmi_i2c.c +0 -2

@@ -38,8 +38,6 @@ struct rmi_i2c_data {
  #define RMI_PAGE_SELECT_REGISTER 0xff
  #define RMI_I2C_PAGE(addr) (((addr) >> 8) & 0xff)
 
-static char *xport_proto_name = "i2c";
-
  /*
   * rmi_set_page - Set RMI page
   * @xport: The pointer to the rmi_transport_dev struct

@@ -217,7 +215,7 @@ static int rmi_i2c_probe(struct i2c_client

*client,

Quoted 9 lines

drivers/input/rmi4/rmi_i2c.c +1 -1

  	xport->write_block = rmi_i2c_write_block;
  	xport->read_block = rmi_i2c_read_block;
-	xport->info.proto = xport_proto_name;
+	xport->info.proto = "i2c";
 
  	mutex_init(&data->page_mutex);
 
 

Quoted 15 lines · Christopher Heiny

Quoted 15 lines · Dmitry Torokhov

Quoted 3 lines

Quoted 3 lines · Dmitry Torokhov

Quoted 2 lines

Quoted 2 lines · Christopher Heiny

Quoted 1 line

Quoted 1 line

This is a trivial change to replace the sprintf loop with snprintf

using

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

up-to-date format capability.

Hmm, how about we do this instead:

And another small one...

Input: synaptics-rmi4 - transport name should be a const pointer

From: Dmitry Torokhov<[email protected]>

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Signed-off-by Dmitry Torokhov<[email protected]>

Looks good to me. I think the correct protocol is to Ack the patch, so...

Acked-by Christopher Heiny<[email protected]>

Both or only the const name one?

Quoted 2 lines · Christopher Heiny

This is a trivial change to replace the sprintf loop with snprintf using up-to-date format capability.

Hmm, how about we do this instead:

Input: synaptics-rmi4 - clean up debug handling in rmi_i2c

From: Dmitry Torokhov <[email protected]>

Kernel now has standard facility to format and print binary buffers, let's use it. By doing so we no longer need to allocate memory for debug buffers and we can let debugfs code go as well.

We have a new limitation however - output is limited to the first 64 bytes of the buffer. However if we really need to capture larger messages dumping them in dmesg is not right interface anyway.

Also clean up extra includes, while we are at it.

Signed-off-by Dmitry Torokhov <[email protected]>

---
  drivers/input/rmi4/rmi_i2c.c |  131 +++++-------------------------------------
  1 file changed, 15 insertions(+), 116 deletions(-)

drivers/input/rmi4/rmi_i2c.c +15 -116

index b33074c..c2ad1aa 100644
@@ -8,16 +8,10 @@
   */
 
  #include <linux/kernel.h>
-#include <linux/delay.h>
  #include <linux/i2c.h>
-#include <linux/interrupt.h>
-#include <linux/kconfig.h>
-#include <linux/lockdep.h>
  #include <linux/module.h>
-#include <linux/pm.h>
  #include <linux/rmi.h>
  #include <linux/slab.h>
-#include <linux/uaccess.h>
  #include "rmi_driver.h"
 
  #define BUFFER_SIZE_INCREMENT 32

@@ -31,11 +25,6 @@
   *
   * @tx_buf: Buffer used for transmitting data to the sensor over i2c.
   * @tx_buf_size: Size of the buffer
- * @debug_buf: Buffer used for exposing buffer contents using dev_dbg
- * @debug_buf_size: Size of the debug buffer.
- *
- * @comms_debug: Latest data read/written for debugging I2C communications
- * @debugfs_comms: Debugfs file for debugging I2C communications
   */
  struct rmi_i2c_data {
  	struct mutex page_mutex;

@@ -44,56 +33,8 @@ struct rmi_i2c_data {
 
  	u8 *tx_buf;
  	int tx_buf_size;
-	u8 *debug_buf;
-	int debug_buf_size;
-
-	u32 comms_debug;
-#ifdef CONFIG_RMI4_DEBUG
-	struct dentry *debugfs_comms;
-#endif
  };
 
-#ifdef CONFIG_RMI4_DEBUG
-
-static int setup_debugfs(struct rmi_device *rmi_dev, struct rmi_i2c_data *data)
-{
-	if (!rmi_dev->debugfs_root)
-		return -ENODEV;
-
-	data->debugfs_comms = debugfs_create_bool("comms_debug",
-			(S_IRUGO | S_IWUGO), rmi_dev->debugfs_root,
-			&data->comms_debug);
-	if (!data->debugfs_comms || IS_ERR(data->debugfs_comms)) {
-		dev_warn(&rmi_dev->dev,
-			 "Failed to create debugfs comms_debug.\n");
-		data->debugfs_comms = NULL;
-	}
-
-	return 0;
-}
-
-static void teardown_debugfs(struct rmi_i2c_data *data)
-{
-	if (data->debugfs_comms)
-		debugfs_remove(data->debugfs_comms);
-}
-
-#else
-
-static inline int setup_debugfs(struct rmi_device *rmi_dev,
-				struct rmi_i2c_data *data)
-{
-	return 0;
-}
-
-static inline void teardown_debugfs(struct rmi_i2c_data *data)
-{
-}
-
-#endif
-
-#define COMMS_DEBUG(data) (IS_ENABLED(CONFIG_RMI4_DEBUG) && data->comms_debug)
-
  #define RMI_PAGE_SELECT_REGISTER 0xff
  #define RMI_I2C_PAGE(addr) (((addr) >> 8) & 0xff)
 

@@ -120,8 +61,7 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
  	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
  	int retval;
 
-	if (COMMS_DEBUG(data))
-		dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
+	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
  			txbuf[0], txbuf[1]);
  	xport->info.tx_count++;
  	xport->info.tx_bytes += sizeof(txbuf);

@@ -136,34 +76,6 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
  	return 0;
  }
 
-static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
-			     const u8 *buf, const int len) {
-	int i;
-	int n = 0;
-	char *temp;
-	int dbg_size = 3 * len + 1;
-
-	if (!data->debug_buf || data->debug_buf_size < dbg_size) {
-		if (data->debug_buf)
-			devm_kfree(dev, data->debug_buf);
-		data->debug_buf_size = dbg_size + BUFFER_SIZE_INCREMENT;
-		data->debug_buf = devm_kzalloc(dev, data->debug_buf_size,
-					       GFP_KERNEL);
-		if (!data->debug_buf) {
-			data->debug_buf_size = 0;
-			return -ENOMEM;
-		}
-	}
-	temp = data->debug_buf;
-
-	for (i = 0; i < len; i++) {
-		n = sprintf(temp, " %02x", buf[i]);
-		temp += n;
-	}
-
-	return 0;
-}
-
  static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
  			       const void *buf, const int len)
  {

@@ -195,12 +107,8 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
  			goto exit;
  	}
 
-	if (COMMS_DEBUG(data)) {
-		int rc = copy_to_debug_buf(&client->dev, data, (u8 *) buf, len);
-		if (!rc)
-			dev_dbg(&client->dev, "writes %d bytes at %#06x:%s\n",
-				len, addr, data->debug_buf);
-	}
+	dev_dbg(&client->dev,
+		"writes %d bytes at %#06x: %*ph\n", len, addr, len, buf);
 
  	xport->info.tx_count++;
  	xport->info.tx_bytes += tx_size;

@@ -232,8 +140,7 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
  			goto exit;
  	}
 
-	if (COMMS_DEBUG(data))
-		dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
+	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
 
  	xport->info.tx_count++;
  	xport->info.tx_bytes += sizeof(txbuf);

@@ -244,18 +151,16 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
  		goto exit;
  	}
 
-	retval = i2c_master_recv(client, (u8 *) buf, len);
+	retval = i2c_master_recv(client, buf, len);
 
  	xport->info.rx_count++;
  	xport->info.rx_bytes += len;
  	if (retval < 0)
  		xport->info.rx_errs++;
-	else if (COMMS_DEBUG(data)) {
-		int rc = copy_to_debug_buf(&client->dev, data, (u8 *) buf, len);
-		if (!rc)
-			dev_dbg(&client->dev, "read %d bytes at %#06x:%s\n",
-				len, addr, data->debug_buf);
-	}
+	else
+		dev_dbg(&client->dev,
+			"read %d bytes at %#06x: %*ph\n",
+			len, addr, len, buf);
 
  exit:
  	mutex_unlock(&data->page_mutex);

@@ -265,9 +170,9 @@ exit:
  static int rmi_i2c_probe(struct i2c_client *client,
  			 const struct i2c_device_id *id)
  {
+	const struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
  	struct rmi_transport_dev *xport;
  	struct rmi_i2c_data *data;
-	struct rmi_device_platform_data *pdata = client->dev.platform_data;
  	int retval;
 
  	if (!pdata) {

@@ -318,7 +223,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
 
  	mutex_init(&data->page_mutex);
 
-	/* Setting the page to zero will (a) make sure the PSR is in a
+	/*
+	 * Setting the page to zero will (a) make sure the PSR is in a
  	 * known state, and (b) make sure we can talk to the device.
  	 */
  	retval = rmi_set_page(xport, 0);

@@ -335,11 +241,6 @@ static int rmi_i2c_probe(struct i2c_client *client,
  	}
  	i2c_set_clientdata(client, xport);
 
-	retval = setup_debugfs(xport->rmi_dev, data);
-	if (retval < 0)
-		dev_warn(&client->dev, "Failed to setup debugfs. Code: %d.\n",
-			 retval);
-
  	dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
  			client->addr);
  	return 0;

@@ -353,14 +254,12 @@ err_gpio:
  static int rmi_i2c_remove(struct i2c_client *client)
  {
  	struct rmi_transport_dev *xport = i2c_get_clientdata(client);
-	struct rmi_device_platform_data *pd = client->dev.platform_data;
-
-	teardown_debugfs(xport->data);
+	struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
 
  	rmi_unregister_transport_device(xport);
 
-	if (pd->gpio_config)
-		pd->gpio_config(&pd->gpio_data, false);
+	if (pdata->gpio_config)
+		pdata->gpio_config(&pdata->gpio_data, false);
 
  	return 0;
  }

Quoted 6 lines · Dmitry Torokhov

Quoted 2 lines · Christopher Heiny

This is a trivial change to replace the sprintf loop with snprintf using up-to-date format capability.

Hmm, how about we do this instead:

Quoted 7 lines

Input: synaptics-rmi4 - clean up debug handling in rmi_i2c

From: Dmitry Torokhov <[email protected]>

Kernel now has standard facility to format and print binary buffers, let's use it. By doing so we no longer need to allocate memory for debug buffers and we can let debugfs code go as well.

Not sure where to put this comment, so I'll drop it here.

I agree the buffers can go. I realized that on the drive home last night, but was too tired to follow up.

Talking with some of the folks who use this feature, there's a desire to keep some sort of finer control on whether the comms buffers are printed or not - either the existing debugfs setup (preferred, since it lets them turn on the dmesg clutter only when needed), or by converting to a config option such as CONFIG_RMI4_COMMS_DEBUG. It's very useful in new platform development, since there's a surprising number of ways in which the reads and writes can go wonky on new hardware.

Quoted 20 lines · Christopher Heiny

Quoted 20 lines · Dmitry Torokhov

Quoted 6 lines

Quoted 6 lines · Christopher Heiny

Quoted 2 lines

Quoted 2 lines

This is a trivial change to replace the sprintf loop with snprintf using up-to-date format capability.

Hmm, how about we do this instead:

Quoted 7 lines

Quoted 7 lines

Input: synaptics-rmi4 - clean up debug handling in rmi_i2c

From: Dmitry Torokhov<[email protected]>

Kernel now has standard facility to format and print binary buffers, let's use it. By doing so we no longer need to allocate memory for debug buffers and we can let debugfs code go as well.

Not sure where to put this comment, so I'll drop it here.

I agree the buffers can go. I realized that on the drive home last night, but was too tired to follow up.

Quoted 8 lines

Quoted 8 lines

Talking with some of the folks who use this feature, there's a desire to keep some sort of finer control on whether the comms buffers are printed or not - either the existing debugfs setup (preferred, since it lets them turn on the dmesg clutter only when needed), or by converting to a config option such as CONFIG_RMI4_COMMS_DEBUG. It's very useful in new platform development, since there's a surprising number of ways in which the reads and writes can go wonky on new hardware.

That is why you have CONFIG_DYNAMIC_DEBUG: you can activate these debug statements at will using the common kernel mechanisms.

Or we could convert them to dev_vdbg() and then it will be just a tiny transport module recompile with DEBUG defined.

Quoted 33 lines · Dmitry Torokhov

Quoted 31 lines · Christopher Heiny

Quoted 31 lines · Dmitry Torokhov

Quoted 12 lines

Quoted 2 lines · Christopher Heiny

Quoted 1 line

Quoted 1 line

This is a trivial change to replace the sprintf loop with

snprintf using

Quoted 9 lines

Quoted 1 line

Quoted 1 line

up-to-date format capability.

Hmm, how about we do this instead:

Input: synaptics-rmi4 - clean up debug handling in rmi_i2c

From: Dmitry Torokhov<[email protected]>

Kernel now has standard facility to format and print binary

buffers, let's

Quoted 1 line

Quoted 1 line

use it. By doing so we no longer need to allocate memory for

debug buffers

Quoted 1 line

Quoted 1 line

and we can let debugfs code go as well.

Not sure where to put this comment, so I'll drop it here.

I agree the buffers can go. I realized that on the drive home last night, but was too tired to follow up.

Talking with some of the folks who use this feature, there's a desire to keep some sort of finer control on whether the comms buffers are printed or not - either the existing debugfs setup (preferred, since it lets them turn on the dmesg clutter only when needed), or by converting to a config option such as CONFIG_RMI4_COMMS_DEBUG. It's very useful in new platform development, since there's a surprising number of ways in which the reads and writes can go wonky on new hardware.

Quoted 2 lines

That is why you have CONFIG_DYNAMIC_DEBUG: you can activate these debug statements at will using the common kernel mechanisms.

I'll check this out and get back.

Quoted 34 lines · Christopher Heiny

Quoted 33 lines · Dmitry Torokhov

Quoted 31 lines · Christopher Heiny

Quoted 31 lines · Dmitry Torokhov

Quoted 12 lines

Quoted 2 lines · Christopher Heiny

Quoted 1 line

Quoted 1 line

This is a trivial change to replace the sprintf loop with

snprintf using

Quoted 9 lines

Quoted 1 line

Quoted 1 line

up-to-date format capability.

Hmm, how about we do this instead:

Input: synaptics-rmi4 - clean up debug handling in rmi_i2c

From: Dmitry Torokhov<[email protected]>

Kernel now has standard facility to format and print binary

buffers, let's

Quoted 1 line

Quoted 1 line

use it. By doing so we no longer need to allocate memory for

debug buffers

Quoted 1 line

Quoted 1 line

and we can let debugfs code go as well.

Not sure where to put this comment, so I'll drop it here.

I agree the buffers can go. I realized that on the drive home last night, but was too tired to follow up.

Talking with some of the folks who use this feature, there's a desire to keep some sort of finer control on whether the comms buffers are printed or not - either the existing debugfs setup (preferred, since it lets them turn on the dmesg clutter only when needed), or by converting to a config option such as CONFIG_RMI4_COMMS_DEBUG. It's very useful in new platform development, since there's a surprising number of ways in which the reads and writes can go wonky on new hardware.

Quoted 4 lines

Quoted 2 lines

That is why you have CONFIG_DYNAMIC_DEBUG: you can activate these debug statements at will using the common kernel mechanisms.

I'll check this out and get back.

Looks CONFIG_DYNAMIC_DEBUG will work fine!

Acked-by Christopher Heiny <[email protected]>

Input: synaptics-rmi4 - fix disabling gpio config in i2c transport

From: Dmitry Torokhov<[email protected]>

We need to pass into pdata->gpio_config() gpio_data which is already a pointer, and not its address.

Signed-off-by Dmitry Torokhov<[email protected]>

---
  drivers/input/rmi4/rmi_i2c.c |    8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

drivers/input/rmi4/rmi_i2c.c +3 -2

index ebe74ec..12aea8c 100644
@@ -168,7 +168,8 @@ exit:
  static int rmi_i2c_probe(struct i2c_client *client,
  			 const struct i2c_device_id *id)
  {
-	const struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
+	const struct rmi_device_platform_data *pdata =
+				dev_get_platdata(&client->dev);
  	struct rmi_transport_dev *xport;
  	struct rmi_i2c_data *data;
  	int retval;

@@ -250,12 +251,13 @@ err_gpio:
  static int rmi_i2c_remove(struct i2c_client *client)
  {
  	struct rmi_transport_dev *xport = i2c_get_clientdata(client);
-	struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
+	const struct rmi_device_platform_data *pdata =