Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.
From: Dr. H. Nikolaus Schaller <hidden>
Date: 2015-03-20 09:34:33
Also in:
lkml
Am 20.03.2015 um 09:54 schrieb NeilBrown [off-list ref]:
On Fri, 20 Mar 2015 08:54:38 +0100 "Dr. H. Nikolaus Schaller" [off-list ref] wrote:quoted
Am 18.03.2015 um 06:58 schrieb NeilBrown [off-list ref]:quoted
If a platform has a particular device permanently attached to a UART, there may be out-of-band signaling necessary to power the device on and off. This driver controls that signalling for a number of different devices. It can - enable/disable a regulator - toggle a GPIO - register an 'rfkill' which can force the device to be off. When the rfkill is absent or unblocked, the device will be on when the associated tty device is open, and closed otherwise. Signed-off-by: NeilBrown <neil@brown.name> --- .../bindings/tty_slave/wi2wi,w2cbw003.txt | 19 + .../bindings/tty_slave/wi2wi,w2sg0004.txt | 37 + .../devicetree/bindings/vendor-prefixes.txt | 1 drivers/tty/slave/Kconfig | 14 + drivers/tty/slave/Makefile | 2 drivers/tty/slave/serial-power-manager.c | 510 ++++++++++++++++++++ 6 files changed, 583 insertions(+) create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt create mode 100644 drivers/tty/slave/serial-power-manager.cdiff --git a/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt new file mode 100644 index 000000000000..cfe6ee5e01e9 --- /dev/null +++ b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt@@ -0,0 +1,19 @@ +wi2wi bluetooth module + +This is accessed via a serial port and is largely controlled via that +link. Extra configuration is needed to enable power on/off + +Required properties: +- compatible: "wi2wi,w2cbw003" +- vdd-supply: regulator used to power the device. + +The node for this device must be the child of a UART. + +Example: + +&uart1 { + bluetooth { + compatible = "wi2wi,w2cbw003"; + vdd-supply = <&vaux4>; + }; +};Wouldn’t it be easier to simply write &uart1 { vdd-suppy = <&vaux4>; }Easier to write: certainly. Easier to justify? No.
I just justified.
Easier to get merged upstream? Definitely not.
Are you the maintainer?
After all, the uart itself doesn't require a power supply. It is the device connected to the uart which requires the power supply.
Yes.
quoted
quoted
diff --git a/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt new file mode 100644 index 000000000000..fdc52cf56533 --- /dev/null +++ b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt@@ -0,0 +1,37 @@ +wi2wi GPS device + +This is accessed via a serial port and is largely controlled via that +link. Extra configuration is needed to enable power on/off + +Required properties: +- compatible: "wi2wi,w2sg0004" +- gpios: gpios used to toggle 'on/off' pin +- interrupts: interrupt generated by RX pin when device + should be off + +Optional properties: +- vdd-supply: regulator used to power antenna +- pinctrl: "default", "off" + if "off" setting is provided it is imposed when device should + be off. This can route the RX pin to a GPIO interrupt. + +The w2sg0004 uses a pin-toggle both to power-on and to +power-off, so the driver needs to detect what state it is in. +It does this by detecting characters on the RX line. +When it should be off, these can optionally be detected by a GPIO. + +The node for this device must be the child of a UART. + +Example: +&uart2 { + gps { + compatible = "wi2iw,w2sg0004"; + vdd-supply = <&vsim>; + gpios = <&gpio5 17 0>; /* GPIO_145 */ + interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */ + /* When off, switch RX to be an interrupt */ + pinctrl-names = "default", "off"; + pinctrl-0 = <&uart2_pins>; + pinctrl-1 = <&uart2_pins_rx_gpio>; + }; +};If the wi2wi driver is a regulator driver one would write / { gps-regulator: gps { compatible = "wi2iw,w2sg0004"; vdd-supply = <&vsim>; gpios = <&gpio5 17 0>; /* GPIO_145 */ interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */ /* When off, switch RX to be an interrupt */ pinctrl-names = "default", "off"; pinctrl-0 = <&uart2_pins>; pinctrl-1 = <&uart2_pins_rx_gpio>; }; } &uart2 { vdd-suppy = <&gps-regulator>; }; Which IMHO better describes that the uart controls power of a separate driver.But the uart doesn’t control the power. An 'open' on the tty causes one driver to turn on a regulator, and another driver to activate a uart so that the device represented by the tty can be communicated with.
Ok, that is a detail I have mixed up tty and uart.
quoted
And this pattern for writing a DT would IMHO be more flexible because you can „connect“ to any regulator, e.g. a regulator for a RS232 level shifter.I'm sure there are lots of ways we could find to make DT more flexible, only then it wouldn't be DT any more - it would be something similar but different. There needs to be one device-node for each device, and that device-node needs to be a child of the device-node for the device which is the primary connection to the child device.
Then please explain to me nodes like
/ {
compatible = "ti,omap3-gta04", "ti,omap36xx", "ti,omap3";
cpus {
cpu@0 {
cpu0-supply = <&vcc>;
};
};
According to the rule you apply here it should be something like
cpu@0 {
regulator {
…
}
That is how devicetree is structured - for better or worse.quoted
quoted
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 389ca1347a77..81d259303710 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt@@ -189,6 +189,7 @@ variscite Variscite Ltd.via VIA Technologies, Inc. virtio Virtual I/O Device Specification, developed by the OASIS consortium voipac Voipac Technologies s.r.o. +wi2wi wi2wi Inc. http://www.wi2wi.com/ winbond Winbond Electronics corp. wlf Wolfson Microelectronics wm Wondermedia Technologies, Inc.diff --git a/drivers/tty/slave/Kconfig b/drivers/tty/slave/Kconfig index 3976760c2e28..05c5d966ae57 100644 --- a/drivers/tty/slave/Kconfig +++ b/drivers/tty/slave/Kconfig@@ -5,3 +5,17 @@ menuconfig TTY_SLAVEDevices which attach via a uart, but need extra driver support for power management etc. +if TTY_SLAVE + +config SERIAL_POWER_MANAGER + tristate "Power Management controller for serial-attached devices" + default n + help + Some devices permanently attached via a UART can benefit from + being power-managed when the tty device is opened or closed. + This driver can support several such devices with simple + power requirements such as enabling a regulator. + + If in doubt, say 'N' + +endifdiff --git a/drivers/tty/slave/Makefile b/drivers/tty/slave/Makefile index 65669acb392e..a2f7d2847319 100644 --- a/drivers/tty/slave/Makefile +++ b/drivers/tty/slave/Makefile@@ -1,2 +1,4 @@obj-$(CONFIG_TTY_SLAVE) += tty_slave_core.o + +obj-$(CONFIG_SERIAL_POWER_MANAGER) += serial-power-manager.odiff --git a/drivers/tty/slave/serial-power-manager.c b/drivers/tty/slave/serial-power-manager.c new file mode 100644 index 000000000000..662a526d8630 --- /dev/null +++ b/drivers/tty/slave/serial-power-manager.c@@ -0,0 +1,510 @@ +/* + * Serial-power-manager + * tty-slave device that intercepts open/close events on the tty, + * and turns power on/off for the device which is connected. + * + * Currently supported devices: + * wi2wi,w2sg0004 - GPS with on/off toggle on a GPIO + * wi2wi,w2cbw003 - bluetooth port; powered by regulator. + * + * When appropriate, an RFKILL will be registered which + * can power-down the device even when it is open. + * + * Device can be turned on either by + * - enabling a regulator. Disable to turn off + * - toggling a GPIO. Toggle again to turn off. This requires + * that we know the current state. It is assumed to be 'off' + * at boot, however if an interrupt can be generated when on, + * such as by connecting RX to a GPIO, that can be used to detect + * if the device is on when it should be off.Why does this driver mix both things? The only thing they have in common is that both are uart slaves and that they have a serial interface. But power control is very different.Because if I wrote two drivers, they would have more code in common than they would have differences.
I mostly can see the w2sg0004 part as an addition on top of a common boilerplate part.
quoted
One driver per fundamentally different chip...quoted
+ */ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/regulator/consumer.h> +#include <linux/platform_device.h> +#include <linux/of_device.h> +#include <linux/tty.h> +#include <linux/gpio.h> +#include <linux/of_gpio.h> +#include <linux/of_irq.h> +#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/rfkill.h> + +#include <linux/tty_slave.h> + +/* This is used for testing. Setting this module parameter + * will simulate booting with the device "on" + */ +static bool toggle_on_probe = false; +module_param(toggle_on_probe, bool, 0); +MODULE_PARM_DESC(toggle_on_probe, "simulate power-on with devices active"); + +struct spm_config { + int rfkill_type; /* type of rfkill to register */ + int toggle_time; /* msec to pulse GPIO for on/off */ + int toggle_gap; /* min msecs between toggles */ + bool off_in_suspend; +} + simple_config = { + .off_in_suspend = true, + }, + w2sg_config = { + .rfkill_type = RFKILL_TYPE_GPS,The driver pretends to be generic by its name but incorporates a lot of specific knowledge about the w2sg chip, e.g. that it is a GPS chip.quoted
+ .toggle_time = 10, + .toggle_gap = 500, + .off_in_suspend = true, + }; + +const static struct of_device_id spm_dt_ids[] = { + { .compatible = "wi2wi,w2sg0004", .data = &w2sg_config}, + { .compatible = "wi2wi,w2cbw003", .data = &simple_config},Well, how large will this table become if other uart slave device types are added?When that becomes a problem it can trivially be solved. While it is not a problem there is no value in solving it.
thought-terminating argument?
quoted
quoted
+ {} +}; + +struct spm_data { + const struct spm_config *config; + struct gpio_desc *gpiod; + int irq; /* irq line from RX pin when pinctrl + * set to 'idle' */ + struct regulator *reg; + + unsigned long toggle_time; + unsigned long toggle_gap; + unsigned long last_toggle; /* jiffies when last toggle completed. */ + unsigned long backoff; /* jiffies since last_toggle when + * we try again + */ + enum {Idle, Down, Up} state; /* state-machine state. */ + + int open_cnt; + bool requested, is_on; + bool suspended; + bool reg_enabled; + + struct pinctrl *pins; + struct pinctrl_state *pins_off; + + struct delayed_work work; + spinlock_t lock; + struct device *dev; + + struct rfkill *rfkill; + + int (*old_open)(struct tty_struct * tty, struct file * filp); + void (*old_close)(struct tty_struct * tty, struct file * filp); + +}; + +/* When a device is powered on/off by toggling a GPIO we perform + * all the toggling via a workqueue to ensure only one toggle happens + * at a time and to allow easy timing. + * This is managed as a state machine which transitions + * Idle -> Down -> Up -> Idle + * The GPIO is held down for toggle_time and then up for toggle_time, + * and then we assume the device has changed state. + * We never toggle until at least toggle_gap has passed since the + * last toggle. + */ +static void toggle_work(struct work_struct *work) +{ + struct spm_data *data = container_of( + work, struct spm_data, work.work); + + if (data->gpiod == NULL) + return; + + spin_lock_irq(&data->lock); + switch (data->state) { + case Up: + data->state = Idle; + if (data->requested == data->is_on) + break; + if (!data->requested) + /* Assume it is off unless activity is detected */ + break; + /* Try again in a while unless we get some activity */ + dev_dbg(data->dev, "Wait %dusec until retry\n", + jiffies_to_msecs(data->backoff)); + schedule_delayed_work(&data->work, data->backoff); + break; + case Idle: + if (data->requested == data->is_on) + break; + + /* Time to toggle */ + dev_dbg(data->dev, "Starting toggle to turn %s\n", + data->requested ? "on" : "off"); + data->state = Down; + spin_unlock_irq(&data->lock); + gpiod_set_value_cansleep(data->gpiod, 1); + schedule_delayed_work(&data->work, data->toggle_time); + + return; + + case Down: + data->state = Up; + data->last_toggle = jiffies; + dev_dbg(data->dev, "Toggle completed, should be %s now.\n", + data->is_on ? "off" : "on"); + data->is_on = ! data->is_on; + spin_unlock_irq(&data->lock); + + gpiod_set_value_cansleep(data->gpiod, 0); + schedule_delayed_work(&data->work, data->toggle_time); + + return; + } + spin_unlock_irq(&data->lock); +} + +static irqreturn_t spm_isr(int irq, void *dev_id) +{ + struct spm_data *data = dev_id; + unsigned long flags; + + spin_lock_irqsave(&data->lock, flags); + if (!data->requested && !data->is_on && data->state == Idle && + time_after(jiffies, data->last_toggle + data->backoff)) { + data->is_on = 1; + data->backoff *= 2; + dev_dbg(data->dev, "Received data, must be on. Try to turn off\n"); + if (!data->suspended) + schedule_delayed_work(&data->work, 0); + } + spin_unlock_irqrestore(&data->lock, flags); + return IRQ_HANDLED; +} + +static void spm_on(struct spm_data *data) +{ + if (!data->rfkill || !rfkill_blocked(data->rfkill)) { + unsigned long flags; + + if (!data->reg_enabled && + data->reg && + regulator_enable(data->reg) == 0) + data->reg_enabled = true; + + spin_lock_irqsave(&data->lock, flags); + if (!data->requested) { + dev_dbg(data->dev, "TTY open - turn device on\n"); + data->requested = true; + data->backoff = data->toggle_gap; + if (data->irq > 0) { + disable_irq(data->irq); + pinctrl_pm_select_default_state(data->dev); + } + if (!data->suspended && data->state == Idle) + schedule_delayed_work(&data->work, 0); + } + spin_unlock_irqrestore(&data->lock, flags); + } +} + +static int spm_open(struct tty_struct *tty, struct file *filp) +{ + struct spm_data *data = dev_get_drvdata(tty->dev->parent); + + data->open_cnt++; + spm_on(data); + if (data->old_open) + return data->old_open(tty, filp); +} + +static void spm_off(struct spm_data *data) +{ + unsigned long flags; + + if (data->reg && data->reg_enabled) + if (regulator_disable(data->reg) == 0) + data->reg_enabled = false; + + spin_lock_irqsave(&data->lock, flags); + if (data->requested) { + data->requested = false; + data->backoff = data->toggle_gap; + if (data->pins_off) { + pinctrl_select_state(data->pins, + data->pins_off); + enable_irq(data->irq); + } + if (!data->suspended && data->state == Idle) + schedule_delayed_work(&data->work, 0); + } + spin_unlock_irqrestore(&data->lock, flags); +} + +static void spm_close(struct tty_struct *tty, struct file *filp) +{ + struct spm_data *data = dev_get_drvdata(tty->dev->parent); + + data->open_cnt--; + if (!data->open_cnt) { + dev_dbg(data->dev, "TTY closed - turn device off\n"); + spm_off(data); + } + + if (data->old_close) + data->old_close(tty, filp); +} + +static int spm_rfkill_set_block(void *vdata, bool blocked) +{ + struct spm_data *data = vdata; + + dev_dbg(data->dev, "rfkill_set_blocked %d\n", blocked); + if (blocked) + spm_off(data); + + if (!blocked && + data->open_cnt) + spm_on(data); + + return 0; +} + +static struct rfkill_ops spm_rfkill_ops = { + .set_block = spm_rfkill_set_block, +}; + +static int spm_suspend(struct device *dev) +{ + /* Ignore incoming data and just turn device off. + * we cannot really wait for a separate thread to + * do things, so we disable that and do it all + * here + */ + struct spm_data *data = dev_get_drvdata(dev); + + spin_lock_irq(&data->lock); + data->suspended = true; + spin_unlock_irq(&data->lock); + if (!data->config->off_in_suspend) + return 0; + + if (data->gpiod) { + + cancel_delayed_work_sync(&data->work); + if (data->state == Down) { + dev_dbg(data->dev, "Suspending while GPIO down - raising\n"); + msleep(data->config->toggle_time); + gpiod_set_value_cansleep(data->gpiod, 0); + data->last_toggle = jiffies; + data->is_on = !data->is_on; + data->state = Up; + } + if (data->state == Up) { + msleep(data->config->toggle_time); + data->state = Idle; + } + if (data->is_on) { + dev_dbg(data->dev, "Suspending while device on: toggling\n"); + gpiod_set_value_cansleep(data->gpiod, 1); + msleep(data->config->toggle_time); + gpiod_set_value_cansleep(data->gpiod, 0); + data->is_on = 0; + } + } + + if (data->reg && data->reg_enabled) + if (regulator_disable(data->reg) == 0) + data->reg_enabled = false; + + return 0; +} + +static int spm_resume(struct device *dev) +{ + struct spm_data *data = dev_get_drvdata(dev); + + spin_lock_irq(&data->lock); + data->suspended = false; + spin_unlock_irq(&data->lock); + schedule_delayed_work(&data->work, 0); + + if (data->open_cnt && + (!data->rfkill || !rfkill_blocked(data->rfkill))) { + if (!data->reg_enabled && + data->reg && + regulator_enable(data->reg) == 0) + data->reg_enabled = true; + } + return 0; +} + +static const struct dev_pm_ops spm_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(spm_suspend, spm_resume) +}; + +static int spm_probe(struct device *dev) +{ + struct tty_slave *slave = container_of(dev, struct tty_slave, dev); + struct spm_data *data; + struct regulator *reg; + int err; + const struct of_device_id *id; + const char *name; + + if (dev->parent == NULL) + return -ENODEV; + + id = of_match_device(spm_dt_ids, dev); + if (!id) + return -ENODEV; + + if (dev->of_node && dev->of_node->name) + name = dev->of_node->name; + else + name = "serial-power-manager"; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->config = id->data; + data->toggle_time = msecs_to_jiffies(data->config->toggle_time) + 1; + data->toggle_gap = msecs_to_jiffies(data->config->toggle_gap) + 1; + data->last_toggle = jiffies; + data->backoff = data->toggle_gap; + data->state = Idle; + spin_lock_init(&data->lock); + INIT_DELAYED_WORK(&data->work, toggle_work); + + /* If a regulator is provided, it is enabled on 'open' + * and disabled on 'release' + */ + reg = devm_regulator_get(dev, "vdd"); + if (IS_ERR(reg)) { + err = PTR_ERR(reg); + if (err != -ENODEV) + goto out; + } else + data->reg = reg; + + /* If an irq is provided, any transitions are taken as + * indication that the device is currently "on" + */ + data->irq = of_irq_get(dev->of_node, 0); + if (data->irq < 0) { + err = data->irq; + if (err != -EINVAL) + goto out; + } else { + dev_dbg(dev, "IRQ configured: %d\n", data->irq); + + irq_set_status_flags(data->irq, IRQ_NOAUTOEN); + err = devm_request_irq(dev, data->irq, spm_isr, + IRQF_TRIGGER_FALLING, + name, data); + + if (err) + goto out; + + }Up to here it is generic and makes no assumptions about a specific device.quoted
+ + /* If a gpio is provided, then it is used to turn the device + * on/off.You have a compatible record (for two well defined chips), but you do control which functions are really used by providing/not providing some DT parameters. Either one is redundant.quoted
+ * If toggle_time is zero, then the GPIO directly controls + * the device.Very hidden and non-obvoius functionality.Yes, that should probably be documented more clearly, or removed.quoted
quoted
If non-zero, then the GPIO must be toggled to + * change the state of the device.All the following is very special logic for the w2sg0004 chip which should be divided out into a separate driver. Marek and me already had proposed such a chip specific driver (to be located in drivers/misc) some months ago. It would encapsulate everything w2sg0004 specific and present itself as a regulator (because that is its main purpose: control the LDO regulator inside the w2sg0004 chip).Presenting itself as a regulator would be wrong because it isn’t a regulator.
It has a regulator that can be controlled by a gpio… Another example to think about: the twl4030 is also not a regulator. Nevertheless they present some regulator nodes. Or take the OMAP3 pbias_regulator. The OMAP3 isn’t a regulator as well but has an internal pbias_regulator that needs to be controlled. BR, Nikolaus
Thanks, NeilBrownquoted
We can resubmit it as soon as this driver stabilizes and finds acceptance.quoted
+ */ + data->gpiod = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW); + if (IS_ERR(data->gpiod)) { + err = PTR_ERR(data->gpiod); + if (err != -ENOENT) + goto out; + data->gpiod = NULL; + } else + dev_dbg(dev, "GPIO configured: %d\n", + desc_to_gpio(data->gpiod)); + + /* If an 'off' pinctrl state is defined, we apply that + * when the device is assumed to be off. This is expected to + * route the 'rx' line to the 'irq' interrupt. + */ + data->pins = devm_pinctrl_get(dev); + if (data->pins && data->irq > 0) { + data->pins_off = pinctrl_lookup_state(data->pins, "off"); + if (IS_ERR(data->pins_off)) + data->pins_off = NULL; + } + + if (data->config->rfkill_type) { + data->rfkill = rfkill_alloc(name, dev, + data->config->rfkill_type, + &spm_rfkill_ops, data); + if (!data->rfkill) { + err = -ENOMEM; + goto out; + } + err = rfkill_register(data->rfkill); + if (err) { + dev_err(dev, "Cannot register rfkill device"); + rfkill_destroy(data->rfkill); + goto out; + } + } + dev_set_drvdata(dev, data); + data->dev = dev; + data->old_open = slave->ops.open; + data->old_close = slave->ops.close; + slave->ops.open = spm_open; + slave->ops.close = spm_close; + tty_slave_finalize(slave); + + if (data->pins_off) + pinctrl_select_state(data->pins, data->pins_off); + if (data->irq > 0) + enable_irq(data->irq); + + if (toggle_on_probe && data->gpiod) { + dev_dbg(data->dev, "Performing initial toggle\n"); + gpiod_set_value_cansleep(data->gpiod, 1); + msleep(data->config->toggle_time); + gpiod_set_value_cansleep(data->gpiod, 0); + msleep(data->config->toggle_time); + } + err = 0; +out: + dev_dbg(data->dev, "Probed: err=%d\n", err); + return err; +} + +static int spm_remove(struct device *dev) +{ + struct spm_data *data = dev_get_drvdata(dev); + + if (data->rfkill) { + rfkill_unregister(data->rfkill); + rfkill_destroy(data->rfkill); + } + return 0; +} + +static struct device_driver spm_driver = { + .name = "serial-power-manager", + .owner = THIS_MODULE, + .of_match_table = spm_dt_ids, + .probe = spm_probe, + .remove = spm_remove, +}; + +static int __init spm_init(void) +{ + return tty_slave_driver_register(&spm_driver); +} +module_init(spm_init); + +static void __exit spm_exit(void) +{ + driver_unregister(&spm_driver); +} +module_exit(spm_exit); + +MODULE_AUTHOR("NeilBrown [off-list ref]"); +MODULE_DEVICE_TABLE(of, spm_dt_ids); +MODULE_DESCRIPTION("Power management for Serial-attached device."); +MODULE_LICENSE("GPL v2"); _______________________________________________ Gta04-owner mailing list Gta04-owner@goldelico.com http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner-- 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/