[PATCH 0/6] BCM59056 PMU regulator support

[PATCH 0/6] BCM59056 PMU regulator support

[Matt Porter]

The BCM59056 is a multi-function power management unit used with the
BCM281xx family of SoCs. This series adds an MFD and voltage regulator
driver to support the BCM59056. The bcm28155-ap DT support is updated
to enable use of regulators on the otg and sdhci peripherals.

Matt Porter (6):
  i2c: bcm-kona: register with subsys_initcall
  regulator: add bcm59056 pmu DT binding
  mfd: add bcm59056 pmu driver
  regulator: add bcm59056 regulator driver
  ARM: configs: bcm_defconfig: enable bcm59056 regulator support
  ARM: dts: add bcm59056 pmu support and enable for bcm28155-ap

 .../devicetree/bindings/regulator/bcm59056.txt     |  37 ++
 arch/arm/boot/dts/bcm28155-ap.dts                  |  41 ++
 arch/arm/boot/dts/bcm59056.dtsi                    | 158 ++++++++
 arch/arm/configs/bcm_defconfig                     |   7 +
 drivers/i2c/busses/i2c-bcm-kona.c                  |  14 +-
 drivers/mfd/Kconfig                                |   8 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/bcm59056.c                             | 119 ++++++
 drivers/regulator/Kconfig                          |   8 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/bcm59056-regulator.c             | 445 +++++++++++++++++++++
 include/linux/mfd/bcm59056.h                       |  35 ++
 12 files changed, 873 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/bcm59056.txt
 create mode 100644 arch/arm/boot/dts/bcm59056.dtsi
 create mode 100644 drivers/mfd/bcm59056.c
 create mode 100644 drivers/regulator/bcm59056-regulator.c
 create mode 100644 include/linux/mfd/bcm59056.h

-- 
1.8.4

[PATCH 1/6] i2c: bcm-kona: register with subsys_initcall

[Matt Porter]

Voltage regulators are needed very early due to deferred probe
being incompatible with built-in USB gadget drivers. In order to
have the PMU driver available before USB UDC, make i2c available
during subsys_initcall.

Signed-off-by: Matt Porter <mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-bcm-kona.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-kona.c b/drivers/i2c/busses/i2c-bcm-kona.c
index 18a74a6..bfd4056 100644
--- a/drivers/i2c/busses/i2c-bcm-kona.c
+++ b/drivers/i2c/busses/i2c-bcm-kona.c
@@ -901,7 +901,19 @@ static struct platform_driver bcm_kona_i2c_driver = {
 	.probe = bcm_kona_i2c_probe,
 	.remove = bcm_kona_i2c_remove,
 };
-module_platform_driver(bcm_kona_i2c_driver);
+
+static int __init bcm_kona_i2c_init_driver(void)
+{
+	return platform_driver_register(&bcm_kona_i2c_driver);
+}
+/* PMU access is needed early so use subsys init */
+subsys_initcall(bcm_kona_i2c_init_driver);
+
+static void __exit bcm_kona_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&bcm_kona_i2c_driver);
+}
+module_exit(bcm_kona_i2c_exit_driver);
 
 MODULE_AUTHOR("Tim Kryger <tkryger-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>");
 MODULE_DESCRIPTION("Broadcom Kona I2C Driver");
-- 
1.8.4

--
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/6] i2c: bcm-kona: register with subsys_initcall

[Wolfram Sang]

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

On Tue, Feb 04, 2014 at 07:19:07AM -0500, Matt Porter wrote:

> Voltage regulators are needed very early due to deferred probe
> being incompatible with built-in USB gadget drivers.

What does it need to fix those instead?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/6] i2c: bcm-kona: register with subsys_initcall

[Matt Porter]

On Wed, Feb 05, 2014 at 10:08:18AM +0100, Wolfram Sang wrote:
> On Tue, Feb 04, 2014 at 07:19:07AM -0500, Matt Porter wrote:
> 
> > Voltage regulators are needed very early due to deferred probe
> > being incompatible with built-in USB gadget drivers.
> 
> What does it need to fix those instead?

[added Alan/Felipe for more insight]

Discussion on that topic came about from this submission:
http://www.spinics.net/lists/linux-usb/msg94217.html

End of it is:
http://www.spinics.net/lists/linux-usb/msg94731.html

We can either add to the many drivers that already do subsys_initcall()
for similar reasons...or I can drop this from the series and add gadget
probe ordering to my TODO list.

In short, it can't be a late_initcall() hack like the original post and
really could be solved by converting to a real bus (and letting
deferred probe do its job)..but Alan voiced concerns about that.

-Matt

Re: [PATCH 1/6] i2c: bcm-kona: register with subsys_initcall

[Alan Stern]

On Wed, 5 Feb 2014, Matt Porter wrote:

> On Wed, Feb 05, 2014 at 10:08:18AM +0100, Wolfram Sang wrote:
> > On Tue, Feb 04, 2014 at 07:19:07AM -0500, Matt Porter wrote:
> > 
> > > Voltage regulators are needed very early due to deferred probe
> > > being incompatible with built-in USB gadget drivers.
> > 
> > What does it need to fix those instead?
> 
> [added Alan/Felipe for more insight]
> 
> Discussion on that topic came about from this submission:
> http://www.spinics.net/lists/linux-usb/msg94217.html
> 
> End of it is:
> http://www.spinics.net/lists/linux-usb/msg94731.html
> 
> We can either add to the many drivers that already do subsys_initcall()
> for similar reasons...or I can drop this from the series and add gadget
> probe ordering to my TODO list.
> 
> In short, it can't be a late_initcall() hack like the original post and
> really could be solved by converting to a real bus (and letting
> deferred probe do its job)..but Alan voiced concerns about that.

Don't worry too much about what I said.  If adding a "gadget" bus will 
solve the problem in an appropriate way, and if nobody else objects 
(particularly Felipe, who is on vacation now), then go for it.

Alan Stern

--
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/6] i2c: bcm-kona: register with subsys_initcall

[Matt Porter]

On Wed, Feb 05, 2014 at 10:30:29AM -0500, Alan Stern wrote:
> On Wed, 5 Feb 2014, Matt Porter wrote:
> 
> > On Wed, Feb 05, 2014 at 10:08:18AM +0100, Wolfram Sang wrote:
> > > On Tue, Feb 04, 2014 at 07:19:07AM -0500, Matt Porter wrote:
> > > 
> > > > Voltage regulators are needed very early due to deferred probe
> > > > being incompatible with built-in USB gadget drivers.
> > > 
> > > What does it need to fix those instead?
> > 
> > [added Alan/Felipe for more insight]
> > 
> > Discussion on that topic came about from this submission:
> > http://www.spinics.net/lists/linux-usb/msg94217.html
> > 
> > End of it is:
> > http://www.spinics.net/lists/linux-usb/msg94731.html
> > 
> > We can either add to the many drivers that already do subsys_initcall()
> > for similar reasons...or I can drop this from the series and add gadget
> > probe ordering to my TODO list.
> > 
> > In short, it can't be a late_initcall() hack like the original post and
> > really could be solved by converting to a real bus (and letting
> > deferred probe do its job)..but Alan voiced concerns about that.
> 
> Don't worry too much about what I said.  If adding a "gadget" bus will 
> solve the problem in an appropriate way, and if nobody else objects 
> (particularly Felipe, who is on vacation now), then go for it.

Ok, I'll take a look at what can be done and restart the conversation
when Felipe returns.

Wolfram: given this, as I mentioned, I'll simply drop this patch from
the series and work around it for now. This will probably make Lee and
Mark happy to not see subsys_initcall() in the MFD/regulator drivers as
well.

-Matt

[PATCH 5/6] ARM: configs: bcm_defconfig: enable bcm59056 regulator support

[Matt Porter]

Enable BCM59056 MFD and regulator drivers to manage voltage
regulators on BCM281xx platforms.

Signed-off-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Porter <mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/configs/bcm_defconfig | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/configs/bcm_defconfig b/arch/arm/configs/bcm_defconfig
index 2519d6d..35752cc 100644
--- a/arch/arm/configs/bcm_defconfig
+++ b/arch/arm/configs/bcm_defconfig
@@ -79,6 +79,13 @@ CONFIG_HW_RANDOM=y
 CONFIG_I2C=y
 CONFIG_I2C_CHARDEV=y
 # CONFIG_HWMON is not set
+CONFIG_MFD_BCM59056=y
+CONFIG_REGULATOR=y
+CONFIG_REGULATOR_FIXED_VOLTAGE=y
+CONFIG_REGULATOR_VIRTUAL_CONSUMER=y
+CONFIG_REGULATOR_USERSPACE_CONSUMER=y
+CONFIG_REGULATOR_BCM59056=y
+
 CONFIG_VIDEO_OUTPUT_CONTROL=y
 CONFIG_FB=y
 CONFIG_BACKLIGHT_LCD_SUPPORT=y
-- 
1.8.4

--
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 0/6] BCM59056 PMU regulator support

[Lee Jones]

> The BCM59056 is a multi-function power management unit used with the
> BCM281xx family of SoCs. This series adds an MFD and voltage regulator
> driver to support the BCM59056. The bcm28155-ap DT support is updated
> to enable use of regulators on the otg and sdhci peripherals.
> 
> Matt Porter (6):
>   i2c: bcm-kona: register with subsys_initcall
>   regulator: add bcm59056 pmu DT binding
>   mfd: add bcm59056 pmu driver
>   regulator: add bcm59056 regulator driver
>   ARM: configs: bcm_defconfig: enable bcm59056 regulator support
>   ARM: dts: add bcm59056 pmu support and enable for bcm28155-ap
> 
>  .../devicetree/bindings/regulator/bcm59056.txt     |  37 ++
>  arch/arm/boot/dts/bcm28155-ap.dts                  |  41 ++
>  arch/arm/boot/dts/bcm59056.dtsi                    | 158 ++++++++
>  arch/arm/configs/bcm_defconfig                     |   7 +
>  drivers/i2c/busses/i2c-bcm-kona.c                  |  14 +-
>  drivers/mfd/Kconfig                                |   8 +
>  drivers/mfd/Makefile                               |   1 +
>  drivers/mfd/bcm59056.c                             | 119 ++++++
>  drivers/regulator/Kconfig                          |   8 +
>  drivers/regulator/Makefile                         |   1 +
>  drivers/regulator/bcm59056-regulator.c             | 445 +++++++++++++++++++++
>  include/linux/mfd/bcm59056.h                       |  35 ++
>  12 files changed, 873 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/bcm59056.txt
>  create mode 100644 arch/arm/boot/dts/bcm59056.dtsi
>  create mode 100644 drivers/mfd/bcm59056.c
>  create mode 100644 drivers/regulator/bcm59056-regulator.c
>  create mode 100644 include/linux/mfd/bcm59056.h

FYI: Mark's email address is not correct. Not sure where you pulled
this one up from, but he doesn't have access to it anymore.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/6] BCM59056 PMU regulator support

[Matt Porter]

On Tue, Feb 04, 2014 at 01:40:43PM +0000, Lee Jones wrote:
> > The BCM59056 is a multi-function power management unit used with the
> > BCM281xx family of SoCs. This series adds an MFD and voltage regulator
> > driver to support the BCM59056. The bcm28155-ap DT support is updated
> > to enable use of regulators on the otg and sdhci peripherals.
> > 
> > Matt Porter (6):
> >   i2c: bcm-kona: register with subsys_initcall
> >   regulator: add bcm59056 pmu DT binding
> >   mfd: add bcm59056 pmu driver
> >   regulator: add bcm59056 regulator driver
> >   ARM: configs: bcm_defconfig: enable bcm59056 regulator support
> >   ARM: dts: add bcm59056 pmu support and enable for bcm28155-ap
> > 
> >  .../devicetree/bindings/regulator/bcm59056.txt     |  37 ++
> >  arch/arm/boot/dts/bcm28155-ap.dts                  |  41 ++
> >  arch/arm/boot/dts/bcm59056.dtsi                    | 158 ++++++++
> >  arch/arm/configs/bcm_defconfig                     |   7 +
> >  drivers/i2c/busses/i2c-bcm-kona.c                  |  14 +-
> >  drivers/mfd/Kconfig                                |   8 +
> >  drivers/mfd/Makefile                               |   1 +
> >  drivers/mfd/bcm59056.c                             | 119 ++++++
> >  drivers/regulator/Kconfig                          |   8 +
> >  drivers/regulator/Makefile                         |   1 +
> >  drivers/regulator/bcm59056-regulator.c             | 445 +++++++++++++++++++++
> >  include/linux/mfd/bcm59056.h                       |  35 ++
> >  12 files changed, 873 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/regulator/bcm59056.txt
> >  create mode 100644 arch/arm/boot/dts/bcm59056.dtsi
> >  create mode 100644 drivers/mfd/bcm59056.c
> >  create mode 100644 drivers/regulator/bcm59056-regulator.c
> >  create mode 100644 include/linux/mfd/bcm59056.h
> 
> FYI: Mark's email address is not correct. Not sure where you pulled
> this one up from, but he doesn't have access to it anymore.

Thanks, I already updated (it was a stale .mutt/aliases entry here) it
after the bounces.

-Matt

[PATCH 2/6] regulator: add bcm59056 pmu DT binding

[Matt Porter]

Add a DT binding for the BCM59056 PMU. The binding inherits from
the generic regulator bindings.

Signed-off-by: Matt Porter <mporter@linaro.org>
Reviewed-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 .../devicetree/bindings/regulator/bcm59056.txt     | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/bcm59056.txt

diff --git a/Documentation/devicetree/bindings/regulator/bcm59056.txt b/Documentation/devicetree/bindings/regulator/bcm59056.txt
new file mode 100644
index 0000000..bf6b633
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/bcm59056.txt
@@ -0,0 +1,37 @@
+BCM59056 Power Management Unit
+
+Required properties:
+- compatible: "brcm,bcm59056"
+- reg: I2C slave address
+- interrupts: interrupt for the PMU. Generic interrupt client node bindings
+  are described in interrupt-controller/interrupts.txt
+- regulators: This is the list of child nodes that specify the regulator
+  initialization data for defined regulators.  Generic regulator bindings
+  are described in regulator/regulator.txt.
+
+  The valid regulator-compatible values are:
+  	rfldo, camldo1, camldo2, simldo1, simlso2, sdldo, sdxldo,
+	mmcldo1, mmcldo2, audldo, micldo, usbldo, vibldo,
+	csr, iosr1, iosr2, msr, sdsr1, sdsr2, vsr
+
+Example:
+	pmu: bcm59056@8 {
+		compatible = "brcm,bcm59056";
+		reg = <0x08>;
+		interrupts = <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>;
+		regulators {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			rfldo_reg: regulator@0 {
+				reg = <0>;
+				regulator-compatible = "rfldo";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			...
+		};
+	};
-- 
1.8.4

Re: [PATCH 2/6] regulator: add bcm59056 pmu DT binding

[Mark Brown]

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

On Tue, Feb 04, 2014 at 07:19:08AM -0500, Matt Porter wrote:
> Add a DT binding for the BCM59056 PMU. The binding inherits from
> the generic regulator bindings.

Is this really only a regulator - does the chip have no other functions?

> +- regulators: This is the list of child nodes that specify the regulator
> +  initialization data for defined regulators.  Generic regulator bindings
> +  are described in regulator/regulator.txt.

The regulators property should always be optional, the driver should be
able to start up and read back the hardware state without any further
configuration.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/6] regulator: add bcm59056 pmu DT binding

[Matt Porter]

On Tue, Feb 04, 2014 at 05:23:09PM +0000, Mark Brown wrote:
> On Tue, Feb 04, 2014 at 07:19:08AM -0500, Matt Porter wrote:
> > Add a DT binding for the BCM59056 PMU. The binding inherits from
> > the generic regulator bindings.
> 
> Is this really only a regulator - does the chip have no other functions?

It's your average multi-function device with other functions as you are
suspecting.  Buried in the the MFD driver comments is me admitting that
I need to split this into two bindings. The base device, "bcm59056" and
then "bcm59056-reg". So point noted, I'll updated with the appropriate
binding for each.

> > +- regulators: This is the list of child nodes that specify the regulator
> > +  initialization data for defined regulators.  Generic regulator bindings
> > +  are described in regulator/regulator.txt.
> 
> The regulators property should always be optional, the driver should be
> able to start up and read back the hardware state without any further
> configuration.

Ahh, ok. I will make it so.

Thanks,
Matt

Re: [PATCH 2/6] regulator: add bcm59056 pmu DT binding

[Mark Brown]

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

On Tue, Feb 04, 2014 at 04:16:38PM -0500, Matt Porter wrote:
> On Tue, Feb 04, 2014 at 05:23:09PM +0000, Mark Brown wrote:

> > Is this really only a regulator - does the chip have no other functions?

> It's your average multi-function device with other functions as you are
> suspecting.  Buried in the the MFD driver comments is me admitting that
> I need to split this into two bindings. The base device, "bcm59056" and
> then "bcm59056-reg". So point noted, I'll updated with the appropriate
> binding for each.

It doesn't need to be two bindings - just move it to the MFD section and
document it there.  The existing binding is totally fine from a
regulator standpoint and should continue to be so as other functions are
added.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/6] mfd: add bcm59056 pmu driver

[Matt Porter]

Add a driver for the BCM59056 PMU multi-function device. The driver
initially supports regmap initialization and instantiation of the
voltage regulator device function of the PMU.

Signed-off-by: Matt Porter <mporter@linaro.org>
Reviewed-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 drivers/mfd/Kconfig          |   8 +++
 drivers/mfd/Makefile         |   1 +
 drivers/mfd/bcm59056.c       | 119 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/bcm59056.h |  35 +++++++++++++
 4 files changed, 163 insertions(+)
 create mode 100644 drivers/mfd/bcm59056.c
 create mode 100644 include/linux/mfd/bcm59056.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 49bb445..36e96c2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -59,6 +59,14 @@ config MFD_AAT2870_CORE
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+config MFD_BCM59056
+	bool "Broadcom BCM59056 PMU"
+	select MFD_CORE
+	select REGMAP_I2C
+	depends on I2C=y
+	help
+	  Support for the BCM59056 PMU from Broadcom
+
 config MFD_CROS_EC
 	tristate "ChromeOS Embedded Controller"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5aea5ef..8d7e110 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_88PM800)	+= 88pm800.o 88pm80x.o
 obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
+obj-$(CONFIG_MFD_BCM59056)	+= bcm59056.o
 obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
diff --git a/drivers/mfd/bcm59056.c b/drivers/mfd/bcm59056.c
new file mode 100644
index 0000000..f193bfb
--- /dev/null
+++ b/drivers/mfd/bcm59056.c
@@ -0,0 +1,119 @@
+/*
+ * Broadcom BCM59056 PMU
+ *
+ * Copyright 2014 Linaro Limited
+ * Author: Matt Porter <mporter@linaro.org>
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/bcm59056.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+static struct mfd_cell bcm59056s[] = {
+	{
+		.name = "bcm59056-pmu",
+	},
+};
+
+static const struct regmap_config bcm59056_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= BCM59056_MAX_REGISTER - 1,
+	.cache_type	= REGCACHE_RBTREE,
+};
+
+static int bcm59056_i2c_probe(struct i2c_client *i2c,
+			      const struct i2c_device_id *id)
+{
+	struct bcm59056 *bcm59056;
+	int chip_id = id->driver_data;
+	int ret = 0;
+
+	bcm59056 = devm_kzalloc(&i2c->dev, sizeof(*bcm59056), GFP_KERNEL);
+	if (!bcm59056)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, bcm59056);
+	bcm59056->dev = &i2c->dev;
+	bcm59056->i2c_client = i2c;
+	bcm59056->id = chip_id;
+
+	bcm59056->regmap = devm_regmap_init_i2c(i2c, &bcm59056_regmap_config);
+	if (IS_ERR(bcm59056->regmap)) {
+		ret = PTR_ERR(bcm59056->regmap);
+		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = mfd_add_devices(bcm59056->dev, -1,
+			      bcm59056s, ARRAY_SIZE(bcm59056s),
+			      NULL, 0, NULL);
+	if (ret < 0)
+		dev_err(&i2c->dev, "mfd_add_devices failed: %d\n", ret);
+
+	return ret;
+}
+
+static int bcm59056_i2c_remove(struct i2c_client *i2c)
+{
+	struct bcm59056 *bcm59056 = i2c_get_clientdata(i2c);
+
+	mfd_remove_devices(bcm59056->dev);
+
+	return 0;
+}
+
+static const struct of_device_id bcm59056_of_match[] = {
+	{ .compatible = "brcm,bcm59056", .data = (void *)BCM59056 },
+	{ }
+};
+
+static const struct i2c_device_id bcm59056_i2c_id[] = {
+	{ "bcm59056", BCM59056 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bcm59056_i2c_id);
+
+static struct i2c_driver bcm59056_i2c_driver = {
+	.driver = {
+		   .name = "bcm59056",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(bcm59056_of_match),
+	},
+	.probe = bcm59056_i2c_probe,
+	.remove = bcm59056_i2c_remove,
+	.id_table = bcm59056_i2c_id,
+};
+
+static int __init bcm59056_init(void)
+{
+	return i2c_add_driver(&bcm59056_i2c_driver);
+}
+subsys_initcall(bcm59056_init);
+
+static void __exit bcm59056_exit(void)
+{
+	i2c_del_driver(&bcm59056_i2c_driver);
+}
+module_exit(bcm59056_exit);
+
+MODULE_AUTHOR("Matt Porter <mporter@linaro.org>");
+MODULE_DESCRIPTION("BCM59056 multi-function driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:bcm59056");
diff --git a/include/linux/mfd/bcm59056.h b/include/linux/mfd/bcm59056.h
new file mode 100644
index 0000000..967395d
--- /dev/null
+++ b/include/linux/mfd/bcm59056.h
@@ -0,0 +1,35 @@
+/*
+ * Broadcom BCM59056 PMU
+ *
+ * Copyright 2014 Linaro Limited
+ * Author: Matt Porter <mporter@linaro.org>
+ *
+ * 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.
+ *
+ */
+
+#ifndef __LINUX_MFD_BCM59056_H
+#define __LINUX_MFD_BCM59056_H
+
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+/* chip id */
+#define BCM59056		0
+
+/* max register address */
+#define BCM59056_MAX_REGISTER	0xe8
+
+/* bcm59056 chip access */
+struct bcm59056 {
+	struct device *dev;
+	struct i2c_client *i2c_client;
+	struct regmap *regmap;
+	unsigned int id;
+};
+
+#endif /*  __LINUX_MFD_BCM59056_H */
-- 
1.8.4

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Lee Jones]

> Add a driver for the BCM59056 PMU multi-function device. The driver
> initially supports regmap initialization and instantiation of the
> voltage regulator device function of the PMU.
> 
> Signed-off-by: Matt Porter <mporter@linaro.org>
> Reviewed-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> ---
>  drivers/mfd/Kconfig          |   8 +++
>  drivers/mfd/Makefile         |   1 +
>  drivers/mfd/bcm59056.c       | 119 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/bcm59056.h |  35 +++++++++++++
>  4 files changed, 163 insertions(+)
>  create mode 100644 drivers/mfd/bcm59056.c
>  create mode 100644 include/linux/mfd/bcm59056.h

<snip>

> +static struct mfd_cell bcm59056s[] = {
> +	{
> +		.name = "bcm59056-pmu",

If you plan to use *->of_node in the PMU driver, which it looks like
you do, you should set the compatible string here.

> +	},
> +};
> +
> +static const struct regmap_config bcm59056_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= BCM59056_MAX_REGISTER - 1,

If you've just set this manually i.e. it's not part of an enum table,
can't you set it a value you don't need to do math on? It's not used
anywhere else is it?

> +	.cache_type	= REGCACHE_RBTREE,
> +};
> +
> +static int bcm59056_i2c_probe(struct i2c_client *i2c,
> +			      const struct i2c_device_id *id)
> +{
> +	struct bcm59056 *bcm59056;
> +	int chip_id = id->driver_data;

I thought this was a DT only driver? If so, you probably want to use
of_match_device() instead.

> +	int ret = 0;
> +
> +	bcm59056 = devm_kzalloc(&i2c->dev, sizeof(*bcm59056), GFP_KERNEL);
> +	if (!bcm59056)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, bcm59056);
> +	bcm59056->dev = &i2c->dev;
> +	bcm59056->i2c_client = i2c;
> +	bcm59056->id = chip_id;
> +
> +	bcm59056->regmap = devm_regmap_init_i2c(i2c, &bcm59056_regmap_config);
> +	if (IS_ERR(bcm59056->regmap)) {
> +		ret = PTR_ERR(bcm59056->regmap);
> +		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mfd_add_devices(bcm59056->dev, -1,
> +			      bcm59056s, ARRAY_SIZE(bcm59056s),
> +			      NULL, 0, NULL);

Are you sure you need all of your #includes?

I notice that irqdomain is there, but you don't actually have one?

> +	if (ret < 0)
> +		dev_err(&i2c->dev, "mfd_add_devices failed: %d\n", ret);

What if we change the name of the function? Probably better to say
something like "device registration failed" or thelike.

> +	return ret;
> +}
> +
> +static int bcm59056_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct bcm59056 *bcm59056 = i2c_get_clientdata(i2c);
> +
> +	mfd_remove_devices(bcm59056->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id bcm59056_of_match[] = {
> +	{ .compatible = "brcm,bcm59056", .data = (void *)BCM59056 },

You've gone to the trouble of setting a device version here, but you
don't seem to use it?

> +	{ }
> +};
> +
> +static const struct i2c_device_id bcm59056_i2c_id[] = {
> +	{ "bcm59056", BCM59056 },
> +	{ }
> +};

Ah, I guess this is where id->driver comes from.

It's pretty silly that the I2C subsystem demands the presence of this
table, despite not using it if an of_device_id table is present.

> +MODULE_DEVICE_TABLE(i2c, bcm59056_i2c_id);
> +
> +static struct i2c_driver bcm59056_i2c_driver = {
> +	.driver = {
> +		   .name = "bcm59056",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = of_match_ptr(bcm59056_of_match),

No need to use of_match_ptr() here.

> +	},
> +	.probe = bcm59056_i2c_probe,
> +	.remove = bcm59056_i2c_remove,
> +	.id_table = bcm59056_i2c_id,

*grumble*

> +};
> +
> +static int __init bcm59056_init(void)
> +{
> +	return i2c_add_driver(&bcm59056_i2c_driver);
> +}
> +subsys_initcall(bcm59056_init);

Really? :(

Maybe you'll want to comment this, in case people do not know the back
ground and attempts to clean it up.

> +static void __exit bcm59056_exit(void)
> +{
> +	i2c_del_driver(&bcm59056_i2c_driver);
> +}
> +module_exit(bcm59056_exit);

<snip>

> +/* chip id */
> +#define BCM59056		0

Lonely, oh so lonely!

> +/* max register address */
> +#define BCM59056_MAX_REGISTER	0xe8

Don't you have a table of registers which you care about?

> +/* bcm59056 chip access */

Superfluous comment? Don't we all know what these containers do?

> +struct bcm59056 {
> +	struct device *dev;
> +	struct i2c_client *i2c_client;
> +	struct regmap *regmap;
> +	unsigned int id;
> +};
> +
> +#endif /*  __LINUX_MFD_BCM59056_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Matt Porter]

On Tue, Feb 04, 2014 at 01:29:51PM +0000, Lee Jones wrote:
> > Add a driver for the BCM59056 PMU multi-function device. The driver
> > initially supports regmap initialization and instantiation of the
> > voltage regulator device function of the PMU.
> > 
> > Signed-off-by: Matt Porter <mporter@linaro.org>
> > Reviewed-by: Tim Kryger <tim.kryger@linaro.org>
> > Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> > ---
> >  drivers/mfd/Kconfig          |   8 +++
> >  drivers/mfd/Makefile         |   1 +
> >  drivers/mfd/bcm59056.c       | 119 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/bcm59056.h |  35 +++++++++++++
> >  4 files changed, 163 insertions(+)
> >  create mode 100644 drivers/mfd/bcm59056.c
> >  create mode 100644 include/linux/mfd/bcm59056.h
> 
> <snip>
> 
> > +static struct mfd_cell bcm59056s[] = {
> > +	{
> > +		.name = "bcm59056-pmu",
> 
> If you plan to use *->of_node in the PMU driver, which it looks like
> you do, you should set the compatible string here.

Ok. I missed that..I'll split bindings to reflect this as well. There's
an error in that the regulator properties are all defined in the base
compatible of brcm,bcm59056 atm and they'll need to be in
brcm,bcm59056-pmu's binding.

> > +	},
> > +};
> > +
> > +static const struct regmap_config bcm59056_regmap_config = {
> > +	.reg_bits	= 8,
> > +	.val_bits	= 8,
> > +	.max_register	= BCM59056_MAX_REGISTER - 1,
> 
> If you've just set this manually i.e. it's not part of an enum table,
> can't you set it a value you don't need to do math on? It's not used
> anywhere else is it?

Yes, I'll update this. It's not used elsewhere.

> 
> > +	.cache_type	= REGCACHE_RBTREE,
> > +};
> > +
> > +static int bcm59056_i2c_probe(struct i2c_client *i2c,
> > +			      const struct i2c_device_id *id)
> > +{
> > +	struct bcm59056 *bcm59056;
> > +	int chip_id = id->driver_data;
> 
> I thought this was a DT only driver? If so, you probably want to use
> of_match_device() instead.

Correct, I'll use of_match_device()

> > +	int ret = 0;
> > +
> > +	bcm59056 = devm_kzalloc(&i2c->dev, sizeof(*bcm59056), GFP_KERNEL);
> > +	if (!bcm59056)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(i2c, bcm59056);
> > +	bcm59056->dev = &i2c->dev;
> > +	bcm59056->i2c_client = i2c;
> > +	bcm59056->id = chip_id;
> > +
> > +	bcm59056->regmap = devm_regmap_init_i2c(i2c, &bcm59056_regmap_config);
> > +	if (IS_ERR(bcm59056->regmap)) {
> > +		ret = PTR_ERR(bcm59056->regmap);
> > +		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = mfd_add_devices(bcm59056->dev, -1,
> > +			      bcm59056s, ARRAY_SIZE(bcm59056s),
> > +			      NULL, 0, NULL);
> 
> Are you sure you need all of your #includes?
> 
> I notice that irqdomain is there, but you don't actually have one?

Right, I'll do a sweep on them. In that particular case, I don't yet
have a need to implement interrupt support so it needs to go.

> 
> > +	if (ret < 0)
> > +		dev_err(&i2c->dev, "mfd_add_devices failed: %d\n", ret);
> 
> What if we change the name of the function? Probably better to say
> something like "device registration failed" or thelike.

Will do.

> 
> > +	return ret;
> > +}
> > +
> > +static int bcm59056_i2c_remove(struct i2c_client *i2c)
> > +{
> > +	struct bcm59056 *bcm59056 = i2c_get_clientdata(i2c);
> > +
> > +	mfd_remove_devices(bcm59056->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id bcm59056_of_match[] = {
> > +	{ .compatible = "brcm,bcm59056", .data = (void *)BCM59056 },
> 
> You've gone to the trouble of setting a device version here, but you
> don't seem to use it?

I'll drop the device version

> > +	{ }
> > +};
> > +
> > +static const struct i2c_device_id bcm59056_i2c_id[] = {
> > +	{ "bcm59056", BCM59056 },
> > +	{ }
> > +};
> 
> Ah, I guess this is where id->driver comes from.
> 
> It's pretty silly that the I2C subsystem demands the presence of this
> table, despite not using it if an of_device_id table is present.

It does make it very difficult to follow DT enabled I2C client drivers
:( I'll drop the BCM59056 too.

> > +MODULE_DEVICE_TABLE(i2c, bcm59056_i2c_id);
> > +
> > +static struct i2c_driver bcm59056_i2c_driver = {
> > +	.driver = {
> > +		   .name = "bcm59056",
> > +		   .owner = THIS_MODULE,
> > +		   .of_match_table = of_match_ptr(bcm59056_of_match),
> 
> No need to use of_match_ptr() here.

Will remove.

> > +	},
> > +	.probe = bcm59056_i2c_probe,
> > +	.remove = bcm59056_i2c_remove,
> > +	.id_table = bcm59056_i2c_id,
> 
> *grumble*

:) Yes, unavoidable for now.

> > +};
> > +
> > +static int __init bcm59056_init(void)
> > +{
> > +	return i2c_add_driver(&bcm59056_i2c_driver);
> > +}
> > +subsys_initcall(bcm59056_init);
> 
> Really? :(
> 
> Maybe you'll want to comment this, in case people do not know the back
> ground and attempts to clean it up.

I'll add a comment. This is the old problem of needing regulators really
early. It's exasperated by the fact that the USB gadget framework
drivers do not use driver probing. This means that they can not be
deferred after i2c/mfd/regulator init...the problem is particularly
noticeable when building in a gadget driver for nfsroot purposes.

Because of this I have the regulator, MFD, and i2c drivers all using
subsys_initcall() in this series. FWIW, there's some discussion about
how to resolve the USB gadget driver case but it's going to take a
while.

> > +static void __exit bcm59056_exit(void)
> > +{
> > +	i2c_del_driver(&bcm59056_i2c_driver);
> > +}
> > +module_exit(bcm59056_exit);
> 
> <snip>
> 
> > +/* chip id */
> > +#define BCM59056		0
> 
> Lonely, oh so lonely!

Understood. Will remove.

> > +/* max register address */
> > +#define BCM59056_MAX_REGISTER	0xe8
> 
> Don't you have a table of registers which you care about?

I placed the register defs that I actually use in the regulator driver
itself since that is the only place they are used. I notice most MFD
drivers centralize this in linux/mfd/*.h. However, generally chunk
of register defs are only used in one subdriver and not shared. If
it's preferred to move all register defs centrally I can do that.

> > +/* bcm59056 chip access */
> 
> Superfluous comment? Don't we all know what these containers do?

Indeed, will remove.

Thanks for reviewing.

-Matt

> > +struct bcm59056 {
> > +	struct device *dev;
> > +	struct i2c_client *i2c_client;
> > +	struct regmap *regmap;
> > +	unsigned int id;
> > +};
> > +
> > +#endif /*  __LINUX_MFD_BCM59056_H */
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Lee Jones]

Hold your horses. :)

> > > Add a driver for the BCM59056 PMU multi-function device. The driver
> > > initially supports regmap initialization and instantiation of the
> > > voltage regulator device function of the PMU.
> > > 
> > > Signed-off-by: Matt Porter <mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > Reviewed-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > ---
> > >  drivers/mfd/Kconfig          |   8 +++
> > >  drivers/mfd/Makefile         |   1 +
> > >  drivers/mfd/bcm59056.c       | 119 +++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/bcm59056.h |  35 +++++++++++++
> > >  4 files changed, 163 insertions(+)
> > >  create mode 100644 drivers/mfd/bcm59056.c
> > >  create mode 100644 include/linux/mfd/bcm59056.h

<snip>

> > I thought this was a DT only driver? If so, you probably want to use
> > of_match_device() instead.
> 
> Correct, I'll use of_match_device()

If you're going to use this ...

<snip>

> > You've gone to the trouble of setting a device version here, but you
> > don't seem to use it?
> 
> I'll drop the device version

... then you'll still need this.

> > > +static const struct i2c_device_id bcm59056_i2c_id[] = {
> > > +	{ "bcm59056", BCM59056 },
> > > +	{ }
> > > +};
> > 
> > Ah, I guess this is where id->driver comes from.
> > 
> > It's pretty silly that the I2C subsystem demands the presence of this
> > table, despite not using it if an of_device_id table is present.
> 
> It does make it very difficult to follow DT enabled I2C client drivers
> :( I'll drop the BCM59056 too.

And I don't think you can drop this, as the I2C subsystem still
insists on it.

> > > +/* chip id */
> > > +#define BCM59056		0
> > 
> > Lonely, oh so lonely!
> 
> Understood. Will remove.

I think you need to keep this to supply the silly I2C ID table.

I would just omit the '.data = (void *) VERSION' from the
of_match_table until you require it. 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Matt Porter]

On Tue, Feb 04, 2014 at 02:47:31PM +0000, Lee Jones wrote:
> Hold your horses. :)

Holding :)

> > > > Add a driver for the BCM59056 PMU multi-function device. The driver
> > > > initially supports regmap initialization and instantiation of the
> > > > voltage regulator device function of the PMU.
> > > > 
> > > > Signed-off-by: Matt Porter <mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > Reviewed-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > ---
> > > >  drivers/mfd/Kconfig          |   8 +++
> > > >  drivers/mfd/Makefile         |   1 +
> > > >  drivers/mfd/bcm59056.c       | 119 +++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/mfd/bcm59056.h |  35 +++++++++++++
> > > >  4 files changed, 163 insertions(+)
> > > >  create mode 100644 drivers/mfd/bcm59056.c
> > > >  create mode 100644 include/linux/mfd/bcm59056.h
> 
> <snip>
> 
> > > I thought this was a DT only driver? If so, you probably want to use
> > > of_match_device() instead.
> > 
> > Correct, I'll use of_match_device()
> 
> If you're going to use this ...
> 
> <snip>
> 
> > > You've gone to the trouble of setting a device version here, but you
> > > don't seem to use it?
> > 
> > I'll drop the device version
> 
> ... then you'll still need this.

Yes, I was far too vague..I'm going to stop explicitly populating the
data field.

static const struct of_device_id bcm59056_of_match[] = {
	{ .compatible = "brcm,bcm59056"},
	{ }
};

> > > > +static const struct i2c_device_id bcm59056_i2c_id[] = {
> > > > +	{ "bcm59056", BCM59056 },
> > > > +	{ }
> > > > +};
> > > 
> > > Ah, I guess this is where id->driver comes from.
> > > 
> > > It's pretty silly that the I2C subsystem demands the presence of this
> > > table, despite not using it if an of_device_id table is present.
> > 
> > It does make it very difficult to follow DT enabled I2C client drivers
> > :( I'll drop the BCM59056 too.
> 
> And I don't think you can drop this, as the I2C subsystem still
> insists on it.

Agreed. I'm just dropping explicit population of the driver_data field
here.

static const struct i2c_device_id bcm59056_i2c_id[] = {
	{ "bcm59056" },
	{ }
};

> 
> > > > +/* chip id */
> > > > +#define BCM59056		0
> > > 
> > > Lonely, oh so lonely!
> > 
> > Understood. Will remove.
> 
> I think you need to keep this to supply the silly I2C ID table.
> 
> I would just omit the '.data = (void *) VERSION' from the
> of_match_table until you require it. 

Well, it's not even necessary for I2C ID table. the I2C subsystem is
happy with just a matching entry on the i2c_device_id name field and
NULL driver_data. palmas is implemented in this manner.

-Matt

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Lee Jones]

> > ... then you'll still need this.
> 
> Yes, I was far too vague..I'm going to stop explicitly populating the
> data field.
> 
> static const struct of_device_id bcm59056_of_match[] = {
> 	{ .compatible = "brcm,bcm59056"},
> 	{ }
> };

+1

> > And I don't think you can drop this, as the I2C subsystem still
> > insists on it.
> 
> Agreed. I'm just dropping explicit population of the driver_data field
> here.
> 
> static const struct i2c_device_id bcm59056_i2c_id[] = {
> 	{ "bcm59056" },
> 	{ }
> };

+1

> > I think you need to keep this to supply the silly I2C ID table.
> > 
> > I would just omit the '.data = (void *) VERSION' from the
> > of_match_table until you require it. 
> 
> Well, it's not even necessary for I2C ID table. the I2C subsystem is
> happy with just a matching entry on the i2c_device_id name field and
> NULL driver_data. palmas is implemented in this manner.

Guess what? +1 :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 3/6] mfd: add bcm59056 pmu driver

[Mark Brown]

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

On Tue, Feb 04, 2014 at 09:31:19AM -0500, Matt Porter wrote:
> On Tue, Feb 04, 2014 at 01:29:51PM +0000, Lee Jones wrote:

> > > +static struct i2c_driver bcm59056_i2c_driver = {
> > > +	.driver = {
> > > +		   .name = "bcm59056",
> > > +		   .owner = THIS_MODULE,
> > > +		   .of_match_table = of_match_ptr(bcm59056_of_match),

> > No need to use of_match_ptr() here.

> Will remove.

What using of_match_ptr() should do is allow the compiler to figure out
that the table isn't used when DT is disabled and discard it without
ifdefs.  Not sure if that actually works yet but that's the idea.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Lee Jones]

> > > > +static struct i2c_driver bcm59056_i2c_driver = {
> > > > +	.driver = {
> > > > +		   .name = "bcm59056",
> > > > +		   .owner = THIS_MODULE,
> > > > +		   .of_match_table = of_match_ptr(bcm59056_of_match),
> 
> > > No need to use of_match_ptr() here.
> 
> > Will remove.
> 
> What using of_match_ptr() should do is allow the compiler to figure out
> that the table isn't used when DT is disabled and discard it without
> ifdefs.  Not sure if that actually works yet but that's the idea.

Right, but I'm guessing that as there is no support for platform data
then this device(s) is going to be DT only? If that's the case perhaps
a 'depends OF' might be in order. If that's not the case then I'm more
than happy to leave the of_match_ptr() in.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] mfd: add bcm59056 pmu driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 583 bytes --]

On Tue, Feb 04, 2014 at 05:08:32PM +0000, Lee Jones wrote:

> > What using of_match_ptr() should do is allow the compiler to figure out
> > that the table isn't used when DT is disabled and discard it without
> > ifdefs.  Not sure if that actually works yet but that's the idea.

> Right, but I'm guessing that as there is no support for platform data
> then this device(s) is going to be DT only? If that's the case perhaps
> a 'depends OF' might be in order. If that's not the case then I'm more
> than happy to leave the of_match_ptr() in.

Hey, we'll all be using ACPI soon!  :P

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] regulator: add bcm59056 regulator driver

[Matt Porter]

Add a regulator driver for the BCM59056 PMU voltage regulators.
The driver supports LDOs and DCDCs in normal mode only. There is
no support for low-power mode or power sequencing.

Signed-off-by: Matt Porter <mporter@linaro.org>
Reviewed-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 drivers/regulator/Kconfig              |   8 +
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/bcm59056-regulator.c | 445 +++++++++++++++++++++++++++++++++
 3 files changed, 454 insertions(+)
 create mode 100644 drivers/regulator/bcm59056-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 6a79328..e09c9ea5 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -139,6 +139,14 @@ config REGULATOR_AS3722
 	  AS3722 PMIC. This will enable support for all the software
 	  controllable DCDC/LDO regulators.
 
+config REGULATOR_BCM59056
+	tristate "Broadcom BCM59056 PMU Regulators"
+	depends on MFD_BCM59056
+	help
+	  This driver provides support for the voltage regulators on the
+	  BCM59056 PMU. This will enable support for the software
+	  controllable LDO/Switching regulators.
+
 config REGULATOR_DA903X
 	tristate "Dialog Semiconductor DA9030/DA9034 regulators"
 	depends on PMIC_DA903X
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 979f9dd..bb65035 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 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_BCM59056) += bcm59056-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
 obj-$(CONFIG_REGULATOR_DA9052)	+= da9052-regulator.o
 obj-$(CONFIG_REGULATOR_DA9055)	+= da9055-regulator.o
diff --git a/drivers/regulator/bcm59056-regulator.c b/drivers/regulator/bcm59056-regulator.c
new file mode 100644
index 0000000..3fbc1d5
--- /dev/null
+++ b/drivers/regulator/bcm59056-regulator.c
@@ -0,0 +1,445 @@
+/*
+ * Broadcom BCM59056 regulator driver
+ *
+ * Copyright 2014 Linaro Limited
+ * Author: Matt Porter <mporter@linaro.org>
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mfd/bcm59056.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+/* Register defs */
+#define BCM59056_RFLDOPMCTRL1	0x60
+#define BCM59056_IOSR1PMCTRL1	0x7a
+#define BCM59056_IOSR2PMCTRL1	0x7c
+#define BCM59056_CSRPMCTRL1	0x7e
+#define BCM59056_SDSR1PMCTRL1	0x82
+#define BCM59056_SDSR2PMCTRL1	0x86
+#define BCM59056_MSRPMCTRL1	0x8a
+#define BCM59056_VSRPMCTRL1	0x8e
+#define BCM59056_REG_ENABLE	BIT(7)
+
+#define BCM59056_RFLDOCTRL	0x96
+#define BCM59056_CSRVOUT1	0xc0
+#define BCM59056_LDO_VSEL_MASK	GENMASK(5, 3)
+#define BCM59056_SR_VSEL_MASK	GENMASK(5, 0)
+
+/* LDO regulator IDs */
+#define BCM59056_REG_RFLDO	0
+#define BCM59056_REG_CAMLDO1	1
+#define BCM59056_REG_CAMLDO2	2
+#define BCM59056_REG_SIMLDO1	3
+#define BCM59056_REG_SIMLDO2	4
+#define BCM59056_REG_SDLDO	5
+#define BCM59056_REG_SDXLDO	6
+#define BCM59056_REG_MMCLDO1	7
+#define BCM59056_REG_MMCLDO2	8
+#define BCM59056_REG_AUDLDO	9
+#define BCM59056_REG_MICLDO	10
+#define BCM59056_REG_USBLDO	11
+#define BCM59056_REG_VIBLDO	12
+
+/* DCDC regulator IDs */
+#define BCM59056_REG_CSR	13
+#define BCM59056_REG_IOSR1	14
+#define BCM59056_REG_IOSR2	15
+#define BCM59056_REG_MSR	16
+#define BCM59056_REG_SDSR1	17
+#define BCM59056_REG_SDSR2	18
+#define BCM59056_REG_VSR	19
+
+#define BCM59056_NUM_REGS	20
+
+#define BCM59056_REG_IS_LDO(n)	(n < BCM59056_REG_CSR)
+
+struct bcm59056_board {
+	struct regulator_init_data *bcm59056_pmu_init_data[BCM59056_NUM_REGS];
+};
+
+/* LDO group A: supported voltages in microvolts */
+static const unsigned int ldo_a_table[] = {
+	1200000, 1800000, 2500000, 2700000, 2800000,
+	2900000, 3000000, 3300000,
+};
+
+/* LDO group C: supported voltages in microvolts */
+static const unsigned int ldo_c_table[] = {
+	3100000, 1800000, 2500000, 2700000, 2800000,
+	2900000, 3000000, 3300000,
+};
+
+/* DCDC group CSR: supported voltages in microvolts */
+static const struct regulator_linear_range dcdc_csr_ranges[] = {
+	REGULATOR_LINEAR_RANGE(860000, 2, 50, 10000),
+	REGULATOR_LINEAR_RANGE(1360000, 51, 55, 20000),
+	REGULATOR_LINEAR_RANGE(900000, 56, 63, 0),
+};
+
+/* DCDC group IOSR1: supported voltages in microvolts */
+static const struct regulator_linear_range dcdc_iosr1_ranges[] = {
+	REGULATOR_LINEAR_RANGE(860000, 2, 51, 10000),
+	REGULATOR_LINEAR_RANGE(1500000, 52, 52, 0),
+	REGULATOR_LINEAR_RANGE(1800000, 53, 53, 0),
+	REGULATOR_LINEAR_RANGE(900000, 54, 63, 0),
+};
+
+/* DCDC group SDSR1: supported voltages in microvolts */
+static const struct regulator_linear_range dcdc_sdsr1_ranges[] = {
+	REGULATOR_LINEAR_RANGE(860000, 2, 50, 10000),
+	REGULATOR_LINEAR_RANGE(1340000, 51, 51, 0),
+	REGULATOR_LINEAR_RANGE(900000, 52, 63, 0),
+};
+
+struct bcm59056_info {
+	const char *name;
+	const char *vin_name;
+	u8 n_voltages;
+	const unsigned int *volt_table;
+	u8 n_linear_ranges;
+	const struct regulator_linear_range *linear_ranges;
+};
+
+#define BCM59056_REG_TABLE(_name, _table) \
+	{ \
+		.name = #_name, \
+		.n_voltages = ARRAY_SIZE(_table), \
+		.volt_table = _table, \
+	}
+
+#define BCM59056_REG_RANGES(_name, _ranges) \
+	{ \
+		.name = #_name, \
+		.n_linear_ranges = ARRAY_SIZE(_ranges), \
+		.linear_ranges = _ranges, \
+	}
+
+static struct bcm59056_info bcm59056_regs[] = {
+	BCM59056_REG_TABLE(rfldo, ldo_a_table),
+	BCM59056_REG_TABLE(camldo1, ldo_c_table),
+	BCM59056_REG_TABLE(camldo2, ldo_c_table),
+	BCM59056_REG_TABLE(simldo1, ldo_a_table),
+	BCM59056_REG_TABLE(simldo2, ldo_a_table),
+	BCM59056_REG_TABLE(sdldo, ldo_c_table),
+	BCM59056_REG_TABLE(sdxldo, ldo_a_table),
+	BCM59056_REG_TABLE(mmcldo1, ldo_a_table),
+	BCM59056_REG_TABLE(mmcldo2, ldo_a_table),
+	BCM59056_REG_TABLE(audldo, ldo_a_table),
+	BCM59056_REG_TABLE(micldo, ldo_a_table),
+	BCM59056_REG_TABLE(usbldo, ldo_a_table),
+	BCM59056_REG_TABLE(vibldo, ldo_c_table),
+	BCM59056_REG_RANGES(csr, dcdc_csr_ranges),
+	BCM59056_REG_RANGES(iosr1, dcdc_iosr1_ranges),
+	BCM59056_REG_RANGES(iosr2, dcdc_iosr1_ranges),
+	BCM59056_REG_RANGES(msr, dcdc_iosr1_ranges),
+	BCM59056_REG_RANGES(sdsr1, dcdc_sdsr1_ranges),
+	BCM59056_REG_RANGES(sdsr2, dcdc_iosr1_ranges),
+	BCM59056_REG_RANGES(vsr, dcdc_iosr1_ranges),
+};
+
+struct bcm59056_reg {
+	struct regulator_desc *desc;
+	struct bcm59056 *mfd;
+	struct regulator_dev **rdev;
+	struct bcm59056_info **info;
+};
+
+static int bcm59056_get_vsel_register(int id)
+{
+	if (BCM59056_REG_IS_LDO(id))
+		return BCM59056_RFLDOCTRL + id;
+	else
+		return BCM59056_CSRVOUT1 + (id - BCM59056_REG_CSR) * 3;
+}
+
+static int bcm59056_get_enable_register(int id)
+{
+	int reg = 0;
+
+	if (BCM59056_REG_IS_LDO(id))
+		reg = BCM59056_RFLDOPMCTRL1 + id * 2;
+	else
+		switch (id) {
+		case BCM59056_REG_CSR:
+			reg = BCM59056_CSRPMCTRL1;
+			break;
+		case BCM59056_REG_IOSR1:
+			reg = BCM59056_IOSR1PMCTRL1;
+			break;
+		case BCM59056_REG_IOSR2:
+			reg = BCM59056_IOSR2PMCTRL1;
+			break;
+		case BCM59056_REG_MSR:
+			reg = BCM59056_MSRPMCTRL1;
+			break;
+		case BCM59056_REG_SDSR1:
+			reg = BCM59056_SDSR1PMCTRL1;
+			break;
+		case BCM59056_REG_SDSR2:
+			reg = BCM59056_SDSR2PMCTRL1;
+			break;
+		};
+
+	return reg;
+}
+
+static unsigned int bcm59056_get_mode(struct regulator_dev *dev)
+{
+	return REGULATOR_MODE_NORMAL;
+}
+
+static int bcm59056_set_mode(struct regulator_dev *dev, unsigned int mode)
+{
+	if (mode == REGULATOR_MODE_NORMAL)
+		return 0;
+	else
+		return -EINVAL;
+}
+
+static struct regulator_ops bcm59056_ops_ldo = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.set_mode		= bcm59056_set_mode,
+	.get_mode		= bcm59056_get_mode,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_table,
+	.map_voltage		= regulator_map_voltage_iterate,
+};
+
+static struct regulator_ops bcm59056_ops_dcdc = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.set_mode		= bcm59056_set_mode,
+	.get_mode		= bcm59056_get_mode,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+};
+
+#define BCM59056_MATCH(_name, _id) \
+	{ \
+		.name = #_name, \
+		.driver_data = (void *)&bcm59056_regs[BCM59056_REG_##_id], \
+	}
+
+static struct of_regulator_match bcm59056_matches[] = {
+	BCM59056_MATCH(rfldo, RFLDO),
+	BCM59056_MATCH(camldo1, CAMLDO1),
+	BCM59056_MATCH(camldo2, CAMLDO2),
+	BCM59056_MATCH(simldo1, SIMLDO1),
+	BCM59056_MATCH(simldo2, SIMLDO2),
+	BCM59056_MATCH(sdldo, SDLDO),
+	BCM59056_MATCH(sdxldo, SDXLDO),
+	BCM59056_MATCH(mmcldo1, MMCLDO1),
+	BCM59056_MATCH(mmcldo2, MMCLDO2),
+	BCM59056_MATCH(audldo, AUDLDO),
+	BCM59056_MATCH(micldo, MICLDO),
+	BCM59056_MATCH(usbldo, USBLDO),
+	BCM59056_MATCH(vibldo, VIBLDO),
+	BCM59056_MATCH(csr, CSR),
+	BCM59056_MATCH(iosr1, IOSR1),
+	BCM59056_MATCH(iosr2, IOSR2),
+	BCM59056_MATCH(msr, MSR),
+	BCM59056_MATCH(sdsr1, SDSR1),
+	BCM59056_MATCH(sdsr2, SDSR2),
+	BCM59056_MATCH(vsr, VSR),
+};
+
+static struct bcm59056_board *bcm59056_parse_dt_reg_data(
+		struct platform_device *pdev,
+		struct of_regulator_match **bcm59056_reg_matches)
+{
+	struct bcm59056_board *data;
+	struct device_node *np, *regulators;
+	struct of_regulator_match *matches = bcm59056_matches;
+	int count = ARRAY_SIZE(bcm59056_matches);
+	int idx = 0;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&pdev->dev, "failed to allocate regulator board data\n");
+		return NULL;
+	}
+
+	np = of_node_get(pdev->dev.parent->of_node);
+	regulators = of_get_child_by_name(np, "regulators");
+	if (!regulators) {
+		dev_err(&pdev->dev, "regulator node not found\n");
+		return NULL;
+	}
+
+	ret = of_regulator_match(&pdev->dev, regulators, matches, count);
+	of_node_put(regulators);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Error parsing regulator init data: %d\n",
+			ret);
+		return NULL;
+	}
+
+	*bcm59056_reg_matches = matches;
+
+	for (idx = 0; idx < count; idx++) {
+		if (!matches[idx].init_data || !matches[idx].of_node)
+			continue;
+
+		data->bcm59056_pmu_init_data[idx] = matches[idx].init_data;
+	}
+
+	return data;
+}
+
+static int bcm59056_probe(struct platform_device *pdev)
+{
+	struct bcm59056 *bcm59056 = dev_get_drvdata(pdev->dev.parent);
+	struct bcm59056_board *pmu_data = NULL;
+	struct bcm59056_reg *pmu;
+	struct regulator_config config = { };
+	struct bcm59056_info *info;
+	struct regulator_init_data *reg_data;
+	struct regulator_dev *rdev;
+	struct of_regulator_match *bcm59056_reg_matches = NULL;
+	int i;
+
+	if (bcm59056->dev->of_node)
+		pmu_data = bcm59056_parse_dt_reg_data(pdev,
+						      &bcm59056_reg_matches);
+
+	if (!pmu_data) {
+		dev_err(&pdev->dev, "Platform data not found\n");
+		return -EINVAL;
+	}
+
+	pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu) {
+		dev_err(&pdev->dev, "Memory allocation failed for pmu\n");
+		return -ENOMEM;
+	}
+
+	pmu->mfd = bcm59056;
+
+	platform_set_drvdata(pdev, pmu);
+
+	pmu->desc = devm_kzalloc(&pdev->dev, BCM59056_NUM_REGS *
+			sizeof(struct regulator_desc), GFP_KERNEL);
+	if (!pmu->desc) {
+		dev_err(&pdev->dev, "Memory alloc fails for desc\n");
+		return -ENOMEM;
+	}
+
+	pmu->info = devm_kzalloc(&pdev->dev, BCM59056_NUM_REGS *
+			sizeof(struct bcm59056_info *), GFP_KERNEL);
+	if (!pmu->info) {
+		dev_err(&pdev->dev, "Memory alloc fails for info\n");
+		return -ENOMEM;
+	}
+
+	pmu->rdev = devm_kzalloc(&pdev->dev, BCM59056_NUM_REGS *
+			sizeof(struct regulator_dev *), GFP_KERNEL);
+	if (!pmu->rdev) {
+		dev_err(&pdev->dev, "Memory alloc fails for rdev\n");
+		return -ENOMEM;
+	}
+
+	info = bcm59056_regs;
+
+	for (i = 0; i < BCM59056_NUM_REGS; i++, info++) {
+		reg_data = pmu_data->bcm59056_pmu_init_data[i];
+
+		/*
+		 * Regulator API handles empty constraints but not NULL
+		 * constraints
+		 */
+		if (!reg_data)
+			continue;
+
+		/* Register the regulators */
+		pmu->info[i] = info;
+
+		pmu->desc[i].name = info->name;
+		pmu->desc[i].supply_name = info->vin_name;
+		pmu->desc[i].id = i;
+		pmu->desc[i].volt_table = info->volt_table;
+		pmu->desc[i].n_voltages = info->n_voltages;
+		pmu->desc[i].linear_ranges = info->linear_ranges;
+		pmu->desc[i].n_linear_ranges = info->n_linear_ranges;
+
+		if (BCM59056_REG_IS_LDO(i)) {
+			pmu->desc[i].ops = &bcm59056_ops_ldo;
+			pmu->desc[i].vsel_mask = BCM59056_LDO_VSEL_MASK;
+		} else {
+			pmu->desc[i].ops = &bcm59056_ops_dcdc;
+			pmu->desc[i].vsel_mask = BCM59056_SR_VSEL_MASK;
+		}
+
+		pmu->desc[i].vsel_reg = bcm59056_get_vsel_register(i);
+		pmu->desc[i].enable_is_inverted = true;
+		pmu->desc[i].enable_mask = BCM59056_REG_ENABLE;
+		pmu->desc[i].enable_reg = bcm59056_get_enable_register(i);
+		pmu->desc[i].type = REGULATOR_VOLTAGE;
+		pmu->desc[i].owner = THIS_MODULE;
+
+		config.dev = bcm59056->dev;
+		config.init_data = reg_data;
+		config.driver_data = pmu;
+		config.regmap = bcm59056->regmap;
+
+		if (bcm59056_reg_matches)
+			config.of_node = bcm59056_reg_matches[i].of_node;
+
+		rdev = devm_regulator_register(&pdev->dev, &pmu->desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(bcm59056->dev,
+				"failed to register %s regulator\n",
+				pdev->name);
+			return PTR_ERR(rdev);
+		}
+
+		pmu->rdev[i] = rdev;
+	}
+
+	return 0;
+}
+
+static struct platform_driver bcm59056_regulator_driver = {
+	.driver = {
+		.name = "bcm59056-pmu",
+		.owner = THIS_MODULE,
+	},
+	.probe = bcm59056_probe,
+};
+
+static int __init bcm59056_regulator_init(void)
+{
+	return platform_driver_register(&bcm59056_regulator_driver);
+}
+subsys_initcall(bcm59056_regulator_init);
+
+static void __exit bcm59056_regulator_exit(void)
+{
+	platform_driver_unregister(&bcm59056_regulator_driver);
+}
+module_exit(bcm59056_regulator_exit);
+
+MODULE_AUTHOR("Matt Porter <mporter@linaro.org>");
+MODULE_DESCRIPTION("BCM59056 voltage regulator driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:bcm59056-regulator");
-- 
1.8.4

Re: [PATCH 4/6] regulator: add bcm59056 regulator driver

[Mark Brown]

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

On Tue, Feb 04, 2014 at 07:19:10AM -0500, Matt Porter wrote:

> +static unsigned int bcm59056_get_mode(struct regulator_dev *dev)
> +{
> +	return REGULATOR_MODE_NORMAL;
> +}
> +
> +static int bcm59056_set_mode(struct regulator_dev *dev, unsigned int mode)
> +{
> +	if (mode == REGULATOR_MODE_NORMAL)
> +		return 0;
> +	else
> +		return -EINVAL;
> +}

These do nothing, don't implement them.

> +	if (bcm59056->dev->of_node)
> +		pmu_data = bcm59056_parse_dt_reg_data(pdev,
> +						      &bcm59056_reg_matches);

It'd seem a bit neater to put the OF check into the parse function but
meh.

> +	if (!pmu_data) {
> +		dev_err(&pdev->dev, "Platform data not found\n");
> +		return -EINVAL;
> +	}

Like I said when reviewing the binding this should not cause the driver
to fail to load.

> +		/*
> +		 * Regulator API handles empty constraints but not NULL
> +		 * constraints
> +		 */
> +		if (!reg_data)
> +			continue;

It should do...  if not then make it so since that'd mean most drivers
are buggy.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/6] regulator: add bcm59056 regulator driver

[Matt Porter]

On Tue, Feb 04, 2014 at 05:28:36PM +0000, Mark Brown wrote:
> On Tue, Feb 04, 2014 at 07:19:10AM -0500, Matt Porter wrote:
> 
> > +static unsigned int bcm59056_get_mode(struct regulator_dev *dev)
> > +{
> > +	return REGULATOR_MODE_NORMAL;
> > +}
> > +
> > +static int bcm59056_set_mode(struct regulator_dev *dev, unsigned int mode)
> > +{
> > +	if (mode == REGULATOR_MODE_NORMAL)
> > +		return 0;
> > +	else
> > +		return -EINVAL;
> > +}
> 
> These do nothing, don't implement them.

Will remove. Maybe some day.

> > +	if (bcm59056->dev->of_node)
> > +		pmu_data = bcm59056_parse_dt_reg_data(pdev,
> > +						      &bcm59056_reg_matches);
> 
> It'd seem a bit neater to put the OF check into the parse function but
> meh.

On second look, I'd agree. Easy enough to clean up.

> > +	if (!pmu_data) {
> > +		dev_err(&pdev->dev, "Platform data not found\n");
> > +		return -EINVAL;
> > +	}
> 
> Like I said when reviewing the binding this should not cause the driver
> to fail to load.

Will fix.

> > +		/*
> > +		 * Regulator API handles empty constraints but not NULL
> > +		 * constraints
> > +		 */
> > +		if (!reg_data)
> > +			continue;
> 
> It should do...  if not then make it so since that'd mean most drivers
> are buggy.

Ahh, I see there is a check for NULL in the core. Will drop.

-Matt

Re: [PATCH 4/6] regulator: add bcm59056 regulator driver

[Mark Brown]

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

On Tue, Feb 04, 2014 at 04:29:14PM -0500, Matt Porter wrote:
> On Tue, Feb 04, 2014 at 05:28:36PM +0000, Mark Brown wrote:

> > > +		/*
> > > +		 * Regulator API handles empty constraints but not NULL
> > > +		 * constraints
> > > +		 */
> > > +		if (!reg_data)
> > > +			continue;

> > It should do...  if not then make it so since that'd mean most drivers
> > are buggy.

> Ahh, I see there is a check for NULL in the core. Will drop.

It was missing in some older kernels (much older now IIRC) - it's
possible that comment was written before this got fixed.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 6/6] ARM: dts: add bcm59056 pmu support and enable for bcm28155-ap

[Matt Porter]

Add a dtsi to support the BCM59056 PMU used by the BCM281xx family
of SoCs. Enable regulators for use with the dwc2 and sdhci on
bcm28155-ap.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Signed-off-by: Matt Porter <mporter@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 arch/arm/boot/dts/bcm28155-ap.dts |  41 ++++++++++
 arch/arm/boot/dts/bcm59056.dtsi   | 158 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 199 insertions(+)
 create mode 100644 arch/arm/boot/dts/bcm59056.dtsi

diff --git a/arch/arm/boot/dts/bcm28155-ap.dts b/arch/arm/boot/dts/bcm28155-ap.dts
index 5ff2382..1240f42 100644
--- a/arch/arm/boot/dts/bcm28155-ap.dts
+++ b/arch/arm/boot/dts/bcm28155-ap.dts
@@ -47,6 +47,10 @@
 	i2c@3500d000 {
 		status="okay";
 		clock-frequency = <400000>;
+
+		pmu: pmu@8 {
+			reg = <0x08>;
+		};
 	};
 
 	sdio1: sdio@3f180000 {
@@ -57,16 +61,22 @@
 	sdio2: sdio@3f190000 {
 		non-removable;
 		max-frequency = <48000000>;
+		vmmc-supply = <&camldo1_reg>;
+		vqmmc-supply = <&iosr1_reg>;
 		status = "okay";
 	};
 
 	sdio4: sdio@3f1b0000 {
 		max-frequency = <48000000>;
 		cd-gpios = <&gpio 14 GPIO_ACTIVE_LOW>;
+		vmmc-supply = <&sdldo_reg>;
+		vqmmc-supply = <&sdxldo_reg>;
 		status = "okay";
 	};
 
 	usbotg: usb@3f120000 {
+		vusb_d-supply = <&usbldo_reg>;
+		vusb_a-supply = <&iosr1_reg>;
 		status = "okay";
 	};
 
@@ -74,3 +84,34 @@
 		status = "okay";
 	};
 };
+
+#include "bcm59056.dtsi"
+
+&pmu {
+	interrupts = <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>;
+	status = "okay";
+
+	regulators {
+		camldo1_reg: regulator@1 {
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		sdldo_reg: regulator@5 {
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <3000000>;
+		};
+
+		usbldo_reg: regulator@11 {
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-always-on;
+		};
+
+		iosr1_reg: regulator@14 {
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-always-on;
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/bcm59056.dtsi b/arch/arm/boot/dts/bcm59056.dtsi
new file mode 100644
index 0000000..08ea3da
--- /dev/null
+++ b/arch/arm/boot/dts/bcm59056.dtsi
@@ -0,0 +1,158 @@
+/*
+ * Copyright 2014 Linaro Limited
+ * Author: Matt Porter <mporter@linaro.org>
+ *
+ * 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.
+ */
+
+&pmu {
+	compatible = "brcm,bcm59056";
+
+	regulators {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		rfldo_reg: regulator@0 {
+			reg = <0>;
+			regulator-compatible = "rfldo";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		camldo1_reg: regulator@1 {
+			reg = <1>;
+			regulator-compatible = "camldo1";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		camldo2_reg: regulator@2 {
+			reg = <2>;
+			regulator-compatible = "camldo2";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		simldo1_reg: regulator@3 {
+			reg = <3>;
+			regulator-compatible = "simldo1";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		simldo2_reg: regulator@4 {
+			reg = <4>;
+			regulator-compatible = "simldo2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		sdldo_reg: regulator@5 {
+			reg = <5>;
+			regulator-compatible = "sdldo";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		sdxldo_reg: regulator@6 {
+			reg = <6>;
+			regulator-compatible = "sdxldo";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		mmcldo1_reg: regulator@7 {
+			reg = <7>;
+			regulator-compatible = "mmcldo1";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		mmcldo2_reg: regulator@8 {
+			reg = <8>;
+			regulator-compatible = "mmcldo2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		audldo_reg: regulator@9 {
+			reg = <9>;
+			regulator-compatible = "audldo";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		micldo_reg: regulator@10 {
+			reg = <10>;
+			regulator-compatible = "micldo";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		usbldo_reg: regulator@11 {
+			reg = <11>;
+			regulator-compatible = "usbldo";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		vibldo_reg: regulator@12 {
+			reg = <12>;
+			regulator-compatible = "vibldo";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		csr_reg: regulator@13 {
+			reg = <13>;
+			regulator-compatible = "csr";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1440000>;
+		};
+
+		iosr1_reg: regulator@14 {
+			reg = <14>;
+			regulator-compatible = "iosr1";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1800000>;
+		};
+
+		iosr2_reg: regulator@15 {
+			reg = <15>;
+			regulator-compatible = "iosr2";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1800000>;
+		};
+
+		msr_reg: regulator@16 {
+			reg = <16>;
+			regulator-compatible = "msr";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1800000>;
+		};
+
+		sdsr1_reg: regulator@17 {
+			reg = <17>;
+			regulator-compatible = "sdsr1";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1340000>;
+		};
+
+		sdsr2_reg: regulator@18 {
+			reg = <18>;
+			regulator-compatible = "sdsr2";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1800000>;
+		};
+
+		vsr_reg: regulator@19 {
+			reg = <19>;
+			regulator-compatible = "vsr";
+			regulator-min-microvolt = <860000>;
+			regulator-max-microvolt = <1800000>;
+		};
+	};
+};
-- 
1.8.4