[PATCH 1/2] regulator: Add regmap support to regulator-fixed device tree binding

[PATCH 1/2] regulator: Add regmap support to regulator-fixed device tree binding

[Simon Arlott]

Add properties for regmap to the regulator-fixed device tree binding:
* Reference the regmap phandle
* Specify the enable offset and enable mask
* Reuse enable-active-high for regmap

This is required for Broadcom BCM63xx SoCs that enable power to
individual peripherals by clearing a bit in the miscIddqCtrl register.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 .../bindings/regulator/fixed-regulator.txt          | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 4fae41d..b499a65 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -5,13 +5,18 @@ Required properties:
 
 Optional properties:
 - gpio: gpio to use for enable control
+- regmap: regmap phandle to use for enable control
+- regmap-offset: register offset when using regmap for enable control
+- regmap-mask: register enable mask when using regmap for enable control
 - startup-delay-us: startup time in microseconds
-- enable-active-high: Polarity of GPIO is Active high
+- enable-active-high: Polarity of GPIO/regmap is Active high
 If this property is missing, the default assumed is Active low.
 - gpio-open-drain: GPIO is open drain type.
   If this property is missing then default assumption is false.
 -vin-supply: Input supply name.
 
+Only one of gpio/regmap should be specified.
+
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
 However a fixed voltage regulator is expected to have the
@@ -32,3 +37,17 @@ Example:
 		gpio-open-drain;
 		vin-supply = <&parent_reg>;
 	};
+
+	xyz: fixedregulator@1 {
+		compatible = "regulator-fixed";
+		regulator-name = "fixed-supply";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regmap = <&syscon>;
+		regmap-offset = <0x4>;
+		regmap-mask = <0x200>;
+		startup-delay-us = <70000>;
+		enable-active-high;
+		regulator-boot-on;
+		vin-supply = <&parent_reg>;
+	};
-- 
2.1.4

-- 
Simon Arlott

[PATCH 2/2] regulator: fixed: Add support for regmap

[Simon Arlott]

Use the device tree properties for regmap:
* Lookup the regmap phandle
* Use the enable offset and enable mask
* Reuse enable-active-high value

Use an alternative set of regulator_ops when the regmap is set,
specifying the regmap helper functions.

As syscon_regmap_lookup_by_phandle() can only return -ENODEV or -ENOSYS
as errors and -EPROBE_DEFER doesn't make sense for a register, the
specific error is never propagated.

This is required for Broadcom BCM63xx SoCs that enable power to
individual peripherals by clearing a bit in the miscIddqCtrl register.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/regulator/fixed.c       | 30 +++++++++++++++++++++++++++++-
 include/linux/regulator/fixed.h |  9 ++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index ff62d69..5a3fa44 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -30,6 +30,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/machine.h>
+#include <linux/mfd/syscon.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;
@@ -92,6 +93,19 @@ of_get_fixed_voltage_config(struct device *dev,
 	if ((config->gpio == -ENODEV) || (config->gpio == -EPROBE_DEFER))
 		return ERR_PTR(-EPROBE_DEFER);
 
+	config->regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
+	if (!IS_ERR(config->regmap)) {
+		u32 val;
+
+		if (of_property_read_u32(np, "regmap-offset", &val))
+			return ERR_PTR(-EINVAL);
+		config->enable_reg = val;
+
+		if (of_property_read_u32(np, "regmap-mask", &val))
+			return ERR_PTR(-EINVAL);
+		config->enable_mask = val;
+	}
+
 	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
 
 	config->enable_high = of_property_read_bool(np, "enable-active-high");
@@ -107,6 +121,12 @@ of_get_fixed_voltage_config(struct device *dev,
 static struct regulator_ops fixed_voltage_ops = {
 };
 
+static struct regulator_ops fixed_voltage_regmap_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+};
+
 static int reg_fixed_voltage_probe(struct platform_device *pdev)
 {
 	struct fixed_voltage_config *config;
@@ -140,7 +160,15 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	}
 	drvdata->desc.type = REGULATOR_VOLTAGE;
 	drvdata->desc.owner = THIS_MODULE;
-	drvdata->desc.ops = &fixed_voltage_ops;
+	if (!IS_ERR_OR_NULL(config->regmap)) {
+		drvdata->desc.ops = &fixed_voltage_regmap_ops;
+		cfg.regmap = config->regmap;
+		drvdata->desc.enable_reg = config->enable_reg;
+		drvdata->desc.enable_mask = config->enable_mask;
+		drvdata->desc.enable_is_inverted = !config->enable_high;
+	} else {
+		drvdata->desc.ops = &fixed_voltage_ops;
+	}
 
 	drvdata->desc.enable_time = config->startup_delay;
 
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index 48918be..7cec0d1 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -26,6 +26,10 @@ struct regulator_init_data;
  * @microvolts:		Output voltage of regulator
  * @gpio:		GPIO to use for enable control
  * 			set to -EINVAL if not used
+ * @regmap:		Regmap to use for enable control
+ *			set to -ENODEV if not used
+ * @enable_reg:		Register for control when using regmap
+ * @enable_mask:	Mask for control when using regmap
  * @startup_delay:	Start-up time in microseconds
  * @gpio_is_open_drain: Gpio pin is open drain or normal type.
  *			If it is open drain type then HIGH will be set
@@ -33,7 +37,7 @@ struct regulator_init_data;
  *			and low will be set as gpio-output with driven
  *			to low. For non-open-drain case, the gpio will
  *			will be in output and drive to low/high accordingly.
- * @enable_high:	Polarity of enable GPIO
+ * @enable_high:	Polarity of enable GPIO/regmap
  *			1 = Active high, 0 = Active low
  * @enabled_at_boot:	Whether regulator has been enabled at
  * 			boot or not. 1 = Yes, 0 = No
@@ -50,6 +54,9 @@ struct fixed_voltage_config {
 	const char *input_supply;
 	int microvolts;
 	int gpio;
+	struct regmap *regmap;
+	unsigned int enable_reg;
+	unsigned int enable_mask;
 	unsigned startup_delay;
 	unsigned gpio_is_open_drain:1;
 	unsigned enable_high:1;
-- 
2.1.4

-- 
Simon Arlott

[PATCH 2/2] regulator: fixed: Add support for regmap

[Simon Arlott]

Use the device tree properties for regmap:
* Lookup the regmap phandle
* Use the enable offset and enable mask
* Reuse enable-active-high value

Use an alternative set of regulator_ops when the regmap is set,
specifying the regmap helper functions.

As syscon_regmap_lookup_by_phandle() can only return -ENODEV or -ENOSYS
as errors and -EPROBE_DEFER doesn't make sense for a register, the
specific error is never propagated.

This is required for Broadcom BCM63xx SoCs that enable power to
individual peripherals by clearing a bit in the miscIddqCtrl register.

Signed-off-by: Simon Arlott <simon-A6De1vDTPLDsq35pWSNszA@public.gmane.org>
---
 drivers/regulator/fixed.c       | 30 +++++++++++++++++++++++++++++-
 include/linux/regulator/fixed.h |  9 ++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index ff62d69..5a3fa44 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -30,6 +30,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/machine.h>
+#include <linux/mfd/syscon.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;
@@ -92,6 +93,19 @@ of_get_fixed_voltage_config(struct device *dev,
 	if ((config->gpio == -ENODEV) || (config->gpio == -EPROBE_DEFER))
 		return ERR_PTR(-EPROBE_DEFER);
 
+	config->regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
+	if (!IS_ERR(config->regmap)) {
+		u32 val;
+
+		if (of_property_read_u32(np, "regmap-offset", &val))
+			return ERR_PTR(-EINVAL);
+		config->enable_reg = val;
+
+		if (of_property_read_u32(np, "regmap-mask", &val))
+			return ERR_PTR(-EINVAL);
+		config->enable_mask = val;
+	}
+
 	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
 
 	config->enable_high = of_property_read_bool(np, "enable-active-high");
@@ -107,6 +121,12 @@ of_get_fixed_voltage_config(struct device *dev,
 static struct regulator_ops fixed_voltage_ops = {
 };
 
+static struct regulator_ops fixed_voltage_regmap_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+};
+
 static int reg_fixed_voltage_probe(struct platform_device *pdev)
 {
 	struct fixed_voltage_config *config;
@@ -140,7 +160,15 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	}
 	drvdata->desc.type = REGULATOR_VOLTAGE;
 	drvdata->desc.owner = THIS_MODULE;
-	drvdata->desc.ops = &fixed_voltage_ops;
+	if (!IS_ERR_OR_NULL(config->regmap)) {
+		drvdata->desc.ops = &fixed_voltage_regmap_ops;
+		cfg.regmap = config->regmap;
+		drvdata->desc.enable_reg = config->enable_reg;
+		drvdata->desc.enable_mask = config->enable_mask;
+		drvdata->desc.enable_is_inverted = !config->enable_high;
+	} else {
+		drvdata->desc.ops = &fixed_voltage_ops;
+	}
 
 	drvdata->desc.enable_time = config->startup_delay;
 
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index 48918be..7cec0d1 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -26,6 +26,10 @@ struct regulator_init_data;
  * @microvolts:		Output voltage of regulator
  * @gpio:		GPIO to use for enable control
  * 			set to -EINVAL if not used
+ * @regmap:		Regmap to use for enable control
+ *			set to -ENODEV if not used
+ * @enable_reg:		Register for control when using regmap
+ * @enable_mask:	Mask for control when using regmap
  * @startup_delay:	Start-up time in microseconds
  * @gpio_is_open_drain: Gpio pin is open drain or normal type.
  *			If it is open drain type then HIGH will be set
@@ -33,7 +37,7 @@ struct regulator_init_data;
  *			and low will be set as gpio-output with driven
  *			to low. For non-open-drain case, the gpio will
  *			will be in output and drive to low/high accordingly.
- * @enable_high:	Polarity of enable GPIO
+ * @enable_high:	Polarity of enable GPIO/regmap
  *			1 = Active high, 0 = Active low
  * @enabled_at_boot:	Whether regulator has been enabled at
  * 			boot or not. 1 = Yes, 0 = No
@@ -50,6 +54,9 @@ struct fixed_voltage_config {
 	const char *input_supply;
 	int microvolts;
 	int gpio;
+	struct regmap *regmap;
+	unsigned int enable_reg;
+	unsigned int enable_mask;
 	unsigned startup_delay;
 	unsigned gpio_is_open_drain:1;
 	unsigned enable_high:1;
-- 
2.1.4

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

Re: [PATCH 1/2] regulator: Add regmap support to regulator-fixed device tree binding

[Mark Brown]

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

On Sat, Nov 28, 2015 at 09:13:01PM +0000, Simon Arlott wrote:
> Add properties for regmap to the regulator-fixed device tree binding:
> * Reference the regmap phandle
> * Specify the enable offset and enable mask
> * Reuse enable-active-high for regmap
> 
> This is required for Broadcom BCM63xx SoCs that enable power to
> individual peripherals by clearing a bit in the miscIddqCtrl register.

Just write a driver for this, the fixed driver is really intended for
things like discrete regulators and if we were going to do a generic
regmap driver (which I'm not terribly keen on as it means we're not able
to later add additional information or custom handling in the driver) we
should just do that rather than shoehorning things into the fixed driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add regmap support to regulator-fixed device tree binding

[Mark Brown]

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

On Sat, Nov 28, 2015 at 09:13:01PM +0000, Simon Arlott wrote:
> Add properties for regmap to the regulator-fixed device tree binding:
> * Reference the regmap phandle
> * Specify the enable offset and enable mask
> * Reuse enable-active-high for regmap
> 
> This is required for Broadcom BCM63xx SoCs that enable power to
> individual peripherals by clearing a bit in the miscIddqCtrl register.

Just write a driver for this, the fixed driver is really intended for
things like discrete regulators and if we were going to do a generic
regmap driver (which I'm not terribly keen on as it means we're not able
to later add additional information or custom handling in the driver) we
should just do that rather than shoehorning things into the fixed driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

The BCM63xx has one or more registers with bits that act as regulators
to enable/disable power to individual chip peripherals.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 .../bindings/regulator/brcm,bcm63xx-regulator.txt  | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt b/Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt
new file mode 100644
index 0000000..e905241
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt
@@ -0,0 +1,33 @@
+BCM63xx regulators
+
+The BCM63xx has one or more registers with bits that act as regulators
+to enable/disable power to individual chip peripherals.
+
+Required properties:
+- compatible: Must be "brcm,bcm<soc>-regulator", "brcm,bcm63xx-regulator";
+- regmap: regmap phandle to use for enable control
+- offset: register offset
+- mask: register enable mask
+- startup-delay-us: startup time in microseconds
+
+Any property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+However the regulator is expected to have the same values
+for regulator-min-microvolt and regulator-max-microvolt.
+
+Example:
+
+misc: syscon@10001800 {
+	compatible = "syscon";
+	reg = <0x10001800 0xd0>;
+};
+
+robosw_power: robosw {
+	compatible = "brcm,bcm63168-regulator", "brcm,bcm63xx-regulator";
+	regulator-name = "robosw_power";
+	regmap = <&misc>;
+	offset = <0x4c>;
+	mask = <0x40>;
+	startup-delay-us = <100000>;
+};
-- 
2.1.4

-- 
Simon Arlott

[PATCH 2/2] regulator: bcm63xx: Add BCM63xx fixed regulator device

[Simon Arlott]

The BCM63xx has one or more registers with bits that act as regulators
to enable/disable power to individual chip peripherals.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 MAINTAINERS                           |   1 +
 drivers/regulator/Kconfig             |   9 +++
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/bcm63xx-regulator.c | 125 ++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 drivers/regulator/bcm63xx-regulator.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a381176..29e0589 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2376,6 +2376,7 @@ F:	arch/mips/boot/dts/brcm/bcm*.dts*
 F:	drivers/irqchip/irq-bcm63*
 F:	drivers/irqchip/irq-bcm7*
 F:	drivers/irqchip/irq-brcmstb*
+F:	drivers/regulator/bcm63*
 F:	include/linux/bcm63xx_wdt.h
 
 BROADCOM TG3 GIGABIT ETHERNET DRIVER
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8df0b0e..8a2d5db 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -146,6 +146,15 @@ config REGULATOR_AXP20X
 	  This driver provides support for the voltage regulators on the
 	  AXP20X PMIC.
 
+config REGULATOR_BCM63XX
+	tristate "Broadcom BCM63xx Regulators"
+	depends on OF
+	depends on MFD_SYSCON
+	depends on BMIPS_GENERIC
+	help
+	  This driver provides support for the fixed regulators on the
+	  BCM63xx SoCs.
+
 config REGULATOR_BCM590XX
 	tristate "Broadcom BCM590xx PMU Regulators"
 	depends on MFD_BCM590XX
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0f81749..23e8a4b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_REGULATOR_ARIZONA) += arizona-micsupp.o arizona-ldo1.o
 obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
 obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
 obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
+obj-$(CONFIG_REGULATOR_BCM63XX) += bcm63xx-regulator.o
 obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
 obj-$(CONFIG_REGULATOR_DA9052)	+= da9052-regulator.o
diff --git a/drivers/regulator/bcm63xx-regulator.c b/drivers/regulator/bcm63xx-regulator.c
new file mode 100644
index 0000000..7f9afad
--- /dev/null
+++ b/drivers/regulator/bcm63xx-regulator.c
@@ -0,0 +1,125 @@
+/*
+ * Copyright 2015 Simon Arlott
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * Derived from fixed.c:
+ * Copyright 2008 Wolfson Microelectronics PLC.
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ * Copyright (c) 2009 Nokia Corporation
+ * Roger Quadros <ext-roger.quadros@nokia.com>
+ */
+
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/syscon.h>
+
+static struct regulator_ops bcm63xx_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+};
+
+static int __init bcm63xx_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regulator_desc *desc;
+	struct regulator_init_data *init_data;
+	struct regulator_config cfg = { };
+	struct regulator_dev *reg_dev;
+	u32 val;
+	int ret;
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	init_data = of_get_regulator_init_data(dev, np, desc);
+	if (!init_data) {
+		dev_err(dev, "Failed to get regulator init data\n");
+		return -EINVAL;
+	}
+
+	/* Only status change is supported, regardless of DT init data */
+	init_data->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
+
+	desc->name = devm_kstrdup(dev, init_data->constraints.name, GFP_KERNEL);
+	if (desc->name == NULL)
+		return -ENOMEM;
+	desc->type = REGULATOR_VOLTAGE;
+	desc->owner = THIS_MODULE;
+	desc->ops = &bcm63xx_regulator_ops;
+
+	/* Clearing a bit in the register enables power */
+	desc->enable_is_inverted = true;
+
+	cfg.dev = dev;
+	cfg.init_data = init_data;
+	cfg.of_node = np;
+
+	cfg.regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
+	if (IS_ERR(cfg.regmap)) {
+		ret = PTR_ERR(cfg.regmap);
+		dev_err(dev, "Failed to get regmap for %s: %d\n",
+			desc->name, ret);
+		return ret;
+	}
+
+	if (of_property_read_u32(np, "offset", &val)) {
+		dev_err(dev, "Missing register offset for %s\n", desc->name);
+		return -EINVAL;
+	}
+	desc->enable_reg = val;
+
+	if (of_property_read_u32(np, "mask", &val)) {
+		dev_err(dev, "Missing register mask for %s\n", desc->name);
+		return -EINVAL;
+	}
+	desc->enable_mask = val;
+
+	if (!of_property_read_u32(np, "startup-delay-us", &val))
+		desc->enable_time = val;
+
+	reg_dev = devm_regulator_register(dev, desc, &cfg);
+	if (IS_ERR(reg_dev)) {
+		ret = PTR_ERR(reg_dev);
+		dev_err(dev, "Failed to register regulator %s: %d\n",
+			desc->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm63xx_regulator_of_match[] __initconst = {
+	{ .compatible = "brcm,bcm63xx-regulator", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm63xx_regulator_of_match);
+
+static struct platform_driver bcm63xx_regulator_driver __refdata = {
+	.probe		= bcm63xx_regulator_probe,
+	.driver		= {
+		.name		= "bcm63xx-regulator",
+		.of_match_table = bcm63xx_regulator_of_match,
+	},
+};
+
+module_platform_driver(bcm63xx_regulator_driver);
+
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("BCM63xx regulator");
+MODULE_LICENSE("GPL");
-- 
2.1.4

-- 
Simon Arlott

[PATCH (v2) 2/2] regulator: bcm63xx: Add BCM63xx fixed regulator device

[Simon Arlott]

The BCM63xx has one or more registers with bits that act as regulators
to enable/disable power to individual chip peripherals.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
On 30/11/15 20:30, Simon Arlott wrote:
> +	/* Only status change is supported, regardless of DT init data */
> +	init_data->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;

Sorry, I thought I had tested it again after adding that line.

 MAINTAINERS                           |   1 +
 drivers/regulator/Kconfig             |   9 +++
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/bcm63xx-regulator.c | 125 ++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 drivers/regulator/bcm63xx-regulator.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a381176..29e0589 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2376,6 +2376,7 @@ F:	arch/mips/boot/dts/brcm/bcm*.dts*
 F:	drivers/irqchip/irq-bcm63*
 F:	drivers/irqchip/irq-bcm7*
 F:	drivers/irqchip/irq-brcmstb*
+F:	drivers/regulator/bcm63*
 F:	include/linux/bcm63xx_wdt.h
 
 BROADCOM TG3 GIGABIT ETHERNET DRIVER
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8df0b0e..8a2d5db 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -146,6 +146,15 @@ config REGULATOR_AXP20X
 	  This driver provides support for the voltage regulators on the
 	  AXP20X PMIC.
 
+config REGULATOR_BCM63XX
+	tristate "Broadcom BCM63xx Regulators"
+	depends on OF
+	depends on MFD_SYSCON
+	depends on BMIPS_GENERIC
+	help
+	  This driver provides support for the fixed regulators on the
+	  BCM63xx SoCs.
+
 config REGULATOR_BCM590XX
 	tristate "Broadcom BCM590xx PMU Regulators"
 	depends on MFD_BCM590XX
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0f81749..23e8a4b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_REGULATOR_ARIZONA) += arizona-micsupp.o arizona-ldo1.o
 obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
 obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
 obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
+obj-$(CONFIG_REGULATOR_BCM63XX) += bcm63xx-regulator.o
 obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
 obj-$(CONFIG_REGULATOR_DA9052)	+= da9052-regulator.o
diff --git a/drivers/regulator/bcm63xx-regulator.c b/drivers/regulator/bcm63xx-regulator.c
new file mode 100644
index 0000000..13a0fbc
--- /dev/null
+++ b/drivers/regulator/bcm63xx-regulator.c
@@ -0,0 +1,125 @@
+/*
+ * Copyright 2015 Simon Arlott
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * Derived from fixed.c:
+ * Copyright 2008 Wolfson Microelectronics PLC.
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ * Copyright (c) 2009 Nokia Corporation
+ * Roger Quadros <ext-roger.quadros@nokia.com>
+ */
+
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/syscon.h>
+
+static struct regulator_ops bcm63xx_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+};
+
+static int __init bcm63xx_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regulator_desc *desc;
+	struct regulator_init_data *init_data;
+	struct regulator_config cfg = { };
+	struct regulator_dev *reg_dev;
+	u32 val;
+	int ret;
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	init_data = of_get_regulator_init_data(dev, np, desc);
+	if (!init_data) {
+		dev_err(dev, "Failed to get regulator init data\n");
+		return -EINVAL;
+	}
+
+	/* Only status change is supported, regardless of DT init data */
+	init_data->constraints.valid_ops_mask &= REGULATOR_CHANGE_STATUS;
+
+	desc->name = devm_kstrdup(dev, init_data->constraints.name, GFP_KERNEL);
+	if (desc->name == NULL)
+		return -ENOMEM;
+	desc->type = REGULATOR_VOLTAGE;
+	desc->owner = THIS_MODULE;
+	desc->ops = &bcm63xx_regulator_ops;
+
+	/* Clearing a bit in the register enables power */
+	desc->enable_is_inverted = true;
+
+	cfg.dev = dev;
+	cfg.init_data = init_data;
+	cfg.of_node = np;
+
+	cfg.regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
+	if (IS_ERR(cfg.regmap)) {
+		ret = PTR_ERR(cfg.regmap);
+		dev_err(dev, "Failed to get regmap for %s: %d\n",
+			desc->name, ret);
+		return ret;
+	}
+
+	if (of_property_read_u32(np, "offset", &val)) {
+		dev_err(dev, "Missing register offset for %s\n", desc->name);
+		return -EINVAL;
+	}
+	desc->enable_reg = val;
+
+	if (of_property_read_u32(np, "mask", &val)) {
+		dev_err(dev, "Missing register mask for %s\n", desc->name);
+		return -EINVAL;
+	}
+	desc->enable_mask = val;
+
+	if (!of_property_read_u32(np, "startup-delay-us", &val))
+		desc->enable_time = val;
+
+	reg_dev = devm_regulator_register(dev, desc, &cfg);
+	if (IS_ERR(reg_dev)) {
+		ret = PTR_ERR(reg_dev);
+		dev_err(dev, "Failed to register regulator %s: %d\n",
+			desc->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm63xx_regulator_of_match[] __initconst = {
+	{ .compatible = "brcm,bcm63xx-regulator", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm63xx_regulator_of_match);
+
+static struct platform_driver bcm63xx_regulator_driver __refdata = {
+	.probe		= bcm63xx_regulator_probe,
+	.driver		= {
+		.name		= "bcm63xx-regulator",
+		.of_match_table = bcm63xx_regulator_of_match,
+	},
+};
+
+module_platform_driver(bcm63xx_regulator_driver);
+
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("BCM63xx regulator");
+MODULE_LICENSE("GPL");
-- 
2.1.4

-- 
Simon Arlott

Re: [PATCH (v2) 2/2] regulator: bcm63xx: Add BCM63xx fixed regulator device

[Mark Brown]

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

On Mon, Nov 30, 2015 at 08:38:30PM +0000, Simon Arlott wrote:
> The BCM63xx has one or more registers with bits that act as regulators
> to enable/disable power to individual chip peripherals.

Please don't submit new revisions of single patches in followup to an
existing series, it just makes everything more confusing.  If you need
to update a series please just post an update to the series.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH (v2) 2/2] regulator: bcm63xx: Add BCM63xx fixed regulator device

[Mark Brown]

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

On Mon, Nov 30, 2015 at 08:38:30PM +0000, Simon Arlott wrote:
> The BCM63xx has one or more registers with bits that act as regulators
> to enable/disable power to individual chip peripherals.

Please don't submit new revisions of single patches in followup to an
existing series, it just makes everything more confusing.  If you need
to update a series please just post an update to the series.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Mon, Nov 30, 2015 at 08:30:07PM +0000, Simon Arlott wrote:

> +- offset: register offset
> +- mask: register enable mask
> +- startup-delay-us: startup time in microseconds

Why are these in the DT, I would expect that if this is a driver for a
specific SoC all these properties would be known as a result of that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Mon, Nov 30, 2015 at 08:30:07PM +0000, Simon Arlott wrote:

> +- offset: register offset
> +- mask: register enable mask
> +- startup-delay-us: startup time in microseconds

Why are these in the DT, I would expect that if this is a driver for a
specific SoC all these properties would be known as a result of that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On Tue, December 1, 2015 22:16, Mark Brown wrote:
> On Mon, Nov 30, 2015 at 08:30:07PM +0000, Simon Arlott wrote:
>
>> +- offset: register offset
>> +- mask: register enable mask
>> +- startup-delay-us: startup time in microseconds
>
> Why are these in the DT, I would expect that if this is a driver for a
> specific SoC all these properties would be known as a result of that.

This is a driver for multiple SoCs with the same regulator control in
different places on different SoCs, so the location of it within the misc
register needs to be provided in the DT:

BCM6362:
  #define MISC_BASE 0xb0001800 /* Miscellaneous Registers */
  uint32 miscIddqCtrl; /* 0x48 */

BCM63268/BCM63168:
  #define MISC_BASE 0xb0001800 /* Miscellaneous Registers */
  uint32 miscIddqCtrl; /* 0x4c */


I'll remove the startup-delay-us property and put a default in the driver.


The mask is used as there's one bit per regulator in the register, but
there's more than one way to express this in the DT:

It currently looks like this:
  regulators {
    compatible = "simple-bus";
    #address-cells = <0>;
    #size-cells = <0>;

    sar {
      compatible = "brcm,bcm63168-regulator", "brcm,bcm6345-regulator";
      regulator-name = "sar_power";
      regmap = <&misc>;
      offset = <0x4c>;
      mask = <0x1>;
      startup-delay-us = <100000>;
    };

    ipsec {
      compatible = "brcm,bcm63168-regulator", "brcm,bcm6345-regulator";
      regulator-name = "ipsec_power";
      regmap = <&misc>;
      offset = <0x4c>;
      mask = <0x2>;
      startup-delay-us = <100000>;
    };

    ...
  };

  ubus {
    compatible = "brcm,ubus", "simple-bus";
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    misc: syscon@10001800 {
      compatible = "syscon";
      reg = <0x10001800 0xd0>;
    };
  };

Would this be more appropriate?
  regulators {
    compatible = "simple-bus";
    #address-cells = <0>;
    #size-cells = <0>;

    misc_iddq_ctrl {
      compatible = "brcm,bcm63168-regulator", "brcm,bcm6345-regulator";
      regmap = <&misc>;
      offset = <0x4c>;
      #address-cells = <1>;
      #size-cells = <0>;

      sar@0 {
        regulator-name = "sar_power";
      };

      ipsec@1 {
        regulator-name = "ipsec_power";
      };

      ...
    };
  };

  ubus {
    compatible = "brcm,ubus", "simple-bus";
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    misc: syscon@10001800 {
      compatible = "syscon";
      reg = <0x10001800 0xd0>;
    };
  };

-- 
Simon Arlott

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Wed, Dec 02, 2015 at 12:45:50PM -0000, Simon Arlott wrote:
> On Tue, December 1, 2015 22:16, Mark Brown wrote:

> > Why are these in the DT, I would expect that if this is a driver for a
> > specific SoC all these properties would be known as a result of that.

> This is a driver for multiple SoCs with the same regulator control in
> different places on different SoCs, so the location of it within the misc
> register needs to be provided in the DT:

> BCM6362:
>   #define MISC_BASE 0xb0001800 /* Miscellaneous Registers */
>   uint32 miscIddqCtrl; /* 0x48 */

This is the sort of thing you can pick up from the SoC compatible
strings.  As things stand there is zero content in this driver that
relates to this SoC.

> The mask is used as there's one bit per regulator in the register, but
> there's more than one way to express this in the DT:

I wouldn't expect to see it in the device tree at all for a device
specific driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 02/12/15 12:53, Mark Brown wrote:
> On Wed, Dec 02, 2015 at 12:45:50PM -0000, Simon Arlott wrote:
>> On Tue, December 1, 2015 22:16, Mark Brown wrote:
> 
>> > Why are these in the DT, I would expect that if this is a driver for a
>> > specific SoC all these properties would be known as a result of that.
> 
>> This is a driver for multiple SoCs with the same regulator control in
>> different places on different SoCs, so the location of it within the misc
>> register needs to be provided in the DT:
> 
>> BCM6362:
>>   #define MISC_BASE 0xb0001800 /* Miscellaneous Registers */
>>   uint32 miscIddqCtrl; /* 0x48 */
> 
> This is the sort of thing you can pick up from the SoC compatible
> strings.  As things stand there is zero content in this driver that
> relates to this SoC.

There's always going to be very little content in the driver that
relates to this SoC, given that a single bit flip enables/disables
power.

All other device tree drivers allow a register address to be specified
for the device, how is an offset in the regmap any different?

>> The mask is used as there's one bit per regulator in the register, but
>> there's more than one way to express this in the DT:
> 
> I wouldn't expect to see it in the device tree at all for a device
> specific driver.

If there isn't an individual entry in DT for each regulator, how is it
supposed to work? There's no #regulator-cells property.

-- 
Simon Arlott

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 02/12/15 12:53, Mark Brown wrote:
> On Wed, Dec 02, 2015 at 12:45:50PM -0000, Simon Arlott wrote:
>> On Tue, December 1, 2015 22:16, Mark Brown wrote:
> 
>> > Why are these in the DT, I would expect that if this is a driver for a
>> > specific SoC all these properties would be known as a result of that.
> 
>> This is a driver for multiple SoCs with the same regulator control in
>> different places on different SoCs, so the location of it within the misc
>> register needs to be provided in the DT:
> 
>> BCM6362:
>>   #define MISC_BASE 0xb0001800 /* Miscellaneous Registers */
>>   uint32 miscIddqCtrl; /* 0x48 */
> 
> This is the sort of thing you can pick up from the SoC compatible
> strings.  As things stand there is zero content in this driver that
> relates to this SoC.

There's always going to be very little content in the driver that
relates to this SoC, given that a single bit flip enables/disables
power.

All other device tree drivers allow a register address to be specified
for the device, how is an offset in the regmap any different?

>> The mask is used as there's one bit per regulator in the register, but
>> there's more than one way to express this in the DT:
> 
> I wouldn't expect to see it in the device tree at all for a device
> specific driver.

If there isn't an individual entry in DT for each regulator, how is it
supposed to work? There's no #regulator-cells property.

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

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Wed, Dec 02, 2015 at 08:26:36PM +0000, Simon Arlott wrote:
> On 02/12/15 12:53, Mark Brown wrote:

> > This is the sort of thing you can pick up from the SoC compatible
> > strings.  As things stand there is zero content in this driver that
> > relates to this SoC.

> There's always going to be very little content in the driver that
> relates to this SoC, given that a single bit flip enables/disables
> power.

> All other device tree drivers allow a register address to be specified
> for the device, how is an offset in the regmap any different?

It's not that it's an offset in regmap, it's that you're trying to make
a device driver with literally *no* content that is specific to the
actual device.  If you're making a driver for a specific device like
this it should know at least something about how to control the device
from the compatible string.  If you're making a generic driver it should
not make reference to specific devices.

In general I would prefer to have a driver with a SoC specific
compatible string and the data tables in the kernel, that way we keep
the device trees stable and our ABI more robust.

> >> The mask is used as there's one bit per regulator in the register, but
> >> there's more than one way to express this in the DT:

> > I wouldn't expect to see it in the device tree at all for a device
> > specific driver.

> If there isn't an individual entry in DT for each regulator, how is it
> supposed to work? There's no #regulator-cells property.

There could be one if it would help, but we do normally manage to do
this without - look at how other regulator drivers work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 03/12/15 00:06, Mark Brown wrote:
> On Wed, Dec 02, 2015 at 08:26:36PM +0000, Simon Arlott wrote:
>> On 02/12/15 12:53, Mark Brown wrote:
> 
>> > This is the sort of thing you can pick up from the SoC compatible
>> > strings.  As things stand there is zero content in this driver that
>> > relates to this SoC.
> 
>> There's always going to be very little content in the driver that
>> relates to this SoC, given that a single bit flip enables/disables
>> power.
> 
>> All other device tree drivers allow a register address to be specified
>> for the device, how is an offset in the regmap any different?
> 
> It's not that it's an offset in regmap, it's that you're trying to make
> a device driver with literally *no* content that is specific to the
> actual device.  If you're making a driver for a specific device like

The device that only has a single register, I don't want to make it
specific to the device by hard coding a register address in the driver
that changes when the same device is used on different SoCs.

> this it should know at least something about how to control the device
> from the compatible string.  If you're making a generic driver it should
> not make reference to specific devices.

Will you accept a generic driver for a simple enable regulator device on
a regmap? What should its compatible string be?

> In general I would prefer to have a driver with a SoC specific
> compatible string and the data tables in the kernel, that way we keep
> the device trees stable and our ABI more robust.
> 
>> >> The mask is used as there's one bit per regulator in the register, but
>> >> there's more than one way to express this in the DT:
> 
>> > I wouldn't expect to see it in the device tree at all for a device
>> > specific driver.
> 
>> If there isn't an individual entry in DT for each regulator, how is it
>> supposed to work? There's no #regulator-cells property.
> 
> There could be one if it would help, but we do normally manage to do
> this without - look at how other regulator drivers work.

Several of them have a fixed list of supported regulator names in the
driver. The regulator names for this device are meaningless to the
driver because all of the regulators look the same. They don't have a
known or controllable voltage and can only be turned on or off.

Any table mapping regulator names to bits in the register would be
different for each SoC making the list of regulator names in the device
tree redundant. If they're not listed in the device tree then I can't
use them as a phandle for other devices.

-- 
Simon Arlott

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 03/12/15 00:06, Mark Brown wrote:
> On Wed, Dec 02, 2015 at 08:26:36PM +0000, Simon Arlott wrote:
>> On 02/12/15 12:53, Mark Brown wrote:
> 
>> > This is the sort of thing you can pick up from the SoC compatible
>> > strings.  As things stand there is zero content in this driver that
>> > relates to this SoC.
> 
>> There's always going to be very little content in the driver that
>> relates to this SoC, given that a single bit flip enables/disables
>> power.
> 
>> All other device tree drivers allow a register address to be specified
>> for the device, how is an offset in the regmap any different?
> 
> It's not that it's an offset in regmap, it's that you're trying to make
> a device driver with literally *no* content that is specific to the
> actual device.  If you're making a driver for a specific device like

The device that only has a single register, I don't want to make it
specific to the device by hard coding a register address in the driver
that changes when the same device is used on different SoCs.

> this it should know at least something about how to control the device
> from the compatible string.  If you're making a generic driver it should
> not make reference to specific devices.

Will you accept a generic driver for a simple enable regulator device on
a regmap? What should its compatible string be?

> In general I would prefer to have a driver with a SoC specific
> compatible string and the data tables in the kernel, that way we keep
> the device trees stable and our ABI more robust.
> 
>> >> The mask is used as there's one bit per regulator in the register, but
>> >> there's more than one way to express this in the DT:
> 
>> > I wouldn't expect to see it in the device tree at all for a device
>> > specific driver.
> 
>> If there isn't an individual entry in DT for each regulator, how is it
>> supposed to work? There's no #regulator-cells property.
> 
> There could be one if it would help, but we do normally manage to do
> this without - look at how other regulator drivers work.

Several of them have a fixed list of supported regulator names in the
driver. The regulator names for this device are meaningless to the
driver because all of the regulators look the same. They don't have a
known or controllable voltage and can only be turned on or off.

Any table mapping regulator names to bits in the register would be
different for each SoC making the list of regulator names in the device
tree redundant. If they're not listed in the device tree then I can't
use them as a phandle for other devices.

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

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Thu, Dec 03, 2015 at 08:14:33AM +0000, Simon Arlott wrote:
> On 03/12/15 00:06, Mark Brown wrote:

> > this it should know at least something about how to control the device
> > from the compatible string.  If you're making a generic driver it should
> > not make reference to specific devices.

> Will you accept a generic driver for a simple enable regulator device on
> a regmap? What should its compatible string be?

Perhaps.  I really don't like putting the entire driver into DT, it's
not a pattern I want to encourage.

> > There could be one if it would help, but we do normally manage to do
> > this without - look at how other regulator drivers work.

> Several of them have a fixed list of supported regulator names in the

Yes, that's the way this is handled.

> driver. The regulator names for this device are meaningless to the
> driver because all of the regulators look the same. They don't have a
> known or controllable voltage and can only be turned on or off.

Nonsense.  The names are useful to identify which supply is being
referred to.  Having voltage control is irrelevant to identifying
regulators.

> Any table mapping regulator names to bits in the register would be
> different for each SoC making the list of regulator names in the device
> tree redundant. If they're not listed in the device tree then I can't
> use them as a phandle for other devices.

The list of regulator nodes in device tree is not redundant, it is as
you say used to connect things together.  The question is where to put
the control data for those regulators (in this case the enable time and
the switch).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Thu, Dec 03, 2015 at 08:14:33AM +0000, Simon Arlott wrote:
> On 03/12/15 00:06, Mark Brown wrote:

> > this it should know at least something about how to control the device
> > from the compatible string.  If you're making a generic driver it should
> > not make reference to specific devices.

> Will you accept a generic driver for a simple enable regulator device on
> a regmap? What should its compatible string be?

Perhaps.  I really don't like putting the entire driver into DT, it's
not a pattern I want to encourage.

> > There could be one if it would help, but we do normally manage to do
> > this without - look at how other regulator drivers work.

> Several of them have a fixed list of supported regulator names in the

Yes, that's the way this is handled.

> driver. The regulator names for this device are meaningless to the
> driver because all of the regulators look the same. They don't have a
> known or controllable voltage and can only be turned on or off.

Nonsense.  The names are useful to identify which supply is being
referred to.  Having voltage control is irrelevant to identifying
regulators.

> Any table mapping regulator names to bits in the register would be
> different for each SoC making the list of regulator names in the device
> tree redundant. If they're not listed in the device tree then I can't
> use them as a phandle for other devices.

The list of regulator nodes in device tree is not redundant, it is as
you say used to connect things together.  The question is where to put
the control data for those regulators (in this case the enable time and
the switch).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 03/12/15 15:05, Mark Brown wrote:
> On Thu, Dec 03, 2015 at 08:14:33AM +0000, Simon Arlott wrote:
>> On 03/12/15 00:06, Mark Brown wrote:
> 
>> > this it should know at least something about how to control the device
>> > from the compatible string.  If you're making a generic driver it should
>> > not make reference to specific devices.
> 
>> Will you accept a generic driver for a simple enable regulator device on
>> a regmap? What should its compatible string be?
> 
> Perhaps.  I really don't like putting the entire driver into DT, it's
> not a pattern I want to encourage.

I don't like putting the list of which bit is which regulator name into
a driver because it means that new hardware that adds a regulator or
moves them around requires a change to the driver.

Documentation/devicetree/usage-model.txt:
  2.1 High Level View
  -------------------
  [...] Using
  it allows board and device support to become data driven; to make
  setup decisions based on data passed into the kernel instead of on
  per-machine hard coded selections.

I agree that an entire driver shouldn't be put in the DT (especially
given that you then need the DT to debug any issue), but this isn't much
of a driver when it involves flipping a single bit.

The key thing would be to avoid it growing into something too complex
(e.g. set register X to 42 to enable and set register Y to 90 to
disable, would not belong in DT).

Should I be looking at trying to use generic_pm_domain instead of a
regulator? Those don't appear to require a name so a phandle with an
argument for the bit would work.

>> > There could be one if it would help, but we do normally manage to do
>> > this without - look at how other regulator drivers work.
> 
>> Several of them have a fixed list of supported regulator names in the
> 
> Yes, that's the way this is handled.

I see two variants of the hardware bits (three if you consider that the
GMAC isn't on every SoC):

#define MISC_IDDQ_CTRL_GMAC         (1<<18)
#define MISC_IDDQ_CTRL_WLAN_PADS    (1<<13)
#define MISC_IDDQ_CTRL_PCIE         (1<<12)
#define MISC_IDDQ_CTRL_FAP          (1<<11)
#define MISC_IDDQ_CTRL_VDSL_MIPS    (1<<10)
#define MISC_IDDQ_CTRL_VDSL_PHY     (1<<9)
#define MISC_IDDQ_CTRL_PERIPH       (1<<8)
#define MISC_IDDQ_CTRL_PCM          (1<<7)
#define MISC_IDDQ_CTRL_ROBOSW       (1<<6)
#define MISC_IDDQ_CTRL_USBD         (1<<5)
#define MISC_IDDQ_CTRL_USBH         (1<<4)
#define MISC_IDDQ_CTRL_DECT         (1<<3)
#define MISC_IDDQ_CTRL_MIPS         (1<<2)
#define MISC_IDDQ_CTRL_IPSEC        (1<<1)
#define MISC_IDDQ_CTRL_SAR          (1<<0)

#define MISC_IDDQ_CTRL_EPHY         (1<<9)
#define MISC_IDDQ_CTRL_ROBOSW       (1<<8)
#define MISC_IDDQ_CTRL_PCIE         (1<<7)
#define MISC_IDDQ_CTRL_USBH         (1<<6)
#define MISC_IDDQ_CTRL_USBD         (1<<5)
#define MISC_IDDQ_CTRL_PCM          (1<<4)
#define MISC_IDDQ_CTRL_SAR          (1<<3)
#define MISC_IDDQ_CTRL_ADSL2_AFE    (1<<2)
#define MISC_IDDQ_CTRL_ADSL2_PHY    (1<<1)
#define MISC_IDDQ_CTRL_ADSL2_MIPS   (1<<0)

I could put these lists of bits in a driver (associated with the names)
but I'd strongly prefer to put the list of bits in the DT.

If they are in the driver, then there is nothing to stop someone from
deciding to force it to fit a different new piece of hardware by
deliberately using the wrong names in the DT just because they match
the bits they want to use, and they think they can't wait for a new 
list of bits to be added to the kernel.

e.g. someone adds an LTE chip and reuses the obsolete DECT regulator:

misc_iddq_ctrl@42 {
  compatible = "...-regulator";
  reg = <0x42>;

  regulators {
    lte_power: dect {
      regulator-name = "LTE";
    };
  };
};

Associating the bits with the names in the DT avoids this problem at
the expense of having a very generic driver.

A lot of the existing regulator drivers have mostly numbered regulator
pins which could have been connected to anything on a hardware variant.

>> driver. The regulator names for this device are meaningless to the
>> driver because all of the regulators look the same. They don't have a
>> known or controllable voltage and can only be turned on or off.
> 
> Nonsense.  The names are useful to identify which supply is being
> referred to.  Having voltage control is irrelevant to identifying
> regulators.

Ok, but from the driver's point of view there's nothing different about
any of the regulators on this hardware. I could have numbered them
"BIT0" to "BIT31" in the driver and used a meaningful alias in DT.

>> Any table mapping regulator names to bits in the register would be
>> different for each SoC making the list of regulator names in the device
>> tree redundant. If they're not listed in the device tree then I can't
>> use them as a phandle for other devices.
> 
> The list of regulator nodes in device tree is not redundant, it is as
> you say used to connect things together.  The question is where to put
> the control data for those regulators (in this case the enable time and
> the switch).

Ok, I hadn't considered that regulator names could be in DT without
any enable bit information when this was in the driver.

-- 
Simon Arlott

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 03/12/15 15:05, Mark Brown wrote:
> On Thu, Dec 03, 2015 at 08:14:33AM +0000, Simon Arlott wrote:
>> On 03/12/15 00:06, Mark Brown wrote:
> 
>> > this it should know at least something about how to control the device
>> > from the compatible string.  If you're making a generic driver it should
>> > not make reference to specific devices.
> 
>> Will you accept a generic driver for a simple enable regulator device on
>> a regmap? What should its compatible string be?
> 
> Perhaps.  I really don't like putting the entire driver into DT, it's
> not a pattern I want to encourage.

I don't like putting the list of which bit is which regulator name into
a driver because it means that new hardware that adds a regulator or
moves them around requires a change to the driver.

Documentation/devicetree/usage-model.txt:
  2.1 High Level View
  -------------------
  [...] Using
  it allows board and device support to become data driven; to make
  setup decisions based on data passed into the kernel instead of on
  per-machine hard coded selections.

I agree that an entire driver shouldn't be put in the DT (especially
given that you then need the DT to debug any issue), but this isn't much
of a driver when it involves flipping a single bit.

The key thing would be to avoid it growing into something too complex
(e.g. set register X to 42 to enable and set register Y to 90 to
disable, would not belong in DT).

Should I be looking at trying to use generic_pm_domain instead of a
regulator? Those don't appear to require a name so a phandle with an
argument for the bit would work.

>> > There could be one if it would help, but we do normally manage to do
>> > this without - look at how other regulator drivers work.
> 
>> Several of them have a fixed list of supported regulator names in the
> 
> Yes, that's the way this is handled.

I see two variants of the hardware bits (three if you consider that the
GMAC isn't on every SoC):

#define MISC_IDDQ_CTRL_GMAC         (1<<18)
#define MISC_IDDQ_CTRL_WLAN_PADS    (1<<13)
#define MISC_IDDQ_CTRL_PCIE         (1<<12)
#define MISC_IDDQ_CTRL_FAP          (1<<11)
#define MISC_IDDQ_CTRL_VDSL_MIPS    (1<<10)
#define MISC_IDDQ_CTRL_VDSL_PHY     (1<<9)
#define MISC_IDDQ_CTRL_PERIPH       (1<<8)
#define MISC_IDDQ_CTRL_PCM          (1<<7)
#define MISC_IDDQ_CTRL_ROBOSW       (1<<6)
#define MISC_IDDQ_CTRL_USBD         (1<<5)
#define MISC_IDDQ_CTRL_USBH         (1<<4)
#define MISC_IDDQ_CTRL_DECT         (1<<3)
#define MISC_IDDQ_CTRL_MIPS         (1<<2)
#define MISC_IDDQ_CTRL_IPSEC        (1<<1)
#define MISC_IDDQ_CTRL_SAR          (1<<0)

#define MISC_IDDQ_CTRL_EPHY         (1<<9)
#define MISC_IDDQ_CTRL_ROBOSW       (1<<8)
#define MISC_IDDQ_CTRL_PCIE         (1<<7)
#define MISC_IDDQ_CTRL_USBH         (1<<6)
#define MISC_IDDQ_CTRL_USBD         (1<<5)
#define MISC_IDDQ_CTRL_PCM          (1<<4)
#define MISC_IDDQ_CTRL_SAR          (1<<3)
#define MISC_IDDQ_CTRL_ADSL2_AFE    (1<<2)
#define MISC_IDDQ_CTRL_ADSL2_PHY    (1<<1)
#define MISC_IDDQ_CTRL_ADSL2_MIPS   (1<<0)

I could put these lists of bits in a driver (associated with the names)
but I'd strongly prefer to put the list of bits in the DT.

If they are in the driver, then there is nothing to stop someone from
deciding to force it to fit a different new piece of hardware by
deliberately using the wrong names in the DT just because they match
the bits they want to use, and they think they can't wait for a new 
list of bits to be added to the kernel.

e.g. someone adds an LTE chip and reuses the obsolete DECT regulator:

misc_iddq_ctrl@42 {
  compatible = "...-regulator";
  reg = <0x42>;

  regulators {
    lte_power: dect {
      regulator-name = "LTE";
    };
  };
};

Associating the bits with the names in the DT avoids this problem at
the expense of having a very generic driver.

A lot of the existing regulator drivers have mostly numbered regulator
pins which could have been connected to anything on a hardware variant.

>> driver. The regulator names for this device are meaningless to the
>> driver because all of the regulators look the same. They don't have a
>> known or controllable voltage and can only be turned on or off.
> 
> Nonsense.  The names are useful to identify which supply is being
> referred to.  Having voltage control is irrelevant to identifying
> regulators.

Ok, but from the driver's point of view there's nothing different about
any of the regulators on this hardware. I could have numbered them
"BIT0" to "BIT31" in the driver and used a meaningful alias in DT.

>> Any table mapping regulator names to bits in the register would be
>> different for each SoC making the list of regulator names in the device
>> tree redundant. If they're not listed in the device tree then I can't
>> use them as a phandle for other devices.
> 
> The list of regulator nodes in device tree is not redundant, it is as
> you say used to connect things together.  The question is where to put
> the control data for those regulators (in this case the enable time and
> the switch).

Ok, I hadn't considered that regulator names could be in DT without
any enable bit information when this was in the driver.

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

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Thu, Dec 03, 2015 at 11:38:28PM +0000, Simon Arlott wrote:

> #define MISC_IDDQ_CTRL_GMAC         (1<<18)
> #define MISC_IDDQ_CTRL_WLAN_PADS    (1<<13)
> #define MISC_IDDQ_CTRL_PCIE         (1<<12)
> #define MISC_IDDQ_CTRL_FAP          (1<<11)
> #define MISC_IDDQ_CTRL_VDSL_MIPS    (1<<10)
> #define MISC_IDDQ_CTRL_VDSL_PHY     (1<<9)
> #define MISC_IDDQ_CTRL_PERIPH       (1<<8)
> #define MISC_IDDQ_CTRL_PCM          (1<<7)
> #define MISC_IDDQ_CTRL_ROBOSW       (1<<6)
> #define MISC_IDDQ_CTRL_USBD         (1<<5)
> #define MISC_IDDQ_CTRL_USBH         (1<<4)
> #define MISC_IDDQ_CTRL_DECT         (1<<3)
> #define MISC_IDDQ_CTRL_MIPS         (1<<2)
> #define MISC_IDDQ_CTRL_IPSEC        (1<<1)
> #define MISC_IDDQ_CTRL_SAR          (1<<0)

Are you *sure* these are regulators and not power domains?  These names
look a lot like they could be power domains.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On 03/12/15 23:45, Mark Brown wrote:
> On Thu, Dec 03, 2015 at 11:38:28PM +0000, Simon Arlott wrote:
> 
>> #define MISC_IDDQ_CTRL_GMAC         (1<<18)
>> #define MISC_IDDQ_CTRL_WLAN_PADS    (1<<13)
>> #define MISC_IDDQ_CTRL_PCIE         (1<<12)
>> #define MISC_IDDQ_CTRL_FAP          (1<<11)
>> #define MISC_IDDQ_CTRL_VDSL_MIPS    (1<<10)
>> #define MISC_IDDQ_CTRL_VDSL_PHY     (1<<9)
>> #define MISC_IDDQ_CTRL_PERIPH       (1<<8)
>> #define MISC_IDDQ_CTRL_PCM          (1<<7)
>> #define MISC_IDDQ_CTRL_ROBOSW       (1<<6)
>> #define MISC_IDDQ_CTRL_USBD         (1<<5)
>> #define MISC_IDDQ_CTRL_USBH         (1<<4)
>> #define MISC_IDDQ_CTRL_DECT         (1<<3)
>> #define MISC_IDDQ_CTRL_MIPS         (1<<2)
>> #define MISC_IDDQ_CTRL_IPSEC        (1<<1)
>> #define MISC_IDDQ_CTRL_SAR          (1<<0)
> 
> Are you *sure* these are regulators and not power domains?  These names
> look a lot like they could be power domains.

No, I'm not sure. Some of them are may actually be regulators (the "PHY"
ones) while others are almost definitely power domains (like the "FAP"
Forwarding Assist Processor).

-- 
Simon Arlott

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Thu, Dec 03, 2015 at 11:51:16PM +0000, Simon Arlott wrote:
> On 03/12/15 23:45, Mark Brown wrote:

> > Are you *sure* these are regulators and not power domains?  These names
> > look a lot like they could be power domains.

> No, I'm not sure. Some of them are may actually be regulators (the "PHY"
> ones) while others are almost definitely power domains (like the "FAP"
> Forwarding Assist Processor).

OK, so the power domains should be being represented and managed as such
rather than using regulators - it's a better fit (doing things like
support atomic context) and it also sidesteps this.  For the things that
you say are clearly regulators should we have more information about
those?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Simon Arlott]

On Fri, December 4, 2015 11:00, Mark Brown wrote:
> On Thu, Dec 03, 2015 at 11:51:16PM +0000, Simon Arlott wrote:
>> On 03/12/15 23:45, Mark Brown wrote:
>
>> > Are you *sure* these are regulators and not power domains?  These names
>> > look a lot like they could be power domains.
>
>> No, I'm not sure. Some of them are may actually be regulators (the "PHY"
>> ones) while others are almost definitely power domains (like the "FAP"
>> Forwarding Assist Processor).
>
> OK, so the power domains should be being represented and managed as such
> rather than using regulators - it's a better fit (doing things like
> support atomic context) and it also sidesteps this.  For the things that
> you say are clearly regulators should we have more information about
> those?

I'd prefer to handle them all as power domains since it fits better. Even
if some of them are regulators there's no extra control or status interface
and they're used like power domains to disable unused functionality.

-- 
Simon Arlott

Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding

[Mark Brown]

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

On Fri, Dec 04, 2015 at 12:26:58PM -0000, Simon Arlott wrote:
> On Fri, December 4, 2015 11:00, Mark Brown wrote:

> > OK, so the power domains should be being represented and managed as such
> > rather than using regulators - it's a better fit (doing things like
> > support atomic context) and it also sidesteps this.  For the things that
> > you say are clearly regulators should we have more information about
> > those?

> I'd prefer to handle them all as power domains since it fits better. Even
> if some of them are regulators there's no extra control or status interface
> and they're used like power domains to disable unused functionality.

OK, great - runtime PM is just generally a more idiomatic interface for
this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]