Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280255 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree binding Simon Arlott <simon@fire.lp0.eu> - 2015-11-30 21:40 +0100
    [PATCH 2/2] regulator: bcm63xx: Add BCM63xx fixed regulator device Simon Arlott <simon@fire.lp0.eu> - 2015-11-30 21:40 +0100
    Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-01 23:20 +0100
      Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding "Simon Arlott" <simon@fire.lp0.eu> - 2015-12-02 13:50 +0100
        Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-02 14:00 +0100
          Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Simon Arlott <simon@fire.lp0.eu> - 2015-12-02 21:30 +0100
            Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-03 01:10 +0100
              Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Simon Arlott <simon@fire.lp0.eu> - 2015-12-03 09:20 +0100
                Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-03 16:10 +0100
                  Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Simon Arlott <simon@fire.lp0.eu> - 2015-12-04 00:40 +0100
                    Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-04 00:50 +0100
                      Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Simon Arlott <simon@fire.lp0.eu> - 2015-12-04 01:00 +0100
                        Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-04 12:10 +0100
                          Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding "Simon Arlott" <simon@fire.lp0.eu> - 2015-12-04 13:30 +0100
                            Re: [PATCH 1/2] regulator: Add brcm,bcm63xx-regulator device tree  binding Mark Brown <broonie@kernel.org> - 2015-12-05 18:00 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web