Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 26763 bytes --]

On Fri, 20 Mar 2015 08:54:38 +0100 "Dr. H. Nikolaus Schaller"
<hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org> wrote:

> 
> Am 18.03.2015 um 06:58 schrieb NeilBrown <neil-+NVA1uvv1dVBDLzU/O5InQ@public.gmane.org>:
> 
> > 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-+NVA1uvv1dVBDLzU/O5InQ@public.gmane.org>
> > ---
> > .../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-+NVA1uvv1dVBDLzU/O5InQ@public.gmane.org>");
> > +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-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org
> > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]