[PATCH 1/4] Input: synaptics-rmi4 - split of transport ops into a separate structure

[PATCH 1/4] Input: synaptics-rmi4 - split of transport ops into a separate structure

[Dmitry Torokhov]

Split off transport operations from rmi_transport_dev into a separate
structure that will be shared between all devices using the same transport
and use const pointer to access it.

Change signature on transport methods so that length is using the proper
tyep - size_t.

Also rename rmi_transport_info to rmi_transport_stats and move protocol
name (which is the only immutable piece of data there) into the transport
device itself.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_bus.h    | 64 ++++++++++++++++++++++++-----------------
 drivers/input/rmi4/rmi_driver.c |  8 +++---
 drivers/input/rmi4/rmi_i2c.c    | 49 ++++++++++++++++---------------
 3 files changed, 67 insertions(+), 54 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 3e8b57a..ccf26dc 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -135,26 +135,25 @@ struct rmi_driver {
 #define to_rmi_driver(d) \
 	container_of(d, struct rmi_driver, driver);
 
-/** struct rmi_transport_info - diagnostic information about the RMI transport
+/**
+ * struct rmi_transport_stats - diagnostic information about the RMI transport
  * device, used in the xport_info debugfs file.
  *
  * @proto String indicating the protocol being used.
  * @tx_count Number of transmit operations.
- * @tx_bytes Number of bytes transmitted.
  * @tx_errs  Number of errors encountered during transmit operations.
+ * @tx_bytes Number of bytes transmitted.
  * @rx_count Number of receive operations.
- * @rx_bytes Number of bytes received.
  * @rx_errs  Number of errors encountered during receive operations.
- * @att_count Number of times ATTN assertions have been handled.
+ * @rx_bytes Number of bytes received.
  */
-struct rmi_transport_info {
-	const char *proto;
-	long tx_count;
-	long tx_bytes;
-	long tx_errs;
-	long rx_count;
-	long rx_bytes;
-	long rx_errs;
+struct rmi_transport_stats {
+	unsigned long tx_count;
+	unsigned long tx_errs;
+	size_t tx_bytes;
+	unsigned long rx_count;
+	unsigned long rx_errs;
+	size_t rx_bytes;
 };
 
 /**
@@ -162,13 +161,14 @@ struct rmi_transport_info {
  *
  * @dev: Pointer to the communication device, e.g. i2c or spi
  * @rmi_dev: Pointer to the RMI device
- * @write_block: Writing a block of data to the specified address
- * @read_block: Read a block of data from the specified address.
  * @irq_thread: if not NULL, the sensor driver will use this instead of the
  * default irq_thread implementation.
  * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
  * handling
  * @data: Private data pointer
+ * @proto_name: name of the transport protocol (SPI, i2c, etc)
+ * @ops: pointer to transport operations implementation
+ * @stats: transport statistics
  *
  * The RMI transport device implements the glue between different communication
  * buses such as I2C and SPI.
@@ -178,20 +178,30 @@ struct rmi_transport_dev {
 	struct device *dev;
 	struct rmi_device *rmi_dev;
 
-	int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
-			   const void *buf, const int len);
-	int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
-			  void *buf, const int len);
-
-	int (*enable_device) (struct rmi_transport_dev *xport);
-	void (*disable_device) (struct rmi_transport_dev *xport);
-
 	irqreturn_t (*irq_thread)(int irq, void *p);
 	irqreturn_t (*hard_irq)(int irq, void *p);
 
 	void *data;
 
-	struct rmi_transport_info info;
+	const char *proto_name;
+	const struct rmi_transport_ops *ops;
+	struct rmi_transport_stats stats;
+};
+
+/**
+ * struct rmi_transport_ops - defines transport protocol operations.
+ *
+ * @write_block: Writing a block of data to the specified address
+ * @read_block: Read a block of data from the specified address.
+ */
+struct rmi_transport_ops {
+	int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
+			   const void *buf, size_t len);
+	int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
+			  void *buf, size_t len);
+
+	int (*enable_device) (struct rmi_transport_dev *xport);
+	void (*disable_device) (struct rmi_transport_dev *xport);
 };
 
 /**
@@ -232,7 +242,7 @@ bool rmi_is_physical_device(struct device *dev);
  */
 static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
 {
-	return d->xport->read_block(d->xport, addr, buf, 1);
+	return d->xport->ops->read_block(d->xport, addr, buf, 1);
 }
 
 /**
@@ -248,7 +258,7 @@ static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
 static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
 				 const int len)
 {
-	return d->xport->read_block(d->xport, addr, buf, len);
+	return d->xport->ops->read_block(d->xport, addr, buf, len);
 }
 
 /**
@@ -262,7 +272,7 @@ static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
  */
 static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
 {
-	return d->xport->write_block(d->xport, addr, &data, 1);
+	return d->xport->ops->write_block(d->xport, addr, &data, 1);
 }
 
 /**
@@ -278,7 +288,7 @@ static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
 static inline int rmi_write_block(struct rmi_device *d, u16 addr,
 				  const void *buf, const int len)
 {
-	return d->xport->write_block(d->xport, addr, buf, len);
+	return d->xport->ops->write_block(d->xport, addr, buf, len);
 }
 
 int rmi_register_transport_device(struct rmi_transport_dev *xport);
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index eb790ff..3483e5b 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -126,8 +126,8 @@ static void disable_sensor(struct rmi_device *rmi_dev)
 	if (!data->irq)
 		disable_polling(rmi_dev);
 
-	if (rmi_dev->xport->disable_device)
-		rmi_dev->xport->disable_device(rmi_dev->xport);
+	if (rmi_dev->xport->ops->disable_device)
+		rmi_dev->xport->ops->disable_device(rmi_dev->xport);
 
 	if (data->irq) {
 		disable_irq(data->irq);
@@ -146,8 +146,8 @@ static int enable_sensor(struct rmi_device *rmi_dev)
 	if (data->enabled)
 		return 0;
 
-	if (rmi_dev->xport->enable_device) {
-		retval = rmi_dev->xport->enable_device(rmi_dev->xport);
+	if (rmi_dev->xport->ops->enable_device) {
+		retval = rmi_dev->xport->ops->enable_device(rmi_dev->xport);
 		if (retval)
 			return retval;
 	}
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index 12aea8c..40badf3 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -61,11 +61,11 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
 
 	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
 			txbuf[0], txbuf[1]);
-	xport->info.tx_count++;
-	xport->info.tx_bytes += sizeof(txbuf);
+	xport->stats.tx_count++;
+	xport->stats.tx_bytes += sizeof(txbuf);
 	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
 	if (retval != sizeof(txbuf)) {
-		xport->info.tx_errs++;
+		xport->stats.tx_errs++;
 		dev_err(&client->dev,
 			"%s: set page failed: %d.", __func__, retval);
 		return (retval < 0) ? retval : -EIO;
@@ -75,12 +75,12 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
 }
 
 static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
-			       const void *buf, const int len)
+			       const void *buf, size_t len)
 {
 	struct i2c_client *client = to_i2c_client(xport->dev);
 	struct rmi_i2c_data *data = xport->data;
+	size_t tx_size = len + 1;
 	int retval;
-	int tx_size = len + 1;
 
 	mutex_lock(&data->page_mutex);
 
@@ -106,13 +106,13 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
 	}
 
 	dev_dbg(&client->dev,
-		"writes %d bytes at %#06x: %*ph\n", len, addr, len, buf);
+		"writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
 
-	xport->info.tx_count++;
-	xport->info.tx_bytes += tx_size;
+	xport->stats.tx_count++;
+	xport->stats.tx_bytes += tx_size;
 	retval = i2c_master_send(client, data->tx_buf, tx_size);
 	if (retval < 0)
-		xport->info.tx_errs++;
+		xport->stats.tx_errs++;
 	else
 		retval--; /* don't count the address byte */
 
@@ -121,9 +121,8 @@ exit:
 	return retval;
 }
 
-
 static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
-			      void *buf, const int len)
+			      void *buf, size_t len)
 {
 	struct i2c_client *client = to_i2c_client(xport->dev);
 	struct rmi_i2c_data *data = xport->data;
@@ -140,31 +139,36 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
 
 	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
 
-	xport->info.tx_count++;
-	xport->info.tx_bytes += sizeof(txbuf);
+	xport->stats.tx_count++;
+	xport->stats.tx_bytes += sizeof(txbuf);
 	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
 	if (retval != sizeof(txbuf)) {
-		xport->info.tx_errs++;
+		xport->stats.tx_errs++;
 		retval = (retval < 0) ? retval : -EIO;
 		goto exit;
 	}
 
-	retval = i2c_master_recv(client, buf, len);
+	xport->stats.rx_count++;
+	xport->stats.rx_bytes += len;
 
-	xport->info.rx_count++;
-	xport->info.rx_bytes += len;
+	retval = i2c_master_recv(client, buf, len);
 	if (retval < 0)
-		xport->info.rx_errs++;
+		xport->stats.rx_errs++;
 	else
 		dev_dbg(&client->dev,
-			"read %d bytes at %#06x: %*ph\n",
-			len, addr, len, buf);
+			"read %zd bytes at %#06x: %*ph\n",
+			len, addr, (int)len, buf);
 
 exit:
 	mutex_unlock(&data->page_mutex);
 	return retval;
 }
 
+static const struct rmi_transport_ops rmi_i2c_ops = {
+	.write_block	= rmi_i2c_write_block,
+	.read_block	= rmi_i2c_read_block,
+};
+
 static int rmi_i2c_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -214,9 +218,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
 	xport->data = data;
 	xport->dev = &client->dev;
 
-	xport->write_block = rmi_i2c_write_block;
-	xport->read_block = rmi_i2c_read_block;
-	xport->info.proto = "i2c";
+	xport->proto_name = "i2c";
+	xport->ops = &rmi_i2c_ops;
 
 	mutex_init(&data->page_mutex);
 
-- 
1.8.4.2

[PATCH 2/4] Input: synaptics-rmi4 - rework transport device allocation

[Dmitry Torokhov]

Instead of allocating common and private part of transport device
separately make private wrap common part and get rid of private data
pointer in the transport device.

Also rename rmi_i2c_data -> rmi_i2c_xport and data -> rmi_i2c.

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

diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index ccf26dc..decb479 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -165,7 +165,6 @@ struct rmi_transport_stats {
  * default irq_thread implementation.
  * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
  * handling
- * @data: Private data pointer
  * @proto_name: name of the transport protocol (SPI, i2c, etc)
  * @ops: pointer to transport operations implementation
  * @stats: transport statistics
@@ -181,8 +180,6 @@ struct rmi_transport_dev {
 	irqreturn_t (*irq_thread)(int irq, void *p);
 	irqreturn_t (*hard_irq)(int irq, void *p);
 
-	void *data;
-
 	const char *proto_name;
 	const struct rmi_transport_ops *ops;
 	struct rmi_transport_stats stats;
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index 40badf3..cdc8527 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -17,22 +17,25 @@
 #define BUFFER_SIZE_INCREMENT 32
 
 /**
- * struct rmi_i2c_data - stores information for i2c communication
+ * struct rmi_i2c_xport - stores information for i2c communication
+ *
+ * @xport: The transport interface structure
  *
  * @page_mutex: Locks current page to avoid changing pages in unexpected ways.
  * @page: Keeps track of the current virtual page
- * @xport: Pointer to the transport interface
  *
  * @tx_buf: Buffer used for transmitting data to the sensor over i2c.
  * @tx_buf_size: Size of the buffer
  */
-struct rmi_i2c_data {
+struct rmi_i2c_xport {
+	struct rmi_transport_dev xport;
+	struct i2c_client *client;
+
 	struct mutex page_mutex;
 	int page;
-	struct rmi_transport_dev *xport;
 
 	u8 *tx_buf;
-	int tx_buf_size;
+	size_t tx_buf_size;
 };
 
 #define RMI_PAGE_SELECT_REGISTER 0xff
@@ -52,10 +55,10 @@ struct rmi_i2c_data {
  *
  * Returns zero on success, non-zero on failure.
  */
-static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
+static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
 {
-	struct i2c_client *client = to_i2c_client(xport->dev);
-	struct rmi_i2c_data *data = xport->data;
+	struct rmi_transport_dev *xport = &rmi_i2c->xport;
+	struct i2c_client *client = rmi_i2c->client;
 	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
 	int retval;
 
@@ -70,37 +73,40 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
 			"%s: set page failed: %d.", __func__, retval);
 		return (retval < 0) ? retval : -EIO;
 	}
-	data->page = page;
+	rmi_i2c->page = page;
 	return 0;
 }
 
 static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
 			       const void *buf, size_t len)
 {
-	struct i2c_client *client = to_i2c_client(xport->dev);
-	struct rmi_i2c_data *data = xport->data;
+	struct rmi_i2c_xport *rmi_i2c =
+		container_of(xport, struct rmi_i2c_xport, xport);
+	struct i2c_client *client = rmi_i2c->client;
 	size_t tx_size = len + 1;
 	int retval;
 
-	mutex_lock(&data->page_mutex);
-
-	if (!data->tx_buf || data->tx_buf_size < tx_size) {
-		if (data->tx_buf)
-			devm_kfree(&client->dev, data->tx_buf);
-		data->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
-		data->tx_buf = devm_kzalloc(&client->dev, data->tx_buf_size,
-					    GFP_KERNEL);
-		if (!data->tx_buf) {
-			data->tx_buf_size = 0;
+	mutex_lock(&rmi_i2c->page_mutex);
+
+	if (!rmi_i2c->tx_buf || rmi_i2c->tx_buf_size < tx_size) {
+		if (rmi_i2c->tx_buf)
+			devm_kfree(&client->dev, rmi_i2c->tx_buf);
+		rmi_i2c->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
+		rmi_i2c->tx_buf = devm_kzalloc(&client->dev,
+					       rmi_i2c->tx_buf_size,
+					       GFP_KERNEL);
+		if (!rmi_i2c->tx_buf) {
+			rmi_i2c->tx_buf_size = 0;
 			retval = -ENOMEM;
 			goto exit;
 		}
 	}
-	data->tx_buf[0] = addr & 0xff;
-	memcpy(data->tx_buf + 1, buf, len);
 
-	if (RMI_I2C_PAGE(addr) != data->page) {
-		retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
+	rmi_i2c->tx_buf[0] = addr & 0xff;
+	memcpy(rmi_i2c->tx_buf + 1, buf, len);
+
+	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
+		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
 		if (retval < 0)
 			goto exit;
 	}
@@ -110,29 +116,30 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
 
 	xport->stats.tx_count++;
 	xport->stats.tx_bytes += tx_size;
-	retval = i2c_master_send(client, data->tx_buf, tx_size);
+	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
 	if (retval < 0)
 		xport->stats.tx_errs++;
 	else
 		retval--; /* don't count the address byte */
 
 exit:
-	mutex_unlock(&data->page_mutex);
+	mutex_unlock(&rmi_i2c->page_mutex);
 	return retval;
 }
 
 static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
 			      void *buf, size_t len)
 {
-	struct i2c_client *client = to_i2c_client(xport->dev);
-	struct rmi_i2c_data *data = xport->data;
+	struct rmi_i2c_xport *rmi_i2c =
+		container_of(xport, struct rmi_i2c_xport, xport);
+	struct i2c_client *client = rmi_i2c->client;
 	u8 txbuf[1] = {addr & 0xff};
 	int retval;
 
-	mutex_lock(&data->page_mutex);
+	mutex_lock(&rmi_i2c->page_mutex);
 
-	if (RMI_I2C_PAGE(addr) != data->page) {
-		retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
+	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
+		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
 		if (retval < 0)
 			goto exit;
 	}
@@ -160,7 +167,7 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
 			len, addr, (int)len, buf);
 
 exit:
-	mutex_unlock(&data->page_mutex);
+	mutex_unlock(&rmi_i2c->page_mutex);
 	return retval;
 }
 
@@ -174,14 +181,14 @@ static int rmi_i2c_probe(struct i2c_client *client,
 {
 	const struct rmi_device_platform_data *pdata =
 				dev_get_platdata(&client->dev);
-	struct rmi_transport_dev *xport;
-	struct rmi_i2c_data *data;
+	struct rmi_i2c_xport *rmi_i2c;
 	int retval;
 
 	if (!pdata) {
 		dev_err(&client->dev, "no platform data\n");
 		return -EINVAL;
 	}
+
 	dev_dbg(&client->dev, "Probing %s at %#02x (GPIO %d).\n",
 		pdata->sensor_name ? pdata->sensor_name : "-no name-",
 		client->addr, pdata->attn_gpio);
@@ -202,44 +209,36 @@ static int rmi_i2c_probe(struct i2c_client *client,
 		}
 	}
 
-	xport = devm_kzalloc(&client->dev, sizeof(struct rmi_transport_dev),
+	rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport),
 				GFP_KERNEL);
-
-	if (!xport)
+	if (!rmi_i2c)
 		return -ENOMEM;
 
-	data = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_data),
-				GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->xport = xport;
-
-	xport->data = data;
-	xport->dev = &client->dev;
+	rmi_i2c->client = client;
+	mutex_init(&rmi_i2c->page_mutex);
 
-	xport->proto_name = "i2c";
-	xport->ops = &rmi_i2c_ops;
-
-	mutex_init(&data->page_mutex);
+	rmi_i2c->xport.dev = &client->dev;
+	rmi_i2c->xport.proto_name = "i2c";
+	rmi_i2c->xport.ops = &rmi_i2c_ops;
 
 	/*
 	 * 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);
+	retval = rmi_set_page(rmi_i2c, 0);
 	if (retval) {
 		dev_err(&client->dev, "Failed to set page select to 0.\n");
 		return retval;
 	}
 
-	retval = rmi_register_transport_device(xport);
+	retval = rmi_register_transport_device(&rmi_i2c->xport);
 	if (retval) {
 		dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
 			client->addr);
 		goto err_gpio;
 	}
-	i2c_set_clientdata(client, xport);
+
+	i2c_set_clientdata(client, rmi_i2c);
 
 	dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
 			client->addr);
@@ -248,16 +247,17 @@ static int rmi_i2c_probe(struct i2c_client *client,
 err_gpio:
 	if (pdata->gpio_config)
 		pdata->gpio_config(pdata->gpio_data, false);
+
 	return retval;
 }
 
 static int rmi_i2c_remove(struct i2c_client *client)
 {
-	struct rmi_transport_dev *xport = i2c_get_clientdata(client);
 	const struct rmi_device_platform_data *pdata =
 				dev_get_platdata(&client->dev);
+	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 
-	rmi_unregister_transport_device(xport);
+	rmi_unregister_transport_device(&rmi_i2c->xport);
 
 	if (pdata->gpio_config)
 		pdata->gpio_config(pdata->gpio_data, false);
-- 
1.8.4.2

[PATCH 3/4] Input: synaptics-rmi4 - fix I2C functionality check

[Dmitry Torokhov]

When adapter does not support required functionality (I2C_FUNC_I2C) we were
returning 0 to the upper layers, making them believe that device bound
successfully.

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

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index cdc8527..c176218 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -193,11 +193,10 @@ static int rmi_i2c_probe(struct i2c_client *client,
 		pdata->sensor_name ? pdata->sensor_name : "-no name-",
 		client->addr, pdata->attn_gpio);
 
-	retval = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
-	if (!retval) {
-		dev_err(&client->dev, "i2c_check_functionality error %d.\n",
-			retval);
-		return retval;
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev,
+			"adapter does not support required functionality.\n");
+		return -ENODEV;
 	}
 
 	if (pdata->gpio_config) {
-- 
1.8.4.2

[PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()

[Dmitry Torokhov]

Instead of using 2 separate transactions when reading from the device let's
use i2c_transfer. Because we now have single point of failure I had to
change how we collect statistics. I elected to drop control data from the
stats and only track number of bytes read/written for the device data.

Also, since we are not prepared to deal with short reads and writes change
read_block_data and write_block_data to indicate error if we detect short
transfers.

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

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index c176218..51f5bc8 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -57,22 +57,17 @@ struct rmi_i2c_xport {
  */
 static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
 {
-	struct rmi_transport_dev *xport = &rmi_i2c->xport;
 	struct i2c_client *client = rmi_i2c->client;
 	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
 	int retval;
 
-	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
-			txbuf[0], txbuf[1]);
-	xport->stats.tx_count++;
-	xport->stats.tx_bytes += sizeof(txbuf);
 	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
 	if (retval != sizeof(txbuf)) {
-		xport->stats.tx_errs++;
 		dev_err(&client->dev,
 			"%s: set page failed: %d.", __func__, retval);
 		return (retval < 0) ? retval : -EIO;
 	}
+
 	rmi_i2c->page = page;
 	return 0;
 }
@@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
 
 	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
 		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
-		if (retval < 0)
+		if (retval)
 			goto exit;
 	}
 
+	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
+	if (retval == tx_size)
+		retval = 0;
+	else if (retval >= 0)
+		retval = -EIO;
+
+exit:
 	dev_dbg(&client->dev,
-		"writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
+		"write %zd bytes at %#06x: %d (%*ph)\n",
+		len, addr, retval, (int)len, buf);
 
 	xport->stats.tx_count++;
-	xport->stats.tx_bytes += tx_size;
-	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
-	if (retval < 0)
+	if (retval)
 		xport->stats.tx_errs++;
 	else
-		retval--; /* don't count the address byte */
+		xport->stats.tx_bytes += len;
 
-exit:
 	mutex_unlock(&rmi_i2c->page_mutex);
 	return retval;
 }
@@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
 	struct rmi_i2c_xport *rmi_i2c =
 		container_of(xport, struct rmi_i2c_xport, xport);
 	struct i2c_client *client = rmi_i2c->client;
-	u8 txbuf[1] = {addr & 0xff};
+	u8 addr_offset = addr & 0xff;
 	int retval;
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.len	= sizeof(addr_offset),
+			.buf	= &addr_offset,
+		},
+		{
+			.addr	= client->addr,
+			.flags	= I2C_M_RD,
+			.len	= len,
+			.buf	= buf,
+		},
+	};
 
 	mutex_lock(&rmi_i2c->page_mutex);
 
 	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
 		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
-		if (retval < 0)
+		if (retval)
 			goto exit;
 	}
 
-	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
+	retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
+	if (retval == sizeof(msgs))
+		retval = 0; /* success */
+	else if (retval >= 0)
+		retval = -EIO;
 
-	xport->stats.tx_count++;
-	xport->stats.tx_bytes += sizeof(txbuf);
-	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
-	if (retval != sizeof(txbuf)) {
-		xport->stats.tx_errs++;
-		retval = (retval < 0) ? retval : -EIO;
-		goto exit;
-	}
+exit:
+	dev_dbg(&client->dev,
+		"read %zd bytes at %#06x: %d (%*ph)\n",
+		len, addr, retval, (int)len, buf);
 
 	xport->stats.rx_count++;
-	xport->stats.rx_bytes += len;
-
-	retval = i2c_master_recv(client, buf, len);
-	if (retval < 0)
+	if (retval)
 		xport->stats.rx_errs++;
 	else
-		dev_dbg(&client->dev,
-			"read %zd bytes at %#06x: %*ph\n",
-			len, addr, (int)len, buf);
+		xport->stats.rx_bytes += len;
 
-exit:
 	mutex_unlock(&rmi_i2c->page_mutex);
 	return retval;
 }
-- 
1.8.4.2

Re: [PATCH 1/4] Input: synaptics-rmi4 - split of transport ops into a separate structure

[Christopher Heiny]

On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Split off transport operations from rmi_transport_dev into a separate
> structure that will be shared between all devices using the same transport
> and use const pointer to access it.
>
> Change signature on transport methods so that length is using the proper
> tyep - size_t.
>
> Also rename rmi_transport_info to rmi_transport_stats and move protocol
> name (which is the only immutable piece of data there) into the transport
> device itself.

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

>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_bus.h    | 64 ++++++++++++++++++++++++-----------------
>   drivers/input/rmi4/rmi_driver.c |  8 +++---
>   drivers/input/rmi4/rmi_i2c.c    | 49 ++++++++++++++++---------------
>   3 files changed, 67 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index 3e8b57a..ccf26dc 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -135,26 +135,25 @@ struct rmi_driver {
>   #define to_rmi_driver(d) \
>   	container_of(d, struct rmi_driver, driver);
>
> -/** struct rmi_transport_info - diagnostic information about the RMI transport
> +/**
> + * struct rmi_transport_stats - diagnostic information about the RMI transport
>    * device, used in the xport_info debugfs file.
>    *
>    * @proto String indicating the protocol being used.
>    * @tx_count Number of transmit operations.
> - * @tx_bytes Number of bytes transmitted.
>    * @tx_errs  Number of errors encountered during transmit operations.
> + * @tx_bytes Number of bytes transmitted.
>    * @rx_count Number of receive operations.
> - * @rx_bytes Number of bytes received.
>    * @rx_errs  Number of errors encountered during receive operations.
> - * @att_count Number of times ATTN assertions have been handled.
> + * @rx_bytes Number of bytes received.
>    */
> -struct rmi_transport_info {
> -	const char *proto;
> -	long tx_count;
> -	long tx_bytes;
> -	long tx_errs;
> -	long rx_count;
> -	long rx_bytes;
> -	long rx_errs;
> +struct rmi_transport_stats {
> +	unsigned long tx_count;
> +	unsigned long tx_errs;
> +	size_t tx_bytes;
> +	unsigned long rx_count;
> +	unsigned long rx_errs;
> +	size_t rx_bytes;
>   };
>
>   /**
> @@ -162,13 +161,14 @@ struct rmi_transport_info {
>    *
>    * @dev: Pointer to the communication device, e.g. i2c or spi
>    * @rmi_dev: Pointer to the RMI device
> - * @write_block: Writing a block of data to the specified address
> - * @read_block: Read a block of data from the specified address.
>    * @irq_thread: if not NULL, the sensor driver will use this instead of the
>    * default irq_thread implementation.
>    * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
>    * handling
>    * @data: Private data pointer
> + * @proto_name: name of the transport protocol (SPI, i2c, etc)
> + * @ops: pointer to transport operations implementation
> + * @stats: transport statistics
>    *
>    * The RMI transport device implements the glue between different communication
>    * buses such as I2C and SPI.
> @@ -178,20 +178,30 @@ struct rmi_transport_dev {
>   	struct device *dev;
>   	struct rmi_device *rmi_dev;
>
> -	int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
> -			   const void *buf, const int len);
> -	int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
> -			  void *buf, const int len);
> -
> -	int (*enable_device) (struct rmi_transport_dev *xport);
> -	void (*disable_device) (struct rmi_transport_dev *xport);
> -
>   	irqreturn_t (*irq_thread)(int irq, void *p);
>   	irqreturn_t (*hard_irq)(int irq, void *p);
>
>   	void *data;
>
> -	struct rmi_transport_info info;
> +	const char *proto_name;
> +	const struct rmi_transport_ops *ops;
> +	struct rmi_transport_stats stats;
> +};
> +
> +/**
> + * struct rmi_transport_ops - defines transport protocol operations.
> + *
> + * @write_block: Writing a block of data to the specified address
> + * @read_block: Read a block of data from the specified address.
> + */
> +struct rmi_transport_ops {
> +	int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
> +			   const void *buf, size_t len);
> +	int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
> +			  void *buf, size_t len);
> +
> +	int (*enable_device) (struct rmi_transport_dev *xport);
> +	void (*disable_device) (struct rmi_transport_dev *xport);
>   };
>
>   /**
> @@ -232,7 +242,7 @@ bool rmi_is_physical_device(struct device *dev);
>    */
>   static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
>   {
> -	return d->xport->read_block(d->xport, addr, buf, 1);
> +	return d->xport->ops->read_block(d->xport, addr, buf, 1);
>   }
>
>   /**
> @@ -248,7 +258,7 @@ static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
>   static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
>   				 const int len)
>   {
> -	return d->xport->read_block(d->xport, addr, buf, len);
> +	return d->xport->ops->read_block(d->xport, addr, buf, len);
>   }
>
>   /**
> @@ -262,7 +272,7 @@ static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
>    */
>   static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
>   {
> -	return d->xport->write_block(d->xport, addr, &data, 1);
> +	return d->xport->ops->write_block(d->xport, addr, &data, 1);
>   }
>
>   /**
> @@ -278,7 +288,7 @@ static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
>   static inline int rmi_write_block(struct rmi_device *d, u16 addr,
>   				  const void *buf, const int len)
>   {
> -	return d->xport->write_block(d->xport, addr, buf, len);
> +	return d->xport->ops->write_block(d->xport, addr, buf, len);
>   }
>
>   int rmi_register_transport_device(struct rmi_transport_dev *xport);
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index eb790ff..3483e5b 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -126,8 +126,8 @@ static void disable_sensor(struct rmi_device *rmi_dev)
>   	if (!data->irq)
>   		disable_polling(rmi_dev);
>
> -	if (rmi_dev->xport->disable_device)
> -		rmi_dev->xport->disable_device(rmi_dev->xport);
> +	if (rmi_dev->xport->ops->disable_device)
> +		rmi_dev->xport->ops->disable_device(rmi_dev->xport);
>
>   	if (data->irq) {
>   		disable_irq(data->irq);
> @@ -146,8 +146,8 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>   	if (data->enabled)
>   		return 0;
>
> -	if (rmi_dev->xport->enable_device) {
> -		retval = rmi_dev->xport->enable_device(rmi_dev->xport);
> +	if (rmi_dev->xport->ops->enable_device) {
> +		retval = rmi_dev->xport->ops->enable_device(rmi_dev->xport);
>   		if (retval)
>   			return retval;
>   	}
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 12aea8c..40badf3 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -61,11 +61,11 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
>
>   	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
>   			txbuf[0], txbuf[1]);
> -	xport->info.tx_count++;
> -	xport->info.tx_bytes += sizeof(txbuf);
> +	xport->stats.tx_count++;
> +	xport->stats.tx_bytes += sizeof(txbuf);
>   	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
>   	if (retval != sizeof(txbuf)) {
> -		xport->info.tx_errs++;
> +		xport->stats.tx_errs++;
>   		dev_err(&client->dev,
>   			"%s: set page failed: %d.", __func__, retval);
>   		return (retval < 0) ? retval : -EIO;
> @@ -75,12 +75,12 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
>   }
>
>   static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> -			       const void *buf, const int len)
> +			       const void *buf, size_t len)
>   {
>   	struct i2c_client *client = to_i2c_client(xport->dev);
>   	struct rmi_i2c_data *data = xport->data;
> +	size_t tx_size = len + 1;
>   	int retval;
> -	int tx_size = len + 1;
>
>   	mutex_lock(&data->page_mutex);
>
> @@ -106,13 +106,13 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>   	}
>
>   	dev_dbg(&client->dev,
> -		"writes %d bytes at %#06x: %*ph\n", len, addr, len, buf);
> +		"writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
>
> -	xport->info.tx_count++;
> -	xport->info.tx_bytes += tx_size;
> +	xport->stats.tx_count++;
> +	xport->stats.tx_bytes += tx_size;
>   	retval = i2c_master_send(client, data->tx_buf, tx_size);
>   	if (retval < 0)
> -		xport->info.tx_errs++;
> +		xport->stats.tx_errs++;
>   	else
>   		retval--; /* don't count the address byte */
>
> @@ -121,9 +121,8 @@ exit:
>   	return retval;
>   }
>
> -
>   static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> -			      void *buf, const int len)
> +			      void *buf, size_t len)
>   {
>   	struct i2c_client *client = to_i2c_client(xport->dev);
>   	struct rmi_i2c_data *data = xport->data;
> @@ -140,31 +139,36 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>
>   	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
>
> -	xport->info.tx_count++;
> -	xport->info.tx_bytes += sizeof(txbuf);
> +	xport->stats.tx_count++;
> +	xport->stats.tx_bytes += sizeof(txbuf);
>   	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
>   	if (retval != sizeof(txbuf)) {
> -		xport->info.tx_errs++;
> +		xport->stats.tx_errs++;
>   		retval = (retval < 0) ? retval : -EIO;
>   		goto exit;
>   	}
>
> -	retval = i2c_master_recv(client, buf, len);
> +	xport->stats.rx_count++;
> +	xport->stats.rx_bytes += len;
>
> -	xport->info.rx_count++;
> -	xport->info.rx_bytes += len;
> +	retval = i2c_master_recv(client, buf, len);
>   	if (retval < 0)
> -		xport->info.rx_errs++;
> +		xport->stats.rx_errs++;
>   	else
>   		dev_dbg(&client->dev,
> -			"read %d bytes at %#06x: %*ph\n",
> -			len, addr, len, buf);
> +			"read %zd bytes at %#06x: %*ph\n",
> +			len, addr, (int)len, buf);
>
>   exit:
>   	mutex_unlock(&data->page_mutex);
>   	return retval;
>   }
>
> +static const struct rmi_transport_ops rmi_i2c_ops = {
> +	.write_block	= rmi_i2c_write_block,
> +	.read_block	= rmi_i2c_read_block,
> +};
> +
>   static int rmi_i2c_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
> @@ -214,9 +218,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
>   	xport->data = data;
>   	xport->dev = &client->dev;
>
> -	xport->write_block = rmi_i2c_write_block;
> -	xport->read_block = rmi_i2c_read_block;
> -	xport->info.proto = "i2c";
> +	xport->proto_name = "i2c";
> +	xport->ops = &rmi_i2c_ops;
>
>   	mutex_init(&data->page_mutex);
>
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 2/4] Input: synaptics-rmi4 - rework transport device allocation

[Christopher Heiny]

On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of allocating common and private part of transport device
> separately make private wrap common part and get rid of private data
> pointer in the transport device.
>
> Also rename rmi_i2c_data -> rmi_i2c_xport and data -> rmi_i2c.

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

>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_bus.h |   3 --
>   drivers/input/rmi4/rmi_i2c.c | 112 +++++++++++++++++++++----------------------
>   2 files changed, 56 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index ccf26dc..decb479 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -165,7 +165,6 @@ struct rmi_transport_stats {
>    * default irq_thread implementation.
>    * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
>    * handling
> - * @data: Private data pointer
>    * @proto_name: name of the transport protocol (SPI, i2c, etc)
>    * @ops: pointer to transport operations implementation
>    * @stats: transport statistics
> @@ -181,8 +180,6 @@ struct rmi_transport_dev {
>   	irqreturn_t (*irq_thread)(int irq, void *p);
>   	irqreturn_t (*hard_irq)(int irq, void *p);
>
> -	void *data;
> -
>   	const char *proto_name;
>   	const struct rmi_transport_ops *ops;
>   	struct rmi_transport_stats stats;
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 40badf3..cdc8527 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -17,22 +17,25 @@
>   #define BUFFER_SIZE_INCREMENT 32
>
>   /**
> - * struct rmi_i2c_data - stores information for i2c communication
> + * struct rmi_i2c_xport - stores information for i2c communication
> + *
> + * @xport: The transport interface structure
>    *
>    * @page_mutex: Locks current page to avoid changing pages in unexpected ways.
>    * @page: Keeps track of the current virtual page
> - * @xport: Pointer to the transport interface
>    *
>    * @tx_buf: Buffer used for transmitting data to the sensor over i2c.
>    * @tx_buf_size: Size of the buffer
>    */
> -struct rmi_i2c_data {
> +struct rmi_i2c_xport {
> +	struct rmi_transport_dev xport;
> +	struct i2c_client *client;
> +
>   	struct mutex page_mutex;
>   	int page;
> -	struct rmi_transport_dev *xport;
>
>   	u8 *tx_buf;
> -	int tx_buf_size;
> +	size_t tx_buf_size;
>   };
>
>   #define RMI_PAGE_SELECT_REGISTER 0xff
> @@ -52,10 +55,10 @@ struct rmi_i2c_data {
>    *
>    * Returns zero on success, non-zero on failure.
>    */
> -static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> +static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
>   {
> -	struct i2c_client *client = to_i2c_client(xport->dev);
> -	struct rmi_i2c_data *data = xport->data;
> +	struct rmi_transport_dev *xport = &rmi_i2c->xport;
> +	struct i2c_client *client = rmi_i2c->client;
>   	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
>   	int retval;
>
> @@ -70,37 +73,40 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
>   			"%s: set page failed: %d.", __func__, retval);
>   		return (retval < 0) ? retval : -EIO;
>   	}
> -	data->page = page;
> +	rmi_i2c->page = page;
>   	return 0;
>   }
>
>   static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>   			       const void *buf, size_t len)
>   {
> -	struct i2c_client *client = to_i2c_client(xport->dev);
> -	struct rmi_i2c_data *data = xport->data;
> +	struct rmi_i2c_xport *rmi_i2c =
> +		container_of(xport, struct rmi_i2c_xport, xport);
> +	struct i2c_client *client = rmi_i2c->client;
>   	size_t tx_size = len + 1;
>   	int retval;
>
> -	mutex_lock(&data->page_mutex);
> -
> -	if (!data->tx_buf || data->tx_buf_size < tx_size) {
> -		if (data->tx_buf)
> -			devm_kfree(&client->dev, data->tx_buf);
> -		data->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
> -		data->tx_buf = devm_kzalloc(&client->dev, data->tx_buf_size,
> -					    GFP_KERNEL);
> -		if (!data->tx_buf) {
> -			data->tx_buf_size = 0;
> +	mutex_lock(&rmi_i2c->page_mutex);
> +
> +	if (!rmi_i2c->tx_buf || rmi_i2c->tx_buf_size < tx_size) {
> +		if (rmi_i2c->tx_buf)
> +			devm_kfree(&client->dev, rmi_i2c->tx_buf);
> +		rmi_i2c->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
> +		rmi_i2c->tx_buf = devm_kzalloc(&client->dev,
> +					       rmi_i2c->tx_buf_size,
> +					       GFP_KERNEL);
> +		if (!rmi_i2c->tx_buf) {
> +			rmi_i2c->tx_buf_size = 0;
>   			retval = -ENOMEM;
>   			goto exit;
>   		}
>   	}
> -	data->tx_buf[0] = addr & 0xff;
> -	memcpy(data->tx_buf + 1, buf, len);
>
> -	if (RMI_I2C_PAGE(addr) != data->page) {
> -		retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
> +	rmi_i2c->tx_buf[0] = addr & 0xff;
> +	memcpy(rmi_i2c->tx_buf + 1, buf, len);
> +
> +	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> +		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
>   		if (retval < 0)
>   			goto exit;
>   	}
> @@ -110,29 +116,30 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
>   	xport->stats.tx_count++;
>   	xport->stats.tx_bytes += tx_size;
> -	retval = i2c_master_send(client, data->tx_buf, tx_size);
> +	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
>   	if (retval < 0)
>   		xport->stats.tx_errs++;
>   	else
>   		retval--; /* don't count the address byte */
>
>   exit:
> -	mutex_unlock(&data->page_mutex);
> +	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
>
>   static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>   			      void *buf, size_t len)
>   {
> -	struct i2c_client *client = to_i2c_client(xport->dev);
> -	struct rmi_i2c_data *data = xport->data;
> +	struct rmi_i2c_xport *rmi_i2c =
> +		container_of(xport, struct rmi_i2c_xport, xport);
> +	struct i2c_client *client = rmi_i2c->client;
>   	u8 txbuf[1] = {addr & 0xff};
>   	int retval;
>
> -	mutex_lock(&data->page_mutex);
> +	mutex_lock(&rmi_i2c->page_mutex);
>
> -	if (RMI_I2C_PAGE(addr) != data->page) {
> -		retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
> +	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> +		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
>   		if (retval < 0)
>   			goto exit;
>   	}
> @@ -160,7 +167,7 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>   			len, addr, (int)len, buf);
>
>   exit:
> -	mutex_unlock(&data->page_mutex);
> +	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
>
> @@ -174,14 +181,14 @@ static int rmi_i2c_probe(struct i2c_client *client,
>   {
>   	const struct rmi_device_platform_data *pdata =
>   				dev_get_platdata(&client->dev);
> -	struct rmi_transport_dev *xport;
> -	struct rmi_i2c_data *data;
> +	struct rmi_i2c_xport *rmi_i2c;
>   	int retval;
>
>   	if (!pdata) {
>   		dev_err(&client->dev, "no platform data\n");
>   		return -EINVAL;
>   	}
> +
>   	dev_dbg(&client->dev, "Probing %s at %#02x (GPIO %d).\n",
>   		pdata->sensor_name ? pdata->sensor_name : "-no name-",
>   		client->addr, pdata->attn_gpio);
> @@ -202,44 +209,36 @@ static int rmi_i2c_probe(struct i2c_client *client,
>   		}
>   	}
>
> -	xport = devm_kzalloc(&client->dev, sizeof(struct rmi_transport_dev),
> +	rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport),
>   				GFP_KERNEL);
> -
> -	if (!xport)
> +	if (!rmi_i2c)
>   		return -ENOMEM;
>
> -	data = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_data),
> -				GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	data->xport = xport;
> -
> -	xport->data = data;
> -	xport->dev = &client->dev;
> +	rmi_i2c->client = client;
> +	mutex_init(&rmi_i2c->page_mutex);
>
> -	xport->proto_name = "i2c";
> -	xport->ops = &rmi_i2c_ops;
> -
> -	mutex_init(&data->page_mutex);
> +	rmi_i2c->xport.dev = &client->dev;
> +	rmi_i2c->xport.proto_name = "i2c";
> +	rmi_i2c->xport.ops = &rmi_i2c_ops;
>
>   	/*
>   	 * 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);
> +	retval = rmi_set_page(rmi_i2c, 0);
>   	if (retval) {
>   		dev_err(&client->dev, "Failed to set page select to 0.\n");
>   		return retval;
>   	}
>
> -	retval = rmi_register_transport_device(xport);
> +	retval = rmi_register_transport_device(&rmi_i2c->xport);
>   	if (retval) {
>   		dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
>   			client->addr);
>   		goto err_gpio;
>   	}
> -	i2c_set_clientdata(client, xport);
> +
> +	i2c_set_clientdata(client, rmi_i2c);
>
>   	dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
>   			client->addr);
> @@ -248,16 +247,17 @@ static int rmi_i2c_probe(struct i2c_client *client,
>   err_gpio:
>   	if (pdata->gpio_config)
>   		pdata->gpio_config(pdata->gpio_data, false);
> +
>   	return retval;
>   }
>
>   static int rmi_i2c_remove(struct i2c_client *client)
>   {
> -	struct rmi_transport_dev *xport = i2c_get_clientdata(client);
>   	const struct rmi_device_platform_data *pdata =
>   				dev_get_platdata(&client->dev);
> +	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>
> -	rmi_unregister_transport_device(xport);
> +	rmi_unregister_transport_device(&rmi_i2c->xport);
>
>   	if (pdata->gpio_config)
>   		pdata->gpio_config(pdata->gpio_data, false);
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 3/4] Input: synaptics-rmi4 - fix I2C functionality check

[Christopher Heiny]

On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> When adapter does not support required functionality (I2C_FUNC_I2C) we were
> returning 0 to the upper layers, making them believe that device bound
> successfully.

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

>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_i2c.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index cdc8527..c176218 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -193,11 +193,10 @@ static int rmi_i2c_probe(struct i2c_client *client,
>   		pdata->sensor_name ? pdata->sensor_name : "-no name-",
>   		client->addr, pdata->attn_gpio);
>
> -	retval = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> -	if (!retval) {
> -		dev_err(&client->dev, "i2c_check_functionality error %d.\n",
> -			retval);
> -		return retval;
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev,
> +			"adapter does not support required functionality.\n");
> +		return -ENODEV;
>   	}
>
>   	if (pdata->gpio_config) {
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()

[Christopher Heiny]

On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of using 2 separate transactions when reading from the device let's
> use i2c_transfer. Because we now have single point of failure I had to
> change how we collect statistics. I elected to drop control data from the
> stats and only track number of bytes read/written for the device data.
>
> Also, since we are not prepared to deal with short reads and writes change
> read_block_data and write_block_data to indicate error if we detect short
> transfers.

We tried this change once before a couple of years ago, but the 
conversion was unsuccessful on some older platforms.  I've tested it on 
some more current platforms, though, and it works there.  The old 
platforms are running 2.6.xx series kernels, and don't look likely ever 
to be updated, So....

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

>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++--------------------
>   1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index c176218..51f5bc8 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -57,22 +57,17 @@ struct rmi_i2c_xport {
>    */
>   static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
>   {
> -	struct rmi_transport_dev *xport = &rmi_i2c->xport;
>   	struct i2c_client *client = rmi_i2c->client;
>   	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
>   	int retval;
>
> -	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> -			txbuf[0], txbuf[1]);
> -	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += sizeof(txbuf);
>   	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
>   	if (retval != sizeof(txbuf)) {
> -		xport->stats.tx_errs++;
>   		dev_err(&client->dev,
>   			"%s: set page failed: %d.", __func__, retval);
>   		return (retval < 0) ? retval : -EIO;
>   	}
> +
>   	rmi_i2c->page = page;
>   	return 0;
>   }
> @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
>   	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
>   		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> -		if (retval < 0)
> +		if (retval)
>   			goto exit;
>   	}
>
> +	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> +	if (retval == tx_size)
> +		retval = 0;
> +	else if (retval >= 0)
> +		retval = -EIO;
> +
> +exit:
>   	dev_dbg(&client->dev,
> -		"writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
> +		"write %zd bytes at %#06x: %d (%*ph)\n",
> +		len, addr, retval, (int)len, buf);
>
>   	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += tx_size;
> -	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> -	if (retval < 0)
> +	if (retval)
>   		xport->stats.tx_errs++;
>   	else
> -		retval--; /* don't count the address byte */
> +		xport->stats.tx_bytes += len;
>
> -exit:
>   	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
> @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>   	struct rmi_i2c_xport *rmi_i2c =
>   		container_of(xport, struct rmi_i2c_xport, xport);
>   	struct i2c_client *client = rmi_i2c->client;
> -	u8 txbuf[1] = {addr & 0xff};
> +	u8 addr_offset = addr & 0xff;
>   	int retval;
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.len	= sizeof(addr_offset),
> +			.buf	= &addr_offset,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= len,
> +			.buf	= buf,
> +		},
> +	};
>
>   	mutex_lock(&rmi_i2c->page_mutex);
>
>   	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
>   		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> -		if (retval < 0)
> +		if (retval)
>   			goto exit;
>   	}
>
> -	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
> +	retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
> +	if (retval == sizeof(msgs))
> +		retval = 0; /* success */
> +	else if (retval >= 0)
> +		retval = -EIO;
>
> -	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += sizeof(txbuf);
> -	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> -	if (retval != sizeof(txbuf)) {
> -		xport->stats.tx_errs++;
> -		retval = (retval < 0) ? retval : -EIO;
> -		goto exit;
> -	}
> +exit:
> +	dev_dbg(&client->dev,
> +		"read %zd bytes at %#06x: %d (%*ph)\n",
> +		len, addr, retval, (int)len, buf);
>
>   	xport->stats.rx_count++;
> -	xport->stats.rx_bytes += len;
> -
> -	retval = i2c_master_recv(client, buf, len);
> -	if (retval < 0)
> +	if (retval)
>   		xport->stats.rx_errs++;
>   	else
> -		dev_dbg(&client->dev,
> -			"read %zd bytes at %#06x: %*ph\n",
> -			len, addr, (int)len, buf);
> +		xport->stats.rx_bytes += len;
>
> -exit:
>   	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()

[Christopher Heiny]

On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of using 2 separate transactions when reading from the device let's
> use i2c_transfer. Because we now have single point of failure I had to
> change how we collect statistics. I elected to drop control data from the
> stats and only track number of bytes read/written for the device data.
>
> Also, since we are not prepared to deal with short reads and writes change
> read_block_data and write_block_data to indicate error if we detect short
> transfers.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++--------------------
>   1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index c176218..51f5bc8 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -57,22 +57,17 @@ struct rmi_i2c_xport {
>    */
>   static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
>   {
> -	struct rmi_transport_dev *xport = &rmi_i2c->xport;
>   	struct i2c_client *client = rmi_i2c->client;
>   	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
>   	int retval;
>
> -	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> -			txbuf[0], txbuf[1]);
> -	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += sizeof(txbuf);
>   	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
>   	if (retval != sizeof(txbuf)) {
> -		xport->stats.tx_errs++;
>   		dev_err(&client->dev,
>   			"%s: set page failed: %d.", __func__, retval);
>   		return (retval < 0) ? retval : -EIO;
>   	}
> +
>   	rmi_i2c->page = page;
>   	return 0;
>   }
> @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
>   	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
>   		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> -		if (retval < 0)
> +		if (retval)
>   			goto exit;
>   	}
>
> +	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> +	if (retval == tx_size)
> +		retval = 0;
> +	else if (retval >= 0)
> +		retval = -EIO;
> +
> +exit:
>   	dev_dbg(&client->dev,
> -		"writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
> +		"write %zd bytes at %#06x: %d (%*ph)\n",
> +		len, addr, retval, (int)len, buf);
>
>   	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += tx_size;
> -	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> -	if (retval < 0)
> +	if (retval)
>   		xport->stats.tx_errs++;
>   	else
> -		retval--; /* don't count the address byte */
> +		xport->stats.tx_bytes += len;
>
> -exit:
>   	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
> @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>   	struct rmi_i2c_xport *rmi_i2c =
>   		container_of(xport, struct rmi_i2c_xport, xport);
>   	struct i2c_client *client = rmi_i2c->client;
> -	u8 txbuf[1] = {addr & 0xff};
> +	u8 addr_offset = addr & 0xff;
>   	int retval;
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.len	= sizeof(addr_offset),
> +			.buf	= &addr_offset,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= len,
> +			.buf	= buf,
> +		},
> +	};
>
>   	mutex_lock(&rmi_i2c->page_mutex);
>
>   	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
>   		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> -		if (retval < 0)
> +		if (retval)
>   			goto exit;
>   	}
>
> -	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
> +	retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
> +	if (retval == sizeof(msgs))

I think this should be:
	retval = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
	if (retval == ARRAY_SIZE(msgs))
At least, that change resolved some random misbehaviors, including 
kernel panics.


> +		retval = 0; /* success */
> +	else if (retval >= 0)
> +		retval = -EIO;
>
> -	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += sizeof(txbuf);
> -	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> -	if (retval != sizeof(txbuf)) {
> -		xport->stats.tx_errs++;
> -		retval = (retval < 0) ? retval : -EIO;
> -		goto exit;
> -	}
> +exit:
> +	dev_dbg(&client->dev,
> +		"read %zd bytes at %#06x: %d (%*ph)\n",
> +		len, addr, retval, (int)len, buf);
>
>   	xport->stats.rx_count++;
> -	xport->stats.rx_bytes += len;
> -
> -	retval = i2c_master_recv(client, buf, len);
> -	if (retval < 0)
> +	if (retval)
>   		xport->stats.rx_errs++;
>   	else
> -		dev_dbg(&client->dev,
> -			"read %zd bytes at %#06x: %*ph\n",
> -			len, addr, (int)len, buf);
> +		xport->stats.rx_bytes += len;
>
> -exit:
>   	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()

[Dmitry Torokhov]

On Tue, Jan 14, 2014 at 12:26:47AM -0800, Christopher Heiny wrote:
> On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> >
> >-	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
> >+	retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
> >+	if (retval == sizeof(msgs))
> 
> I think this should be:
> 	retval = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> 	if (retval == ARRAY_SIZE(msgs))
> At least, that change resolved some random misbehaviors, including
> kernel panics.

You are absolutely right, I just committed a fix for that.

Thanks.

-- 
Dmitry