* [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
@ 2013-12-28 1:26 Christopher Heiny
2013-12-28 2:36 ` Dmitry Torokhov
0 siblings, 1 reply; 3+ messages in thread
From: Christopher Heiny @ 2013-12-28 1:26 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 (somewhat) 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 | 64 ++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2ae9af9..766954f 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_transport_dev *xport;
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,21 @@ 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);
+ const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
disable_sensor(rmi_dev);
rmi_free_function_list(rmi_dev);
+ if (gpio_is_valid(pdata->attn_gpio) && data->gpio_held)
+ 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 +940,9 @@ static int rmi_driver_probe(struct device *dev)
mutex_init(&data->suspend_mutex);
}
- if (pdata->attn_gpio) {
+ if (gpio_is_valid(pdata->attn_gpio)) {
+ ulong gpio_flags = GPIOF_DIR_IN;
+
data->irq = gpio_to_irq(pdata->attn_gpio);
if (pdata->level_triggered) {
data->irq_flags = IRQF_ONESHOT |
@@ -948,6 +953,32 @@ static int rmi_driver_probe(struct device *dev)
(pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
}
+
+ if (IS_ENABLED(CONFIG_RMI4_DEV))
+ gpio_flags |= GPIOF_EXPORT;
+ retval = gpio_request_one(pdata->attn_gpio, gpio_flags,
+ GPIO_LABEL);
+ if (retval) {
+ dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n",
+ pdata->attn_gpio, retval);
+ retval = 0;
+ } else {
+ dev_info(dev, "Obtained ATTN gpio %d.\n",
+ pdata->attn_gpio);
+ data->gpio_held = true;
+ if (IS_ENABLED(CONFIG_RMI4_DEV)) {
+ retval = gpio_export_link(dev,
+ GPIO_LABEL, pdata->attn_gpio);
+ if (retval) {
+ dev_warn(dev,
+ "WARNING: Failed to symlink ATTN gpio!\n");
+ retval = 0;
+ } else {
+ dev_info(dev, "Exported ATTN gpio %d.",
+ pdata->attn_gpio);
+ }
+ }
+ }
} else
data->poll_interval = ktime_set(0,
(pdata->poll_interval_ms ? pdata->poll_interval_ms :
@@ -958,25 +989,6 @@ static int rmi_driver_probe(struct device *dev)
enable_sensor(rmi_dev);
}
- if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
- retval = gpio_export(pdata->attn_gpio, false);
- if (retval) {
- dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
- retval = 0;
- } else {
- retval = gpio_export_link(dev,
- "attn", pdata->attn_gpio);
- if (retval) {
- dev_warn(dev,
- "WARNING: Failed to symlink ATTN gpio!\n");
- retval = 0;
- } else {
- dev_info(dev, "Exported ATTN GPIO %d.",
- pdata->attn_gpio);
- }
- }
- }
-
return 0;
err_free_data:
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
2013-12-28 1:26 [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling Christopher Heiny
@ 2013-12-28 2:36 ` Dmitry Torokhov
2013-12-31 1:25 ` Christopher Heiny
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2013-12-28 2:36 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
On Fri, Dec 27, 2013 at 05:26:44PM -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 (somewhat) 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: In function ‘rmi_driver_probe’:
drivers/input/rmi4/rmi_driver.c:920:8: error: ‘struct rmi_driver_data’
has no member named ‘gpio_held’
data->gpio_held = true;
You forgot to include header file changes...
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 64 ++++++++++++++++++++++++-----------------
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 2ae9af9..766954f 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_transport_dev *xport;
> 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,21 @@ 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);
> + const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>
> disable_sensor(rmi_dev);
> rmi_free_function_list(rmi_dev);
>
> + if (gpio_is_valid(pdata->attn_gpio) && data->gpio_held)
> + gpio_free(pdata->attn_gpio);
You only need to check data->gpio_held here...
> +
> return 0;
> }
>
> +static const char GPIO_LABEL[] = "attn";
> +
While you are updating paatch could you move it in rmi_driver_probe()
under if (gpio_is_valid()).
By the way, maybe we should have platform supply gpio name or, in its
absence, generate unique gpio name, so that we could have several RMI
devices be present in a box?
That can be a followup patch at a later time.
> static int rmi_driver_probe(struct device *dev)
> {
> struct rmi_driver *rmi_driver;
> @@ -937,7 +940,9 @@ static int rmi_driver_probe(struct device *dev)
> mutex_init(&data->suspend_mutex);
> }
>
> - if (pdata->attn_gpio) {
> + if (gpio_is_valid(pdata->attn_gpio)) {
> + ulong gpio_flags = GPIOF_DIR_IN;
> +
> data->irq = gpio_to_irq(pdata->attn_gpio);
> if (pdata->level_triggered) {
> data->irq_flags = IRQF_ONESHOT |
> @@ -948,6 +953,32 @@ static int rmi_driver_probe(struct device *dev)
> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
> }
> +
> + if (IS_ENABLED(CONFIG_RMI4_DEV))
> + gpio_flags |= GPIOF_EXPORT;
> + retval = gpio_request_one(pdata->attn_gpio, gpio_flags,
> + GPIO_LABEL);
> + if (retval) {
> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n",
> + pdata->attn_gpio, retval);
> + retval = 0;
> + } else {
> + dev_info(dev, "Obtained ATTN gpio %d.\n",
> + pdata->attn_gpio);
> + data->gpio_held = true;
> + if (IS_ENABLED(CONFIG_RMI4_DEV)) {
> + retval = gpio_export_link(dev,
> + GPIO_LABEL, pdata->attn_gpio);
> + if (retval) {
> + dev_warn(dev,
> + "WARNING: Failed to symlink ATTN gpio!\n");
> + retval = 0;
> + } else {
> + dev_info(dev, "Exported ATTN gpio %d.",
> + pdata->attn_gpio);
> + }
> + }
> + }
> } else
} else {
> data->poll_interval = ktime_set(0,
> (pdata->poll_interval_ms ? pdata->poll_interval_ms :
Another thing I was wondering - polling support is really unusable for
device in production (battery gets killed) so maybe it should be removed
altogether?
> @@ -958,25 +989,6 @@ static int rmi_driver_probe(struct device *dev)
> enable_sensor(rmi_dev);
> }
>
> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
> - retval = gpio_export(pdata->attn_gpio, false);
> - if (retval) {
> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
> - retval = 0;
> - } else {
> - retval = gpio_export_link(dev,
> - "attn", pdata->attn_gpio);
> - if (retval) {
> - dev_warn(dev,
> - "WARNING: Failed to symlink ATTN gpio!\n");
> - retval = 0;
> - } else {
> - dev_info(dev, "Exported ATTN GPIO %d.",
> - pdata->attn_gpio);
> - }
> - }
> - }
> -
> return 0;
>
> err_free_data:
Thanks!
--
Dmitry
--
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
2013-12-28 2:36 ` Dmitry Torokhov
@ 2013-12-31 1:25 ` Christopher Heiny
0 siblings, 0 replies; 3+ messages in thread
From: Christopher Heiny @ 2013-12-31 1:25 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/27/2013 06:36 PM, Dmitry Torokhov wrote:
> On Fri, Dec 27, 2013 at 05:26:44PM -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 (somewhat) 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: In function ‘rmi_driver_probe’:
> drivers/input/rmi4/rmi_driver.c:920:8: error: ‘struct rmi_driver_data’
> has no member named ‘gpio_held’
> data->gpio_held = true;
>
> You forgot to include header file changes...
Oh, $%^&*(!
>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 64 ++++++++++++++++++++++++-----------------
>> 1 file changed, 38 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index 2ae9af9..766954f 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_transport_dev *xport;
>> 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,21 @@ 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);
>> + const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>>
>> disable_sensor(rmi_dev);
>> rmi_free_function_list(rmi_dev);
>>
>> + if (gpio_is_valid(pdata->attn_gpio) && data->gpio_held)
>> + gpio_free(pdata->attn_gpio);
>
> You only need to check data->gpio_held here...
Good point.
>
>> +
>> return 0;
>> }
>>
>> +static const char GPIO_LABEL[] = "attn";
>> +
>
> While you are updating paatch could you move it in rmi_driver_probe()
> under if (gpio_is_valid()).
Can do.
>
> By the way, maybe we should have platform supply gpio name or, in its
> absence, generate unique gpio name, so that we could have several RMI
> devices be present in a box?
>
> That can be a followup patch at a later time.
Hmmm. I think it'd be better to name it as attn0, attn1, and so on.
But as you say, we can address that at a later time.
>> static int rmi_driver_probe(struct device *dev)
>> {
>> struct rmi_driver *rmi_driver;
>> @@ -937,7 +940,9 @@ static int rmi_driver_probe(struct device *dev)
>> mutex_init(&data->suspend_mutex);
>> }
>>
>> - if (pdata->attn_gpio) {
>> + if (gpio_is_valid(pdata->attn_gpio)) {
>> + ulong gpio_flags = GPIOF_DIR_IN;
>> +
>> data->irq = gpio_to_irq(pdata->attn_gpio);
>> if (pdata->level_triggered) {
>> data->irq_flags = IRQF_ONESHOT |
>> @@ -948,6 +953,32 @@ static int rmi_driver_probe(struct device *dev)
>> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
>> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>> }
>> +
>> + if (IS_ENABLED(CONFIG_RMI4_DEV))
>> + gpio_flags |= GPIOF_EXPORT;
>> + retval = gpio_request_one(pdata->attn_gpio, gpio_flags,
>> + GPIO_LABEL);
>> + if (retval) {
>> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n",
>> + pdata->attn_gpio, retval);
>> + retval = 0;
>> + } else {
>> + dev_info(dev, "Obtained ATTN gpio %d.\n",
>> + pdata->attn_gpio);
>> + data->gpio_held = true;
>> + if (IS_ENABLED(CONFIG_RMI4_DEV)) {
>> + retval = gpio_export_link(dev,
>> + GPIO_LABEL, pdata->attn_gpio);
>> + if (retval) {
>> + dev_warn(dev,
>> + "WARNING: Failed to symlink ATTN gpio!\n");
>> + retval = 0;
>> + } else {
>> + dev_info(dev, "Exported ATTN gpio %d.",
>> + pdata->attn_gpio);
>> + }
>> + }
>> + }
>> } else
>
> } else {
>
>> data->poll_interval = ktime_set(0,
>> (pdata->poll_interval_ms ? pdata->poll_interval_ms :
>
>
> Another thing I was wondering - polling support is really unusable for
> device in production (battery gets killed) so maybe it should be removed
> altogether?
You're right that polling is not good in shipping kernels, but the
polling capability is extensively used when bringing up new hardware or
initially porting the driver onto existing hardware. I suppose we could
make the polling capability contingent on CONFIG_RMI4_DEBUG.
>
>> @@ -958,25 +989,6 @@ static int rmi_driver_probe(struct device *dev)
>> enable_sensor(rmi_dev);
>> }
>>
>> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
>> - retval = gpio_export(pdata->attn_gpio, false);
>> - if (retval) {
>> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
>> - retval = 0;
>> - } else {
>> - retval = gpio_export_link(dev,
>> - "attn", pdata->attn_gpio);
>> - if (retval) {
>> - dev_warn(dev,
>> - "WARNING: Failed to symlink ATTN gpio!\n");
>> - retval = 0;
>> - } else {
>> - dev_info(dev, "Exported ATTN GPIO %d.",
>> - pdata->attn_gpio);
>> - }
>> - }
>> - }
>> -
>> return 0;
>>
>> err_free_data:
>
> Thanks!
--
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-12-31 1:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-28 1:26 [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling Christopher Heiny
2013-12-28 2:36 ` Dmitry Torokhov
2013-12-31 1:25 ` 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).