| Message ID | 1550070569-18588-5-git-send-email-lollivier@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/4] dt-bindings: Add vendor prefix for "GlobalTop Technology, Inc." | expand |
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
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
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
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
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");
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