[PATCH v4 0/4] Add driver for Mediatek-based GNSS receivers

[PATCH v4 0/4] Add driver for Mediatek-based GNSS receivers

[Loys Ollivier]

Hi,

This patch series adds a new GNSS driver for the Mediatek-based GNSS receivers.
These receivers transmits NMEA output sequence after boot.
Power management can be done via the main supply and optional backup supply
as defined in the device tree.

The driver has been tested using a GlobalTop pa6h chipset on a Libretech-cc
board using the expansion header. Changes made in the board device tree can
be found below for reference and testing.

Loys

Version 4 changes:
- dt: bindings: use standard name "reset-gpios"
- dt: bindings: use standard name "gnss-fix-gpios"
- dt: bindings: s/an/a UART

Version 3 changes [0]:
- driver: Removed "mediatek,mt3339" compatible
- driver: moved gnss_types_names string diff in the GNSS_TYPE patch
- driver: edited [patch 3/4] commit message to remove ref to vendor protocol
- driver: retained the original copyright
- dt: removed interfaces that are not available on the globaltop SoC
- dt: Added missing pins documentation
- dt: added optional properties pin names

Version 2 changes:
- driver: Renamed driver from Globaltop/gtop to Mediatek/mtk
- driver: Added "mediatek,mt3339" compatible
- dt: Renamed bindings from Globaltop to Mediatek
- dt: Moved the current-speed property as optional
- dt: removed the status line in example
- cover-letter: removed the alias that is not needed anymore

[0]: https://lkml.kernel.org/r/1548693481-1162-1-git-send-email-lollivier@baylibre.com

Loys Ollivier (4):
  dt-bindings: Add vendor prefix for "GlobalTop Technology, Inc."
  dt-bindings: gnss: add mediatek binding
  gnss: add mtk receiver type support
  gnss: add driver for mediatek receivers

 .../devicetree/bindings/gnss/mediatek.txt          |  37 +++++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/gnss/Kconfig                               |  13 ++
 drivers/gnss/Makefile                              |   3 +
 drivers/gnss/core.c                                |   1 +
 drivers/gnss/mtk.c                                 | 152 +++++++++++++++++++++
 include/linux/gnss.h                               |   1 +
 7 files changed, 208 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/mediatek.txt
 create mode 100644 drivers/gnss/mtk.c

---
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts
index 90a56af967a7..3b3d4dcc47aa 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts
@@ -17,6 +17,7 @@
 
        aliases {
                serial0 = &uart_AO;
                ethernet0 = &ethmac;
        };
 
@@ -269,7 +270,20 @@
        pinctrl-names = "default";
 };
 
+/* This is brought out on the UART_A_TX (8) and UART_A_RX (10) pins: */
+&uart_A {
+       status = "okay";
+       pinctrl-0 = <&uart_a_pins>;
+       pinctrl-names = "default";
+
+       gnss {
+               compatible = "globaltop,pa6h";
+               v-bckp-supply = <&vcc_3v3>;
+               vcc-supply = <&vcc_3v3>;
+               current-speed = <9600>;
+       };
+};
+
 &usb0 {
        status = "okay";
 };

-- 
2.7.4

[PATCH v4 1/4] dt-bindings: Add vendor prefix for "GlobalTop Technology, Inc."

[Loys Ollivier]

Add globaltop vendor definition.

Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v4: No changes (Reviewed-by tag added)

v3: No changes

v2: Alphabetical order

 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 389508584f48..d80a70343b36 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -150,6 +150,7 @@ geniatech	Geniatech, Inc.
 giantec	Giantec Semiconductor, Inc.
 giantplus	Giantplus Technology Co., Ltd.
 globalscale	Globalscale Technologies, Inc.
+globaltop	GlobalTop Technology, Inc.
 gmt	Global Mixed-mode Technology, Inc.
 goodix	Shenzhen Huiding Technology Co., Ltd.
 google	Google, Inc.
-- 
2.7.4

[PATCH v4 2/4] dt-bindings: gnss: add mediatek binding

[Loys Ollivier]

Add binding for Mediatek-based GNSS receivers.

Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
---
v4:
Use standard names for reset-gpios and gnss-fix-gpios
s/an/a UART

v3:
Removed the I2C and SPI interfaces references.
Removed the "mediatek,mt3339" compatible line.
Added the optional propertied (reset, fix)
Added the pin names for the optional properties.

v2:
Renamed bindings from Globaltop/gtop to Mediatek/mtk.
Moved current-speed as an optional propertie.
Removed the status line in the example.
Added "mediatek,mt3339" compatible.

 .../devicetree/bindings/gnss/mediatek.txt          | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/mediatek.txt

diff --git a/Documentation/devicetree/bindings/gnss/mediatek.txt b/Documentation/devicetree/bindings/gnss/mediatek.txt
new file mode 100644
index 000000000000..12283d429baa
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/mediatek.txt
@@ -0,0 +1,37 @@
+Mediatek-based GNSS Receiver DT binding
+
+Mediatek chipsets are used in GNSS-receiver modules produced by several
+vendors and can use a UART interface.
+
+Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
+properties.
+
+Required properties:
+
+- compatible	: Must be
+
+			"globaltop,pa6h"
+
+- vcc-supply	: Main voltage regulator (pin name: VCC)
+
+Optional properties:
+
+- current-speed		: Default UART baud rate
+- gnss-fix-gpios	: GPIO used to determine device position fix state
+			  (pin name: FIX, 3D_FIX)
+- reset-gpios		: GPIO used to reset the device
+  		  	  (pin name: RESET, NRESET)
+- timepulse-gpios	: Time pulse GPIO (pin name: PPS1, 1PPS)
+- v-bckp-supply		: Backup voltage regulator
+  			  (pin name: VBAT, VBACKUP)
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "globaltop,pa6h";
+		vcc-supply = <&vcc_3v3>;
+	};
+};
-- 
2.7.4

[PATCH v4 3/4] gnss: add mtk receiver type support

[Loys Ollivier]

Add an MTK (Mediatek) type to the "GNSS_TYPE" attribute.

Note that MTK receivers support a subset of NMEA 0183 with vendor
extensions.

Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
---
v4:
No changes

v3:
Moved the gnss_type_names string addition to this patch.
Edited the commit message to remove the reference to a vendor protocol.

v2:
Renamed from GTOP to MTK.

 drivers/gnss/core.c  | 1 +
 include/linux/gnss.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
index 4291a0dd22aa..320cfca80d5f 100644
--- a/drivers/gnss/core.c
+++ b/drivers/gnss/core.c
@@ -334,6 +334,7 @@ static const char * const gnss_type_names[GNSS_TYPE_COUNT] = {
 	[GNSS_TYPE_NMEA]	= "NMEA",
 	[GNSS_TYPE_SIRF]	= "SiRF",
 	[GNSS_TYPE_UBX]		= "UBX",
+	[GNSS_TYPE_MTK]		= "MTK",
 };
 
 static const char *gnss_type_name(struct gnss_device *gdev)
diff --git a/include/linux/gnss.h b/include/linux/gnss.h
index 43546977098c..36968a0f33e8 100644
--- a/include/linux/gnss.h
+++ b/include/linux/gnss.h
@@ -22,6 +22,7 @@ enum gnss_type {
 	GNSS_TYPE_NMEA = 0,
 	GNSS_TYPE_SIRF,
 	GNSS_TYPE_UBX,
+	GNSS_TYPE_MTK,
 
 	GNSS_TYPE_COUNT
 };
-- 
2.7.4

[PATCH v4 4/4] gnss: add driver for mediatek receivers

[Loys Ollivier]

Add driver for serial-connected Mediatek-based GNSS receivers.

These devices typically boot transmitting vendor specific NMEA output
sequences. The serial port bit rate is read from the device tree
"current-speed".

Note that the driver uses the generic GNSS serial implementation and
therefore essentially only manages power abstracted into three power
states: ACTIVE, STANDBY, and OFF.

For mediatek receivers with a main supply and no enable-gpios, this simply
means that the main supply is disabled in STANDBY and OFF (the optional
backup supply is kept enabled while the driver is bound).

Note that the timepulse-support is left unimplemented.

Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
---
v4:
No changes

v3:
Sorted the Kconfig and Makefile
Retained the original Copyright
Removed "mediatek,mt3339" compatible.

v2: 
Renamed from gtop/Globaltop to mtk/Mediatek.
Added "mediatek,mt3339" compatible.

 drivers/gnss/Kconfig  |  13 +++++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/mtk.c    | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 drivers/gnss/mtk.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index 6abc88514512..6d8c8027e1cd 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -15,6 +15,19 @@ if GNSS
 config GNSS_SERIAL
 	tristate
 
+config GNSS_MTK_SERIAL
+	tristate "Mediatek GNSS receiver support"
+	depends on SERIAL_DEV_BUS
+	select GNSS_SERIAL
+	help
+	  Say Y here if you have a Mediatek-based GNSS receiver which uses a
+	  serial interface.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss-mtk.
+
+	  If unsure, say N.
+
 config GNSS_SIRF_SERIAL
 	tristate "SiRFstar GNSS receiver support"
 	depends on SERIAL_DEV_BUS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index 5cf0ebe0330a..451f11401ecc 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -9,6 +9,9 @@ gnss-y := core.o
 obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
 gnss-serial-y := serial.o
 
+obj-$(CONFIG_GNSS_MTK_SERIAL)		+= gnss-mtk.o
+gnss-mtk-y := mtk.o
+
 obj-$(CONFIG_GNSS_SIRF_SERIAL)		+= gnss-sirf.o
 gnss-sirf-y := sirf.o
 
diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c
new file mode 100644
index 000000000000..a5aad08fb9d7
--- /dev/null
+++ b/drivers/gnss/mtk.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Mediatek GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+
+#include "serial.h"
+
+struct mtk_data {
+	struct regulator *v_bckp;
+	struct regulator *vcc;
+};
+
+static int mtk_set_active(struct gnss_serial *gserial)
+{
+	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
+	int ret;
+
+	ret = regulator_enable(data->vcc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mtk_set_standby(struct gnss_serial *gserial)
+{
+	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
+	int ret;
+
+	ret = regulator_disable(data->vcc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mtk_set_power(struct gnss_serial *gserial,
+			 enum gnss_serial_pm_state state)
+{
+	switch (state) {
+	case GNSS_SERIAL_ACTIVE:
+		return mtk_set_active(gserial);
+	case GNSS_SERIAL_OFF:
+	case GNSS_SERIAL_STANDBY:
+		return mtk_set_standby(gserial);
+	}
+
+	return -EINVAL;
+}
+
+static const struct gnss_serial_ops mtk_gserial_ops = {
+	.set_power = mtk_set_power,
+};
+
+static int mtk_probe(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial;
+	struct mtk_data *data;
+	int ret;
+
+	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	if (IS_ERR(gserial)) {
+		ret = PTR_ERR(gserial);
+		return ret;
+	}
+
+	gserial->ops = &mtk_gserial_ops;
+
+	gserial->gdev->type = GNSS_TYPE_MTK;
+
+	data = gnss_serial_get_drvdata(gserial);
+
+	data->vcc = devm_regulator_get(&serdev->dev, "vcc");
+	if (IS_ERR(data->vcc)) {
+		ret = PTR_ERR(data->vcc);
+		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 = gnss_serial_register(gserial);
+	if (ret)
+		goto err_disable_v_bckp;
+
+	return 0;
+
+err_disable_v_bckp:
+	if (data->v_bckp)
+		regulator_disable(data->v_bckp);
+err_free_gserial:
+	gnss_serial_free(gserial);
+
+	return ret;
+}
+
+static void mtk_remove(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
+
+	gnss_serial_deregister(gserial);
+	if (data->v_bckp)
+		regulator_disable(data->v_bckp);
+	gnss_serial_free(gserial);
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id mtk_of_match[] = {
+	{ .compatible = "globaltop,pa6h" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_of_match);
+#endif
+
+static struct serdev_device_driver mtk_driver = {
+	.driver	= {
+		.name		= "gnss-mtk",
+		.of_match_table	= of_match_ptr(mtk_of_match),
+		.pm		= &gnss_serial_pm_ops,
+	},
+	.probe	= mtk_probe,
+	.remove	= mtk_remove,
+};
+module_serdev_device_driver(mtk_driver);
+
+MODULE_AUTHOR("Loys Ollivier <lollivier@baylibre.com>");
+MODULE_DESCRIPTION("Mediatek GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v4 2/4] dt-bindings: gnss: add mediatek binding

[Rob Herring]

On Wed, Feb 13, 2019 at 04:09:27PM +0100, Loys Ollivier wrote:
> Add binding for Mediatek-based GNSS receivers.
> 
> Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
> ---
> v4:
> Use standard names for reset-gpios and gnss-fix-gpios
> s/an/a UART
> 
> v3:
> Removed the I2C and SPI interfaces references.
> Removed the "mediatek,mt3339" compatible line.
> Added the optional propertied (reset, fix)
> Added the pin names for the optional properties.
> 
> v2:
> Renamed bindings from Globaltop/gtop to Mediatek/mtk.
> Moved current-speed as an optional propertie.
> Removed the status line in the example.
> Added "mediatek,mt3339" compatible.
> 
>  .../devicetree/bindings/gnss/mediatek.txt          | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/mediatek.txt

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

Re: [PATCH v4 4/4] gnss: add driver for mediatek receivers

[Corentin Labbe]

On Wed, Feb 13, 2019 at 04:09:29PM +0100, Loys Ollivier wrote:
> Add driver for serial-connected Mediatek-based GNSS receivers.
> 
> These devices typically boot transmitting vendor specific NMEA output
> sequences. The serial port bit rate is read from the device tree
> "current-speed".
> 
> Note that the driver uses the generic GNSS serial implementation and
> therefore essentially only manages power abstracted into three power
> states: ACTIVE, STANDBY, and OFF.
> 
> For mediatek receivers with a main supply and no enable-gpios, this simply
> means that the main supply is disabled in STANDBY and OFF (the optional
> backup supply is kept enabled while the driver is bound).
> 
> Note that the timepulse-support is left unimplemented.
> 
> Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
> ---
> v4:
> No changes
> 
> v3:
> Sorted the Kconfig and Makefile
> Retained the original Copyright
> Removed "mediatek,mt3339" compatible.
> 
> v2: 
> Renamed from gtop/Globaltop to mtk/Mediatek.
> Added "mediatek,mt3339" compatible.
> 
>  drivers/gnss/Kconfig  |  13 +++++
>  drivers/gnss/Makefile |   3 +
>  drivers/gnss/mtk.c    | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 168 insertions(+)
>  create mode 100644 drivers/gnss/mtk.c
> 
> diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
> index 6abc88514512..6d8c8027e1cd 100644
> --- a/drivers/gnss/Kconfig
> +++ b/drivers/gnss/Kconfig
> @@ -15,6 +15,19 @@ if GNSS
>  config GNSS_SERIAL
>  	tristate
>  
> +config GNSS_MTK_SERIAL
> +	tristate "Mediatek GNSS receiver support"
> +	depends on SERIAL_DEV_BUS
> +	select GNSS_SERIAL
> +	help
> +	  Say Y here if you have a Mediatek-based GNSS receiver which uses a
> +	  serial interface.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called gnss-mtk.
> +
> +	  If unsure, say N.
> +
>  config GNSS_SIRF_SERIAL
>  	tristate "SiRFstar GNSS receiver support"
>  	depends on SERIAL_DEV_BUS
> diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
> index 5cf0ebe0330a..451f11401ecc 100644
> --- a/drivers/gnss/Makefile
> +++ b/drivers/gnss/Makefile
> @@ -9,6 +9,9 @@ gnss-y := core.o
>  obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
>  gnss-serial-y := serial.o
>  
> +obj-$(CONFIG_GNSS_MTK_SERIAL)		+= gnss-mtk.o
> +gnss-mtk-y := mtk.o
> +
>  obj-$(CONFIG_GNSS_SIRF_SERIAL)		+= gnss-sirf.o
>  gnss-sirf-y := sirf.o
>  
> diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c
> new file mode 100644
> index 000000000000..a5aad08fb9d7
> --- /dev/null
> +++ b/drivers/gnss/mtk.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Mediatek GNSS receiver driver
> + *
> + * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/gnss.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/serdev.h>
> +
> +#include "serial.h"
> +
> +struct mtk_data {
> +	struct regulator *v_bckp;
> +	struct regulator *vcc;
> +};
> +
> +static int mtk_set_active(struct gnss_serial *gserial)
> +{
> +	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
> +	int ret;
> +
> +	ret = regulator_enable(data->vcc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

Hello

This could be simplified to return regulator_enable(data->vcc);
Furthermore, after this simplification, the function seems useless.

Same comment for mtk_set_standby()

Regards

Re: [PATCH v4 4/4] gnss: add driver for mediatek receivers

[Johan Hovold]

On Thu, Feb 14, 2019 at 11:00:19AM +0100, Corentin Labbe wrote:
> On Wed, Feb 13, 2019 at 04:09:29PM +0100, Loys Ollivier wrote:
> > Add driver for serial-connected Mediatek-based GNSS receivers.
> > 
> > These devices typically boot transmitting vendor specific NMEA output
> > sequences. The serial port bit rate is read from the device tree
> > "current-speed".
> > 
> > Note that the driver uses the generic GNSS serial implementation and
> > therefore essentially only manages power abstracted into three power
> > states: ACTIVE, STANDBY, and OFF.
> > 
> > For mediatek receivers with a main supply and no enable-gpios, this simply
> > means that the main supply is disabled in STANDBY and OFF (the optional
> > backup supply is kept enabled while the driver is bound).
> > 
> > Note that the timepulse-support is left unimplemented.
> > 
> > Signed-off-by: Loys Ollivier <lollivier@baylibre.com>

> > +static int mtk_set_active(struct gnss_serial *gserial)
> > +{
> > +	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
> > +	int ret;
> > +
> > +	ret = regulator_enable(data->vcc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> 
> Hello
> 
> This could be simplified to return regulator_enable(data->vcc);

Indeed, but I prefer this style which clearly separates the error path
from the success path while making the success return value explicit.

> Furthermore, after this simplification, the function seems useless.

Why do you think so? You still need to retrieve the regulator from the
driver data. Sure, this could be folded into mtk_set_power(), but that
would be less ideal if there are more resources that need to be managed
(e.g. an external lna supply).

Thanks,
Johan

Re: [PATCH v4 4/4] gnss: add driver for mediatek receivers

[Loys Ollivier]

On Thu 14 Feb 2019 at 10:12, Johan Hovold <johan@kernel.org> wrote:

> On Thu, Feb 14, 2019 at 11:00:19AM +0100, Corentin Labbe wrote:
>> On Wed, Feb 13, 2019 at 04:09:29PM +0100, Loys Ollivier wrote:
>> > Add driver for serial-connected Mediatek-based GNSS receivers.
>> > 
>> > These devices typically boot transmitting vendor specific NMEA output
>> > sequences. The serial port bit rate is read from the device tree
>> > "current-speed".
>> > 
>> > Note that the driver uses the generic GNSS serial implementation and
>> > therefore essentially only manages power abstracted into three power
>> > states: ACTIVE, STANDBY, and OFF.
>> > 
>> > For mediatek receivers with a main supply and no enable-gpios, this simply
>> > means that the main supply is disabled in STANDBY and OFF (the optional
>> > backup supply is kept enabled while the driver is bound).
>> > 
>> > Note that the timepulse-support is left unimplemented.
>> > 
>> > Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
>
>> > +static int mtk_set_active(struct gnss_serial *gserial)
>> > +{
>> > +	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
>> > +	int ret;
>> > +
>> > +	ret = regulator_enable(data->vcc);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	return 0;
>> > +}
>> 
>> Hello
>> 
>> This could be simplified to return regulator_enable(data->vcc);
>
> Indeed, but I prefer this style which clearly separates the error path
> from the success path while making the success return value explicit.
>

It respects the coding style/standard and I prefer the original version.
It's also easier to debug if you want to add extra code on error.

>> Furthermore, after this simplification, the function seems useless.
>
> Why do you think so? You still need to retrieve the regulator from the
> driver data. Sure, this could be folded into mtk_set_power(), but that
> would be less ideal if there are more resources that need to be managed
> (e.g. an external lna supply).

I would be in favor to keep mtk_set_active. It clearly shows when the
regulator is enabled.
It also seems logic to have an enable/standby function in the driver.
Even if those are trivial (for now).

>
> Thanks,
> Johan


-- 
-L

Re: [PATCH v4 2/4] dt-bindings: gnss: add mediatek binding

[Johan Hovold]

On Wed, Feb 13, 2019 at 04:09:27PM +0100, Loys Ollivier wrote:
> Add binding for Mediatek-based GNSS receivers.
> 
> Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
> ---
> v4:
> Use standard names for reset-gpios and gnss-fix-gpios
> s/an/a UART
> 
> v3:
> Removed the I2C and SPI interfaces references.
> Removed the "mediatek,mt3339" compatible line.
> Added the optional propertied (reset, fix)
> Added the pin names for the optional properties.
> 
> v2:
> Renamed bindings from Globaltop/gtop to Mediatek/mtk.
> Moved current-speed as an optional propertie.
> Removed the status line in the example.
> Added "mediatek,mt3339" compatible.
> 
>  .../devicetree/bindings/gnss/mediatek.txt          | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/mediatek.txt
> 
> diff --git a/Documentation/devicetree/bindings/gnss/mediatek.txt b/Documentation/devicetree/bindings/gnss/mediatek.txt
> new file mode 100644
> index 000000000000..12283d429baa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gnss/mediatek.txt
> @@ -0,0 +1,37 @@
> +Mediatek-based GNSS Receiver DT binding
> +
> +Mediatek chipsets are used in GNSS-receiver modules produced by several
> +vendors and can use a UART interface.
> +
> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> +properties.
> +
> +Required properties:
> +
> +- compatible	: Must be
> +
> +			"globaltop,pa6h"
> +
> +- vcc-supply	: Main voltage regulator (pin name: VCC)
> +
> +Optional properties:
> +
> +- current-speed		: Default UART baud rate
> +- gnss-fix-gpios	: GPIO used to determine device position fix state
> +			  (pin name: FIX, 3D_FIX)
> +- reset-gpios		: GPIO used to reset the device
> +  		  	  (pin name: RESET, NRESET)
> +- timepulse-gpios	: Time pulse GPIO (pin name: PPS1, 1PPS)
> +- v-bckp-supply		: Backup voltage regulator
> +  			  (pin name: VBAT, VBACKUP)

As we discussed, I changed this to "vbackup-supply" (here and in the
driver) before applying.

Note that git reported some whitespace errors above too (spaces before
tabs) that I fixed up. Not sure why checkpatch doesn't complain.

All now applied.

Thanks,
Johan

Re: [PATCH v4 2/4] dt-bindings: gnss: add mediatek binding

[Loys Ollivier]

On Fri 15 Feb 2019 at 16:04, Johan Hovold <johan@kernel.org> wrote:

> On Wed, Feb 13, 2019 at 04:09:27PM +0100, Loys Ollivier wrote:
>> Add binding for Mediatek-based GNSS receivers.
>> 
>> Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
>> ---
>> v4:
>> Use standard names for reset-gpios and gnss-fix-gpios
>> s/an/a UART
>> 
>> v3:
>> Removed the I2C and SPI interfaces references.
>> Removed the "mediatek,mt3339" compatible line.
>> Added the optional propertied (reset, fix)
>> Added the pin names for the optional properties.
>> 
>> v2:
>> Renamed bindings from Globaltop/gtop to Mediatek/mtk.
>> Moved current-speed as an optional propertie.
>> Removed the status line in the example.
>> Added "mediatek,mt3339" compatible.
>> 
>>  .../devicetree/bindings/gnss/mediatek.txt          | 37 ++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gnss/mediatek.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/gnss/mediatek.txt b/Documentation/devicetree/bindings/gnss/mediatek.txt
>> new file mode 100644
>> index 000000000000..12283d429baa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gnss/mediatek.txt
>> @@ -0,0 +1,37 @@
>> +Mediatek-based GNSS Receiver DT binding
>> +
>> +Mediatek chipsets are used in GNSS-receiver modules produced by several
>> +vendors and can use a UART interface.
>> +
>> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> +properties.
>> +
>> +Required properties:
>> +
>> +- compatible	: Must be
>> +
>> +			"globaltop,pa6h"
>> +
>> +- vcc-supply	: Main voltage regulator (pin name: VCC)
>> +
>> +Optional properties:
>> +
>> +- current-speed		: Default UART baud rate
>> +- gnss-fix-gpios	: GPIO used to determine device position fix state
>> +			  (pin name: FIX, 3D_FIX)
>> +- reset-gpios		: GPIO used to reset the device
>> +  		  	  (pin name: RESET, NRESET)
>> +- timepulse-gpios	: Time pulse GPIO (pin name: PPS1, 1PPS)
>> +- v-bckp-supply		: Backup voltage regulator
>> +  			  (pin name: VBAT, VBACKUP)
>
> As we discussed, I changed this to "vbackup-supply" (here and in the
> driver) before applying.
>
> Note that git reported some whitespace errors above too (spaces before
> tabs) that I fixed up. Not sure why checkpatch doesn't complain.
>
Oops, thanks for fixing it. I ran checkpatch again and it did not
complain. Apprently it's not checking for spaces before tabs in "txt"
files.
I'll add a step in my flow to apply the patches I create before sending
them to the list so that won't happen again ;)

> All now applied.
>
Yay ! Thanks !
Loys


> Thanks,
> Johan


-- 
-L

Re: [PATCH v4 4/4] gnss: add driver for mediatek receivers

[Pavel Machek]

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

On Thu 2019-02-14 11:12:12, Johan Hovold wrote:
> On Thu, Feb 14, 2019 at 11:00:19AM +0100, Corentin Labbe wrote:
> > On Wed, Feb 13, 2019 at 04:09:29PM +0100, Loys Ollivier wrote:
> > > Add driver for serial-connected Mediatek-based GNSS receivers.
> > > 
> > > These devices typically boot transmitting vendor specific NMEA output
> > > sequences. The serial port bit rate is read from the device tree
> > > "current-speed".
> > > 
> > > Note that the driver uses the generic GNSS serial implementation and
> > > therefore essentially only manages power abstracted into three power
> > > states: ACTIVE, STANDBY, and OFF.
> > > 
> > > For mediatek receivers with a main supply and no enable-gpios, this simply
> > > means that the main supply is disabled in STANDBY and OFF (the optional
> > > backup supply is kept enabled while the driver is bound).
> > > 
> > > Note that the timepulse-support is left unimplemented.
> > > 
> > > Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
> 
> > > +static int mtk_set_active(struct gnss_serial *gserial)
> > > +{
> > > +	struct mtk_data *data = gnss_serial_get_drvdata(gserial);
> > > +	int ret;
> > > +
> > > +	ret = regulator_enable(data->vcc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Hello
> > 
> > This could be simplified to return regulator_enable(data->vcc);
> 
> Indeed, but I prefer this style which clearly separates the error path
> from the success path while making the success return value explicit.

What is clear about useless code? That function can be two lines, this
only leads people to wonder "what is going on here?".
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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