[2/4] mfd: bcm590xx: add support for second i2c slave address space

Message ID 1397501428-8857-3-git-send-email-mporter@linaro.org
State New
Headers show

Commit Message

Matt Porter April 14, 2014, 6:50 p.m.
BCM590xx utilizes a second i2c slave address to access additional
register space. Add support for the second address space by
instantiated a dummy i2c device with the appropriate secondary
i2c slave address. Expose a second regmap register space so that
mfd drivers can access this secondary i2c slave address space.

Signed-off-by: Matt Porter <mporter@linaro.org>
---
 drivers/mfd/bcm590xx.c       | 60 +++++++++++++++++++++++++++++++++-----------
 include/linux/mfd/bcm590xx.h |  9 ++++---
 2 files changed, 52 insertions(+), 17 deletions(-)

Comments

Lee Jones April 16, 2014, 11:06 a.m. | #1
On Mon, 14 Apr 2014, Matt Porter wrote:

> BCM590xx utilizes a second i2c slave address to access additional

s/i2c/I2C

> register space. Add support for the second address space by
> instantiated a dummy i2c device with the appropriate secondary

s/instantiated/instantiating

> i2c slave address. Expose a second regmap register space so that

s/i2c/I2C

Exposing?

s/regmap/Regmap

> mfd drivers can access this secondary i2c slave address space.

s/mfd/MFD

s/i2c/I2C

> Signed-off-by: Matt Porter <mporter@linaro.org>
> ---
>  drivers/mfd/bcm590xx.c       | 60 +++++++++++++++++++++++++++++++++-----------
>  include/linux/mfd/bcm590xx.h |  9 ++++---
>  2 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
> index e9a33c7..b710ffa 100644
> --- a/drivers/mfd/bcm590xx.c
> +++ b/drivers/mfd/bcm590xx.c
> @@ -28,39 +28,71 @@ static const struct mfd_cell bcm590xx_devs[] = {
>  	},
>  };
>  
> -static const struct regmap_config bcm590xx_regmap_config = {
> +static const struct regmap_config bcm590xx_regmap_config_0 = {

Not loving _0 and _1 appendages.

Is one of them {primary|master} and the other {secondary|slave}?

>  	.reg_bits	= 8,
>  	.val_bits	= 8,
> -	.max_register	= BCM590XX_MAX_REGISTER,
> +	.max_register	= BCM590XX_MAX_REGISTER_0,
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> -static int bcm590xx_i2c_probe(struct i2c_client *i2c,
> +static const struct regmap_config bcm590xx_regmap_config_1 = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= BCM590XX_MAX_REGISTER_1,
> +	.cache_type	= REGCACHE_RBTREE,
> +};
> +
> +static int bcm590xx_i2c_probe(struct i2c_client *addmap0,

Would this be best left as i2c, then naming the other one
i2c_secondary for instance?

addmap{0,1} doesn't quite sit right with me.

REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
as I first thought, but still, is there a better naming convention you
could use?

>  			      const struct i2c_device_id *id)
>  {
>  	struct bcm590xx *bcm590xx;
>  	int ret;
>  
> -	bcm590xx = devm_kzalloc(&i2c->dev, sizeof(*bcm590xx), GFP_KERNEL);
> +	bcm590xx = devm_kzalloc(&addmap0->dev, sizeof(*bcm590xx), GFP_KERNEL);
>  	if (!bcm590xx)
>  		return -ENOMEM;
>  
> -	i2c_set_clientdata(i2c, bcm590xx);
> -	bcm590xx->dev = &i2c->dev;
> -	bcm590xx->i2c_client = i2c;
> +	i2c_set_clientdata(addmap0, bcm590xx);
> +	bcm590xx->dev = &addmap0->dev;
> +	bcm590xx->addmap0 = addmap0;
>  
> -	bcm590xx->regmap = devm_regmap_init_i2c(i2c, &bcm590xx_regmap_config);
> -	if (IS_ERR(bcm590xx->regmap)) {
> -		ret = PTR_ERR(bcm590xx->regmap);
> -		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
> +	bcm590xx->regmap0 = devm_regmap_init_i2c(addmap0,
> +						 &bcm590xx_regmap_config_0);
> +	if (IS_ERR(bcm590xx->regmap0)) {
> +		ret = PTR_ERR(bcm590xx->regmap0);
> +		dev_err(&addmap0->dev, "regmap 0 init failed: %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = mfd_add_devices(&i2c->dev, -1, bcm590xx_devs,
> +	/* Second I2C slave address is the base address with A(2) asserted */
> +	bcm590xx->addmap1 = i2c_new_dummy(addmap0->adapter,
> +					  addmap0->addr | BIT(2));
> +	if (IS_ERR_OR_NULL(bcm590xx->addmap1)) {
> +		dev_err(&addmap0->dev, "failed to add address map 1 device\n");
> +		return -ENODEV;
> +	}
> +	i2c_set_clientdata(bcm590xx->addmap1, bcm590xx);
> +
> +	bcm590xx->regmap1 = devm_regmap_init_i2c(bcm590xx->addmap1,
> +						&bcm590xx_regmap_config_1);
> +	if (IS_ERR(bcm590xx->regmap1)) {
> +		ret = PTR_ERR(bcm590xx->regmap1);
> +		dev_err(&bcm590xx->addmap1->dev,
> +			"regmap 1 init failed: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = mfd_add_devices(&addmap0->dev, -1, bcm590xx_devs,
>  			      ARRAY_SIZE(bcm590xx_devs), NULL, 0, NULL);
> -	if (ret < 0)
> -		dev_err(&i2c->dev, "failed to add sub-devices: %d\n", ret);
> +	if (ret < 0) {
> +		dev_err(&addmap0->dev, "failed to add sub-devices: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
>  
> +err:
> +	i2c_unregister_device(bcm590xx->addmap1);
>  	return ret;
>  }
>  
> diff --git a/include/linux/mfd/bcm590xx.h b/include/linux/mfd/bcm590xx.h
> index 434df2d..a2723f2 100644
> --- a/include/linux/mfd/bcm590xx.h
> +++ b/include/linux/mfd/bcm590xx.h
> @@ -19,12 +19,15 @@
>  #include <linux/regmap.h>
>  
>  /* max register address */
> -#define BCM590XX_MAX_REGISTER	0xe7
> +#define BCM590XX_MAX_REGISTER_0	0xe7
> +#define BCM590XX_MAX_REGISTER_1	0xf0
>  
>  struct bcm590xx {
>  	struct device *dev;
> -	struct i2c_client *i2c_client;
> -	struct regmap *regmap;
> +	struct i2c_client *addmap0;
> +	struct i2c_client *addmap1;
> +	struct regmap *regmap0;
> +	struct regmap *regmap1;
>  	unsigned int id;
>  };
>
Mark Brown April 16, 2014, 9:31 p.m. | #2
On Wed, Apr 16, 2014 at 12:06:03PM +0100, Lee Jones wrote:

> s/regmap/Regmap

It's consistently written regmap in all the documentation and so on :)

> addmap{0,1} doesn't quite sit right with me.

> REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> as I first thought, but still, is there a better naming convention you
> could use?

addrmap or something?
Lee Jones April 17, 2014, 6:57 a.m. | #3
> > s/regmap/Regmap
> 
> It's consistently written regmap in all the documentation and so on :)

Furry muff; but the comments still stand for the acronyms.

> > addmap{0,1} doesn't quite sit right with me.
> 
> > REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> > as I first thought, but still, is there a better naming convention you
> > could use?
> 
> addrmap or something?

Right, that was what I was thinking. However, I prefer something along
the lines of 'i2c' and 'i2c_sec' or 'client' and 'client_slv' etc.
Matt Porter April 17, 2014, 10:26 p.m. | #4
On Thu, Apr 17, 2014 at 07:57:53AM +0100, Lee Jones wrote:
> > > s/regmap/Regmap
> > 
> > It's consistently written regmap in all the documentation and so on :)
> 
> Furry muff; but the comments still stand for the acronyms.
> 
> > > addmap{0,1} doesn't quite sit right with me.
> > 
> > > REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> > > as I first thought, but still, is there a better naming convention you
> > > could use?
> > 
> > addrmap or something?
> 
> Right, that was what I was thinking. However, I prefer something along
> the lines of 'i2c' and 'i2c_sec' or 'client' and 'client_slv' etc.

FWIW, the reason it's addmap{0,1} is that the datasheet has documents
ADDMAP=0 and the first bank of registers and ADDMAP=1 as the second bank
of registers. I adopted that to match the docs for the part.

I guess we could do i2c and i2c_sec, I'll just have to put a comment
correlating it to the h/w. Calling it 'slv' implies something else
so we should avoid that here. The notion of a "secondary" i2c device
is completely a Linux I2C subsystem fabrication which wouldn't exist
if it allowed multiple slave addresses per device. From a h/w
perspective there is really no primary and secondary relationship.

I'm fine with i2c/i2c_sec or addrmap0/1 and I will just comment to
correlate with the datasheet..pick one.

-Matt
> 
> -- 
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Porter April 17, 2014, 10:30 p.m. | #5
On Wed, Apr 16, 2014 at 12:06:03PM +0100, Lee Jones wrote:
> On Mon, 14 Apr 2014, Matt Porter wrote:
> 
> > BCM590xx utilizes a second i2c slave address to access additional
> 
> s/i2c/I2C
> 
> > register space. Add support for the second address space by
> > instantiated a dummy i2c device with the appropriate secondary
> 
> s/instantiated/instantiating
> 
> > i2c slave address. Expose a second regmap register space so that
> 
> s/i2c/I2C
> 
> Exposing?
> 
> s/regmap/Regmap
> 
> > mfd drivers can access this secondary i2c slave address space.
> 
> s/mfd/MFD
> 
> s/i2c/I2C

Ok, I'll fix the capitalization and wording..except for regmap as
noted by Mark.

> 
> > Signed-off-by: Matt Porter <mporter@linaro.org>
> > ---
> >  drivers/mfd/bcm590xx.c       | 60 +++++++++++++++++++++++++++++++++-----------
> >  include/linux/mfd/bcm590xx.h |  9 ++++---
> >  2 files changed, 52 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
> > index e9a33c7..b710ffa 100644
> > --- a/drivers/mfd/bcm590xx.c
> > +++ b/drivers/mfd/bcm590xx.c
> > @@ -28,39 +28,71 @@ static const struct mfd_cell bcm590xx_devs[] = {
> >  	},
> >  };
> >  
> > -static const struct regmap_config bcm590xx_regmap_config = {
> > +static const struct regmap_config bcm590xx_regmap_config_0 = {
> 
> Not loving _0 and _1 appendages.
> 
> Is one of them {primary|master} and the other {secondary|slave}?

I guess from a Linux I2C subsystem, we can view _1 as the
"secondary"...it does correspond the the i2c_new_dummy() device
that we create in the mfd probe. That device corresponds to the
ADDMAP=1 address on the PMU. This is why I used those appendages.

-Matt
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones April 22, 2014, 8:21 a.m. | #6
> > > > s/regmap/Regmap
> > > 
> > > It's consistently written regmap in all the documentation and so on :)
> > 
> > Furry muff; but the comments still stand for the acronyms.
> > 
> > > > addmap{0,1} doesn't quite sit right with me.
> > > 
> > > > REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> > > > as I first thought, but still, is there a better naming convention you
> > > > could use?
> > > 
> > > addrmap or something?
> > 
> > Right, that was what I was thinking. However, I prefer something along
> > the lines of 'i2c' and 'i2c_sec' or 'client' and 'client_slv' etc.
> 
> FWIW, the reason it's addmap{0,1} is that the datasheet has documents
> ADDMAP=0 and the first bank of registers and ADDMAP=1 as the second bank
> of registers. I adopted that to match the docs for the part.
> 
> I guess we could do i2c and i2c_sec, I'll just have to put a comment
> correlating it to the h/w. Calling it 'slv' implies something else
> so we should avoid that here. The notion of a "secondary" i2c device
> is completely a Linux I2C subsystem fabrication which wouldn't exist
> if it allowed multiple slave addresses per device. From a h/w
> perspective there is really no primary and secondary relationship.
> 
> I'm fine with i2c/i2c_sec or addrmap0/1 and I will just comment to
> correlate with the datasheet..pick one.

Let's stick method fabricated by the I2C subsystem. It may seem strange
from a h/w perspective, but it is the way we (you) have coded it, as
the first parameter of i2c_new_dummy() is the 'managing' (primary,
parent, master, whatever) device, so '_sec' would suit as an
identifying appendage for the resultant device.
Matt Porter April 23, 2014, 10:01 p.m. | #7
On Tue, Apr 22, 2014 at 09:21:39AM +0100, Lee Jones wrote:
> > > > > s/regmap/Regmap
> > > > 
> > > > It's consistently written regmap in all the documentation and so on :)
> > > 
> > > Furry muff; but the comments still stand for the acronyms.
> > > 
> > > > > addmap{0,1} doesn't quite sit right with me.
> > > > 
> > > > > REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> > > > > as I first thought, but still, is there a better naming convention you
> > > > > could use?
> > > > 
> > > > addrmap or something?
> > > 
> > > Right, that was what I was thinking. However, I prefer something along
> > > the lines of 'i2c' and 'i2c_sec' or 'client' and 'client_slv' etc.
> > 
> > FWIW, the reason it's addmap{0,1} is that the datasheet has documents
> > ADDMAP=0 and the first bank of registers and ADDMAP=1 as the second bank
> > of registers. I adopted that to match the docs for the part.
> > 
> > I guess we could do i2c and i2c_sec, I'll just have to put a comment
> > correlating it to the h/w. Calling it 'slv' implies something else
> > so we should avoid that here. The notion of a "secondary" i2c device
> > is completely a Linux I2C subsystem fabrication which wouldn't exist
> > if it allowed multiple slave addresses per device. From a h/w
> > perspective there is really no primary and secondary relationship.
> > 
> > I'm fine with i2c/i2c_sec or addrmap0/1 and I will just comment to
> > correlate with the datasheet..pick one.
> 
> Let's stick method fabricated by the I2C subsystem. It may seem strange
> from a h/w perspective, but it is the way we (you) have coded it, as
> the first parameter of i2c_new_dummy() is the 'managing' (primary,
> parent, master, whatever) device, so '_sec' would suit as an
> identifying appendage for the resultant device.

That works, I'll also switch to addrmap_[pri|sec] which touches the
regulator driver as well. That will keep the relationship between device
and regmap clear.

-Matt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Matt Porter April 23, 2014, 10:05 p.m. | #8
On Wed, Apr 23, 2014 at 06:01:26PM -0400, Matt Porter wrote:
> On Tue, Apr 22, 2014 at 09:21:39AM +0100, Lee Jones wrote:
> > > > > > s/regmap/Regmap
> > > > > 
> > > > > It's consistently written regmap in all the documentation and so on :)
> > > > 
> > > > Furry muff; but the comments still stand for the acronyms.
> > > > 
> > > > > > addmap{0,1} doesn't quite sit right with me.
> > > > > 
> > > > > > REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> > > > > > as I first thought, but still, is there a better naming convention you
> > > > > > could use?
> > > > > 
> > > > > addrmap or something?
> > > > 
> > > > Right, that was what I was thinking. However, I prefer something along
> > > > the lines of 'i2c' and 'i2c_sec' or 'client' and 'client_slv' etc.
> > > 
> > > FWIW, the reason it's addmap{0,1} is that the datasheet has documents
> > > ADDMAP=0 and the first bank of registers and ADDMAP=1 as the second bank
> > > of registers. I adopted that to match the docs for the part.
> > > 
> > > I guess we could do i2c and i2c_sec, I'll just have to put a comment
> > > correlating it to the h/w. Calling it 'slv' implies something else
> > > so we should avoid that here. The notion of a "secondary" i2c device
> > > is completely a Linux I2C subsystem fabrication which wouldn't exist
> > > if it allowed multiple slave addresses per device. From a h/w
> > > perspective there is really no primary and secondary relationship.
> > > 
> > > I'm fine with i2c/i2c_sec or addrmap0/1 and I will just comment to
> > > correlate with the datasheet..pick one.
> > 
> > Let's stick method fabricated by the I2C subsystem. It may seem strange
> > from a h/w perspective, but it is the way we (you) have coded it, as
> > the first parameter of i2c_new_dummy() is the 'managing' (primary,
> > parent, master, whatever) device, so '_sec' would suit as an
> > identifying appendage for the resultant device.
> 
> That works, I'll also switch to addrmap_[pri|sec] which touches the
> regulator driver as well. That will keep the relationship  between device
> and regmap clear.

Misspoke...I'm switching regmap[0|1] to regmap_[pri|sec] to keep that
synced with i2c_[pri|sec]

-Matt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Lee Jones April 28, 2014, 9:29 a.m. | #9
> > > > > > > s/regmap/Regmap
> > > > > > 
> > > > > > It's consistently written regmap in all the documentation and so on :)
> > > > > 
> > > > > Furry muff; but the comments still stand for the acronyms.
> > > > > 
> > > > > > > addmap{0,1} doesn't quite sit right with me.
> > > > > > 
> > > > > > > REVISIT: Ah, it's address-map, rather than add map. Okay, not as bad
> > > > > > > as I first thought, but still, is there a better naming convention you
> > > > > > > could use?
> > > > > > 
> > > > > > addrmap or something?
> > > > > 
> > > > > Right, that was what I was thinking. However, I prefer something along
> > > > > the lines of 'i2c' and 'i2c_sec' or 'client' and 'client_slv' etc.
> > > > 
> > > > FWIW, the reason it's addmap{0,1} is that the datasheet has documents
> > > > ADDMAP=0 and the first bank of registers and ADDMAP=1 as the second bank
> > > > of registers. I adopted that to match the docs for the part.
> > > > 
> > > > I guess we could do i2c and i2c_sec, I'll just have to put a comment
> > > > correlating it to the h/w. Calling it 'slv' implies something else
> > > > so we should avoid that here. The notion of a "secondary" i2c device
> > > > is completely a Linux I2C subsystem fabrication which wouldn't exist
> > > > if it allowed multiple slave addresses per device. From a h/w
> > > > perspective there is really no primary and secondary relationship.
> > > > 
> > > > I'm fine with i2c/i2c_sec or addrmap0/1 and I will just comment to
> > > > correlate with the datasheet..pick one.
> > > 
> > > Let's stick method fabricated by the I2C subsystem. It may seem strange
> > > from a h/w perspective, but it is the way we (you) have coded it, as
> > > the first parameter of i2c_new_dummy() is the 'managing' (primary,
> > > parent, master, whatever) device, so '_sec' would suit as an
> > > identifying appendage for the resultant device.
> > 
> > That works, I'll also switch to addrmap_[pri|sec] which touches the
> > regulator driver as well. That will keep the relationship  between device
> > and regmap clear.
> 
> Misspoke...I'm switching regmap[0|1] to regmap_[pri|sec] to keep that
> synced with i2c_[pri|sec]

Sounds good, thanks Matt.

Patch

diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
index e9a33c7..b710ffa 100644
--- a/drivers/mfd/bcm590xx.c
+++ b/drivers/mfd/bcm590xx.c
@@ -28,39 +28,71 @@  static const struct mfd_cell bcm590xx_devs[] = {
 	},
 };
 
-static const struct regmap_config bcm590xx_regmap_config = {
+static const struct regmap_config bcm590xx_regmap_config_0 = {
 	.reg_bits	= 8,
 	.val_bits	= 8,
-	.max_register	= BCM590XX_MAX_REGISTER,
+	.max_register	= BCM590XX_MAX_REGISTER_0,
 	.cache_type	= REGCACHE_RBTREE,
 };
 
-static int bcm590xx_i2c_probe(struct i2c_client *i2c,
+static const struct regmap_config bcm590xx_regmap_config_1 = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= BCM590XX_MAX_REGISTER_1,
+	.cache_type	= REGCACHE_RBTREE,
+};
+
+static int bcm590xx_i2c_probe(struct i2c_client *addmap0,
 			      const struct i2c_device_id *id)
 {
 	struct bcm590xx *bcm590xx;
 	int ret;
 
-	bcm590xx = devm_kzalloc(&i2c->dev, sizeof(*bcm590xx), GFP_KERNEL);
+	bcm590xx = devm_kzalloc(&addmap0->dev, sizeof(*bcm590xx), GFP_KERNEL);
 	if (!bcm590xx)
 		return -ENOMEM;
 
-	i2c_set_clientdata(i2c, bcm590xx);
-	bcm590xx->dev = &i2c->dev;
-	bcm590xx->i2c_client = i2c;
+	i2c_set_clientdata(addmap0, bcm590xx);
+	bcm590xx->dev = &addmap0->dev;
+	bcm590xx->addmap0 = addmap0;
 
-	bcm590xx->regmap = devm_regmap_init_i2c(i2c, &bcm590xx_regmap_config);
-	if (IS_ERR(bcm590xx->regmap)) {
-		ret = PTR_ERR(bcm590xx->regmap);
-		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
+	bcm590xx->regmap0 = devm_regmap_init_i2c(addmap0,
+						 &bcm590xx_regmap_config_0);
+	if (IS_ERR(bcm590xx->regmap0)) {
+		ret = PTR_ERR(bcm590xx->regmap0);
+		dev_err(&addmap0->dev, "regmap 0 init failed: %d\n", ret);
 		return ret;
 	}
 
-	ret = mfd_add_devices(&i2c->dev, -1, bcm590xx_devs,
+	/* Second I2C slave address is the base address with A(2) asserted */
+	bcm590xx->addmap1 = i2c_new_dummy(addmap0->adapter,
+					  addmap0->addr | BIT(2));
+	if (IS_ERR_OR_NULL(bcm590xx->addmap1)) {
+		dev_err(&addmap0->dev, "failed to add address map 1 device\n");
+		return -ENODEV;
+	}
+	i2c_set_clientdata(bcm590xx->addmap1, bcm590xx);
+
+	bcm590xx->regmap1 = devm_regmap_init_i2c(bcm590xx->addmap1,
+						&bcm590xx_regmap_config_1);
+	if (IS_ERR(bcm590xx->regmap1)) {
+		ret = PTR_ERR(bcm590xx->regmap1);
+		dev_err(&bcm590xx->addmap1->dev,
+			"regmap 1 init failed: %d\n", ret);
+		goto err;
+	}
+
+	ret = mfd_add_devices(&addmap0->dev, -1, bcm590xx_devs,
 			      ARRAY_SIZE(bcm590xx_devs), NULL, 0, NULL);
-	if (ret < 0)
-		dev_err(&i2c->dev, "failed to add sub-devices: %d\n", ret);
+	if (ret < 0) {
+		dev_err(&addmap0->dev, "failed to add sub-devices: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
 
+err:
+	i2c_unregister_device(bcm590xx->addmap1);
 	return ret;
 }
 
diff --git a/include/linux/mfd/bcm590xx.h b/include/linux/mfd/bcm590xx.h
index 434df2d..a2723f2 100644
--- a/include/linux/mfd/bcm590xx.h
+++ b/include/linux/mfd/bcm590xx.h
@@ -19,12 +19,15 @@ 
 #include <linux/regmap.h>
 
 /* max register address */
-#define BCM590XX_MAX_REGISTER	0xe7
+#define BCM590XX_MAX_REGISTER_0	0xe7
+#define BCM590XX_MAX_REGISTER_1	0xf0
 
 struct bcm590xx {
 	struct device *dev;
-	struct i2c_client *i2c_client;
-	struct regmap *regmap;
+	struct i2c_client *addmap0;
+	struct i2c_client *addmap1;
+	struct regmap *regmap0;
+	struct regmap *regmap1;
 	unsigned int id;
 };