Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>

---

  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]
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux