diff mbox

[2/3] TTY: add slave driver to power-on device via a regulator.

Message ID 20141211215944.4127.4186.stgit@notabene.brown
State New, archived
Headers show

Commit Message

NeilBrown Dec. 11, 2014, 9:59 p.m. UTC
The regulator is identified in devicetree as 'vdd-supply'

Signed-off-by: NeilBrown <neilb@suse.de>
---
 .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
 drivers/tty/Kconfig                                |    2 
 drivers/tty/Makefile                               |    1 
 drivers/tty/slaves/Kconfig                         |   12 ++
 drivers/tty/slaves/Makefile                        |    2 
 drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
 6 files changed, 137 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
 create mode 100644 drivers/tty/slaves/Kconfig
 create mode 100644 drivers/tty/slaves/Makefile
 create mode 100644 drivers/tty/slaves/tty-reg.c



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Sebastian Reichel Dec. 11, 2014, 10:58 p.m. UTC | #1
Hi,

On Fri, Dec 12, 2014 at 08:59:44AM +1100, NeilBrown wrote:
> The regulator is identified in devicetree as 'vdd-supply'

long description is kind of useless...

> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
>  drivers/tty/Kconfig                                |    2 
>  drivers/tty/Makefile                               |    1 
>  drivers/tty/slaves/Kconfig                         |   12 ++
>  drivers/tty/slaves/Makefile                        |    2 
>  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
>  6 files changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
>  create mode 100644 drivers/tty/slaves/Kconfig
>  create mode 100644 drivers/tty/slaves/Makefile
>  create mode 100644 drivers/tty/slaves/tty-reg.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index 000000000000..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device
> +
> +
> +This is listed as a child node of a UART.  When the
> +UART is opened, the device is powered.
> +
> +Example:
> +
> +&uart1 {
> +	bluetooth {
> +		compatible = "tty,regulator";
> +		vdd-supply = <&vaux4>;
> +	};
> +};

NACK. The compatible value should describe the connected device. You
did not connect a regulator, but a bluetooth chip! DT should look
like this:

&uart1 {
    bluetooth {
        compatible = "vendor,bluetooth-chip";
        vdd-supply = <&vaux4>;
    };
};

I think it would be ok to use your generic driver to handle the
specific compatible value, though.

Having the proper compatible value means, that there can be a more
specific driver later, that other operating systems can expose the
device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
userspace is able to know there is a bluetooth device connected to
/dev/ttyXY by parsing the DT and results in easier-to-understand
DTS.

> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
>  	help
>  	  This enables a console on a Dash channel.
>  
> +source drivers/tty/slaves/Kconfig
> +
>  endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_R3964)		+= n_r3964.o
>  obj-y				+= vt/
>  obj-$(CONFIG_HVC_DRIVER)	+= hvc/
>  obj-y				+= serial/
> +obj-$(CONFIG_TTY_SLAVE)		+= slaves/
>  
>  # tty drivers
>  obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index 000000000000..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> +	bool "TTY slave devices"
> +	help
> +	 Devices which attach via a uart, but need extra
> +	 driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> +	tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index 000000000000..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index 000000000000..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + *   Support for any device which needs a regulator turned on
> + *   when a tty is opened.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/tty.h>
> +
> +struct tty_reg_data {
> +	struct regulator *reg;
> +	bool	reg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +	if (!data->reg_enabled &&
> +	    regulator_enable(data->reg) == 0) {
> +		dev_dbg(slave, "power on\n");
> +		data->reg_enabled = true;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> +	if (data->reg_enabled &&
> +	    regulator_disable(data->reg) == 0) {
> +		dev_dbg(slave, "power off\n");
> +		data->reg_enabled = false;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> +	struct tty_reg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	err = -ENODEV;
> +	if (pdev->dev.parent == NULL)
> +		goto out;
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	err = PTR_ERR(reg);
> +	if (IS_ERR(reg))
> +		goto out;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	err = -ENOMEM;
> +	if (!data)
> +		goto out;
> +	data->reg = reg;
> +	data->reg_enabled = false;
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, data);
> +	err = 0;
> +out:
> +	return err;
> +}
> +
> +static struct of_device_id tty_reg_dt_ids[] = {
> +	{ .compatible = "tty,regulator", },
> +	{}
> +};
> +
> +static const struct dev_pm_ops tty_reg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
> +			   tty_reg_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver tty_reg_driver = {
> +	.driver.name	= "tty-regulator",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.of_match_table = tty_reg_dt_ids,
> +	.driver.pm	= &tty_reg_pm_ops,
> +	.probe		= tty_reg_probe,
> +};
> +
> +static int __init tty_reg_init(void)
> +{
> +	return platform_driver_register(&tty_reg_driver);
> +}
> +module_init(tty_reg_init);
> +
> +static void __exit tty_reg_exit(void)
> +{
> +	platform_driver_unregister(&tty_reg_driver);
> +}
> +module_exit(tty_reg_exit);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
> +MODULE_LICENSE("GPL v2");
> 
> 
> --
> 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/
Peter Hurley Dec. 11, 2014, 11:32 p.m. UTC | #2
On 12/11/2014 04:59 PM, NeilBrown wrote:
> The regulator is identified in devicetree as 'vdd-supply'
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

tty_* is only for tty core functions/data.

Regards,
Peter Hurley

> ---
>  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
>  drivers/tty/Kconfig                                |    2 
>  drivers/tty/Makefile                               |    1 
>  drivers/tty/slaves/Kconfig                         |   12 ++
>  drivers/tty/slaves/Makefile                        |    2 
>  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
>  6 files changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
>  create mode 100644 drivers/tty/slaves/Kconfig
>  create mode 100644 drivers/tty/slaves/Makefile
>  create mode 100644 drivers/tty/slaves/tty-reg.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index 000000000000..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device
> +
> +
> +This is listed as a child node of a UART.  When the
> +UART is opened, the device is powered.
> +
> +Example:
> +
> +&uart1 {
> +	bluetooth {
> +		compatible = "tty,regulator";
> +		vdd-supply = <&vaux4>;
> +	};
> +};
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
>  	help
>  	  This enables a console on a Dash channel.
>  
> +source drivers/tty/slaves/Kconfig
> +
>  endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_R3964)		+= n_r3964.o
>  obj-y				+= vt/
>  obj-$(CONFIG_HVC_DRIVER)	+= hvc/
>  obj-y				+= serial/
> +obj-$(CONFIG_TTY_SLAVE)		+= slaves/
>  
>  # tty drivers
>  obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index 000000000000..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> +	bool "TTY slave devices"
> +	help
> +	 Devices which attach via a uart, but need extra
> +	 driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> +	tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index 000000000000..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index 000000000000..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + *   Support for any device which needs a regulator turned on
> + *   when a tty is opened.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/tty.h>
> +
> +struct tty_reg_data {
> +	struct regulator *reg;
> +	bool	reg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +	if (!data->reg_enabled &&
> +	    regulator_enable(data->reg) == 0) {
> +		dev_dbg(slave, "power on\n");
> +		data->reg_enabled = true;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> +	if (data->reg_enabled &&
> +	    regulator_disable(data->reg) == 0) {
> +		dev_dbg(slave, "power off\n");
> +		data->reg_enabled = false;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> +	struct tty_reg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	err = -ENODEV;
> +	if (pdev->dev.parent == NULL)
> +		goto out;
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	err = PTR_ERR(reg);
> +	if (IS_ERR(reg))
> +		goto out;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	err = -ENOMEM;
> +	if (!data)
> +		goto out;
> +	data->reg = reg;
> +	data->reg_enabled = false;
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, data);
> +	err = 0;
> +out:
> +	return err;
> +}
> +
> +static struct of_device_id tty_reg_dt_ids[] = {
> +	{ .compatible = "tty,regulator", },
> +	{}
> +};
> +
> +static const struct dev_pm_ops tty_reg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
> +			   tty_reg_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver tty_reg_driver = {
> +	.driver.name	= "tty-regulator",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.of_match_table = tty_reg_dt_ids,
> +	.driver.pm	= &tty_reg_pm_ops,
> +	.probe		= tty_reg_probe,
> +};
> +
> +static int __init tty_reg_init(void)
> +{
> +	return platform_driver_register(&tty_reg_driver);
> +}
> +module_init(tty_reg_init);
> +
> +static void __exit tty_reg_exit(void)
> +{
> +	platform_driver_unregister(&tty_reg_driver);
> +}
> +module_exit(tty_reg_exit);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
> +MODULE_LICENSE("GPL v2");


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann Dec. 12, 2014, 12:46 a.m. UTC | #3
Hi Sebastian,

>> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
>> new file mode 100644
>> index 000000000000..7896bce8dfe4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
>> @@ -0,0 +1,18 @@
>> +Regulator powered UART-attached device
>> +
>> +Required properties:
>> +- compatible: "tty,regulator"
>> +- vdd-supply: regulator to power the device
>> +
>> +
>> +This is listed as a child node of a UART.  When the
>> +UART is opened, the device is powered.
>> +
>> +Example:
>> +
>> +&uart1 {
>> +	bluetooth {
>> +		compatible = "tty,regulator";
>> +		vdd-supply = <&vaux4>;
>> +	};
>> +};
> 
> NACK. The compatible value should describe the connected device. You
> did not connect a regulator, but a bluetooth chip! DT should look
> like this:
> 
> &uart1 {
>    bluetooth {
>        compatible = "vendor,bluetooth-chip";
>        vdd-supply = <&vaux4>;
>    };
> };
> 
> I think it would be ok to use your generic driver to handle the
> specific compatible value, though.
> 
> Having the proper compatible value means, that there can be a more
> specific driver later, that other operating systems can expose the
> device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
> userspace is able to know there is a bluetooth device connected to
> /dev/ttyXY by parsing the DT and results in easier-to-understand
> DTS.

I think that is up to udev to be able to make this a nice device node. However the device node name is pretty much irrelevant. What you want is enough meta data to tell that there is Bluetooth chip behind this TTY. From a Bluetooth stack perspective only hciattach needs to be run to get the N_HCI line discipline up and running for that chip.

However what would be interesting for hciattach would be a possibility to expose the Bluetooth manufacturer. Since the init routine is different for each manufacturer. So if that could be encoded in the DT, then that would be certainly helpful.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel Dec. 12, 2014, 1:31 a.m. UTC | #4
Hi Marcel,

On Fri, Dec 12, 2014 at 01:46:33AM +0100, Marcel Holtmann wrote:
> > NACK. The compatible value should describe the connected device. You
> > did not connect a regulator, but a bluetooth chip! DT should look
> > like this:
> > 
> > &uart1 {
> >    bluetooth {
> >        compatible = "vendor,bluetooth-chip";
> >        vdd-supply = <&vaux4>;
> >    };
> > };
> > 
> > I think it would be ok to use your generic driver to handle the
> > specific compatible value, though.
> > 
> > Having the proper compatible value means, that there can be a more
> > specific driver later, that other operating systems can expose the
> > device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
> > userspace is able to know there is a bluetooth device connected to
> > /dev/ttyXY by parsing the DT and results in easier-to-understand
> > DTS.
> 
> I think that is up to udev to be able to make this a nice device
> node. However the device node name is pretty much irrelevant. 

Ah that was not about Linux, but about $random-os using DT. Just
ignore the device name its not important for the argument.

> What you want is enough meta data to tell that there is Bluetooth
> chip behind this TTY.

Yes, that is what I tried to say :)

> From a Bluetooth stack perspective only hciattach needs to be run
> to get the N_HCI line discipline up and running for that chip.

Yes, but with the metadata from Neil's proposed binding you do not
know, that there is a bluetooth chip connected, so you need to do
this manually. Not very nice, when it could be done automatically.

> However what would be interesting for hciattach would be a
> possibility to expose the Bluetooth manufacturer. Since the init
> routine is different for each manufacturer.

Yes and it may be, that a manufacturer breaks its init routine at
some point. The problem is not much different from other devices
and normally solved through the compatible string.

> So if that could be encoded in the DT, then that would be
> certainly helpful.

All needed data should be encoded when specified as in the following
example:

&uart1 {
    bluetooth {
        compatible = "wi2wi,w2cbw003";
        vdd-supply = <&vaux4>;
    };
};

Note: wi2wi is not yet in
Documentation/devicetree/bindings/vendor-prefixes.txt

-- Sebastian
NeilBrown Dec. 12, 2014, 5:01 a.m. UTC | #5
On Thu, 11 Dec 2014 23:58:48 +0100 Sebastian Reichel <sre@kernel.org> wrote:

> Hi,
> 
> On Fri, Dec 12, 2014 at 08:59:44AM +1100, NeilBrown wrote:
> > The regulator is identified in devicetree as 'vdd-supply'
> 
> long description is kind of useless...
> 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
> >  drivers/tty/Kconfig                                |    2 
> >  drivers/tty/Makefile                               |    1 
> >  drivers/tty/slaves/Kconfig                         |   12 ++
> >  drivers/tty/slaves/Makefile                        |    2 
> >  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
> >  6 files changed, 137 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
> >  create mode 100644 drivers/tty/slaves/Kconfig
> >  create mode 100644 drivers/tty/slaves/Makefile
> >  create mode 100644 drivers/tty/slaves/tty-reg.c
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> > new file mode 100644
> > index 000000000000..7896bce8dfe4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> > @@ -0,0 +1,18 @@
> > +Regulator powered UART-attached device
> > +
> > +Required properties:
> > +- compatible: "tty,regulator"
> > +- vdd-supply: regulator to power the device
> > +
> > +
> > +This is listed as a child node of a UART.  When the
> > +UART is opened, the device is powered.
> > +
> > +Example:
> > +
> > +&uart1 {
> > +	bluetooth {
> > +		compatible = "tty,regulator";
> > +		vdd-supply = <&vaux4>;
> > +	};
> > +};
> 
> NACK. The compatible value should describe the connected device. You
> did not connect a regulator, but a bluetooth chip! DT should look
> like this:
> 
> &uart1 {
>     bluetooth {
>         compatible = "vendor,bluetooth-chip";
>         vdd-supply = <&vaux4>;
>     };
> };
> 
> I think it would be ok to use your generic driver to handle the
> specific compatible value, though.
> 
> Having the proper compatible value means, that there can be a more
> specific driver later, that other operating systems can expose the
> device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
> userspace is able to know there is a bluetooth device connected to
> /dev/ttyXY by parsing the DT and results in easier-to-understand
> DTS.

So the tty_reg_dt_ids array could conceivably grow a long list of compatible
devices, starting with this one.  I guess that makes sense.

Thanks,
NeilBrown
NeilBrown Dec. 12, 2014, 5:27 a.m. UTC | #6
On Thu, 11 Dec 2014 18:32:52 -0500 Peter Hurley <peter@hurleysoftware.com>
wrote:

> On 12/11/2014 04:59 PM, NeilBrown wrote:
> > The regulator is identified in devicetree as 'vdd-supply'
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> tty_* is only for tty core functions/data.

I think you are just objecting to the function and type names - is that
correct?  I suspect I can come up with something different.

Thanks,
NeilBrown
Peter Hurley Dec. 12, 2014, 11:59 a.m. UTC | #7
On 12/12/2014 12:27 AM, NeilBrown wrote:
> On Thu, 11 Dec 2014 18:32:52 -0500 Peter Hurley <peter@hurleysoftware.com>
> wrote:
> 
>> On 12/11/2014 04:59 PM, NeilBrown wrote:
>>> The regulator is identified in devicetree as 'vdd-supply'
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>
>> tty_* is only for tty core functions/data.
> 
> I think you are just objecting to the function and type names - is that
> correct?

Yeah, exactly.

>  I suspect I can come up with something different.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely Dec. 12, 2014, 12:05 p.m. UTC | #8
On Fri, 12 Dec 2014 08:59:44 +1100
, NeilBrown <neilb@suse.de>
 wrote:
> The regulator is identified in devicetree as 'vdd-supply'
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
>  drivers/tty/Kconfig                                |    2 
>  drivers/tty/Makefile                               |    1 
>  drivers/tty/slaves/Kconfig                         |   12 ++
>  drivers/tty/slaves/Makefile                        |    2 
>  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
>  6 files changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
>  create mode 100644 drivers/tty/slaves/Kconfig
>  create mode 100644 drivers/tty/slaves/Makefile
>  create mode 100644 drivers/tty/slaves/tty-reg.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index 000000000000..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device

The binding should be specfic to the device. Trying to do a generic
binding like this doesn't scale well. Do a specific binding for the
bluetooth chipset and make that driver understand how to do PM
operations on that device. That way each kind of tty slave device driver
can have its own set of PM mechanism which may be completely different.

> +This is listed as a child node of a UART.  When the
> +UART is opened, the device is powered.

Another thought, what if I *don't* want the device to be powered
merely because the uart is opened?

> +
> +Example:
> +
> +&uart1 {
> +	bluetooth {
> +		compatible = "tty,regulator";
> +		vdd-supply = <&vaux4>;
> +	};
> +};
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
>  	help
>  	  This enables a console on a Dash channel.
>  
> +source drivers/tty/slaves/Kconfig
> +
>  endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_R3964)		+= n_r3964.o
>  obj-y				+= vt/
>  obj-$(CONFIG_HVC_DRIVER)	+= hvc/
>  obj-y				+= serial/
> +obj-$(CONFIG_TTY_SLAVE)		+= slaves/
>  
>  # tty drivers
>  obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index 000000000000..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> +	bool "TTY slave devices"
> +	help
> +	 Devices which attach via a uart, but need extra
> +	 driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> +	tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index 000000000000..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index 000000000000..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + *   Support for any device which needs a regulator turned on
> + *   when a tty is opened.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/tty.h>
> +
> +struct tty_reg_data {
> +	struct regulator *reg;
> +	bool	reg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +	if (!data->reg_enabled &&
> +	    regulator_enable(data->reg) == 0) {
> +		dev_dbg(slave, "power on\n");
> +		data->reg_enabled = true;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> +	if (data->reg_enabled &&
> +	    regulator_disable(data->reg) == 0) {
> +		dev_dbg(slave, "power off\n");
> +		data->reg_enabled = false;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> +	struct tty_reg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	err = -ENODEV;
> +	if (pdev->dev.parent == NULL)
> +		goto out;
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	err = PTR_ERR(reg);
> +	if (IS_ERR(reg))
> +		goto out;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	err = -ENOMEM;
> +	if (!data)
> +		goto out;
> +	data->reg = reg;
> +	data->reg_enabled = false;
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, data);
> +	err = 0;
> +out:
> +	return err;
> +}
> +
> +static struct of_device_id tty_reg_dt_ids[] = {
> +	{ .compatible = "tty,regulator", },
> +	{}
> +};
> +
> +static const struct dev_pm_ops tty_reg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
> +			   tty_reg_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver tty_reg_driver = {
> +	.driver.name	= "tty-regulator",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.of_match_table = tty_reg_dt_ids,
> +	.driver.pm	= &tty_reg_pm_ops,
> +	.probe		= tty_reg_probe,
> +};

Instead of creating a full driver that binds against the device, would
it not be better to provide a library of stock PM operations that other
device drivers (like the bluetooth driver) can use as-is instead of
defining its own?

> +
> +static int __init tty_reg_init(void)
> +{
> +	return platform_driver_register(&tty_reg_driver);
> +}
> +module_init(tty_reg_init);
> +
> +static void __exit tty_reg_exit(void)
> +{
> +	platform_driver_unregister(&tty_reg_driver);
> +}
> +module_exit(tty_reg_exit);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
> +MODULE_LICENSE("GPL v2");
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
new file mode 100644
index 000000000000..7896bce8dfe4
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
@@ -0,0 +1,18 @@ 
+Regulator powered UART-attached device
+
+Required properties:
+- compatible: "tty,regulator"
+- vdd-supply: regulator to power the device
+
+
+This is listed as a child node of a UART.  When the
+UART is opened, the device is powered.
+
+Example:
+
+&uart1 {
+	bluetooth {
+		compatible = "tty,regulator";
+		vdd-supply = <&vaux4>;
+	};
+};
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index b24aa010f68c..ea3383c71701 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -419,4 +419,6 @@  config DA_CONSOLE
 	help
 	  This enables a console on a Dash channel.
 
+source drivers/tty/slaves/Kconfig
+
 endif # TTY
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index 58ad1c05b7f8..45957d671e33 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -13,6 +13,7 @@  obj-$(CONFIG_R3964)		+= n_r3964.o
 obj-y				+= vt/
 obj-$(CONFIG_HVC_DRIVER)	+= hvc/
 obj-y				+= serial/
+obj-$(CONFIG_TTY_SLAVE)		+= slaves/
 
 # tty drivers
 obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
new file mode 100644
index 000000000000..2dd1acf80f8c
--- /dev/null
+++ b/drivers/tty/slaves/Kconfig
@@ -0,0 +1,12 @@ 
+menuconfig TTY_SLAVE
+	bool "TTY slave devices"
+	help
+	 Devices which attach via a uart, but need extra
+	 driver support for power management etc.
+
+if TTY_SLAVE
+
+config TTY_SLAVE_REGULATOR
+	tristate "TTY slave device powered by regulator"
+
+endif
diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
new file mode 100644
index 000000000000..e636bf49f1cc
--- /dev/null
+++ b/drivers/tty/slaves/Makefile
@@ -0,0 +1,2 @@ 
+
+obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
new file mode 100644
index 000000000000..613f8b10967c
--- /dev/null
+++ b/drivers/tty/slaves/tty-reg.c
@@ -0,0 +1,102 @@ 
+/*
+ * tty-reg:
+ *   Support for any device which needs a regulator turned on
+ *   when a tty is opened.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/tty.h>
+
+struct tty_reg_data {
+	struct regulator *reg;
+	bool	reg_enabled;
+};
+
+static int tty_reg_runtime_resume(struct device *slave)
+{
+	struct tty_reg_data *data = dev_get_drvdata(slave);
+	if (!data->reg_enabled &&
+	    regulator_enable(data->reg) == 0) {
+		dev_dbg(slave, "power on\n");
+		data->reg_enabled = true;
+	}
+	return 0;
+}
+
+static int tty_reg_runtime_suspend(struct device *slave)
+{
+	struct tty_reg_data *data = dev_get_drvdata(slave);
+
+	if (data->reg_enabled &&
+	    regulator_disable(data->reg) == 0) {
+		dev_dbg(slave, "power off\n");
+		data->reg_enabled = false;
+	}
+	return 0;
+}
+
+static int tty_reg_probe(struct platform_device *pdev)
+{
+	struct tty_reg_data *data;
+	struct regulator *reg;
+	int err;
+
+	err = -ENODEV;
+	if (pdev->dev.parent == NULL)
+		goto out;
+	reg = devm_regulator_get(&pdev->dev, "vdd");
+	err = PTR_ERR(reg);
+	if (IS_ERR(reg))
+		goto out;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	err = -ENOMEM;
+	if (!data)
+		goto out;
+	data->reg = reg;
+	data->reg_enabled = false;
+	pm_runtime_enable(&pdev->dev);
+	platform_set_drvdata(pdev, data);
+	err = 0;
+out:
+	return err;
+}
+
+static struct of_device_id tty_reg_dt_ids[] = {
+	{ .compatible = "tty,regulator", },
+	{}
+};
+
+static const struct dev_pm_ops tty_reg_pm_ops = {
+	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
+			   tty_reg_runtime_resume, NULL)
+};
+
+static struct platform_driver tty_reg_driver = {
+	.driver.name	= "tty-regulator",
+	.driver.owner	= THIS_MODULE,
+	.driver.of_match_table = tty_reg_dt_ids,
+	.driver.pm	= &tty_reg_pm_ops,
+	.probe		= tty_reg_probe,
+};
+
+static int __init tty_reg_init(void)
+{
+	return platform_driver_register(&tty_reg_driver);
+}
+module_init(tty_reg_init);
+
+static void __exit tty_reg_exit(void)
+{
+	platform_driver_unregister(&tty_reg_driver);
+}
+module_exit(tty_reg_exit);
+
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
+MODULE_LICENSE("GPL v2");