| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150320195451.146a7915@notabene.brown>
Date: Fri, 20 Mar 2015 19:54:51 +1100
From: NeilBrown <neilb@...e.de>
To: "Dr. H. Nikolaus Schaller" <hns@...delico.com>
Cc: List for communicating with real GTA04 owners
<gta04-owner@...delico.com>, Mark Rutland <mark.rutland@....com>,
One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
Peter Hurley <peter@...leysoftware.com>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sebastian Reichel <sre@...nel.org>,
Pavel Machek <pavel@....cz>,
Grant Likely <grant.likely@...aro.org>,
Jiri Slaby <jslaby@...e.cz>, devicetree@...r.kernel.org,
lkml <linux-kernel@...r.kernel.org>
Subject: Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power
on/off UART attached devices.
On Fri, 20 Mar 2015 08:54:38 +0100 "Dr. H. Nikolaus Schaller"
<hns@...delico.com> wrote:
>
> Am 18.03.2015 um 06:58 schrieb NeilBrown <neil@...wn.name>:
>
> > 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@...wn.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.c
> >
> > diff --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.
Easier to get merged upstream? Definitely not.
After all, the uart itself doesn't require a power supply.
It is the device connected to the uart which requires the power supply.
>
> > 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.
>
> 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. That is how devicetree is structured - for
better or worse.
>
>
> > 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_SLAVE
> > Devices 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'
> > +
> > +endif
> > diff --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.o
> > diff --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.
>
> One driver per fundamentally different chip...
>
> > + */
> > +
> > +#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.
>
> > + .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.
>
> > + {}
> > +};
> > +
> > +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.
>
> > +
> > + /* 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.
>
> > + * 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.
>
> > 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.
Thanks,
NeilBrown
>
> We can resubmit it as soon as this driver stabilizes and finds acceptance.
>
> > + */
> > + 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 <neil@...wn.name>");
> > +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@...delico.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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists