[PATCH v3 0/3] gnss: ubx: updates to support the Renesas KingFisher board

[PATCH v3 0/3] gnss: ubx: updates to support the Renesas KingFisher board

[Wolfram Sang]

For that, we need "reset-gpio" support (patches 2+3). But first,
simplify regulator handling with a new helper (patch 1).

Changes since v2:
* rebased to 6.6-rc2
* added tags from Conor & Geert (Thanks!)

Wolfram Sang (3):
  gnss: ubx: use new helper to remove open coded regulator handling
  dt-bindings: gnss: u-blox: add "reset-gpios" binding
  gnss: ubx: add support for the reset gpio

 .../bindings/gnss/u-blox,neo-6m.yaml          |  5 +++
 drivers/gnss/ubx.c                            | 35 ++++++++-----------
 2 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.35.1

[PATCH v3 1/3] gnss: ubx: use new helper to remove open coded regulator handling

[Wolfram Sang]

v_bckp shall always be enabled as long as the device exists. We now have
a regulator helper for that, use it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gnss/ubx.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index c951be202ca2..9b76b101ba5e 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -17,7 +17,6 @@
 #include "serial.h"
 
 struct ubx_data {
-	struct regulator *v_bckp;
 	struct regulator *vcc;
 };
 
@@ -87,30 +86,16 @@ static int ubx_probe(struct serdev_device *serdev)
 		goto err_free_gserial;
 	}
 
-	data->v_bckp = devm_regulator_get_optional(&serdev->dev, "v-bckp");
-	if (IS_ERR(data->v_bckp)) {
-		ret = PTR_ERR(data->v_bckp);
-		if (ret == -ENODEV)
-			data->v_bckp = NULL;
-		else
-			goto err_free_gserial;
-	}
-
-	if (data->v_bckp) {
-		ret = regulator_enable(data->v_bckp);
-		if (ret)
-			goto err_free_gserial;
-	}
+	ret = devm_regulator_get_enable_optional(&serdev->dev, "v-bckp");
+	if (ret < 0 && ret != -ENODEV)
+		goto err_free_gserial;
 
 	ret = gnss_serial_register(gserial);
 	if (ret)
-		goto err_disable_v_bckp;
+		goto err_free_gserial;
 
 	return 0;
 
-err_disable_v_bckp:
-	if (data->v_bckp)
-		regulator_disable(data->v_bckp);
 err_free_gserial:
 	gnss_serial_free(gserial);
 
@@ -120,11 +105,8 @@ static int ubx_probe(struct serdev_device *serdev)
 static void ubx_remove(struct serdev_device *serdev)
 {
 	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
-	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
 
 	gnss_serial_deregister(gserial);
-	if (data->v_bckp)
-		regulator_disable(data->v_bckp);
 	gnss_serial_free(gserial);
 }
 
-- 
2.35.1

[PATCH v3 2/3] dt-bindings: gnss: u-blox: add "reset-gpios" binding

[Wolfram Sang]

Needed to enable this chip on a Renesas KingFisher board. Description
copied over from the Mediatek driver which already supports it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/gnss/u-blox,neo-6m.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/gnss/u-blox,neo-6m.yaml b/Documentation/devicetree/bindings/gnss/u-blox,neo-6m.yaml
index 4835a280b3bf..8e97e475613f 100644
--- a/Documentation/devicetree/bindings/gnss/u-blox,neo-6m.yaml
+++ b/Documentation/devicetree/bindings/gnss/u-blox,neo-6m.yaml
@@ -41,6 +41,9 @@ properties:
     description: >
       Backup voltage regulator
 
+  reset-gpios:
+    maxItems: 1
+
 required:
   - compatible
   - vcc-supply
@@ -49,10 +52,12 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     serial {
         gnss {
             compatible = "u-blox,neo-8";
             v-bckp-supply = <&gnss_v_bckp_reg>;
             vcc-supply = <&gnss_vcc_reg>;
+            reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
         };
     };
-- 
2.35.1

Re: [PATCH v3 2/3] dt-bindings: gnss: u-blox: add "reset-gpios" binding

[Johan Hovold]

On Thu, Sep 21, 2023 at 03:32:00PM +0200, Wolfram Sang wrote:
> Needed to enable this chip on a Renesas KingFisher board.

What is needed? Please make the commit message self-contained.

And what GNSS chip/module is this? This should also be included in the
commit message.

Do you have a link to a datasheet?

None of the u-blox modules I've seen have a reset line so I'd like to
where this came from and how it is intended to be used.

> Description
> copied over from the Mediatek driver which already supports it.

The mediatek driver does not support managing a reset line, but the
binding includes a description of this pin for completeness. Also you
don't seem include any description of the property below (which is fine)
so perhaps you can just drop this sentence.

Johan

Re: [PATCH v3 2/3] dt-bindings: gnss: u-blox: add "reset-gpios" binding

[Wolfram Sang]

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

Hi Johan,

> And what GNSS chip/module is this? This should also be included in the
> commit message.

Ok. UBlox Neo-M8.

> Do you have a link to a datasheet?

https://www.u-blox.com/sites/default/files/NEO-M8-FW3_DataSheet_UBX-15031086.pdf

> None of the u-blox modules I've seen have a reset line so I'd like to
> where this came from and how it is intended to be used.

I didn't know that old modules did not have the reset pin. I thought
they were simply not used, so far. This one has. Check pin8 in chapter
2.1 in the datasheet.

> The mediatek driver does not support managing a reset line, but the
> binding includes a description of this pin for completeness. Also you
> don't seem include any description of the property below (which is fine)
> so perhaps you can just drop this sentence.

Correct.

Kind regards,

   Wolfram


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

Re: [PATCH v3 2/3] dt-bindings: gnss: u-blox: add "reset-gpios" binding

[Johan Hovold]

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

On Mon, Oct 23, 2023 at 08:52:25AM +0200, Wolfram Sang wrote:
> > And what GNSS chip/module is this? This should also be included in the
> > commit message.
> 
> Ok. UBlox Neo-M8.
> 
> > Do you have a link to a datasheet?
> 
> https://www.u-blox.com/sites/default/files/NEO-M8-FW3_DataSheet_UBX-15031086.pdf
> 
> > None of the u-blox modules I've seen have a reset line so I'd like to
> > where this came from and how it is intended to be used.
> 
> I didn't know that old modules did not have the reset pin. I thought
> they were simply not used, so far. This one has. Check pin8 in chapter
> 2.1 in the datasheet.

Indeed. I must have looked at the datasheet of the older neo-6, which
does not have a reset pin, before replying.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 3/3] gnss: ubx: add support for the reset gpio

[Wolfram Sang]

Tested with a Renesas KingFisher board. Because my GNSS device is hooked
up via UART and I2C simultaneously, I could verify functionality by
opening/closing the GNSS device using UART and see if the corresponding
I2C device was visible on the bus.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gnss/ubx.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index 9b76b101ba5e..e7d151cbc8c3 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -7,6 +7,7 @@
 
 #include <linux/errno.h>
 #include <linux/gnss.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -18,6 +19,7 @@
 
 struct ubx_data {
 	struct regulator *vcc;
+	struct gpio_desc *reset_gpio;
 };
 
 static int ubx_set_active(struct gnss_serial *gserial)
@@ -29,6 +31,8 @@ static int ubx_set_active(struct gnss_serial *gserial)
 	if (ret)
 		return ret;
 
+	gpiod_set_value_cansleep(data->reset_gpio, 0);
+
 	return 0;
 }
 
@@ -37,6 +41,8 @@ static int ubx_set_standby(struct gnss_serial *gserial)
 	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
 	int ret;
 
+	gpiod_set_value_cansleep(data->reset_gpio, 1);
+
 	ret = regulator_disable(data->vcc);
 	if (ret)
 		return ret;
@@ -90,6 +96,13 @@ static int ubx_probe(struct serdev_device *serdev)
 	if (ret < 0 && ret != -ENODEV)
 		goto err_free_gserial;
 
+	/* Start with reset asserted */
+	data->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpio)) {
+		ret = PTR_ERR(data->reset_gpio);
+		goto err_free_gserial;
+	}
+
 	ret = gnss_serial_register(gserial);
 	if (ret)
 		goto err_free_gserial;
-- 
2.35.1

Re: [PATCH v3 3/3] gnss: ubx: add support for the reset gpio

[Johan Hovold]

On Thu, Sep 21, 2023 at 03:32:01PM +0200, Wolfram Sang wrote:
> Tested with a Renesas KingFisher board. Because my GNSS device is hooked
> up via UART and I2C simultaneously, I could verify functionality by
> opening/closing the GNSS device using UART and see if the corresponding
> I2C device was visible on the bus.

Again, please try to make the commit message self-contained (e.g.
without implicitly referring to Subject).

Also say something about which u-blox module this is needed for since
the modules I've seen do not have a reset line.

>  static int ubx_set_active(struct gnss_serial *gserial)
> @@ -29,6 +31,8 @@ static int ubx_set_active(struct gnss_serial *gserial)
>  	if (ret)
>  		return ret;
>  
> +	gpiod_set_value_cansleep(data->reset_gpio, 0);
> +
>  	return 0;
>  }
>  
> @@ -37,6 +41,8 @@ static int ubx_set_standby(struct gnss_serial *gserial)
>  	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
>  	int ret;
>  
> +	gpiod_set_value_cansleep(data->reset_gpio, 1);
> +
>  	ret = regulator_disable(data->vcc);
>  	if (ret)
>  		return ret;
> @@ -90,6 +96,13 @@ static int ubx_probe(struct serdev_device *serdev)
>  	if (ret < 0 && ret != -ENODEV)
>  		goto err_free_gserial;
>  
> +	/* Start with reset asserted */
> +	data->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		ret = PTR_ERR(data->reset_gpio);
> +		goto err_free_gserial;
> +	}

So this means that the reset line will be asserted indefinitely in case
the GNSS receiver is not used. Are you sure that's a good idea?

I don't know yet which module this is for, or how this signal is
supposed to be used, but the above makes this look more like an
active-high (regulator) enable signal. Perhaps that's what it is or
should be modelled as (i.e. as a fixed regulator).

> +
>  	ret = gnss_serial_register(gserial);
>  	if (ret)
>  		goto err_free_gserial;

Johan

Re: [PATCH v3 3/3] gnss: ubx: add support for the reset gpio

[Wolfram Sang]

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


> So this means that the reset line will be asserted indefinitely in case
> the GNSS receiver is not used. Are you sure that's a good idea?

Yup. Saves power. We do this for our ethernet PHYs as well. Any reasons
not to do this?

> I don't know yet which module this is for, or how this signal is
> supposed to be used, but the above makes this look more like an
> active-high (regulator) enable signal. Perhaps that's what it is or
> should be modelled as (i.e. as a fixed regulator).

Nope, it is a RESET_N pin.


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

Re: [PATCH v3 3/3] gnss: ubx: add support for the reset gpio

[Johan Hovold]

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

On Mon, Oct 23, 2023 at 09:09:03AM +0200, Wolfram Sang wrote:
> > So this means that the reset line will be asserted indefinitely in case
> > the GNSS receiver is not used. Are you sure that's a good idea?
> 
> Yup. Saves power. We do this for our ethernet PHYs as well. Any reasons
> not to do this?

AFAIK you should generally not try to use reset this way as you may end
up leaking current (and possibly other complications).
 
> > I don't know yet which module this is for, or how this signal is
> > supposed to be used, but the above makes this look more like an
> > active-high (regulator) enable signal. Perhaps that's what it is or
> > should be modelled as (i.e. as a fixed regulator).
> 
> Nope, it is a RESET_N pin.

And the neo-m8 hardware integration manual explicitly says that it shall
not be used as a disable signal:

	1.5 I/O pins
	RESET_N: Reset input
	Driving RESET_N low activates a hardware reset of the system.
	Use this pin only to reset the module. Do not use RESET_N to
	turn the module on and off, since the reset state increases
	power consumption.

Some of the other modules in this family says so explicitly also in the
datasheet.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]