[PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This series contains the minimum number of patches required to make the
Atmel MXT touchpad work on NVIDIA Tegra, which requires the device to be
instantiated from device tree rather than from a C board file.

These patches are based on patches in the ChromeOS kernel. However, I took
a different approach to the devicetree-doesn't-provide-platform-data issue.
Rather than amending all users of platform data to work with or without it,
as the patches in the ChromeOS kernel do, I implemented a single function
mxt_parse_dt() to create a platform data structure from DT. This isolates
the changes required to a single function, rather than spreading them all
over the driver.

Benson Leung (1):
  Input: atmel_mxt_ts - Set pointer emulation if is_tp

Stephen Warren (2):
  Input: atmel_mxt_ts: define a device tree binding
  Input: atmel_mxt_ts: implement device tree parsing

Yufeng Shen (1):
  Input: atmel_mxt_ts - Read resolution from device memory

 .../devicetree/bindings/input/atmel,mxt-tp.txt     |  26 +++++
 drivers/input/touchscreen/atmel_mxt_ts.c           | 121 +++++++++++++++++----
 include/linux/i2c/atmel_mxt_ts.h                   |   5 +-
 3 files changed, 128 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/atmel,mxt-tp.txt

-- 
1.8.1.5

[PATCH 1/4] Input: atmel_mxt_ts - Set pointer emulation if is_tp

[Stephen Warren]

From: Benson Leung <bleung@chromium.org>

Touchpads are pointers, so make sure to pass the correct values to
input_mt_report_pointer_emulation(). Without this, tap-to-click doesn't
work.

Signed-off-by: Benson Leung <bleung@chromium.org>
[swarren, rewrote the commit description, since the downstream version
concentrated on patch rebase history rather than the functionality]
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a70400754e92..9ba7e30c7894 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -619,7 +619,8 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 	} while (reportid != 0xff);
 
 	if (update_input) {
-		input_mt_report_pointer_emulation(data->input_dev, false);
+		input_mt_report_pointer_emulation(data->input_dev,
+						  data->is_tp);
 		input_sync(data->input_dev);
 	}
 
-- 
1.8.1.5

[PATCH 2/4] Input: atmel_mxt_ts - Read resolution from device memory

[Stephen Warren]

From: Yufeng Shen <miletus@chromium.org>

Currently mxt_calc_resolution() computes device resolution from
provided platform data. In order to support device tree, we would need
a way to represent these values there too. To avoid that, we rework
mxt_calc_resolution() so it reads the actual resolution from the
configured device memory.

And also move mxt_calc_resolution() into mxt_initialize() after
the device is configured.

Signed-off-by: Yufeng Shen <miletus@chromium.org>
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
[swarren, augmented patch description to mention device tree]
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 71 ++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 9ba7e30c7894..c7ab14cf84b7 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -812,6 +812,44 @@ static void mxt_free_object_table(struct mxt_data *data)
 	data->T19_reportid = 0;
 }
 
+static int mxt_calc_resolution(struct mxt_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct mxt_object *T9;
+	u8 orient;
+	__le16 xyrange[2];
+	unsigned int max_x, max_y;
+	int ret;
+
+	T9 = mxt_get_object(data, MXT_TOUCH_MULTI_T9);
+	if (T9 == NULL)
+		return -EINVAL;
+
+	/* Get touchscreen resolution */
+	ret = __mxt_read_reg(client, T9->start_address + MXT_TOUCH_XRANGE_LSB,
+			4, xyrange);
+	if (ret)
+		return ret;
+
+	ret = __mxt_read_reg(client, T9->start_address + MXT_TOUCH_ORIENT,
+			1, &orient);
+	if (ret)
+		return ret;
+
+	max_x = le16_to_cpu(xyrange[0]);
+	max_y = le16_to_cpu(xyrange[1]);
+
+	if (orient & MXT_XY_SWITCH) {
+		data->max_x = max_y;
+		data->max_y = max_x;
+	} else {
+		data->max_x = max_x;
+		data->max_y = max_y;
+	}
+
+	return 0;
+}
+
 static int mxt_initialize(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
@@ -844,14 +882,17 @@ static int mxt_initialize(struct mxt_data *data)
 	mxt_handle_pdata(data);
 
 	/* Backup to memory */
-	mxt_write_object(data, MXT_GEN_COMMAND_T6,
-			MXT_COMMAND_BACKUPNV,
-			MXT_BACKUP_VALUE);
+	error = mxt_write_object(data, MXT_GEN_COMMAND_T6,
+				 MXT_COMMAND_BACKUPNV, MXT_BACKUP_VALUE);
+	if (error)
+		return error;
 	msleep(MXT_BACKUP_TIME);
 
 	/* Soft reset */
-	mxt_write_object(data, MXT_GEN_COMMAND_T6,
-			MXT_COMMAND_RESET, 1);
+	error = mxt_write_object(data, MXT_GEN_COMMAND_T6,
+				 MXT_COMMAND_RESET, 1);
+	if (error)
+		return error;
 	msleep(MXT_RESET_TIME);
 
 	/* Update matrix size at info struct */
@@ -875,6 +916,10 @@ static int mxt_initialize(struct mxt_data *data)
 			info->matrix_xsize, info->matrix_ysize,
 			info->object_num);
 
+	error = mxt_calc_resolution(data);
+	if (error)
+		return error;
+
 	return 0;
 
 err_free_object_table:
@@ -882,20 +927,6 @@ err_free_object_table:
 	return error;
 }
 
-static void mxt_calc_resolution(struct mxt_data *data)
-{
-	unsigned int max_x = data->pdata->x_size - 1;
-	unsigned int max_y = data->pdata->y_size - 1;
-
-	if (data->pdata->orient & MXT_XY_SWITCH) {
-		data->max_x = max_y;
-		data->max_y = max_x;
-	} else {
-		data->max_x = max_x;
-		data->max_y = max_y;
-	}
-}
-
 /* Firmware Version is returned as Major.Minor.Build */
 static ssize_t mxt_fw_version_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
@@ -1166,8 +1197,6 @@ static int mxt_probe(struct i2c_client *client,
 	data->pdata = pdata;
 	data->irq = client->irq;
 
-	mxt_calc_resolution(data);
-
 	error = mxt_initialize(data);
 	if (error)
 		goto err_free_mem;
-- 
1.8.1.5

[PATCH 3/4] Input: atmel_mxt_ts: define a device tree binding

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This document describes how to represent an Atmel MXT touchpad in device
tree.

The device may show up in bootloader mode if reset by SW, or if
configuration/firmware is missing. Or, it may present itself as the final
touchpad device. These modes have different I2C addresses, and hence
different I2C nodes with different compatible values representing their
feature-set.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 .../devicetree/bindings/input/atmel,mxt-tp.txt     | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/atmel,mxt-tp.txt

diff --git a/Documentation/devicetree/bindings/input/atmel,mxt-tp.txt b/Documentation/devicetree/bindings/input/atmel,mxt-tp.txt
new file mode 100644
index 000000000000..c62798ef3a82
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/atmel,mxt-tp.txt
@@ -0,0 +1,26 @@
+Atmel MXT touchpad
+
+Required properties:
+- compatible: One of:
+    atmel,mxt-tp (for the main touchpad I2C address)
+    atmel,mxt-tp-bootloader (for the bootloader I2C address)
+
+- reg: The I2C address of the device
+
+- interrupts: The sink for the touchpad's IRQ output
+    See ../interrupt-controller/interrupts.txt
+
+Optional properties for main touchpad device:
+
+- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
+    keycode generated by each GPIO. Linux keycodes are defined in
+    <dt-bindings/input/input.h>.
+
+Example:
+
+	trackpad@4b {
+		compatible = "atmel,mxt-tp";
+		reg = <0x4b>;
+		interrupt-parent = <&gpio>;
+		interrupts = <TEGRA_GPIO(W, 3) GPIO_ACTIVE_HIGH>;
+	};
-- 
1.8.1.5

[PATCH 4/4] Input: atmel_mxt_ts: implement device tree parsing

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

FIXME:

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 47 +++++++++++++++++++++++++++++++-
 include/linux/i2c/atmel_mxt_ts.h         |  5 +++-
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index c7ab14cf84b7..6080f28637d2 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -18,6 +18,7 @@
 #include <linux/i2c/atmel_mxt_ts.h>
 #include <linux/input/mt.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 
 /* Version */
@@ -879,7 +880,8 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		goto err_free_object_table;
 
-	mxt_handle_pdata(data);
+	if (!data->pdata->skip_hw_config)
+		mxt_handle_pdata(data);
 
 	/* Backup to memory */
 	error = mxt_write_object(data, MXT_GEN_COMMAND_T6,
@@ -1158,6 +1160,44 @@ static void mxt_input_close(struct input_dev *dev)
 	mxt_stop(data);
 }
 
+static const struct of_device_id mxt_of_match[] = {
+	{ .compatible = "atmel,mxt-tp", },
+	{ .compatible = "atmel,mxt-tp-bootloader", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mxt_of_match);
+
+static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client)
+{
+	struct mxt_platform_data *pdata;
+	int i, ret;
+	u32 keycode;
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	/*
+	 * Later, we could add some optional properties to allow the HW
+	 * configuation fields and IRQ configuration to be set in device tree.
+	 */
+	pdata->skip_hw_config = true;
+	pdata->irqflags = IRQF_TRIGGER_FALLING;
+
+	/* This can be sourced from mxt_of_match[].data if it varies */
+	pdata->is_tp = true;
+
+	for (i = 0; i < MXT_NUM_GPIO; i++) {
+		ret = of_property_read_u32_index(client->dev.of_node,
+				"linux,gpio-keymap", i, &keycode);
+		if (ret)
+			continue;
+		pdata->key_map[i] = keycode;
+	}
+
+	return pdata;
+}
+
 static int mxt_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
@@ -1167,6 +1207,10 @@ static int mxt_probe(struct i2c_client *client,
 	int error;
 	unsigned int num_mt_slots;
 
+#if IS_ENABLED(CONFIG_OF)
+	if (!pdata && client->dev.of_node)
+		pdata = mxt_parse_dt(client);
+#endif
 	if (!pdata)
 		return -EINVAL;
 
@@ -1356,6 +1400,7 @@ static struct i2c_driver mxt_driver = {
 	.driver = {
 		.name	= "atmel_mxt_ts",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mxt_of_match),
 		.pm	= &mxt_pm_ops,
 	},
 	.probe		= mxt_probe,
diff --git a/include/linux/i2c/atmel_mxt_ts.h b/include/linux/i2c/atmel_mxt_ts.h
index 99e379b74398..30c5fa067baa 100644
--- a/include/linux/i2c/atmel_mxt_ts.h
+++ b/include/linux/i2c/atmel_mxt_ts.h
@@ -33,6 +33,8 @@ struct mxt_platform_data {
 	const u8 *config;
 	size_t config_length;
 
+	/* true if x_line..orient are not set */
+	bool skip_hw_config;
 	unsigned int x_line;
 	unsigned int y_line;
 	unsigned int x_size;
@@ -41,9 +43,10 @@ struct mxt_platform_data {
 	unsigned int threshold;
 	unsigned int voltage;
 	unsigned char orient;
+
 	unsigned long irqflags;
 	bool is_tp;
-	const unsigned int key_map[MXT_NUM_GPIO];
+	unsigned int key_map[MXT_NUM_GPIO];
 };
 
 #endif /* __LINUX_ATMEL_MXT_TS_H */
-- 
1.8.1.5

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Benson Leung]

Hi Stephen,

On Tue, May 6, 2014 at 3:13 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This series contains the minimum number of patches required to make the
> Atmel MXT touchpad work on NVIDIA Tegra, which requires the device to be
> instantiated from device tree rather than from a C board file.
>
> These patches are based on patches in the ChromeOS kernel. However, I took
> a different approach to the devicetree-doesn't-provide-platform-data issue.
> Rather than amending all users of platform data to work with or without it,
> as the patches in the ChromeOS kernel do, I implemented a single function
> mxt_parse_dt() to create a platform data structure from DT. This isolates
> the changes required to a single function, rather than spreading them all
> over the driver.

Please coordinate with Nick Dyer, who has a long patch series that's
been approved but not yet merged into the input tree. The few patches
you've picked from the Chrome OS kernel overlap with his patch series.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/06/2014 04:16 PM, Benson Leung wrote:
> Hi Stephen,
> 
> On Tue, May 6, 2014 at 3:13 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This series contains the minimum number of patches required to make the
>> Atmel MXT touchpad work on NVIDIA Tegra, which requires the device to be
>> instantiated from device tree rather than from a C board file.
>>
>> These patches are based on patches in the ChromeOS kernel. However, I took
>> a different approach to the devicetree-doesn't-provide-platform-data issue.
>> Rather than amending all users of platform data to work with or without it,
>> as the patches in the ChromeOS kernel do, I implemented a single function
>> mxt_parse_dt() to create a platform data structure from DT. This isolates
>> the changes required to a single function, rather than spreading them all
>> over the driver.
> 
> Please coordinate with Nick Dyer, who has a long patch series that's
> been approved but not yet merged into the input tree. The few patches
> you've picked from the Chrome OS kernel overlap with his patch series.

Ah I remember you mentioning that before. Is Nick's work still active?
The link you sent me to the "latest status" was nearly a year ago:

https://lkml.org/lkml/2013/6/27/311

... and while the github link you sent:

https://github.com/ndyer/linux/commits/for-next-20140316-v8

... has much more recent activity, it hasn't been touched /that/
recently either.

To be honest, it'd be a bit off-putting to hold up a trivial patch
series like this and make it wait for a huge refactoring series that's
obviously been dragging on for a long time...

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Nick Dyer]

Stephen Warren wrote:
> On 05/06/2014 04:16 PM, Benson Leung wrote:
>> Please coordinate with Nick Dyer, who has a long patch series that's
>> been approved but not yet merged into the input tree. The few patches
>> you've picked from the Chrome OS kernel overlap with his patch series.
> 
> Ah I remember you mentioning that before. Is Nick's work still active?
> The link you sent me to the "latest status" was nearly a year ago:
> 
> https://lkml.org/lkml/2013/6/27/311

My latest set of patches for upstream was sent just over a month ago:
https://lkml.org/lkml/2014/3/17/403

Dmitry Torokov has signed-off on them but not merged them into his tree
yet. It would be good to hear something from him!

> ... and while the github link you sent:
> 
> https://github.com/ndyer/linux/commits/for-next-20140316-v8
> 
> ... has much more recent activity, it hasn't been touched /that/
> recently either.
> 
> To be honest, it'd be a bit off-putting to hold up a trivial patch
> series like this and make it wait for a huge refactoring series that's
> obviously been dragging on for a long time...

>From my point of view, it would be better if everyone with a stake in this
driver worked together to test and review a single set of improvements that
fixed bugs, added new features, and supported new chips, rather than
everyone implementing trivial fixes in various different ways that cause
merge conflicts and strange bugs.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/08/2014 10:01 AM, Nick Dyer wrote:
> Stephen Warren wrote:
>> On 05/06/2014 04:16 PM, Benson Leung wrote:
>>> Please coordinate with Nick Dyer, who has a long patch series that's
>>> been approved but not yet merged into the input tree. The few patches
>>> you've picked from the Chrome OS kernel overlap with his patch series.
>>
>> Ah I remember you mentioning that before. Is Nick's work still active?
>> The link you sent me to the "latest status" was nearly a year ago:
>>
>> https://lkml.org/lkml/2013/6/27/311
> 
> My latest set of patches for upstream was sent just over a month ago:
> https://lkml.org/lkml/2014/3/17/403
> 
> Dmitry Torokov has signed-off on them but not merged them into his tree
> yet. It would be good to hear something from him!

I don't understand the following comment in patch 0 at that link:

> Hi Dimitry-
>
> Here is a set of patches for atmel_mxt_ts that you've already
> signed-off.

Surely Dmitry would only have signed off on the patches if he had
applied them somewhere. I'm puzzled why he would have applied them but
not pushed them out into linux-next.

Perhaps he didn't sign off on the patches but rather just said they were
OK, or gave an ack? In which case, did you add Dmitry's S-o-b line to
the patches yourself? That's certainly not the correct procedure.
Perhaps he ignored the patches because of that?

Anyway, I'd like to pull these patches into my local repo to build on.
Can you point me at a tree where Dmitry applied them even if not in
linux-next? Alternatively, does your github repo contain exactly the
patches from the recent mailing list posting you linked above?

...
> From my point of view, it would be better if everyone with a stake in this
> driver worked together to test and review a single set of improvements that
> fixed bugs, added new features, and supported new chips, rather than
> everyone implementing trivial fixes in various different ways that cause
> merge conflicts and strange bugs.

Luckily, it doesn't look like it will be too hard to rebase my patches
on top of your work. However, I'd really like some feedback from Dmitry
re: when and where your patches will be applied, so that I know if/when
it makes sense to rebase on top of them.

Thanks.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Dmitry Torokhov]

On Thu, May 08, 2014 at 10:41:45AM -0600, Stephen Warren wrote:
> On 05/08/2014 10:01 AM, Nick Dyer wrote:
> > Stephen Warren wrote:
> >> On 05/06/2014 04:16 PM, Benson Leung wrote:
> >>> Please coordinate with Nick Dyer, who has a long patch series that's
> >>> been approved but not yet merged into the input tree. The few patches
> >>> you've picked from the Chrome OS kernel overlap with his patch series.
> >>
> >> Ah I remember you mentioning that before. Is Nick's work still active?
> >> The link you sent me to the "latest status" was nearly a year ago:
> >>
> >> https://lkml.org/lkml/2013/6/27/311
> > 
> > My latest set of patches for upstream was sent just over a month ago:
> > https://lkml.org/lkml/2014/3/17/403
> > 
> > Dmitry Torokov has signed-off on them but not merged them into his tree
> > yet. It would be good to hear something from him!
> 
> I don't understand the following comment in patch 0 at that link:
> 
> > Hi Dimitry-
> >
> > Here is a set of patches for atmel_mxt_ts that you've already
> > signed-off.
> 
> Surely Dmitry would only have signed off on the patches if he had
> applied them somewhere. I'm puzzled why he would have applied them but
> not pushed them out into linux-next.

I stashed them into a topic branch but never merged it to next because
there were concerns about the firmware loader interface causing long
pauses at startup when driver was built into the kernel. I need to go
over the series again.

Thanks.

-- 
Dmitry

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Nick Dyer]

Stephen Warren wrote:
> Anyway, I'd like to pull these patches into my local repo to build on.
> Can you point me at a tree where Dmitry applied them even if not in
> linux-next? Alternatively, does your github repo contain exactly the
> patches from the recent mailing list posting you linked above?

https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/log/?h=atmel-mxt-ts

However, I have made minor updates on top of that to take account of API
changes since he worked on them (reinit_completion).

The patches I posted at the end of March are the first 22 out of this tag:

https://github.com/ndyer/linux/tree/for-next-20140316-v8

> ...
>> From my point of view, it would be better if everyone with a stake in this
>> driver worked together to test and review a single set of improvements that
>> fixed bugs, added new features, and supported new chips, rather than
>> everyone implementing trivial fixes in various different ways that cause
>> merge conflicts and strange bugs.
> 
> Luckily, it doesn't look like it will be too hard to rebase my patches
> on top of your work. However, I'd really like some feedback from Dmitry
> re: when and where your patches will be applied, so that I know if/when
> it makes sense to rebase on top of them.

OK. I agree that it's a good thing if we can get a sensible device tree
patch in as soon as possible. You will notice that a lot of the existing
platform data is removed in that sequence, and we already have a patch to
read the screen resolution from the chip.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Nick Dyer]

Dmitry Torokhov wrote:
>>> Here is a set of patches for atmel_mxt_ts that you've already
>>> signed-off.
>>
>> Surely Dmitry would only have signed off on the patches if he had
>> applied them somewhere. I'm puzzled why he would have applied them but
>> not pushed them out into linux-next.
> 
> I stashed them into a topic branch but never merged it to next because
> there were concerns about the firmware loader interface causing long
> pauses at startup when driver was built into the kernel. I need to go
> over the series again.

Thanks! I had wondered why you had left it, you never communicated this to
me unfortunately. Would it be preferable for me to switch to
request_firmware_nowait() and complete the probe asynchronously after it
returns?

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Thu, May 08, 2014 at 08:56:04PM +0100, Nick Dyer wrote:

> Thanks! I had wondered why you had left it, you never communicated this to
> me unfortunately. Would it be preferable for me to switch to
> request_firmware_nowait() and complete the probe asynchronously after it
> returns?

Another common option is to defer the firmware request/load until the
device is opened, but that doesn't work for all devices.  That should
mean that userspace is up and running.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/08/2014 01:50 PM, Nick Dyer wrote:
> Stephen Warren wrote:
>> Anyway, I'd like to pull these patches into my local repo to build on.
>> Can you point me at a tree where Dmitry applied them even if not in
>> linux-next? Alternatively, does your github repo contain exactly the
>> patches from the recent mailing list posting you linked above?
> 
> https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/log/?h=atmel-mxt-ts
> 
> However, I have made minor updates on top of that to take account of API
> changes since he worked on them (reinit_completion).
> 
> The patches I posted at the end of March are the first 22 out of this tag:
> 
> https://github.com/ndyer/linux/tree/for-next-20140316-v8

I took those 22 patches, applied them on top of next-20150507 (which is
just what I happened to be developing on top of right now), and rebased
my patches which add DT support. You can find the result here if you want:

git://github.com/swarren/linux-tegra.git tegra_dev

However, with that code-base, the touchpad doesn't work for me. I see a
long pause and errors during boot:

> [    1.246253] atmel_mxt_ts 1-004b: Direct firmware load failed with error -2
> [    1.253120] atmel_mxt_ts 1-004b: Falling back to user helper
> [   61.394348] atmel_mxt_ts 1-004b: Failure to request config file maxtouch.cfg
> [   61.403717] atmel_mxt_ts 1-004b: Family: 130 Variant: 1 Firmware V1.0.AA Objects: 22

... and the touchpad doesn't work once X starts. With next-20140507 plus
the patches I sent to the mailing list, everything works.

I'm not sure if I'm supposed to have some configuration file now and
that's why it doesn't work, or whether some other problem has been
introduced? If I do need a config file, can you tell me what to put into it?

Thanks for any pointers.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Dmitry Torokhov]

On Mon, May 12, 2014 at 02:02:24PM -0600, Stephen Warren wrote:
> On 05/08/2014 01:50 PM, Nick Dyer wrote:
> > Stephen Warren wrote:
> >> Anyway, I'd like to pull these patches into my local repo to build on.
> >> Can you point me at a tree where Dmitry applied them even if not in
> >> linux-next? Alternatively, does your github repo contain exactly the
> >> patches from the recent mailing list posting you linked above?
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/log/?h=atmel-mxt-ts
> > 
> > However, I have made minor updates on top of that to take account of API
> > changes since he worked on them (reinit_completion).
> > 
> > The patches I posted at the end of March are the first 22 out of this tag:
> > 
> > https://github.com/ndyer/linux/tree/for-next-20140316-v8
> 
> I took those 22 patches, applied them on top of next-20150507 (which is
> just what I happened to be developing on top of right now), and rebased
> my patches which add DT support. You can find the result here if you want:
> 
> git://github.com/swarren/linux-tegra.git tegra_dev
> 
> However, with that code-base, the touchpad doesn't work for me. I see a
> long pause and errors during boot:
> 
> > [    1.246253] atmel_mxt_ts 1-004b: Direct firmware load failed with error -2
> > [    1.253120] atmel_mxt_ts 1-004b: Falling back to user helper
> > [   61.394348] atmel_mxt_ts 1-004b: Failure to request config file maxtouch.cfg
> > [   61.403717] atmel_mxt_ts 1-004b: Family: 130 Variant: 1 Firmware V1.0.AA Objects: 22
> 
> ... and the touchpad doesn't work once X starts. With next-20140507 plus
> the patches I sent to the mailing list, everything works.
> 
> I'm not sure if I'm supposed to have some configuration file now and
> that's why it doesn't work, or whether some other problem has been
> introduced? If I do need a config file, can you tell me what to put into it?
> 
> Thanks for any pointers.

If you build firmware into kernel or build the driver as a module it
will work, but we do need to solve it general case as well.

Thanks.

-- 
Dmitry

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Dmitry Torokhov]

On Fri, May 09, 2014 at 07:49:08PM +0100, Mark Brown wrote:
> On Thu, May 08, 2014 at 08:56:04PM +0100, Nick Dyer wrote:
> 
> > Thanks! I had wondered why you had left it, you never communicated this to
> > me unfortunately. Would it be preferable for me to switch to
> > request_firmware_nowait() and complete the probe asynchronously after it
> > returns?
> 
> Another common option is to defer the firmware request/load until the
> device is opened, but that doesn't work for all devices.  That should
> mean that userspace is up and running.

That would not work for input devices as firmware quite often defines
capabilities of the devices and we need them before registering input
device (which is the object that userspace opens).

Thanks.

-- 
Dmitry

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/12/2014 07:16 PM, Dmitry Torokhov wrote:
> On Mon, May 12, 2014 at 02:02:24PM -0600, Stephen Warren wrote:
>> On 05/08/2014 01:50 PM, Nick Dyer wrote:
>>> Stephen Warren wrote:
>>>> Anyway, I'd like to pull these patches into my local repo to build on.
>>>> Can you point me at a tree where Dmitry applied them even if not in
>>>> linux-next? Alternatively, does your github repo contain exactly the
>>>> patches from the recent mailing list posting you linked above?
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/log/?h=atmel-mxt-ts
>>>
>>> However, I have made minor updates on top of that to take account of API
>>> changes since he worked on them (reinit_completion).
>>>
>>> The patches I posted at the end of March are the first 22 out of this tag:
>>>
>>> https://github.com/ndyer/linux/tree/for-next-20140316-v8
>>
>> I took those 22 patches, applied them on top of next-20150507 (which is
>> just what I happened to be developing on top of right now), and rebased
>> my patches which add DT support. You can find the result here if you want:
>>
>> git://github.com/swarren/linux-tegra.git tegra_dev
>>
>> However, with that code-base, the touchpad doesn't work for me. I see a
>> long pause and errors during boot:
>>
>>> [    1.246253] atmel_mxt_ts 1-004b: Direct firmware load failed with error -2
>>> [    1.253120] atmel_mxt_ts 1-004b: Falling back to user helper
>>> [   61.394348] atmel_mxt_ts 1-004b: Failure to request config file maxtouch.cfg
>>> [   61.403717] atmel_mxt_ts 1-004b: Family: 130 Variant: 1 Firmware V1.0.AA Objects: 22
>>
>> ... and the touchpad doesn't work once X starts. With next-20140507 plus
>> the patches I sent to the mailing list, everything works.
>>
>> I'm not sure if I'm supposed to have some configuration file now and
>> that's why it doesn't work, or whether some other problem has been
>> introduced? If I do need a config file, can you tell me what to put into it?
>>
>> Thanks for any pointers.
> 
> If you build firmware into kernel or build the driver as a module it
> will work, but we do need to solve it general case as well.

Building it as a module does solve the long delay at bootup, but doesn't
make the device actually work.

I believe the HW doesn't require any firmware; it has non-volatile
storage for both the firmware and configuration. However, the driver
doesn't seem to work without a firmware file being present.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Nick Dyer]

Stephen Warren wrote:
> On 05/08/2014 01:50 PM, Nick Dyer wrote:
>> The patches I posted at the end of March are the first 22 out of this tag:
>>
>> https://github.com/ndyer/linux/tree/for-next-20140316-v8
> 
> I took those 22 patches, applied them on top of next-20150507 (which is
> just what I happened to be developing on top of right now), and rebased
> my patches which add DT support. You can find the result here if you want:
> 
> git://github.com/swarren/linux-tegra.git tegra_dev

Thanks for this. Would you be happy for me to pick these changes up and
include them along with the other work I am sending to Dmitry? I am just
beginning to do various updates to the whole series, one of the things I
need to sort out is the device tree support.

I will need to add device tree parameters for the touchscreen as well as
the touchpad, of course.

By the way, the driver should work without any firmware file and just use
the firmware and configuration from NVRAM - request_firmware() returns a
failure and it continues through mxt_initialize().

In a later patch in my long series, I make the MXT_CFG_NAME configurable
from platform data (because you may have multiple devices needing different
configs), and leaving it null means the call to request_firmware() is skipped.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/16/2014 10:21 AM, Nick Dyer wrote:
> Stephen Warren wrote:
>> On 05/08/2014 01:50 PM, Nick Dyer wrote:
>>> The patches I posted at the end of March are the first 22 out of this tag:
>>>
>>> https://github.com/ndyer/linux/tree/for-next-20140316-v8
>>
>> I took those 22 patches, applied them on top of next-20150507 (which is
>> just what I happened to be developing on top of right now), and rebased
>> my patches which add DT support. You can find the result here if you want:
>>
>> git://github.com/swarren/linux-tegra.git tegra_dev
> 
> Thanks for this. Would you be happy for me to pick these changes up and
> include them along with the other work I am sending to Dmitry? I am just
> beginning to do various updates to the whole series, one of the things I
> need to sort out is the device tree support.

That would be fine. I assume you'd only take the 2 Atmel driver patches.
I'll send the Tegra patches separately once the driver is merged.

One thing I wasn't really sure about: With your latest patches, it seems
like the bootloader address is auto-calculated from the application
address. As such, do I still need separate struct i2c_device for the
application and bootloader I2C addresses? If not, we should remove the
atmel,mxt-tp-bootloader from those patches. If so, I need to add the
second DT node back into the Tegra DT. Either way, it might be
preferable if we only had 1 node in DT, and the driver automatically
handled the two separate I2C addresses.

> I will need to add device tree parameters for the touchscreen as well as
> the touchpad, of course.
> 
> By the way, the driver should work without any firmware file and just use
> the firmware and configuration from NVRAM - request_firmware() returns a
> failure and it continues through mxt_initialize().

Hmmm. I couldn't get that to work after applying the patches you posted.
However, it did with what's already in linux-next plus the patches I sent.

> In a later patch in my long series, I make the MXT_CFG_NAME configurable
> from platform data (because you may have multiple devices needing different
> configs), and leaving it null means the call to request_firmware() is skipped.

It'd be preferable to automatically derive the firmware name from the
device type (touchscreen/touchpad) or some other parameter that can be
calculated at run-time, or queried from HW (e.g. version # from
bootloader?). I'm not sure that putting firmware filenames in DT is a
good idea, but perhaps that would work. Deriving firmware filename from
the DT compatible value would work best. If different HW needs different
firmware, it should probably have a different compatible value in DT.

I don't want to build firmware filenames into the kernel at
compile-time, since I want to be able to take a single kernel and run it
on any ARM board, each of which might use different firmware, but all
the same, use the same root filesystem (on an SD card) on all boards,
without having to rename firmware.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Nick Dyer]

Stephen Warren wrote:
> On 05/16/2014 10:21 AM, Nick Dyer wrote:
>> Thanks for this. Would you be happy for me to pick these changes up and
>> include them along with the other work I am sending to Dmitry? I am just
>> beginning to do various updates to the whole series, one of the things I
>> need to sort out is the device tree support.
> 
> That would be fine. I assume you'd only take the 2 Atmel driver patches.
> I'll send the Tegra patches separately once the driver is merged.

Great! Dmitry has merged some of the patches I sent now, so I'm just
working on updating to take account of that and adding the device tree
changes, and taking account of a couple of other review comments.

> One thing I wasn't really sure about: With your latest patches, it seems
> like the bootloader address is auto-calculated from the application
> address. As such, do I still need separate struct i2c_device for the
> application and bootloader I2C addresses? If not, we should remove the
> atmel,mxt-tp-bootloader from those patches. If so, I need to add the
> second DT node back into the Tegra DT. Either way, it might be
> preferable if we only had 1 node in DT, and the driver automatically
> handled the two separate I2C addresses.

Currently you shouldn't need the extra node for the bootloader, it should
figure it out itself. I think in the future, the driver should register the
bootloader address with i2c_new_dummy() to prevent it being bound to a
different driver.

>> I will need to add device tree parameters for the touchscreen as well as
>> the touchpad, of course.
>>
>> By the way, the driver should work without any firmware file and just use
>> the firmware and configuration from NVRAM - request_firmware() returns a
>> failure and it continues through mxt_initialize().
> 
> Hmmm. I couldn't get that to work after applying the patches you posted.
> However, it did with what's already in linux-next plus the patches I sent.

I will test this on my setup and see if I can figure out what is causing
the problem.

>> In a later patch in my long series, I make the MXT_CFG_NAME configurable
>> from platform data (because you may have multiple devices needing different
>> configs), and leaving it null means the call to request_firmware() is skipped.
> 
> It'd be preferable to automatically derive the firmware name from the
> device type (touchscreen/touchpad) or some other parameter that can be
> calculated at run-time, or queried from HW (e.g. version # from
> bootloader?). I'm not sure that putting firmware filenames in DT is a
> good idea, but perhaps that would work. Deriving firmware filename from
> the DT compatible value would work best.

Yes, I was planning to allow the firmware filename to be specified in DT. I
think that coming up with a scheme to automatically derive it would fall
down in various corner cases (as an example, you might have two devices
which have the same family/variant IDs but require different firmwares), so
it gives maximum flexibility to not dictate the naming policy.

> If different HW needs different
> firmware, it should probably have a different compatible value in DT.

There are literally hundreds of different combinations of hardware/firmware
supported by this driver. Trying to generate a comprehensive list would be
a never-ending task to support. I don't think this is a good idea.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Sekhar Nori]

Hi Nick,

Nick Dyer <nick.dyer <at> itdev.co.uk> writes:

> Thanks for this. Would you be happy for me to pick these changes up and
> include them along with the other work I am sending to Dmitry? I am just
> beginning to do various updates to the whole series, one of the things I
> need to sort out is the device tree support.

I am starting work on support for Atmel mxT224 present
on TI's DRA7x EVM. What would be a good tree to base
my work on? Like Stephen, my platform is device-tree only.

I looked at your tree:

git://github.com/ndyer/linux.git for-next

it was updated 3 months ago and does not have Stephen's
DT patches yet. It will be great to have a common tree
which we can all base-off on to avoid rework.

Thanks,
Sekhar

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/20/2014 10:19 AM, Nick Dyer wrote:
> Stephen Warren wrote:
>> On 05/16/2014 10:21 AM, Nick Dyer wrote:
>>> Thanks for this. Would you be happy for me to pick these changes up and
>>> include them along with the other work I am sending to Dmitry? I am just
>>> beginning to do various updates to the whole series, one of the things I
>>> need to sort out is the device tree support.
>>
>> That would be fine. I assume you'd only take the 2 Atmel driver patches.
>> I'll send the Tegra patches separately once the driver is merged.
> 
> Great! Dmitry has merged some of the patches I sent now, so I'm just
> working on updating to take account of that and adding the device tree
> changes, and taking account of a couple of other review comments.
...
>>> I will need to add device tree parameters for the touchscreen as well as
>>> the touchpad, of course.
>>>
>>> By the way, the driver should work without any firmware file and just use
>>> the firmware and configuration from NVRAM - request_firmware() returns a
>>> failure and it continues through mxt_initialize().
>>
>> Hmmm. I couldn't get that to work after applying the patches you posted.
>> However, it did with what's already in linux-next plus the patches I sent.
> 
> I will test this on my setup and see if I can figure out what is causing
> the problem.

Nick, did you get anywhere reproducing my problem? Unfortunately with
your recent patches merged, I can't get the touchpad working at all,
whereas before I had a few simple patches that made it work:-(

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Nick Dyer]

Stephen Warren wrote:
>> I will test this on my setup and see if I can figure out what is causing
>> the problem.
> 
> Nick, did you get anywhere reproducing my problem? Unfortunately with
> your recent patches merged, I can't get the touchpad working at all,
> whereas before I had a few simple patches that made it work:-(

I haven't been able to reproduce your issue, I have a devicetree based
system that will successfully probe without a config file.

Could you send me the dmesg output with all of the driver debug enabled? It
would be good to see how far it is getting.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 06/12/2014 05:25 AM, Nick Dyer wrote:
> Stephen Warren wrote:
>>> I will test this on my setup and see if I can figure out what is causing
>>> the problem.
>>
>> Nick, did you get anywhere reproducing my problem? Unfortunately with
>> your recent patches merged, I can't get the touchpad working at all,
>> whereas before I had a few simple patches that made it work:-(
> 
> I haven't been able to reproduce your issue, I have a devicetree based
> system that will successfully probe without a config file.
> 
> Could you send me the dmesg output with all of the driver debug enabled? It
> would be good to see how far it is getting.

My apologies; I should have retested before asking about this again. I
guess something must have changed in linux-next since the last time I
tested this.

With next-20140611 plus the following patches on top:

d3168ccd7d57 ARM: tegra: enable Atmel touchpad in defconfig
fc1d7c4aefe0 ARM: tegra: add touchpad to Venice2 DT
da5f283a52ea Input: atmel_mxt_ts: implement device tree parsing
22911ed92e76 Input: atmel_mxt_ts: define a device tree binding
57f79f65a8f9 Input: atmel_mxt_ts - Set pointer emulation on touchpads

... the driver built into the kernel, and no firmware available in
/lib/firmware, the touchpad now works.

Are you still expecting to take my DT binding patches above and resend them?

One bug I see is that the mouse doesn't seem to release when I drag it
(either through double clicking and holding on the second click, or by a
two-finger drag). Is that a known issue?

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 06/12/2014 11:12 AM, Stephen Warren wrote:
...
> One bug I see is that the mouse doesn't seem to release when I drag it
> (either through double clicking and holding on the second click, or by a
> two-finger drag). Is that a known issue?

Let me refine the issue: It's nothing to do with drags, but rather any
time I push the touchpad hard to physically press the mouse button,
there's no release event.

"Soft" taps of the touchpad work fine, for single, double, or even
triple clicks:

(single soft click)
> Event: time 1402594144.964326, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 1
> Event: time 1402594144.964326, type 1 (EV_KEY), code 325 (BTN_TOOL_FINGER), value 1
...
...
> Event: time 1402594145.026351, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0
> Event: time 1402594145.026351, type 1 (EV_KEY), code 325 (BTN_TOOL_FINGER), value 0

However, "hard" pushes that physically depress the touchpad and activate
the physical button don't generate release events, so clicks and drags
never end.

Single hard click:

> Event: time 1402594196.593745, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 1
> Event: time 1402594196.593745, type 1 (EV_KEY), code 325 (BTN_TOOL_FINGER), value 1
...
> Event: time 1402594196.614456, type 1 (EV_KEY), code 272 (BTN_LEFT), value 1
...
> Event: time 1402594196.946386, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0
> Event: time 1402594196.946386, type 1 (EV_KEY), code 325 (BTN_TOOL_FINGER), value 0

(BTN_LEFT value 0 not sent)

Perhaps commit "Input: atmel_mxt_ts - Set pointer emulation on
touchpads" is incomplete? However, without that applied, neither soft
nor hard click generate any kind of mouse button events, just BTN_TOUCH
which X doesn't seem to interpret as a mouse click.

Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra

[Stephen Warren]

On 05/20/2014 10:19 AM, Nick Dyer wrote:
> Stephen Warren wrote:
>> On 05/16/2014 10:21 AM, Nick Dyer wrote:
>>> Thanks for this. Would you be happy for me to pick these changes up and
>>> include them along with the other work I am sending to Dmitry? I am just
>>> beginning to do various updates to the whole series, one of the things I
>>> need to sort out is the device tree support.
>>
>> That would be fine. I assume you'd only take the 2 Atmel driver patches.
>> I'll send the Tegra patches separately once the driver is merged.
> 
> Great! Dmitry has merged some of the patches I sent now, so I'm just
> working on updating to take account of that and adding the device tree
> changes, and taking account of a couple of other review comments.

Nick, did you get anywhere with taking my DT patches and resending them?
Would you like me to resend them instead? I was really hoping to see
them working in 3.17. Unfortunately, I'm away for ~2.5 weeks starting in
a couple of days, so I won't be able to get them into 3.17 myself.