2018-11-18 21:59:36

by Andreas Kemnade

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

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

2018-11-18 22:00:06

by Andreas Kemnade

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

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 <[email protected]>
---
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

2018-11-18 22:00:22

by Andreas Kemnade

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

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

Signed-off-by: Andreas Kemnade <[email protected]>
---
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

2018-11-18 22:00:34

by Andreas Kemnade

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

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 <[email protected]> and H. Nikolaus Schaller <[email protected]>.

Signed-off-by: Andreas Kemnade <[email protected]>
---
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

2018-11-18 22:00:38

by Andreas Kemnade

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

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

Signed-off-by: Andreas Kemnade <[email protected]>
---
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

2018-11-18 22:01:05

by Andreas Kemnade

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

Add lna-supply property.

Signed-off-by: Andreas Kemnade <[email protected]>
---
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

2018-11-19 08:24:46

by H. Nikolaus Schaller

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

Hi,

> Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <[email protected]>:
>
> 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
> [email protected]
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

BR,
Nikolaus

2018-11-19 08:38:13

by H. Nikolaus Schaller

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

> Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <[email protected]>:
>
> 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 <[email protected]> and H. Nikolaus Schaller <[email protected]>.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> 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
> [email protected]
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

BR,
Nikolaus

2018-11-19 08:43:02

by H. Nikolaus Schaller

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

> Am 18.11.2018 um 22:58 schrieb Andreas Kemnade <[email protected]>:
>
> 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 <[email protected]>
> ---
> 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
> [email protected]
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

Looks good and to do what it is designed for.

Acked-by: H. Nikolaus Schaller <[email protected]>

BR and thanks,
Nikolaus

2018-11-19 18:54:49

by Andreas Kemnade

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

Hi,

On Mon, 19 Nov 2018 09:22:59 +0100
"H. Nikolaus Schaller" <[email protected]> wrote:

> Hi,
>
> > Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <[email protected]>:
> >
> > 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

Attachments:

2018-11-19 19:07:12

by H. Nikolaus Schaller

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

Hi,

> Am 19.11.2018 um 19:44 schrieb Andreas Kemnade <[email protected]>:
>
> Hi,
>
> On Mon, 19 Nov 2018 09:22:59 +0100
> "H. Nikolaus Schaller" <[email protected]> wrote:
>
>> Hi,
>>
>>> Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <[email protected]>:
>>>
>>> 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

2018-11-27 18:06:51

by Pavel Machek

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

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

2018-11-30 06:39:11

by Andreas Kemnade

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

Hi,

On Tue, 27 Nov 2018 19:03:57 +0100
Pavel Machek <[email protected]> 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

Attachments:

2018-11-30 08:46:15

by Pavel Machek

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

On Fri 2018-11-30 07:38:04, Andreas Kemnade wrote:
> Hi,
>
> On Tue, 27 Nov 2018 19:03:57 +0100
> Pavel Machek <[email protected]> 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

Attachments:

2018-12-04 22:58:45

by Rob Herring (Arm)

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

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 <[email protected]>
> ---
> Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
> 1 file changed, 1 insertion(+)
>

Reviewed-by: Rob Herring <[email protected]>

2018-12-04 23:00:31

by Rob Herring (Arm)

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

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

Reviewed-by: Rob Herring <[email protected]>

2018-12-05 14:49:07

by Johan Hovold

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

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 <[email protected]>
> ---
> 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

2018-12-05 15:03:51

by Johan Hovold

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

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 <[email protected]> and H. Nikolaus Schaller <[email protected]>.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> 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

2018-12-05 15:04:31

by Johan Hovold

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

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 <[email protected]>
> ---
> 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

2018-12-05 15:08:04

by Johan Hovold

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

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 <[email protected]>
> ---
> 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

2018-12-05 15:11:05

by Johan Hovold

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

On Sun, Nov 18, 2018 at 10:58:01PM +0100, Andreas Kemnade wrote:
> Add lna-supply property.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> 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

2018-12-05 15:20:16

by Johan Hovold

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

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" <[email protected]> wrote:

> > > Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <[email protected]>:
> > >
> > > 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

2018-12-05 16:03:17

by Johan Hovold

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

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" <[email protected]> wrote:
>
> > > > Am 18.11.2018 um 22:57 schrieb Andreas Kemnade <[email protected]>:
> > > >
> > > > 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

2018-12-05 20:17:01

by Andreas Kemnade

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

On Wed, 5 Dec 2018 15:47:39 +0100
Johan Hovold <[email protected]> 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 <[email protected]>
> > ---
> > 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

Attachments:

2018-12-05 22:17:17

by Andreas Kemnade

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

On Wed, 5 Dec 2018 16:01:16 +0100
Johan Hovold <[email protected]> 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 <[email protected]> and H. Nikolaus Schaller <[email protected]>.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > 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

Attachments:

2018-12-09 19:12:41

by Andreas Kemnade

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

On Wed, 5 Dec 2018 16:09:39 +0100
Johan Hovold <[email protected]> wrote:

> On Sun, Nov 18, 2018 at 10:58:01PM +0100, Andreas Kemnade wrote:
> > Add lna-supply property.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > 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

Attachments: