atmel_mxt_ts and mxt224e

atmel_mxt_ts and mxt224e

[Peter Meerwald]

Hello,

I had some issues in using the atmel_mxt_ts driver with different mxt224 parts 
(different firmware version, mxt224 vs. mxt224); configuration of the chip
via mxt_platform_data.config is super-fragile when device objects change

mxt_handle_pdata() assumes that certain object table entries are present and
fails on the mxt224e

the patch series aims to support the mxt224e better and adds a message to detect
inconsistencies between config data and the device object table

if there is interest I can propose a more robust mechanism to set the config data;
similar patches are floating around for Android in various trees

thanks, regards, p.

[PATCH 1/7] input: atmel_mxt_ts: drop unused MXT_VER_ from atmel_mxt_ts driver

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

not used, and not clear what it is used for

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 1df2396..fc52dc5 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -21,11 +21,6 @@
 #include <linux/interrupt.h>
 #include <linux/slab.h>
 
-/* Version */
-#define MXT_VER_20		20
-#define MXT_VER_21		21
-#define MXT_VER_22		22
-
 /* Slave addresses */
 #define MXT_APP_LOW		0x4a
 #define MXT_APP_HIGH		0x4b
-- 
1.7.9.5

[PATCH 2/7] input: atmel_mxt_ts: allow to read/display MXT_GEN_MESSAGE_T5 object

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index fc52dc5..63a82bb 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -251,6 +251,7 @@ struct mxt_data {
 static bool mxt_object_readable(unsigned int type)
 {
 	switch (type) {
+	case MXT_GEN_MESSAGE_T5:
 	case MXT_GEN_COMMAND_T6:
 	case MXT_GEN_POWER_T7:
 	case MXT_GEN_ACQUIRE_T8:
-- 
1.7.9.5

[PATCH 3/7] input: atmel_mxt_ts: check mxt224 family before writing touchscreen voltage

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

only the mxt224 has a SPT_CTECONFIG_T28 object, the mxt224e doesn't

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 63a82bb..17cc15a 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -41,6 +41,10 @@
 #define MXT_OBJECT_NUM		0x06
 #define MXT_OBJECT_START	0x07
 
+/* Family IDs */
+#define MXT224_ID	0x80
+#define MXT224E_ID	0x81
+
 #define MXT_OBJECT_SIZE		6
 
 /* Object types */
@@ -687,7 +691,7 @@ static void mxt_handle_pdata(struct mxt_data *data)
 			MXT_TOUCH_YRANGE_MSB, (pdata->y_size - 1) >> 8);
 
 	/* Set touchscreen voltage */
-	if (pdata->voltage) {
+	if (pdata->voltage && data->info.family_id == MXT224_ID) {
 		if (pdata->voltage < MXT_VOLTAGE_DEFAULT) {
 			voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) /
 				MXT_VOLTAGE_STEP;
-- 
1.7.9.5

[PATCH 4/7] input: atmel_mxt_ts: allow writing of SPT_USERDATA_T38 in atmel_mxt_ts driver

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

beware: this may break boards using mxt_platform_data.config (which
is dangerous by design); boards in the kernel (mach-nuri.c, mach-goni.c)
don't use .config

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 17cc15a..7fdd439 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -309,6 +309,7 @@ static bool mxt_object_writable(unsigned int type)
 	case MXT_SPT_GPIOPWM_T19:
 	case MXT_SPT_SELFTEST_T25:
 	case MXT_SPT_CTECONFIG_T28:
+	case MXT_SPT_USERDATA_T38:
 	case MXT_SPT_DIGITIZER_T43:
 	case MXT_SPT_CTECONFIG_T46:
 		return true;
-- 
1.7.9.5

[PATCH 5/7] input: atmel_mxt_ts: only apply mxt_platform_data blen and threshold on mxt224, not mxt224e

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

on the mxt224e, the upper 4 bits of blen are gain and the threshold should probably be set in CTECONFIG

mxt_handle_pdata() is problematic as there is no way to NOT apply settings

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7fdd439..313b201 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -673,14 +673,6 @@ static void mxt_handle_pdata(struct mxt_data *data)
 	mxt_write_object(data, MXT_TOUCH_MULTI_T9, MXT_TOUCH_ORIENT,
 			pdata->orient);
 
-	/* Set touchscreen burst length */
-	mxt_write_object(data, MXT_TOUCH_MULTI_T9,
-			MXT_TOUCH_BLEN, pdata->blen);
-
-	/* Set touchscreen threshold */
-	mxt_write_object(data, MXT_TOUCH_MULTI_T9,
-			MXT_TOUCH_TCHTHR, pdata->threshold);
-
 	/* Set touchscreen resolution */
 	mxt_write_object(data, MXT_TOUCH_MULTI_T9,
 			MXT_TOUCH_XRANGE_LSB, (pdata->x_size - 1) & 0xff);
@@ -693,6 +685,14 @@ static void mxt_handle_pdata(struct mxt_data *data)
 
 	/* Set touchscreen voltage */
 	if (pdata->voltage && data->info.family_id == MXT224_ID) {
+		/* Set touchscreen burst length */
+		mxt_write_object(data, MXT_TOUCH_MULTI_T9,
+				MXT_TOUCH_BLEN, pdata->blen);
+
+		/* Set touchscreen threshold */
+		mxt_write_object(data, MXT_TOUCH_MULTI_T9,
+				MXT_TOUCH_TCHTHR, pdata->threshold);
+
 		if (pdata->voltage < MXT_VOLTAGE_DEFAULT) {
 			voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) /
 				MXT_VOLTAGE_STEP;
-- 
1.7.9.5

[PATCH 6/7] input: atmel_mxt_ts: add PROCI_ADAPTIVETHRESHOLD_T55 object readable/writable

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 313b201..7ade272 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -66,6 +66,7 @@
 #define MXT_PROCI_PALM_T41		41
 #define MXT_PROCI_TOUCHSUPPRESSION_T42	42
 #define MXT_PROCI_STYLUS_T47		47
+#define MXT_PROCI_ADAPTIVETHRESHOLD_T55 55
 #define MXT_PROCG_NOISESUPPRESSION_T48	48
 #define MXT_SPT_COMMSCONFIG_T18		18
 #define MXT_SPT_GPIOPWM_T19		19
@@ -272,6 +273,7 @@ static bool mxt_object_readable(unsigned int type)
 	case MXT_PROCI_PALM_T41:
 	case MXT_PROCI_TOUCHSUPPRESSION_T42:
 	case MXT_PROCI_STYLUS_T47:
+	case MXT_PROCI_ADAPTIVETHRESHOLD_T55:
 	case MXT_PROCG_NOISESUPPRESSION_T48:
 	case MXT_SPT_COMMSCONFIG_T18:
 	case MXT_SPT_GPIOPWM_T19:
@@ -305,6 +307,7 @@ static bool mxt_object_writable(unsigned int type)
 	case MXT_PROCI_TOUCHSUPPRESSION_T42:
 	case MXT_PROCI_STYLUS_T47:
 	case MXT_PROCG_NOISESUPPRESSION_T48:
+	case MXT_PROCI_ADAPTIVETHRESHOLD_T55:
 	case MXT_SPT_COMMSCONFIG_T18:
 	case MXT_SPT_GPIOPWM_T19:
 	case MXT_SPT_SELFTEST_T25:
-- 
1.7.9.5

[PATCH 7/7] input: atmel_mxt_ts: check if all config pdata has been written to object table

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

firmware revisions may introduce or alter device objects; error out if there
is an inconsistency (otherwise the device may silently fail)

mxt_platform_data.config is dangerous because the data must match the device object
exactly; if the order or size changes, or if device object disappear, garbage is written
to the device

Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7ade272..b85fb30 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -636,6 +636,11 @@ static int mxt_check_reg_init(struct mxt_data *data)
 		index += size;
 	}
 
+	if (index != pdata->config_length) {
+		dev_err(dev, "Config data inconsistent with device objects!\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 2/7] input: atmel_mxt_ts: allow to read/display MXT_GEN_MESSAGE_T5 object

[Daniel Kurtz]

On Thu, Feb 7, 2013 at 6:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
>
> From: Peter Meerwald <p.meerwald@bct-electronic.com>

Hi Peter,

Hmm.  Why do you want to read T5?

We specifically removed T5 from the readble list in this patch:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=55d6867fe659f4783e57db7b2ae0bb04e4ac816e

T5 is the message processor object.  Reading it will only have two
outcomes, neither of which is particularly useful:
1) the message count decrements, and a valid message will be lost
2) an invalid message will be read (reportid == 0xff)

Thanks!
-Daniel

>
> Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index fc52dc5..63a82bb 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -251,6 +251,7 @@ struct mxt_data {
>  static bool mxt_object_readable(unsigned int type)
>  {
>         switch (type) {
> +       case MXT_GEN_MESSAGE_T5:
>         case MXT_GEN_COMMAND_T6:
>         case MXT_GEN_POWER_T7:
>         case MXT_GEN_ACQUIRE_T8:
> --
> 1.7.9.5
>

Re: [PATCH 2/7] input: atmel_mxt_ts: allow to read/display MXT_GEN_MESSAGE_T5 object

[Peter Meerwald]

Hello Daniel,

> Hmm.  Why do you want to read T5?
> We specifically removed T5 from the readble list in this patch:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=55d6867fe659f4783e57db7b2ae0bb04e4ac816e

ah; haven't seen that patch

> T5 is the message processor object.  Reading it will only have two
> outcomes, neither of which is particularly useful:
> 1) the message count decrements, and a valid message will be lost
> 2) an invalid message will be read (reportid == 0xff)

I found it useful for debugging; but I agree that it interferes with 
normal operation of the driver -- T5 should not be added

regards, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: atmel_mxt_ts and mxt224e

[Yufeng Shen]

re-sent due to delivery failure on HTML tags.


On Tue, Feb 19, 2013 at 3:41 PM, Yufeng Shen <miletus@chromium.org> wrote:
>
> Hi Peter,
>
> The Chromium team has been recently working on the atmel_mxt_ts driver extensively
> (see patches history here
> http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=history;f=drivers/input/touchscreen/atmel_mxt_ts.c;h=5e40f6d75e7f7e24370e3a7d266c6ebcc06b46a6;hb=refs/heads/chromeos-3.4)
>
> As you pointed out, the chip configuration process and mxt_handle_pdata() is very fragile regarding
> different chip sets with different object layouts. So we have taken the approach to move to file based
> configuration (see patch
> http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=commit;h=95a1d96b785af4e16138e6cc7b4d01fed5b9dccb)
>
> So the platform data are moved to a config file with predefined format. Userspace can interact with the driver
> through sysfs to initiate loading a particular config file for the device. When configuration happens, config file chip ID & chip info
> block CRC are compared against the device chip ID & device chip info block to make sure the consistence between
> the config and the chip set. Then the configuration is written to the device dynamically which does not require the device reboot.
>
> Other works include exposing various FW/config attributes through sysfs, e.g. FW version and config CRC, which can be used
> for FW/config version controlling.
>
> We have been using and testing this approach for a long time and now in the process of upstreaming our patch set, so to avoid duplicate effort, it would be great if you can give some thoughts (or try out the patch set if you are interested and we would give you all the help as
> we can) on our patch set and let us iterate on it.
>
> Let me know what you think.
>
> Thanks,
>
> --- Yufeng Shen
>
>
>
>
>
> On Thu, Feb 7, 2013 at 9:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
>>
>> Hello,
>>
>> I had some issues in using the atmel_mxt_ts driver with different mxt224 parts
>> (different firmware version, mxt224 vs. mxt224); configuration of the chip
>> via mxt_platform_data.config is super-fragile when device objects change
>>
>> mxt_handle_pdata() assumes that certain object table entries are present and
>> fails on the mxt224e
>>
>> the patch series aims to support the mxt224e better and adds a message to detect
>> inconsistencies between config data and the device object table
>>
>> if there is interest I can propose a more robust mechanism to set the config data;
>> similar patches are floating around for Android in various trees
>>
>> thanks, regards, p.
>>
>> --
>> 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
>
>

Re: atmel_mxt_ts and mxt224e

[Peter Meerwald]

Hello,

> > The Chromium team has been recently working on the atmel_mxt_ts driver extensively
> > (see patches history here
> > http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=history;f=drivers/input/touchscreen/atmel_mxt_ts.c;h=5e40f6d75e7f7e24370e3a7d266c6ebcc06b46a6;hb=refs/heads/chromeos-3.4)

thank you for the pointer; I was aware of Chromiumos working on this but 
not the official place to look :)

> > As you pointed out, the chip configuration process and mxt_handle_pdata() is very fragile regarding
> > different chip sets with different object layouts. So we have taken the approach to move to file based
> > configuration (see patch
> > http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=commit;h=95a1d96b785af4e16138e6cc7b4d01fed5b9dccb)

I think that's a good idea to support robust configuration loading

> > So the platform data are moved to a config file with predefined format. Userspace can interact with the driver
> > through sysfs to initiate loading a particular config file for the device. When configuration happens, config file chip ID & chip info
> > block CRC are compared against the device chip ID & device chip info block to make sure the consistence between
> > the config and the chip set. Then the configuration is written to the device dynamically which does not require the device reboot.

but what to do with the initial configuration at _probe() time?

mxt_handle_pdata() is still there
I'm pretty sure setting of the threshold, burst length, orientation do not 
work as intended; voltage setting is still broken for mxt224e

is the plan to remove pdata support altogether? this then requires 
userspace interaction to get the device working

> > We have been using and testing this approach for a long time and now 
> > in the process of upstreaming our patch set, so to avoid duplicate 
> > effort, it would be great if you can give some thoughts (or try out the 
> > patch set if you are interested and we would give you all the help as
> > we can) on our patch set and let us iterate on it.

I have no need for flexible post-probe device configuration

my patches 1,3,4,5,7 are mostly complementary to your patch series

can the proposed file format replace the pdata config data? 
mxt_apply_pdata_config() would need to be adapted then...

regards, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 3/7] input: atmel_mxt_ts: check mxt224 family before writing touchscreen voltage

[Benson Leung]

Hi Peter,

Is the problem that the 224e is missing T28 object?

This change seems extremely 224/224e specific. This driver, however,
supports Atmel touch devices that are neither 224 or 224e, many have
the  SPT_CTECONFIG_T28 object.

The driver does read out a supported object table from the device
during initialize before handle_pdata. What sort of problem is there
in trying to write the voltage to a non existent T28 object?  If the
object is missing, then mxt_write_object for voltage should simply
return -EINVAL, and no harm should be done.


On Thu, Feb 7, 2013 at 6:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> From: Peter Meerwald <p.meerwald@bct-electronic.com>
>
> only the mxt224 has a SPT_CTECONFIG_T28 object, the mxt224e doesn't
>
> Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 63a82bb..17cc15a 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -41,6 +41,10 @@
>  #define MXT_OBJECT_NUM         0x06
>  #define MXT_OBJECT_START       0x07
>
> +/* Family IDs */
> +#define MXT224_ID      0x80
> +#define MXT224E_ID     0x81
> +
>  #define MXT_OBJECT_SIZE                6
>
>  /* Object types */
> @@ -687,7 +691,7 @@ static void mxt_handle_pdata(struct mxt_data *data)
>                         MXT_TOUCH_YRANGE_MSB, (pdata->y_size - 1) >> 8);
>
>         /* Set touchscreen voltage */
> -       if (pdata->voltage) {
> +       if (pdata->voltage && data->info.family_id == MXT224_ID) {
As an alternative, could you not check for the existence of the
voltage object in the table here using mxt_get_object rather than
specifically switching on family ID number?
>                 if (pdata->voltage < MXT_VOLTAGE_DEFAULT) {
>                         voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) /
>                                 MXT_VOLTAGE_STEP;
> --
> 1.7.9.5
>
> --
> 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

Thanks.

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

Re: atmel_mxt_ts and mxt224e

[Yufeng Shen]

On Wed, Feb 20, 2013 at 5:50 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> Hello,
>
>> > The Chromium team has been recently working on the atmel_mxt_ts driver extensively
>> > (see patches history here
>> > http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=history;f=drivers/input/touchscreen/atmel_mxt_ts.c;h=5e40f6d75e7f7e24370e3a7d266c6ebcc06b46a6;hb=refs/heads/chromeos-3.4)
>
> thank you for the pointer; I was aware of Chromiumos working on this but
> not the official place to look :)
>
>> > As you pointed out, the chip configuration process and mxt_handle_pdata() is very fragile regarding
>> > different chip sets with different object layouts. So we have taken the approach to move to file based
>> > configuration (see patch
>> > http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=commit;h=95a1d96b785af4e16138e6cc7b4d01fed5b9dccb)
>
> I think that's a good idea to support robust configuration loading
>
>> > So the platform data are moved to a config file with predefined format. Userspace can interact with the driver
>> > through sysfs to initiate loading a particular config file for the device. When configuration happens, config file chip ID & chip info
>> > block CRC are compared against the device chip ID & device chip info block to make sure the consistence between
>> > the config and the chip set. Then the configuration is written to the device dynamically which does not require the device reboot.
>
> but what to do with the initial configuration at _probe() time?
>

In order to not break the existing code path, we keep the
mxt_handle_pdata() there
but add the support that if pdata is NULL, then we skip applying the
configuration from
pdata, see
http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=commit;h=5650201eb029de18517c40610d8b4122aff7369f

As noted in the commit message, most of the time the device only need
to be configured once and later on it
can just use backed up config from NVRAM. When using pdata to config
device, the config is written to device
memory every time the device boots (or probed), which adds latency to
boot time. While in our dev process, we
pre-flash the device with a workable config, set pdata to NULL, and
most of the time the device just comes up
with the right config without need of re-configuration. And if a
different config is needed later, we do the reconfig
by loading the new config from file and back it up in NVRAM.

> mxt_handle_pdata() is still there
> I'm pretty sure setting of the threshold, burst length, orientation do not
> work as intended; voltage setting is still broken for mxt224e
>

Right. If you have to take the path of mxt_handle_pdata(), you will
have to branch on different device models.
Your patch 3 does the quick hack, but as more device models need to be
supported, adapting mxt_handle_pdata()
does not scale well, while loading a corresponding config file for a
specific chipset is more preferable.

> is the plan to remove pdata support altogether? this then requires
> userspace interaction to get the device working
>

see notes above.

>> > We have been using and testing this approach for a long time and now
>> > in the process of upstreaming our patch set, so to avoid duplicate
>> > effort, it would be great if you can give some thoughts (or try out the
>> > patch set if you are interested and we would give you all the help as
>> > we can) on our patch set and let us iterate on it.
>
> I have no need for flexible post-probe device configuration
>
> my patches 1,3,4,5,7 are mostly complementary to your patch series
>

> can the proposed file format replace the pdata config data?
> mxt_apply_pdata_config() would need to be adapted then...
>
see the above comments.

> regards, p.
>
> --
>
> Peter Meerwald
> +43-664-2444418 (mobile)

Re: [PATCH 5/7] input: atmel_mxt_ts: only apply mxt_platform_data blen and threshold on mxt224, not mxt224e

[Yufeng Shen]

On Thu, Feb 7, 2013 at 9:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> From: Peter Meerwald <p.meerwald@bct-electronic.com>
>
> on the mxt224e, the upper 4 bits of blen are gain and the threshold should probably be set in CTECONFIG
>
> mxt_handle_pdata() is problematic as there is no way to NOT apply settings
>
> Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 7fdd439..313b201 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -673,14 +673,6 @@ static void mxt_handle_pdata(struct mxt_data *data)
>         mxt_write_object(data, MXT_TOUCH_MULTI_T9, MXT_TOUCH_ORIENT,
>                         pdata->orient);
>
> -       /* Set touchscreen burst length */
> -       mxt_write_object(data, MXT_TOUCH_MULTI_T9,
> -                       MXT_TOUCH_BLEN, pdata->blen);
> -
> -       /* Set touchscreen threshold */
> -       mxt_write_object(data, MXT_TOUCH_MULTI_T9,
> -                       MXT_TOUCH_TCHTHR, pdata->threshold);
> -
>         /* Set touchscreen resolution */
>         mxt_write_object(data, MXT_TOUCH_MULTI_T9,
>                         MXT_TOUCH_XRANGE_LSB, (pdata->x_size - 1) & 0xff);
> @@ -693,6 +685,14 @@ static void mxt_handle_pdata(struct mxt_data *data)
>
>         /* Set touchscreen voltage */
>         if (pdata->voltage && data->info.family_id == MXT224_ID) {

As Benson noted in patch 5, what about the device that is neither 224
nor 224E ? burst length
and  threshold will no longer be applied.

> +               /* Set touchscreen burst length */
> +               mxt_write_object(data, MXT_TOUCH_MULTI_T9,
> +                               MXT_TOUCH_BLEN, pdata->blen);
> +
> +               /* Set touchscreen threshold */
> +               mxt_write_object(data, MXT_TOUCH_MULTI_T9,
> +                               MXT_TOUCH_TCHTHR, pdata->threshold);
> +
>                 if (pdata->voltage < MXT_VOLTAGE_DEFAULT) {
>                         voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) /
>                                 MXT_VOLTAGE_STEP;
> --
> 1.7.9.5
>
> --
> 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

Re: [PATCH 5/7] input: atmel_mxt_ts: only apply mxt_platform_data blen and threshold on mxt224, not mxt224e

[Benson Leung]

Furthermore, this patch has the effect of only applying blen and
tchthr when voltage is present.

If this is a 224 device, but the platform data does not specify
voltage (say, we are ok with the existing value loaded from NVRAM),
then we won't apply blen and tchthr either.


On Wed, Feb 20, 2013 at 10:29 AM, Yufeng Shen <miletus@google.com> wrote:
> On Thu, Feb 7, 2013 at 9:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
>>         /* Set touchscreen voltage */
>>         if (pdata->voltage && data->info.family_id == MXT224_ID) {
>
> As Benson noted in patch 5, what about the device that is neither 224
> nor 224E ? burst length
> and  threshold will no longer be applied.
>


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

Re: [PATCH 3/7] input: atmel_mxt_ts: check mxt224 family before writing touchscreen voltage

[Peter Meerwald]

Hello Benson,

> Is the problem that the 224e is missing T28 object?
> This change seems extremely 224/224e specific. This driver, however,
> supports Atmel touch devices that are neither 224 or 224e, many have
> the  SPT_CTECONFIG_T28 object.

yes, I only have access to 224 and 224e; it is not clear what devices are 
supposed to be supported

> The driver does read out a supported object table from the device
> during initialize before handle_pdata. What sort of problem is there
> in trying to write the voltage to a non existent T28 object?  If the
> object is missing, then mxt_write_object for voltage should simply
> return -EINVAL, and no harm should be done.

a bogus error message is written, 'Invalid object type' in 
mxt_get_object()

> On Thu, Feb 7, 2013 at 6:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> > From: Peter Meerwald <p.meerwald@bct-electronic.com>
> >
> > only the mxt224 has a SPT_CTECONFIG_T28 object, the mxt224e doesn't
> >
> > Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 63a82bb..17cc15a 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -41,6 +41,10 @@
> >  #define MXT_OBJECT_NUM         0x06
> >  #define MXT_OBJECT_START       0x07
> >
> > +/* Family IDs */
> > +#define MXT224_ID      0x80
> > +#define MXT224E_ID     0x81
> > +
> >  #define MXT_OBJECT_SIZE                6
> >
> >  /* Object types */
> > @@ -687,7 +691,7 @@ static void mxt_handle_pdata(struct mxt_data *data)
> >                         MXT_TOUCH_YRANGE_MSB, (pdata->y_size - 1) >> 8);
> >
> >         /* Set touchscreen voltage */
> > -       if (pdata->voltage) {
> > +       if (pdata->voltage && data->info.family_id == MXT224_ID) {
> As an alternative, could you not check for the existence of the
> voltage object in the table here using mxt_get_object rather than
> specifically switching on family ID number?

yes, I think that is a good suggestion

> >                 if (pdata->voltage < MXT_VOLTAGE_DEFAULT) {
> >                         voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) /
> >                                 MXT_VOLTAGE_STEP;

regards, p. 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 3/7] input: atmel_mxt_ts: check mxt224 family before writing touchscreen voltage

[Benson Leung]

On Wed, Feb 20, 2013 at 2:22 PM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> Hello Benson,

>> The driver does read out a supported object table from the device
>> during initialize before handle_pdata. What sort of problem is there
>> in trying to write the voltage to a non existent T28 object?  If the
>> object is missing, then mxt_write_object for voltage should simply
>> return -EINVAL, and no harm should be done.
>
> a bogus error message is written, 'Invalid object type' in
> mxt_get_object()
>

Ah, I see. There is a dev_err in mxt_get_object. In that case, my
suggestion to check mxt_get_object will also throw the same bogus
error message. Maybe that should be a dev_warn instead?

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

Re: [PATCH 5/7] input: atmel_mxt_ts: only apply mxt_platform_data blen and threshold on mxt224, not mxt224e

[Peter Meerwald]

Hello,

> Furthermore, this patch has the effect of only applying blen and
> tchthr when voltage is present.
 
> If this is a 224 device, but the platform data does not specify
> voltage (say, we are ok with the existing value loaded from NVRAM),
> then we won't apply blen and tchthr either.

I think all those specific settings in handle_pdata() should be removed 
altogether:

burst length is only the upper 4 bits actually;
for the touchscreen threshold I think there are additional registers 
elsewhere which should be kept in sync

expect not using pdata at all, there is no way to NOT make these settings

I think there are other places (on the mxt224e) as well where the 
orientation has to be set, so pdata->orient setting 
MXT_TOUCH_MULTI_T9@MXT_TOUCH_ORIENT is only half of the story, you still 
need correct pdata->config and pdata->orient is basically useless

the idea of the patch was to preserve the behaviour on the mxt224 but 
disable at least those two dubious settings; clearly not perfect

> On Wed, Feb 20, 2013 at 10:29 AM, Yufeng Shen <miletus@google.com> wrote:
> > On Thu, Feb 7, 2013 at 9:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> >>         /* Set touchscreen voltage */
> >>         if (pdata->voltage && data->info.family_id == MXT224_ID) {
> >
> > As Benson noted in patch 5, what about the device that is neither 224
> > nor 224E ? burst length
> > and  threshold will no longer be applied.

regards, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)