[PATCH] input synaptics-rmi4: PDT scan cleanup

[PATCH] input synaptics-rmi4: PDT scan cleanup

[Christopher Heiny]

Eliminates copy-paste code that handled scans of the Page Descriptor Table,
replacing it with a single PDT scan routine that invokes a callback function.
The scan routine is not static so that it can be used by the firmware update
code (under development, not yet submitted).

Updated the copyright dates while we were at it.

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

---

 drivers/input/rmi4/rmi_driver.c | 155 ++++++++++++++++++++--------------------
 drivers/input/rmi4/rmi_driver.h |   6 +-
 2 files changed, 83 insertions(+), 78 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index eb790ff..cbd6485 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011-2013 Synaptics Incorporated
+ * Copyright (c) 2011-2014 Synaptics Incorporated
  * Copyright (c) 2011 Unixphere
  *
  * This driver provides the core support for a single RMI4-based device.
@@ -566,85 +566,80 @@ err_free_mem:
 	return error;
 }
 
-/*
- * Scan the PDT for F01 so we can force a reset before anything else
- * is done.  This forces the sensor into a known state, and also
- * forces application of any pending updates from reflashing the
- * firmware or configuration.
- *
- * We have to do this before actually building the PDT because the reflash
- * updates (if any) might cause various registers to move around.
- */
-static int rmi_initial_reset(struct rmi_device *rmi_dev)
+
+#define RMI_SCAN_CONTINUE	0
+#define RMI_SCAN_DONE		1
+
+static int rmi_initial_reset(struct rmi_device *rmi_dev,
+		void *clbk_ctx, struct pdt_entry *pdt_entry, int page)
 {
-	struct pdt_entry pdt_entry;
-	int page;
-	struct device *dev = &rmi_dev->dev;
-	bool done = false;
-	bool has_f01 = false;
-	int i;
 	int retval;
-	const struct rmi_device_platform_data *pdata =
-			to_rmi_platform_data(rmi_dev);
 
-	dev_dbg(dev, "Initial reset.\n");
+	if (pdt_entry->function_number == 0x01) {
+		u16 cmd_addr = page + pdt_entry->command_base_addr;
+		u8 cmd_buf = RMI_DEVICE_RESET_CMD;
+		const struct rmi_device_platform_data *pdata =
+				to_rmi_platform_data(rmi_dev);
 
-	for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
-		u16 page_start = RMI4_PAGE_SIZE * page;
-		u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
-		u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
+		retval = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
+		if (retval < 0) {
+			dev_err(&rmi_dev->dev,
+				"Initial reset failed. Code = %d.\n", retval);
+			return retval;
+		}
+		mdelay(pdata->reset_delay_ms);
 
-		done = true;
-		for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
-			retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
-			if (retval < 0)
-				return retval;
+		return RMI_SCAN_DONE;
+	}
 
-			if (RMI4_END_OF_PDT(pdt_entry.function_number))
-				break;
-			done = false;
+	/* F01 should always be on page 0. If we don't find it there, fail. */
+	return (!page) ? RMI_SCAN_CONTINUE : -ENODEV;
+}
 
-			if (pdt_entry.function_number == 0x01) {
-				u16 cmd_addr = page_start +
-					pdt_entry.command_base_addr;
-				u8 cmd_buf = RMI_DEVICE_RESET_CMD;
-				retval = rmi_write_block(rmi_dev, cmd_addr,
-						&cmd_buf, 1);
-				if (retval < 0) {
-					dev_err(dev, "Initial reset failed. Code = %d.\n",
-						retval);
-					return retval;
-				}
-				mdelay(pdata->reset_delay_ms);
-				done = true;
-				has_f01 = true;
-				break;
-			}
-		}
-	}
+static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
+		void *clbk_ctx, struct pdt_entry *entry, int page)
+{
+	int *irq_count = (int *)clbk_ctx;
 
-	if (!has_f01) {
-		dev_warn(dev, "WARNING: Failed to find F01 for initial reset.\n");
-		return -ENODEV;
-	}
+	return create_function(rmi_dev, entry, irq_count,
+				RMI4_PAGE_SIZE * page);
+}
+
+static int rmi_create_functions(struct rmi_device *rmi_dev)
+{
+	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
+	struct device *dev = &rmi_dev->dev;
+	int irq_count = 0;
+	int retval;
+
+	// XXX need to make sure we create F01 first...
+	// XXX or do we?  It might not be required in the updated structure.
+	retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_functions_clbk);
+
+	if (retval)
+		return retval;
+
+	// TODO: I think we need to count the IRQs before creating the
+	// functions.
+	data->irq_count = irq_count;
+	data->num_of_irq_regs = (irq_count + 7) / 8;
 
 	return 0;
 }
 
-static int rmi_scan_pdt(struct rmi_device *rmi_dev)
+int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
+		int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
+		void *clbk_ctx, struct pdt_entry *entry, int page))
 {
-	struct rmi_driver_data *data;
+	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
 	struct pdt_entry pdt_entry;
 	int page;
-	struct device *dev = &rmi_dev->dev;
-	int irq_count = 0;
-	bool done = false;
 	int i;
-	int retval;
-
-	dev_dbg(dev, "Scanning PDT...\n");
+	bool done = false;
+	int retval = 0;
 
-	data = dev_get_drvdata(&rmi_dev->dev);
+	// TODO: With F01 and reflash as part of the core now, is this
+	// lock still required?
 	mutex_lock(&data->pdt_mutex);
 
 	for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
@@ -656,27 +651,27 @@ static int rmi_scan_pdt(struct rmi_device *rmi_dev)
 		for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
 			retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
 			if (retval < 0)
-				goto error_exit;
+				return retval;
 
 			if (RMI4_END_OF_PDT(pdt_entry.function_number))
 				break;
 
-			dev_dbg(dev, "Found F%02X on page %#04x\n",
+			dev_dbg(&rmi_dev->dev, "Found F%02X on page %#04x\n",
 					pdt_entry.function_number, page);
 			done = false;
 
-			// XXX need to make sure we create F01 first...
-			retval = create_function(rmi_dev,
-					&pdt_entry, &irq_count, page_start);
-
-			if (retval)
+			retval = rmi_pdt_scan_clbk(rmi_dev, ctx,
+							&pdt_entry, page);
+			if (retval < 0) {
 				goto error_exit;
+			} else if (retval == RMI_SCAN_DONE) {
+				done = true;
+				break;
+			}
 		}
 		done = done || data->f01_bootloader_mode;
 	}
-	data->irq_count = irq_count;
-	data->num_of_irq_regs = (irq_count + 7) / 8;
-	dev_dbg(dev, "%s: Done with PDT scan.\n", __func__);
+
 	retval = 0;
 
 error_exit:
@@ -684,6 +679,7 @@ error_exit:
 	return retval;
 }
 
+
 #ifdef CONFIG_PM_SLEEP
 static int rmi_driver_suspend(struct device *dev)
 {
@@ -797,10 +793,15 @@ static int rmi_driver_probe(struct device *dev)
 
 	/*
 	 * Right before a warm boot, the sensor might be in some unusual state,
-	 * such as F54 diagnostics, or F34 bootloader mode.  In order to clear
-	 * the sensor to a known state, we issue a initial reset to clear any
+	 * such as F54 diagnostics, or F34 bootloader mode after a firmware
+	 * or configuration update.  In order to clear the sensor to a known
+	 * state and/or apply any updates, we issue a initial reset to clear any
 	 * previous settings and force it into normal operation.
 	 *
+	 * We have to do this before actually building the PDT because
+	 * the reflash updates (if any) might cause various registers to move
+	 * around.
+	 *
 	 * For a number of reasons, this initial reset may fail to return
 	 * within the specified time, but we'll still be able to bring up the
 	 * driver normally after that failure.  This occurs most commonly in
@@ -813,11 +814,11 @@ static int rmi_driver_probe(struct device *dev)
 	 */
 	if (!pdata->reset_delay_ms)
 		pdata->reset_delay_ms = DEFAULT_RESET_DELAY_MS;
-	retval = rmi_initial_reset(rmi_dev);
+	retval = rmi_scan_pdt(rmi_dev, NULL, rmi_initial_reset);
 	if (retval)
 		dev_warn(dev, "RMI initial reset failed! Continuing in spite of this.\n");
 
-	retval = rmi_scan_pdt(rmi_dev);
+	retval = rmi_create_functions(rmi_dev);
 	if (retval) {
 		dev_err(dev, "PDT scan for %s failed with code %d.\n",
 			pdata->sensor_name, retval);
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index df9ddd8..f73be73 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011-2013 Synaptics Incorporated
+ * Copyright (c) 2011-2014 Synaptics Incorporated
  * Copyright (c) 2011 Unixphere
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -108,6 +108,10 @@ struct pdt_entry {
 int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
 			u16 pdt_address);
 
+int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
+	int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
+	void *clbk_ctx, struct pdt_entry *entry, int page));
+
 bool rmi_is_physical_driver(struct device_driver *);
 int rmi_register_physical_driver(void);
 void rmi_unregister_physical_driver(void);

Re: [PATCH] input synaptics-rmi4: PDT scan cleanup

[Christopher Heiny]

On 01/07/2014 12:33 PM, Christopher Heiny wrote:
> Eliminates copy-paste code that handled scans of the Page Descriptor Table,
> replacing it with a single PDT scan routine that invokes a callback function.
> The scan routine is not static so that it can be used by the firmware update
> code (under development, not yet submitted).
>
> Updated the copyright dates while we were at it.

Hi Dmitry,

Could you apply this or provide some feedback on it?  We've got a 
pending patch that depends on it, and that pending work will bring the 
driver back to a working (if not necessarily beautiful) state.  I don't 
want to submit it if this change isn't satisfactory, though.

					Thanks,
						Chris

>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
>
>   drivers/input/rmi4/rmi_driver.c | 155 ++++++++++++++++++++--------------------
>   drivers/input/rmi4/rmi_driver.h |   6 +-
>   2 files changed, 83 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index eb790ff..cbd6485 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
>    * Copyright (c) 2011 Unixphere
>    *
>    * This driver provides the core support for a single RMI4-based device.
> @@ -566,85 +566,80 @@ err_free_mem:
>   	return error;
>   }
>
> -/*
> - * Scan the PDT for F01 so we can force a reset before anything else
> - * is done.  This forces the sensor into a known state, and also
> - * forces application of any pending updates from reflashing the
> - * firmware or configuration.
> - *
> - * We have to do this before actually building the PDT because the reflash
> - * updates (if any) might cause various registers to move around.
> - */
> -static int rmi_initial_reset(struct rmi_device *rmi_dev)
> +
> +#define RMI_SCAN_CONTINUE	0
> +#define RMI_SCAN_DONE		1
> +
> +static int rmi_initial_reset(struct rmi_device *rmi_dev,
> +		void *clbk_ctx, struct pdt_entry *pdt_entry, int page)
>   {
> -	struct pdt_entry pdt_entry;
> -	int page;
> -	struct device *dev = &rmi_dev->dev;
> -	bool done = false;
> -	bool has_f01 = false;
> -	int i;
>   	int retval;
> -	const struct rmi_device_platform_data *pdata =
> -			to_rmi_platform_data(rmi_dev);
>
> -	dev_dbg(dev, "Initial reset.\n");
> +	if (pdt_entry->function_number == 0x01) {
> +		u16 cmd_addr = page + pdt_entry->command_base_addr;
> +		u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> +		const struct rmi_device_platform_data *pdata =
> +				to_rmi_platform_data(rmi_dev);
>
> -	for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> -		u16 page_start = RMI4_PAGE_SIZE * page;
> -		u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
> -		u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
> +		retval = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
> +		if (retval < 0) {
> +			dev_err(&rmi_dev->dev,
> +				"Initial reset failed. Code = %d.\n", retval);
> +			return retval;
> +		}
> +		mdelay(pdata->reset_delay_ms);
>
> -		done = true;
> -		for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
> -			retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
> -			if (retval < 0)
> -				return retval;
> +		return RMI_SCAN_DONE;
> +	}
>
> -			if (RMI4_END_OF_PDT(pdt_entry.function_number))
> -				break;
> -			done = false;
> +	/* F01 should always be on page 0. If we don't find it there, fail. */
> +	return (!page) ? RMI_SCAN_CONTINUE : -ENODEV;
> +}
>
> -			if (pdt_entry.function_number == 0x01) {
> -				u16 cmd_addr = page_start +
> -					pdt_entry.command_base_addr;
> -				u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> -				retval = rmi_write_block(rmi_dev, cmd_addr,
> -						&cmd_buf, 1);
> -				if (retval < 0) {
> -					dev_err(dev, "Initial reset failed. Code = %d.\n",
> -						retval);
> -					return retval;
> -				}
> -				mdelay(pdata->reset_delay_ms);
> -				done = true;
> -				has_f01 = true;
> -				break;
> -			}
> -		}
> -	}
> +static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
> +		void *clbk_ctx, struct pdt_entry *entry, int page)
> +{
> +	int *irq_count = (int *)clbk_ctx;
>
> -	if (!has_f01) {
> -		dev_warn(dev, "WARNING: Failed to find F01 for initial reset.\n");
> -		return -ENODEV;
> -	}
> +	return create_function(rmi_dev, entry, irq_count,
> +				RMI4_PAGE_SIZE * page);
> +}
> +
> +static int rmi_create_functions(struct rmi_device *rmi_dev)
> +{
> +	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> +	struct device *dev = &rmi_dev->dev;
> +	int irq_count = 0;
> +	int retval;
> +
> +	// XXX need to make sure we create F01 first...
> +	// XXX or do we?  It might not be required in the updated structure.
> +	retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_functions_clbk);
> +
> +	if (retval)
> +		return retval;
> +
> +	// TODO: I think we need to count the IRQs before creating the
> +	// functions.
> +	data->irq_count = irq_count;
> +	data->num_of_irq_regs = (irq_count + 7) / 8;
>
>   	return 0;
>   }
>
> -static int rmi_scan_pdt(struct rmi_device *rmi_dev)
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> +		int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
> +		void *clbk_ctx, struct pdt_entry *entry, int page))
>   {
> -	struct rmi_driver_data *data;
> +	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>   	struct pdt_entry pdt_entry;
>   	int page;
> -	struct device *dev = &rmi_dev->dev;
> -	int irq_count = 0;
> -	bool done = false;
>   	int i;
> -	int retval;
> -
> -	dev_dbg(dev, "Scanning PDT...\n");
> +	bool done = false;
> +	int retval = 0;
>
> -	data = dev_get_drvdata(&rmi_dev->dev);
> +	// TODO: With F01 and reflash as part of the core now, is this
> +	// lock still required?
>   	mutex_lock(&data->pdt_mutex);
>
>   	for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> @@ -656,27 +651,27 @@ static int rmi_scan_pdt(struct rmi_device *rmi_dev)
>   		for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
>   			retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
>   			if (retval < 0)
> -				goto error_exit;
> +				return retval;
>
>   			if (RMI4_END_OF_PDT(pdt_entry.function_number))
>   				break;
>
> -			dev_dbg(dev, "Found F%02X on page %#04x\n",
> +			dev_dbg(&rmi_dev->dev, "Found F%02X on page %#04x\n",
>   					pdt_entry.function_number, page);
>   			done = false;
>
> -			// XXX need to make sure we create F01 first...
> -			retval = create_function(rmi_dev,
> -					&pdt_entry, &irq_count, page_start);
> -
> -			if (retval)
> +			retval = rmi_pdt_scan_clbk(rmi_dev, ctx,
> +							&pdt_entry, page);
> +			if (retval < 0) {
>   				goto error_exit;
> +			} else if (retval == RMI_SCAN_DONE) {
> +				done = true;
> +				break;
> +			}
>   		}
>   		done = done || data->f01_bootloader_mode;
>   	}
> -	data->irq_count = irq_count;
> -	data->num_of_irq_regs = (irq_count + 7) / 8;
> -	dev_dbg(dev, "%s: Done with PDT scan.\n", __func__);
> +
>   	retval = 0;
>
>   error_exit:
> @@ -684,6 +679,7 @@ error_exit:
>   	return retval;
>   }
>
> +
>   #ifdef CONFIG_PM_SLEEP
>   static int rmi_driver_suspend(struct device *dev)
>   {
> @@ -797,10 +793,15 @@ static int rmi_driver_probe(struct device *dev)
>
>   	/*
>   	 * Right before a warm boot, the sensor might be in some unusual state,
> -	 * such as F54 diagnostics, or F34 bootloader mode.  In order to clear
> -	 * the sensor to a known state, we issue a initial reset to clear any
> +	 * such as F54 diagnostics, or F34 bootloader mode after a firmware
> +	 * or configuration update.  In order to clear the sensor to a known
> +	 * state and/or apply any updates, we issue a initial reset to clear any
>   	 * previous settings and force it into normal operation.
>   	 *
> +	 * We have to do this before actually building the PDT because
> +	 * the reflash updates (if any) might cause various registers to move
> +	 * around.
> +	 *
>   	 * For a number of reasons, this initial reset may fail to return
>   	 * within the specified time, but we'll still be able to bring up the
>   	 * driver normally after that failure.  This occurs most commonly in
> @@ -813,11 +814,11 @@ static int rmi_driver_probe(struct device *dev)
>   	 */
>   	if (!pdata->reset_delay_ms)
>   		pdata->reset_delay_ms = DEFAULT_RESET_DELAY_MS;
> -	retval = rmi_initial_reset(rmi_dev);
> +	retval = rmi_scan_pdt(rmi_dev, NULL, rmi_initial_reset);
>   	if (retval)
>   		dev_warn(dev, "RMI initial reset failed! Continuing in spite of this.\n");
>
> -	retval = rmi_scan_pdt(rmi_dev);
> +	retval = rmi_create_functions(rmi_dev);
>   	if (retval) {
>   		dev_err(dev, "PDT scan for %s failed with code %d.\n",
>   			pdata->sensor_name, retval);
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> index df9ddd8..f73be73 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
>    * Copyright (c) 2011 Unixphere
>    *
>    * This program is free software; you can redistribute it and/or modify it
> @@ -108,6 +108,10 @@ struct pdt_entry {
>   int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
>   			u16 pdt_address);
>
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> +	int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
> +	void *clbk_ctx, struct pdt_entry *entry, int page));
> +
>   bool rmi_is_physical_driver(struct device_driver *);
>   int rmi_register_physical_driver(void);
>   void rmi_unregister_physical_driver(void);
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated

Re: [PATCH] input synaptics-rmi4: PDT scan cleanup

[Dmitry Torokhov]

On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
> On 01/07/2014 12:33 PM, Christopher Heiny wrote:
> > Eliminates copy-paste code that handled scans of the Page Descriptor
> > Table,
> > replacing it with a single PDT scan routine that invokes a callback
> > function. The scan routine is not static so that it can be used by the
> > firmware update code (under development, not yet submitted).
> > 
> > Updated the copyright dates while we were at it.
> 
> Hi Dmitry,
> 
> Could you apply this or provide some feedback on it?  We've got a
> pending patch that depends on it, and that pending work will bring the
> driver back to a working (if not necessarily beautiful) state.  I don't
> want to submit it if this change isn't satisfactory, though.

Speaking of the devil. I was just thinking about it and I wanted to ask
you to send me an example of how it is used as I can;t make my mind about
it.

In general it is OK to submit a few patches at a time even if they are
depend on each other - it gives me better context (as long as there
aren't 80 of those so that I down in them ;) ).

Thanks.

-- 
Dmitry

Re: [PATCH] input synaptics-rmi4: PDT scan cleanup

[Christopher Heiny]

On 01/10/2014 12:31 PM, Dmitry Torokhov wrote:
> On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
>> >On 01/07/2014 12:33 PM, Christopher Heiny wrote:
>>> > >Eliminates copy-paste code that handled scans of the Page Descriptor
>>> > >Table,
>>> > >replacing it with a single PDT scan routine that invokes a callback
>>> > >function. The scan routine is not static so that it can be used by the
>>> > >firmware update code (under development, not yet submitted).
>>> > >
>>> > >Updated the copyright dates while we were at it.
>> >
>> >Hi Dmitry,
>> >
>> >Could you apply this or provide some feedback on it?  We've got a
>> >pending patch that depends on it, and that pending work will bring the
>> >driver back to a working (if not necessarily beautiful) state.  I don't
>> >want to submit it if this change isn't satisfactory, though.
 >
> Speaking of the devil. I was just thinking about it and I wanted to ask
> you to send me an example of how it is used as I can;t make my mind about
> it.
>
> In general it is OK to submit a few patches at a time even if they are
> depend on each other - it gives me better context (as long as there
> aren't 80 of those so that I down in them ;) ).
>
> Thanks.

No problem!

Currently the rmi_driver.c iterates over the PDT twice during probe():

1) find F01 and reset the ASIC;
2) create and initialize the function devices & count the number of 
interrupt sources.

followed by

3) a loop over the function devices allocating each one's interrupt 
related bitmasks.

Unfortunately, depending on how the function drivers are loaded, a 
function driver might access the device's and the function device's 
bitmasks before step (3) is complete.  This causes all kinds of 
unintended consequences, none of which are amusing.

We considered fixing this by splitting the function device creation and 
initialization, like so:

1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources and create the function devices

followed by

3) a loop over the function devices to allocate their interrupt related 
bitmasks and initialize the function devices

However, this led to function device setup being in two different 
places, which obscured the code and could lead to bugs if someone added 
new code in the wrong place (initialization instead of creation, or vice 
versa).

The PDT scan loops have a lot of redundant boilerplate and were quite 
large compared to the core code that was being iterated, obscuring just 
what the loop was supposed to be doing.  We found it clearer to 
implement a single PDT scan routine, and put the logic for the different 
step in callbacks.  Plus it eliminated the maintenance headaches of 
three (or more, see reflash, below) copy-paste PDT scan loops.

The pending patch (I'll send that right after this note) changes 
rmi_driver.c the order to scan the PDT 3 times:

1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources
3) create and initialize the function devices

Similar logic applies in the reflash code, which has rescan the PDT 
after forcing the touch sensor firmware into bootloader mode (the 
bootloader might have a different register map, unfortunately).  Once we 
had the scan+callback routine in rmi_driver.c, it only made sense for 
the reflash library to use that as well.

				Thanks,
					Chris

Re: [PATCH] input synaptics-rmi4: PDT scan cleanup

[Christopher Heiny]

Ping - any followup on this and related patches?

On 01/10/2014 01:50 PM, Christopher Heiny wrote:
> On 01/10/2014 12:31 PM, Dmitry Torokhov wrote:
>> On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
>>> >On 01/07/2014 12:33 PM, Christopher Heiny wrote:
>>>> > >Eliminates copy-paste code that handled scans of the Page Descriptor
>>>> > >Table,
>>>> > >replacing it with a single PDT scan routine that invokes a callback
>>>> > >function. The scan routine is not static so that it can be used
>>>> by the
>>>> > >firmware update code (under development, not yet submitted).
>>>> > >
>>>> > >Updated the copyright dates while we were at it.
>>> >
>>> >Hi Dmitry,
>>> >
>>> >Could you apply this or provide some feedback on it?  We've got a
>>> >pending patch that depends on it, and that pending work will bring the
>>> >driver back to a working (if not necessarily beautiful) state.  I don't
>>> >want to submit it if this change isn't satisfactory, though.
>  >
>> Speaking of the devil. I was just thinking about it and I wanted to ask
>> you to send me an example of how it is used as I can;t make my mind about
>> it.
>>
>> In general it is OK to submit a few patches at a time even if they are
>> depend on each other - it gives me better context (as long as there
>> aren't 80 of those so that I down in them ;) ).
>>
>> Thanks.
>
> No problem!
>
> Currently the rmi_driver.c iterates over the PDT twice during probe():
>
> 1) find F01 and reset the ASIC;
> 2) create and initialize the function devices & count the number of
> interrupt sources.
>
> followed by
>
> 3) a loop over the function devices allocating each one's interrupt
> related bitmasks.
>
> Unfortunately, depending on how the function drivers are loaded, a
> function driver might access the device's and the function device's
> bitmasks before step (3) is complete.  This causes all kinds of
> unintended consequences, none of which are amusing.
>
> We considered fixing this by splitting the function device creation and
> initialization, like so:
>
> 1) first to find F01 and reset the ASIC;
> 2) count the number of interrupt sources and create the function devices
>
> followed by
>
> 3) a loop over the function devices to allocate their interrupt related
> bitmasks and initialize the function devices
>
> However, this led to function device setup being in two different
> places, which obscured the code and could lead to bugs if someone added
> new code in the wrong place (initialization instead of creation, or vice
> versa).
>
> The PDT scan loops have a lot of redundant boilerplate and were quite
> large compared to the core code that was being iterated, obscuring just
> what the loop was supposed to be doing.  We found it clearer to
> implement a single PDT scan routine, and put the logic for the different
> step in callbacks.  Plus it eliminated the maintenance headaches of
> three (or more, see reflash, below) copy-paste PDT scan loops.
>
> The pending patch (I'll send that right after this note) changes
> rmi_driver.c the order to scan the PDT 3 times:
>
> 1) first to find F01 and reset the ASIC;
> 2) count the number of interrupt sources
> 3) create and initialize the function devices
>
> Similar logic applies in the reflash code, which has rescan the PDT
> after forcing the touch sensor firmware into bootloader mode (the
> bootloader might have a different register map, unfortunately).  Once we
> had the scan+callback routine in rmi_driver.c, it only made sense for
> the reflash library to use that as well.
>
>                  Thanks,
>                      Chris
> --
> 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] input synaptics-rmi4: PDT scan cleanup

[Dmitry Torokhov]

Hi Christopher,

On Tue, Jan 07, 2014 at 12:33:41PM -0800, Christopher Heiny wrote:
> Eliminates copy-paste code that handled scans of the Page Descriptor Table,
> replacing it with a single PDT scan routine that invokes a callback function.
> The scan routine is not static so that it can be used by the firmware update
> code (under development, not yet submitted).
> 
> Updated the copyright dates while we were at it.
> 
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> ---
> 
>  drivers/input/rmi4/rmi_driver.c | 155 ++++++++++++++++++++--------------------
>  drivers/input/rmi4/rmi_driver.h |   6 +-
>  2 files changed, 83 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index eb790ff..cbd6485 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
>   * Copyright (c) 2011 Unixphere
>   *
>   * This driver provides the core support for a single RMI4-based device.
> @@ -566,85 +566,80 @@ err_free_mem:
>  	return error;
>  }
>  
> -/*
> - * Scan the PDT for F01 so we can force a reset before anything else
> - * is done.  This forces the sensor into a known state, and also
> - * forces application of any pending updates from reflashing the
> - * firmware or configuration.
> - *
> - * We have to do this before actually building the PDT because the reflash
> - * updates (if any) might cause various registers to move around.
> - */
> -static int rmi_initial_reset(struct rmi_device *rmi_dev)
> +
> +#define RMI_SCAN_CONTINUE	0
> +#define RMI_SCAN_DONE		1

Can we add RMI_SCAN_ERROR here and use it instead of -EXXXX errors: I'd
rather not mix the 2 namespaces.

> +
> +static int rmi_initial_reset(struct rmi_device *rmi_dev,
> +		void *clbk_ctx, struct pdt_entry *pdt_entry, int page)
>  {
> -	struct pdt_entry pdt_entry;
> -	int page;
> -	struct device *dev = &rmi_dev->dev;
> -	bool done = false;
> -	bool has_f01 = false;
> -	int i;
>  	int retval;
> -	const struct rmi_device_platform_data *pdata =
> -			to_rmi_platform_data(rmi_dev);
>  
> -	dev_dbg(dev, "Initial reset.\n");
> +	if (pdt_entry->function_number == 0x01) {
> +		u16 cmd_addr = page + pdt_entry->command_base_addr;
> +		u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> +		const struct rmi_device_platform_data *pdata =
> +				to_rmi_platform_data(rmi_dev);
>  
> -	for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> -		u16 page_start = RMI4_PAGE_SIZE * page;
> -		u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
> -		u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
> +		retval = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
> +		if (retval < 0) {
> +			dev_err(&rmi_dev->dev,
> +				"Initial reset failed. Code = %d.\n", retval);
> +			return retval;
> +		}
> +		mdelay(pdata->reset_delay_ms);
>  
> -		done = true;
> -		for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
> -			retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
> -			if (retval < 0)
> -				return retval;
> +		return RMI_SCAN_DONE;
> +	}
>  
> -			if (RMI4_END_OF_PDT(pdt_entry.function_number))
> -				break;
> -			done = false;
> +	/* F01 should always be on page 0. If we don't find it there, fail. */
> +	return (!page) ? RMI_SCAN_CONTINUE : -ENODEV;
> +}
>  
> -			if (pdt_entry.function_number == 0x01) {
> -				u16 cmd_addr = page_start +
> -					pdt_entry.command_base_addr;
> -				u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> -				retval = rmi_write_block(rmi_dev, cmd_addr,
> -						&cmd_buf, 1);
> -				if (retval < 0) {
> -					dev_err(dev, "Initial reset failed. Code = %d.\n",
> -						retval);
> -					return retval;
> -				}
> -				mdelay(pdata->reset_delay_ms);
> -				done = true;
> -				has_f01 = true;
> -				break;
> -			}
> -		}
> -	}
> +static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
> +		void *clbk_ctx, struct pdt_entry *entry, int page)
> +{
> +	int *irq_count = (int *)clbk_ctx;
>  
> -	if (!has_f01) {
> -		dev_warn(dev, "WARNING: Failed to find F01 for initial reset.\n");
> -		return -ENODEV;
> -	}
> +	return create_function(rmi_dev, entry, irq_count,
> +				RMI4_PAGE_SIZE * page);
> +}
> +
> +static int rmi_create_functions(struct rmi_device *rmi_dev)
> +{
> +	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> +	struct device *dev = &rmi_dev->dev;
> +	int irq_count = 0;
> +	int retval;
> +
> +	// XXX need to make sure we create F01 first...
> +	// XXX or do we?  It might not be required in the updated structure.

Prefer not add more C99-style comments.

> +	retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_functions_clbk);
> +
> +	if (retval)
> +		return retval;
> +
> +	// TODO: I think we need to count the IRQs before creating the
> +	// functions.

Same here.

> +	data->irq_count = irq_count;
> +	data->num_of_irq_regs = (irq_count + 7) / 8;
>  
>  	return 0;
>  }
>  
> -static int rmi_scan_pdt(struct rmi_device *rmi_dev)
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> +		int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
> +		void *clbk_ctx, struct pdt_entry *entry, int page))
>  {
> -	struct rmi_driver_data *data;
> +	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>  	struct pdt_entry pdt_entry;
>  	int page;
> -	struct device *dev = &rmi_dev->dev;
> -	int irq_count = 0;
> -	bool done = false;
>  	int i;
> -	int retval;
> -
> -	dev_dbg(dev, "Scanning PDT...\n");
> +	bool done = false;
> +	int retval = 0;
>  
> -	data = dev_get_drvdata(&rmi_dev->dev);
> +	// TODO: With F01 and reflash as part of the core now, is this
> +	// lock still required?
>  	mutex_lock(&data->pdt_mutex);
>  
>  	for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> @@ -656,27 +651,27 @@ static int rmi_scan_pdt(struct rmi_device *rmi_dev)
>  		for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
>  			retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
>  			if (retval < 0)
> -				goto error_exit;
> +				return retval;
>  
>  			if (RMI4_END_OF_PDT(pdt_entry.function_number))
>  				break;
>  
> -			dev_dbg(dev, "Found F%02X on page %#04x\n",
> +			dev_dbg(&rmi_dev->dev, "Found F%02X on page %#04x\n",
>  					pdt_entry.function_number, page);
>  			done = false;
>  
> -			// XXX need to make sure we create F01 first...
> -			retval = create_function(rmi_dev,
> -					&pdt_entry, &irq_count, page_start);
> -
> -			if (retval)
> +			retval = rmi_pdt_scan_clbk(rmi_dev, ctx,
> +							&pdt_entry, page);
> +			if (retval < 0) {
>  				goto error_exit;
> +			} else if (retval == RMI_SCAN_DONE) {
> +				done = true;
> +				break;
> +			}
>  		}
>  		done = done || data->f01_bootloader_mode;

This should be simplified even more: the callback should return
RMI_SCAN_DONE if device is in bootloader mode.

>  	}
> -	data->irq_count = irq_count;
> -	data->num_of_irq_regs = (irq_count + 7) / 8;
> -	dev_dbg(dev, "%s: Done with PDT scan.\n", __func__);
> +
>  	retval = 0;
>  
>  error_exit:
> @@ -684,6 +679,7 @@ error_exit:
>  	return retval;
>  }
>  
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int rmi_driver_suspend(struct device *dev)
>  {
> @@ -797,10 +793,15 @@ static int rmi_driver_probe(struct device *dev)
>  
>  	/*
>  	 * Right before a warm boot, the sensor might be in some unusual state,
> -	 * such as F54 diagnostics, or F34 bootloader mode.  In order to clear
> -	 * the sensor to a known state, we issue a initial reset to clear any
> +	 * such as F54 diagnostics, or F34 bootloader mode after a firmware
> +	 * or configuration update.  In order to clear the sensor to a known
> +	 * state and/or apply any updates, we issue a initial reset to clear any
>  	 * previous settings and force it into normal operation.
>  	 *
> +	 * We have to do this before actually building the PDT because
> +	 * the reflash updates (if any) might cause various registers to move
> +	 * around.
> +	 *
>  	 * For a number of reasons, this initial reset may fail to return
>  	 * within the specified time, but we'll still be able to bring up the
>  	 * driver normally after that failure.  This occurs most commonly in
> @@ -813,11 +814,11 @@ static int rmi_driver_probe(struct device *dev)
>  	 */
>  	if (!pdata->reset_delay_ms)
>  		pdata->reset_delay_ms = DEFAULT_RESET_DELAY_MS;
> -	retval = rmi_initial_reset(rmi_dev);
> +	retval = rmi_scan_pdt(rmi_dev, NULL, rmi_initial_reset);
>  	if (retval)
>  		dev_warn(dev, "RMI initial reset failed! Continuing in spite of this.\n");
>  
> -	retval = rmi_scan_pdt(rmi_dev);
> +	retval = rmi_create_functions(rmi_dev);
>  	if (retval) {
>  		dev_err(dev, "PDT scan for %s failed with code %d.\n",
>  			pdata->sensor_name, retval);
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> index df9ddd8..f73be73 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
>   * Copyright (c) 2011 Unixphere
>   *
>   * This program is free software; you can redistribute it and/or modify it
> @@ -108,6 +108,10 @@ struct pdt_entry {
>  int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
>  			u16 pdt_address);
>  
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> +	int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
> +	void *clbk_ctx, struct pdt_entry *entry, int page));
> +
>  bool rmi_is_physical_driver(struct device_driver *);
>  int rmi_register_physical_driver(void);
>  void rmi_unregister_physical_driver(void);

Thanks.

-- 
Dmitry