[RFC PATCH 0/5] gnss: updates to support the Renesas KingFisher board

[RFC PATCH 0/5] gnss: updates to support the Renesas KingFisher board

[Wolfram Sang]

Hi, here are some patches with which I can use the GPS receiver on the
Renesas KingFisher board. They are RFC because of my proposal to merge
UBX and MTK drivers in patch 1. Patches 2-5 should be good to go from my
point of view. Looking forward to comments!

   Wolfram


Wolfram Sang (5):
  WIP: gnss: merge MTK driver into UBX driver
  gnss: ubx: use new helper to remove open coded regulator handling
  gnss: ubx: 'vcc' can be optional
  gnss: ubx: add support for the reset gpio
  arm64: dts: renesas: ulcb-kf: add node for GPS

 arch/arm64/boot/dts/renesas/ulcb-kf.dtsi |  9 ++-
 drivers/gnss/ubx.c                       | 82 +++++++++++++++---------
 2 files changed, 58 insertions(+), 33 deletions(-)

-- 
2.35.1

[RFC PATCH 1/5] WIP: gnss: merge MTK driver into UBX driver

[Wolfram Sang]

This is a proof-of-concept how easy it is to merge those two drivers as
they are extremly similar. The differences can be abstracted away
easily. Further work (renaming from 'ubx' to something more generic,
removing the MTK driver, ...) is postponed until there is agrement that
merging these drivers is actually wanted. I vote for it, though. I have
updates to the UBX driver which do make sense for the MTK driver as
well. Code saving is also a plus. We can always fork into a specific
driver again if we'd ever need that.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/gnss/ubx.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index c951be202ca2..c01e1e875727 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -63,12 +63,31 @@ static const struct gnss_serial_ops ubx_gserial_ops = {
 	.set_power = ubx_set_power,
 };
 
+struct ubx_info {
+	enum gnss_type type;
+	char *v_bckp_name;
+};
+
+const struct ubx_info ubx_info_ubx = {
+	.type = GNSS_TYPE_UBX,
+	.v_bckp_name = "v-bckp",
+};
+
+const struct ubx_info ubx_info_mtk = {
+	.type = GNSS_TYPE_MTK,
+	.v_bckp_name = "vbackup",
+};
+
 static int ubx_probe(struct serdev_device *serdev)
 {
+	const struct ubx_info *info = of_device_get_match_data(&serdev->dev);
 	struct gnss_serial *gserial;
 	struct ubx_data *data;
 	int ret;
 
+	if (!info)
+		return -ENOENT;
+
 	gserial = gnss_serial_allocate(serdev, sizeof(*data));
 	if (IS_ERR(gserial)) {
 		ret = PTR_ERR(gserial);
@@ -77,7 +96,7 @@ static int ubx_probe(struct serdev_device *serdev)
 
 	gserial->ops = &ubx_gserial_ops;
 
-	gserial->gdev->type = GNSS_TYPE_UBX;
+	gserial->gdev->type = info->type;
 
 	data = gnss_serial_get_drvdata(gserial);
 
@@ -87,7 +106,7 @@ static int ubx_probe(struct serdev_device *serdev)
 		goto err_free_gserial;
 	}
 
-	data->v_bckp = devm_regulator_get_optional(&serdev->dev, "v-bckp");
+	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
 	if (IS_ERR(data->v_bckp)) {
 		ret = PTR_ERR(data->v_bckp);
 		if (ret == -ENODEV)
@@ -130,9 +149,10 @@ static void ubx_remove(struct serdev_device *serdev)
 
 #ifdef CONFIG_OF
 static const struct of_device_id ubx_of_match[] = {
-	{ .compatible = "u-blox,neo-6m" },
-	{ .compatible = "u-blox,neo-8" },
-	{ .compatible = "u-blox,neo-m8" },
+	{ .compatible = "u-blox,neo-6m", .data = &ubx_info_ubx, },
+	{ .compatible = "u-blox,neo-8", .data = &ubx_info_ubx, },
+	{ .compatible = "u-blox,neo-m8", .data = &ubx_info_ubx, },
+	{ .compatible = "globaltop,pa6h", .data = &ubx_info_mtk },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ubx_of_match);
-- 
2.35.1

[RFC PATCH 2/5] 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>
---
 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 c01e1e875727..c8d027973b85 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;
 };
 
@@ -106,30 +105,16 @@ static int ubx_probe(struct serdev_device *serdev)
 		goto err_free_gserial;
 	}
 
-	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
-	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, info->v_bckp_name);
+	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);
 
@@ -139,11 +124,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

[RFC PATCH 3/5] gnss: ubx: 'vcc' can be optional

[Wolfram Sang]

There are devices where 'vcc' is always on. The reset pin is used for
silencing the chip. Make the 'vcc' regulator optional.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/gnss/ubx.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index c8d027973b85..d5281bfae9cb 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -25,9 +25,11 @@ static int ubx_set_active(struct gnss_serial *gserial)
 	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
 	int ret;
 
-	ret = regulator_enable(data->vcc);
-	if (ret)
-		return ret;
+	if (data->vcc) {
+		ret = regulator_enable(data->vcc);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -37,9 +39,11 @@ static int ubx_set_standby(struct gnss_serial *gserial)
 	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
 	int ret;
 
-	ret = regulator_disable(data->vcc);
-	if (ret)
-		return ret;
+	if (data->vcc) {
+		ret = regulator_disable(data->vcc);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -99,10 +103,13 @@ static int ubx_probe(struct serdev_device *serdev)
 
 	data = gnss_serial_get_drvdata(gserial);
 
-	data->vcc = devm_regulator_get(&serdev->dev, "vcc");
+	data->vcc = devm_regulator_get_optional(&serdev->dev, "vcc");
 	if (IS_ERR(data->vcc)) {
 		ret = PTR_ERR(data->vcc);
-		goto err_free_gserial;
+		if (ret == -ENODEV)
+			data->vcc = NULL;
+		else
+			goto err_free_gserial;
 	}
 
 	ret = devm_regulator_get_enable_optional(&serdev->dev, info->v_bckp_name);
-- 
2.35.1

[RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Wolfram Sang]

Tested with a Renesas KingFisher board. The chip correctly disappears
from the I2C bus when the 'gnss0' device is not opened.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/gnss/ubx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index d5281bfae9cb..be393b123d9a 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)
@@ -31,6 +33,8 @@ static int ubx_set_active(struct gnss_serial *gserial)
 			return ret;
 	}
 
+	gpiod_set_value_cansleep(data->reset_gpio, 0);
+
 	return 0;
 }
 
@@ -45,6 +49,8 @@ static int ubx_set_standby(struct gnss_serial *gserial)
 			return ret;
 	}
 
+	gpiod_set_value_cansleep(data->reset_gpio, 1);
+
 	return 0;
 }
 
@@ -116,6 +122,11 @@ static int ubx_probe(struct serdev_device *serdev)
 	if (ret < 0 && ret != -ENODEV)
 		goto err_free_gserial;
 
+	/* Start with reset asserted (GPIO must be active low!) */
+	data->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpio))
+		return PTR_ERR(data->reset_gpio);
+
 	ret = gnss_serial_register(gserial);
 	if (ret)
 		goto err_free_gserial;
-- 
2.35.1

[RFC PATCH 5/5] arm64: dts: renesas: ulcb-kf: add node for GPS

[Wolfram Sang]

Add the node for the GPS receiver and remove flow control for SCIF1
which is not present. The schematics are a bit misleading. The pins are
sometimes named SCIF1 but they are for HSCIF1.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm64/boot/dts/renesas/ulcb-kf.dtsi | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
index efc80960380f..f6705cad78e4 100644
--- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
+++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
@@ -367,7 +367,7 @@ hscif0_pins: hscif0 {
 	};
 
 	scif1_pins: scif1 {
-		groups = "scif1_data_b", "scif1_ctrl";
+		groups = "scif1_data_b";
 		function = "scif1";
 	};
 
@@ -397,9 +397,14 @@ &sound_clk_pins
 &scif1 {
 	pinctrl-0 = <&scif1_pins>;
 	pinctrl-names = "default";
-	uart-has-rtscts;
 
 	status = "okay";
+
+	gnss {
+		compatible = "u-blox,neo-m8";
+		reset-gpios = <&gpio_exp_75 6 GPIO_ACTIVE_LOW>;
+		current-speed = <9600>;
+	};
 };
 
 &sdhi3 {
-- 
2.35.1

Re: [RFC PATCH 1/5] WIP: gnss: merge MTK driver into UBX driver

[Wolfram Sang]

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

On Tue, May 23, 2023 at 08:43:06AM +0200, Wolfram Sang wrote:
> This is a proof-of-concept how easy it is to merge those two drivers as
> they are extremly similar. The differences can be abstracted away
> easily. Further work (renaming from 'ubx' to something more generic,
> removing the MTK driver, ...) is postponed until there is agrement that
> merging these drivers is actually wanted. I vote for it, though. I have
> updates to the UBX driver which do make sense for the MTK driver as
> well. Code saving is also a plus. We can always fork into a specific
> driver again if we'd ever need that.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Johan, just from a gut feeling or a hi level glimpse: is this merging of
the driver worth pursuing?

Thanks and all the best,

   Wolfram

> ---
>  drivers/gnss/ubx.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
> index c951be202ca2..c01e1e875727 100644
> --- a/drivers/gnss/ubx.c
> +++ b/drivers/gnss/ubx.c
> @@ -63,12 +63,31 @@ static const struct gnss_serial_ops ubx_gserial_ops = {
>  	.set_power = ubx_set_power,
>  };
>  
> +struct ubx_info {
> +	enum gnss_type type;
> +	char *v_bckp_name;
> +};
> +
> +const struct ubx_info ubx_info_ubx = {
> +	.type = GNSS_TYPE_UBX,
> +	.v_bckp_name = "v-bckp",
> +};
> +
> +const struct ubx_info ubx_info_mtk = {
> +	.type = GNSS_TYPE_MTK,
> +	.v_bckp_name = "vbackup",
> +};
> +
>  static int ubx_probe(struct serdev_device *serdev)
>  {
> +	const struct ubx_info *info = of_device_get_match_data(&serdev->dev);
>  	struct gnss_serial *gserial;
>  	struct ubx_data *data;
>  	int ret;
>  
> +	if (!info)
> +		return -ENOENT;
> +
>  	gserial = gnss_serial_allocate(serdev, sizeof(*data));
>  	if (IS_ERR(gserial)) {
>  		ret = PTR_ERR(gserial);
> @@ -77,7 +96,7 @@ static int ubx_probe(struct serdev_device *serdev)
>  
>  	gserial->ops = &ubx_gserial_ops;
>  
> -	gserial->gdev->type = GNSS_TYPE_UBX;
> +	gserial->gdev->type = info->type;
>  
>  	data = gnss_serial_get_drvdata(gserial);
>  
> @@ -87,7 +106,7 @@ static int ubx_probe(struct serdev_device *serdev)
>  		goto err_free_gserial;
>  	}
>  
> -	data->v_bckp = devm_regulator_get_optional(&serdev->dev, "v-bckp");
> +	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
>  	if (IS_ERR(data->v_bckp)) {
>  		ret = PTR_ERR(data->v_bckp);
>  		if (ret == -ENODEV)
> @@ -130,9 +149,10 @@ static void ubx_remove(struct serdev_device *serdev)
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id ubx_of_match[] = {
> -	{ .compatible = "u-blox,neo-6m" },
> -	{ .compatible = "u-blox,neo-8" },
> -	{ .compatible = "u-blox,neo-m8" },
> +	{ .compatible = "u-blox,neo-6m", .data = &ubx_info_ubx, },
> +	{ .compatible = "u-blox,neo-8", .data = &ubx_info_ubx, },
> +	{ .compatible = "u-blox,neo-m8", .data = &ubx_info_ubx, },
> +	{ .compatible = "globaltop,pa6h", .data = &ubx_info_mtk },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ubx_of_match);
> -- 
> 2.35.1
> 

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

Re: [RFC PATCH 1/5] WIP: gnss: merge MTK driver into UBX driver

[Johan Hovold]

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

Hi Wolfram,

and sorry about the late feedback on this. Didn't have time to process
my queue before some holiday last week.

On Mon, Jun 12, 2023 at 01:35:26PM +0200, Wolfram Sang wrote:
> On Tue, May 23, 2023 at 08:43:06AM +0200, Wolfram Sang wrote:
> > This is a proof-of-concept how easy it is to merge those two drivers as
> > they are extremly similar. The differences can be abstracted away
> > easily. Further work (renaming from 'ubx' to something more generic,
> > removing the MTK driver, ...) is postponed until there is agrement that
> > merging these drivers is actually wanted. I vote for it, though. I have
> > updates to the UBX driver which do make sense for the MTK driver as
> > well. Code saving is also a plus. We can always fork into a specific
> > driver again if we'd ever need that.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Johan, just from a gut feeling or a hi level glimpse: is this merging of
> the driver worth pursuing?

No, sorry, keeping separate drivers per vendor was a deliberate choice.

First, some of these devices support vendor specific protocols which we
may add driver support for at some point beyond currently just exporting
this information to user space (e.g. to change line speed from the
driver).

Second, device families are expected to share at least some common
properties like reset signalling and supplies, which are better kept
separate.

Johan

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

Re: [RFC PATCH 3/5] gnss: ubx: 'vcc' can be optional

[Johan Hovold]

On Tue, May 23, 2023 at 08:43:08AM +0200, Wolfram Sang wrote:
> There are devices where 'vcc' is always on. The reset pin is used for
> silencing the chip. Make the 'vcc' regulator optional.

The device still has a vcc supply even if it happens to be always-on on
a particular board (and this should be reflected in the DT).

Johan

Re: [RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Johan Hovold]

On Tue, May 23, 2023 at 08:43:09AM +0200, Wolfram Sang wrote:
> Tested with a Renesas KingFisher board. The chip correctly disappears
> from the I2C bus when the 'gnss0' device is not opened.

What do you mean by "disappears from the I2C bus"?

Does this device support both I2C and UART?

Johan

Re: [RFC PATCH 2/5] gnss: ubx: use new helper to remove open coded regulator handling

[Johan Hovold]

On Tue, May 23, 2023 at 08:43:07AM +0200, Wolfram Sang wrote:
> 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>
> ---
>  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 c01e1e875727..c8d027973b85 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;
>  };
>  
> @@ -106,30 +105,16 @@ static int ubx_probe(struct serdev_device *serdev)
>  		goto err_free_gserial;
>  	}
>  
> -	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
> -	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, info->v_bckp_name);
> +	if (ret < 0 && ret != -ENODEV)
> +		goto err_free_gserial;

I'm a bit torn about this one as I'm generally sceptical of devres and
especially helpers that enable or register resources, which just tends to
lead to subtle bugs.

But if there's one place where this new helper, which importantly does
not return a pointer to the regulator, may be useful I guess it's here.

Care to respin this one after dropping the merge patch?

Johan

Re: [RFC PATCH 1/5] WIP: gnss: merge MTK driver into UBX driver

[Wolfram Sang]

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

Hi Johan,

> and sorry about the late feedback on this. Didn't have time to process
> my queue before some holiday last week.

No worries. I hope you had some nice holiday!

> No, sorry, keeping separate drivers per vendor was a deliberate choice.

Yes, OK. I am not really surprised.

> First, some of these devices support vendor specific protocols which we
> may add driver support for at some point beyond currently just exporting
> this information to user space (e.g. to change line speed from the
> driver).

OK, I thought we can seperatae again if this really coming somewhen, but
we can also leave it like this.

> Second, device families are expected to share at least some common
> properties like reset signalling and supplies, which are better kept
> separate.

Thanks for the feedback!

   Wolfram


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

Re: [RFC PATCH 2/5] gnss: ubx: use new helper to remove open coded regulator handling

[Wolfram Sang]

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


> I'm a bit torn about this one as I'm generally sceptical of devres and
> especially helpers that enable or register resources, which just tends to
> lead to subtle bugs.

It is good to think twice with devres, but I also really like this
helper. En-/Disabling the regulator matches the life cycle of the device
itself. The boilerplate code it removes tends also to be error prone.

> Care to respin this one after dropping the merge patch?

Sure, I'll respin this series right away


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

Re: [RFC PATCH 3/5] gnss: ubx: 'vcc' can be optional

[Wolfram Sang]

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


> The device still has a vcc supply even if it happens to be always-on on
> a particular board (and this should be reflected in the DT).

I'll drop this patch and update the DT.


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

Re: [RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Wolfram Sang]

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


> > Tested with a Renesas KingFisher board. The chip correctly disappears
> > from the I2C bus when the 'gnss0' device is not opened.
> 
> What do you mean by "disappears from the I2C bus"?
> 
> Does this device support both I2C and UART?

Yes, and on my board, both are wired.


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

Re: [RFC PATCH 2/5] gnss: ubx: use new helper to remove open coded regulator handling

[Johan Hovold]

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

On Tue, Jun 20, 2023 at 11:04:27AM +0200, Wolfram Sang wrote:
> > I'm a bit torn about this one as I'm generally sceptical of devres and
> > especially helpers that enable or register resources, which just tends to
> > lead to subtle bugs.
> 
> It is good to think twice with devres, but I also really like this
> helper. En-/Disabling the regulator matches the life cycle of the device
> itself. The boilerplate code it removes tends also to be error prone.

So can the "trival" devres conversions be:

	https://lore.kernel.org/lkml/ZJFqCQ8bbBoX3l1g@hovoldconsulting.com

I meant to CC you on my reply there.

Johan

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

Re: [RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Johan Hovold]

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

On Tue, Jun 20, 2023 at 11:06:33AM +0200, Wolfram Sang wrote:
> 
> > > Tested with a Renesas KingFisher board. The chip correctly disappears
> > > from the I2C bus when the 'gnss0' device is not opened.
> > 
> > What do you mean by "disappears from the I2C bus"?
> > 
> > Does this device support both I2C and UART?
> 
> Yes, and on my board, both are wired.

Ah, yes, it was ublox device.

But can you elaborate on the "disappearing" bit? How exactly does it
"disappear" when the gnss0 device is *not* opened?

Johan

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

Re: [RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Wolfram Sang]

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


> But can you elaborate on the "disappearing" bit? How exactly does it
> "disappear" when the gnss0 device is *not* opened?

When you send an I2C message with the address of the device, it usually
ACKs it to say "Hi, I am here". When the ublox device is in reset state,
it does not ACK its address. So, tools like "i2cdetect" will not report
the device as "someone is listening there".

Here is the i2cdetect output with the GNSS device open:

# i2cdetect -y -r 15
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                         -- -- -- -- -- -- -- -- 
10: 10 -- -- -- -- -- -- -- -- -- -- -- -- 1d -- -- 
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- 42 -- -- -- -- -- -- -- -- -- -- -- -- UU 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- -- -- 6b -- -- -- -- 
70: -- UU -- -- UU UU -- --                         

And with the device closed. Note address 0x42 disappearing:

# i2cdetect -y -r 15
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                         -- -- -- -- -- -- -- -- 
10: 10 -- -- -- -- -- -- -- -- -- -- -- -- 1d -- -- 
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- UU 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- -- -- 6b -- -- -- -- 
70: -- UU -- -- UU UU -- --                         


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

Re: [RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Johan Hovold]

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

On Tue, Jun 20, 2023 at 11:41:39AM +0200, Wolfram Sang wrote:
> 
> > But can you elaborate on the "disappearing" bit? How exactly does it
> > "disappear" when the gnss0 device is *not* opened?
> 
> When you send an I2C message with the address of the device, it usually
> ACKs it to say "Hi, I am here". When the ublox device is in reset state,
> it does not ACK its address. So, tools like "i2cdetect" will not report
> the device as "someone is listening there".

Ah, ok, so you used it to verify that reset is working as intended.

Rereading the commit message now it makes more sense, I guess I was
thrown off by the reference to I2C. Perhaps adding a sentence or two
about the alternate interface and using it as a means of verification
would make things a bit more clear.

Johan

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

Re: [RFC PATCH 4/5] gnss: ubx: add support for the reset gpio

[Wolfram Sang]

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


> Rereading the commit message now it makes more sense, I guess I was
> thrown off by the reference to I2C. Perhaps adding a sentence or two
> about the alternate interface and using it as a means of verification
> would make things a bit more clear.

Will do!


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

Re: [RFC PATCH 1/5] WIP: gnss: merge MTK driver into UBX driver

[Wolfram Sang]

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


> First, some of these devices support vendor specific protocols which we
> may add driver support for at some point beyond currently just exporting
> this information to user space (e.g. to change line speed from the
> driver).

Speaking of proprietary protocols: did you ever get in contact with the
gpsd folks? I stumbled over this mail from February this year [1] where
nobody knew where this /dev/gnss0 device came from. I'd think we should
be in contact with them if we want to support e.g. line speed changes?

[1] https://lists.gnu.org/archive/html/gpsd-dev/2023-02/msg00017.html


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

Re: [RFC PATCH 1/5] WIP: gnss: merge MTK driver into UBX driver

[Johan Hovold]

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

On Tue, Jun 20, 2023 at 12:21:40PM +0200, Wolfram Sang wrote:

> Speaking of proprietary protocols: did you ever get in contact with the
> gpsd folks? I stumbled over this mail from February this year [1] where
> nobody knew where this /dev/gnss0 device came from. I'd think we should
> be in contact with them if we want to support e.g. line speed changes?

No, I haven't and I have very little motivation for spending time on
this. With the features we have today gpsd works as-is (last time I
checked).

Line-speed configuration would likely also be handled transparently by
the driver without user space interaction, but there is also room for
extending the interface should need arise.

Johan

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