diff mbox

[3/6] mfd: add bcm59056 pmu driver

Message ID 1391516352-32359-4-git-send-email-mporter@linaro.org
State Not Applicable
Headers show

Commit Message

Matt Porter Feb. 4, 2014, 12:19 p.m. UTC
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

Comments

Lee Jones Feb. 4, 2014, 1:29 p.m. UTC | #1
> 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 */
Matt Porter Feb. 4, 2014, 2:31 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Feb. 4, 2014, 2:47 p.m. UTC | #3
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@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>

> > 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.
Matt Porter Feb. 4, 2014, 3:01 p.m. UTC | #4
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@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>
> 
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Feb. 4, 2014, 3:20 p.m. UTC | #5
> > ... 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 :)
Mark Brown Feb. 4, 2014, 4:59 p.m. UTC | #6
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.
Lee Jones Feb. 4, 2014, 5:08 p.m. UTC | #7
> > > > +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.
Mark Brown Feb. 4, 2014, 5:11 p.m. UTC | #8
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
diff mbox

Patch

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