[PATCH v4 0/5] gnss: sirf: add support for w2sg0004 + lna

[PATCH v4 0/5] 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

Changes in v4:
  - rebased on top of "gnss: sirf: fixes and cleanups"
    so the initial powerdown patch is unneeded
  - cleanups

Andreas Kemnade (5):
  gnss: sirf: write data to gnss only when the gnss device is open
  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                                | 212 ++++++++++++++++++---
 3 files changed, 188 insertions(+), 26 deletions(-)

-- 
2.11.0

[PATCH v4 1/5] 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>
---
No changes in v4
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 59cde7e923b8..49bc021325e9 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 v4 2/5] 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 communication
over the serial lines.
This approach requires a report cycle set to a value less than 2 seconds
to be reliable.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v4:
 - was 3/6 earlier
 - more cleanup
 - separate no wakeup version for sirf_wait_for_power_state
 - cleaned up optimisation for initial power off

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 | 124 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 105 insertions(+), 19 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 49bc021325e9..b42a383e26e0 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -25,6 +25,15 @@
 #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 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.
+ */
+#define SIRF_REPORT_CYCLE	2000
 
 struct sirf_data {
 	struct gnss_device *gdev;
@@ -39,9 +48,42 @@ struct sirf_data {
 	struct mutex gdev_mutex;
 	bool open;
 
+	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--;
+			goto out_unlock;
+		}
+
+		serdev_device_set_baudrate(data->serdev, data->speed);
+		serdev_device_set_flow_control(data->serdev, false);
+	}
+
+out_unlock:
+	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 +94,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 +102,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 +112,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 +126,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 +166,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);
@@ -158,11 +201,40 @@ static irqreturn_t sirf_wakeup_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int sirf_wait_for_power_state_nowakeup(struct sirf_data *data,
+						bool active,
+						unsigned long timeout)
+{
+	int ret;
+
+	/* Wait for state change change (including any shutdown messages). */
+	msleep(timeout);
+	data->active = false;
+	/* 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)
+		return -ETIMEDOUT;
+
+	if (ret == 0 && active)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
 					unsigned long timeout)
 {
 	int ret;
 
+	if (!data->wakeup)
+		return sirf_wait_for_power_state_nowakeup(data, active,
+							  timeout);
+
 	ret = wait_event_interruptible_timeout(data->power_wait,
 			data->active == active, msecs_to_jiffies(timeout));
 	if (ret < 0)
@@ -195,6 +267,12 @@ 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);
 		ret = sirf_wait_for_power_state(data, active, timeout);
@@ -202,12 +280,17 @@ static int sirf_set_active(struct sirf_data *data, bool active)
 			if (ret == -ETIMEDOUT)
 				continue;
 
-			return ret;
 		}
-
 		break;
+
 	} while (retries--);
 
+	if (!data->wakeup)
+		sirf_serdev_close(data);
+
+	if (ret)
+		return ret;
+
 	if (retries < 0)
 		return -ETIMEDOUT;
 
@@ -303,6 +386,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,16 +413,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;
-		}
-
 		ret = regulator_enable(data->vcc);
 		if (ret)
 			goto err_put_device;
@@ -366,6 +440,17 @@ static int sirf_probe(struct serdev_device *serdev)
 	}
 
 	if (data->on_off) {
+		if (!data->wakeup) {
+			data->active = false;
+
+			ret = sirf_serdev_open(data);
+			if (ret)
+				goto err_disable_vcc;
+
+			msleep(SIRF_REPORT_CYCLE);
+			sirf_serdev_close(data);
+		}
+
 		/* Force hibernate mode if already active. */
 		if (data->active) {
 			ret = sirf_set_active(data, false);
@@ -433,6 +518,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 v4 3/5] 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 v4 4/5] 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 v4:
 - cleaned up error checking
 - was 5/6 earlier

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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index b42a383e26e0..bd6bd9a94c55 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -40,6 +40,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;
@@ -300,21 +301,60 @@ 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 ret2;
+	int ret;
+
+	if (data->on_off)
+		ret = sirf_set_active(data, false);
+	else
+		ret = regulator_disable(data->vcc);
+
+	if (ret)
+		return ret;
+
+	ret = regulator_disable(data->lna);
+	if (ret)
+		goto err_reenable;
+
+	return 0;
+
+err_reenable:
+	if (data->on_off)
+		ret2 = sirf_set_active(data, true);
+	else
+		ret2 = regulator_enable(data->vcc);
 
-	if (!data->on_off)
-		return regulator_disable(data->vcc);
+	if (ret2)
+		dev_err(dev,
+			"failed to reenable power on failed suspend: %d\n",
+			ret2);
 
-	return sirf_set_active(data, false);
+	return ret;
 }
 
 static int sirf_runtime_resume(struct device *dev)
 {
 	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret;
 
-	if (!data->on_off)
-		return regulator_enable(data->vcc);
+	ret = regulator_enable(data->lna);
+	if (ret)
+		return ret;
+
+	if (data->on_off)
+		ret = sirf_set_active(data, true);
+	else
+		ret = regulator_enable(data->vcc);
+
+	if (ret)
+		goto err_disable_lna;
 
-	return sirf_set_active(data, true);
+	return 0;
+
+err_disable_lna:
+	regulator_disable(data->lna);
+
+	return ret;
 }
 
 static int __maybe_unused sirf_suspend(struct device *dev)
@@ -402,6 +442,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 v4 5/5] dt-bindings: gnss: add lna-supply property

[Andreas Kemnade]

Add lna-supply property.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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..f547bd4549fe 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 an LNA
 - enable-gpios	: GPIO used to enable the device
 - timepulse-gpios	: Time pulse GPIO
 
-- 
2.11.0

Re: [PATCH v4 2/5] gnss: sirf: add support for configurations without wakeup signal

[Johan Hovold]

On Thu, Jan 24, 2019 at 07:34:36AM +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 communication
> over the serial lines.
> This approach requires a report cycle set to a value less than 2 seconds
> to be reliable.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v4:
>  - was 3/6 earlier
>  - more cleanup
>  - separate no wakeup version for sirf_wait_for_power_state
>  - cleaned up optimisation for initial power off
> 
> 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 | 124 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 105 insertions(+), 19 deletions(-)

>  static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
>  					unsigned long timeout)
>  {
>  	int ret;
>  
> +	if (!data->wakeup)
> +		return sirf_wait_for_power_state_nowakeup(data, active,
> +							  timeout);
> +
>  	ret = wait_event_interruptible_timeout(data->power_wait,
>  			data->active == active, msecs_to_jiffies(timeout));
>  	if (ret < 0)
> @@ -195,6 +267,12 @@ 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);
>  		ret = sirf_wait_for_power_state(data, active, timeout);
> @@ -202,12 +280,17 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  			if (ret == -ETIMEDOUT)
>  				continue;
>  
> -			return ret;
>  		}
> -
>  		break;
> +
>  	} while (retries--);

I noticed there were some odd white-space changes here after you
reverted the modified error handling from v3 which initially looked a
little out of place to me.

I decided to add those changed back in instead after looking at the end
result and noticing that the (retries < 0) check below also becomes
superfluous.

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

Series now applied, good job!

Johan