[v3,3/3] gnss: ubx: add support for the reset gpio
Commit Message
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(+)
Comments
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
> 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.
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
@@ -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;