Message-Id: <14FB51CF-9568-4BF4-B917-2C019D992FDB@goldelico.com>
Date:	Fri, 20 Mar 2015 10:34:18 +0100
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	NeilBrown <neilb@...e.de>
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.


Am 20.03.2015 um 09:54 schrieb NeilBrown <neilb@...e.de>:

> 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.

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.

> 
> 
>> 
>>> 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.

> 
>> 
>> 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.
> 
> 
> 
>> 
>> 
>>> 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.

I mostly can see the w2sg0004 part as an addition on top of a common boilerplate part.

> 
> 
>> 
>> 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.

thought-terminating argument?

> 
> 
>> 
>>> +       {}
>>> +};
>>> +
>>> +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.

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,
> 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/
> 

--
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/

Powered by blists - more mailing lists