[PATCH 0/3] gnss: sirf: fixes and cleanup

[PATCH 0/3] gnss: sirf: fixes and cleanup

[Johan Hovold]

These patches fixes an issue where the sirf wakeup interrupt was
prematurely enabled during probe. As Andreas suggested, we should also
make sure that the receiver is indeed suspended at probe (to avoid
wasting power until first open or suspend).

Johan


Johan Hovold (3):
  gnss: sirf: fix premature wakeup interrupt enable
  gnss: sirf: force hibernate mode on probe
  gnss: sirf: drop redundant double negation

 drivers/gnss/sirf.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH 1/3] gnss: sirf: fix premature wakeup interrupt enable

[Johan Hovold]

Make sure the receiver is powered (and booted) before enabling the
wakeup interrupt to avoid spurious interrupts due to a floating input.

Similarly, disable the interrupt before powering off on probe errors and
on unbind.

Fixes: d2efbbd18b1e ("gnss: add driver for sirfstar-based receivers")
Cc: stable <stable@vger.kernel.org>	# 4.19
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/sirf.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 226f6e6fe01b..8e3f6a776e02 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -310,30 +310,26 @@ static int sirf_probe(struct serdev_device *serdev)
 			ret = -ENODEV;
 			goto err_put_device;
 		}
+
+		ret = regulator_enable(data->vcc);
+		if (ret)
+			goto err_put_device;
+
+		/* Wait for chip to boot into hibernate mode. */
+		msleep(SIRF_BOOT_DELAY);
 	}
 
 	if (data->wakeup) {
 		ret = gpiod_to_irq(data->wakeup);
 		if (ret < 0)
-			goto err_put_device;
-
+			goto err_disable_vcc;
 		data->irq = ret;
 
-		ret = devm_request_threaded_irq(dev, data->irq, NULL,
-				sirf_wakeup_handler,
+		ret = request_threaded_irq(data->irq, NULL, sirf_wakeup_handler,
 				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				"wakeup", data);
 		if (ret)
-			goto err_put_device;
-	}
-
-	if (data->on_off) {
-		ret = regulator_enable(data->vcc);
-		if (ret)
-			goto err_put_device;
-
-		/* Wait for chip to boot into hibernate mode */
-		msleep(SIRF_BOOT_DELAY);
+			goto err_disable_vcc;
 	}
 
 	if (IS_ENABLED(CONFIG_PM)) {
@@ -342,7 +338,7 @@ static int sirf_probe(struct serdev_device *serdev)
 	} else {
 		ret = sirf_runtime_resume(dev);
 		if (ret < 0)
-			goto err_disable_vcc;
+			goto err_free_irq;
 	}
 
 	ret = gnss_register_device(gdev);
@@ -356,6 +352,9 @@ static int sirf_probe(struct serdev_device *serdev)
 		pm_runtime_disable(dev);
 	else
 		sirf_runtime_suspend(dev);
+err_free_irq:
+	if (data->wakeup)
+		free_irq(data->irq, data);
 err_disable_vcc:
 	if (data->on_off)
 		regulator_disable(data->vcc);
@@ -376,6 +375,9 @@ static void sirf_remove(struct serdev_device *serdev)
 	else
 		sirf_runtime_suspend(&serdev->dev);
 
+	if (data->wakeup)
+		free_irq(data->irq, data);
+
 	if (data->on_off)
 		regulator_disable(data->vcc);
 
-- 
2.20.1

[PATCH 2/3] gnss: sirf: force hibernate mode on probe

[Johan Hovold]

Make sure to put the receiver in hibernate mode in case it is already
active during probe in order to avoid wasting power until first open or
suspend.

This can happen, for example, after a reset or non-clean shutdown, and
possibly also due to glitches during power-on.

Reported-by: Andreas Kemnade <andreas@kemnade.info>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/sirf.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 8e3f6a776e02..f9a9d00dec98 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -320,6 +320,11 @@ static int sirf_probe(struct serdev_device *serdev)
 	}
 
 	if (data->wakeup) {
+		ret = gpiod_get_value_cansleep(data->wakeup);
+		if (ret < 0)
+			goto err_disable_vcc;
+		data->active = ret;
+
 		ret = gpiod_to_irq(data->wakeup);
 		if (ret < 0)
 			goto err_disable_vcc;
@@ -332,6 +337,18 @@ static int sirf_probe(struct serdev_device *serdev)
 			goto err_disable_vcc;
 	}
 
+	if (data->on_off) {
+		/* Force hibernate mode if already active. */
+		if (data->active) {
+			ret = sirf_set_active(data, false);
+			if (ret) {
+				dev_err(dev, "failed to set hibernate mode: %d\n",
+						ret);
+				goto err_free_irq;
+			}
+		}
+	}
+
 	if (IS_ENABLED(CONFIG_PM)) {
 		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
 		pm_runtime_enable(dev);
-- 
2.20.1

[PATCH 3/3] gnss: sirf: drop redundant double negation

[Johan Hovold]

The active flag is of type bool so drop the redundant double negation
when storing the gpio state.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/sirf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index f9a9d00dec98..59cde7e923b8 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -125,7 +125,7 @@ static irqreturn_t sirf_wakeup_handler(int irq, void *dev_id)
 	if (ret < 0)
 		goto out;
 
-	data->active = !!ret;
+	data->active = ret;
 	wake_up_interruptible(&data->power_wait);
 out:
 	return IRQ_HANDLED;
-- 
2.20.1

Re: [PATCH 0/3] gnss: sirf: fixes and cleanup

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

Hi Johan,

On Tue, 22 Jan 2019 18:22:52 +0100
Johan Hovold <johan@kernel.org> wrote:

> These patches fixes an issue where the sirf wakeup interrupt was
> prematurely enabled during probe. As Andreas suggested, we should also
> make sure that the receiver is indeed suspended at probe (to avoid
> wasting power until first open or suspend).
> 
I will rebase my stuff on top of it.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]