[PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[Christopher Heiny]

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/rmi_f01.c |  96 ++-----------------------------------
 drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+), 92 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index ee5f4a1..41cb795 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -13,95 +13,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include "rmi_driver.h"
-
-#define RMI_PRODUCT_ID_LENGTH    10
-#define RMI_PRODUCT_INFO_LENGTH   2
-
-#define RMI_DATE_CODE_LENGTH      3
-
-#define PRODUCT_ID_OFFSET 0x10
-#define PRODUCT_INFO_OFFSET 0x1E
-
-
-/* Force a firmware reset of the sensor */
-#define RMI_F01_CMD_DEVICE_RESET	1
-
-/* Various F01_RMI_QueryX bits */
-
-#define RMI_F01_QRY1_CUSTOM_MAP		(1 << 0)
-#define RMI_F01_QRY1_NON_COMPLIANT	(1 << 1)
-#define RMI_F01_QRY1_HAS_LTS		(1 << 2)
-#define RMI_F01_QRY1_HAS_SENSOR_ID	(1 << 3)
-#define RMI_F01_QRY1_HAS_CHARGER_INP	(1 << 4)
-#define RMI_F01_QRY1_HAS_ADJ_DOZE	(1 << 5)
-#define RMI_F01_QRY1_HAS_ADJ_DOZE_HOFF	(1 << 6)
-#define RMI_F01_QRY1_HAS_PROPS_2	(1 << 7)
-
-#define RMI_F01_QRY5_YEAR_MASK		0x1f
-#define RMI_F01_QRY6_MONTH_MASK		0x0f
-#define RMI_F01_QRY7_DAY_MASK		0x1f
-
-#define RMI_F01_QRY2_PRODINFO_MASK	0x7f
-
-#define RMI_F01_BASIC_QUERY_LEN		21 /* From Query 00 through 20 */
-
-struct f01_basic_properties {
-	u8 manufacturer_id;
-	bool has_lts;
-	bool has_adjustable_doze;
-	bool has_adjustable_doze_holdoff;
-	char dom[11]; /* YYYY/MM/DD + '\0' */
-	u8 product_id[RMI_PRODUCT_ID_LENGTH + 1];
-	u16 productinfo;
-};
-
-/* F01 device status bits */
-
-/* Most recent device status event */
-#define RMI_F01_STATUS_CODE(status)		((status) & 0x0f)
-/* The device has lost its configuration for some reason. */
-#define RMI_F01_STATUS_UNCONFIGURED(status)	(!!((status) & 0x80))
-
-/* Control register bits */
-
-/*
- * Sleep mode controls power management on the device and affects all
- * functions of the device.
- */
-#define RMI_F01_CTRL0_SLEEP_MODE_MASK	0x03
-
-#define RMI_SLEEP_MODE_NORMAL		0x00
-#define RMI_SLEEP_MODE_SENSOR_SLEEP	0x01
-#define RMI_SLEEP_MODE_RESERVED0	0x02
-#define RMI_SLEEP_MODE_RESERVED1	0x03
-
-#define RMI_IS_VALID_SLEEPMODE(mode) \
-	(mode >= RMI_SLEEP_MODE_NORMAL && mode <= RMI_SLEEP_MODE_RESERVED1)
-
-/*
- * This bit disables whatever sleep mode may be selected by the sleep_mode
- * field and forces the device to run at full power without sleeping.
- */
-#define RMI_F01_CRTL0_NOSLEEP_BIT	(1 << 2)
-
-/*
- * When this bit is set, the touch controller employs a noise-filtering
- * algorithm designed for use with a connected battery charger.
- */
-#define RMI_F01_CRTL0_CHARGER_BIT	(1 << 5)
-
-/*
- * Sets the report rate for the device. The effect of this setting is
- * highly product dependent. Check the spec sheet for your particular
- * touch sensor.
- */
-#define RMI_F01_CRTL0_REPORTRATE_BIT	(1 << 6)
-
-/*
- * Written by the host as an indicator that the device has been
- * successfully configured.
- */
-#define RMI_F01_CRTL0_CONFIGURED_BIT	(1 << 7)
+#include "rmi_f01.h"
 
 /**
  * @ctrl0 - see the bit definitions above.
@@ -136,8 +48,7 @@ struct f01_data {
 	unsigned int num_of_irq_regs;
 };
 
-static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
-				   u16 query_base_addr,
+int rmi_f01_read_properties(struct rmi_device *rmi_dev, u16 query_base_addr,
 				   struct f01_basic_properties *props)
 {
 	u8 basic_query[RMI_F01_BASIC_QUERY_LEN];
@@ -180,7 +91,8 @@ static int rmi_f01_probe(struct rmi_function *fn)
 {
 	struct rmi_device *rmi_dev = fn->rmi_dev;
 	struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
-	const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
+	const struct rmi_device_platform_data *pdata =
+						rmi_get_platform_data(rmi_dev);
 	struct f01_data *f01;
 	int error;
 	u16 ctrl_base_addr = fn->fd.control_base_addr;
diff --git a/drivers/input/rmi4/rmi_f01.h b/drivers/input/rmi4/rmi_f01.h
new file mode 100644
index 0000000..9e5cc2b
--- /dev/null
+++ b/drivers/input/rmi4/rmi_f01.h
@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2014 Synaptics Incorporated
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#ifndef _RMI_F01_H
+#define _RMI_F01_H
+
+
+#define RMI_PRODUCT_ID_LENGTH    10
+#define RMI_PRODUCT_INFO_LENGTH   2
+
+#define RMI_DATE_CODE_LENGTH      3
+
+#define PRODUCT_ID_OFFSET 0x10
+#define PRODUCT_INFO_OFFSET 0x1E
+
+
+/* Force a firmware reset of the sensor */
+#define RMI_F01_CMD_DEVICE_RESET	1
+
+/* Various F01_RMI_QueryX bits */
+
+#define RMI_F01_QRY1_CUSTOM_MAP		(1 << 0)
+#define RMI_F01_QRY1_NON_COMPLIANT	(1 << 1)
+#define RMI_F01_QRY1_HAS_LTS		(1 << 2)
+#define RMI_F01_QRY1_HAS_SENSOR_ID	(1 << 3)
+#define RMI_F01_QRY1_HAS_CHARGER_INP	(1 << 4)
+#define RMI_F01_QRY1_HAS_ADJ_DOZE	(1 << 5)
+#define RMI_F01_QRY1_HAS_ADJ_DOZE_HOFF	(1 << 6)
+#define RMI_F01_QRY1_HAS_PROPS_2	(1 << 7)
+
+#define RMI_F01_QRY5_YEAR_MASK		0x1f
+#define RMI_F01_QRY6_MONTH_MASK		0x0f
+#define RMI_F01_QRY7_DAY_MASK		0x1f
+
+#define RMI_F01_QRY2_PRODINFO_MASK	0x7f
+
+#define RMI_F01_BASIC_QUERY_LEN		21 /* From Query 00 through 20 */
+
+struct f01_basic_properties {
+	u8 manufacturer_id;
+	bool has_lts;
+	bool has_adjustable_doze;
+	bool has_adjustable_doze_holdoff;
+	char dom[11]; /* YYYY/MM/DD + '\0' */
+	u8 product_id[RMI_PRODUCT_ID_LENGTH + 1];
+	u16 productinfo;
+};
+
+/* F01 device status bits */
+
+/* Most recent device status event */
+#define RMI_F01_STATUS_CODE(status)		((status) & 0x0f)
+/* The device has lost its configuration for some reason. */
+#define RMI_F01_STATUS_UNCONFIGURED(status)	(!!((status) & 0x80))
+
+/* Control register bits */
+
+/*
+ * Sleep mode controls power management on the device and affects all
+ * functions of the device.
+ */
+#define RMI_F01_CTRL0_SLEEP_MODE_MASK	0x03
+
+#define RMI_SLEEP_MODE_NORMAL		0x00
+#define RMI_SLEEP_MODE_SENSOR_SLEEP	0x01
+#define RMI_SLEEP_MODE_RESERVED0	0x02
+#define RMI_SLEEP_MODE_RESERVED1	0x03
+
+#define RMI_IS_VALID_SLEEPMODE(mode) \
+	(mode >= RMI_SLEEP_MODE_NORMAL && mode <= RMI_SLEEP_MODE_RESERVED1)
+
+/*
+ * This bit disables whatever sleep mode may be selected by the sleep_mode
+ * field and forces the device to run at full power without sleeping.
+ */
+#define RMI_F01_CRTL0_NOSLEEP_BIT	(1 << 2)
+
+/*
+ * When this bit is set, the touch controller employs a noise-filtering
+ * algorithm designed for use with a connected battery charger.
+ */
+#define RMI_F01_CRTL0_CHARGER_BIT	(1 << 5)
+
+/*
+ * Sets the report rate for the device. The effect of this setting is
+ * highly product dependent. Check the spec sheet for your particular
+ * touch sensor.
+ */
+#define RMI_F01_CRTL0_REPORTRATE_BIT	(1 << 6)
+
+/*
+ * Written by the host as an indicator that the device has been
+ * successfully configured.
+ */
+#define RMI_F01_CRTL0_CONFIGURED_BIT	(1 << 7)
+
+/** Read the F01 query registers and populate the basic_properties structure.
+ * @rmi_dev - the device to be queries.
+ * @query_base_addr - address of the start of the query registers.
+ * @props - pointer to the structure to be filled in.
+ */
+int rmi_f01_read_properties(struct rmi_device *rmi_dev, u16 query_base_addr,
+			    struct f01_basic_properties *props);
+
+#endif

[PATCH 02/05] input synaptics-rmi4: Add some additional F01 properties for the use of reflash.

[Christopher Heiny]

The reflash code will need access to the firmware build ID and
some other info.  As long as we're taking the scenic route to
the firmware build ID, we remember a number of other interesting
settings for use by diagnostic modules.

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/rmi_f01.c | 170 ++++++++++++++++++++++++++++++++++++++++---
 drivers/input/rmi4/rmi_f01.h | 101 +++++++++++++++++++++++++
 2 files changed, 262 insertions(+), 9 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 41cb795..8504865 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -48,11 +48,18 @@ struct f01_data {
 	unsigned int num_of_irq_regs;
 };
 
+#define PACKAGE_ID_BYTES 4
+#define BUILD_ID_BYTES 3
+
 int rmi_f01_read_properties(struct rmi_device *rmi_dev, u16 query_base_addr,
 				   struct f01_basic_properties *props)
 {
 	u8 basic_query[RMI_F01_BASIC_QUERY_LEN];
+	u8 info_buf[4];
 	int error;
+	int i;
+	u16 query_addr = query_base_addr;
+	u16 prod_info_addr;
 
 	error = rmi_read_block(rmi_dev, query_base_addr,
 			       basic_query, sizeof(basic_query));
@@ -70,19 +77,164 @@ int rmi_f01_read_properties(struct rmi_device *rmi_dev, u16 query_base_addr,
 			basic_query[1] & RMI_F01_QRY1_HAS_ADJ_DOZE;
 	props->has_adjustable_doze_holdoff =
 			basic_query[1] & RMI_F01_QRY1_HAS_ADJ_DOZE_HOFF;
+	props->has_query42 = basic_query[1] & RMI_F01_QRY1_HAS_PROPS_2;
+
+	props->productinfo =
+			((basic_query[2] & RMI_F01_QRY2_PRODINFO_MASK) << 7) |
+			(basic_query[3] & RMI_F01_QRY2_PRODINFO_MASK);
 
 	snprintf(props->dom, sizeof(props->dom), "20%02d/%02d/%02d",
 		 basic_query[5] & RMI_F01_QRY5_YEAR_MASK,
 		 basic_query[6] & RMI_F01_QRY6_MONTH_MASK,
 		 basic_query[7] & RMI_F01_QRY7_DAY_MASK);
 
-	memcpy(props->product_id, &basic_query[11],
-		RMI_PRODUCT_ID_LENGTH);
+	memcpy(props->product_id, &basic_query[11], RMI_PRODUCT_ID_LENGTH);
 	props->product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
+	query_addr += 11;
 
-	props->productinfo =
-			((basic_query[2] & RMI_F01_QRY2_PRODINFO_MASK) << 7) |
-			(basic_query[3] & RMI_F01_QRY2_PRODINFO_MASK);
+
+	error = rmi_read_block(rmi_dev, query_addr, props->product_id,
+				RMI_PRODUCT_ID_LENGTH);
+	if (error < 0) {
+		dev_err(&rmi_dev->dev, "Failed to read product ID.\n");
+		return error;
+	}
+	props->product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
+
+	/* We'll come back and use this later, depending on some other query
+	 * bits.
+	 */
+	prod_info_addr = query_addr + 6;
+
+	query_addr += RMI_PRODUCT_ID_LENGTH;
+	if (props->has_lts) {
+		error = rmi_read(rmi_dev, query_addr, info_buf);
+		if (error < 0) {
+			dev_err(&rmi_dev->dev, "Failed to read LTS info.\n");
+			return error;
+		}
+		props->slave_asic_rows = info_buf[0] &
+				RMI_F01_QRY21_SLAVE_ROWS_MASK;
+		props->slave_asic_columns = (info_buf[1] &
+				RMI_F01_QRY21_SLAVE_COLUMNS_MASK) >> 3;
+		query_addr++;
+	}
+
+	if (props->has_sensor_id) {
+		error = rmi_read(rmi_dev, query_addr, &props->sensor_id);
+		if (error < 0) {
+			dev_err(&rmi_dev->dev, "Failed to read sensor ID.\n");
+			return error;
+		}
+		query_addr++;
+	}
+
+	/* Maybe skip a block of undefined LTS registers. */
+	if (props->has_lts)
+		query_addr += RMI_F01_LTS_RESERVED_SIZE;
+
+	if (props->has_query42) {
+		error = rmi_read(rmi_dev, query_addr, info_buf);
+		if (error < 0) {
+			dev_err(&rmi_dev->dev, "Failed to read additional properties.\n");
+			return error;
+		}
+		props->has_ds4_queries = info_buf[0] &
+				RMI_F01_QRY42_DS4_QUERIES;
+		props->has_multi_physical = info_buf[0] &
+				RMI_F01_QRY42_MULTI_PHYS;
+		props->has_guest = info_buf[0] & RMI_F01_QRY42_GUEST;
+		props->has_swr = info_buf[0] & RMI_F01_QRY42_SWR;
+		props->has_nominal_report_rate = info_buf[0] &
+				RMI_F01_QRY42_NOMINAL_REPORT;
+		props->has_recalibration_interval = info_buf[0] &
+				RMI_F01_QRY42_RECAL_INTERVAL;
+		query_addr++;
+	}
+
+	if (props->has_ds4_queries) {
+		error = rmi_read(rmi_dev, query_addr, &props->ds4_query_length);
+		if (error < 0) {
+			dev_err(&rmi_dev->dev, "Failed to read DS4 query length size.\n");
+			return error;
+		}
+		query_addr++;
+	}
+
+	for (i = 1; i <= props->ds4_query_length; i++) {
+		u8 val;
+		error = rmi_read(rmi_dev, query_addr, &val);
+		query_addr++;
+		if (error < 0) {
+			dev_err(&rmi_dev->dev, "Failed to read F01_RMI_QUERY43.%02d, code: %d.\n",
+				i, error);
+			continue;
+		}
+		switch (i) {
+		case 1:
+			props->has_package_id_query = val &
+					RMI_F01_QRY43_01_PACKAGE_ID;
+			props->has_build_id_query = val &
+					RMI_F01_QRY43_01_BUILD_ID;
+			props->has_reset_query = val & RMI_F01_QRY43_01_RESET;
+			props->has_maskrev_query = val &
+					RMI_F01_QRY43_01_PACKAGE_ID;
+			break;
+		case 2:
+			props->has_i2c_control = val & RMI_F01_QRY43_02_I2C_CTL;
+			props->has_spi_control = val & RMI_F01_QRY43_02_SPI_CTL;
+			props->has_attn_control = val &
+					RMI_F01_QRY43_02_ATTN_CTL;
+			props->has_win8_vendor_info = val &
+					RMI_F01_QRY43_02_WIN8;
+			props->has_timestamp = val & RMI_F01_QRY43_02_TIMESTAMP;
+			break;
+		case 3:
+			props->has_tool_id_query = val &
+					RMI_F01_QRY43_03_TOOL_ID;
+			props->has_fw_revision_query = val &
+					RMI_F01_QRY43_03_FW_REVISION;
+			break;
+		default:
+			dev_warn(&rmi_dev->dev, "No handling for F01_RMI_QUERY43.%02d.\n",
+				 i);
+		}
+	}
+
+	/* If present, the ASIC package ID registers are overlaid on the
+	 * product ID. Go back to the right address (saved previously) and
+	 * read them.
+	 */
+	if (props->has_package_id_query) {
+		error = rmi_read_block(rmi_dev, prod_info_addr, info_buf,
+				PACKAGE_ID_BYTES);
+		if (error < 0)
+			dev_warn(&rmi_dev->dev, "Failed to read package ID.\n");
+		else {
+			u16 *val = (u16 *)info_buf;
+			props->package_id = le16_to_cpu(*val);
+			val = (u16 *)(info_buf + 2);
+			props->package_rev = le16_to_cpu(*val);
+		}
+	}
+	prod_info_addr++;
+
+	/* The firmware build id (if present) is similarly overlaid on product
+	 * ID registers.  Go back again and read that data.
+	 */
+	if (props->has_build_id_query) {
+		error = rmi_read_block(rmi_dev, prod_info_addr, info_buf,
+				BUILD_ID_BYTES);
+		if (error < 0)
+			dev_warn(&rmi_dev->dev, "Failed to read FW build ID.\n");
+		else {
+			u16 *val = (u16 *)info_buf;
+			props->build_id = le16_to_cpu(*val);
+			props->build_id += info_buf[2] * 65536;
+			dev_info(&rmi_dev->dev, "FW build ID: %#08x (%u).\n",
+				props->build_id, props->build_id);
+		}
+	}
 
 	return 0;
 }
@@ -164,10 +316,10 @@ static int rmi_f01_probe(struct rmi_function *fn)
 		dev_err(&fn->dev, "Failed to read F01 properties.\n");
 		return error;
 	}
-
-	dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
-		 f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
-		 f01->properties.product_id);
+	dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s, fw build: %d.\n",
+		 f01->properties.manufacturer_id == 1 ?
+							"Synaptics" : "unknown",
+		 f01->properties.product_id, f01->properties.build_id);
 
 	/* Advance to interrupt control registers, then skip over them. */
 	ctrl_base_addr++;
diff --git a/drivers/input/rmi4/rmi_f01.h b/drivers/input/rmi4/rmi_f01.h
index 9e5cc2b..af82fb7 100644
--- a/drivers/input/rmi4/rmi_f01.h
+++ b/drivers/input/rmi4/rmi_f01.h
@@ -21,6 +21,9 @@
 
 /* Force a firmware reset of the sensor */
 #define RMI_F01_CMD_DEVICE_RESET	1
+#define RMI_F01_DEFAULT_RESET_DELAY_MS	100
+
+#define F01_SERIALIZATION_SIZE 7
 
 /* Various F01_RMI_QueryX bits */
 
@@ -41,20 +44,118 @@
 
 #define RMI_F01_BASIC_QUERY_LEN		21 /* From Query 00 through 20 */
 
+#define RMI_F01_QRY21_SLAVE_ROWS_MASK   0x07
+#define RMI_F01_QRY21_SLAVE_COLUMNS_MASK 0x38
+
+#define RMI_F01_LTS_RESERVED_SIZE 19
+
+#define RMI_F01_QRY42_DS4_QUERIES	(1 << 0)
+#define RMI_F01_QRY42_MULTI_PHYS	(1 << 1)
+#define RMI_F01_QRY42_GUEST		(1 << 2)
+#define RMI_F01_QRY42_SWR		(1 << 3)
+#define RMI_F01_QRY42_NOMINAL_REPORT	(1 << 4)
+#define RMI_F01_QRY42_RECAL_INTERVAL	(1 << 5)
+
+#define RMI_F01_QRY43_01_PACKAGE_ID     (1 << 0)
+#define RMI_F01_QRY43_01_BUILD_ID       (1 << 1)
+#define RMI_F01_QRY43_01_RESET          (1 << 2)
+#define RMI_F01_QRY43_01_MASK_REV       (1 << 3)
+
+#define RMI_F01_QRY43_02_I2C_CTL	(1 << 0)
+#define RMI_F01_QRY43_02_SPI_CTL	(1 << 1)
+#define RMI_F01_QRY43_02_ATTN_CTL	(1 << 2)
+#define RMI_F01_QRY43_02_WIN8		(1 << 3)
+#define RMI_F01_QRY43_02_TIMESTAMP	(1 << 4)
+
+#define RMI_F01_QRY43_03_TOOL_ID	(1 << 0)
+#define RMI_F01_QRY43_03_FW_REVISION	(1 << 1)
+
+#define RMI_F01_QRY44_RST_ENABLED	(1 << 0)
+#define RMI_F01_QRY44_RST_POLARITY	(1 << 1)
+#define RMI_F01_QRY44_PULLUP_ENABLED	(1 << 2)
+#define RMI_F01_QRY44_RST_PIN_MASK	0xF0
+
+#define RMI_TOOL_ID_LENGTH		16
+#define RMI_FW_REVISION_LENGTH		16
+
 struct f01_basic_properties {
 	u8 manufacturer_id;
 	bool has_lts;
+	bool has_sensor_id;
 	bool has_adjustable_doze;
 	bool has_adjustable_doze_holdoff;
+	bool has_query42;
 	char dom[11]; /* YYYY/MM/DD + '\0' */
 	u8 product_id[RMI_PRODUCT_ID_LENGTH + 1];
 	u16 productinfo;
+	u16 package_id;
+	u16 package_rev;
+	u32 build_id;
+
+	/* These are meaningful only if has_lts is true. */
+	u8 slave_asic_rows;
+	u8 slave_asic_columns;
+
+	/* This is meaningful only if has_sensor_id is true. */
+	u8 sensor_id;
+
+	/* These are meaningful only if has_query42 is true. */
+	bool has_ds4_queries;
+	bool has_multi_physical;
+	bool has_guest;
+	bool has_swr;
+	bool has_nominal_report_rate;
+	bool has_recalibration_interval;
+
+	/* Tells how many of the Query43.xx registers are present.
+	 */
+	u8 ds4_query_length;
+
+	/* Query 43.1 */
+	bool has_package_id_query;
+	bool has_build_id_query;
+	bool has_reset_query;
+	bool has_maskrev_query;
+
+	/* Query 43.2 */
+	bool has_i2c_control;
+	bool has_spi_control;
+	bool has_attn_control;
+	bool has_win8_vendor_info;
+	bool has_timestamp;
+
+	/* Query 43.3 */
+	bool has_tool_id_query;
+	bool has_fw_revision_query;
+
+	/* Query 44 */
+	bool reset_enabled;
+	bool reset_polarity;
+	bool pullup_enabled;
+	u8 reset_pin;
+
+	/* Query 45 */
+	char tool_id[RMI_TOOL_ID_LENGTH + 1];
+
+	/* Query 46 */
+	char fw_revision[RMI_FW_REVISION_LENGTH + 1];
 };
 
+
+/** Read the F01 query registers and populate the basic_properties structure.
+ * @rmi_dev - the device to be queries.
+ * @query_base_addr - address of the start of the query registers.
+ * @props - pointer to the structure to be filled in.
+ */
+int rmi_f01_read_properties(struct rmi_device *rmi_dev, u16 query_base_addr,
+			    struct f01_basic_properties *props);
+
 /* F01 device status bits */
 
 /* Most recent device status event */
 #define RMI_F01_STATUS_CODE(status)		((status) & 0x0f)
+/* Indicates that flash programming is enabled (bootloader mode). */
+#define RMI_F01_STATUS_BOOTLOADER(status)	(!!((status) & 0x40))
 /* The device has lost its configuration for some reason. */
 #define RMI_F01_STATUS_UNCONFIGURED(status)	(!!((status) & 0x80))
 

[PATCH 03/05] input synaptics-rmi4: rmi_f01 - Fix a comment and add a diagnostic output message.

[Christopher Heiny]

In debugging certain touch sensor failures, it's useful to know
whether the device is stuck in bootloader, so print a message
to that effect.

Also, point to the actual location of the defs for the F01 CTRL0
bitfields.

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/rmi_f01.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 8504865..a078d7d 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -16,7 +16,7 @@
 #include "rmi_f01.h"
 
 /**
- * @ctrl0 - see the bit definitions above.
+ * @ctrl0 - see the bit definitions in rmi_f01.h.
  * @doze_interval - controls the interval between checks for finger presence
  * when the touch sensor is in doze mode, in units of 10ms.
  * @wakeup_threshold - controls the capacitance threshold at which the touch
@@ -415,6 +415,13 @@ static int rmi_f01_probe(struct rmi_function *fn)
 		return error;
 	}
 
+	driver_data->f01_bootloader_mode =
+			RMI_F01_STATUS_BOOTLOADER(device_status);
+	if (driver_data->f01_bootloader_mode)
+		dev_warn(&rmi_dev->dev,
+			 "WARNING: RMI4 device is in bootloader mode!\n");
+
+
 	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
 		dev_err(&fn->dev,
 			"Device was reset during configuration process, status: %#02x!\n",

[PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.

[Christopher Heiny]

Reflash functionality will need to unload the existing functions and
rescan the PDT before starting reflash; then reload the functions
afterwards.

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/rmi_driver.c | 165 ++++++++++++++++++++++------------------
 drivers/input/rmi4/rmi_driver.h |  22 +++---
 2 files changed, 101 insertions(+), 86 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2172c80..70410e8 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -31,14 +31,20 @@
 #include <uapi/linux/input.h>
 #include "rmi_bus.h"
 #include "rmi_driver.h"
+#include "rmi_f01.h"
 
 #define HAS_NONSTANDARD_PDT_MASK 0x40
 #define RMI4_MAX_PAGE 0xff
 #define RMI4_PAGE_SIZE 0x100
 #define RMI4_PAGE_MASK 0xFF00
 
-#define RMI_DEVICE_RESET_CMD	0x01
-#define DEFAULT_RESET_DELAY_MS	100
+#define RMI_PDT_ENTRY_SIZE 6
+#define RMI_PDT_FUNCTION_VERSION_MASK   0x60
+#define RMI_PDT_INT_SOURCE_COUNT_MASK   0x07
+
+#define PDT_START_SCAN_LOCATION 0x00e9
+#define PDT_END_SCAN_LOCATION	0x0005
+#define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
 
 #define DEFAULT_POLL_INTERVAL_MS	13
 
@@ -174,7 +180,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
 	return process_interrupt_requests(rmi_dev);
 }
 
-static void rmi_free_function_list(struct rmi_device *rmi_dev)
+void rmi_free_function_list(struct rmi_device *rmi_dev)
 {
 	struct rmi_function *fn, *tmp;
 	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
@@ -435,8 +441,8 @@ static int rmi_driver_reset_handler(struct rmi_device *rmi_dev)
 	return 0;
 }
 
-int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
-			u16 pdt_address)
+static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
+			      struct pdt_entry *entry, u16 pdt_address)
 {
 	u8 buf[RMI_PDT_ENTRY_SIZE];
 	int error;
@@ -459,7 +465,6 @@ int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rmi_read_pdt_entry);
 
 static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
 				      struct rmi_function_descriptor *fd)
@@ -473,9 +478,6 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
 	fd->function_version = pdt->function_version;
 }
 
-#define RMI_SCAN_CONTINUE	0
-#define RMI_SCAN_DONE		1
-
 static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 			     int page,
 			     void *ctx,
@@ -508,10 +510,9 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 	return data->f01_bootloader_mode ? RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
 }
 
-static int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
-			int (*callback)(struct rmi_device *rmi_dev,
-					void *ctx,
-					const struct pdt_entry *entry))
+int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
+		 int (*callback)(struct rmi_device *rmi_dev,
+				 void *ctx, const struct pdt_entry *entry))
 {
 	int page;
 	int retval = RMI_SCAN_DONE;
@@ -525,16 +526,13 @@ static int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
 	return retval < 0 ? retval : 0;
 }
 
-/* Indicates that flash programming is enabled (bootloader mode). */
-#define RMI_F01_STATUS_BOOTLOADER(status)	(!!((status) & 0x40))
-
 /*
  * Given the PDT entry for F01, read the device status register to determine
  * if we're stuck in bootloader mode or not.
  *
  */
-static int rmi_check_bootloader_mode(struct rmi_device *rmi_dev,
-				     const struct pdt_entry *pdt)
+int rmi_check_bootloader_mode(struct rmi_device *rmi_dev,
+			      const struct pdt_entry *pdt)
 {
 	int error;
 	u8 device_status;
@@ -575,7 +573,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
 
 	if (pdt->function_number == 0x01) {
 		u16 cmd_addr = pdt->page_start + pdt->command_base_addr;
-		u8 cmd_buf = RMI_DEVICE_RESET_CMD;
+		u8 cmd_buf = RMI_F01_CMD_DEVICE_RESET;
 		const struct rmi_device_platform_data *pdata =
 				rmi_get_platform_data(rmi_dev);
 
@@ -586,7 +584,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
 			return error;
 		}
 
-		mdelay(pdata->reset_delay_ms ?: DEFAULT_RESET_DELAY_MS);
+		mdelay(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
 
 		return RMI_SCAN_DONE;
 	}
@@ -728,15 +726,80 @@ static int rmi_driver_remove(struct device *dev)
 	return 0;
 }
 
+int rmi_driver_detect_functions(struct rmi_device *rmi_dev)
+{
+	int error;
+	int irq_count;
+	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
+	void *irq_memory;
+	size_t size;
+
+	/*
+	 * We need to count the IRQs and allocate their storage before scanning
+	 * the PDT and creating the function entries, because adding a new
+	 * function can trigger events that result in the IRQ related storage
+	 * being accessed.
+	 */
+	dev_dbg(&rmi_dev->dev, "Counting IRQs.\n");
+	irq_count = 0;
+	error = rmi_scan_pdt(rmi_dev, &irq_count, rmi_count_irqs);
+	if (error) {
+		error = -ENODEV;
+		dev_err(&rmi_dev->dev, "IRQ counting failed with code %d.\n",
+			error);
+		goto err_free_mem;
+	}
+	data->irq_count = irq_count;
+	data->num_of_irq_regs = (data->irq_count + 7) / 8;
+
+	size = BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long);
+	irq_memory = kzalloc(size * 4, GFP_KERNEL);
+	if (!irq_memory) {
+		dev_err(&rmi_dev->dev, "Failed to allocate memory for irq masks.\n");
+		goto err_free_mem;
+	}
+	kfree(data->irq_status); /* free the memory if it's allocated already */
+	data->irq_status	= irq_memory + size * 0;
+	data->fn_irq_bits	= irq_memory + size * 1;
+	data->current_irq_mask	= irq_memory + size * 2;
+	data->new_irq_mask	= irq_memory + size * 3;
+
+	irq_count = 0;
+	dev_dbg(&rmi_dev->dev, "Creating functions.");
+	error = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_function);
+	if (error < 0) {
+		dev_err(&rmi_dev->dev, "Function creation failed with code %d.\n",
+			error);
+		goto err_destroy_functions;
+	}
+	if (!data->f01_container) {
+		dev_err(&rmi_dev->dev, "missing F01 container!\n");
+		error = -EINVAL;
+		goto err_destroy_functions;
+	}
+	error = rmi_read_block(rmi_dev,
+				data->f01_container->fd.control_base_addr+1,
+				data->current_irq_mask, data->num_of_irq_regs);
+	if (error) {
+		dev_err(&rmi_dev->dev, "%s: Failed to read current IRQ mask.\n",
+			__func__);
+		goto err_destroy_functions;
+	}
+	return 0;
+
+err_destroy_functions:
+	rmi_free_function_list(rmi_dev);
+	kfree(irq_memory);
+err_free_mem:
+	return error;
+}
+
 static int rmi_driver_probe(struct device *dev)
 {
 	struct rmi_driver *rmi_driver;
 	struct rmi_driver_data *data;
 	const struct rmi_device_platform_data *pdata;
 	struct rmi_device *rmi_dev;
-	size_t size;
-	void *irq_memory;
-	int irq_count;
 	int retval;
 
 	dev_dbg(dev, "%s: Starting probe.\n", __func__);
@@ -796,59 +859,11 @@ static int rmi_driver_probe(struct device *dev)
 		dev_warn(dev, "Could not read PDT properties from %#06x (code %d). Assuming 0x00.\n",
 			 PDT_PROPERTIES_LOCATION, retval);
 	}
-
-	/*
-	 * We need to count the IRQs and allocate their storage before scanning
-	 * the PDT and creating the function entries, because adding a new
-	 * function can trigger events that result in the IRQ related storage
-	 * being accessed.
-	 */
-	dev_dbg(dev, "Counting IRQs.\n");
-	irq_count = 0;
-	retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_count_irqs);
-	if (retval < 0) {
-		dev_err(dev, "IRQ counting for %s failed with code %d.\n",
-			pdata->sensor_name, retval);
-		goto err_free_mem;
-	}
-	data->irq_count = irq_count;
-	data->num_of_irq_regs = (data->irq_count + 7) / 8;
-
 	mutex_init(&data->irq_mutex);
 
-	size = BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long);
-	irq_memory = kzalloc(size * 4, GFP_KERNEL);
-	if (!irq_memory) {
-		dev_err(dev, "Failed to allocate memory for irq masks.\n");
-		goto err_free_mem;
-	}
-
-	data->irq_status	= irq_memory + size * 0;
-	data->fn_irq_bits	= irq_memory + size * 1;
-	data->current_irq_mask	= irq_memory + size * 2;
-	data->new_irq_mask	= irq_memory + size * 3;
-
-	irq_count = 0;
-	dev_dbg(dev, "Creating functions.");
-	retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_function);
-	if (retval < 0) {
-		dev_err(dev, "Function creation failed with code %d.\n",
-			retval);
-		goto err_destroy_functions;
-	}
-
-	if (!data->f01_container) {
-		dev_err(dev, "Missing F01 container!\n");
-		retval = -EINVAL;
-		goto err_destroy_functions;
-	}
-
-	retval = rmi_read_block(rmi_dev,
-				data->f01_container->fd.control_base_addr + 1,
-				data->current_irq_mask, data->num_of_irq_regs);
-	if (retval < 0) {
-		dev_err(dev, "%s: Failed to read current IRQ mask.\n",
-			__func__);
+	retval = rmi_driver_detect_functions(rmi_dev);
+	if (retval) {
+		dev_err(dev, "Failed to detect functions!\n");
 		goto err_destroy_functions;
 	}
 
@@ -918,8 +933,6 @@ static int rmi_driver_probe(struct device *dev)
 
 err_destroy_functions:
 	rmi_free_function_list(rmi_dev);
-	kfree(irq_memory);
-err_free_mem:
 	if (data->gpio_held)
 		gpio_free(pdata->attn_gpio);
 	kfree(data);
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 34f7a7d..0cc9089 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -84,14 +84,6 @@ struct rmi_driver_data {
 	void *data;
 };
 
-#define RMI_PDT_ENTRY_SIZE 6
-#define RMI_PDT_FUNCTION_VERSION_MASK   0x60
-#define RMI_PDT_INT_SOURCE_COUNT_MASK   0x07
-
-#define PDT_START_SCAN_LOCATION 0x00e9
-#define PDT_END_SCAN_LOCATION	0x0005
-#define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
-
 struct pdt_entry {
 	u16 page_start;
 	u8 query_base_addr;
@@ -103,8 +95,12 @@ struct pdt_entry {
 	u8 function_number;
 };
 
-int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
-			u16 pdt_address);
+#define RMI_SCAN_CONTINUE	0
+#define RMI_SCAN_DONE		1
+
+int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
+		 int (*callback)(struct rmi_device *rmi_dev,
+				 void *ctx, const struct pdt_entry *entry));
 
 bool rmi_is_physical_driver(struct device_driver *);
 int rmi_register_physical_driver(void);
@@ -113,4 +109,10 @@ void rmi_unregister_physical_driver(void);
 int rmi_register_f01_handler(void);
 void rmi_unregister_f01_handler(void);
 
+int check_bootloader_mode(struct rmi_device *rmi_dev,
+			  const struct pdt_entry *pdt);
+void rmi_free_function_list(struct rmi_device *rmi_dev);
+int rmi_driver_detect_functions(struct rmi_device *rmi_dev);
+
+
 #endif

[PATCH 05/05] input synaptics-rmi4: Add reflash support

[Christopher Heiny]

Add support for reflashing V5 bootloader firmwares into 
RMI4 devices.

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/Kconfig         |   9 +
 drivers/input/rmi4/Makefile        |   1 +
 drivers/input/rmi4/rmi_bus.c       |   3 +
 drivers/input/rmi4/rmi_driver.h    |  11 +
 drivers/input/rmi4/rmi_fw_update.c | 961 +++++++++++++++++++++++++++++++++++++
 5 files changed, 985 insertions(+)

diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
index d0c7b6e..9b88b6a 100644
--- a/drivers/input/rmi4/Kconfig
+++ b/drivers/input/rmi4/Kconfig
@@ -25,6 +25,15 @@ config RMI4_DEBUG
 
 	  If unsure, say N.
 
+config RMI4_FWLIB
+	bool "RMI4 Firmware Update"
+	depends on RMI4_CORE
+	help
+	  Say Y here to enable in-kernel firmware update capability.
+
+	  Add support to the RMI4 core to enable reflashing of device
+	  firmware.
+
 config RMI4_I2C
 	tristate "RMI4 I2C Support"
 	depends on RMI4_CORE && I2C
diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
index 5c6bad5..570ea80 100644
--- a/drivers/input/rmi4/Makefile
+++ b/drivers/input/rmi4/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_RMI4_CORE) += rmi_core.o
 rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
+rmi_core-$(CONFIG_RMI4_FWLIB) += rmi_fw_update.o
 
 # Function drivers
 obj-$(CONFIG_RMI4_F11) += rmi_f11.o
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 6e0454a..3c93d08 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -117,6 +117,8 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
 	if (error)
 		goto err_put_device;
 
+	rmi_reflash_init(rmi_dev);
+
 	dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
 		pdata->sensor_name, dev_name(&rmi_dev->dev));
 
@@ -139,6 +141,7 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
 	struct rmi_device *rmi_dev = xport->rmi_dev;
 
 	device_del(&rmi_dev->dev);
+	rmi_reflash_cleanup(rmi_dev);
 	rmi_physical_teardown_debugfs(rmi_dev);
 	put_device(&rmi_dev->dev);
 }
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 0cc9089..c49b123 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -29,6 +29,8 @@
 
 #define RMI_PDT_PROPS_HAS_BSR 0x02
 
+struct rmi_reflash_data;
+
 struct rmi_driver_data {
 	struct list_head function_list;
 
@@ -81,6 +83,8 @@ struct rmi_driver_data {
 	u8 reg_debug_size;
 #endif
 
+	struct rmi_reflash_data *reflash_data;
+
 	void *data;
 };
 
@@ -114,5 +118,12 @@ int check_bootloader_mode(struct rmi_device *rmi_dev,
 void rmi_free_function_list(struct rmi_device *rmi_dev);
 int rmi_driver_detect_functions(struct rmi_device *rmi_dev);
 
+#ifdef CONFIG_RMI4_FWLIB
+void rmi_reflash_init(struct rmi_device *rmi_dev);
+void rmi_reflash_cleanup(struct rmi_device *rmi_dev);
+#else
+#define rmi_reflash_init(rmi_dev)
+#define rmi_reflash_cleanup(rmi_dev)
+#endif
 
 #endif
diff --git a/drivers/input/rmi4/rmi_fw_update.c b/drivers/input/rmi4/rmi_fw_update.c
new file mode 100644
index 0000000..7118401
--- /dev/null
+++ b/drivers/input/rmi4/rmi_fw_update.c
@@ -0,0 +1,961 @@
+/*
+ * Copyright (c) 2012-2014 Synaptics Incorporated
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/firmware.h>
+#include <linux/ihex.h>
+#include <linux/kernel.h>
+#include <linux/rmi.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+#include "rmi_driver.h"
+#include "rmi_f01.h"
+
+/* F34 Query register defs. */
+
+#define RMI_F34_QUERY_SIZE		7
+#define RMI_F34_HAS_NEW_REG_MAP		(1 << 0)
+#define RMI_F34_IS_UNLOCKED		(1 << 1)
+#define RMI_F34_HAS_CONFIG_ID		(1 << 2)
+#define RMI_F34_BLOCK_SIZE_OFFSET	1
+#define RMI_F34_FW_BLOCKS_OFFSET	3
+#define RMI_F34_CONFIG_BLOCKS_OFFSET	5
+
+struct rmi_f34_queries {
+	bool	new_reg_map;
+	bool	unlocked;
+	bool	has_config_id;
+	u16	block_size;
+	u16	fw_block_count;
+	u16	config_block_count;
+};
+
+/* F34 Data register defs. */
+
+#define RMI_F34_BLOCK_DATA_OFFSET	2
+
+#define RMI_F34_COMMAND_MASK		0x0F
+#define RMI_F34_STATUS_MASK		0x07
+#define RMI_F34_STATUS_SHIFT		4
+#define RMI_F34_ENABLED_MASK		0x80
+
+#define RMI_F34_WRITE_FW_BLOCK        0x02
+#define RMI_F34_ERASE_ALL             0x03
+#define RMI_F34_WRITE_CONFIG_BLOCK    0x06
+#define RMI_F34_ENABLE_FLASH_PROG     0x0f
+
+struct rmi_f34_control_status {
+	u8 command;
+	u8 status;
+	bool program_enabled;
+};
+
+/* Timeouts for various F34 operations. */
+#define RMI_F34_ENABLE_WAIT_MS 300
+#define RMI_F34_ERASE_WAIT_MS (5 * 1000)
+#define RMI_F34_IDLE_WAIT_MS 500
+
+#define IS_IDLE(ctl_ptr) ((!ctl_ptr->status) && (!ctl_ptr->command))
+
+
+/* Image file defs. */
+#define RMI_IMG_CHECKSUM_OFFSET			0
+#define RMI_IMG_IO_OFFSET			0x06
+#define RMI_IMG_BOOTLOADER_VERSION_OFFSET	0x07
+#define RMI_IMG_IMAGE_SIZE_OFFSET		0x08
+#define RMI_IMG_CONFIG_SIZE_OFFSET		0x0C
+#define RMI_IMG_PACKAGE_ID_OFFSET		0x1A
+#define RMI_IMG_FW_BUILD_ID_OFFSET		0x50
+
+#define RMI_IMG_PRODUCT_INFO_LENGTH		2
+
+#define RMI_IMG_PRODUCT_ID_OFFSET		0x10
+#define RMI_IMG_PRODUCT_INFO_OFFSET		0x1E
+
+#define RMI_F34_FW_IMAGE_OFFSET 0x100
+
+/* Image file V5, Option 0 */
+struct rmi_image_header {
+	u32 checksum;
+	unsigned int image_size;
+	unsigned int config_size;
+	u8 options;
+	u8 io;
+	u32 fw_build_id;
+	u32 package_id;
+	u8 bootloader_version;
+	u8 product_id[RMI_PRODUCT_ID_LENGTH + 1];
+	u8 product_info[RMI_IMG_PRODUCT_INFO_LENGTH];
+};
+
+static u32 rmi_extract_u32(const u8 *ptr)
+{
+	return (u32)ptr[0] +
+		(u32)ptr[1] * 0x100 +
+		(u32)ptr[2] * 0x10000 +
+		(u32)ptr[3] * 0x1000000;
+}
+
+#define RMI_NAME_BUFFER_SIZE 64
+
+struct rmi_reflash_data {
+	struct rmi_device *rmi_dev;
+	bool force;
+	ulong busy;
+	char name_buf[RMI_NAME_BUFFER_SIZE];
+	char *img_name;
+	struct pdt_entry f01_pdt;
+	struct f01_basic_properties f01_props;
+	u8 device_status;
+	struct pdt_entry f34_pdt;
+	u8 bootloader_id[2];
+	struct rmi_f34_queries f34_queries;
+	u16 f34_status_address;
+	struct rmi_f34_control_status f34_controls;
+	const u8 *firmware_data;
+	const u8 *config_data;
+	struct work_struct reflash_work;
+};
+
+static void rmi_extract_header(const u8 *data, int pos,
+			       struct rmi_image_header *header)
+{
+	header->checksum =
+			rmi_extract_u32(&data[pos + RMI_IMG_CHECKSUM_OFFSET]);
+	header->io = data[pos + RMI_IMG_IO_OFFSET];
+	header->bootloader_version =
+			data[pos + RMI_IMG_BOOTLOADER_VERSION_OFFSET];
+	header->image_size =
+			rmi_extract_u32(&data[pos + RMI_IMG_IMAGE_SIZE_OFFSET]);
+	header->config_size =
+		       rmi_extract_u32(&data[pos + RMI_IMG_CONFIG_SIZE_OFFSET]);
+	if (header->io == 1) {
+		header->fw_build_id =
+		       rmi_extract_u32(&data[pos + RMI_IMG_FW_BUILD_ID_OFFSET]);
+		header->package_id =
+			rmi_extract_u32(&data[pos + RMI_IMG_PACKAGE_ID_OFFSET]);
+	}
+	memcpy(header->product_id, &data[pos + RMI_IMG_PRODUCT_ID_OFFSET],
+	       RMI_PRODUCT_ID_LENGTH);
+	header->product_id[RMI_PRODUCT_ID_LENGTH] = 0;
+	memcpy(header->product_info, &data[pos + RMI_IMG_PRODUCT_INFO_OFFSET],
+	       RMI_IMG_PRODUCT_INFO_LENGTH);
+}
+
+static int rmi_find_functions(struct rmi_device *rmi_dev,
+			      void *ctx, const struct pdt_entry *pdt)
+{
+	struct rmi_reflash_data *data = ctx;
+
+	if (pdt->page_start > 0)
+		return RMI_SCAN_DONE;
+
+	if (pdt->function_number == 0x01)
+		memcpy(&data->f01_pdt, pdt, sizeof(struct pdt_entry));
+	else if (pdt->function_number == 0x34)
+		memcpy(&data->f34_pdt, pdt, sizeof(struct pdt_entry));
+
+	return RMI_SCAN_CONTINUE;
+}
+
+static int rmi_find_f01_and_f34(struct rmi_reflash_data *data)
+{
+	struct rmi_device *rmi_dev = data->rmi_dev;
+	int retval;
+
+	data->f01_pdt.function_number = data->f34_pdt.function_number = 0;
+	retval = rmi_scan_pdt(rmi_dev, data, rmi_find_functions);
+	if (retval < 0)
+		return retval;
+
+	if (!data->f01_pdt.function_number) {
+		dev_err(&rmi_dev->dev, "Failed to find F01 for reflash.\n");
+		return -ENODEV;
+	}
+
+	if (!data->f34_pdt.function_number) {
+		dev_err(&rmi_dev->dev, "Failed to find F34 for reflash.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int rmi_read_f34_controls(struct rmi_reflash_data *data)
+{
+	int retval;
+	u8 buf;
+
+	retval = rmi_read(data->rmi_dev, data->f34_status_address, &buf);
+	if (retval)
+		return retval;
+
+	data->f34_controls.command = buf & RMI_F34_COMMAND_MASK;
+	data->f34_controls.status = (buf >> RMI_F34_STATUS_SHIFT)
+						& RMI_F34_STATUS_MASK;
+	data->f34_controls.program_enabled = !!(buf & RMI_F34_ENABLED_MASK);
+
+	return 0;
+}
+
+static int rmi_read_f01_status(struct rmi_reflash_data *data)
+{
+	int retval;
+
+	retval = rmi_read(data->rmi_dev, data->f01_pdt.data_base_addr,
+			  &data->device_status);
+	if (retval)
+		return retval;
+
+	return 0;
+}
+
+#define RMI_MIN_SLEEP_TIME_US 50
+#define RMI_MAX_SLEEP_TIME_US 100
+
+/*
+ * Wait until the status is idle and we're ready to continue.
+ *
+ * In order to indicate that the previous command has succeeded, the
+ * bootloader is supposed to signal idle state by:
+ *
+ *  + atomically do the following by writing 0x80 to F34_FLASH_Data3
+ *     - set status to 0,
+ *     - set command to 0, and
+ *     - leave progam_enabled as 1
+ *  + assert attn
+ *
+ * and then we're supposed to be able to see that we're in IDLE state.
+ * But some (most?) bootloaders do this
+ *
+ *  + clear F34_FLASH_Data3
+ *  + assert attn
+ *  + set the program_enabled bit
+ *
+ * and a significant number of those don't even bother to assert
+ * ATTN, so you've got to poll them (which is what we're doing
+ * here). Regardless of whether you're polling or using ATTN,
+ * there's a race condition between clearing Data3 and setting
+ * program_enabled. So when we lose that race, we emit this warning
+ * (using dev_WARN_ONCE to avoid filling the log with complaints)
+ * and retry a few times. If a correct idle state is reached during
+ * the retries, then we just continue with the process. If it's not
+ * reached (that is, if Data3 contains anything but 0x80 after the
+ * timeout), then something has gone horribly wrong, and we abort the
+ * reflash process.
+ *
+ */
+static int rmi_wait_for_idle(struct rmi_reflash_data *data, int timeout_ms)
+{
+	int timeout_count = ((timeout_ms * 1000) / RMI_MAX_SLEEP_TIME_US) + 1;
+	int count = 0;
+	struct rmi_f34_control_status *controls = &data->f34_controls;
+	int retval;
+
+	do {
+		if (count || timeout_count == 1)
+			usleep_range(RMI_MIN_SLEEP_TIME_US,
+				     RMI_MAX_SLEEP_TIME_US);
+		retval = rmi_read_f34_controls(data);
+		count++;
+		if (retval)
+			continue;
+		else if (IS_IDLE(controls)) {
+			if (dev_WARN_ONCE(&data->rmi_dev->dev,
+					  !data->f34_controls.program_enabled,
+					  "Reflash is idle but program_enabled bit isn't set.\n"
+					  ))
+				/*
+				 * This works around a bug in certain device
+				 * firmwares, where the idle state is reached,
+				 * but the program_enabled bit is not yet set.
+				 */
+				continue;
+			return 0;
+		}
+	} while (count < timeout_count);
+
+	dev_err(&data->rmi_dev->dev,
+		"ERROR: Timeout waiting for idle status.\n");
+	dev_err(&data->rmi_dev->dev, "Command: %#04x\n", controls->command);
+	dev_err(&data->rmi_dev->dev, "Status:  %#04x\n", controls->status);
+	dev_err(&data->rmi_dev->dev, "Enabled: %d\n",
+			controls->program_enabled);
+	dev_err(&data->rmi_dev->dev, "Idle:    %d\n", IS_IDLE(controls));
+	return -ETIMEDOUT;
+}
+
+static int rmi_read_f34_queries(struct rmi_reflash_data *data)
+{
+	int retval;
+	u8 id_str[3];
+	u8 buf[RMI_F34_QUERY_SIZE];
+
+	retval = rmi_read_block(data->rmi_dev, data->f34_pdt.query_base_addr,
+				data->bootloader_id, 2);
+	if (retval) {
+		dev_err(&data->rmi_dev->dev,
+			"Failed to read F34 bootloader_id (code %d).\n",
+			retval);
+		return retval;
+	}
+
+	retval = rmi_read_block(data->rmi_dev, data->f34_pdt.query_base_addr+2,
+				buf, RMI_F34_QUERY_SIZE);
+	if (retval) {
+		dev_err(&data->rmi_dev->dev,
+			"Failed to read F34 queries (code %d).\n", retval);
+		return retval;
+	}
+
+	data->f34_queries.new_reg_map = buf[0] & RMI_F34_HAS_NEW_REG_MAP;
+	data->f34_queries.unlocked = buf[0] & RMI_F34_IS_UNLOCKED;
+	data->f34_queries.has_config_id = buf[0] & RMI_F34_HAS_CONFIG_ID;
+	data->f34_queries.block_size =
+		le16_to_cpu(*((u16 *)(buf + RMI_F34_BLOCK_SIZE_OFFSET)));
+	data->f34_queries.fw_block_count =
+		le16_to_cpu(*((u16 *)(buf + RMI_F34_FW_BLOCKS_OFFSET)));
+	data->f34_queries.config_block_count =
+		le16_to_cpu(*((u16 *)(buf + RMI_F34_CONFIG_BLOCKS_OFFSET)));
+
+	id_str[0] = data->bootloader_id[0];
+	id_str[1] = data->bootloader_id[1];
+	id_str[2] = 0;
+
+	dev_dbg(&data->rmi_dev->dev, "F34 bootloader id: %s (%#04x %#04x)\n",
+		id_str, data->bootloader_id[0], data->bootloader_id[1]);
+	dev_dbg(&data->rmi_dev->dev, "F34 has config id: %d\n",
+		data->f34_queries.has_config_id);
+	dev_dbg(&data->rmi_dev->dev, "F34 unlocked:      %d\n",
+		data->f34_queries.unlocked);
+	dev_dbg(&data->rmi_dev->dev, "F34 new reg map:   %d\n",
+		data->f34_queries.new_reg_map);
+	dev_dbg(&data->rmi_dev->dev, "F34 block size:    %d\n",
+		data->f34_queries.block_size);
+	dev_dbg(&data->rmi_dev->dev, "F34 fw blocks:     %d\n",
+		data->f34_queries.fw_block_count);
+	dev_dbg(&data->rmi_dev->dev, "F34 config blocks: %d\n",
+		data->f34_queries.config_block_count);
+
+	data->f34_status_address = data->f34_pdt.data_base_addr +
+		RMI_F34_BLOCK_DATA_OFFSET + data->f34_queries.block_size;
+
+	return 0;
+}
+
+static int rmi_write_bootloader_id(struct rmi_reflash_data *data)
+{
+	int retval;
+	struct rmi_device *rmi_dev = data->rmi_dev;
+
+	retval = rmi_write_block(rmi_dev,
+		      data->f34_pdt.data_base_addr + RMI_F34_BLOCK_DATA_OFFSET,
+		      data->bootloader_id, ARRAY_SIZE(data->bootloader_id));
+	if (retval < 0) {
+		dev_err(&rmi_dev->dev,
+			"Failed to write bootloader ID. Code: %d.\n", retval);
+		return retval;
+	}
+
+	return 0;
+}
+
+static int rmi_write_f34_command(struct rmi_reflash_data *data, u8 command)
+{
+	int retval;
+	struct rmi_device *rmi_dev = data->rmi_dev;
+
+	retval = rmi_write(rmi_dev, data->f34_status_address, command);
+	if (retval < 0) {
+		dev_err(&rmi_dev->dev,
+			"Failed to write F34 command %#04x. Code: %d.\n",
+			command, retval);
+		return retval;
+	}
+
+	return 0;
+}
+
+static int rmi_enter_flash_programming(struct rmi_reflash_data *data)
+{
+	int retval;
+	struct rmi_device *rmi_dev = data->rmi_dev;
+	u8 f01_control_0;
+
+	retval = rmi_write_bootloader_id(data);
+	if (retval < 0)
+		return retval;
+
+	dev_dbg(&rmi_dev->dev, "Enabling flash programming.\n");
+	retval = rmi_write_f34_command(data, RMI_F34_ENABLE_FLASH_PROG);
+	if (retval < 0)
+		return retval;
+
+	retval = rmi_wait_for_idle(data, RMI_F34_ENABLE_WAIT_MS);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "Did not reach idle state after %d ms. Code: %d.\n",
+			RMI_F34_ENABLE_WAIT_MS, retval);
+		return retval;
+	}
+	if (!data->f34_controls.program_enabled) {
+		dev_err(&rmi_dev->dev, "Reached idle, but programming not enabled.\n");
+		return -EINVAL;
+	}
+	dev_dbg(&rmi_dev->dev, "HOORAY! Programming is enabled!\n");
+
+	retval = rmi_find_f01_and_f34(data);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "Failed to rescan pdt.  Code: %d.\n",
+			retval);
+		return retval;
+	}
+
+	retval = rmi_read_f01_status(data);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "Failed to read F01 status after enabling reflash. Code: %d.\n",
+			retval);
+		return retval;
+	}
+	if (!RMI_F01_STATUS_BOOTLOADER(data->device_status)) {
+		dev_err(&rmi_dev->dev, "Device reports as not in flash programming mode.\n");
+		return -EINVAL;
+	}
+
+	retval = rmi_read_f34_queries(data);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
+			retval);
+		return retval;
+	}
+
+	retval = rmi_read(rmi_dev, data->f01_pdt.control_base_addr,
+			  &f01_control_0);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "F01_CTRL_0 read failed, code = %d.\n",
+			retval);
+		return retval;
+	}
+	f01_control_0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+	f01_control_0 = (f01_control_0 & ~RMI_F01_CTRL0_SLEEP_MODE_MASK)
+			| RMI_SLEEP_MODE_NORMAL;
+
+	retval = rmi_write(rmi_dev, data->f01_pdt.control_base_addr,
+			   f01_control_0);
+	if (retval < 0) {
+		dev_err(&rmi_dev->dev, "F01_CTRL_0 write failed, code = %d.\n",
+			retval);
+		return retval;
+	}
+
+	return 0;
+}
+
+static void rmi_reset_device(struct rmi_reflash_data *data)
+{
+	int retval;
+	const struct rmi_device_platform_data *pdata =
+				rmi_get_platform_data(data->rmi_dev);
+
+	dev_dbg(&data->rmi_dev->dev, "Resetting...\n");
+	retval = rmi_write(data->rmi_dev, data->f01_pdt.command_base_addr,
+			   RMI_F01_CMD_DEVICE_RESET);
+	if (retval < 0)
+		dev_warn(&data->rmi_dev->dev,
+			 "WARNING - post-flash reset failed, code: %d.\n",
+			 retval);
+	msleep(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
+	dev_dbg(&data->rmi_dev->dev, "Reset completed.\n");
+}
+
+/*
+ * Send data to the device one block at a time.
+ */
+static int rmi_write_blocks(struct rmi_reflash_data *data, u8 *block_ptr,
+			    u16 block_count, u8 cmd)
+{
+	int block_num;
+	u8 zeros[] = {0, 0};
+	int retval;
+	u16 addr = data->f34_pdt.data_base_addr + RMI_F34_BLOCK_DATA_OFFSET;
+
+	retval = rmi_write_block(data->rmi_dev, data->f34_pdt.data_base_addr,
+				 zeros, ARRAY_SIZE(zeros));
+	if (retval < 0) {
+		dev_err(&data->rmi_dev->dev, "Failed to write initial zeros. Code=%d.\n",
+			retval);
+		return retval;
+	}
+
+	for (block_num = 0; block_num < block_count; ++block_num) {
+		retval = rmi_write_block(data->rmi_dev, addr, block_ptr,
+					 data->f34_queries.block_size);
+		if (retval < 0) {
+			dev_err(&data->rmi_dev->dev, "Failed to write block %d. Code=%d.\n",
+				block_num, retval);
+			return retval;
+		}
+
+		retval = rmi_write_f34_command(data, cmd);
+		if (retval) {
+			dev_err(&data->rmi_dev->dev, "Failed to write command for block %d. Code=%d.\n",
+				block_num, retval);
+			return retval;
+		}
+
+
+		retval = rmi_wait_for_idle(data, RMI_F34_IDLE_WAIT_MS);
+		if (retval) {
+			dev_err(&data->rmi_dev->dev, "Failed to go idle after writing block %d. Code=%d.\n",
+				block_num, retval);
+			return retval;
+		}
+
+		block_ptr += data->f34_queries.block_size;
+	}
+
+	return 0;
+}
+
+static int rmi_write_firmware(struct rmi_reflash_data *data)
+{
+	return rmi_write_blocks(data, (u8 *) data->firmware_data,
+		data->f34_queries.fw_block_count, RMI_F34_WRITE_FW_BLOCK);
+}
+
+static int rmi_write_configuration(struct rmi_reflash_data *data)
+{
+	return rmi_write_blocks(data, (u8 *) data->config_data,
+		data->f34_queries.config_block_count,
+		RMI_F34_WRITE_CONFIG_BLOCK);
+}
+
+static void rmi_reflash_firmware(struct rmi_reflash_data *data)
+{
+	struct timespec start;
+	struct timespec end;
+	s64 duration_ns;
+	int retval = 0;
+
+	retval = rmi_enter_flash_programming(data);
+	if (retval) {
+		dev_err(&data->rmi_dev->dev, "Failed to enter flash programming (code: %d).\n",
+			retval);
+		return;
+	}
+
+	retval = rmi_write_bootloader_id(data);
+	if (retval) {
+		dev_err(&data->rmi_dev->dev, "Failed to enter write bootloader ID (code: %d).\n",
+			retval);
+		return;
+	}
+
+	dev_dbg(&data->rmi_dev->dev, "Erasing FW...\n");
+	getnstimeofday(&start);
+	retval = rmi_write_f34_command(data, RMI_F34_ERASE_ALL);
+	if (retval) {
+		dev_err(&data->rmi_dev->dev, "Erase failed (code: %d).\n",
+			retval);
+		return;
+	}
+
+	retval = rmi_wait_for_idle(data, RMI_F34_ERASE_WAIT_MS);
+	if (retval) {
+		dev_err(&data->rmi_dev->dev,
+			"Failed to reach idle state. Code: %d.\n", retval);
+		return;
+	}
+	getnstimeofday(&end);
+	duration_ns = timespec_to_ns(&end) - timespec_to_ns(&start);
+	dev_dbg(&data->rmi_dev->dev,
+		 "Erase complete, time: %lld ns.\n", duration_ns);
+
+	if (data->firmware_data) {
+		dev_dbg(&data->rmi_dev->dev, "Writing firmware...\n");
+		getnstimeofday(&start);
+		retval = rmi_write_firmware(data);
+		if (retval) {
+			dev_err(&data->rmi_dev->dev,
+				"Failed to write FW (code: %d).\n", retval);
+			return;
+		}
+		getnstimeofday(&end);
+		duration_ns = timespec_to_ns(&end) - timespec_to_ns(&start);
+		dev_dbg(&data->rmi_dev->dev,
+			 "Done writing FW, time: %lld ns.\n", duration_ns);
+	}
+
+	if (data->config_data) {
+		dev_dbg(&data->rmi_dev->dev, "Writing configuration...\n");
+		getnstimeofday(&start);
+		retval = rmi_write_configuration(data);
+		if (retval) {
+			dev_err(&data->rmi_dev->dev,
+				"Failed to write config (code: %d).\n", retval);
+			return;
+		}
+		getnstimeofday(&end);
+		duration_ns = timespec_to_ns(&end) - timespec_to_ns(&start);
+		dev_dbg(&data->rmi_dev->dev,
+			 "Done writing config, time: %lld ns.\n", duration_ns);
+	}
+}
+
+static bool rmi_go_nogo(struct rmi_reflash_data *data,
+			struct rmi_image_header *header)
+{
+	if (data->force) {
+		dev_dbg(&data->rmi_dev->dev, "Reflash force flag in effect.\n");
+		return true;
+	}
+
+	if (header->io == 1) {
+		if (header->fw_build_id > data->f01_props.build_id) {
+			dev_dbg(&data->rmi_dev->dev, "Image file has newer Packrat.\n");
+			return true;
+		} else {
+			dev_dbg(&data->rmi_dev->dev, "Image file has lower Packrat ID than device.\n");
+		}
+	}
+
+	return false;
+}
+
+static const char rmi_fw_name_format[] = "%s.img";
+
+static void rmi_fw_update(struct rmi_device *rmi_dev)
+{
+	struct timespec start;
+	struct timespec end;
+	s64 duration_ns;
+	char *firmware_name;
+	const struct firmware *fw_entry = NULL;
+	int retval;
+	struct rmi_image_header *header = NULL;
+	u8 pdt_props;
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+	const struct rmi_device_platform_data *pdata =
+				rmi_get_platform_data(rmi_dev);
+
+	dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
+	dev_dbg(&rmi_dev->dev, "force: %d\n", data->force);
+	dev_dbg(&rmi_dev->dev, "img_name: %s\n", data->img_name);
+	dev_dbg(&rmi_dev->dev, "firmware_name: %s\n", pdata->firmware_name);
+
+	getnstimeofday(&start);
+
+	firmware_name = kcalloc(RMI_NAME_BUFFER_SIZE, sizeof(char), GFP_KERNEL);
+	if (!firmware_name) {
+		dev_err(&rmi_dev->dev, "Failed to allocate firmware_name.\n");
+		goto done;
+	}
+	header = kzalloc(sizeof(struct rmi_image_header), GFP_KERNEL);
+	if (!header) {
+		dev_err(&rmi_dev->dev, "Failed to allocate header.\n");
+		goto done;
+	}
+
+	retval = rmi_read(rmi_dev, PDT_PROPERTIES_LOCATION, &pdt_props);
+	if (retval) {
+		dev_warn(&rmi_dev->dev,
+			 "Failed to read PDT props at %#06x (code %d). Assuming 0x00.\n",
+			 PDT_PROPERTIES_LOCATION, retval);
+	}
+	if (pdt_props & RMI_PDT_PROPS_HAS_BSR) {
+		dev_warn(&rmi_dev->dev,
+			 "Firmware update for LTS not currently supported.\n");
+		goto done;
+	}
+
+	retval = rmi_f01_read_properties(rmi_dev, data->f01_pdt.query_base_addr,
+					 &data->f01_props);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "F01 queries failed, code = %d.\n",
+			retval);
+		goto done;
+	}
+	retval = rmi_read_f34_queries(data);
+	if (retval) {
+		dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
+			retval);
+		goto done;
+	}
+	if (data->img_name && strlen(data->img_name))
+		snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
+			 rmi_fw_name_format, data->img_name);
+	else if (pdata->firmware_name && strlen(pdata->firmware_name))
+		snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
+			 rmi_fw_name_format, pdata->firmware_name);
+	else {
+		if (!strlen(data->f01_props.product_id)) {
+			dev_err(&rmi_dev->dev, "Product ID is missing or empty - will not reflash.\n");
+			goto done;
+		}
+		snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
+			 rmi_fw_name_format, data->f01_props.product_id);
+	}
+	dev_info(&rmi_dev->dev, "Requesting %s.\n", firmware_name);
+	retval = request_firmware(&fw_entry, firmware_name, &rmi_dev->dev);
+	if (retval != 0) {
+		dev_err(&rmi_dev->dev, "Firmware %s not available, code = %d\n",
+			firmware_name, retval);
+		goto done;
+	}
+
+	dev_dbg(&rmi_dev->dev, "Got firmware %s, size: %d.\n", firmware_name,
+		fw_entry->size);
+	rmi_extract_header(fw_entry->data, 0, header);
+	dev_dbg(&rmi_dev->dev, "Img checksum:           %#08X\n",
+		header->checksum);
+	dev_dbg(&rmi_dev->dev, "Img io:                 %#04X\n",
+		header->io);
+	dev_dbg(&rmi_dev->dev, "Img image size:         %d\n",
+		header->image_size);
+	dev_dbg(&rmi_dev->dev, "Img config size:        %d\n",
+		header->config_size);
+	dev_dbg(&rmi_dev->dev, "Img bootloader version: %d\n",
+		header->bootloader_version);
+	dev_dbg(&rmi_dev->dev, "Img product id:         %s\n",
+		header->product_id);
+	dev_dbg(&rmi_dev->dev, "Img product info:       %#04x %#04x\n",
+		header->product_info[0], header->product_info[1]);
+	if (header->io == 1) {
+		dev_dbg(&rmi_dev->dev, "Img Packrat:            %d\n",
+			header->fw_build_id);
+		dev_dbg(&rmi_dev->dev, "Img package:            %d\n",
+			header->package_id);
+	}
+
+	if (header->image_size)
+		data->firmware_data = fw_entry->data + RMI_F34_FW_IMAGE_OFFSET;
+	if (header->config_size)
+		data->config_data = fw_entry->data + RMI_F34_FW_IMAGE_OFFSET +
+			header->image_size;
+
+	if (rmi_go_nogo(data, header)) {
+		dev_dbg(&rmi_dev->dev, "Go/NoGo said go.\n");
+		rmi_free_function_list(rmi_dev);
+		rmi_reflash_firmware(data);
+		rmi_reset_device(data);
+		rmi_driver_detect_functions(rmi_dev);
+	} else
+		dev_dbg(&rmi_dev->dev, "Go/NoGo said don't reflash.\n");
+
+	if (fw_entry)
+		release_firmware(fw_entry);
+
+
+done:
+	getnstimeofday(&end);
+	duration_ns = timespec_to_ns(&end) - timespec_to_ns(&start);
+	dev_dbg(&rmi_dev->dev, "Time to reflash: %lld ns.\n", duration_ns);
+
+	kfree(firmware_name);
+	kfree(header);
+	return;
+}
+
+/*
+ * We also reflash the device if (a) in kernel reflashing is
+ * enabled, and (b) the reflash module decides it requires reflashing.
+ *
+ * We have to do this before actually building the PDT because the reflash
+ * might cause various registers to move around.
+ */
+static int rmi_device_reflash(struct rmi_device *rmi_dev)
+{
+	struct device *dev = &rmi_dev->dev;
+	struct rmi_driver_data *drv_data = dev_get_drvdata(dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+	int retval;
+
+	retval = rmi_find_f01_and_f34(data);
+	if (retval < 0)
+		return retval;
+
+	rmi_fw_update(rmi_dev);
+
+	clear_bit(0, &data->busy);
+
+	return 0;
+}
+
+static ssize_t rmi_reflash_img_name_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct rmi_device *rmi_dev = to_rmi_device(dev);
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", data->img_name);
+}
+
+static ssize_t rmi_reflash_img_name_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct rmi_device *rmi_dev = to_rmi_device(dev);
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+
+	if (test_and_set_bit(0, &data->busy))
+		return -EBUSY;
+
+	if (!count) {
+		data->img_name = NULL;
+	} else {
+		strlcpy(data->name_buf, buf, count);
+		data->img_name = strstrip(data->name_buf);
+	}
+
+	clear_bit(0, &data->busy);
+	return count;
+}
+
+static ssize_t rmi_reflash_force_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct rmi_device *rmi_dev = to_rmi_device(dev);
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", data->force);
+}
+
+static ssize_t rmi_reflash_force_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct rmi_device *rmi_dev = to_rmi_device(dev);
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+	int retval;
+	unsigned long val;
+
+	if (test_and_set_bit(0, &data->busy))
+		return -EBUSY;
+
+	retval = kstrtoul(buf, 10, &val);
+	if (retval)
+		count = retval;
+	else
+		data->force = !!val;
+
+	clear_bit(0, &data->busy);
+
+	return count;
+}
+
+static ssize_t rmi_reflash_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct rmi_device *rmi_dev = to_rmi_device(dev);
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", test_bit(0, &data->busy));
+}
+
+static ssize_t rmi_reflash_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	int retval;
+	unsigned long val;
+	struct rmi_device *rmi_dev = to_rmi_device(dev);
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+
+	retval = kstrtoul(buf, 10, &val);
+	if (retval)
+		return retval;
+
+	if (test_and_set_bit(0, &data->busy))
+		return -EBUSY;
+
+	if (val)
+		/*
+		 * TODO: Here we start a work thread to go do the reflash, but
+		 * maybe we can just use request_firmware_timeout().
+		 */
+		schedule_work(&data->reflash_work);
+	else
+		clear_bit(0, &data->busy);
+
+	return count;
+}
+
+static void rmi_reflash_work(struct work_struct *work)
+{
+	struct rmi_reflash_data *data =
+		container_of(work, struct rmi_reflash_data, reflash_work);
+	struct rmi_device *rmi_dev = data->rmi_dev;
+	int error;
+
+	dev_dbg(&rmi_dev->dev, "%s runs.\n", __func__);
+	error = rmi_device_reflash(rmi_dev);
+	if (error < 0)
+		dev_err(&rmi_dev->dev, "Reflash attempt failed with code: %d.",
+			error);
+	clear_bit(0, &data->busy);
+}
+
+static DEVICE_ATTR(reflash_force,
+			(S_IRUGO | S_IWUGO),
+			rmi_reflash_force_show, rmi_reflash_force_store);
+static DEVICE_ATTR(reflash_name,
+			(S_IRUGO | S_IWUGO),
+			rmi_reflash_img_name_show, rmi_reflash_img_name_store);
+static DEVICE_ATTR(reflash,
+			(S_IRUGO | S_IWUGO),
+			rmi_reflash_show, rmi_reflash_store);
+
+static struct attribute *rmi_reflash_attrs[] = {
+	&dev_attr_reflash_force.attr,
+	&dev_attr_reflash_name.attr,
+	&dev_attr_reflash.attr,
+	NULL
+};
+
+static const struct attribute_group rmi_reflash_attributes = {
+	.attrs = rmi_reflash_attrs,
+};
+
+void rmi_reflash_init(struct rmi_device *rmi_dev)
+{
+	int error;
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data;
+
+	dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
+
+	data = devm_kzalloc(&rmi_dev->dev, sizeof(struct rmi_reflash_data),
+			    GFP_KERNEL);
+
+	error = sysfs_create_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
+	if (error) {
+		dev_warn(&rmi_dev->dev, "Failed to create reflash sysfs attributes.\n");
+		return;
+	}
+
+	INIT_WORK(&data->reflash_work, rmi_reflash_work);
+	data->rmi_dev = rmi_dev;
+	drv_data->reflash_data = data;
+}
+
+void rmi_reflash_cleanup(struct rmi_device *rmi_dev)
+{
+	struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+	struct rmi_reflash_data *data = drv_data->reflash_data;
+
+	sysfs_remove_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
+	devm_kfree(&rmi_dev->dev, data);
+}

Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[Courtney Cavin]

On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> 
> ---
> 
>  drivers/input/rmi4/rmi_f01.c |  96 ++-----------------------------------
>  drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+), 92 deletions(-)

$SUBJECT is 83 characters long.  Please be more brief and provide a
description.

[...]
> +#define RMI_SLEEP_MODE_NORMAL		0x00
> +#define RMI_SLEEP_MODE_SENSOR_SLEEP	0x01
> +#define RMI_SLEEP_MODE_RESERVED0	0x02
> +#define RMI_SLEEP_MODE_RESERVED1	0x03
> +
> +#define RMI_IS_VALID_SLEEPMODE(mode) \
> +	(mode >= RMI_SLEEP_MODE_NORMAL && mode <= RMI_SLEEP_MODE_RESERVED1)
> +

I might be missing something, but these seem like the only defines used
in the flash code.  Why not keep these in the f01 driver, and export
a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?

-Courtney

Re: [PATCH 03/05] input synaptics-rmi4: rmi_f01 - Fix a comment and add a diagnostic output message.

[Courtney Cavin]

On Sat, Mar 08, 2014 at 03:29:53AM +0100, Christopher Heiny wrote:
> In debugging certain touch sensor failures, it's useful to know
> whether the device is stuck in bootloader, so print a message
> to that effect.
> 
> Also, point to the actual location of the defs for the F01 CTRL0
> bitfields.
> 
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> 
> ---
> 
>  drivers/input/rmi4/rmi_f01.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 8504865..a078d7d 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -16,7 +16,7 @@
>  #include "rmi_f01.h"
>  
>  /**
> - * @ctrl0 - see the bit definitions above.
> + * @ctrl0 - see the bit definitions in rmi_f01.h.
>   * @doze_interval - controls the interval between checks for finger presence
>   * when the touch sensor is in doze mode, in units of 10ms.
>   * @wakeup_threshold - controls the capacitance threshold at which the touch
> @@ -415,6 +415,13 @@ static int rmi_f01_probe(struct rmi_function *fn)
>  		return error;
>  	}
>  
> +	driver_data->f01_bootloader_mode =
> +			RMI_F01_STATUS_BOOTLOADER(device_status);
> +	if (driver_data->f01_bootloader_mode)
> +		dev_warn(&rmi_dev->dev,
> +			 "WARNING: RMI4 device is in bootloader mode!\n");
> +
> +

The logic here is a bit odd.  Would it make sense to put this warning in
the if condition below?  IIRC you can't have a configured device while
in bootloader mode.

>  	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
>  		dev_err(&fn->dev,
>  			"Device was reset during configuration process, status: %#02x!\n",
-Courtney

Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.

[Courtney Cavin]

On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
> Reflash functionality will need to unload the existing functions and
> rescan the PDT before starting reflash; then reload the functions
> afterwards.
> 
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> 
> ---
> 
>  drivers/input/rmi4/rmi_driver.c | 165 ++++++++++++++++++++++------------------
>  drivers/input/rmi4/rmi_driver.h |  22 +++---
>  2 files changed, 101 insertions(+), 86 deletions(-)
[...]
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[...]
> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
> -			u16 pdt_address);
> +#define RMI_SCAN_CONTINUE	0
> +#define RMI_SCAN_DONE		1
> +
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> +		 int (*callback)(struct rmi_device *rmi_dev,
> +				 void *ctx, const struct pdt_entry *entry));

I don't really like this callback.  The main reason for it is early
abort of PDT scanning, right?  It is really that beneficial?
>  
>  bool rmi_is_physical_driver(struct device_driver *);
>  int rmi_register_physical_driver(void);
> @@ -113,4 +109,10 @@ void rmi_unregister_physical_driver(void);
>  int rmi_register_f01_handler(void);
>  void rmi_unregister_f01_handler(void);
>  
> +int check_bootloader_mode(struct rmi_device *rmi_dev,
> +			  const struct pdt_entry *pdt);

This is a silly function name to put in a header. rmi_* perhaps?

> +void rmi_free_function_list(struct rmi_device *rmi_dev);
> +int rmi_driver_detect_functions(struct rmi_device *rmi_dev);
> +
> +
>  #endif

-Courtney

Re: [PATCH 05/05] input synaptics-rmi4: Add reflash support

[Courtney Cavin]

On Sat, Mar 08, 2014 at 03:29:55AM +0100, Christopher Heiny wrote:
> Add support for reflashing V5 bootloader firmwares into
> RMI4 devices.

Throughout this driver: I'm not sure of the name 'reflash'. Maybe just
'flash(ing)'?

> 
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> 
> ---
> 
>  drivers/input/rmi4/Kconfig         |   9 +
>  drivers/input/rmi4/Makefile        |   1 +
>  drivers/input/rmi4/rmi_bus.c       |   3 +
>  drivers/input/rmi4/rmi_driver.h    |  11 +
>  drivers/input/rmi4/rmi_fw_update.c | 961 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 985 insertions(+)
> 
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index d0c7b6e..9b88b6a 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -25,6 +25,15 @@ config RMI4_DEBUG
> 
>           If unsure, say N.
> 
> +config RMI4_FWLIB

Err. LIB?

> +       bool "RMI4 Firmware Update"
> +       depends on RMI4_CORE
> +       help
> +         Say Y here to enable in-kernel firmware update capability.
> +
> +         Add support to the RMI4 core to enable reflashing of device
> +         firmware.

Please provide more description here of what someone is supposed to do
to utilize this support, and what it actually does.  The term "update"
here is generic enough to cause some confusion. 

> +
>  config RMI4_I2C
>         tristate "RMI4 I2C Support"
>         depends on RMI4_CORE && I2C
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 5c6bad5..570ea80 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_RMI4_CORE) += rmi_core.o
>  rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
> +rmi_core-$(CONFIG_RMI4_FWLIB) += rmi_fw_update.o

Ok.  Now I'm thoroughly confused, and I haven't even gotten to the code
yet.  FWLIB => rmi_fw_update => rmi_reflash.  What are we doing again?

> 
>  # Function drivers
>  obj-$(CONFIG_RMI4_F11) += rmi_f11.o
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 6e0454a..3c93d08 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -117,6 +117,8 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
>         if (error)
>                 goto err_put_device;
> 
> +       rmi_reflash_init(rmi_dev);
> +
>         dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
>                 pdata->sensor_name, dev_name(&rmi_dev->dev));
> 
> @@ -139,6 +141,7 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
>         struct rmi_device *rmi_dev = xport->rmi_dev;
> 
>         device_del(&rmi_dev->dev);
> +       rmi_reflash_cleanup(rmi_dev);
>         rmi_physical_teardown_debugfs(rmi_dev);
>         put_device(&rmi_dev->dev);
>  }
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[...]
> +#ifdef CONFIG_RMI4_FWLIB
> +void rmi_reflash_init(struct rmi_device *rmi_dev);
> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev);
> +#else
> +#define rmi_reflash_init(rmi_dev)
> +#define rmi_reflash_cleanup(rmi_dev)

Please use static inline functions here.  It helps the compiler tell
you when you do something wrong.

> +#endif
> 
>  #endif
> diff --git a/drivers/input/rmi4/rmi_fw_update.c b/drivers/input/rmi4/rmi_fw_update.c
[...]
> +struct rmi_reflash_data {
> +       struct rmi_device *rmi_dev;
> +       bool force;
> +       ulong busy;
> +       char name_buf[RMI_NAME_BUFFER_SIZE];
> +       char *img_name;

const?

> +       struct pdt_entry f01_pdt;
> +       struct f01_basic_properties f01_props;
> +       u8 device_status;
> +       struct pdt_entry f34_pdt;
> +       u8 bootloader_id[2];
> +       struct rmi_f34_queries f34_queries;
> +       u16 f34_status_address;
> +       struct rmi_f34_control_status f34_controls;
> +       const u8 *firmware_data;
> +       const u8 *config_data;
> +       struct work_struct reflash_work;
> +};
[...]
> +static int rmi_read_f01_status(struct rmi_reflash_data *data)
> +{
> +       int retval;
> +
> +       retval = rmi_read(data->rmi_dev, data->f01_pdt.data_base_addr,
> +                         &data->device_status);
> +       if (retval)
> +               return retval;
> +
> +       return 0;
> +}

This function is used one time, just calls rmi_read... There's no need
to wrap this read.

[...]
> +static int rmi_wait_for_idle(struct rmi_reflash_data *data, int timeout_ms)
> +{
> +       int timeout_count = ((timeout_ms * 1000) / RMI_MAX_SLEEP_TIME_US) + 1;
> +       int count = 0;
> +       struct rmi_f34_control_status *controls = &data->f34_controls;
> +       int retval;
> +
> +       do {
> +               if (count || timeout_count == 1)
> +                       usleep_range(RMI_MIN_SLEEP_TIME_US,
> +                                    RMI_MAX_SLEEP_TIME_US);
> +               retval = rmi_read_f34_controls(data);
> +               count++;
> +               if (retval)
> +                       continue;
> +               else if (IS_IDLE(controls)) {
> +                       if (dev_WARN_ONCE(&data->rmi_dev->dev,
> +                                         !data->f34_controls.program_enabled,
> +                                         "Reflash is idle but program_enabled bit isn't set.\n"
> +                                         ))
> +                               /*
> +                                * This works around a bug in certain device
> +                                * firmwares, where the idle state is reached,
> +                                * but the program_enabled bit is not yet set.
> +                                */

If it's a bug in devices, but it's ok to try again as a workaround, is
there a good reason to print a stacktrace?  Does the user care?

> +                               continue;
> +                       return 0;
> +               }
> +       } while (count < timeout_count);
> +
> +       dev_err(&data->rmi_dev->dev,
> +               "ERROR: Timeout waiting for idle status.\n");
> +       dev_err(&data->rmi_dev->dev, "Command: %#04x\n", controls->command);
> +       dev_err(&data->rmi_dev->dev, "Status:  %#04x\n", controls->status);
> +       dev_err(&data->rmi_dev->dev, "Enabled: %d\n",
> +                       controls->program_enabled);
> +       dev_err(&data->rmi_dev->dev, "Idle:    %d\n", IS_IDLE(controls));
> +       return -ETIMEDOUT;
> +}
[...]
> +static int rmi_write_f34_command(struct rmi_reflash_data *data, u8 command)
> +{
> +       int retval;
> +       struct rmi_device *rmi_dev = data->rmi_dev;
> +
> +       retval = rmi_write(rmi_dev, data->f34_status_address, command);
> +       if (retval < 0) {
> +               dev_err(&rmi_dev->dev,
> +                       "Failed to write F34 command %#04x. Code: %d.\n",
> +                       command, retval);
> +               return retval;
> +       }
> +
> +       return 0;
> +}

This function is used three times, and calls one function without any
wrapping magic.  You could easily call rmi_write in each place.

[...]
> +static void rmi_reset_device(struct rmi_reflash_data *data)

It really feels like this should have an error return.

> +{
> +       int retval;
> +       const struct rmi_device_platform_data *pdata =
> +                               rmi_get_platform_data(data->rmi_dev);
> +
> +       dev_dbg(&data->rmi_dev->dev, "Resetting...\n");
> +       retval = rmi_write(data->rmi_dev, data->f01_pdt.command_base_addr,
> +                          RMI_F01_CMD_DEVICE_RESET);
> +       if (retval < 0)
> +               dev_warn(&data->rmi_dev->dev,
> +                        "WARNING - post-flash reset failed, code: %d.\n",
> +                        retval);
> +       msleep(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
> +       dev_dbg(&data->rmi_dev->dev, "Reset completed.\n");
> +}

> +static int rmi_write_firmware(struct rmi_reflash_data *data)
> +{
> +       return rmi_write_blocks(data, (u8 *) data->firmware_data,
> +               data->f34_queries.fw_block_count, RMI_F34_WRITE_FW_BLOCK);
> +}

Called once.

> +
> +static int rmi_write_configuration(struct rmi_reflash_data *data)
> +{
> +       return rmi_write_blocks(data, (u8 *) data->config_data,
> +               data->f34_queries.config_block_count,
> +               RMI_F34_WRITE_CONFIG_BLOCK);
> +}

Called once.

> +
> +static void rmi_reflash_firmware(struct rmi_reflash_data *data)

Also feels like this should have an error return.

[...]
> +static void rmi_fw_update(struct rmi_device *rmi_dev)

Same.

> +{
[...]
> +       retval = rmi_read_f34_queries(data);
> +       if (retval) {
> +               dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
> +                       retval);
> +               goto done;
> +       }
> +       if (data->img_name && strlen(data->img_name))

data->img_name && data->img_name[0] ?  No need to waste extra cycles.

> +               snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> +                        rmi_fw_name_format, data->img_name);
> +       else if (pdata->firmware_name && strlen(pdata->firmware_name))

Same.

> +               snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> +                        rmi_fw_name_format, pdata->firmware_name);
> +       else {
> +               if (!strlen(data->f01_props.product_id)) {

Same.

> +                       dev_err(&rmi_dev->dev, "Product ID is missing or empty - will not reflash.\n");
> +                       goto done;
> +               }
> +               snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> +                        rmi_fw_name_format, data->f01_props.product_id);
> +       }
[...]
> +       if (rmi_go_nogo(data, header)) {
> +               dev_dbg(&rmi_dev->dev, "Go/NoGo said go.\n");
> +               rmi_free_function_list(rmi_dev);
> +               rmi_reflash_firmware(data);
> +               rmi_reset_device(data);
> +               rmi_driver_detect_functions(rmi_dev);
> +       } else
> +               dev_dbg(&rmi_dev->dev, "Go/NoGo said don't reflash.\n");

Documentation/CodingStyle says:
} else {
	...
}

[...]
> +}
[...]
> +static ssize_t rmi_reflash_force_store(struct device *dev,
> +                                      struct device_attribute *attr,
> +                                      const char *buf, size_t count)
> +{
> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> +       struct rmi_reflash_data *data = drv_data->reflash_data;
> +       int retval;
> +       unsigned long val;
> +
> +       if (test_and_set_bit(0, &data->busy))
> +               return -EBUSY;
> +
> +       retval = kstrtoul(buf, 10, &val);
> +       if (retval)
> +               count = retval;
> +       else
> +               data->force = !!val;

Hrm. Perhaps '42' doesn't make sense here.  Maybe add:

else if (val > 1)
	count = -EINVAL;

> +
> +       clear_bit(0, &data->busy);
> +
> +       return count;
> +}
> +
> +static ssize_t rmi_reflash_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> +       struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> +       return snprintf(buf, PAGE_SIZE, "%u\n", test_bit(0, &data->busy));
> +}
> +
> +static ssize_t rmi_reflash_store(struct device *dev,
> +                                struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +       int retval;
> +       unsigned long val;
> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> +       struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> +       retval = kstrtoul(buf, 10, &val);
> +       if (retval)
> +               return retval;
> +
> +       if (test_and_set_bit(0, &data->busy))
> +               return -EBUSY;
> +
> +       if (val)
> +               /*
> +                * TODO: Here we start a work thread to go do the reflash, but
> +                * maybe we can just use request_firmware_timeout().
> +                */

Mmm.. Yes.  It would make the lifecycle of this busy bit much more
obvious.  Otherwise perhaps call request_firmware_nowait() ?  It already
does this work queue ... uh ... work for you.

> +               schedule_work(&data->reflash_work);
> +       else
> +               clear_bit(0, &data->busy);
> +
> +       return count;
> +}
[...]
> +void rmi_reflash_init(struct rmi_device *rmi_dev)
> +{
> +       int error;
> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> +       struct rmi_reflash_data *data;
> +
> +       dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
> +
> +       data = devm_kzalloc(&rmi_dev->dev, sizeof(struct rmi_reflash_data),
> +                           GFP_KERNEL);

The memory ownership here is odd.  Maybe kzalloc, and just return that
data?

> +
> +       error = sysfs_create_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
> +       if (error) {
> +               dev_warn(&rmi_dev->dev, "Failed to create reflash sysfs attributes.\n");
> +               return;
> +       }
> +
> +       INIT_WORK(&data->reflash_work, rmi_reflash_work);
> +       data->rmi_dev = rmi_dev;
> +       drv_data->reflash_data = data;
> +}
> +
> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev)
> +{
> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> +       struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> +       sysfs_remove_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
> +       devm_kfree(&rmi_dev->dev, data);

Who owns this memory again? devm_ doesn't seem right for this use-case.

> +}

-Courtney

Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[Christopher Heiny]

On 03/10/2014 07:46 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>>   drivers/input/rmi4/rmi_f01.c |  96 ++-----------------------------------
>>   drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 114 insertions(+), 92 deletions(-)
>
> $SUBJECT is 83 characters long.  Please be more brief and provide a
> description.

OK.

>
> [...]
>> +#define RMI_SLEEP_MODE_NORMAL		0x00
>> +#define RMI_SLEEP_MODE_SENSOR_SLEEP	0x01
>> +#define RMI_SLEEP_MODE_RESERVED0	0x02
>> +#define RMI_SLEEP_MODE_RESERVED1	0x03
>> +
>> +#define RMI_IS_VALID_SLEEPMODE(mode) \
>> +	(mode >= RMI_SLEEP_MODE_NORMAL && mode <= RMI_SLEEP_MODE_RESERVED1)
>> +
>
> I might be missing something, but these seem like the only defines used
> in the flash code.  Why not keep these in the f01 driver, and export
> a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?

It seems better to me to have the information defined in a single place, 
rather than scattered hither and yon through the source files.

Re: [PATCH 03/05] input synaptics-rmi4: rmi_f01 - Fix a comment and add a diagnostic output message.

[Christopher Heiny]

On 03/10/2014 07:51 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:53AM +0100, Christopher Heiny wrote:
>> In debugging certain touch sensor failures, it's useful to know
>> whether the device is stuck in bootloader, so print a message
>> to that effect.
>>
>> Also, point to the actual location of the defs for the F01 CTRL0
>> bitfields.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>>   drivers/input/rmi4/rmi_f01.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 8504865..a078d7d 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
>> @@ -16,7 +16,7 @@
>>   #include "rmi_f01.h"
>>
>>   /**
>> - * @ctrl0 - see the bit definitions above.
>> + * @ctrl0 - see the bit definitions in rmi_f01.h.
>>    * @doze_interval - controls the interval between checks for finger presence
>>    * when the touch sensor is in doze mode, in units of 10ms.
>>    * @wakeup_threshold - controls the capacitance threshold at which the touch
>> @@ -415,6 +415,13 @@ static int rmi_f01_probe(struct rmi_function *fn)
>>   		return error;
>>   	}
>>
>> +	driver_data->f01_bootloader_mode =
>> +			RMI_F01_STATUS_BOOTLOADER(device_status);
>> +	if (driver_data->f01_bootloader_mode)
>> +		dev_warn(&rmi_dev->dev,
>> +			 "WARNING: RMI4 device is in bootloader mode!\n");
>> +
>> +
>
> The logic here is a bit odd.  Would it make sense to put this warning in
> the if condition below?  IIRC you can't have a configured device while
> in bootloader mode.

Actually, you can write the configured bit (thus clearing the 
unconfigured bit) at any time, whether you're in bootloader mode or not.

>
>>   	if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
>>   		dev_err(&fn->dev,
>>   			"Device was reset during configuration process, status: %#02x!\n",
> -Courtney
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[Courtney Cavin]

On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
> On 03/10/2014 07:46 AM, Courtney Cavin wrote:
> > On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
> >> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >> Cc: Linux Walleij <linus.walleij@linaro.org>
> >> Cc: David Herrmann <dh.herrmann@gmail.com>
> >> Cc: Jiri Kosina <jkosina@suse.cz>
> >>
> >> ---
> >>
> >>   drivers/input/rmi4/rmi_f01.c |  96 ++-----------------------------------
> >>   drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 114 insertions(+), 92 deletions(-)
[...]
> >
> > I might be missing something, but these seem like the only defines used
> > in the flash code.  Why not keep these in the f01 driver, and export
> > a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
> 
> It seems better to me to have the information defined in a single place, 
> rather than scattered hither and yon through the source files.

Uh.  Exactly?  This is why I'm suggesting that you keep this information
isolated in the driver to which is directly related.

Perhaps what you mean is that the regs/bits for the entire chip
functionality should be exposed in header files, so one can read/write
it from anywhere?  That seems backwards to the idea of separating these
'functions' out into drivers.

-Courtney

Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.

[Christopher Heiny]

On 03/10/2014 08:04 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
>> Reflash functionality will need to unload the existing functions and
>> rescan the PDT before starting reflash; then reload the functions
>> afterwards.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>>   drivers/input/rmi4/rmi_driver.c | 165 ++++++++++++++++++++++------------------
>>   drivers/input/rmi4/rmi_driver.h |  22 +++---
>>   2 files changed, 101 insertions(+), 86 deletions(-)
> [...]
>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> [...]
>> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
>> -			u16 pdt_address);
>> +#define RMI_SCAN_CONTINUE	0
>> +#define RMI_SCAN_DONE		1
>> +
>> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
>> +		 int (*callback)(struct rmi_device *rmi_dev,
>> +				 void *ctx, const struct pdt_entry *entry));
>
> I don't really like this callback.  The main reason for it is early
> abort of PDT scanning, right?  It is really that beneficial?

Well, the main reason for adding this that there are several places 
where we perform a PDT scan, and they were proving vulnerable to 
cut-and-paste errors and code drift.  The boilerplate code of the 
process of doing a PDT scan was also obscuring the actual purpose of 
each PDT scan.

Early abort of the PDT scan is also important - in some of the scans you 
want to quit as soon as you've found the function(s) of interest, or if 
you detect that the device is still in bootloader mode.  Since there are 
256 possible RMI4 pages to scan, stopping early provides serious time 
savings at boot time and during the reflash process.  It also simplifies 
the code when the device comes up in bootloader mode.


>>
>>   bool rmi_is_physical_driver(struct device_driver *);
>>   int rmi_register_physical_driver(void);
>> @@ -113,4 +109,10 @@ void rmi_unregister_physical_driver(void);
>>   int rmi_register_f01_handler(void);
>>   void rmi_unregister_f01_handler(void);
>>
>> +int check_bootloader_mode(struct rmi_device *rmi_dev,
>> +			  const struct pdt_entry *pdt);
>
> This is a silly function name to put in a header. rmi_* perhaps?

Yes, I noticed that too while preparing the patch, along with a lot of 
other instances.  I decided to do an overall namespace cleanup later, 
and not piggyback it onto this particular patchset.  I'll fix this one 
if it's a blocking issue.

>
>> +void rmi_free_function_list(struct rmi_device *rmi_dev);
>> +int rmi_driver_detect_functions(struct rmi_device *rmi_dev);
>> +
>> +
>>   #endif
>
> -Courtney
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[Courtney Cavin]

On Mon, Mar 10, 2014 at 11:45:50PM +0100, Courtney Cavin wrote:
> On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
> > On 03/10/2014 07:46 AM, Courtney Cavin wrote:
> > > On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
> > >> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >> Cc: Linux Walleij <linus.walleij@linaro.org>
> > >> Cc: David Herrmann <dh.herrmann@gmail.com>
> > >> Cc: Jiri Kosina <jkosina@suse.cz>
> > >>
> > >> ---
> > >>
> > >>   drivers/input/rmi4/rmi_f01.c |  96 ++-----------------------------------
> > >>   drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
> > >>   2 files changed, 114 insertions(+), 92 deletions(-)
> [...]
> > >
> > > I might be missing something, but these seem like the only defines used
> > > in the flash code.  Why not keep these in the f01 driver, and export
> > > a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
> > 
> > It seems better to me to have the information defined in a single place, 
> > rather than scattered hither and yon through the source files.
> 
> Uh.  Exactly?  This is why I'm suggesting that you keep this information
> isolated in the driver to which is directly related.
> 
> Perhaps what you mean is that the regs/bits for the entire chip
> functionality should be exposed in header files, so one can read/write
> it from anywhere?  That seems backwards to the idea of separating these
> 'functions' out into drivers.

Ah.  Wait.  I think there was some mis-communication on my part.  What I
should have said:  Why not keep all of the defines in the driver, and
export a couple more functions?

My point is exactly yours.  Keep the defines with the code.  Expose
what's needed.

-Courtney

Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.

[Courtney Cavin]

On Mon, Mar 10, 2014 at 11:54:59PM +0100, Christopher Heiny wrote:
> On 03/10/2014 08:04 AM, Courtney Cavin wrote:
> > On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
> >> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> > [...]
> >> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
> >> -			u16 pdt_address);
> >> +#define RMI_SCAN_CONTINUE	0
> >> +#define RMI_SCAN_DONE		1
> >> +
> >> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> >> +		 int (*callback)(struct rmi_device *rmi_dev,
> >> +				 void *ctx, const struct pdt_entry *entry));
> >
> > I don't really like this callback.  The main reason for it is early
> > abort of PDT scanning, right?  It is really that beneficial?
> 
> Well, the main reason for adding this that there are several places 
> where we perform a PDT scan, and they were proving vulnerable to 
> cut-and-paste errors and code drift.  The boilerplate code of the 
> process of doing a PDT scan was also obscuring the actual purpose of 
> each PDT scan.
> 
> Early abort of the PDT scan is also important - in some of the scans you 
> want to quit as soon as you've found the function(s) of interest, or if 
> you detect that the device is still in bootloader mode.  Since there are 
> 256 possible RMI4 pages to scan, stopping early provides serious time 
> savings at boot time and during the reflash process.  It also simplifies 
> the code when the device comes up in bootloader mode.

Ok.  I'm not entirely familiar with the whole paging thing, as it wasn't
part of the spec when I was working with this stuff.  Is it not true
that you can cancel looking through pages when you encounter one with no
functions?  I guess it could be possible that there is a device with all
256 pages populated, where we could encounter a large enough time
difference though.  Bummer.

Maybe we can do something like the following:

struct rmi_pdt {
	u8 page;
	u8 offset;
};

int rmi_pdt_init(struct rmi_dev *dev, struct rmi_pdt *pdt);
int rmi_pdt_next(struct rmi_dev *dev, struct rmi_pdt *pdt,
		struct rmi_pdt_entry *e);

Where you can do:

	struct rmi_pdt_entry e;
	struct rmi_pdt pdt;

	rmi_pdt_init(dev, &pdt);

	while (!rmi_pdt_next(dev, &pdt, &e)) {
		do something; break when done
	}

With this, you can drive the scanning directly from where you want the
data, instead of playing with void pointers.  It's also hard to use
incorrectly, and easy to follow.

What do you think?

> >> +int check_bootloader_mode(struct rmi_device *rmi_dev,
> >> +			  const struct pdt_entry *pdt);
> >
> > This is a silly function name to put in a header. rmi_* perhaps?
> 
> Yes, I noticed that too while preparing the patch, along with a lot of 
> other instances.  I decided to do an overall namespace cleanup later, 
> and not piggyback it onto this particular patchset.  I'll fix this one 
> if it's a blocking issue.

I'd like it if we could fix this in this series, so we don't forget
later.

-Courtney

Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.

[Christopher Heiny]

On 03/10/2014 04:34 PM, Courtney Cavin wrote:
> On Mon, Mar 10, 2014 at 11:54:59PM +0100, Christopher Heiny wrote:
>> On 03/10/2014 08:04 AM, Courtney Cavin wrote:
>>> On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
>>>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
>>> [...]
>>>> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
>>>> -			u16 pdt_address);
>>>> +#define RMI_SCAN_CONTINUE	0
>>>> +#define RMI_SCAN_DONE		1
>>>> +
>>>> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
>>>> +		 int (*callback)(struct rmi_device *rmi_dev,
>>>> +				 void *ctx, const struct pdt_entry *entry));
>>>
>>> I don't really like this callback.  The main reason for it is early
>>> abort of PDT scanning, right?  It is really that beneficial?
>>
>> Well, the main reason for adding this that there are several places
>> where we perform a PDT scan, and they were proving vulnerable to
>> cut-and-paste errors and code drift.  The boilerplate code of the
>> process of doing a PDT scan was also obscuring the actual purpose of
>> each PDT scan.
>>
>> Early abort of the PDT scan is also important - in some of the scans you
>> want to quit as soon as you've found the function(s) of interest, or if
>> you detect that the device is still in bootloader mode.  Since there are
>> 256 possible RMI4 pages to scan, stopping early provides serious time
>> savings at boot time and during the reflash process.  It also simplifies
>> the code when the device comes up in bootloader mode.
>
> Ok.  I'm not entirely familiar with the whole paging thing, as it wasn't
> part of the spec when I was working with this stuff.  Is it not true
> that you can cancel looking through pages when you encounter one with no
> functions?  I guess it could be possible that there is a device with all
> 256 pages populated, where we could encounter a large enough time
> difference though.  Bummer.
>
> Maybe we can do something like the following:
>
> struct rmi_pdt {
> 	u8 page;
> 	u8 offset;
> };
>
> int rmi_pdt_init(struct rmi_dev *dev, struct rmi_pdt *pdt);
> int rmi_pdt_next(struct rmi_dev *dev, struct rmi_pdt *pdt,
> 		struct rmi_pdt_entry *e);
>
> Where you can do:
>
> 	struct rmi_pdt_entry e;
> 	struct rmi_pdt pdt;
>
> 	rmi_pdt_init(dev, &pdt);
>
> 	while (!rmi_pdt_next(dev, &pdt, &e)) {
> 		do something; break when done
> 	}
>
> With this, you can drive the scanning directly from where you want the
> data, instead of playing with void pointers.  It's also hard to use
> incorrectly, and easy to follow.
>
> What do you think?

Hmmmm.  I'd like to noodle with it a bit and see what the resulting code 
looks like.  Can we split that off into a separate patch set from the 
firmware update patches?


>
>>>> +int check_bootloader_mode(struct rmi_device *rmi_dev,
>>>> +			  const struct pdt_entry *pdt);
>>>
>>> This is a silly function name to put in a header. rmi_* perhaps?
>>
>> Yes, I noticed that too while preparing the patch, along with a lot of
>> other instances.  I decided to do an overall namespace cleanup later,
>> and not piggyback it onto this particular patchset.  I'll fix this one
>> if it's a blocking issue.
>
> I'd like it if we could fix this in this series, so we don't forget
> later.

OK.

Re: [PATCH 05/05] input synaptics-rmi4: Add reflash support

[Christopher Heiny]

On 03/10/2014 09:24 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:55AM +0100, Christopher Heiny wrote:
>> Add support for reflashing V5 bootloader firmwares into
>> RMI4 devices.
>
> Throughout this driver: I'm not sure of the name 'reflash'. Maybe just
> 'flash(ing)'?

That's a good point.  We use the terms 'flash', 'reflash', and 'update' 
pretty much interchangeably internally, and didn't realize it might 
cause confusion.  Using 'fw update' and similar terms would probably 
eliminate that.

>
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>>   drivers/input/rmi4/Kconfig         |   9 +
>>   drivers/input/rmi4/Makefile        |   1 +
>>   drivers/input/rmi4/rmi_bus.c       |   3 +
>>   drivers/input/rmi4/rmi_driver.h    |  11 +
>>   drivers/input/rmi4/rmi_fw_update.c | 961 +++++++++++++++++++++++++++++++++++++
>>   5 files changed, 985 insertions(+)
>>
>> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
>> index d0c7b6e..9b88b6a 100644
>> --- a/drivers/input/rmi4/Kconfig
>> +++ b/drivers/input/rmi4/Kconfig
>> @@ -25,6 +25,15 @@ config RMI4_DEBUG
>>
>>            If unsure, say N.
>>
>> +config RMI4_FWLIB
>
> Err. LIB?

It's an evolutionary vestige - originally this was a library module. 
I'll change it as part of the change mentioned above.

>
>> +       bool "RMI4 Firmware Update"
>> +       depends on RMI4_CORE
>> +       help
>> +         Say Y here to enable in-kernel firmware update capability.
>> +
>> +         Add support to the RMI4 core to enable reflashing of device
>> +         firmware.
>
> Please provide more description here of what someone is supposed to do
> to utilize this support, and what it actually does.  The term "update"
> here is generic enough to cause some confusion.

OK

>
>> +
>>   config RMI4_I2C
>>          tristate "RMI4 I2C Support"
>>          depends on RMI4_CORE && I2C
>> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
>> index 5c6bad5..570ea80 100644
>> --- a/drivers/input/rmi4/Makefile
>> +++ b/drivers/input/rmi4/Makefile
>> @@ -1,5 +1,6 @@
>>   obj-$(CONFIG_RMI4_CORE) += rmi_core.o
>>   rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
>> +rmi_core-$(CONFIG_RMI4_FWLIB) += rmi_fw_update.o
>
> Ok.  Now I'm thoroughly confused, and I haven't even gotten to the code
> yet.  FWLIB => rmi_fw_update => rmi_reflash.  What are we doing again?

See my first comment.

>
>>
>>   # Function drivers
>>   obj-$(CONFIG_RMI4_F11) += rmi_f11.o
>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>> index 6e0454a..3c93d08 100644
>> --- a/drivers/input/rmi4/rmi_bus.c
>> +++ b/drivers/input/rmi4/rmi_bus.c
>> @@ -117,6 +117,8 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
>>          if (error)
>>                  goto err_put_device;
>>
>> +       rmi_reflash_init(rmi_dev);
>> +
>>          dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
>>                  pdata->sensor_name, dev_name(&rmi_dev->dev));
>>
>> @@ -139,6 +141,7 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
>>          struct rmi_device *rmi_dev = xport->rmi_dev;
>>
>>          device_del(&rmi_dev->dev);
>> +       rmi_reflash_cleanup(rmi_dev);
>>          rmi_physical_teardown_debugfs(rmi_dev);
>>          put_device(&rmi_dev->dev);
>>   }
>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> [...]
>> +#ifdef CONFIG_RMI4_FWLIB
>> +void rmi_reflash_init(struct rmi_device *rmi_dev);
>> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev);
>> +#else
>> +#define rmi_reflash_init(rmi_dev)
>> +#define rmi_reflash_cleanup(rmi_dev)
>
> Please use static inline functions here.  It helps the compiler tell
> you when you do something wrong.

OK

>
>> +#endif
>>
>>   #endif
>> diff --git a/drivers/input/rmi4/rmi_fw_update.c b/drivers/input/rmi4/rmi_fw_update.c
> [...]
>> +struct rmi_reflash_data {
>> +       struct rmi_device *rmi_dev;
>> +       bool force;
>> +       ulong busy;
>> +       char name_buf[RMI_NAME_BUFFER_SIZE];
>> +       char *img_name;
>
> const?

OK

>
>> +       struct pdt_entry f01_pdt;
>> +       struct f01_basic_properties f01_props;
>> +       u8 device_status;
>> +       struct pdt_entry f34_pdt;
>> +       u8 bootloader_id[2];
>> +       struct rmi_f34_queries f34_queries;
>> +       u16 f34_status_address;
>> +       struct rmi_f34_control_status f34_controls;
>> +       const u8 *firmware_data;
>> +       const u8 *config_data;
>> +       struct work_struct reflash_work;
>> +};
> [...]
>> +static int rmi_read_f01_status(struct rmi_reflash_data *data)
>> +{
>> +       int retval;
>> +
>> +       retval = rmi_read(data->rmi_dev, data->f01_pdt.data_base_addr,
>> +                         &data->device_status);
>> +       if (retval)
>> +               return retval;
>> +
>> +       return 0;
>> +}
>
> This function is used one time, just calls rmi_read... There's no need
> to wrap this read.

OK, we'll unwrap this and the others.


>
> [...]
>> +static int rmi_wait_for_idle(struct rmi_reflash_data *data, int timeout_ms)
>> +{
>> +       int timeout_count = ((timeout_ms * 1000) / RMI_MAX_SLEEP_TIME_US) + 1;
>> +       int count = 0;
>> +       struct rmi_f34_control_status *controls = &data->f34_controls;
>> +       int retval;
>> +
>> +       do {
>> +               if (count || timeout_count == 1)
>> +                       usleep_range(RMI_MIN_SLEEP_TIME_US,
>> +                                    RMI_MAX_SLEEP_TIME_US);
>> +               retval = rmi_read_f34_controls(data);
>> +               count++;
>> +               if (retval)
>> +                       continue;
>> +               else if (IS_IDLE(controls)) {
>> +                       if (dev_WARN_ONCE(&data->rmi_dev->dev,
>> +                                         !data->f34_controls.program_enabled,
>> +                                         "Reflash is idle but program_enabled bit isn't set.\n"
>> +                                         ))
>> +                               /*
>> +                                * This works around a bug in certain device
>> +                                * firmwares, where the idle state is reached,
>> +                                * but the program_enabled bit is not yet set.
>> +                                */
>
> If it's a bug in devices, but it's ok to try again as a workaround, is
> there a good reason to print a stacktrace?  Does the user care?

On some devices with this bug, you can generate a lot of log spam by 
warning on each occurrence, possibly wrapping the log to the point where 
any other information is lost on devices with limited buffer.  I felt 
the stack trace was a small overhead to pay.

>> +                               continue;
>> +                       return 0;
>> +               }
>> +       } while (count < timeout_count);
>> +
>> +       dev_err(&data->rmi_dev->dev,
>> +               "ERROR: Timeout waiting for idle status.\n");
>> +       dev_err(&data->rmi_dev->dev, "Command: %#04x\n", controls->command);
>> +       dev_err(&data->rmi_dev->dev, "Status:  %#04x\n", controls->status);
>> +       dev_err(&data->rmi_dev->dev, "Enabled: %d\n",
>> +                       controls->program_enabled);
>> +       dev_err(&data->rmi_dev->dev, "Idle:    %d\n", IS_IDLE(controls));
>> +       return -ETIMEDOUT;
>> +}
> [...]
>> +static int rmi_write_f34_command(struct rmi_reflash_data *data, u8 command)
>> +{
>> +       int retval;
>> +       struct rmi_device *rmi_dev = data->rmi_dev;
>> +
>> +       retval = rmi_write(rmi_dev, data->f34_status_address, command);
>> +       if (retval < 0) {
>> +               dev_err(&rmi_dev->dev,
>> +                       "Failed to write F34 command %#04x. Code: %d.\n",
>> +                       command, retval);
>> +               return retval;
>> +       }
>> +
>> +       return 0;
>> +}
>
> This function is used three times, and calls one function without any
> wrapping magic.  You could easily call rmi_write in each place.
>
> [...]
>> +static void rmi_reset_device(struct rmi_reflash_data *data)
>
> It really feels like this should have an error return.

I'm not sure that buys us anything.  Regardless of whether we fail or 
succeed, we need to execute the next step after this was called
(rediscover functions), and in the failure case we've already printed an 
appropriate diagnostic message in this routine.

>
>> +{
>> +       int retval;
>> +       const struct rmi_device_platform_data *pdata =
>> +                               rmi_get_platform_data(data->rmi_dev);
>> +
>> +       dev_dbg(&data->rmi_dev->dev, "Resetting...\n");
>> +       retval = rmi_write(data->rmi_dev, data->f01_pdt.command_base_addr,
>> +                          RMI_F01_CMD_DEVICE_RESET);
>> +       if (retval < 0)
>> +               dev_warn(&data->rmi_dev->dev,
>> +                        "WARNING - post-flash reset failed, code: %d.\n",
>> +                        retval);
>> +       msleep(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
>> +       dev_dbg(&data->rmi_dev->dev, "Reset completed.\n");
>> +}
>
>> +static int rmi_write_firmware(struct rmi_reflash_data *data)
>> +{
>> +       return rmi_write_blocks(data, (u8 *) data->firmware_data,
>> +               data->f34_queries.fw_block_count, RMI_F34_WRITE_FW_BLOCK);
>> +}
>
> Called once.
>
>> +
>> +static int rmi_write_configuration(struct rmi_reflash_data *data)
>> +{
>> +       return rmi_write_blocks(data, (u8 *) data->config_data,
>> +               data->f34_queries.config_block_count,
>> +               RMI_F34_WRITE_CONFIG_BLOCK);
>> +}
>
> Called once.
>
>> +
>> +static void rmi_reflash_firmware(struct rmi_reflash_data *data)
>
> Also feels like this should have an error return.

I'm not sure that buys us anything.  Regardless of whether we fail or 
succeed, we need to execute the next two steps after this is called
(reset the device and rediscover functions), and in the failure case 
we've already printed an appropriate diagnostic message closer to the 
actual point of failure.

>
> [...]
>> +static void rmi_fw_update(struct rmi_device *rmi_dev)
>
> Same.

Not sure it's needed here - if we get to the end of this, we're all done 
and there's nothing else we can do if the update didn't work.  We've 
already printed any appropriate diagnostic messages closer to the point 
of failure.

>
>> +{
> [...]
>> +       retval = rmi_read_f34_queries(data);
>> +       if (retval) {
>> +               dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
>> +                       retval);
>> +               goto done;
>> +       }
>> +       if (data->img_name && strlen(data->img_name))
>
> data->img_name && data->img_name[0] ?  No need to waste extra cycles.

That's tidy.  Will apply this here and also as below.

>
>> +               snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
>> +                        rmi_fw_name_format, data->img_name);
>> +       else if (pdata->firmware_name && strlen(pdata->firmware_name))
>
> Same.
>
>> +               snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
>> +                        rmi_fw_name_format, pdata->firmware_name);
>> +       else {
>> +               if (!strlen(data->f01_props.product_id)) {
>
> Same.
>
>> +                       dev_err(&rmi_dev->dev, "Product ID is missing or empty - will not reflash.\n");
>> +                       goto done;
>> +               }
>> +               snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
>> +                        rmi_fw_name_format, data->f01_props.product_id);
>> +       }
> [...]
>> +       if (rmi_go_nogo(data, header)) {
>> +               dev_dbg(&rmi_dev->dev, "Go/NoGo said go.\n");
>> +               rmi_free_function_list(rmi_dev);
>> +               rmi_reflash_firmware(data);
>> +               rmi_reset_device(data);
>> +               rmi_driver_detect_functions(rmi_dev);
>> +       } else
>> +               dev_dbg(&rmi_dev->dev, "Go/NoGo said don't reflash.\n");
>
> Documentation/CodingStyle says:
> } else {

OK

> 	...
> }
>
> [...]
>> +}
> [...]
>> +static ssize_t rmi_reflash_force_store(struct device *dev,
>> +                                      struct device_attribute *attr,
>> +                                      const char *buf, size_t count)
>> +{
>> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
>> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> +       struct rmi_reflash_data *data = drv_data->reflash_data;
>> +       int retval;
>> +       unsigned long val;
>> +
>> +       if (test_and_set_bit(0, &data->busy))
>> +               return -EBUSY;
>> +
>> +       retval = kstrtoul(buf, 10, &val);
>> +       if (retval)
>> +               count = retval;
>> +       else
>> +               data->force = !!val;
>
> Hrm. Perhaps '42' doesn't make sense here.  Maybe add:
>
> else if (val > 1)
> 	count = -EINVAL;

OK.


>
>> +
>> +       clear_bit(0, &data->busy);
>> +
>> +       return count;
>> +}
>> +
>> +static ssize_t rmi_reflash_show(struct device *dev,
>> +                               struct device_attribute *attr, char *buf)
>> +{
>> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
>> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> +       struct rmi_reflash_data *data = drv_data->reflash_data;
>> +
>> +       return snprintf(buf, PAGE_SIZE, "%u\n", test_bit(0, &data->busy));
>> +}
>> +
>> +static ssize_t rmi_reflash_store(struct device *dev,
>> +                                struct device_attribute *attr,
>> +                                const char *buf, size_t count)
>> +{
>> +       int retval;
>> +       unsigned long val;
>> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
>> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> +       struct rmi_reflash_data *data = drv_data->reflash_data;
>> +
>> +       retval = kstrtoul(buf, 10, &val);
>> +       if (retval)
>> +               return retval;
>> +
>> +       if (test_and_set_bit(0, &data->busy))
>> +               return -EBUSY;
>> +
>> +       if (val)
>> +               /*
>> +                * TODO: Here we start a work thread to go do the reflash, but
>> +                * maybe we can just use request_firmware_timeout().
>> +                */
>
> Mmm.. Yes.  It would make the lifecycle of this busy bit much more
> obvious.  Otherwise perhaps call request_firmware_nowait() ?  It already
> does this work queue ... uh ... work for you.

Yeah :-)

>
>> +               schedule_work(&data->reflash_work);
>> +       else
>> +               clear_bit(0, &data->busy);
>> +
>> +       return count;
>> +}
> [...]
>> +void rmi_reflash_init(struct rmi_device *rmi_dev)
>> +{
>> +       int error;
>> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> +       struct rmi_reflash_data *data;
>> +
>> +       dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
>> +
>> +       data = devm_kzalloc(&rmi_dev->dev, sizeof(struct rmi_reflash_data),
>> +                           GFP_KERNEL);
>
> The memory ownership here is odd.  Maybe kzalloc, and just return that
> data?

That would work.  I'll switch it to kzalloc/kfree.


>> +
>> +       error = sysfs_create_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
>> +       if (error) {
>> +               dev_warn(&rmi_dev->dev, "Failed to create reflash sysfs attributes.\n");
>> +               return;
>> +       }
>> +
>> +       INIT_WORK(&data->reflash_work, rmi_reflash_work);
>> +       data->rmi_dev = rmi_dev;
>> +       drv_data->reflash_data = data;
>> +}
>> +
>> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev)
>> +{
>> +       struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> +       struct rmi_reflash_data *data = drv_data->reflash_data;
>> +
>> +       sysfs_remove_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
>> +       devm_kfree(&rmi_dev->dev, data);
>
> Who owns this memory again? devm_ doesn't seem right for this use-case.

See above.

Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.

[Christopher Heiny]

On 03/10/2014 03:57 PM, Courtney Cavin wrote:
> On Mon, Mar 10, 2014 at 11:45:50PM +0100, Courtney Cavin wrote:
>> On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
>>> On 03/10/2014 07:46 AM, Courtney Cavin wrote:
>>>> On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
>>>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>>>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>>> Cc: Linux Walleij <linus.walleij@linaro.org>
>>>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>>>> Cc: Jiri Kosina <jkosina@suse.cz>
>>>>>
>>>>> ---
>>>>>
>>>>>    drivers/input/rmi4/rmi_f01.c |  96 ++-----------------------------------
>>>>>    drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
>>>>>    2 files changed, 114 insertions(+), 92 deletions(-)
>> [...]
>>>>
>>>> I might be missing something, but these seem like the only defines used
>>>> in the flash code.  Why not keep these in the f01 driver, and export
>>>> a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
>>>
>>> It seems better to me to have the information defined in a single place,
>>> rather than scattered hither and yon through the source files.
>>
>> Uh.  Exactly?  This is why I'm suggesting that you keep this information
>> isolated in the driver to which is directly related.
>>
>> Perhaps what you mean is that the regs/bits for the entire chip
>> functionality should be exposed in header files, so one can read/write
>> it from anywhere?  That seems backwards to the idea of separating these
>> 'functions' out into drivers.
>
> Ah.  Wait.  I think there was some mis-communication on my part.  What I
> should have said:  Why not keep all of the defines in the driver, and
> export a couple more functions?
>
> My point is exactly yours.  Keep the defines with the code.  Expose
> what's needed.

I don't see any win to that approach. For example, you can hide the 
sleep mode mask and the nosleep bit #defines in rmi_f01.c, but you've 
got to add a new function with either a tri-state field for the nosleep 
bit (set it, clear it, don't change it) which will require an additional 
3 #defines and the new sleep mode which still needs the sleep mode 
#defines (plus an additional one to indicate that you want to leave the 
sleep mode as it was), or 3 new functions, once of which changes the 
sleep mode, one of which sets/clears the nosleep bit, and one of which 
does both. In either case, you still need the sleep mode #defines, so 
some of the related stuff is in the header and some of it is in the .c 
file.  Now you wind up with more stuff in the .h file (at least twice as 
much as you've removed) and you no longer have one stop shopping for the 
F01_Ctrl0 #defines.

BTW - thanks very much for all of today's input.  Even though we don't 
necessarily agree with all of it, it's been helpful to go back and ask 
"did we make the right decision?".

					Chris