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

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

[Iiro Valkonen]

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 <iiro.valkonen@atmel.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index f8cd478..ca2062a 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -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:
-- 
1.7.0.4

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

[Joonyoung Shim]

Hi, Iiro.

2011/3/17 Iiro Valkonen <iiro.valkonen@atmel.com>:
> 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 <iiro.valkonen@atmel.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index f8cd478..ca2062a 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -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;
> +

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

Thanks.

>        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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
- Joonyoung Shim
--
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

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

[Iiro Valkonen]

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 <iiro.valkonen@atmel.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 4012436..08ea846 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -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;
-- 
1.7.0.4

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

[Joonyoung Shim]

Hi,

2011/4/1 Iiro Valkonen <iiro.valkonen@atmel.com>:
> 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 <iiro.valkonen@atmel.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 4012436..08ea846 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -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;
> +

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

>        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?

>        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.

-- 
- Joonyoung Shim
--
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 2/2 v3] Input: atmel_mxt_ts - Make CHG line high after the interrupts are enabled

[Iiro Valkonen]

Hello,

On 04/08/2011 07:56 AM, Joonyoung Shim wrote:
> Hi,
> 
> 2011/4/1 Iiro Valkonen <iiro.valkonen@atmel.com>:
>> 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 <iiro.valkonen@atmel.com>
>> ---
>>  drivers/input/touchscreen/atmel_mxt_ts.c |   14 ++++++++++----
>>  1 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 4012436..08ea846 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -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;
>> +
> 
> This is unnecessary patch. If you add this for firmware update part,
> how about move to mxt_update_fw_store()?
> 

Good call.

>>        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?
> 

Makes sense, will move it.

Thanks for the feedback.

BR,

-- 
Iiro

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

[Iiro Valkonen]

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 <iiro.valkonen@atmel.com>
---
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(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 4012436..5f09f57 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -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;
-- 
1.7.0.4

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

[Joonyoung Shim]

2011/4/8 Iiro Valkonen <iiro.valkonen@atmel.com>:
> 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 <iiro.valkonen@atmel.com>
> ---
> 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(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 4012436..5f09f57 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -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;
> --
> 1.7.0.4
>
>

Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>

Thanks.

-- 
- Joonyoung Shim
--
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