| Message ID | 1397501428-8857-3-git-send-email-mporter@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }; >
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?
> > 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.
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
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
> > > > 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.
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
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
> > > > > > > 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.
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; };
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(-)