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

[PATCH 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. That part needs probably more discussion, since it might
be an idea to have it more generalized since it has nothing todo
with the chip itself.
I marked the corresponding patches as RFC. The support for
the w2sg0004 without wakeup can imho go in without the lna first
because users of that chip without an additional lna power supply
can already benefit from it if we should do more discussion first.
I just kept them together so that the full picture can be seen.

Andreas Kemnade (5):
  gnss: sirf: write data to gnss only when the gnss device is open
  gnss: sirf: power on logic for devices 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

 .../devicetree/bindings/gnss/sirfstar.txt          |   2 +
 drivers/gnss/sirf.c                                | 126 +++++++++++++++------
 2 files changed, 96 insertions(+), 32 deletions(-)

-- 
2.11.0

[PATCH 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 runtime pm.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/gnss/sirf.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 79cb98950013..b5efbb062316 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -34,6 +34,7 @@ struct sirf_data {
 	struct gpio_desc *wakeup;
 	int irq;
 	bool active;
+	bool opened;
 	wait_queue_head_t power_wait;
 };
 
@@ -43,6 +44,7 @@ static int sirf_open(struct gnss_device *gdev)
 	struct serdev_device *serdev = data->serdev;
 	int ret;
 
+	data->opened = true;
 	ret = serdev_device_open(serdev);
 	if (ret)
 		return ret;
@@ -54,6 +56,7 @@ static int sirf_open(struct gnss_device *gdev)
 	if (ret < 0) {
 		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
 		pm_runtime_put_noidle(&serdev->dev);
+		data->opened = false;
 		goto err_close;
 	}
 
@@ -73,6 +76,7 @@ static void sirf_close(struct gnss_device *gdev)
 	serdev_device_close(serdev);
 
 	pm_runtime_put(&serdev->dev);
+	data->opened = false;
 }
 
 static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
@@ -105,7 +109,17 @@ static int sirf_receive_buf(struct serdev_device *serdev,
 	struct sirf_data *data = serdev_device_get_drvdata(serdev);
 	struct gnss_device *gdev = data->gdev;
 
-	return gnss_insert_raw(gdev, buf, count);
+	/*
+	 * we might come here everytime when runtime is resumed
+	 * and data is received. Two cases are possible
+	 * 1. device is opened during initialisation
+	 * 2. kernel is compiled without runtime pm
+	 *    and device is opened all the time
+	 */
+	if (data->opened)
+		return gnss_insert_raw(gdev, buf, count);
+
+	return 0;
 }
 
 static const struct serdev_device_ops sirf_serdev_ops = {
-- 
2.11.0

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

[Johan Hovold]

On Sun, Nov 18, 2018 at 10:57:57PM +0100, Andreas Kemnade wrote:
> The api forbids writing data there otherwise. Prepare for the
> serdev_open()/close() being a part of runtime pm.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/gnss/sirf.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
 
> @@ -73,6 +76,7 @@ static void sirf_close(struct gnss_device *gdev)
>  	serdev_device_close(serdev);
>  
>  	pm_runtime_put(&serdev->dev);
> +	data->opened = false;
>  }
>  
>  static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
> @@ -105,7 +109,17 @@ static int sirf_receive_buf(struct serdev_device *serdev,
>  	struct sirf_data *data = serdev_device_get_drvdata(serdev);
>  	struct gnss_device *gdev = data->gdev;
>  
> -	return gnss_insert_raw(gdev, buf, count);
> +	/*
> +	 * we might come here everytime when runtime is resumed
> +	 * and data is received. Two cases are possible
> +	 * 1. device is opened during initialisation
> +	 * 2. kernel is compiled without runtime pm
> +	 *    and device is opened all the time
> +	 */
> +	if (data->opened)
> +		return gnss_insert_raw(gdev, buf, count);

This can race with sirf_close() when you move serdev handling out of
sirf_open()/close(). Not sure how best to handle that yet.

Johan

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

[Andreas Kemnade]

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

On Wed, 5 Dec 2018 15:47:39 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Sun, Nov 18, 2018 at 10:57:57PM +0100, Andreas Kemnade wrote:
> > The api forbids writing data there otherwise. Prepare for the
> > serdev_open()/close() being a part of runtime pm.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/gnss/sirf.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)  
>  
> > @@ -73,6 +76,7 @@ static void sirf_close(struct gnss_device *gdev)
> >  	serdev_device_close(serdev);
> >  
> >  	pm_runtime_put(&serdev->dev);
> > +	data->opened = false;
> >  }
> >  
> >  static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
> > @@ -105,7 +109,17 @@ static int sirf_receive_buf(struct serdev_device *serdev,
> >  	struct sirf_data *data = serdev_device_get_drvdata(serdev);
> >  	struct gnss_device *gdev = data->gdev;
> >  
> > -	return gnss_insert_raw(gdev, buf, count);
> > +	/*
> > +	 * we might come here everytime when runtime is resumed
> > +	 * and data is received. Two cases are possible
> > +	 * 1. device is opened during initialisation
> > +	 * 2. kernel is compiled without runtime pm
> > +	 *    and device is opened all the time
> > +	 */
> > +	if (data->opened)
> > +		return gnss_insert_raw(gdev, buf, count);  
> 
> This can race with sirf_close() when you move serdev handling out of
> sirf_open()/close(). Not sure how best to handle that yet.
> 
Ok, first lets check whether we have a common idea of the problem before
coming to a solution. So race condition here can happen if the serdev
is still opened after the pm_runtime_put in sirf_close() which might
happen if runtime is not suspended after that. I missed that case.
Then if (data->opened) 
is checked, data->opened set to false
then gnss_insert_raw() is executed.
There is data inserted into the fifo which will be read by the next one
opening the gnss device. And there might be trouble at deregistering
time.

And now if we simply add locks:
For sirf_receive_buf(), it somehow feels dangerous if we call
gnss_insert_raw() with a lock held but I have not analyzed it
thoroughly. For sirf_close(), we could simply
put a lock around data->opened = false;
Am I missing something?

I will check if anything changes when I move the serdev_open()/close()
calls a bit. In the end me end to have the serdev open whenever the
userspace wants data or we want to change the state of the gnss chip
without wakeup.
Maybe add a special driver state for that situation where the power
state of the gps chip is changed without having it opened. If we check
that at the beginning of that action, we might have luck.

Regards,
Andreas

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

[PATCH 2/5] gnss: sirf: power on logic for devices 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.
Additionally it checks for the initial state of the device during
probing to ensure it is off.
Timeout values need to be increased, because the reaction on serial line
is slower and are in line  with previous patches by
Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/gnss/sirf.c | 97 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 65 insertions(+), 32 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index b5efbb062316..6a0e5c0a2d62 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -22,8 +22,8 @@
 
 #define SIRF_BOOT_DELAY			500
 #define SIRF_ON_OFF_PULSE_TIME		100
-#define SIRF_ACTIVATE_TIMEOUT		200
-#define SIRF_HIBERNATE_TIMEOUT		200
+#define SIRF_ACTIVATE_TIMEOUT		1000
+#define SIRF_HIBERNATE_TIMEOUT		1000
 
 struct sirf_data {
 	struct gnss_device *gdev;
@@ -45,26 +45,14 @@ static int sirf_open(struct gnss_device *gdev)
 	int ret;
 
 	data->opened = true;
-	ret = serdev_device_open(serdev);
-	if (ret)
-		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);
 		pm_runtime_put_noidle(&serdev->dev);
 		data->opened = false;
-		goto err_close;
 	}
 
-	return 0;
-
-err_close:
-	serdev_device_close(serdev);
-
 	return ret;
 }
 
@@ -73,8 +61,6 @@ 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);
-
 	pm_runtime_put(&serdev->dev);
 	data->opened = false;
 }
@@ -109,6 +95,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
 	struct sirf_data *data = serdev_device_get_drvdata(serdev);
 	struct gnss_device *gdev = data->gdev;
 
+	if ((!data->wakeup) && (!data->active)) {
+		data->active = 1;
+		wake_up_interruptible(&data->power_wait);
+	}
+
 	/*
 	 * we might come here everytime when runtime is resumed
 	 * and data is received. Two cases are possible
@@ -149,6 +140,25 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
 {
 	int ret;
 
+	/* no wakeup pin, success condition is that
+	 * no byte comes in in the period
+	 */
+	if ((!data->wakeup) && (!active) && (data->active)) {
+		/* some bytes might come, so sleep a bit first */
+		msleep(timeout);
+		data->active = false;
+		ret = wait_event_interruptible_timeout(data->power_wait,
+			data->active == true, msecs_to_jiffies(timeout));
+
+		if (ret < 0)
+			return ret;
+
+		/* we are still getting woken up -> timeout */
+		if (ret > 0)
+			return -ETIMEDOUT;
+		return 0;
+	}
+
 	ret = wait_event_interruptible_timeout(data->power_wait,
 			data->active == active, msecs_to_jiffies(timeout));
 	if (ret < 0)
@@ -203,21 +213,48 @@ 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);
+	ret = sirf_set_active(data, false);
 
-	return sirf_set_active(data, false);
+	if (ret)
+		dev_err(dev, "failed to deactivate");
+
+	/* we should close it anyways, so the following receptions
+	 * will not run into the empty
+	 */
+	serdev_device_close(data->serdev);
+	return 0;
 }
 
 static int sirf_runtime_resume(struct device *dev)
 {
+	int ret;
 	struct sirf_data *data = dev_get_drvdata(dev);
+	ret = serdev_device_open(data->serdev);
+	if (ret)
+		return ret;
 
-	if (!data->on_off)
-		return regulator_enable(data->vcc);
+	serdev_device_set_baudrate(data->serdev, data->speed);
+	serdev_device_set_flow_control(data->serdev, false);
+
+	if (!data->on_off) {
+		ret = regulator_enable(data->vcc);
+		if (ret)
+			goto err_close_serdev;
+	}
+	ret = sirf_set_active(data, true);
+
+	if (!ret)
+		return 0;
 
-	return sirf_set_active(data, true);
+	if (!data->on_off)
+		regulator_disable(data->vcc);
+err_close_serdev:
+	serdev_device_close(data->serdev);
+	return ret;
 }
 
 static int __maybe_unused sirf_suspend(struct device *dev)
@@ -311,18 +348,6 @@ static int sirf_probe(struct serdev_device *serdev)
 	if (data->on_off) {
 		data->wakeup = devm_gpiod_get_optional(dev, "sirf,wakeup",
 				GPIOD_IN);
-		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) {
@@ -352,6 +377,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 lets first enable it,
+		 * then disable it to bring it into a clear state
+		 */
+		ret = pm_runtime_get_sync(dev);
+		if (ret)
+			goto err_disable_rpm;
+		pm_runtime_put(dev);
 	} else {
 		ret = sirf_runtime_resume(dev);
 		if (ret < 0)
@@ -401,6 +433,7 @@ static const struct of_device_id sirf_of_match[] = {
 	{ .compatible = "linx,r4" },
 	{ .compatible = "wi2wi,w2sg0008i" },
 	{ .compatible = "wi2wi,w2sg0084i" },
+	{ .compatible = "wi2wi,w2sg0004" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, sirf_of_match);
-- 
2.11.0

Re: [PATCH 2/5] gnss: sirf: power on logic for devices without wakeup signal

[H. Nikolaus Schaller]

> Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> 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.
> Additionally it checks for the initial state of the device during
> probing to ensure it is off.
> Timeout values need to be increased, because the reaction on serial line
> is slower and are in line  with previous patches by
> Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> drivers/gnss/sirf.c | 97 +++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 65 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index b5efbb062316..6a0e5c0a2d62 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -22,8 +22,8 @@
> 
> #define SIRF_BOOT_DELAY			500
> #define SIRF_ON_OFF_PULSE_TIME		100
> -#define SIRF_ACTIVATE_TIMEOUT		200
> -#define SIRF_HIBERNATE_TIMEOUT		200
> +#define SIRF_ACTIVATE_TIMEOUT		1000
> +#define SIRF_HIBERNATE_TIMEOUT		1000
> 
> struct sirf_data {
> 	struct gnss_device *gdev;
> @@ -45,26 +45,14 @@ static int sirf_open(struct gnss_device *gdev)
> 	int ret;
> 
> 	data->opened = true;
> -	ret = serdev_device_open(serdev);
> -	if (ret)
> -		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);
> 		pm_runtime_put_noidle(&serdev->dev);
> 		data->opened = false;
> -		goto err_close;
> 	}
> 
> -	return 0;
> -
> -err_close:
> -	serdev_device_close(serdev);
> -
> 	return ret;
> }
> 
> @@ -73,8 +61,6 @@ 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);
> -
> 	pm_runtime_put(&serdev->dev);
> 	data->opened = false;
> }
> @@ -109,6 +95,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
> 	struct sirf_data *data = serdev_device_get_drvdata(serdev);
> 	struct gnss_device *gdev = data->gdev;
> 
> +	if ((!data->wakeup) && (!data->active)) {
> +		data->active = 1;
> +		wake_up_interruptible(&data->power_wait);
> +	}
> +
> 	/*
> 	 * we might come here everytime when runtime is resumed
> 	 * and data is received. Two cases are possible
> @@ -149,6 +140,25 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
> {
> 	int ret;
> 
> +	/* no wakeup pin, success condition is that
> +	 * no byte comes in in the period
> +	 */
> +	if ((!data->wakeup) && (!active) && (data->active)) {
> +		/* some bytes might come, so sleep a bit first */
> +		msleep(timeout);
> +		data->active = false;
> +		ret = wait_event_interruptible_timeout(data->power_wait,
> +			data->active == true, msecs_to_jiffies(timeout));
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		/* we are still getting woken up -> timeout */
> +		if (ret > 0)
> +			return -ETIMEDOUT;
> +		return 0;
> +	}
> +
> 	ret = wait_event_interruptible_timeout(data->power_wait,
> 			data->active == active, msecs_to_jiffies(timeout));
> 	if (ret < 0)
> @@ -203,21 +213,48 @@ 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);
> +	ret = sirf_set_active(data, false);
> 
> -	return sirf_set_active(data, false);
> +	if (ret)
> +		dev_err(dev, "failed to deactivate");
> +
> +	/* we should close it anyways, so the following receptions
> +	 * will not run into the empty
> +	 */
> +	serdev_device_close(data->serdev);
> +	return 0;
> }
> 
> static int sirf_runtime_resume(struct device *dev)
> {
> +	int ret;
> 	struct sirf_data *data = dev_get_drvdata(dev);
> +	ret = serdev_device_open(data->serdev);
> +	if (ret)
> +		return ret;
> 
> -	if (!data->on_off)
> -		return regulator_enable(data->vcc);
> +	serdev_device_set_baudrate(data->serdev, data->speed);
> +	serdev_device_set_flow_control(data->serdev, false);
> +
> +	if (!data->on_off) {
> +		ret = regulator_enable(data->vcc);
> +		if (ret)
> +			goto err_close_serdev;
> +	}
> +	ret = sirf_set_active(data, true);
> +
> +	if (!ret)
> +		return 0;
> 
> -	return sirf_set_active(data, true);
> +	if (!data->on_off)
> +		regulator_disable(data->vcc);
> +err_close_serdev:
> +	serdev_device_close(data->serdev);
> +	return ret;
> }
> 
> static int __maybe_unused sirf_suspend(struct device *dev)
> @@ -311,18 +348,6 @@ static int sirf_probe(struct serdev_device *serdev)
> 	if (data->on_off) {
> 		data->wakeup = devm_gpiod_get_optional(dev, "sirf,wakeup",
> 				GPIOD_IN);
> -		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) {
> @@ -352,6 +377,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 lets first enable it,
> +		 * then disable it to bring it into a clear state
> +		 */

That is an interesting trick to solve the "device is powered on but kernel does
not know" problem.

The assumption to make your trick work is that there is nobody besides the
running kernel who can turn on/off the device so that after this operation,
the kernel can simply track the power state in a boolean flag.

Since the power control gpio is grabbed by this driver, it can't be exported
to user-space and hence I think we can take this assumption for granted.

> +		ret = pm_runtime_get_sync(dev);
> +		if (ret)
> +			goto err_disable_rpm;
> +		pm_runtime_put(dev);
> 	} else {
> 		ret = sirf_runtime_resume(dev);
> 		if (ret < 0)
> @@ -401,6 +433,7 @@ static const struct of_device_id sirf_of_match[] = {
> 	{ .compatible = "linx,r4" },
> 	{ .compatible = "wi2wi,w2sg0008i" },
> 	{ .compatible = "wi2wi,w2sg0084i" },
> +	{ .compatible = "wi2wi,w2sg0004" },
> 	{},
> };
> MODULE_DEVICE_TABLE(of, sirf_of_match);
> -- 
> 2.11.0
> 
> _______________________________________________
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

BR,
Nikolaus

Re: [PATCH 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Johan Hovold]

On Sun, Nov 18, 2018 at 10:57:58PM +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
> over the serial lines.
> Additionally it checks for the initial state of the device during
> probing to ensure it is off.
> Timeout values need to be increased, because the reaction on serial line
> is slower and are in line  with previous patches by
> Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/gnss/sirf.c | 97 +++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 65 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index b5efbb062316..6a0e5c0a2d62 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -22,8 +22,8 @@
>  
>  #define SIRF_BOOT_DELAY			500
>  #define SIRF_ON_OFF_PULSE_TIME		100
> -#define SIRF_ACTIVATE_TIMEOUT		200
> -#define SIRF_HIBERNATE_TIMEOUT		200
> +#define SIRF_ACTIVATE_TIMEOUT		1000
> +#define SIRF_HIBERNATE_TIMEOUT		1000

We shouldn't increase the timeouts for the general case where we have
wakeup connected.

>  struct sirf_data {
>  	struct gnss_device *gdev;
> @@ -45,26 +45,14 @@ static int sirf_open(struct gnss_device *gdev)
>  	int ret;
>  
>  	data->opened = true;
> -	ret = serdev_device_open(serdev);
> -	if (ret)
> -		return ret;
> -
> -	serdev_device_set_baudrate(serdev, data->speed);
> -	serdev_device_set_flow_control(serdev, false);

And also here, I think we shouldn't change the general case (wakeup
connected) unnecessarily. Currently user space can request the receiver
to remain powered, while not keeping the port open unnecessarily.

>  
>  	ret = pm_runtime_get_sync(&serdev->dev);
>  	if (ret < 0) {
>  		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
>  		pm_runtime_put_noidle(&serdev->dev);
>  		data->opened = false;
> -		goto err_close;
>  	}
>  
> -	return 0;
> -
> -err_close:
> -	serdev_device_close(serdev);
> -
>  	return ret;
>  }
>  
> @@ -73,8 +61,6 @@ 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);
> -
>  	pm_runtime_put(&serdev->dev);
>  	data->opened = false;
>  }
> @@ -109,6 +95,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
>  	struct sirf_data *data = serdev_device_get_drvdata(serdev);
>  	struct gnss_device *gdev = data->gdev;
>  
> +	if ((!data->wakeup) && (!data->active)) {

You have superfluous parenthesis like the above throughout the series.

> +		data->active = 1;

active is bool, so use "true".

> +		wake_up_interruptible(&data->power_wait);
> +	}
> +
>  	/*
>  	 * we might come here everytime when runtime is resumed
>  	 * and data is received. Two cases are possible
> @@ -149,6 +140,25 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
>  {
>  	int ret;
>  
> +	/* no wakeup pin, success condition is that
> +	 * no byte comes in in the period
> +	 */

Multiline comment style needs to be fixed throughout. Also use sentence
case and periods where appropriate.

> +	if ((!data->wakeup) && (!active) && (data->active)) {
> +		/* some bytes might come, so sleep a bit first */
> +		msleep(timeout);

This changes the semantics of the functions and effectively doubles the
requested timeout.

> +		data->active = false;
> +		ret = wait_event_interruptible_timeout(data->power_wait,
> +			data->active == true, msecs_to_jiffies(timeout));
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		/* we are still getting woken up -> timeout */
> +		if (ret > 0)
> +			return -ETIMEDOUT;
> +		return 0;
> +	}
> +
>  	ret = wait_event_interruptible_timeout(data->power_wait,
>  			data->active == active, msecs_to_jiffies(timeout));
>  	if (ret < 0)
> @@ -203,21 +213,48 @@ 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);

Missing newline.

> +	ret = sirf_set_active(data, false);
>  

Superfluous newline.

> -	return sirf_set_active(data, false);
> +	if (ret)
> +		dev_err(dev, "failed to deactivate");

Missing '\n'.

> +
> +	/* we should close it anyways, so the following receptions
> +	 * will not run into the empty
> +	 */

Not sure what you mean here, please rephrase.

> +	serdev_device_close(data->serdev);
> +	return 0;
>  }
>  
>  static int sirf_runtime_resume(struct device *dev)
>  {
> +	int ret;

Please, place after the next declaration (reverse xmas style).

>  	struct sirf_data *data = dev_get_drvdata(dev);

Missing newline.

> +	ret = serdev_device_open(data->serdev);
> +	if (ret)
> +		return ret;
>  
> -	if (!data->on_off)
> -		return regulator_enable(data->vcc);
> +	serdev_device_set_baudrate(data->serdev, data->speed);
> +	serdev_device_set_flow_control(data->serdev, false);
> +
> +	if (!data->on_off) {
> +		ret = regulator_enable(data->vcc);
> +		if (ret)
> +			goto err_close_serdev;
> +	}

Newline.

> +	ret = sirf_set_active(data, true);
> +
> +	if (!ret)
> +		return 0;

Add an error label instead, and return 0 unconditionally in the success
path.

>  
> -	return sirf_set_active(data, true);
> +	if (!data->on_off)
> +		regulator_disable(data->vcc);
> +err_close_serdev:
> +	serdev_device_close(data->serdev);
> +	return ret;
>  }
>  
>  static int __maybe_unused sirf_suspend(struct device *dev)
> @@ -311,18 +348,6 @@ static int sirf_probe(struct serdev_device *serdev)
>  	if (data->on_off) {
>  		data->wakeup = devm_gpiod_get_optional(dev, "sirf,wakeup",
>  				GPIOD_IN);
> -		if (IS_ERR(data->wakeup))
> -			goto err_put_device;

You still want to check for errors here.

> -
> -		/*
> -		 * 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) {
> @@ -352,6 +377,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 lets first enable it,
> +		 * then disable it to bring it into a clear state
> +		 */
> +		ret = pm_runtime_get_sync(dev);
> +		if (ret)
> +			goto err_disable_rpm;
> +		pm_runtime_put(dev);
>  	} else {
>  		ret = sirf_runtime_resume(dev);
>  		if (ret < 0)
> @@ -401,6 +433,7 @@ static const struct of_device_id sirf_of_match[] = {
>  	{ .compatible = "linx,r4" },
>  	{ .compatible = "wi2wi,w2sg0008i" },
>  	{ .compatible = "wi2wi,w2sg0084i" },
> +	{ .compatible = "wi2wi,w2sg0004" },

Keep the entries sorted.

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, sirf_of_match);

Johan

Re: [PATCH 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade]

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

On Wed, 5 Dec 2018 16:01:16 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Sun, Nov 18, 2018 at 10:57:58PM +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
> > over the serial lines.
> > Additionally it checks for the initial state of the device during
> > probing to ensure it is off.
> > Timeout values need to be increased, because the reaction on serial line
> > is slower and are in line  with previous patches by
> > Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/gnss/sirf.c | 97 +++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 65 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> > index b5efbb062316..6a0e5c0a2d62 100644
> > --- a/drivers/gnss/sirf.c
> > +++ b/drivers/gnss/sirf.c
> > @@ -22,8 +22,8 @@
> >  
> >  #define SIRF_BOOT_DELAY			500
> >  #define SIRF_ON_OFF_PULSE_TIME		100
> > -#define SIRF_ACTIVATE_TIMEOUT		200
> > -#define SIRF_HIBERNATE_TIMEOUT		200
> > +#define SIRF_ACTIVATE_TIMEOUT		1000
> > +#define SIRF_HIBERNATE_TIMEOUT		1000  
> 
> We shouldn't increase the timeouts for the general case where we have
> wakeup connected.
> 
Well, in most times they are not hit in the general case, only once
if the internal state is not in sync.
But I can add a second pair of defines with more refined defines.

> >  struct sirf_data {
> >  	struct gnss_device *gdev;
> > @@ -45,26 +45,14 @@ static int sirf_open(struct gnss_device *gdev)
> >  	int ret;
> >  
> >  	data->opened = true;
> > -	ret = serdev_device_open(serdev);
> > -	if (ret)
> > -		return ret;
> > -
> > -	serdev_device_set_baudrate(serdev, data->speed);
> > -	serdev_device_set_flow_control(serdev, false);  
> 
> And also here, I think we shouldn't change the general case (wakeup
> connected) unnecessarily. Currently user space can request the receiver
> to remain powered, while not keeping the port open unnecessarily.
> 
Yes, that usecase makes sense. There is even no need to keep that
device opened in the no-wakeup case. If I just open the serdev
during state change, code will probably be cleaner.

> >  
> >  	ret = pm_runtime_get_sync(&serdev->dev);
> >  	if (ret < 0) {
> >  		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
> >  		pm_runtime_put_noidle(&serdev->dev);
> >  		data->opened = false;
> > -		goto err_close;
> >  	}
> >  
> > -	return 0;
> > -
> > -err_close:
> > -	serdev_device_close(serdev);
> > -
> >  	return ret;
> >  }
> >  
> > @@ -73,8 +61,6 @@ 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);
> > -
> >  	pm_runtime_put(&serdev->dev);
> >  	data->opened = false;
> >  }
> > @@ -109,6 +95,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
> >  	struct sirf_data *data = serdev_device_get_drvdata(serdev);
> >  	struct gnss_device *gdev = data->gdev;
> >  
> > +	if ((!data->wakeup) && (!data->active)) {  
> 
> You have superfluous parenthesis like the above throughout the series.
> 
OK, will reduce them.

> > +		data->active = 1;  
> 
> active is bool, so use "true".
> 
> > +		wake_up_interruptible(&data->power_wait);
> > +	}
> > +
> >  	/*
> >  	 * we might come here everytime when runtime is resumed
> >  	 * and data is received. Two cases are possible
> > @@ -149,6 +140,25 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
> >  {
> >  	int ret;
> >  
> > +	/* no wakeup pin, success condition is that
> > +	 * no byte comes in in the period
> > +	 */  
> 
> Multiline comment style needs to be fixed throughout. Also use sentence
> case and periods where appropriate.
>
OK. maybe I did believe too much in checkpatch.pl. It likes this patch.
I thought it would moan about such basic things.

> > +	if ((!data->wakeup) && (!active) && (data->active)) {
> > +		/* some bytes might come, so sleep a bit first */
> > +		msleep(timeout);  
> 
> This changes the semantics of the functions and effectively doubles the
> requested timeout.
>
So maybe I should sort this block out into a properly-named function
with properly named constants?
The logic is to give the device some time first to calm down. And then
check for some time if it is really down.
 
> > +		data->active = false;
> > +		ret = wait_event_interruptible_timeout(data->power_wait,
> > +			data->active == true, msecs_to_jiffies(timeout));
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* we are still getting woken up -> timeout */
> > +		if (ret > 0)
> > +			return -ETIMEDOUT;
> > +		return 0;
> > +	}
> > +
> >  	ret = wait_event_interruptible_timeout(data->power_wait,
> >  			data->active == active, msecs_to_jiffies(timeout));
> >  	if (ret < 0)
> > @@ -203,21 +213,48 @@ 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);  
> 
[..] minor  style issues ... will fix, still wondering
why checkpatch does not complain. Just saved the patch again  and
checked.
> > +
> > +	/* we should close it anyways, so the following receptions
> > +	 * will not run into the empty
> > +	 */  
> 
> Not sure what you mean here, please rephrase.
> 
If the serdev is closed, nothing will be sent to a probably
not-existing-anymore gnss device.
> > +	serdev_device_close(data->serdev);
> > +	return 0;
> >  }
> >  

[...] more minor style issues
> 
> > +	ret = sirf_set_active(data, true);
> > +
> > +	if (!ret)
> > +		return 0;  
> 
> Add an error label instead, and return 0 unconditionally in the success
> path.
> 
Ok, makes sense.

> >  
> > -	return sirf_set_active(data, true);
> > +	if (!data->on_off)
> > +		regulator_disable(data->vcc);
> > +err_close_serdev:
> > +	serdev_device_close(data->serdev);
> > +	return ret;
> >  }
> >  
> >  static int __maybe_unused sirf_suspend(struct device *dev)
> > @@ -311,18 +348,6 @@ static int sirf_probe(struct serdev_device *serdev)
> >  	if (data->on_off) {
> >  		data->wakeup = devm_gpiod_get_optional(dev, "sirf,wakeup",
> >  				GPIOD_IN);
> > -		if (IS_ERR(data->wakeup))
> > -			goto err_put_device;  
> 
> You still want to check for errors here.
> 
Yes, I should only ignore NULL here..

Regards,
Andreas

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

[PATCH 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>
---
 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..1be7597ae884 100644
--- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -14,6 +14,7 @@ Required properties:
 			"linx,r4"
 			"wi2wi,w2sg0008i"
 			"wi2wi,w2sg0084i"
+			"wi2wi,w2sg0004"
 
 - vcc-supply	: Main voltage regulator (pin name: 3V3_IN, VCC, VDD)
 
-- 
2.11.0

Re: [PATCH 3/5] dt-bindings: gnss: add w2sg0004 compatible string

[Rob Herring]

On Sun, 18 Nov 2018 22:57:59 +0100, Andreas Kemnade wrote:
> Add w2sg0004 compatible string since devices without wakeup
> pins are now supported.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

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

Re: [PATCH 3/5] dt-bindings: gnss: add w2sg0004 compatible string

[Johan Hovold]

On Sun, Nov 18, 2018 at 10:57:59PM +0100, Andreas Kemnade wrote:
> Add w2sg0004 compatible string since devices without wakeup
> pins are now supported.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  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..1be7597ae884 100644
> --- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
> +++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
> @@ -14,6 +14,7 @@ Required properties:
>  			"linx,r4"
>  			"wi2wi,w2sg0008i"
>  			"wi2wi,w2sg0084i"
> +			"wi2wi,w2sg0004"

Please keep the entries sorted.

>  
>  - vcc-supply	: Main voltage regulator (pin name: 3V3_IN, VCC, VDD)

Johan

[PATCH RFC 4/5] gnss: sirf: add a separate supply for a lna

[Andreas Kemnade]

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

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/gnss/sirf.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 6a0e5c0a2d62..f7573ca2dacd 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -30,6 +30,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;
@@ -217,6 +218,7 @@ static int sirf_runtime_suspend(struct device *dev)
 
 	if (!data->on_off)
 		return regulator_disable(data->vcc);
+	regulator_disable(data->lna);
 	ret = sirf_set_active(data, false);
 
 	if (ret)
@@ -245,13 +247,20 @@ static int sirf_runtime_resume(struct device *dev)
 		if (ret)
 			goto err_close_serdev;
 	}
+
+	ret = regulator_enable(data->lna);
+	if (ret)
+		goto err_disable_vcc;
+
 	ret = sirf_set_active(data, true);
 
 	if (!ret)
 		return 0;
 
+err_disable_vcc:
 	if (!data->on_off)
 		regulator_disable(data->vcc);
+
 err_close_serdev:
 	serdev_device_close(data->serdev);
 	return ret;
@@ -340,6 +349,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

Re: [Letux-kernel] [PATCH RFC 4/5] gnss: sirf: add a separate supply for a lna

[H. Nikolaus Schaller]

> Am 18.11.2018 um 22:58 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> Devices might have a separate lna between antenna output of the gps
> chip and the antenna which might have a separate supply
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> drivers/gnss/sirf.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index 6a0e5c0a2d62..f7573ca2dacd 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -30,6 +30,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;
> @@ -217,6 +218,7 @@ static int sirf_runtime_suspend(struct device *dev)
> 
> 	if (!data->on_off)
> 		return regulator_disable(data->vcc);
> +	regulator_disable(data->lna);
> 	ret = sirf_set_active(data, false);
> 
> 	if (ret)
> @@ -245,13 +247,20 @@ static int sirf_runtime_resume(struct device *dev)
> 		if (ret)
> 			goto err_close_serdev;
> 	}
> +
> +	ret = regulator_enable(data->lna);
> +	if (ret)
> +		goto err_disable_vcc;
> +
> 	ret = sirf_set_active(data, true);
> 
> 	if (!ret)
> 		return 0;
> 
> +err_disable_vcc:
> 	if (!data->on_off)
> 		regulator_disable(data->vcc);
> +
> err_close_serdev:
> 	serdev_device_close(data->serdev);
> 	return ret;
> @@ -340,6 +349,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
> 
> _______________________________________________
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

Looks good and to do what it is designed for.

Acked-by: H. Nikolaus Schaller <hns@goldelico.com>

BR and thanks,
Nikolaus

Re: [PATCH RFC 4/5] gnss: sirf: add a separate supply for a lna

[Pavel Machek]

Hi!

> Devices might have a separate lna between antenna output of the gps
> chip and the antenna which might have a separate supply

Might have.

> @@ -340,6 +349,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;
> +	}
> +

But it is not optional in the code. Probably should be?
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH RFC 4/5] gnss: sirf: add a separate supply for a lna

[Andreas Kemnade]

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

Hi,

On Tue, 27 Nov 2018 19:03:57 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > Devices might have a separate lna between antenna output of the gps
> > chip and the antenna which might have a separate supply  
> 
> Might have.
> 
> > @@ -340,6 +349,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;
> > +	}
> > +  
> 
> But it is not optional in the code. Probably should be?

well, if it no lna regulator is defined in the dtb, the regulator
framework will return a dummy regulator. devm_regulator_get_optional()
would not do that and would require more error checking in the code.
But if there is some rule which says that devm_regulator_get_optional()
should be used here, I can of course change that.
Before sending a v2, is that the only issue here?

Regards,
Andreas

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

Re: [PATCH RFC 4/5] gnss: sirf: add a separate supply for a lna

[Pavel Machek]

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

On Fri 2018-11-30 07:38:04, Andreas Kemnade wrote:
> Hi,
> 
> On Tue, 27 Nov 2018 19:03:57 +0100
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Hi!
> > 
> > > Devices might have a separate lna between antenna output of the gps
> > > chip and the antenna which might have a separate supply  
> > 
> > Might have.
> > 
> > > @@ -340,6 +349,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;
> > > +	}
> > > +  
> > 
> > But it is not optional in the code. Probably should be?
> 
> well, if it no lna regulator is defined in the dtb, the regulator
> framework will return a dummy

Aha, did not know that detail. It is ok, then...

> would not do that and would require more error checking in the code.
> But if there is some rule which says that devm_regulator_get_optional()
> should be used here, I can of course change that.
> Before sending a v2, is that the only issue here?

Quick look did not reveal anything else.

Best regards,
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH RFC 4/5] gnss: sirf: add a separate supply for a lna

[Johan Hovold]

On Sun, Nov 18, 2018 at 10:58:00PM +0100, Andreas Kemnade wrote:
> Devices might have a separate lna between antenna output of the gps
> chip and the antenna which might have a separate supply
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/gnss/sirf.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index 6a0e5c0a2d62..f7573ca2dacd 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -30,6 +30,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;
> @@ -217,6 +218,7 @@ static int sirf_runtime_suspend(struct device *dev)
>  
>  	if (!data->on_off)
>  		return regulator_disable(data->vcc);
> +	regulator_disable(data->lna);

I don't think you want to disable it until the device has entered
hibernate mode.

>  	ret = sirf_set_active(data, false);
>  
>  	if (ret)
> @@ -245,13 +247,20 @@ static int sirf_runtime_resume(struct device *dev)
>  		if (ret)
>  			goto err_close_serdev;
>  	}
> +
> +	ret = regulator_enable(data->lna);
> +	if (ret)
> +		goto err_disable_vcc;

This one needs to be managed as vcc in the case where we have no
onoff-signal connected, right? Similar for suspend of course.

> +
>  	ret = sirf_set_active(data, true);
>  
>  	if (!ret)
>  		return 0;
>  
> +err_disable_vcc:
>  	if (!data->on_off)
>  		regulator_disable(data->vcc);
> +

Superfluous newline.

>  err_close_serdev:
>  	serdev_device_close(data->serdev);
>  	return ret;

Johan

[PATCH RFC 5/5] dt-bindings: gnss: add lna-supply property

[Andreas Kemnade]

Add lna-supply property.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 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 1be7597ae884..9614774d14bc 100644
--- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -30,6 +30,7 @@ Optional properties:
 - sirf,wakeup-gpios	: GPIO used to determine device power state
 			  (pin name: RFPWRUP, WAKEUP)
 - timepulse-gpios	: Time pulse GPIO (pin name: 1PPS, TM)
+- lna-supply		: Separate supply for a LNA
 
 Example:
 
-- 
2.11.0

Re: [PATCH RFC 5/5] dt-bindings: gnss: add lna-supply property

[Rob Herring]

On Sun, 18 Nov 2018 22:58:01 +0100, Andreas Kemnade wrote:
> Add lna-supply property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

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

Re: [PATCH RFC 5/5] dt-bindings: gnss: add lna-supply property

[Johan Hovold]

On Sun, Nov 18, 2018 at 10:58:01PM +0100, Andreas Kemnade wrote:
> Add lna-supply property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  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 1be7597ae884..9614774d14bc 100644
> --- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
> +++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
> @@ -30,6 +30,7 @@ Optional properties:
>  - sirf,wakeup-gpios	: GPIO used to determine device power state
>  			  (pin name: RFPWRUP, WAKEUP)
>  - timepulse-gpios	: Time pulse GPIO (pin name: 1PPS, TM)
> +- lna-supply		: Separate supply for a LNA

"Supply for external LNA"?

And please keep the entries sorted.

>  
>  Example:

Johan

Re: [PATCH RFC 5/5] dt-bindings: gnss: add lna-supply property

[Andreas Kemnade]

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

On Wed, 5 Dec 2018 16:09:39 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Sun, Nov 18, 2018 at 10:58:01PM +0100, Andreas Kemnade wrote:
> > Add lna-supply property.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  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 1be7597ae884..9614774d14bc 100644
> > --- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
> > +++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
> > @@ -30,6 +30,7 @@ Optional properties:
> >  - sirf,wakeup-gpios	: GPIO used to determine device power state
> >  			  (pin name: RFPWRUP, WAKEUP)
> >  - timepulse-gpios	: Time pulse GPIO (pin name: 1PPS, TM)
> > +- lna-supply		: Separate supply for a LNA  
> 
> "Supply for external LNA"?
> 
"External" might be misleading:
- not part of the gps chip itself: yes
- but might also be inside of the device.

In case of the GTA04 this includes power supply for an antenna switch
(operates by plug detection) + internal or external antenna.

Regards,
Andreas

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

Re: [Letux-kernel] [PATCH 0/5] gnss: sirf: add support for w2sg0004 + lna

[H. Nikolaus Schaller]

Hi,

> Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> 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. That part needs probably more discussion, since it might
> be an idea to have it more generalized since it has nothing todo
> with the chip itself.

On the other hand if we follow the "SoC is the spider in the net"
way of looking at DTS hierarchy, we have the uart as a child of the
SoC and the gnss receiver as a serdev child of the UART. The LNA
is even one step more distantly connected to the gnss. So it makes
sense to me to have it as a property/reference of the gnss chip's
DTS record which is a sibling of the compatible records. So the only
place where it can be reasonably processed is the driver.

> I marked the corresponding patches as RFC. The support for
> the w2sg0004 without wakeup can imho go in without the lna first
> because users of that chip without an additional lna power supply
> can already benefit from it if we should do more discussion first.
> I just kept them together so that the full picture can be seen.
> 
> Andreas Kemnade (5):
>  gnss: sirf: write data to gnss only when the gnss device is open
>  gnss: sirf: power on logic for devices 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
> 
> .../devicetree/bindings/gnss/sirfstar.txt          |   2 +
> drivers/gnss/sirf.c                                | 126 +++++++++++++++------
> 2 files changed, 96 insertions(+), 32 deletions(-)
> 
> -- 
> 2.11.0
> 
> _______________________________________________
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

BR,
Nikolaus

Re: [Letux-kernel] [PATCH 0/5] gnss: sirf: add support for w2sg0004 + lna

[Andreas Kemnade]

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

Hi,

On Mon, 19 Nov 2018 09:22:59 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> Hi,
> 
> > Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <andreas@kemnade.info>:
> > 
> > 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. That part needs probably more discussion, since it might
> > be an idea to have it more generalized since it has nothing todo
> > with the chip itself.  
> 
> On the other hand if we follow the "SoC is the spider in the net"
> way of looking at DTS hierarchy, we have the uart as a child of the
> SoC and the gnss receiver as a serdev child of the UART. The LNA
> is even one step more distantly connected to the gnss. So it makes
> sense to me to have it as a property/reference of the gnss chip's
> DTS record which is a sibling of the compatible records. So the only
> place where it can be reasonably processed is the driver.
> 
Or the lna is a child of the gnss receiver. The whole thing
should probably not be overdesigned, but it does not make sense that
every gnss chip driver has some lna logic.
Maybe the regulator should just be stored in the struct
gnss_device and then drivers/gnss/core.c takes care.

Regards,
Andreas

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

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

[H. Nikolaus Schaller]

Hi,

> Am 19.11.2018 um 19:44 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> Hi,
> 
> On Mon, 19 Nov 2018 09:22:59 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> Hi,
>> 
>>> Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <andreas@kemnade.info>:
>>> 
>>> 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. That part needs probably more discussion, since it might
>>> be an idea to have it more generalized since it has nothing todo
>>> with the chip itself.  
>> 
>> On the other hand if we follow the "SoC is the spider in the net"
>> way of looking at DTS hierarchy, we have the uart as a child of the
>> SoC and the gnss receiver as a serdev child of the UART. The LNA
>> is even one step more distantly connected to the gnss. So it makes
>> sense to me to have it as a property/reference of the gnss chip's
>> DTS record which is a sibling of the compatible records. So the only
>> place where it can be reasonably processed is the driver.
>> 
> Or the lna is a child of the gnss receiver.

Well, this IMHO would assume the gnss receiver is a bus master...

> The whole thing
> should probably not be overdesigned, but it does not make sense that
> every gnss chip driver has some lna logic.
> Maybe the regulator should just be stored in the struct
> gnss_device and then drivers/gnss/core.c takes care.

Probably yes, but likely not difficult to refactor and generalize later
if users of other chips report a similar need. More important is to
get upstream support for the gta04 with this chip.

BR,
Nikolaus

Re: [Letux-kernel] [PATCH 0/5] gnss: sirf: add support for w2sg0004 + lna

[Johan Hovold]

On Mon, Nov 19, 2018 at 07:44:14PM +0100, Andreas Kemnade wrote:

> On Mon, 19 Nov 2018 09:22:59 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> > > Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <andreas@kemnade.info>:
> > > 
> > > 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. That part needs probably more discussion, since it might
> > > be an idea to have it more generalized since it has nothing todo
> > > with the chip itself.  
> > 
> > On the other hand if we follow the "SoC is the spider in the net"
> > way of looking at DTS hierarchy, we have the uart as a child of the
> > SoC and the gnss receiver as a serdev child of the UART. The LNA
> > is even one step more distantly connected to the gnss. So it makes
> > sense to me to have it as a property/reference of the gnss chip's
> > DTS record which is a sibling of the compatible records. So the only
> > place where it can be reasonably processed is the driver.
> > 
> Or the lna is a child of the gnss receiver. The whole thing
> should probably not be overdesigned, but it does not make sense that
> every gnss chip driver has some lna logic.

Did you mean "does make sense" here?

> Maybe the regulator should just be stored in the struct
> gnss_device and then drivers/gnss/core.c takes care.

Maybe eventually, but keeping it per driver is fine for now.

As you say above, this really isn't part of the chip itself, and
therefore should probably be a generic gnss property as it could be
required for any receiver (in principle).

But we still need driver support for coordinating it with the rest of
the receiver power management, so adding it to drivers as need arises
seems reasonable.

Thanks,
Johan

Re: [Letux-kernel] [PATCH 0/5] gnss: sirf: add support for w2sg0004 + lna

[Johan Hovold]

On Wed, Dec 05, 2018 at 04:19:16PM +0100, Johan Hovold wrote:
> On Mon, Nov 19, 2018 at 07:44:14PM +0100, Andreas Kemnade wrote:
> 
> > On Mon, 19 Nov 2018 09:22:59 +0100
> > "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
> > > > Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <andreas@kemnade.info>:
> > > > 
> > > > 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. That part needs probably more discussion, since it might
> > > > be an idea to have it more generalized since it has nothing todo
> > > > with the chip itself.  
> > > 
> > > On the other hand if we follow the "SoC is the spider in the net"
> > > way of looking at DTS hierarchy, we have the uart as a child of the
> > > SoC and the gnss receiver as a serdev child of the UART. The LNA
> > > is even one step more distantly connected to the gnss. So it makes
> > > sense to me to have it as a property/reference of the gnss chip's
> > > DTS record which is a sibling of the compatible records. So the only
> > > place where it can be reasonably processed is the driver.
> > > 
> > Or the lna is a child of the gnss receiver. The whole thing
> > should probably not be overdesigned, but it does not make sense that
> > every gnss chip driver has some lna logic.
> 
> Did you mean "does make sense" here?
> 
> > Maybe the regulator should just be stored in the struct
> > gnss_device and then drivers/gnss/core.c takes care.
> 
> Maybe eventually, but keeping it per driver is fine for now.
> 
> As you say above, this really isn't part of the chip itself, and
> therefore should probably be a generic gnss property as it could be
> required for any receiver (in principle).
> 
> But we still need driver support for coordinating it with the rest of
> the receiver power management, so adding it to drivers as need arises
> seems reasonable.

Actually, the property probably still should go into gnss.txt as a
generic optional property, but driver support for it will be added as
need arises.

Rob?

Thanks,
Johan