Input: atmel_mxt_ts - Make CHG line high after the interrupts are enabled

PATCH In review Iiro Valkonen 2 participants 7 messages Mar 17, 2011 – Apr 13, 2011

Subsystems (3)

HID Input drivers Atmel maxtouch driver

lore

Share & tools

lore

Iiro Valkonen

author

2011-03-17 14:45 UTC #0 lore raw

PATCH 2/2 Input: atmel_mxt_ts - Make CHG line high after the interrupts are enabled

Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.

Signed-off-by Iiro Valkonen <[email protected]>

---
 drivers/input/touchscreen/atmel_mxt_ts.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/input/touchscreen/atmel_mxt_ts.c +4 -4

index f8cd478..ca2062a 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		return error;
 
-	error = mxt_make_highchg(data);
-	if (error)
-		return error;
-
 	mxt_handle_pdata(data);
 
 	/* Backup to memory */

@@ -1098,6 +1094,10 @@ static int __devinit mxt_probe(struct i2c_client *client,
 	if (error)
 		goto err_unregister_device;
 
+	error = mxt_make_highchg(data);
+	if (error)
+		goto err_unregister_device;
+
 	return 0;
 
 err_unregister_device:

Signature · 1 line

1.7.0.4

Joonyoung Shim

2011-03-21 11:56 UTC #1 lore raw

Hi, Iiro.

2011/3/17 Iiro Valkonen <[email protected]>:

Quoted 23 lines

Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.
Signed-off-by Iiro Valkonen <[email protected]>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/input/touchscreen/atmel_mxt_ts.c +0 -4
index f8cd478..ca2062a 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
        if (error)
                return error;
 
-       error = mxt_make_highchg(data);
-       if (error)
-               return error;
-
        mxt_handle_pdata(data);
 
        /* Backup to memory */

drivers/input/touchscreen/atmel_mxt_ts.c static int __devinit mxt_probe(struct i2c_client *client, · line 1100

Quoted 7 lines

drivers/input/touchscreen/atmel_mxt_ts.c +4 -0

@@ -1098,6 +1094,10 @@ static int __devinit mxt_probe(struct i2c_client *client,
        if (error)
                goto err_unregister_device;
 
+       error = mxt_make_highchg(data);
+       if (error)
+               goto err_unregister_device;
+

You are missing one error handling about sysfs_create_group() and please consider firmware update flow also.

Thanks.

Quoted 10 lines

drivers/input/touchscreen/atmel_mxt_ts.c +0 -0
        return 0;
 
err_unregister_device:
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines

- Joonyoung Shim
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Iiro Valkonen

author

2011-04-01 14:04 UTC #2 lore raw

PATCH v2 2/2 Input: atmel_mxt_ts - Make CHG line high after the interrupts are enabled

This is version 2 of the patch, where sysfs group is correctly removed in case of error, and the firmware update is also considered (mxt_make_highchg is also called from mxt_initialize).

Original description:

Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.

Signed-off-by Iiro Valkonen <[email protected]>

---
 drivers/input/touchscreen/atmel_mxt_ts.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

drivers/input/touchscreen/atmel_mxt_ts.c +10 -4

index 4012436..08ea846 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		return error;
 
-	error = mxt_make_highchg(data);
-	if (error)
-		return error;
-
 	mxt_handle_pdata(data);
 
 	/* Backup to memory */

@@ -832,6 +828,10 @@ static int mxt_initialize(struct mxt_data *data)
 		return error;
 	info->matrix_ysize = val;
 
+	error = mxt_make_highchg(data);
+	if (error)
+		return error;
+
 	dev_info(&client->dev,
 			"Family ID: %d Variant ID: %d Version: %d Build: %d\n",
 			info->family_id, info->variant_id, info->version,

@@ -1098,8 +1098,14 @@ static int __devinit mxt_probe(struct i2c_client *client,
 	if (error)
 		goto err_unregister_device;
 
+	error = mxt_make_highchg(data);
+	if (error)
+		goto err_remove_sysfs_group;
+
 	return 0;
 
+err_remove_sysfs_group:
+	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 err_unregister_device:
 	input_unregister_device(input_dev);
 	input_dev = NULL;

Signature · 1 line

1.7.0.4

Joonyoung Shim

2011-04-08 04:56 UTC #3 lore raw

Hi,

2011/4/1 Iiro Valkonen <[email protected]>:

Quoted 27 lines

This is version 2 of the patch, where sysfs group is correctly removed in case of error, and the firmware update is also considered (mxt_make_highchg is also called from mxt_initialize).
Original description:
Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.
Signed-off-by Iiro Valkonen <[email protected]>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/input/touchscreen/atmel_mxt_ts.c +0 -4
index 4012436..08ea846 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
        if (error)
                return error;
 
-       error = mxt_make_highchg(data);
-       if (error)
-               return error;
-
        mxt_handle_pdata(data);
 
        /* Backup to memory */

drivers/input/touchscreen/atmel_mxt_ts.c static int mxt_initialize(struct mxt_data *data) · line 834

Quoted 7 lines

drivers/input/touchscreen/atmel_mxt_ts.c +4 -0

@@ -832,6 +828,10 @@ static int mxt_initialize(struct mxt_data *data)
                return error;
        info->matrix_ysize = val;
 
+       error = mxt_make_highchg(data);
+       if (error)
+               return error;
+

This is unnecessary patch. If you add this for firmware update part, how about move to mxt_update_fw_store()?

drivers/input/touchscreen/atmel_mxt_ts.c static int __devinit mxt_probe(struct i2c_client *client, · line 1104

Quoted 11 lines

drivers/input/touchscreen/atmel_mxt_ts.c +4 -0

        dev_info(&client->dev,
                        "Family ID: %d Variant ID: %d Version: %d Build: %d\n",
                        info->family_id, info->variant_id, info->version,

@@ -1098,8 +1098,14 @@ static int __devinit mxt_probe(struct i2c_client *client,
        if (error)
                goto err_unregister_device;
 
+       error = mxt_make_highchg(data);
+       if (error)
+               goto err_remove_sysfs_group;
+

This is ok, but how about move this to the below request_threaded_irq() instead of adding error handling?

Quoted 10 lines

drivers/input/touchscreen/atmel_mxt_ts.c +2 -0
        return 0;
 
+err_remove_sysfs_group:
+       sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
err_unregister_device:
input_unregister_device(input_dev);
input_dev = NULL;
--
1.7.0.4

Thanks.

Signature · 5 lines

- Joonyoung Shim
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Iiro Valkonen

author

2011-04-08 13:00 UTC #4 lore raw

Hello,

Quoted 33 lines · Joonyoung Shim

Hi,
2011/4/1 Iiro Valkonen <[email protected]>:
Quoted 30 lines
This is version 2 of the patch, where sysfs group is correctly removed in case of error, and the firmware update is also considered (mxt_make_highchg is also called from mxt_initialize).
Original description:
Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.
Signed-off-by Iiro Valkonen <[email protected]>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/input/touchscreen/atmel_mxt_ts.c +0 -4
index 4012436..08ea846 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
       if (error)
               return error;
 
-       error = mxt_make_highchg(data);
-       if (error)
-               return error;
-
       mxt_handle_pdata(data);
 
       /* Backup to memory */
@@ -832,6 +828,10 @@ static int mxt_initialize(struct mxt_data *data)
               return error;
       info->matrix_ysize = val;
 

Quoted 8 lines

drivers/input/touchscreen/atmel_mxt_ts.c static int mxt_initialize(struct mxt_data *data) · line 834
Quoted 4 lines
drivers/input/touchscreen/atmel_mxt_ts.c +4 -0
+       error = mxt_make_highchg(data);
+       if (error)
+               return error;
+
This is unnecessary patch. If you add this for firmware update part, how about move to mxt_update_fw_store()?

Good call.

Quoted 7 lines

drivers/input/touchscreen/atmel_mxt_ts.c +0 -0

       dev_info(&client->dev,
                       "Family ID: %d Variant ID: %d Version: %d Build: %d\n",
                       info->family_id, info->variant_id, info->version,

@@ -1098,8 +1098,14 @@ static int __devinit mxt_probe(struct i2c_client *client,
       if (error)
               goto err_unregister_device;

Quoted 8 lines

drivers/input/touchscreen/atmel_mxt_ts.c static int __devinit mxt_probe(struct i2c_client *client, · line 1104
Quoted 4 lines
drivers/input/touchscreen/atmel_mxt_ts.c +4 -0
+       error = mxt_make_highchg(data);
+       if (error)
+               goto err_remove_sysfs_group;
+
This is ok, but how about move this to the below request_threaded_irq() instead of adding error handling?

Makes sense, will move it.

Thanks for the feedback.

BR,

Signature · 1 line

Iiro

Iiro Valkonen

author

2011-04-08 13:18 UTC #5 lore raw

In reply to Joonyoung Shim

Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.

Signed-off-by Iiro Valkonen <[email protected]>

---
v3: Change the locations where this is done to ensure correct operation at FW update,
and to get rid of some error handling

 drivers/input/touchscreen/atmel_mxt_ts.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

drivers/input/touchscreen/atmel_mxt_ts.c +8 -4

index 4012436..5f09f57 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		return error;
 
-	error = mxt_make_highchg(data);
-	if (error)
-		return error;
-
 	mxt_handle_pdata(data);
 
 	/* Backup to memory */

@@ -981,6 +977,10 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 
 	enable_irq(data->irq);
 
+	error = mxt_make_highchg(data);
+	if (error)
+		return error;
+
 	return count;
 }

@@ -1090,6 +1090,10 @@ static int __devinit mxt_probe(struct i2c_client *client,
 		goto err_free_object;
 	}
 
+	error = mxt_make_highchg(data);
+	if (error)
+		goto err_free_irq;
+
 	error = input_register_device(input_dev);
 	if (error)
 		goto err_free_irq;

Signature · 1 line

1.7.0.4

Joonyoung Shim

Acked-by

2011-04-13 05:17 UTC #6 lore raw

2011/4/8 Iiro Valkonen <[email protected]>:

Quoted 49 lines

Make the CHG line (interrupt line) go high after the interrupts have been enabled to make sure we don't miss the falling edge.

Signed-off-by Iiro Valkonen <[email protected]>

---
v3: Change the locations where this is done to ensure correct operation at FW update, and to get rid of some error handling

drivers/input/touchscreen/atmel_mxt_ts.c | 12 ++++++++----

 1 files changed, 8 insertions(+), 4 deletions(-)

drivers/input/touchscreen/atmel_mxt_ts.c +8 -5

index 4012436..5f09f57 100644
@@ -804,10 +804,6 @@ static int mxt_initialize(struct mxt_data *data)
        if (error)
                return error;
 
-       error = mxt_make_highchg(data);
-       if (error)
-               return error;
-
        mxt_handle_pdata(data);
 
        /* Backup to memory */

@@ -981,6 +977,10 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 
        enable_irq(data->irq);
 
+       error = mxt_make_highchg(data);
+       if (error)
+               return error;
+
        return count;
  }

@@ -1090,6 +1090,10 @@ static int __devinit mxt_probe(struct i2c_client *client,
                goto err_free_object;
        }
 
+       error = mxt_make_highchg(data);
+       if (error)
+               goto err_free_irq;
+
        error = input_register_device(input_dev);
        if (error)
                goto err_free_irq;
--

Quoted 3 lines

1.7.0.4

Acked-by Joonyoung Shim <[email protected]>

Thanks.

Signature · 5 lines

- Joonyoung Shim
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html