[PATCH 1/4] Input: synaptics-rmi4 - fix handling failures from rmi_enable_sensor

[PATCH 1/4] Input: synaptics-rmi4 - fix handling failures from rmi_enable_sensor

[Dmitry Torokhov]

If rmi_enable_sensor() fails in rmi_driver_probe(), we should not return
immediately, but disable IRQs and tear down function list.

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

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index d9cfe4ec93fa..daba87c7e707 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -1229,16 +1229,21 @@ static int rmi_driver_probe(struct device *dev)
 	if (retval < 0)
 		goto err_destroy_functions;
 
-	if (data->f01_container->dev.driver)
+	if (data->f01_container->dev.driver) {
 		/* Driver already bound, so enable ATTN now. */
-		return rmi_enable_sensor(rmi_dev);
+		retval = rmi_enable_sensor(rmi_dev);
+		if (retval)
+			goto err_disable_irq;
+	}
 
 	return 0;
 
+err_disable_irq:
+	rmi_disable_irq(rmi_dev, false);
 err_destroy_functions:
 	rmi_free_function_list(rmi_dev);
 err:
-	return retval < 0 ? retval : 0;
+	return retval;
 }
 
 static struct rmi_driver rmi_physical_driver = {
-- 
2.12.1.578.ge9c3154ca4-goog

[PATCH 2/4] Input: synaptics-rmi4 - fix endianness issue in SMBus transport

[Dmitry Torokhov]

The mapping table holds address in LE form, so we should convert it
to CPU when comparing it.

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

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 76752555d809..c8bf49686460 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -89,17 +89,16 @@ static int rmi_smb_get_command_code(struct rmi_transport_dev *xport,
 
 	mutex_lock(&rmi_smb->mappingtable_mutex);
 	for (i = 0; i < RMI_SMB2_MAP_SIZE; i++) {
-		if (rmi_smb->mapping_table[i].rmiaddr == rmiaddr) {
+		struct mapping_table_entry *entry = &rmi_smb->mapping_table[i];
+		if (le16_to_cpu(entry->rmiaddr) == rmiaddr) {
 			if (isread) {
-				if (rmi_smb->mapping_table[i].readcount
-							== bytecount) {
+				if (entry->readcount == bytecount) {
 					*commandcode = i;
 					retval = 0;
 					goto exit;
 				}
 			} else {
-				if (rmi_smb->mapping_table[i].flags &
-							RMI_SMB2_MAP_FLAGS_WE) {
+				if (entry->flags & RMI_SMB2_MAP_FLAGS_WE) {
 					*commandcode = i;
 					retval = 0;
 					goto exit;
-- 
2.12.1.578.ge9c3154ca4-goog

Re: [PATCH 2/4] Input: synaptics-rmi4 - fix endianness issue in SMBus transport

[Benjamin Tissoires]

On Mar 24 2017 or thereabouts, Dmitry Torokhov wrote:
> The mapping table holds address in LE form, so we should convert it
> to CPU when comparing it.

Are you sure about that?

>From what I can read:
rmiaddr is set by rmi_smb_get_command_code(), and it's forwarded
directly from the caller to the table. When we write the data to the
device, we use cpu_to_le16() so rmiaddr is used as le16, but always
stored as CPU type.

> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/rmi4/rmi_smbus.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 76752555d809..c8bf49686460 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -89,17 +89,16 @@ static int rmi_smb_get_command_code(struct rmi_transport_dev *xport,
>  
>  	mutex_lock(&rmi_smb->mappingtable_mutex);
>  	for (i = 0; i < RMI_SMB2_MAP_SIZE; i++) {
> -		if (rmi_smb->mapping_table[i].rmiaddr == rmiaddr) {
> +		struct mapping_table_entry *entry = &rmi_smb->mapping_table[i];

In case you want to keep the temporary variable here, I believe
checkpatch would complain about a missing empty line here (just
nitpicking).

Cheers,
Benjamin

> +		if (le16_to_cpu(entry->rmiaddr) == rmiaddr) {
>  			if (isread) {
> -				if (rmi_smb->mapping_table[i].readcount
> -							== bytecount) {
> +				if (entry->readcount == bytecount) {
>  					*commandcode = i;
>  					retval = 0;
>  					goto exit;
>  				}
>  			} else {
> -				if (rmi_smb->mapping_table[i].flags &
> -							RMI_SMB2_MAP_FLAGS_WE) {
> +				if (entry->flags & RMI_SMB2_MAP_FLAGS_WE) {
>  					*commandcode = i;
>  					retval = 0;
>  					goto exit;
> -- 
> 2.12.1.578.ge9c3154ca4-goog
> 

[PATCH 3/4] Input: synaptics-rmi4 - cleanup SMbus mapping handling

[Dmitry Torokhov]

There is no reason to copy structures field-by-filed when we can copy
eements at once.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_smbus.c | 43 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index c8bf49686460..724643bf31a6 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -83,62 +83,55 @@ static int rmi_smb_get_command_code(struct rmi_transport_dev *xport,
 {
 	struct rmi_smb_xport *rmi_smb =
 		container_of(xport, struct rmi_smb_xport, xport);
+	struct mapping_table_entry new_map;
 	int i;
-	int retval;
-	struct mapping_table_entry mapping_data[1];
+	int retval = 0;
 
 	mutex_lock(&rmi_smb->mappingtable_mutex);
+
 	for (i = 0; i < RMI_SMB2_MAP_SIZE; i++) {
 		struct mapping_table_entry *entry = &rmi_smb->mapping_table[i];
 		if (le16_to_cpu(entry->rmiaddr) == rmiaddr) {
 			if (isread) {
-				if (entry->readcount == bytecount) {
-					*commandcode = i;
-					retval = 0;
+				if (entry->readcount == bytecount)
 					goto exit;
-				}
 			} else {
 				if (entry->flags & RMI_SMB2_MAP_FLAGS_WE) {
-					*commandcode = i;
-					retval = 0;
 					goto exit;
 				}
 			}
 		}
 	}
+
 	i = rmi_smb->table_index;
 	rmi_smb->table_index = (i + 1) % RMI_SMB2_MAP_SIZE;
 
 	/* constructs mapping table data entry. 4 bytes each entry */
-	memset(mapping_data, 0, sizeof(mapping_data));
-
-	mapping_data[0].rmiaddr = cpu_to_le16(rmiaddr);
-	mapping_data[0].readcount = bytecount;
-	mapping_data[0].flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
-
-	retval = smb_block_write(xport, i + 0x80, mapping_data,
-				 sizeof(mapping_data));
+	memset(&new_map, 0, sizeof(new_map));
+	new_map.rmiaddr = cpu_to_le16(rmiaddr);
+	new_map.readcount = bytecount;
+	new_map.flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
 
+	retval = smb_block_write(xport, i + 0x80, &new_map, sizeof(new_map));
 	if (retval < 0) {
 		/*
 		 * if not written to device mapping table
 		 * clear the driver mapping table records
 		 */
-		rmi_smb->mapping_table[i].rmiaddr = 0x0000;
-		rmi_smb->mapping_table[i].readcount = 0;
-		rmi_smb->mapping_table[i].flags = 0;
-		goto exit;
+		memset(&new_map, 0, sizeof(new_map));
 	}
+
 	/* save to the driver level mapping table */
-	rmi_smb->mapping_table[i].rmiaddr = rmiaddr;
-	rmi_smb->mapping_table[i].readcount = bytecount;
-	rmi_smb->mapping_table[i].flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
-	*commandcode = i;
+	rmi_smb->mapping_table[i] = new_map;
 
 exit:
 	mutex_unlock(&rmi_smb->mappingtable_mutex);
 
-	return retval;
+	if (retval < 0)
+		return retval;
+
+	*commandcode = i;
+	return 0;
 }
 
 static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
-- 
2.12.1.578.ge9c3154ca4-goog

Re: [PATCH 3/4] Input: synaptics-rmi4 - cleanup SMbus mapping handling

[Benjamin Tissoires]

On Mar 24 2017 or thereabouts, Dmitry Torokhov wrote:
> There is no reason to copy structures field-by-filed when we can copy
> eements at once.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

OK, and now I realize why 2/4 was needed. The current code was just
plain wrong because rmiaddr was tagged as __le16. This fixes the storing
issue.

This one (and the previous then):
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/input/rmi4/rmi_smbus.c | 43 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index c8bf49686460..724643bf31a6 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -83,62 +83,55 @@ static int rmi_smb_get_command_code(struct rmi_transport_dev *xport,
>  {
>  	struct rmi_smb_xport *rmi_smb =
>  		container_of(xport, struct rmi_smb_xport, xport);
> +	struct mapping_table_entry new_map;
>  	int i;
> -	int retval;
> -	struct mapping_table_entry mapping_data[1];
> +	int retval = 0;
>  
>  	mutex_lock(&rmi_smb->mappingtable_mutex);
> +
>  	for (i = 0; i < RMI_SMB2_MAP_SIZE; i++) {
>  		struct mapping_table_entry *entry = &rmi_smb->mapping_table[i];
>  		if (le16_to_cpu(entry->rmiaddr) == rmiaddr) {
>  			if (isread) {
> -				if (entry->readcount == bytecount) {
> -					*commandcode = i;
> -					retval = 0;
> +				if (entry->readcount == bytecount)
>  					goto exit;
> -				}
>  			} else {
>  				if (entry->flags & RMI_SMB2_MAP_FLAGS_WE) {
> -					*commandcode = i;
> -					retval = 0;
>  					goto exit;
>  				}
>  			}
>  		}
>  	}
> +
>  	i = rmi_smb->table_index;
>  	rmi_smb->table_index = (i + 1) % RMI_SMB2_MAP_SIZE;
>  
>  	/* constructs mapping table data entry. 4 bytes each entry */
> -	memset(mapping_data, 0, sizeof(mapping_data));
> -
> -	mapping_data[0].rmiaddr = cpu_to_le16(rmiaddr);
> -	mapping_data[0].readcount = bytecount;
> -	mapping_data[0].flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
> -
> -	retval = smb_block_write(xport, i + 0x80, mapping_data,
> -				 sizeof(mapping_data));
> +	memset(&new_map, 0, sizeof(new_map));
> +	new_map.rmiaddr = cpu_to_le16(rmiaddr);
> +	new_map.readcount = bytecount;
> +	new_map.flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
>  
> +	retval = smb_block_write(xport, i + 0x80, &new_map, sizeof(new_map));
>  	if (retval < 0) {
>  		/*
>  		 * if not written to device mapping table
>  		 * clear the driver mapping table records
>  		 */
> -		rmi_smb->mapping_table[i].rmiaddr = 0x0000;
> -		rmi_smb->mapping_table[i].readcount = 0;
> -		rmi_smb->mapping_table[i].flags = 0;
> -		goto exit;
> +		memset(&new_map, 0, sizeof(new_map));
>  	}
> +
>  	/* save to the driver level mapping table */
> -	rmi_smb->mapping_table[i].rmiaddr = rmiaddr;
> -	rmi_smb->mapping_table[i].readcount = bytecount;
> -	rmi_smb->mapping_table[i].flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
> -	*commandcode = i;
> +	rmi_smb->mapping_table[i] = new_map;
>  
>  exit:
>  	mutex_unlock(&rmi_smb->mappingtable_mutex);
>  
> -	return retval;
> +	if (retval < 0)
> +		return retval;
> +
> +	*commandcode = i;
> +	return 0;
>  }
>  
>  static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
> -- 
> 2.12.1.578.ge9c3154ca4-goog
> 

[PATCH 4/4] Input: synaptics-rmi4 - when registering sensors do not call them "drivers"

[Dmitry Torokhov]

We are not registering drivers, but transport devices (AKA sensors), so
let's call them that.

Also let's rename "retval" to "error" in probe() functions as the variables
are used to store error codes.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_i2c.c   | 51 +++++++++++++++++++++---------------------
 drivers/input/rmi4/rmi_smbus.c | 43 +++++++++++++++++------------------
 drivers/input/rmi4/rmi_spi.c   | 44 +++++++++++++++++++-----------------
 3 files changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index 082306d7c207..e28663ef9e5a 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -204,7 +204,7 @@ static int rmi_i2c_probe(struct i2c_client *client,
 	struct rmi_device_platform_data *client_pdata =
 					dev_get_platdata(&client->dev);
 	struct rmi_i2c_xport *rmi_i2c;
-	int retval;
+	int error;
 
 	rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport),
 				GFP_KERNEL);
@@ -220,30 +220,31 @@ static int rmi_i2c_probe(struct i2c_client *client,
 
 	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s.\n",
 			dev_name(&client->dev));
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_err(&client->dev,
-			"adapter does not support required functionality.\n");
+			"adapter does not support required functionality\n");
 		return -ENODEV;
 	}
 
 	rmi_i2c->supplies[0].supply = "vdd";
 	rmi_i2c->supplies[1].supply = "vio";
-	retval = devm_regulator_bulk_get(&client->dev,
+	error = devm_regulator_bulk_get(&client->dev,
 					 ARRAY_SIZE(rmi_i2c->supplies),
 					 rmi_i2c->supplies);
-	if (retval < 0)
-		return retval;
+	if (error < 0)
+		return error;
 
-	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
+	error = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
 				       rmi_i2c->supplies);
-	if (retval < 0)
-		return retval;
+	if (error < 0)
+		return error;
 
-	retval = devm_add_action_or_reset(&client->dev,
+	error = devm_add_action_or_reset(&client->dev,
 					  rmi_i2c_regulator_bulk_disable,
 					  rmi_i2c);
-	if (retval)
-		return retval;
+	if (error)
+		return error;
 
 	of_property_read_u32(client->dev.of_node, "syna,startup-delay-ms",
 			     &rmi_i2c->startup_delay);
@@ -263,26 +264,26 @@ static int rmi_i2c_probe(struct i2c_client *client,
 	 * 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(rmi_i2c, 0);
-	if (retval) {
-		dev_err(&client->dev, "Failed to set page select to 0.\n");
-		return retval;
+	error = rmi_set_page(rmi_i2c, 0);
+	if (error) {
+		dev_err(&client->dev, "Failed to set page select to 0\n");
+		return error;
 	}
 
-	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);
-		return retval;
+	dev_info(&client->dev, "registering I2C-connected sensor\n");
+
+	error = rmi_register_transport_device(&rmi_i2c->xport);
+	if (error) {
+		dev_err(&client->dev, "failed to register sensor: %d\n", error);
+		return error;
 	}
-	retval = devm_add_action_or_reset(&client->dev,
+
+	error = devm_add_action_or_reset(&client->dev,
 					  rmi_i2c_unregister_transport,
 					  rmi_i2c);
-	if (retval)
-		return retval;
+	if (error)
+		return error;
 
-	dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
-			client->addr);
 	return 0;
 }
 
diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 724643bf31a6..e84400301973 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -53,6 +53,7 @@ static int rmi_smb_get_version(struct rmi_smb_xport *rmi_smb)
 		dev_err(&client->dev, "failed to get SMBus version number!\n");
 		return retval;
 	}
+
 	return retval + 1;
 }
 
@@ -274,19 +275,24 @@ static int rmi_smb_probe(struct i2c_client *client,
 {
 	struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct rmi_smb_xport *rmi_smb;
-	int retval;
 	int smbus_version;
+	int error;
+
+	if (!pdata) {
+		dev_err(&client->dev, "no platform data, aborting\n");
+		return -ENOMEM;
+	}
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_READ_BLOCK_DATA |
 				     I2C_FUNC_SMBUS_HOST_NOTIFY)) {
 		dev_err(&client->dev,
-			"adapter does not support required functionality.\n");
+			"adapter does not support required functionality\n");
 		return -ENODEV;
 	}
 
 	if (client->irq <= 0) {
-		dev_err(&client->dev, "no IRQ provided, giving up.\n");
+		dev_err(&client->dev, "no IRQ provided, giving up\n");
 		return client->irq ? client->irq : -ENODEV;
 	}
 
@@ -295,12 +301,7 @@ static int rmi_smb_probe(struct i2c_client *client,
 	if (!rmi_smb)
 		return -ENOMEM;
 
-	if (!pdata) {
-		dev_err(&client->dev, "no platform data, aborting\n");
-		return -ENOMEM;
-	}
-
-	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s.\n",
+	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s\n",
 		dev_name(&client->dev));
 
 	rmi_smb->client = client;
@@ -313,34 +314,30 @@ static int rmi_smb_probe(struct i2c_client *client,
 	rmi_smb->xport.proto_name = "smb2";
 	rmi_smb->xport.ops = &rmi_smb_ops;
 
-	retval = rmi_smb_get_version(rmi_smb);
-	if (retval < 0)
-		return retval;
+	smbus_version = rmi_smb_get_version(rmi_smb);
+	if (smbus_version < 0)
+		return smbus_version;
 
-	smbus_version = retval;
 	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d",
 		smbus_version);
 
 	if (smbus_version != 2) {
-		dev_err(&client->dev, "Unrecognized SMB version %d.\n",
+		dev_err(&client->dev, "Unrecognized SMB version %d\n",
 				smbus_version);
 		return -ENODEV;
 	}
 
 	i2c_set_clientdata(client, rmi_smb);
 
-	retval = rmi_register_transport_device(&rmi_smb->xport);
-	if (retval) {
-		dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
-			client->addr);
-		i2c_set_clientdata(client, NULL);
-		return retval;
+	dev_info(&client->dev, "registering SMbus-connected sensor\n");
+
+	error = rmi_register_transport_device(&rmi_smb->xport);
+	if (error) {
+		dev_err(&client->dev, "failed to register sensor: %d\n", error);
+		return error;
 	}
 
-	dev_info(&client->dev, "registered rmi smb driver at %#04x.\n",
-			client->addr);
 	return 0;
-
 }
 
 static int rmi_smb_remove(struct i2c_client *client)
diff --git a/drivers/input/rmi4/rmi_spi.c b/drivers/input/rmi4/rmi_spi.c
index 69548d7d1f10..d97a85907ed6 100644
--- a/drivers/input/rmi4/rmi_spi.c
+++ b/drivers/input/rmi4/rmi_spi.c
@@ -370,7 +370,7 @@ static int rmi_spi_probe(struct spi_device *spi)
 	struct rmi_spi_xport *rmi_spi;
 	struct rmi_device_platform_data *pdata;
 	struct rmi_device_platform_data *spi_pdata = spi->dev.platform_data;
-	int retval;
+	int error;
 
 	if (spi->master->flags & SPI_MASTER_HALF_DUPLEX)
 		return -EINVAL;
@@ -383,9 +383,9 @@ static int rmi_spi_probe(struct spi_device *spi)
 	pdata = &rmi_spi->xport.pdata;
 
 	if (spi->dev.of_node) {
-		retval = rmi_spi_of_probe(spi, pdata);
-		if (retval)
-			return retval;
+		error = rmi_spi_of_probe(spi, pdata);
+		if (error)
+			return error;
 	} else if (spi_pdata) {
 		*pdata = *spi_pdata;
 	}
@@ -396,10 +396,10 @@ static int rmi_spi_probe(struct spi_device *spi)
 	if (pdata->spi_data.mode)
 		spi->mode = pdata->spi_data.mode;
 
-	retval = spi_setup(spi);
-	if (retval < 0) {
+	error = spi_setup(spi);
+	if (error < 0) {
 		dev_err(&spi->dev, "spi_setup failed!\n");
-		return retval;
+		return error;
 	}
 
 	pdata->irq = spi->irq;
@@ -413,32 +413,34 @@ static int rmi_spi_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, rmi_spi);
 
-	retval = rmi_spi_manage_pools(rmi_spi, RMI_SPI_DEFAULT_XFER_BUF_SIZE);
-	if (retval)
-		return retval;
+	error = rmi_spi_manage_pools(rmi_spi, RMI_SPI_DEFAULT_XFER_BUF_SIZE);
+	if (error)
+		return error;
 
 	/*
 	 * 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(rmi_spi, 0);
-	if (retval) {
+	error = rmi_set_page(rmi_spi, 0);
+	if (error) {
 		dev_err(&spi->dev, "Failed to set page select to 0.\n");
-		return retval;
+		return error;
 	}
 
-	retval = rmi_register_transport_device(&rmi_spi->xport);
-	if (retval) {
-		dev_err(&spi->dev, "failed to register transport.\n");
-		return retval;
+	dev_info(&spi->dev, "registering SPI-connected sensor\n");
+
+	error = rmi_register_transport_device(&rmi_spi->xport);
+	if (error) {
+		dev_err(&spi->dev, "failed to register sensor: %d\n", error);
+		return error;
 	}
-	retval = devm_add_action_or_reset(&spi->dev,
+
+	error = devm_add_action_or_reset(&spi->dev,
 					  rmi_spi_unregister_transport,
 					  rmi_spi);
-	if (retval)
-		return retval;
+	if (error)
+		return error;
 
-	dev_info(&spi->dev, "registered RMI SPI driver\n");
 	return 0;
 }
 
-- 
2.12.1.578.ge9c3154ca4-goog

Re: [PATCH 4/4] Input: synaptics-rmi4 - when registering sensors do not call them "drivers"

[Benjamin Tissoires]

On Mar 24 2017 or thereabouts, Dmitry Torokhov wrote:
> We are not registering drivers, but transport devices (AKA sensors), so
> let's call them that.
> 
> Also let's rename "retval" to "error" in probe() functions as the variables
> are used to store error codes.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Looks good to me:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/input/rmi4/rmi_i2c.c   | 51 +++++++++++++++++++++---------------------
>  drivers/input/rmi4/rmi_smbus.c | 43 +++++++++++++++++------------------
>  drivers/input/rmi4/rmi_spi.c   | 44 +++++++++++++++++++-----------------
>  3 files changed, 69 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 082306d7c207..e28663ef9e5a 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -204,7 +204,7 @@ static int rmi_i2c_probe(struct i2c_client *client,
>  	struct rmi_device_platform_data *client_pdata =
>  					dev_get_platdata(&client->dev);
>  	struct rmi_i2c_xport *rmi_i2c;
> -	int retval;
> +	int error;
>  
>  	rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport),
>  				GFP_KERNEL);
> @@ -220,30 +220,31 @@ static int rmi_i2c_probe(struct i2c_client *client,
>  
>  	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s.\n",
>  			dev_name(&client->dev));
> +
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_err(&client->dev,
> -			"adapter does not support required functionality.\n");
> +			"adapter does not support required functionality\n");
>  		return -ENODEV;
>  	}
>  
>  	rmi_i2c->supplies[0].supply = "vdd";
>  	rmi_i2c->supplies[1].supply = "vio";
> -	retval = devm_regulator_bulk_get(&client->dev,
> +	error = devm_regulator_bulk_get(&client->dev,
>  					 ARRAY_SIZE(rmi_i2c->supplies),
>  					 rmi_i2c->supplies);
> -	if (retval < 0)
> -		return retval;
> +	if (error < 0)
> +		return error;
>  
> -	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> +	error = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
>  				       rmi_i2c->supplies);
> -	if (retval < 0)
> -		return retval;
> +	if (error < 0)
> +		return error;
>  
> -	retval = devm_add_action_or_reset(&client->dev,
> +	error = devm_add_action_or_reset(&client->dev,
>  					  rmi_i2c_regulator_bulk_disable,
>  					  rmi_i2c);
> -	if (retval)
> -		return retval;
> +	if (error)
> +		return error;
>  
>  	of_property_read_u32(client->dev.of_node, "syna,startup-delay-ms",
>  			     &rmi_i2c->startup_delay);
> @@ -263,26 +264,26 @@ static int rmi_i2c_probe(struct i2c_client *client,
>  	 * 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(rmi_i2c, 0);
> -	if (retval) {
> -		dev_err(&client->dev, "Failed to set page select to 0.\n");
> -		return retval;
> +	error = rmi_set_page(rmi_i2c, 0);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to set page select to 0\n");
> +		return error;
>  	}
>  
> -	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);
> -		return retval;
> +	dev_info(&client->dev, "registering I2C-connected sensor\n");
> +
> +	error = rmi_register_transport_device(&rmi_i2c->xport);
> +	if (error) {
> +		dev_err(&client->dev, "failed to register sensor: %d\n", error);
> +		return error;
>  	}
> -	retval = devm_add_action_or_reset(&client->dev,
> +
> +	error = devm_add_action_or_reset(&client->dev,
>  					  rmi_i2c_unregister_transport,
>  					  rmi_i2c);
> -	if (retval)
> -		return retval;
> +	if (error)
> +		return error;
>  
> -	dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> -			client->addr);
>  	return 0;
>  }
>  
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 724643bf31a6..e84400301973 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -53,6 +53,7 @@ static int rmi_smb_get_version(struct rmi_smb_xport *rmi_smb)
>  		dev_err(&client->dev, "failed to get SMBus version number!\n");
>  		return retval;
>  	}
> +
>  	return retval + 1;
>  }
>  
> @@ -274,19 +275,24 @@ static int rmi_smb_probe(struct i2c_client *client,
>  {
>  	struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct rmi_smb_xport *rmi_smb;
> -	int retval;
>  	int smbus_version;
> +	int error;
> +
> +	if (!pdata) {
> +		dev_err(&client->dev, "no platform data, aborting\n");
> +		return -ENOMEM;
> +	}
>  
>  	if (!i2c_check_functionality(client->adapter,
>  				     I2C_FUNC_SMBUS_READ_BLOCK_DATA |
>  				     I2C_FUNC_SMBUS_HOST_NOTIFY)) {
>  		dev_err(&client->dev,
> -			"adapter does not support required functionality.\n");
> +			"adapter does not support required functionality\n");
>  		return -ENODEV;
>  	}
>  
>  	if (client->irq <= 0) {
> -		dev_err(&client->dev, "no IRQ provided, giving up.\n");
> +		dev_err(&client->dev, "no IRQ provided, giving up\n");
>  		return client->irq ? client->irq : -ENODEV;
>  	}
>  
> @@ -295,12 +301,7 @@ static int rmi_smb_probe(struct i2c_client *client,
>  	if (!rmi_smb)
>  		return -ENOMEM;
>  
> -	if (!pdata) {
> -		dev_err(&client->dev, "no platform data, aborting\n");
> -		return -ENOMEM;
> -	}
> -
> -	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s.\n",
> +	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s\n",
>  		dev_name(&client->dev));
>  
>  	rmi_smb->client = client;
> @@ -313,34 +314,30 @@ static int rmi_smb_probe(struct i2c_client *client,
>  	rmi_smb->xport.proto_name = "smb2";
>  	rmi_smb->xport.ops = &rmi_smb_ops;
>  
> -	retval = rmi_smb_get_version(rmi_smb);
> -	if (retval < 0)
> -		return retval;
> +	smbus_version = rmi_smb_get_version(rmi_smb);
> +	if (smbus_version < 0)
> +		return smbus_version;
>  
> -	smbus_version = retval;
>  	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d",
>  		smbus_version);
>  
>  	if (smbus_version != 2) {
> -		dev_err(&client->dev, "Unrecognized SMB version %d.\n",
> +		dev_err(&client->dev, "Unrecognized SMB version %d\n",
>  				smbus_version);
>  		return -ENODEV;
>  	}
>  
>  	i2c_set_clientdata(client, rmi_smb);
>  
> -	retval = rmi_register_transport_device(&rmi_smb->xport);
> -	if (retval) {
> -		dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> -			client->addr);
> -		i2c_set_clientdata(client, NULL);
> -		return retval;
> +	dev_info(&client->dev, "registering SMbus-connected sensor\n");
> +
> +	error = rmi_register_transport_device(&rmi_smb->xport);
> +	if (error) {
> +		dev_err(&client->dev, "failed to register sensor: %d\n", error);
> +		return error;
>  	}
>  
> -	dev_info(&client->dev, "registered rmi smb driver at %#04x.\n",
> -			client->addr);
>  	return 0;
> -
>  }
>  
>  static int rmi_smb_remove(struct i2c_client *client)
> diff --git a/drivers/input/rmi4/rmi_spi.c b/drivers/input/rmi4/rmi_spi.c
> index 69548d7d1f10..d97a85907ed6 100644
> --- a/drivers/input/rmi4/rmi_spi.c
> +++ b/drivers/input/rmi4/rmi_spi.c
> @@ -370,7 +370,7 @@ static int rmi_spi_probe(struct spi_device *spi)
>  	struct rmi_spi_xport *rmi_spi;
>  	struct rmi_device_platform_data *pdata;
>  	struct rmi_device_platform_data *spi_pdata = spi->dev.platform_data;
> -	int retval;
> +	int error;
>  
>  	if (spi->master->flags & SPI_MASTER_HALF_DUPLEX)
>  		return -EINVAL;
> @@ -383,9 +383,9 @@ static int rmi_spi_probe(struct spi_device *spi)
>  	pdata = &rmi_spi->xport.pdata;
>  
>  	if (spi->dev.of_node) {
> -		retval = rmi_spi_of_probe(spi, pdata);
> -		if (retval)
> -			return retval;
> +		error = rmi_spi_of_probe(spi, pdata);
> +		if (error)
> +			return error;
>  	} else if (spi_pdata) {
>  		*pdata = *spi_pdata;
>  	}
> @@ -396,10 +396,10 @@ static int rmi_spi_probe(struct spi_device *spi)
>  	if (pdata->spi_data.mode)
>  		spi->mode = pdata->spi_data.mode;
>  
> -	retval = spi_setup(spi);
> -	if (retval < 0) {
> +	error = spi_setup(spi);
> +	if (error < 0) {
>  		dev_err(&spi->dev, "spi_setup failed!\n");
> -		return retval;
> +		return error;
>  	}
>  
>  	pdata->irq = spi->irq;
> @@ -413,32 +413,34 @@ static int rmi_spi_probe(struct spi_device *spi)
>  
>  	spi_set_drvdata(spi, rmi_spi);
>  
> -	retval = rmi_spi_manage_pools(rmi_spi, RMI_SPI_DEFAULT_XFER_BUF_SIZE);
> -	if (retval)
> -		return retval;
> +	error = rmi_spi_manage_pools(rmi_spi, RMI_SPI_DEFAULT_XFER_BUF_SIZE);
> +	if (error)
> +		return error;
>  
>  	/*
>  	 * 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(rmi_spi, 0);
> -	if (retval) {
> +	error = rmi_set_page(rmi_spi, 0);
> +	if (error) {
>  		dev_err(&spi->dev, "Failed to set page select to 0.\n");
> -		return retval;
> +		return error;
>  	}
>  
> -	retval = rmi_register_transport_device(&rmi_spi->xport);
> -	if (retval) {
> -		dev_err(&spi->dev, "failed to register transport.\n");
> -		return retval;
> +	dev_info(&spi->dev, "registering SPI-connected sensor\n");
> +
> +	error = rmi_register_transport_device(&rmi_spi->xport);
> +	if (error) {
> +		dev_err(&spi->dev, "failed to register sensor: %d\n", error);
> +		return error;
>  	}
> -	retval = devm_add_action_or_reset(&spi->dev,
> +
> +	error = devm_add_action_or_reset(&spi->dev,
>  					  rmi_spi_unregister_transport,
>  					  rmi_spi);
> -	if (retval)
> -		return retval;
> +	if (error)
> +		return error;
>  
> -	dev_info(&spi->dev, "registered RMI SPI driver\n");
>  	return 0;
>  }
>  
> -- 
> 2.12.1.578.ge9c3154ca4-goog
> 

Re: [PATCH 1/4] Input: synaptics-rmi4 - fix handling failures from rmi_enable_sensor

[Benjamin Tissoires]

On Mar 24 2017 or thereabouts, Dmitry Torokhov wrote:
> If rmi_enable_sensor() fails in rmi_driver_probe(), we should not return
> immediately, but disable IRQs and tear down function list.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/input/rmi4/rmi_driver.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index d9cfe4ec93fa..daba87c7e707 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1229,16 +1229,21 @@ static int rmi_driver_probe(struct device *dev)
>  	if (retval < 0)
>  		goto err_destroy_functions;
>  
> -	if (data->f01_container->dev.driver)
> +	if (data->f01_container->dev.driver) {
>  		/* Driver already bound, so enable ATTN now. */
> -		return rmi_enable_sensor(rmi_dev);
> +		retval = rmi_enable_sensor(rmi_dev);
> +		if (retval)
> +			goto err_disable_irq;
> +	}
>  
>  	return 0;
>  
> +err_disable_irq:
> +	rmi_disable_irq(rmi_dev, false);
>  err_destroy_functions:
>  	rmi_free_function_list(rmi_dev);
>  err:
> -	return retval < 0 ? retval : 0;
> +	return retval;
>  }
>  
>  static struct rmi_driver rmi_physical_driver = {
> -- 
> 2.12.1.578.ge9c3154ca4-goog
>