* [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
@ 2013-12-17 20:16 Christopher Heiny
2013-12-18 14:39 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Christopher Heiny @ 2013-12-17 20:16 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
This patch fixes two bugs in handling of the RMI4 attention line GPIO.
1) in enable_sensor(), make sure the attn_gpio is defined before attempting to
get its value.
2) in rmi_driver_probe(), declare the name of the attn_gpio, then
request the attn_gpio before attempting to export it.
Also introduces a GPIO_LABEL constant for identifying the attention GPIO.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index a30c7d3..33fb8f8 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -169,7 +169,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
data->enabled = true;
- if (!pdata->level_triggered &&
+ if (pdata->attn_gpio && !pdata->level_triggered &&
gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
retval = process_interrupt_requests(rmi_dev);
@@ -807,6 +807,8 @@ static int rmi_driver_remove(struct device *dev)
return 0;
}
+static const char GPIO_LABEL[] = "attn";
+
static int rmi_driver_probe(struct device *dev)
{
struct rmi_driver *rmi_driver;
@@ -959,20 +961,24 @@ static int rmi_driver_probe(struct device *dev)
}
if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
- retval = gpio_export(pdata->attn_gpio, false);
+ retval = gpio_request(pdata->attn_gpio, GPIO_LABEL);
if (retval) {
- dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
- retval = 0;
+ dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code: %d.\n",
+ pdata->attn_gpio, retval);
} else {
- retval = gpio_export_link(dev,
- "attn", pdata->attn_gpio);
+ retval = gpio_export(pdata->attn_gpio, false);
if (retval) {
- dev_warn(dev,
- "WARNING: Failed to symlink ATTN gpio!\n");
- retval = 0;
+ dev_warn(dev, "WARNING: Failed to export ATTN gpio %d, code: %d.\n",
+ pdata->attn_gpio, retval);
} else {
- dev_info(dev, "Exported ATTN GPIO %d.",
- pdata->attn_gpio);
+ retval = gpio_export_link(dev, GPIO_LABEL,
+ pdata->attn_gpio);
+ if (retval)
+ dev_warn(dev,
+ "WARNING: Failed to symlink ATTN gpio!\n");
+ else
+ dev_info(dev, "Exported ATTN GPIO %d.",
+ pdata->attn_gpio);
}
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
2013-12-17 20:16 Christopher Heiny
@ 2013-12-18 14:39 ` Dmitry Torokhov
2013-12-19 1:23 ` Christopher Heiny
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2013-12-18 14:39 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
Hi Chris,
On Tue, Dec 17, 2013 at 12:16:01PM -0800, Christopher Heiny wrote:
> This patch fixes two bugs in handling of the RMI4 attention line GPIO.
>
> 1) in enable_sensor(), make sure the attn_gpio is defined before attempting to
> get its value.
>
> 2) in rmi_driver_probe(), declare the name of the attn_gpio, then
> request the attn_gpio before attempting to export it.
>
> Also introduces a GPIO_LABEL constant for identifying the attention GPIO.
>
I was looking at the patch some more and I have some concerns with it.
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index a30c7d3..33fb8f8 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -169,7 +169,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>
> data->enabled = true;
>
> - if (!pdata->level_triggered &&
> + if (pdata->attn_gpio && !pdata->level_triggered &&
O is perfectly fine GPIO number, you want to use gpio_is_valid() hete. I
also wonder why do you need such elaborate check. Can we simply "flush"
device before enabling interrupts?
> gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
> retval = process_interrupt_requests(rmi_dev);
>
> @@ -807,6 +807,8 @@ static int rmi_driver_remove(struct device *dev)
> return 0;
> }
>
> +static const char GPIO_LABEL[] = "attn";
> +
> static int rmi_driver_probe(struct device *dev)
> {
> struct rmi_driver *rmi_driver;
> @@ -959,20 +961,24 @@ static int rmi_driver_probe(struct device *dev)
> }
>
> if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
> - retval = gpio_export(pdata->attn_gpio, false);
> + retval = gpio_request(pdata->attn_gpio, GPIO_LABEL);
Here it is too late to request GPIO. You have been converting it to IRQ,
enabling that IRQ and calling gpio_get_value() so GPIO should have
already been requested by now.
So you need to move this code up. You may also consider using
gpio_request_one() and use GPIOF_EXPORT flag if you want to export it.
It would also be nice to set the direction (GPIOF_DIR_IN).
I also do not see matching call to gpio_free() in remove().
> if (retval) {
> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
> - retval = 0;
> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code: %d.\n",
> + pdata->attn_gpio, retval);
> } else {
> - retval = gpio_export_link(dev,
> - "attn", pdata->attn_gpio);
> + retval = gpio_export(pdata->attn_gpio, false);
> if (retval) {
> - dev_warn(dev,
> - "WARNING: Failed to symlink ATTN gpio!\n");
> - retval = 0;
> + dev_warn(dev, "WARNING: Failed to export ATTN gpio %d, code: %d.\n",
> + pdata->attn_gpio, retval);
> } else {
> - dev_info(dev, "Exported ATTN GPIO %d.",
> - pdata->attn_gpio);
> + retval = gpio_export_link(dev, GPIO_LABEL,
> + pdata->attn_gpio);
> + if (retval)
> + dev_warn(dev,
> + "WARNING: Failed to symlink ATTN gpio!\n");
> + else
> + dev_info(dev, "Exported ATTN GPIO %d.",
> + pdata->attn_gpio);
> }
> }
> }
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
2013-12-18 14:39 ` Dmitry Torokhov
@ 2013-12-19 1:23 ` Christopher Heiny
0 siblings, 0 replies; 6+ messages in thread
From: Christopher Heiny @ 2013-12-19 1:23 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
On 12/18/2013 06:39 AM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Tue, Dec 17, 2013 at 12:16:01PM -0800, Christopher Heiny wrote:
>> This patch fixes two bugs in handling of the RMI4 attention line GPIO.
>>
>> 1) in enable_sensor(), make sure the attn_gpio is defined before attempting to
>> get its value.
>>
>> 2) in rmi_driver_probe(), declare the name of the attn_gpio, then
>> request the attn_gpio before attempting to export it.
>>
>> Also introduces a GPIO_LABEL constant for identifying the attention GPIO.
>>
>
> I was looking at the patch some more and I have some concerns with it.
>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 28 +++++++++++++++++-----------
>> 1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index a30c7d3..33fb8f8 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -169,7 +169,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>>
>> data->enabled = true;
>>
>> - if (!pdata->level_triggered &&
>> + if (pdata->attn_gpio && !pdata->level_triggered &&
>
> O is perfectly fine GPIO number, you want to use gpio_is_valid() hete. I
> also wonder why do you need such elaborate check. Can we simply "flush"
> device before enabling interrupts?
Hmmm. gpio_is_valid() is a good suggestion. However, I think we can do
away with the whole check on the ATTN gpio, and just call
process_interrupt_requests(). That will both flush the state and handle
any important pending events. In the typical use case for
enable_sensor(), the RMI4 device will either be just coming up or else
coming out of a diagnostic mode, and there will at least be a status
event to handle. In the off-case where there is nothing pending (that
is, ATTN not asserted), the overhead is pretty low - just a quick read
of the interrupt status register.
>
>> gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
>> retval = process_interrupt_requests(rmi_dev);
>>
>> @@ -807,6 +807,8 @@ static int rmi_driver_remove(struct device *dev)
>> return 0;
>> }
>>
>> +static const char GPIO_LABEL[] = "attn";
>> +
>> static int rmi_driver_probe(struct device *dev)
>> {
>> struct rmi_driver *rmi_driver;
>> @@ -959,20 +961,24 @@ static int rmi_driver_probe(struct device *dev)
>> }
>>
>> if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
>> - retval = gpio_export(pdata->attn_gpio, false);
>> + retval = gpio_request(pdata->attn_gpio, GPIO_LABEL);
>
> Here it is too late to request GPIO. You have been converting it to IRQ,
> enabling that IRQ and calling gpio_get_value() so GPIO should have
> already been requested by now.
>
> So you need to move this code up.
I'll give that a try.
> You may also consider using
> gpio_request_one() and use GPIOF_EXPORT flag if you want to export it.
> It would also be nice to set the direction (GPIOF_DIR_IN).
Both of these are good ideas.
> I also do not see matching call to gpio_free() in remove().
Neither do I :-(.
We'll update.
>
>> if (retval) {
>> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
>> - retval = 0;
>> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code: %d.\n",
>> + pdata->attn_gpio, retval);
>> } else {
>> - retval = gpio_export_link(dev,
>> - "attn", pdata->attn_gpio);
>> + retval = gpio_export(pdata->attn_gpio, false);
>> if (retval) {
>> - dev_warn(dev,
>> - "WARNING: Failed to symlink ATTN gpio!\n");
>> - retval = 0;
>> + dev_warn(dev, "WARNING: Failed to export ATTN gpio %d, code: %d.\n",
>> + pdata->attn_gpio, retval);
>> } else {
>> - dev_info(dev, "Exported ATTN GPIO %d.",
>> - pdata->attn_gpio);
>> + retval = gpio_export_link(dev, GPIO_LABEL,
>> + pdata->attn_gpio);
>> + if (retval)
>> + dev_warn(dev,
>> + "WARNING: Failed to symlink ATTN gpio!\n");
>> + else
>> + dev_info(dev, "Exported ATTN GPIO %d.",
>> + pdata->attn_gpio);
>> }
>> }
>> }
>
> Thanks.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
@ 2013-12-24 2:44 Christopher Heiny
2013-12-26 22:37 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Christopher Heiny @ 2013-12-24 2:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
This patch fixes some bugs in handling of the RMI4 attention line GPIO.
1) in enable_sensor(), eliminate the complicated check on ATTN and just
call process_interrupt_requests(). This will have minimal overhead if ATTN
is not asserted, and clears the state of the RMI4 device in any case.
2) Correctly free the GPIO in rmi_driver_remove().
3) in rmi_driver_probe()
- declare the name of the attention gpio (GPIO_LABEL)
- use gpio_request_one() to get the gpio and export it.
- simplify conditional gpio acquisition logic and combine with interrupt
setup
4) use gpio_is_valid() instead of comparing to 0.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 43 ++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index a30c7d3..9b02358 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
struct rmi_phys_device *rmi_phys;
int retval = 0;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
if (data->enabled)
return 0;
@@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
data->enabled = true;
- if (!pdata->level_triggered &&
- gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
- retval = process_interrupt_requests(rmi_dev);
-
- return retval;
+ return process_interrupt_requests(rmi_dev);
}
static void rmi_free_function_list(struct rmi_device *rmi_dev)
@@ -800,13 +795,20 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume);
static int rmi_driver_remove(struct device *dev)
{
struct rmi_device *rmi_dev = to_rmi_device(dev);
+ const struct rmi_device_platform_data *pdata =
+ to_rmi_platform_data(rmi_dev);
disable_sensor(rmi_dev);
rmi_free_function_list(rmi_dev);
+ if (gpio_is_valid(pdata->attn_gpio))
+ gpio_free(pdata->attn_gpio);
+
return 0;
}
+static const char GPIO_LABEL[] = "attn";
+
static int rmi_driver_probe(struct device *dev)
{
struct rmi_driver *rmi_driver;
@@ -937,7 +939,7 @@ static int rmi_driver_probe(struct device *dev)
mutex_init(&data->suspend_mutex);
}
- if (pdata->attn_gpio) {
+ if (gpio_is_valid(pdata->attn_gpio)) {
data->irq = gpio_to_irq(pdata->attn_gpio);
if (pdata->level_triggered) {
data->irq_flags = IRQF_ONESHOT |
@@ -948,24 +950,17 @@ static int rmi_driver_probe(struct device *dev)
(pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
}
- } else
- data->poll_interval = ktime_set(0,
- (pdata->poll_interval_ms ? pdata->poll_interval_ms :
- DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000);
-
- if (data->f01_container->dev.driver) {
- /* Driver already bound, so enable ATTN now. */
- enable_sensor(rmi_dev);
- }
- if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
- retval = gpio_export(pdata->attn_gpio, false);
+ retval = gpio_request_one(pdata->attn_gpio,
+ GPIOF_EXPORT | GPIOF_DIR_IN,
+ GPIO_LABEL);
if (retval) {
- dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
+ dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n",
+ pdata->attn_gpio, retval);
retval = 0;
} else {
retval = gpio_export_link(dev,
- "attn", pdata->attn_gpio);
+ GPIO_LABEL, pdata->attn_gpio);
if (retval) {
dev_warn(dev,
"WARNING: Failed to symlink ATTN gpio!\n");
@@ -975,6 +970,14 @@ static int rmi_driver_probe(struct device *dev)
pdata->attn_gpio);
}
}
+ } else
+ data->poll_interval = ktime_set(0,
+ (pdata->poll_interval_ms ? pdata->poll_interval_ms :
+ DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000);
+
+ if (data->f01_container->dev.driver) {
+ /* Driver already bound, so enable ATTN now. */
+ enable_sensor(rmi_dev);
}
return 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
2013-12-24 2:44 [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling Christopher Heiny
@ 2013-12-26 22:37 ` Dmitry Torokhov
2013-12-27 23:43 ` Christopher Heiny
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2013-12-26 22:37 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
Hi Chris,
On Mon, Dec 23, 2013 at 06:44:19PM -0800, Christopher Heiny wrote:
> This patch fixes some bugs in handling of the RMI4 attention line GPIO.
>
> 1) in enable_sensor(), eliminate the complicated check on ATTN and just
> call process_interrupt_requests(). This will have minimal overhead if ATTN
> is not asserted, and clears the state of the RMI4 device in any case.
>
> 2) Correctly free the GPIO in rmi_driver_remove().
>
> 3) in rmi_driver_probe()
> - declare the name of the attention gpio (GPIO_LABEL)
> - use gpio_request_one() to get the gpio and export it.
> - simplify conditional gpio acquisition logic and combine with interrupt
> setup
>
> 4) use gpio_is_valid() instead of comparing to 0.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 43 ++++++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index a30c7d3..9b02358 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> struct rmi_phys_device *rmi_phys;
> int retval = 0;
> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>
> if (data->enabled)
> return 0;
> @@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>
> data->enabled = true;
>
> - if (!pdata->level_triggered &&
> - gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
> - retval = process_interrupt_requests(rmi_dev);
> -
> - return retval;
> + return process_interrupt_requests(rmi_dev);
> }
>
> static void rmi_free_function_list(struct rmi_device *rmi_dev)
> @@ -800,13 +795,20 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume);
> static int rmi_driver_remove(struct device *dev)
> {
> struct rmi_device *rmi_dev = to_rmi_device(dev);
> + const struct rmi_device_platform_data *pdata =
> + to_rmi_platform_data(rmi_dev);
>
> disable_sensor(rmi_dev);
> rmi_free_function_list(rmi_dev);
>
> + if (gpio_is_valid(pdata->attn_gpio))
> + gpio_free(pdata->attn_gpio);
It looks like you let driver registration to continue even if GPIO
request fails. You probably need to introduce a flag indicating whether
you successfully requested gpio or not. Or you can treat failure to
acquire gpio as fatal.
> +
> return 0;
> }
>
> +static const char GPIO_LABEL[] = "attn";
> +
> static int rmi_driver_probe(struct device *dev)
> {
> struct rmi_driver *rmi_driver;
> @@ -937,7 +939,7 @@ static int rmi_driver_probe(struct device *dev)
> mutex_init(&data->suspend_mutex);
> }
>
> - if (pdata->attn_gpio) {
> + if (gpio_is_valid(pdata->attn_gpio)) {
> data->irq = gpio_to_irq(pdata->attn_gpio);
> if (pdata->level_triggered) {
> data->irq_flags = IRQF_ONESHOT |
> @@ -948,24 +950,17 @@ static int rmi_driver_probe(struct device *dev)
> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
> }
> - } else
> - data->poll_interval = ktime_set(0,
> - (pdata->poll_interval_ms ? pdata->poll_interval_ms :
> - DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000);
> -
> - if (data->f01_container->dev.driver) {
> - /* Driver already bound, so enable ATTN now. */
> - enable_sensor(rmi_dev);
> - }
>
> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
Do you really need to export gpio if you do not have RMI4_DEV option?
> - retval = gpio_export(pdata->attn_gpio, false);
> + retval = gpio_request_one(pdata->attn_gpio,
> + GPIOF_EXPORT | GPIOF_DIR_IN,
> + GPIO_LABEL);
> if (retval) {
> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n",
> + pdata->attn_gpio, retval);
> retval = 0;
> } else {
> retval = gpio_export_link(dev,
> - "attn", pdata->attn_gpio);
> + GPIO_LABEL, pdata->attn_gpio);
> if (retval) {
> dev_warn(dev,
> "WARNING: Failed to symlink ATTN gpio!\n");
> @@ -975,6 +970,14 @@ static int rmi_driver_probe(struct device *dev)
> pdata->attn_gpio);
> }
> }
> + } else
> + data->poll_interval = ktime_set(0,
> + (pdata->poll_interval_ms ? pdata->poll_interval_ms :
> + DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000);
> +
> + if (data->f01_container->dev.driver) {
> + /* Driver already bound, so enable ATTN now. */
> + enable_sensor(rmi_dev);
> }
>
> return 0;
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
2013-12-26 22:37 ` Dmitry Torokhov
@ 2013-12-27 23:43 ` Christopher Heiny
0 siblings, 0 replies; 6+ messages in thread
From: Christopher Heiny @ 2013-12-27 23:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
On 12/26/2013 02:37 PM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Mon, Dec 23, 2013 at 06:44:19PM -0800, Christopher Heiny wrote:
>> This patch fixes some bugs in handling of the RMI4 attention line GPIO.
>>
>> 1) in enable_sensor(), eliminate the complicated check on ATTN and just
>> call process_interrupt_requests(). This will have minimal overhead if ATTN
>> is not asserted, and clears the state of the RMI4 device in any case.
>>
>> 2) Correctly free the GPIO in rmi_driver_remove().
>>
>> 3) in rmi_driver_probe()
>> - declare the name of the attention gpio (GPIO_LABEL)
>> - use gpio_request_one() to get the gpio and export it.
>> - simplify conditional gpio acquisition logic and combine with interrupt
>> setup
>>
>> 4) use gpio_is_valid() instead of comparing to 0.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 43 ++++++++++++++++++++++-------------------
>> 1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index a30c7d3..9b02358 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>> struct rmi_phys_device *rmi_phys;
>> int retval = 0;
>> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>>
>> if (data->enabled)
>> return 0;
>> @@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>>
>> data->enabled = true;
>>
>> - if (!pdata->level_triggered &&
>> - gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
>> - retval = process_interrupt_requests(rmi_dev);
>> -
>> - return retval;
>> + return process_interrupt_requests(rmi_dev);
>> }
>>
>> static void rmi_free_function_list(struct rmi_device *rmi_dev)
>> @@ -800,13 +795,20 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume);
>> static int rmi_driver_remove(struct device *dev)
>> {
>> struct rmi_device *rmi_dev = to_rmi_device(dev);
>> + const struct rmi_device_platform_data *pdata =
>> + to_rmi_platform_data(rmi_dev);
>>
>> disable_sensor(rmi_dev);
>> rmi_free_function_list(rmi_dev);
>>
>> + if (gpio_is_valid(pdata->attn_gpio))
>> + gpio_free(pdata->attn_gpio);
>
> It looks like you let driver registration to continue even if GPIO
> request fails. You probably need to introduce a flag indicating whether
> you successfully requested gpio or not. Or you can treat failure to
> acquire gpio as fatal.
In testing, the driver continues to work even if the GPIO is not
acquired, but some diagnostic features may not be available, so we treat
the failure to acquire as a warning. I'll add a flag to indicate
whether it was acquired or not.
>
>> +
>> return 0;
>> }
>>
>> +static const char GPIO_LABEL[] = "attn";
>> +
>> static int rmi_driver_probe(struct device *dev)
>> {
>> struct rmi_driver *rmi_driver;
>> @@ -937,7 +939,7 @@ static int rmi_driver_probe(struct device *dev)
>> mutex_init(&data->suspend_mutex);
>> }
>>
>> - if (pdata->attn_gpio) {
>> + if (gpio_is_valid(pdata->attn_gpio)) {
>> data->irq = gpio_to_irq(pdata->attn_gpio);
>> if (pdata->level_triggered) {
>> data->irq_flags = IRQF_ONESHOT |
>> @@ -948,24 +950,17 @@ static int rmi_driver_probe(struct device *dev)
>> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
>> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>> }
>> - } else
>> - data->poll_interval = ktime_set(0,
>> - (pdata->poll_interval_ms ? pdata->poll_interval_ms :
>> - DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000);
>> -
>> - if (data->f01_container->dev.driver) {
>> - /* Driver already bound, so enable ATTN now. */
>> - enable_sensor(rmi_dev);
>> - }
>>
>> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
>
> Do you really need to export gpio if you do not have RMI4_DEV option?
Not really, but it doesn't hurt to do so. I was trying to simplify the
driver logic, but I guess that went too far. I'll re-add the check.
[snip]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-27 23:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-24 2:44 [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling Christopher Heiny
2013-12-26 22:37 ` Dmitry Torokhov
2013-12-27 23:43 ` Christopher Heiny
-- strict thread matches above, loose matches on Subject: below --
2013-12-17 20:16 Christopher Heiny
2013-12-18 14:39 ` Dmitry Torokhov
2013-12-19 1:23 ` Christopher Heiny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).