[PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

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

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

 drivers/input/rmi4/rmi_i2c.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index d94114b..8a93dad 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -138,9 +138,6 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
 
 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) {
@@ -154,12 +151,8 @@ static int copy_to_debug_buf(struct device *dev, struct rmi_i2c_data *data,
 			return -ENOMEM;
 		}
 	}
-	temp = data->debug_buf;
 
-	for (i = 0; i < len; i++) {
-		n = sprintf(temp, " %02x", buf[i]);
-		temp += n;
-	}
+	snprintf(data->debug_buf, data->debug_buf_size, "%*ph", len, buf);
 
 	return 0;
 }

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Dmitry Torokhov]

On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
> 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 <dmitry.torokhov@gmail.com>

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 <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_i2c.c |  131 +++++-------------------------------------
 1 file changed, 15 insertions(+), 116 deletions(-)

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index b33074c..c2ad1aa 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -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;
 }

-- 
Dmitry

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Dmitry Torokhov]

On Thu, Jan 09, 2014 at 12:04:54AM -0800, Dmitry Torokhov wrote:
> On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
> > 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 <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_bus.h |    2 +-
 drivers/input/rmi4/rmi_i2c.c |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 65dd934..3e8b57a 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -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;
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index ea01823..ebe74ec 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -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);
 

-- 
Dmitry

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

On 01/09/2014 12:28 AM, Dmitry Torokhov wrote:
> On Thu, Jan 09, 2014 at 12:04:54AM -0800, Dmitry Torokhov wrote:
>> On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
>>> 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 <dmitry.torokhov@gmail.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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

Acked-by: Christopher Heiny <cheiny@synaptics.com>


> ---
>   drivers/input/rmi4/rmi_bus.h |    2 +-
>   drivers/input/rmi4/rmi_i2c.c |    4 +---
>   2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index 65dd934..3e8b57a 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -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;
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index ea01823..ebe74ec 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -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);
>
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Dmitry Torokhov]

Christopher Heiny <cheiny@synaptics.com> wrote:
>On 01/09/2014 12:28 AM, Dmitry Torokhov wrote:
>> On Thu, Jan 09, 2014 at 12:04:54AM -0800, Dmitry Torokhov wrote:
>>> On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
>>>> 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 <dmitry.torokhov@gmail.com>
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
>Looks good to me.  I think the correct protocol is to Ack the patch,
>so...
>
>Acked-by: Christopher Heiny <cheiny@synaptics.com>

Both or only the const name one?

>
>
>> ---
>>   drivers/input/rmi4/rmi_bus.h |    2 +-
>>   drivers/input/rmi4/rmi_i2c.c |    4 +---
>>   2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_bus.h
>b/drivers/input/rmi4/rmi_bus.h
>> index 65dd934..3e8b57a 100644
>> --- a/drivers/input/rmi4/rmi_bus.h
>> +++ b/drivers/input/rmi4/rmi_bus.h
>> @@ -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;
>> diff --git a/drivers/input/rmi4/rmi_i2c.c
>b/drivers/input/rmi4/rmi_i2c.c
>> index ea01823..ebe74ec 100644
>> --- a/drivers/input/rmi4/rmi_i2c.c
>> +++ b/drivers/input/rmi4/rmi_i2c.c
>> @@ -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);
>>
>>


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

On 01/09/2014 12:48 PM, Dmitry Torokhov wrote:
> Christopher Heiny<cheiny@synaptics.com>  wrote:
>> >On 01/09/2014 12:28 AM, Dmitry Torokhov wrote:
>>> >>On Thu, Jan 09, 2014 at 12:04:54AM -0800, Dmitry Torokhov wrote:
>>>> >>>On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
>>>>> >>>>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<dmitry.torokhov@gmail.com>
>>> >>
>>> >>Signed-off-by: Dmitry Torokhov<dmitry.torokhov@gmail.com>
>> >
>> >Looks good to me.  I think the correct protocol is to Ack the patch,
>> >so...
>> >
>> >Acked-by: Christopher Heiny<cheiny@synaptics.com>
> Both or only the const name one?
>

The const name one only at this point.  I have a couple of comments on 
the debug output, one.

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

On 01/09/2014 12:04 AM, Dmitry Torokhov wrote:
> On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
>> 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 <dmitry.torokhov@gmail.com>
>
> 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.

> 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.

That's good, too.


> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_i2c.c |  131 +++++-------------------------------------
>   1 file changed, 15 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index b33074c..c2ad1aa 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -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;
>   }
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Dmitry Torokhov]

On Thu, Jan 09, 2014 at 01:23:37PM -0800, Christopher Heiny wrote:
> On 01/09/2014 12:04 AM, Dmitry Torokhov wrote:
> >On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
> >>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 <dmitry.torokhov@gmail.com>
> >
> >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.

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.

Thanks,

-- 
Dmitry

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

On 01/09/2014 01:29 PM, Dmitry Torokhov wrote:
> On Thu, Jan 09, 2014 at 01:23:37PM -0800, Christopher Heiny wrote:
>> >On 01/09/2014 12:04 AM, Dmitry Torokhov wrote:
>>> > >On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
>>>> > >>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<dmitry.torokhov@gmail.com>
>>> > >
>>> > >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.
 >
> 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.

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


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

On 01/09/2014 01:38 PM, Christopher Heiny wrote:
> On 01/09/2014 01:29 PM, Dmitry Torokhov wrote:
>> On Thu, Jan 09, 2014 at 01:23:37PM -0800, Christopher Heiny wrote:
>>> >On 01/09/2014 12:04 AM, Dmitry Torokhov wrote:
>>>> > >On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
>>>>> > >>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<dmitry.torokhov@gmail.com>
>>>> > >
>>>> > >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.
>  >
>> 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 <cheiny@synaptics.com>

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Dmitry Torokhov]

On Thu, Jan 09, 2014 at 02:11:00PM -0800, Christopher Heiny wrote:
> On 01/09/2014 01:38 PM, Christopher Heiny wrote:
> >On 01/09/2014 01:29 PM, Dmitry Torokhov wrote:
> >>On Thu, Jan 09, 2014 at 01:23:37PM -0800, Christopher Heiny wrote:
> >>>>On 01/09/2014 12:04 AM, Dmitry Torokhov wrote:
> >>>>> >On Wed, Jan 08, 2014 at 05:18:39PM -0800, Christopher Heiny wrote:
> >>>>>> >>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<dmitry.torokhov@gmail.com>
> >>>>> >
> >>>>> >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.
> > >
> >>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 <cheiny@synaptics.com>

Cool, thanks! And another one:

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_i2c.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index ebe74ec..12aea8c 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -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 =
+				dev_get_platdata(&client->dev);
 
 	rmi_unregister_transport_device(xport);
 
 	if (pdata->gpio_config)
-		pdata->gpio_config(&pdata->gpio_data, false);
+		pdata->gpio_config(pdata->gpio_data, false);
 
 	return 0;
 }

-- 
Dmitry

Re: [PATCH v2] input: synaptics-rmi4 - use snprintf instead of sprintf in rmi_i2c.c

[Christopher Heiny]

On 01/09/2014 02:25 PM, Dmitry Torokhov wrote:
> Input: synaptics-rmi4 - fix disabling gpio config in i2c transport
>
> From: Dmitry Torokhov<dmitry.torokhov@gmail.com>
>
> We need to pass into pdata->gpio_config() gpio_data which is already a
> pointer, and not its address.
>
> Signed-off-by: Dmitry Torokhov<dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_i2c.c |    8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index ebe74ec..12aea8c 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -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 =
> +				dev_get_platdata(&client->dev);
>
>   	rmi_unregister_transport_device(xport);
>
>   	if (pdata->gpio_config)
> -		pdata->gpio_config(&pdata->gpio_data, false);
> +		pdata->gpio_config(pdata->gpio_data, false);
>
>   	return 0;
>   }

It's just plain freaky that no problem ever arose from that, but 
definitely it's a bug.

Acked-by: Christopher Heiny <cheiny@synaptics.com>