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

[Sebastian Reichel]

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@xxxxxxx>
> ---
>  .../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@xxxxxxx>");
> +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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Attachment: signature.asc
Description: Digital signature