[PATCH v3 0/6] gnss: sirf: add support for w2sg0004 + lna

[PATCH v3 0/6] gnss: sirf: add support for w2sg0004 + lna

[Andreas Kemnade]

Here is another chapter of the story to get gta04 gnss power
management into the mainline kernel.
There is a w2sg0004 without wakeup line in there, so power state
can only be determined indirectly by looking at the serial data lines.
Then there as also an lna which needs to be powered for real gps
reception.

Changes in v2:
  - do not change behavior of devices with wakeup line
    - do not keep serdev open if runtime is active and device is not used
  - style cleanup
  - locking of sirf_close() vs. gnss_insert_raw()
  - name reordering

Changes in v3:
  - more style cleanup
  - more locking
  - better regulator error handling
  - timeout logic cleaned up and problems documented
  - initial power off sorted out as a separate patch
  - renamed patch gnss: sirf: power on logic for devices without wakeup signal

Andreas Kemnade (6):
  gnss: sirf: write data to gnss only when the gnss device is open
  gnss: sirf: set power state initially off
  gnss: sirf: add support for configurations without wakeup signal
  dt-bindings: gnss: add w2sg0004 compatible string
  gnss: sirf: add a separate supply for a lna
  dt-bindings: gnss: add lna-supply property

 Documentation/devicetree/bindings/gnss/gnss.txt    |   1 +
 .../devicetree/bindings/gnss/sirfstar.txt          |   1 +
 drivers/gnss/sirf.c                                | 204 +++++++++++++++++----
 3 files changed, 175 insertions(+), 31 deletions(-)

-- 
2.11.0

[PATCH v3 1/6] gnss: sirf: write data to gnss only when the gnss device is open

[Andreas Kemnade]

The api forbids writing data there otherwise. Prepare for the
serdev_open()/close() being a part of sirf_set_active.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v3:
 - add more locking
 - style cleanup 
 - mutex *not* renamed since we need a second one

Changes in v2:
 - add locking

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

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 226f6e6fe01b..b21e14351b82 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -35,6 +35,10 @@ struct sirf_data {
 	struct gpio_desc *wakeup;
 	int irq;
 	bool active;
+
+	struct mutex gdev_mutex;
+	bool open;
+
 	wait_queue_head_t power_wait;
 };
 
@@ -44,9 +48,18 @@ static int sirf_open(struct gnss_device *gdev)
 	struct serdev_device *serdev = data->serdev;
 	int ret;
 
+	mutex_lock(&data->gdev_mutex);
+	data->open = true;
+	mutex_unlock(&data->gdev_mutex);
+
 	ret = serdev_device_open(serdev);
-	if (ret)
+	if (ret) {
+		mutex_lock(&data->gdev_mutex);
+		data->open = false;
+		mutex_unlock(&data->gdev_mutex);
 		return ret;
+	}
+
 
 	serdev_device_set_baudrate(serdev, data->speed);
 	serdev_device_set_flow_control(serdev, false);
@@ -63,6 +76,10 @@ static int sirf_open(struct gnss_device *gdev)
 err_close:
 	serdev_device_close(serdev);
 
+	mutex_lock(&data->gdev_mutex);
+	data->open = false;
+	mutex_unlock(&data->gdev_mutex);
+
 	return ret;
 }
 
@@ -74,6 +91,10 @@ static void sirf_close(struct gnss_device *gdev)
 	serdev_device_close(serdev);
 
 	pm_runtime_put(&serdev->dev);
+
+	mutex_lock(&data->gdev_mutex);
+	data->open = false;
+	mutex_unlock(&data->gdev_mutex);
 }
 
 static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
@@ -105,8 +126,14 @@ static int sirf_receive_buf(struct serdev_device *serdev,
 {
 	struct sirf_data *data = serdev_device_get_drvdata(serdev);
 	struct gnss_device *gdev = data->gdev;
+	int ret = 0;
 
-	return gnss_insert_raw(gdev, buf, count);
+	mutex_lock(&data->gdev_mutex);
+	if (data->open)
+		ret = gnss_insert_raw(gdev, buf, count);
+	mutex_unlock(&data->gdev_mutex);
+
+	return ret;
 }
 
 static const struct serdev_device_ops sirf_serdev_ops = {
@@ -275,6 +302,7 @@ static int sirf_probe(struct serdev_device *serdev)
 	data->serdev = serdev;
 	data->gdev = gdev;
 
+	mutex_init(&data->gdev_mutex);
 	init_waitqueue_head(&data->power_wait);
 
 	serdev_device_set_drvdata(serdev, data);
-- 
2.11.0

[PATCH v3 2/6] gnss: sirf: set power state initially off

[Andreas Kemnade]

On the GTA04 mobile phone, it was observed that the gps was powered
on sometimes intially. Generally a reboot without powering the device
off (direct reset of the processor, reboot from a system where gps
power toggle was done in userspace) or glitches on the gpio pin during
power on could cause this problem.
This has the drawback that probing takes some seconds on
systems without wakeup signal. On systems with wakeup signal
this penalty is much lower.
But if the chip is initially on and that is not fixed, the suspend
current will be multiple times higher, so this sacrifice should
be justified

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
- was part of 2/5 in v2

 drivers/gnss/sirf.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index b21e14351b82..c7706b91f6f0 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -367,6 +367,13 @@ static int sirf_probe(struct serdev_device *serdev)
 	if (IS_ENABLED(CONFIG_PM)) {
 		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
 		pm_runtime_enable(dev);
+		/*
+		 * Device might be enabled at boot, so ensure it is off.
+		 * This was observed in practice on GTA04.
+		 */
+		ret = sirf_set_active(data, false);
+		if (ret < 0)
+			goto err_disable_rpm;
 	} else {
 		ret = sirf_runtime_resume(dev);
 		if (ret < 0)
-- 
2.11.0

[PATCH v3 3/6] gnss: sirf: add support for configurations without wakeup signal

[Andreas Kemnade]

Some Wi2Wi devices do not have a wakeup output, so device state can
only be indirectly detected by looking whether there is communitcation
over the serial lines.
This approach requires a report cycle set to a low value to be reliable.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v3:
 - was 2/5 earlier
 - changed commit headline
 - more style cleanup
 - split out initial power off as 2/6
 - introduced SIRF_REPORT_CYCLE constant
 - added documentation about limitations
 - ignore first data after power state on so no
   shutdown meassages are treated as power on success
 - clearer logic in sirf_wait_for_power_state

Changes in v2:
 - style cleanup
 - do not keep serdev open just because runtime is active,
   only when needed (gnss device is opened or state is changed)
 - clearer timeout semantics


 drivers/gnss/sirf.c | 117 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 93 insertions(+), 24 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index c7706b91f6f0..943a2ec9b708 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -25,6 +25,16 @@
 #define SIRF_ON_OFF_PULSE_TIME		100
 #define SIRF_ACTIVATE_TIMEOUT		200
 #define SIRF_HIBERNATE_TIMEOUT		200
+/*
+ * If no data arrives for this time, we expect the chip to be off.
+ * REVISIT: The report cycle is configurable and can be several minutes long,
+ * so this will only work reliably if the report cycle is set to a reasonable
+ * low value. Also power saving settings (like send data only on movement)
+ * might things work even worse.
+ * Workaround might be to parse shutdown or bootup messages.
+ * This problem mainly makes error checking uncertain.
+ */
+#define SIRF_REPORT_CYCLE	2000
 
 struct sirf_data {
 	struct gnss_device *gdev;
@@ -39,9 +49,45 @@ struct sirf_data {
 	struct mutex gdev_mutex;
 	bool open;
 
+	/*
+	 * Using the same mutex inside a serdev callback
+	 * and around a serdev call leads to lockdep problems.
+	 */
+	struct mutex serdev_mutex;
+	int serdev_count;
+
 	wait_queue_head_t power_wait;
 };
 
+static int sirf_serdev_open(struct sirf_data *data)
+{
+	int ret = 0;
+
+	mutex_lock(&data->serdev_mutex);
+	if (++data->serdev_count == 1) {
+		ret = serdev_device_open(data->serdev);
+		if (ret) {
+			data->serdev_count--;
+			mutex_unlock(&data->serdev_mutex);
+			return ret;
+		}
+
+		serdev_device_set_baudrate(data->serdev, data->speed);
+		serdev_device_set_flow_control(data->serdev, false);
+	}
+	mutex_unlock(&data->serdev_mutex);
+
+	return ret;
+}
+
+static void sirf_serdev_close(struct sirf_data *data)
+{
+	mutex_lock(&data->serdev_mutex);
+	if (--data->serdev_count == 0)
+		serdev_device_close(data->serdev);
+	mutex_unlock(&data->serdev_mutex);
+}
+
 static int sirf_open(struct gnss_device *gdev)
 {
 	struct sirf_data *data = gnss_get_drvdata(gdev);
@@ -52,7 +98,7 @@ static int sirf_open(struct gnss_device *gdev)
 	data->open = true;
 	mutex_unlock(&data->gdev_mutex);
 
-	ret = serdev_device_open(serdev);
+	ret = sirf_serdev_open(data);
 	if (ret) {
 		mutex_lock(&data->gdev_mutex);
 		data->open = false;
@@ -60,10 +106,6 @@ static int sirf_open(struct gnss_device *gdev)
 		return ret;
 	}
 
-
-	serdev_device_set_baudrate(serdev, data->speed);
-	serdev_device_set_flow_control(serdev, false);
-
 	ret = pm_runtime_get_sync(&serdev->dev);
 	if (ret < 0) {
 		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
@@ -74,7 +116,7 @@ static int sirf_open(struct gnss_device *gdev)
 	return 0;
 
 err_close:
-	serdev_device_close(serdev);
+	sirf_serdev_close(data);
 
 	mutex_lock(&data->gdev_mutex);
 	data->open = false;
@@ -88,7 +130,7 @@ static void sirf_close(struct gnss_device *gdev)
 	struct sirf_data *data = gnss_get_drvdata(gdev);
 	struct serdev_device *serdev = data->serdev;
 
-	serdev_device_close(serdev);
+	sirf_serdev_close(data);
 
 	pm_runtime_put(&serdev->dev);
 
@@ -128,6 +170,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
 	struct gnss_device *gdev = data->gdev;
 	int ret = 0;
 
+	if (!data->wakeup && !data->active) {
+		data->active = true;
+		wake_up_interruptible(&data->power_wait);
+	}
+
 	mutex_lock(&data->gdev_mutex);
 	if (data->open)
 		ret = gnss_insert_raw(gdev, buf, count);
@@ -163,6 +210,24 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
 {
 	int ret;
 
+	if (!data->wakeup) {
+		/* Wait for boot or shutdown messages */
+		msleep(timeout);
+		data->active = false;
+		/* Now check if it is really off. */
+		ret = wait_event_interruptible_timeout(data->power_wait,
+					data->active,
+					msecs_to_jiffies(SIRF_REPORT_CYCLE));
+		if (ret < 0)
+			return ret;
+
+		if ((ret > 0 && !active) ||
+		    (ret == 0 && active))
+			return -ETIMEDOUT;
+
+		return 0;
+	}
+
 	ret = wait_event_interruptible_timeout(data->power_wait,
 			data->active == active, msecs_to_jiffies(timeout));
 	if (ret < 0)
@@ -195,18 +260,29 @@ static int sirf_set_active(struct sirf_data *data, bool active)
 	else
 		timeout = SIRF_HIBERNATE_TIMEOUT;
 
+	if (!data->wakeup) {
+		ret = sirf_serdev_open(data);
+		if (ret)
+			return ret;
+	}
+
 	do {
-		sirf_pulse_on_off(data);
+		/*
+		 * Try to avoid unneeded, time-consuming state toggles
+		 * in the configurations without wakeup signal. This counts
+		 * especially for the initial off state check.
+		 */
+		if (data->wakeup || data->active || active)
+			sirf_pulse_on_off(data);
+
 		ret = sirf_wait_for_power_state(data, active, timeout);
-		if (ret < 0) {
-			if (ret == -ETIMEDOUT)
-				continue;
+	} while (ret == -ETIMEDOUT && retries--);
 
-			return ret;
-		}
+	if (!data->wakeup)
+		sirf_serdev_close(data);
 
-		break;
-	} while (retries--);
+	if (ret)
+		return ret;
 
 	if (retries < 0)
 		return -ETIMEDOUT;
@@ -303,6 +379,7 @@ static int sirf_probe(struct serdev_device *serdev)
 	data->gdev = gdev;
 
 	mutex_init(&data->gdev_mutex);
+	mutex_init(&data->serdev_mutex);
 	init_waitqueue_head(&data->power_wait);
 
 	serdev_device_set_drvdata(serdev, data);
@@ -329,15 +406,6 @@ static int sirf_probe(struct serdev_device *serdev)
 		if (IS_ERR(data->wakeup))
 			goto err_put_device;
 
-		/*
-		 * Configurations where WAKEUP has been left not connected,
-		 * are currently not supported.
-		 */
-		if (!data->wakeup) {
-			dev_err(dev, "no wakeup gpio specified\n");
-			ret = -ENODEV;
-			goto err_put_device;
-		}
 	}
 
 	if (data->wakeup) {
@@ -421,6 +489,7 @@ static void sirf_remove(struct serdev_device *serdev)
 static const struct of_device_id sirf_of_match[] = {
 	{ .compatible = "fastrax,uc430" },
 	{ .compatible = "linx,r4" },
+	{ .compatible = "wi2wi,w2sg0004" },
 	{ .compatible = "wi2wi,w2sg0008i" },
 	{ .compatible = "wi2wi,w2sg0084i" },
 	{},
-- 
2.11.0

[PATCH v3 4/6] dt-bindings: gnss: add w2sg0004 compatible string

[Andreas Kemnade]

Add w2sg0004 compatible string since devices without wakeup
pins are now supported.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Johan Hovold <johan@kernel.org>
---
 Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gnss/sirfstar.txt b/Documentation/devicetree/bindings/gnss/sirfstar.txt
index 648d183cdb77..f4252b6b660b 100644
--- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -12,6 +12,7 @@ Required properties:
 
 			"fastrax,uc430"
 			"linx,r4"
+			"wi2wi,w2sg0004"
 			"wi2wi,w2sg0008i"
 			"wi2wi,w2sg0084i"
 
-- 
2.11.0

[PATCH v3 5/6] gnss: sirf: add a separate supply for a lna

[Andreas Kemnade]

Devices might have a separate lna between antenna input of the gps
chip and the antenna which might have a separate supply.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v3:
 - improved error checking
 - style cleanup

Changes in v2:
 - handle lna also if there is no on-off gpio
 - rebase on changed 2/5

 drivers/gnss/sirf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 943a2ec9b708..7de98edf198a 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -41,6 +41,7 @@ struct sirf_data {
 	struct serdev_device *serdev;
 	speed_t	speed;
 	struct regulator *vcc;
+	struct regulator *lna;
 	struct gpio_desc *on_off;
 	struct gpio_desc *wakeup;
 	int irq;
@@ -293,21 +294,52 @@ static int sirf_set_active(struct sirf_data *data, bool active)
 static int sirf_runtime_suspend(struct device *dev)
 {
 	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret;
 
-	if (!data->on_off)
-		return regulator_disable(data->vcc);
+	if (data->on_off)
+		ret = sirf_set_active(data, false);
+	else
+		ret = regulator_disable(data->vcc);
+
+	if (ret)
+		return ret;
 
-	return sirf_set_active(data, false);
+	ret = regulator_disable(data->lna);
+	if (ret) {
+		int reenable_ret;
+
+		if (data->on_off)
+			reenable_ret = sirf_set_active(data, true);
+		else
+			reenable_ret = regulator_enable(data->vcc);
+
+		if (reenable_ret)
+			dev_err(dev,
+				"failed to reenable power on failed suspend: %d\n",
+				reenable_ret);
+	}
+
+	return ret;
 }
 
 static int sirf_runtime_resume(struct device *dev)
 {
 	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regulator_enable(data->lna);
+	if (ret)
+		return ret;
 
-	if (!data->on_off)
-		return regulator_enable(data->vcc);
+	if (data->on_off)
+		ret = sirf_set_active(data, true);
+	else
+		ret = regulator_enable(data->vcc);
 
-	return sirf_set_active(data, true);
+	if (ret)
+		regulator_disable(data->lna);
+
+	return ret;
 }
 
 static int __maybe_unused sirf_suspend(struct device *dev)
@@ -395,6 +427,12 @@ static int sirf_probe(struct serdev_device *serdev)
 		goto err_put_device;
 	}
 
+	data->lna = devm_regulator_get(dev, "lna");
+	if (IS_ERR(data->lna)) {
+		ret = PTR_ERR(data->lna);
+		goto err_put_device;
+	}
+
 	data->on_off = devm_gpiod_get_optional(dev, "sirf,onoff",
 			GPIOD_OUT_LOW);
 	if (IS_ERR(data->on_off))
-- 
2.11.0

[PATCH v3 6/6] dt-bindings: gnss: add lna-supply property

[Andreas Kemnade]

Add lna-supply property.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v3:
- moved to gnss.txt

 Documentation/devicetree/bindings/gnss/gnss.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gnss/gnss.txt b/Documentation/devicetree/bindings/gnss/gnss.txt
index f1e4a2ff47c5..e31bc225957d 100644
--- a/Documentation/devicetree/bindings/gnss/gnss.txt
+++ b/Documentation/devicetree/bindings/gnss/gnss.txt
@@ -17,6 +17,7 @@ Required properties:
 		  represents
 
 Optional properties:
+- lna-supply	: Separate supply for a LNA
 - enable-gpios	: GPIO used to enable the device
 - timepulse-gpios	: Time pulse GPIO
 
-- 
2.11.0

Re: [PATCH v3 6/6] dt-bindings: gnss: add lna-supply property

[Rob Herring]

On Wed, 16 Jan 2019 22:18:12 +0100, Andreas Kemnade wrote:
> Add lna-supply property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v3:
> - moved to gnss.txt
> 
>  Documentation/devicetree/bindings/gnss/gnss.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/6] gnss: sirf: set power state initially off

[Johan Hovold]

On Wed, Jan 16, 2019 at 10:18:08PM +0100, Andreas Kemnade wrote:
> On the GTA04 mobile phone, it was observed that the gps was powered
> on sometimes intially. Generally a reboot without powering the device
> off (direct reset of the processor, reboot from a system where gps
> power toggle was done in userspace) or glitches on the gpio pin during
> power on could cause this problem.
> This has the drawback that probing takes some seconds on
> systems without wakeup signal. On systems with wakeup signal
> this penalty is much lower.
> But if the chip is initially on and that is not fixed, the suspend
> current will be multiple times higher, so this sacrifice should
> be justified
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> - was part of 2/5 in v2
> 
>  drivers/gnss/sirf.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index b21e14351b82..c7706b91f6f0 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -367,6 +367,13 @@ static int sirf_probe(struct serdev_device *serdev)
>  	if (IS_ENABLED(CONFIG_PM)) {
>  		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
>  		pm_runtime_enable(dev);
> +		/*
> +		 * Device might be enabled at boot, so ensure it is off.
> +		 * This was observed in practice on GTA04.
> +		 */
> +		ret = sirf_set_active(data, false);
> +		if (ret < 0)
> +			goto err_disable_rpm;

I want to handle this case a bit differently. First, we shouldn't
penalise the common case where the receiver is already off by power
cycling when not needed. Second, the above could race with runtime pm.
Third, we mustn't call sirf_set_active() when we have no on-off gpio.

I've prepare a small series which implements this force hibernate mode
at probe if already active. This should be easy to extend with a
function retrieving the current state at boot also for the no-wakeup
case (e.g. keep the port open for one report cycle).

This also allows for a cleaner implementation of sirf_set_active() for
the no-wakeup case (I noticed you added some optimisation there after I
implemented force-hibernate-at-probe).

Johan

Re: [PATCH v3 3/6] gnss: sirf: add support for configurations without wakeup signal

[Johan Hovold]

On Wed, Jan 16, 2019 at 10:18:09PM +0100, Andreas Kemnade wrote:
> Some Wi2Wi devices do not have a wakeup output, so device state can
> only be indirectly detected by looking whether there is communitcation

communication

> over the serial lines.
> This approach requires a report cycle set to a low value to be reliable.

How low? Better to spell out your current assumptions so people can
configure accordingly.

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v3:
>  - was 2/5 earlier
>  - changed commit headline
>  - more style cleanup
>  - split out initial power off as 2/6
>  - introduced SIRF_REPORT_CYCLE constant
>  - added documentation about limitations
>  - ignore first data after power state on so no
>    shutdown meassages are treated as power on success
>  - clearer logic in sirf_wait_for_power_state
> 
> Changes in v2:
>  - style cleanup
>  - do not keep serdev open just because runtime is active,
>    only when needed (gnss device is opened or state is changed)
>  - clearer timeout semantics
> 
> 
>  drivers/gnss/sirf.c | 117 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 93 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index c7706b91f6f0..943a2ec9b708 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -25,6 +25,16 @@
>  #define SIRF_ON_OFF_PULSE_TIME		100
>  #define SIRF_ACTIVATE_TIMEOUT		200
>  #define SIRF_HIBERNATE_TIMEOUT		200
> +/*
> + * If no data arrives for this time, we expect the chip to be off.

"we assume that the chip is off"?

> + * REVISIT: The report cycle is configurable and can be several minutes long,
> + * so this will only work reliably if the report cycle is set to a reasonable
> + * low value. Also power saving settings (like send data only on movement)
> + * might things work even worse.
> + * Workaround might be to parse shutdown or bootup messages.
> + * This problem mainly makes error checking uncertain.

I'd drop the last sentence. You could also fail to detect the state and
waste power indefinitely, right?

> + */
> +#define SIRF_REPORT_CYCLE	2000
>  
>  struct sirf_data {
>  	struct gnss_device *gdev;
> @@ -39,9 +49,45 @@ struct sirf_data {
>  	struct mutex gdev_mutex;
>  	bool open;
>  
> +	/*
> +	 * Using the same mutex inside a serdev callback
> +	 * and around a serdev call leads to lockdep problems.

Lockdep is not a problem; a possible deadlock is. Just drop this comment.

> +	 */
> +	struct mutex serdev_mutex;
> +	int serdev_count;
> +
>  	wait_queue_head_t power_wait;
>  };
>  
> +static int sirf_serdev_open(struct sirf_data *data)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&data->serdev_mutex);
> +	if (++data->serdev_count == 1) {
> +		ret = serdev_device_open(data->serdev);
> +		if (ret) {
> +			data->serdev_count--;
> +			mutex_unlock(&data->serdev_mutex);

Use a common "out_unlock:" path below.

> +			return ret;
> +		}
> +
> +		serdev_device_set_baudrate(data->serdev, data->speed);
> +		serdev_device_set_flow_control(data->serdev, false);
> +	}
> +	mutex_unlock(&data->serdev_mutex);
> +
> +	return ret;
> +}
 
> @@ -128,6 +170,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
>  	struct gnss_device *gdev = data->gdev;
>  	int ret = 0;
>  
> +	if (!data->wakeup && !data->active) {
> +		data->active = true;
> +		wake_up_interruptible(&data->power_wait);
> +	}
> +
>  	mutex_lock(&data->gdev_mutex);
>  	if (data->open)
>  		ret = gnss_insert_raw(gdev, buf, count);
> @@ -163,6 +210,24 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
>  {
>  	int ret;
>  
> +	if (!data->wakeup) {

You currently don't share any code with the wakeup-case; better to keep
this in a separate helper if so.

> +		/* Wait for boot or shutdown messages */

	/* Wait for state change (including any shutdown messages). */

> +		msleep(timeout);
> +		data->active = false;
> +		/* Now check if it is really off. */

You now also use this code to check for activate state:

	/* Wait for data reception or timeout. */

> +		ret = wait_event_interruptible_timeout(data->power_wait,
> +					data->active,
> +					msecs_to_jiffies(SIRF_REPORT_CYCLE));
> +		if (ret < 0)
> +			return ret;
> +
> +		if ((ret > 0 && !active) ||
> +		    (ret == 0 && active))

Split these cases in two add add comments (failed suspend and failed
resume) for readability.

> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
> +
>  	ret = wait_event_interruptible_timeout(data->power_wait,
>  			data->active == active, msecs_to_jiffies(timeout));
>  	if (ret < 0)
> @@ -195,18 +260,29 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  	else
>  		timeout = SIRF_HIBERNATE_TIMEOUT;
>  
> +	if (!data->wakeup) {
> +		ret = sirf_serdev_open(data);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	do {
> -		sirf_pulse_on_off(data);
> +		/*
> +		 * Try to avoid unneeded, time-consuming state toggles
> +		 * in the configurations without wakeup signal. This counts
> +		 * especially for the initial off state check.
> +		 */
> +		if (data->wakeup || data->active || active)
> +			sirf_pulse_on_off(data);

Special casing like this only hurts readability. This is better handled
as a I did for the wakeup case in the series I just posted, that is, by
making sure the receiver is hibernated in probe. You just need to add a
helper to determine the state for no-wakeup.

> +
>  		ret = sirf_wait_for_power_state(data, active, timeout);
> -		if (ret < 0) {
> -			if (ret == -ETIMEDOUT)
> -				continue;
> +	} while (ret == -ETIMEDOUT && retries--);

Why change this?

>  
> -			return ret;

A break should do right?

> -		}
> +	if (!data->wakeup)
> +		sirf_serdev_close(data);
>  
> -		break;
> -	} while (retries--);
> +	if (ret)
> +		return ret;
>  
>  	if (retries < 0)
>  		return -ETIMEDOUT;

Johan

Re: [PATCH v3 5/6] gnss: sirf: add a separate supply for a lna

[Johan Hovold]

On Wed, Jan 16, 2019 at 10:18:11PM +0100, Andreas Kemnade wrote:
> Devices might have a separate lna between antenna input of the gps
> chip and the antenna which might have a separate supply.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v3:
>  - improved error checking
>  - style cleanup
> 
> Changes in v2:
>  - handle lna also if there is no on-off gpio
>  - rebase on changed 2/5
> 
>  drivers/gnss/sirf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index 943a2ec9b708..7de98edf198a 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -41,6 +41,7 @@ struct sirf_data {
>  	struct serdev_device *serdev;
>  	speed_t	speed;
>  	struct regulator *vcc;
> +	struct regulator *lna;
>  	struct gpio_desc *on_off;
>  	struct gpio_desc *wakeup;
>  	int irq;
> @@ -293,21 +294,52 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  static int sirf_runtime_suspend(struct device *dev)
>  {
>  	struct sirf_data *data = dev_get_drvdata(dev);
> +	int ret;
>  
> -	if (!data->on_off)
> -		return regulator_disable(data->vcc);
> +	if (data->on_off)
> +		ret = sirf_set_active(data, false);
> +	else
> +		ret = regulator_disable(data->vcc);
> +
> +	if (ret)
> +		return ret;
>  
> -	return sirf_set_active(data, false);
> +	ret = regulator_disable(data->lna);
> +	if (ret) {

Use an error label for this to avoid the indentation.

> +		int reenable_ret;

Rename ret2 and move to start of function.

> +
> +		if (data->on_off)
> +			reenable_ret = sirf_set_active(data, true);
> +		else
> +			reenable_ret = regulator_enable(data->vcc);
> +
> +		if (reenable_ret)
> +			dev_err(dev,
> +				"failed to reenable power on failed suspend: %d\n",
> +				reenable_ret);
> +	}
> +
> +	return ret;
>  }
>  
>  static int sirf_runtime_resume(struct device *dev)
>  {
>  	struct sirf_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_enable(data->lna);
> +	if (ret)
> +		return ret;
>  
> -	if (!data->on_off)
> -		return regulator_enable(data->vcc);
> +	if (data->on_off)
> +		ret = sirf_set_active(data, true);
> +	else
> +		ret = regulator_enable(data->vcc);
>  
> -	return sirf_set_active(data, true);
> +	if (ret)

Use an error label here too for consistency (and if if ever add further
resources to be manipulated here).

> +		regulator_disable(data->lna);
> +
> +	return ret;
>  }
>  
>  static int __maybe_unused sirf_suspend(struct device *dev)
> @@ -395,6 +427,12 @@ static int sirf_probe(struct serdev_device *serdev)
>  		goto err_put_device;
>  	}
>  
> +	data->lna = devm_regulator_get(dev, "lna");
> +	if (IS_ERR(data->lna)) {
> +		ret = PTR_ERR(data->lna);
> +		goto err_put_device;
> +	}
> +
>  	data->on_off = devm_gpiod_get_optional(dev, "sirf,onoff",
>  			GPIOD_OUT_LOW);
>  	if (IS_ERR(data->on_off))

Johan

Re: [PATCH v3 6/6] dt-bindings: gnss: add lna-supply property

[Johan Hovold]

On Wed, Jan 16, 2019 at 10:18:12PM +0100, Andreas Kemnade wrote:
> Add lna-supply property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v3:
> - moved to gnss.txt
> 
>  Documentation/devicetree/bindings/gnss/gnss.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/gnss/gnss.txt b/Documentation/devicetree/bindings/gnss/gnss.txt
> index f1e4a2ff47c5..e31bc225957d 100644
> --- a/Documentation/devicetree/bindings/gnss/gnss.txt
> +++ b/Documentation/devicetree/bindings/gnss/gnss.txt
> @@ -17,6 +17,7 @@ Required properties:
>  		  represents
>  
>  Optional properties:
> +- lna-supply	: Separate supply for a LNA

s/a/an/

>  - enable-gpios	: GPIO used to enable the device
>  - timepulse-gpios	: Time pulse GPIO

Johan