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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux