Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752500AbbF3Ifw (ORCPT ); Tue, 30 Jun 2015 04:35:52 -0400 Received: from mo4-p00-ob.smtp.rzone.de ([81.169.146.162]:34119 "EHLO mo4-p00-ob.smtp.rzone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752612AbbF3Ife convert rfc822-to-8bit (ORCPT ); Tue, 30 Jun 2015 04:35:34 -0400 X-Greylist: delayed 57058 seconds by postgrey-1.27 at vger.kernel.org; Tue, 30 Jun 2015 04:35:33 EDT X-RZG-AUTH: :JGIXVUS7cutRB/49FwqZ7WcKdUCnXG6JabOfSXKWrat9h9PsyeGM X-RZG-CLASS-ID: mo00 Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\)) Subject: Re: [PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver From: "Dr. H. Nikolaus Schaller" In-Reply-To: <20150630082354.GD17917@localhost.localdomain> Date: Tue, 30 Jun 2015 10:35:21 +0200 Cc: Marek Belisko , jslaby@suse.cz, gregkh@linuxfoundation.org, arnd@arndb.de, peter@hurleysoftware.com, mark.rutland@arm.com, gnomes@lxorguk.ukuu.org.uk, sre@kernel.org, neil@brown.name, grant.likely@linaro.org, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, robh+dt@kernel.org, pawel.moll@arm.com Content-Transfer-Encoding: 8BIT Message-Id: <91726F4B-ADE4-455E-9283-A0AE17C40D77@goldelico.com> References: <1435520786-31867-1-git-send-email-marek@goldelico.com> <1435520786-31867-4-git-send-email-marek@goldelico.com> <20150630082354.GD17917@localhost.localdomain> To: Sergei Zviagintsev X-Mailer: Apple Mail (2.1878.6) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 19007 Lines: 629 Hi, thanks for the valid and very valuable comments! We will integrate them into RFC v3. BR, Nikolaus Am 30.06.2015 um 10:23 schrieb Sergei Zviagintsev : > Hi, > > I left some comments below. > > On Sun, Jun 28, 2015 at 09:46:26PM +0200, Marek Belisko wrote: >> From: "H. Nikolaus Schaller" >> >> Add driver for Wi2Wi w2g004 GPS module connected through uart. Use uart >> slave + notification hooks to glue with tty. >> >> Signed-off-by: H. Nikolaus Schaller >> Signed-off-by: Marek Belisko >> --- >> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt | 18 + >> drivers/misc/Kconfig | 18 + >> drivers/misc/Makefile | 1 + >> drivers/misc/w2sg0004.c | 436 +++++++++++++++++++++ >> include/linux/w2sg0004.h | 28 ++ >> 5 files changed, 501 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt >> create mode 100644 drivers/misc/w2sg0004.c >> create mode 100644 include/linux/w2sg0004.h >> >> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt >> new file mode 100644 >> index 0000000..ef0d6d5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt >> @@ -0,0 +1,18 @@ >> +Wi2Wi GPS module connected through UART >> + >> +Required properties: >> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084 >> +- on-off-gpio: the GPIO that controls the module's on-off toggle input >> +- uart: the uart we are connected to (provides DTR for power control) >> + >> +Optional properties: >> +- lna-suppy: an (optional) LNA regulator that is enabled together with the GPS receiver >> + >> +example: >> + >> + gps_receiver: w2sg0004 { >> + compatible = "wi2wi,w2sg0004"; >> + lna-supply = <&vsim>; /* LNA regulator */ >> + on-off-gpio = <&gpio5 17 0>; /* GPIO_145: trigger for turning on/off w2sg0004 */ >> + uart = <&uart1>; /* we are a slave of uart1 */ >> + } >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig >> index 42c3852..952add4 100644 >> --- a/drivers/misc/Kconfig >> +++ b/drivers/misc/Kconfig >> @@ -527,4 +527,22 @@ source "drivers/misc/mic/Kconfig" >> source "drivers/misc/genwqe/Kconfig" >> source "drivers/misc/echo/Kconfig" >> source "drivers/misc/cxl/Kconfig" >> + >> +menu "GTA04 misc hardware support" >> + >> +config W2SG0004 >> + tristate "W2SG0004 on/off control" >> + depends on GPIOLIB >> + help >> + Enable on/off control of W2SG0004 GPS using a tty slave to >> + to allow powering up if the /dev/tty$n is opened. >> + It also provides a rfkill gps node to control the LNA power. >> + >> +config W2SG0004_DEBUG >> + bool "W2SG0004 on/off debugging" >> + depends on W2SG0004 >> + help >> + Enable driver debugging mode of W2SG0004 GPS. >> + >> +endmenu >> endmenu >> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile >> index d056fb7..3bc67c7 100644 >> --- a/drivers/misc/Makefile >> +++ b/drivers/misc/Makefile >> @@ -53,5 +53,6 @@ obj-$(CONFIG_SRAM) += sram.o >> obj-y += mic/ >> obj-$(CONFIG_GENWQE) += genwqe/ >> obj-$(CONFIG_ECHO) += echo/ >> +obj-$(CONFIG_W2SG0004) += w2sg0004.o >> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o >> obj-$(CONFIG_CXL_BASE) += cxl/ >> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c >> new file mode 100644 >> index 0000000..c5f0f7b >> --- /dev/null >> +++ b/drivers/misc/w2sg0004.c >> @@ -0,0 +1,436 @@ >> +/* >> + * w2sg0004.c >> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver. >> + * >> + * This receiver has an ON/OFF pin which must be toggled to >> + * turn the device 'on' of 'off'. A high->low->high toggle >> + * will switch the device on if it is off, and off if it is on. >> + * >> + * To enable receiving on/off requests we register with the >> + * UART power management notifications. >> + * >> + * It is not possible to directly detect the state of the device. >> + * However when it is on it will send characters on a UART line >> + * regularly. >> + * >> + * To detect that the power state is out of sync (e.g. if GPS >> + * was enabled before a reboot), we register for UART data received >> + * notifications. >> + * >> + * In addition we register as a rfkill client so that we can >> + * control the LNA power. >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#ifdef CONFIG_W2SG0004_DEBUG >> +#undef pr_debug >> +#define pr_debug printk >> +#endif >> + >> +/* >> + * There seems to be restrictions on how quickly we can toggle the >> + * on/off line. data sheets says "two rtc ticks", whatever that means. >> + * If we do it too soon it doesn't work. >> + * So we have a state machine which uses the common work queue to ensure >> + * clean transitions. >> + * When a change is requested we record that request and only act on it >> + * once the previous change has completed. >> + * A change involves a 10ms low pulse, and a 990ms raised level, so only >> + * one change per second. >> + */ >> + >> +enum w2sg_state { >> + W2SG_IDLE, /* is not changing state */ >> + W2SG_PULSE, /* activate on/off impulse */ >> + W2SG_NOPULSE /* deactivate on/off impulse */ >> +}; >> + >> +struct w2sg_data { >> + struct rfkill *rf_kill; >> + struct regulator *lna_regulator; >> + int lna_blocked; /* rfkill block gps active */ >> + int lna_is_off; /* LNA is currently off */ >> + int is_on; /* current state (0/1) */ > > Wouldn't bool types be better for these three and also for `requested' > and `suspended'? > >> + unsigned long last_toggle; >> + unsigned long backoff; /* time to wait since last_toggle */ >> + int on_off_gpio; /* the on-off gpio number */ >> + struct uart_port *uart; /* the drvdata of the uart or tty */ >> + enum w2sg_state state; >> + int requested; /* requested state (0/1) */ >> + int suspended; >> + spinlock_t lock; >> + struct delayed_work work; >> +}; >> + >> +static int w2sg_data_set_lna_power(struct w2sg_data *data) >> +{ >> + int ret = 0; >> + int off = data->suspended || !data->requested || data->lna_blocked; >> + >> + pr_debug("%s: %s\n", __func__, off ? "off" : "on"); >> + >> + if (off != data->lna_is_off) { >> + data->lna_is_off = off; >> + if (!IS_ERR_OR_NULL(data->lna_regulator)) { >> + if (off) >> + regulator_disable(data->lna_regulator); >> + else >> + ret = regulator_enable(data->lna_regulator); >> + } >> + } >> + >> + return ret; >> +} >> + >> +static void w2sg_data_set_power(void *pdata, int val) > > int val -> bool val? > >> +{ >> + struct w2sg_data *data = (struct w2sg_data *) pdata; >> + unsigned long flags; >> + >> + pr_debug("%s to %d (%d)\n", __func__, val, data->requested); >> + >> + spin_lock_irqsave(&data->lock, flags); >> + >> + if (val && !data->requested) { >> + data->requested = true; >> + } else if (!val && data->requested) { >> + data->backoff = HZ; >> + data->requested = false; >> + } else > > Braces issue with `else' branch. > >> + goto unlock; >> + >> + pr_debug("w2sg scheduled for %d\n", data->requested); >> + >> + if (!data->suspended) >> + schedule_delayed_work(&data->work, 0); >> +unlock: >> + spin_unlock_irqrestore(&data->lock, flags); >> +} >> + >> +/* called each time data is received by the host (i.e. sent by the w2sg0004) */ >> + >> +static int rx_notification(void *pdata, unsigned int *c) >> +{ >> + struct w2sg_data *data = (struct w2sg_data *) pdata; >> + unsigned long flags; >> + >> + if (!data->requested && !data->is_on) { > > Isn't it better to invert the if-statement and place return to be less > indented and more readable? Well, it is intended to describe the ?problematic? case, i.e. nothing requested and not on. But a proper comment might help with reversed logic. Something like /* rx was expected */ > >> + pr_debug("%02x!", *c); /* we have received a RX signal while GPS should be off */ >> + if ((data->state == W2SG_IDLE) && >> + time_after(jiffies, >> + data->last_toggle + data->backoff)) { > > Whole time_after() call should be on the same line. > >> + /* Should be off by now, time to toggle again */ >> + pr_debug("w2sg has sent data although it should be off!\n"); >> + data->is_on = true; >> + data->backoff *= 2; >> + spin_lock_irqsave(&data->lock, flags); >> + if (!data->suspended) >> + schedule_delayed_work(&data->work, 0); >> + spin_unlock_irqrestore(&data->lock, flags); >> + } >> + } >> + return 0; /* forward to tty */ >> +} >> + >> +/* called by uart modem control line changes (DTR) */ >> + >> +static int w2sg_mctrl(void *pdata, int val) >> +{ >> + pr_debug("%s(...,%x)\n", __func__, val); >> + val = (val & TIOCM_DTR) != 0; /* DTR controls power on/off */ > > IMO the following would be more readable: > > bool power_on = !!(val & TIOCM_DTR); > >> + w2sg_data_set_power((struct w2sg_data *) pdata, val); >> + return 0; >> +} >> + >> +/* try to toggle the power state by sending a pulse to the on-off GPIO */ >> + >> +static void toggle_work(struct work_struct *work) >> +{ >> + struct w2sg_data *data = container_of(work, struct w2sg_data, work.work); >> + >> + switch (data->state) { >> + case W2SG_IDLE: >> + spin_lock_irq(&data->lock); >> + if (data->requested == data->is_on) { >> + spin_unlock_irq(&data->lock); >> + return; >> + } >> + spin_unlock_irq(&data->lock); >> + w2sg_data_set_lna_power(data); /* update LNA power state */ >> + gpio_set_value_cansleep(data->on_off_gpio, 0); >> + data->state = W2SG_PULSE; >> + >> + pr_debug("w2sg: power gpio ON\n"); >> + >> + schedule_delayed_work(&data->work, >> + msecs_to_jiffies(10)); >> + break; >> + >> + case W2SG_PULSE: >> + gpio_set_value_cansleep(data->on_off_gpio, 1); >> + data->last_toggle = jiffies; >> + data->state = W2SG_NOPULSE; >> + data->is_on = !data->is_on; >> + >> + pr_debug("w2sg: power gpio OFF\n"); >> + >> + schedule_delayed_work(&data->work, >> + msecs_to_jiffies(10)); >> + break; >> + >> + case W2SG_NOPULSE: >> + data->state = W2SG_IDLE; >> + pr_debug("w2sg: idle\n"); >> + >> + break; >> + > > default case? > >> + } >> +} >> + >> +static int w2sg_data_rfkill_set_block(void *pdata, bool blocked) >> +{ >> + struct w2sg_data *data = pdata; >> + >> + pr_debug("%s: blocked: %d\n", __func__, blocked); >> + >> + data->lna_blocked = blocked; >> + >> + return w2sg_data_set_lna_power(data); >> +} >> + >> +static struct rfkill_ops w2sg_data0004_rfkill_ops = { >> + .set_block = w2sg_data_rfkill_set_block, >> +}; >> + >> +static int w2sg_data_probe(struct platform_device *pdev) >> +{ >> + struct w2sg_pdata *pdata = dev_get_platdata(&pdev->dev); >> + struct w2sg_data *data; >> + struct rfkill *rf_kill; >> + int err; >> + >> + pr_debug("%s()\n", __func__); >> + >> + if (pdev->dev.of_node) { >> + struct device *dev = &pdev->dev; >> + enum of_gpio_flags flags; >> + >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + pdata->on_off_gpio = of_get_named_gpio_flags(dev->of_node, "on-off-gpio", 0, &flags); >> + >> + if (pdata->on_off_gpio == -EPROBE_DEFER) >> + return -EPROBE_DEFER; /* defer until we have all gpios */ >> + >> + pdata->lna_regulator = devm_regulator_get_optional(dev, "lna"); >> + >> + pr_debug("%x() lna_regulator = %p\n", __func__, pdata->lna_regulator); >> + >> + pdev->dev.platform_data = pdata; >> + } >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + if (data == NULL) >> + return -ENOMEM; >> + >> + data->lna_regulator = pdata->lna_regulator; >> + data->lna_blocked = true; >> + data->lna_is_off = true; >> + >> + data->on_off_gpio = pdata->on_off_gpio; >> + >> + data->is_on = false; >> + data->requested = false; >> + data->state = W2SG_IDLE; >> + data->last_toggle = jiffies; >> + data->backoff = HZ; >> + >> +#ifdef CONFIG_OF >> + data->uart = devm_serial_get_uart_by_phandle(&pdev->dev, "uart", 0); >> + if (IS_ERR(data->uart)) { >> + if (PTR_ERR(data->uart) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; /* we can't probe yet */ >> + else > > `else' is redundand > >> + data->uart = NULL; /* no UART */ >> + } >> +#endif >> + >> + INIT_DELAYED_WORK(&data->work, toggle_work); >> + spin_lock_init(&data->lock); >> + >> + err = devm_gpio_request(&pdev->dev, data->on_off_gpio, "w2sg0004-on-off"); >> + if (err < 0) >> + goto out; >> + >> + gpio_direction_output(data->on_off_gpio, false); >> + >> + if (data->uart) { >> + struct ktermios termios = { >> + .c_iflag = IGNBRK, >> + .c_oflag = 0, >> + .c_cflag = B9600 | CS8, >> + .c_lflag = 0, >> + .c_line = 0, >> + .c_ispeed = 9600, >> + .c_ospeed = 9600 >> + }; >> + uart_register_slave(data->uart, data); >> + uart_register_mctrl_notification(data->uart, w2sg_mctrl); >> + uart_register_rx_notification(data->uart, rx_notification, &termios); >> + } >> + >> + rf_kill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS, >> + &w2sg_data0004_rfkill_ops, data); >> + if (rf_kill == NULL) { >> + err = -ENOMEM; >> + goto err_rfkill; > > return -ENOMEM; > >> + } >> + >> + err = rfkill_register(rf_kill); >> + if (err) { >> + dev_err(&pdev->dev, "Cannot register rfkill device\n"); >> + goto err_rfkill; >> + } >> + >> + data->rf_kill = rf_kill; >> + >> + platform_set_drvdata(pdev, data); >> + >> + pr_debug("w2sg0004 probed\n"); >> + >> +#ifdef CONFIG_W2SG0004_DEBUG >> + /* turn on for debugging rx notifications */ >> + pr_debug("w2sg power gpio ON\n"); >> + gpio_set_value_cansleep(data->on_off_gpio, 1); >> + mdelay(100); >> + pr_debug("w2sg power gpio OFF\n"); >> + gpio_set_value_cansleep(data->on_off_gpio, 0); >> + mdelay(300); >> +#endif >> + >> + /* if we won't receive mctrl notifications, turn on. >> + * otherwise keep off until DTR is asserted through mctrl. >> + */ >> + >> + w2sg_data_set_power(data, !data->uart); >> + >> + return 0; >> + >> +err_rfkill: >> + rfkill_destroy(rf_kill); >> +out: > > `out' label is useless as no cleanup code here. > > Keeping in mind that the one of two `goto err_rfkill' is incorrect, > `err_rfkill' label can be removed as there is the only one code path > left which is using it. > >> + return err; >> +} >> + >> +static int w2sg_data_remove(struct platform_device *pdev) >> +{ >> + struct w2sg_data *data = platform_get_drvdata(pdev); >> + >> + cancel_delayed_work_sync(&data->work); >> + >> + if (data->uart) { >> + uart_register_rx_notification(data->uart, NULL, NULL); >> + uart_register_slave(data->uart, NULL); >> + } >> + return 0; >> +} >> + >> +static int w2sg_data_suspend(struct device *dev) >> +{ >> + struct w2sg_data *data = dev_get_drvdata(dev); >> + >> + spin_lock_irq(&data->lock); >> + data->suspended = true; >> + spin_unlock_irq(&data->lock); >> + >> + cancel_delayed_work_sync(&data->work); >> + >> + w2sg_data_set_lna_power(data); /* shuts down if needed */ >> + >> + if (data->state == W2SG_PULSE) { >> + msleep(10); >> + gpio_set_value_cansleep(data->on_off_gpio, 1); >> + data->last_toggle = jiffies; >> + data->is_on = !data->is_on; >> + data->state = W2SG_NOPULSE; >> + } >> + >> + if (data->state == W2SG_NOPULSE) { >> + msleep(10); >> + data->state = W2SG_IDLE; >> + } >> + >> + if (data->is_on) { >> + pr_info("GPS off for suspend %d %d %d\n", data->requested, data->is_on, data->lna_is_off); >> + >> + gpio_set_value_cansleep(data->on_off_gpio, 0); >> + msleep(10); >> + gpio_set_value_cansleep(data->on_off_gpio, 1); >> + data->is_on = 0; > > 0 -> false (to stay similar with other code) > >> + } >> + >> + return 0; >> +} >> + >> +static int w2sg_data_resume(struct device *dev) >> +{ >> + struct w2sg_data *data = dev_get_drvdata(dev); >> + >> + spin_lock_irq(&data->lock); >> + data->suspended = false; >> + spin_unlock_irq(&data->lock); >> + >> + pr_info("GPS resuming %d %d %d\n", data->requested, data->is_on, data->lna_is_off); >> + >> + schedule_delayed_work(&data->work, 0); /* enables LNA if needed */ >> + >> + return 0; >> +} >> + >> +#if defined(CONFIG_OF) >> +static const struct of_device_id w2sg0004_of_match[] = { >> + { .compatible = "wi2wi,w2sg0004" }, >> + { .compatible = "wi2wi,w2sg0084" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match); >> +#endif >> + >> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_data_suspend, w2sg_data_resume); >> + >> +static struct platform_driver w2sg_data_driver = { >> + .probe = w2sg_data_probe, >> + .remove = w2sg_data_remove, >> + .driver = { >> + .name = "w2sg0004", >> + .owner = THIS_MODULE, >> + .pm = &w2sg_pm_ops, >> + .of_match_table = of_match_ptr(w2sg0004_of_match) >> + }, >> +}; >> + >> +module_platform_driver(w2sg_data_driver); >> + >> +MODULE_ALIAS("w2sg0004"); >> + >> +MODULE_AUTHOR("NeilBrown "); >> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/w2sg0004.h b/include/linux/w2sg0004.h >> new file mode 100644 >> index 0000000..7aee709 >> --- /dev/null >> +++ b/include/linux/w2sg0004.h >> @@ -0,0 +1,28 @@ >> +/* >> + * UART slave to allow ON/OFF control of w2sg0004 GPS receiver. >> + * >> + * Copyright (C) 2011 Neil Brown >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + */ >> + >> + >> + >> +#ifndef __LINUX_W2SG0004_H >> +#define __LINUX_W2SG0004_H >> + >> +#include >> + >> +struct w2sg_pdata { >> + struct regulator *lna_regulator; /* enable LNA power */ >> + int on_off_gpio; /* connected to the on-off input of the GPS module */ >> +}; >> +#endif /* __LINUX_W2SG0004_H */ >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/