| Message ID | 1608104275-13174-1-git-send-email-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| Headers | show |
| Series | treewide: bd9571mwv: Add support for BD9574MWF | expand |
On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > Use dev_regmap_add_irq_chip() to simplify the code. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/mfd/bd9571mwv.c | 27 ++++++--------------------- > 1 file changed, 6 insertions(+), 21 deletions(-) > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > index e68c3fa..49e968e 100644 > --- a/drivers/mfd/bd9571mwv.c > +++ b/drivers/mfd/bd9571mwv.c > @@ -170,31 +170,17 @@ static int bd9571mwv_probe(struct i2c_client > *client, > if (ret) > return ret; > > - ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0, > - &bd9571mwv_irq_chip, &bd->irq_data); > + ret = devm_regmap_add_irq_chip(bd->dev, bd->regmap, bd->irq, > + IRQF_ONESHOT, 0, > &bd9571mwv_irq_chip, > + &bd->irq_data); > if (ret) { > dev_err(bd->dev, "Failed to register IRQ chip\n"); > return ret; > } > > - ret = devm_mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, > - bd9571mwv_cells, > ARRAY_SIZE(bd9571mwv_cells), > - NULL, 0, regmap_irq_get_domain(bd- > >irq_data)); The funny diff formatting got me fooled. I spent a while wondering why you do remove the devm_mfd_add_devices - untill I noticed that it was just the diff playing tricks - it is added back in the remove. I should practice my diff reading skills :) But the result looks good and clean to me! > - if (ret) { > - regmap_del_irq_chip(bd->irq, bd->irq_data); > - return ret; > - } > - > - return 0; > -} > - > -static int bd9571mwv_remove(struct i2c_client *client) > -{ > - struct bd9571mwv *bd = i2c_get_clientdata(client); > - > - regmap_del_irq_chip(bd->irq, bd->irq_data); > - > - return 0; > + return devm_mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, > + bd9571mwv_cells, > ARRAY_SIZE(bd9571mwv_cells), > + NULL, 0, regmap_irq_get_domain(bd- > >irq_data)); > } > > static const struct of_device_id bd9571mwv_of_match_table[] = { > @@ -215,7 +201,6 @@ static struct i2c_driver bd9571mwv_driver = { > .of_match_table = bd9571mwv_of_match_table, > }, > .probe = bd9571mwv_probe, > - .remove = bd9571mwv_remove, > .id_table = bd9571mwv_id_table, > }; > module_i2c_driver(bd9571mwv_driver);
On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > The new PMIC BD9574MWF inherits features from BD9571MWV. > Add the support of new PMIC to existing bd9571mwv driver. > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > [shimoda: rebase and refactor] > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/mfd/bd9571mwv.c | 86 > ++++++++++++++++++++++++++++++++++++++++++- > include/linux/mfd/bd9571mwv.h | 18 +++++++-- > 2 files changed, 99 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > index ccf1a60..f660de6 100644 > --- a/drivers/mfd/bd9571mwv.c > +++ b/drivers/mfd/bd9571mwv.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * ROHM BD9571MWV-M MFD driver > + * ROHM BD9571MWV-M and BD9574MVF-M MFD driver > * > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > * Copyright (C) 2020 Renesas Electronics Corporation > @@ -11,6 +11,7 @@ > #include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/mfd/core.h> > +#include <linux/mfd/rohm-generic.h> > #include <linux/module.h> > > #include <linux/mfd/bd9571mwv.h> > @@ -28,6 +29,7 @@ struct bd957x_data { > int num_cells; > }; > > +/* For BD9571MWV */ > static const struct mfd_cell bd9571mwv_cells[] = { > { .name = "bd9571mwv-regulator", }, > { .name = "bd9571mwv-gpio", }, > @@ -124,6 +126,81 @@ static const struct bd957x_data bd9571mwv_data = > { > .num_cells = ARRAY_SIZE(bd9571mwv_cells), > }; > > +/* For BD9574MWF */ > +static const struct mfd_cell bd9574mwf_cells[] = { > + { .name = "bd9574mwf-regulator", }, > + { .name = "bd9574mwf-gpio", }, > +}; > + > +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = { > + regmap_reg_range(BD9571MWV_VENDOR_CODE, > BD9571MWV_PRODUCT_REVISION), > + regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, > BD9571MWV_BKUP_MODE_CNT), > + regmap_reg_range(BD9571MWV_DVFS_VINIT, BD9571MWV_DVFS_SETVMAX), > + regmap_reg_range(BD9571MWV_DVFS_SETVID, > BD9571MWV_DVFS_MONIVDAC), > + regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN), > + regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INTMASK), > + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTMASK), > +}; > + > +static const struct regmap_access_table bd9574mwf_readable_table = { > + .yes_ranges = bd9574mwf_readable_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_readable_yes_ranges), > +}; > + > +static const struct regmap_range bd9574mwf_writable_yes_ranges[] = { > + regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, > BD9571MWV_BKUP_MODE_CNT), > + regmap_reg_range(BD9571MWV_DVFS_SETVID, BD9571MWV_DVFS_SETVID), > + regmap_reg_range(BD9571MWV_GPIO_DIR, BD9571MWV_GPIO_OUT), > + regmap_reg_range(BD9571MWV_GPIO_INT_SET, > BD9571MWV_GPIO_INTMASK), > + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTMASK), > +}; > + > +static const struct regmap_access_table bd9574mwf_writable_table = { > + .yes_ranges = bd9574mwf_writable_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_writable_yes_ranges), > +}; > + > +static const struct regmap_range bd9574mwf_volatile_yes_ranges[] = { > + regmap_reg_range(BD9571MWV_DVFS_MONIVDAC, > BD9571MWV_DVFS_MONIVDAC), > + regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN), > + regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INT), > + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTREQ), > +}; > + > +static const struct regmap_access_table bd9574mwf_volatile_table = { > + .yes_ranges = bd9574mwf_volatile_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_volatile_yes_ranges), > +}; > + > +static const struct regmap_config bd9574mwf_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > + .rd_table = &bd9574mwf_readable_table, > + .wr_table = &bd9574mwf_writable_table, > + .volatile_table = &bd9574mwf_volatile_table, > + .max_register = 0xff, > +}; > + > +static struct regmap_irq_chip bd9574mwf_irq_chip = { > + .name = "bd9574mwf", > + .status_base = BD9571MWV_INT_INTREQ, > + .mask_base = BD9571MWV_INT_INTMASK, > + .ack_base = BD9571MWV_INT_INTREQ, > + .init_ack_masked = true, > + .num_regs = 1, > + .irqs = bd9571mwv_irqs, > + .num_irqs = ARRAY_SIZE(bd9571mwv_irqs), > +}; > + > +static const struct bd957x_data bd9574mwf_data = { > + .part_name = BD9574MWF_PART_NAME, > + .regmap_config = &bd9574mwf_regmap_config, > + .irq_chip = &bd9574mwf_irq_chip, > + .cells = bd9574mwf_cells, > + .num_cells = ARRAY_SIZE(bd9574mwf_cells), > +}; > + > static int bd9571mwv_identify(struct device *dev, struct regmap > *regmap, > const char *part_name) > { > @@ -181,6 +258,9 @@ static int bd9571mwv_probe(struct i2c_client > *client, > case BD9571MWV_PRODUCT_CODE_VAL: > data = &bd9571mwv_data; > break; > + case BD9574MWF_PRODUCT_CODE_VAL: > + data = &bd9574mwf_data; > + break; > default: > dev_err(dev, "Unsupported device 0x%x\n", ret); > return -ENOENT; > @@ -210,12 +290,14 @@ static int bd9571mwv_probe(struct i2c_client > *client, > > static const struct of_device_id bd9571mwv_of_match_table[] = { > { .compatible = "rohm,bd9571mwv", }, > + { .compatible = "rohm,bd9574mwf", }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, bd9571mwv_of_match_table); > > static const struct i2c_device_id bd9571mwv_id_table[] = { > - { "bd9571mwv", 0 }, > + { "bd9571mwv", ROHM_CHIP_TYPE_BD9571 }, > + { "bd9574mwf", ROHM_CHIP_TYPE_BD9574 }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(i2c, bd9571mwv_id_table); > diff --git a/include/linux/mfd/bd9571mwv.h > b/include/linux/mfd/bd9571mwv.h > index 5ab976a..0fc7789 100644 > --- a/include/linux/mfd/bd9571mwv.h > +++ b/include/linux/mfd/bd9571mwv.h > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: GPL-2.0-only */ > /* > - * ROHM BD9571MWV-M driver > + * ROHM BD9571MWV-M and BD9574MWF-M driver > * > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > * Copyright (C) 2020 Renesas Electronics Corporation > @@ -14,11 +14,12 @@ > #include <linux/device.h> > #include <linux/regmap.h> > > -/* List of registers for BD9571MWV */ > +/* List of registers for BD9571MWV and BD9574MWF */ > #define BD9571MWV_VENDOR_CODE 0x00 > #define BD9571MWV_VENDOR_CODE_VAL 0xdb > #define BD9571MWV_PRODUCT_CODE 0x01 > #define BD9571MWV_PRODUCT_CODE_VAL 0x60 > +#define BD9574MWF_PRODUCT_CODE_VAL 0x74 > #define BD9571MWV_PRODUCT_REVISION 0x02 > > #define BD9571MWV_I2C_FUSA_MODE 0x10 > @@ -48,6 +49,7 @@ > #define BD9571MWV_VD33_VID 0x44 > > #define BD9571MWV_DVFS_VINIT 0x50 > +#define BD9574MWF_VD09_VINIT 0x51 > #define BD9571MWV_DVFS_SETVMAX 0x52 > #define BD9571MWV_DVFS_BOOSTVID 0x53 > #define BD9571MWV_DVFS_SETVID 0x54 > @@ -61,6 +63,7 @@ > #define BD9571MWV_GPIO_INT_SET 0x64 > #define BD9571MWV_GPIO_INT 0x65 > #define BD9571MWV_GPIO_INTMASK 0x66 > +#define BD9574MWF_GPIO_MUX 0x67 > > #define BD9571MWV_REG_KEEP(n) (0x70 + (n)) > > @@ -70,6 +73,8 @@ > #define BD9571MWV_PROT_ERROR_STATUS2 0x83 > #define BD9571MWV_PROT_ERROR_STATUS3 0x84 > #define BD9571MWV_PROT_ERROR_STATUS4 0x85 > +#define BD9574MWF_PROT_ERROR_STATUS5 0x86 > +#define BD9574MWF_SYSTEM_ERROR_STATUS 0x87 > > #define BD9571MWV_INT_INTREQ 0x90 > #define BD9571MWV_INT_INTREQ_MD1_INT BIT(0) > @@ -82,9 +87,16 @@ > #define BD9571MWV_INT_INTREQ_BKUP_TRG_INT BIT(7) > #define BD9571MWV_INT_INTMASK 0x91 > > +#define BD9574MWF_SSCG_CNT 0xA0 > +#define BD9574MWF_POFFB_MRB 0xA1 > +#define BD9574MWF_SMRB_WR_PROT 0xA2 > +#define BD9574MWF_SMRB_ASSERT 0xA3 > +#define BD9574MWF_SMRB_STATUS 0xA4 > + > #define BD9571MWV_ACCESS_KEY 0xff > > #define BD9571MWV_PART_NAME "BD9571MWV" > +#define BD9574MWF_PART_NAME "BD9574MWF" > > /* Define the BD9571MWV IRQ numbers */ > enum bd9571mwv_irqs { > @@ -93,7 +105,7 @@ enum bd9571mwv_irqs { > BD9571MWV_IRQ_MD2_E2, > BD9571MWV_IRQ_PROT_ERR, > BD9571MWV_IRQ_GP, > - BD9571MWV_IRQ_128H_OF, > + BD9571MWV_IRQ_128H_OF, /* BKUP_HOLD on BD9574MWF */ > BD9571MWV_IRQ_WDT_OF, > BD9571MWV_IRQ_BKUP_TRG, > };
On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > Since the driver supports BD9571MWV PMIC only, > > this patch makes the functions and data structure become more generic > > so that it can support other PMIC variants as well. > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > [shimoda: rebase and refactor] > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Please place any *-by tags *after* the other comments. Fortunately, the first one below was still on my screen, else I would have stopped reading here. > > --- > > drivers/mfd/bd9571mwv.c | 95 +++++++++++++++++++++++++++---- > > ------------ > > include/linux/mfd/bd9571mwv.h | 18 ++------ > > 2 files changed, 63 insertions(+), 50 deletions(-)
On Wed, 2020-12-16 at 09:00 +0000, Lee Jones wrote: > On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > > > Since the driver supports BD9571MWV PMIC only, > > > this patch makes the functions and data structure become more > > > generic > > > so that it can support other PMIC variants as well. > > > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > [shimoda: rebase and refactor] > > > Signed-off-by: Yoshihiro Shimoda < > > > yoshihiro.shimoda.uh@renesas.com> > > > > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Please place any *-by tags *after* the other comments. > > Fortunately, the first one below was still on my screen, else I would > have stopped reading here. > Good point. I'd better keep that on mind. Although missing stuff after a Reviewed-by is not that fatal ;) > > > --- > > > drivers/mfd/bd9571mwv.c | 95 +++++++++++++++++++++++++++ > > > ---- > > > ------------ > > > include/linux/mfd/bd9571mwv.h | 18 ++------ > > > 2 files changed, 63 insertions(+), 50 deletions(-)
On Wed, 2020-12-16 at 09:02 +0000, Lee Jones wrote: > On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > > > The new PMIC BD9574MWF inherits features from BD9571MWV. > > > Add the support of new PMIC to existing bd9571mwv driver. > > > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > [shimoda: rebase and refactor] > > > Signed-off-by: Yoshihiro Shimoda < > > > yoshihiro.shimoda.uh@renesas.com> > > > > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > > > --- > > > drivers/mfd/bd9571mwv.c | 86 > > > ++++++++++++++++++++++++++++++++++++++++++- > > > include/linux/mfd/bd9571mwv.h | 18 +++++++-- > > > 2 files changed, 99 insertions(+), 5 deletions(-) > > ... and please snip long replies. > > Scrolling down past lots of un-reviewed code and/or past the end of > the last review comment is a waste of people's time. Thanks. > Hm. Right. I thought that leaving whole patch there when just adding ack or reviewed-by gives full information as to which exact patch version was reviewed. But I admit scrolling can be annoying - besides, the send/receive time in quote can probably be used to track the excat version which was reviewed. So point taken. Thanks. Best Regards Matti
On Wed, 16 Dec 2020, Yoshihiro Shimoda wrote: > Use dev_regmap_add_irq_chip() to simplify the code. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/mfd/bd9571mwv.c | 27 ++++++--------------------- > 1 file changed, 6 insertions(+), 21 deletions(-) For my own reference (apply this as-is to your sign-off block): Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Wed, 16 Dec 2020, Yoshihiro Shimoda wrote: > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > The new PMIC BD9574MWF inherits features from BD9571MWV. > Add the support of new PMIC to existing bd9571mwv driver. > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > [shimoda: rebase and refactor] > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/mfd/bd9571mwv.c | 86 ++++++++++++++++++++++++++++++++++++++++++- > include/linux/mfd/bd9571mwv.h | 18 +++++++-- > 2 files changed, 99 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > index ccf1a60..f660de6 100644 > --- a/drivers/mfd/bd9571mwv.c > +++ b/drivers/mfd/bd9571mwv.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * ROHM BD9571MWV-M MFD driver > + * ROHM BD9571MWV-M and BD9574MVF-M MFD driver While you're at it, please remove "MFD". Maybe replace with 'core' or something? > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > * Copyright (C) 2020 Renesas Electronics Corporation > @@ -11,6 +11,7 @@ > #include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/mfd/core.h> > +#include <linux/mfd/rohm-generic.h> > #include <linux/module.h> > > #include <linux/mfd/bd9571mwv.h> > @@ -28,6 +29,7 @@ struct bd957x_data { > int num_cells; > }; > > +/* For BD9571MWV */ Don't think this is required? > static const struct mfd_cell bd9571mwv_cells[] = { > { .name = "bd9571mwv-regulator", }, > { .name = "bd9571mwv-gpio", }, > @@ -124,6 +126,81 @@ static const struct bd957x_data bd9571mwv_data = { > .num_cells = ARRAY_SIZE(bd9571mwv_cells), > }; > > +/* For BD9574MWF */ We can see that by the struct name. > +static const struct mfd_cell bd9574mwf_cells[] = { > + { .name = "bd9574mwf-regulator", }, > + { .name = "bd9574mwf-gpio", }, > +}; > + > +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = { > + regmap_reg_range(BD9571MWV_VENDOR_CODE, BD9571MWV_PRODUCT_REVISION), > + regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT), > + regmap_reg_range(BD9571MWV_DVFS_VINIT, BD9571MWV_DVFS_SETVMAX), > + regmap_reg_range(BD9571MWV_DVFS_SETVID, BD9571MWV_DVFS_MONIVDAC), > + regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN), > + regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INTMASK), > + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTMASK), > +}; > + > +static const struct regmap_access_table bd9574mwf_readable_table = { > + .yes_ranges = bd9574mwf_readable_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_readable_yes_ranges), > +}; > + > +static const struct regmap_range bd9574mwf_writable_yes_ranges[] = { > + regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT), > + regmap_reg_range(BD9571MWV_DVFS_SETVID, BD9571MWV_DVFS_SETVID), > + regmap_reg_range(BD9571MWV_GPIO_DIR, BD9571MWV_GPIO_OUT), > + regmap_reg_range(BD9571MWV_GPIO_INT_SET, BD9571MWV_GPIO_INTMASK), > + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTMASK), > +}; > + > +static const struct regmap_access_table bd9574mwf_writable_table = { > + .yes_ranges = bd9574mwf_writable_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_writable_yes_ranges), > +}; > + > +static const struct regmap_range bd9574mwf_volatile_yes_ranges[] = { > + regmap_reg_range(BD9571MWV_DVFS_MONIVDAC, BD9571MWV_DVFS_MONIVDAC), > + regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN), > + regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INT), > + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTREQ), > +}; > + > +static const struct regmap_access_table bd9574mwf_volatile_table = { > + .yes_ranges = bd9574mwf_volatile_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_volatile_yes_ranges), > +}; > + > +static const struct regmap_config bd9574mwf_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > + .rd_table = &bd9574mwf_readable_table, > + .wr_table = &bd9574mwf_writable_table, > + .volatile_table = &bd9574mwf_volatile_table, > + .max_register = 0xff, > +}; > + > +static struct regmap_irq_chip bd9574mwf_irq_chip = { > + .name = "bd9574mwf", > + .status_base = BD9571MWV_INT_INTREQ, > + .mask_base = BD9571MWV_INT_INTMASK, > + .ack_base = BD9571MWV_INT_INTREQ, > + .init_ack_masked = true, > + .num_regs = 1, > + .irqs = bd9571mwv_irqs, > + .num_irqs = ARRAY_SIZE(bd9571mwv_irqs), > +}; > + > +static const struct bd957x_data bd9574mwf_data = { > + .part_name = BD9574MWF_PART_NAME, > + .regmap_config = &bd9574mwf_regmap_config, > + .irq_chip = &bd9574mwf_irq_chip, > + .cells = bd9574mwf_cells, > + .num_cells = ARRAY_SIZE(bd9574mwf_cells), > +}; > + > static int bd9571mwv_identify(struct device *dev, struct regmap *regmap, > const char *part_name) > { > @@ -181,6 +258,9 @@ static int bd9571mwv_probe(struct i2c_client *client, > case BD9571MWV_PRODUCT_CODE_VAL: > data = &bd9571mwv_data; > break; > + case BD9574MWF_PRODUCT_CODE_VAL: > + data = &bd9574mwf_data; > + break; > default: > dev_err(dev, "Unsupported device 0x%x\n", ret); > return -ENOENT; > @@ -210,12 +290,14 @@ static int bd9571mwv_probe(struct i2c_client *client, > > static const struct of_device_id bd9571mwv_of_match_table[] = { > { .compatible = "rohm,bd9571mwv", }, > + { .compatible = "rohm,bd9574mwf", }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, bd9571mwv_of_match_table); > > static const struct i2c_device_id bd9571mwv_id_table[] = { > - { "bd9571mwv", 0 }, > + { "bd9571mwv", ROHM_CHIP_TYPE_BD9571 }, > + { "bd9574mwf", ROHM_CHIP_TYPE_BD9574 }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(i2c, bd9571mwv_id_table); > diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h > index 5ab976a..0fc7789 100644 > --- a/include/linux/mfd/bd9571mwv.h > +++ b/include/linux/mfd/bd9571mwv.h > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: GPL-2.0-only */ > /* > - * ROHM BD9571MWV-M driver > + * ROHM BD9571MWV-M and BD9574MWF-M driver > * > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > * Copyright (C) 2020 Renesas Electronics Corporation > @@ -14,11 +14,12 @@ > #include <linux/device.h> > #include <linux/regmap.h> > > -/* List of registers for BD9571MWV */ > +/* List of registers for BD9571MWV and BD9574MWF */ > #define BD9571MWV_VENDOR_CODE 0x00 > #define BD9571MWV_VENDOR_CODE_VAL 0xdb > #define BD9571MWV_PRODUCT_CODE 0x01 > #define BD9571MWV_PRODUCT_CODE_VAL 0x60 > +#define BD9574MWF_PRODUCT_CODE_VAL 0x74 > #define BD9571MWV_PRODUCT_REVISION 0x02 > > #define BD9571MWV_I2C_FUSA_MODE 0x10 > @@ -48,6 +49,7 @@ > #define BD9571MWV_VD33_VID 0x44 > > #define BD9571MWV_DVFS_VINIT 0x50 > +#define BD9574MWF_VD09_VINIT 0x51 > #define BD9571MWV_DVFS_SETVMAX 0x52 > #define BD9571MWV_DVFS_BOOSTVID 0x53 > #define BD9571MWV_DVFS_SETVID 0x54 > @@ -61,6 +63,7 @@ > #define BD9571MWV_GPIO_INT_SET 0x64 > #define BD9571MWV_GPIO_INT 0x65 > #define BD9571MWV_GPIO_INTMASK 0x66 > +#define BD9574MWF_GPIO_MUX 0x67 > > #define BD9571MWV_REG_KEEP(n) (0x70 + (n)) > > @@ -70,6 +73,8 @@ > #define BD9571MWV_PROT_ERROR_STATUS2 0x83 > #define BD9571MWV_PROT_ERROR_STATUS3 0x84 > #define BD9571MWV_PROT_ERROR_STATUS4 0x85 > +#define BD9574MWF_PROT_ERROR_STATUS5 0x86 > +#define BD9574MWF_SYSTEM_ERROR_STATUS 0x87 > > #define BD9571MWV_INT_INTREQ 0x90 > #define BD9571MWV_INT_INTREQ_MD1_INT BIT(0) > @@ -82,9 +87,16 @@ > #define BD9571MWV_INT_INTREQ_BKUP_TRG_INT BIT(7) > #define BD9571MWV_INT_INTMASK 0x91 > > +#define BD9574MWF_SSCG_CNT 0xA0 > +#define BD9574MWF_POFFB_MRB 0xA1 > +#define BD9574MWF_SMRB_WR_PROT 0xA2 > +#define BD9574MWF_SMRB_ASSERT 0xA3 > +#define BD9574MWF_SMRB_STATUS 0xA4 > + > #define BD9571MWV_ACCESS_KEY 0xff > > #define BD9571MWV_PART_NAME "BD9571MWV" > +#define BD9574MWF_PART_NAME "BD9574MWF" > > /* Define the BD9571MWV IRQ numbers */ > enum bd9571mwv_irqs { > @@ -93,7 +105,7 @@ enum bd9571mwv_irqs { > BD9571MWV_IRQ_MD2_E2, > BD9571MWV_IRQ_PROT_ERR, > BD9571MWV_IRQ_GP, > - BD9571MWV_IRQ_128H_OF, > + BD9571MWV_IRQ_128H_OF, /* BKUP_HOLD on BD9574MWF */ > BD9571MWV_IRQ_WDT_OF, > BD9571MWV_IRQ_BKUP_TRG, > };
Hi Lee, Thank you for your review! > From: Lee Jones, Sent: Thursday, December 17, 2020 12:35 AM > > On Wed, 16 Dec 2020, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > Since the driver supports BD9571MWV PMIC only, > > this patch makes the functions and data structure become more generic > > so that it can support other PMIC variants as well. > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > [shimoda: rebase and refactor] > > This is kind of expected. Please just add Co-developed-by instead. I got it. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/mfd/bd9571mwv.c | 95 +++++++++++++++++++++++++++---------------- > > include/linux/mfd/bd9571mwv.h | 18 ++------ > > 2 files changed, 63 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > > index 49e968e..ccf1a60 100644 > > --- a/drivers/mfd/bd9571mwv.c > > +++ b/drivers/mfd/bd9571mwv.c > > @@ -3,6 +3,7 @@ > > * ROHM BD9571MWV-M MFD driver > > * > > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > > + * Copyright (C) 2020 Renesas Electronics Corporation > > * > > * Based on the TPS65086 driver > > */ > > @@ -14,6 +15,19 @@ > > > > #include <linux/mfd/bd9571mwv.h> > > > > +/** > > This is wrong. Please do not abuse kernel-doc formatting. Oops. I'll use just "/*" here. > > + * struct bd957x_data - internal data for the bd957x driverbd957x_data > > + * > > + * Internal data to distinguish bd957x variants > > + */ > > +struct bd957x_data { > > Call this bd957x_ddata please. > > ddata == driver data. I got it. > > + char *part_name; > > What is this used for besides a print? Those kinds of log messages > are usually frowned upon anyway. Probably best to just remove the > print, along with the variable. I got it. I'll remove the print. > > + const struct regmap_config *regmap_config; > > + const struct regmap_irq_chip *irq_chip; > > + const struct mfd_cell *cells; > > + int num_cells; > > +}; > > + > > static const struct mfd_cell bd9571mwv_cells[] = { > > { .name = "bd9571mwv-regulator", }, > > { .name = "bd9571mwv-gpio", }, > > @@ -102,13 +116,21 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = { > > .num_irqs = ARRAY_SIZE(bd9571mwv_irqs), > > }; > > > > -static int bd9571mwv_identify(struct bd9571mwv *bd) > > +static const struct bd957x_data bd9571mwv_data = { > > + .part_name = BD9571MWV_PART_NAME, > > + .regmap_config = &bd9571mwv_regmap_config, > > + .irq_chip = &bd9571mwv_irq_chip, > > + .cells = bd9571mwv_cells, > > + .num_cells = ARRAY_SIZE(bd9571mwv_cells), > > +}; > > + > > +static int bd9571mwv_identify(struct device *dev, struct regmap *regmap, > > I guess this function name also needs to change? > > And all other occurences of bd9571mwv? Hmm, "bd957x" prefix is already used on a regulator driver (bd9576-regulator.c) so that I'm thinking keep "bd9571mwv" is better to avoid confusing. But, this is not a strong opinion so that if you prefer "bd957x" here, I'll rename. > > + const char *part_name) > > { > > - struct device *dev = bd->dev; > > unsigned int value; > > int ret; > > > > - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value); > > + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value); > > if (ret) { > > dev_err(dev, "Failed to read vendor code register (ret=%i)\n", > > ret); > > @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) > > return -EINVAL; > > } > > > > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value); > > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value); > > if (ret) { > > dev_err(dev, "Failed to read product code register (ret=%i)\n", > > ret); > > return ret; > > } > > - > > - if (value != BD9571MWV_PRODUCT_CODE_VAL) { > > - dev_err(dev, "Invalid product code ID %02x (expected %02x)\n", > > - value, BD9571MWV_PRODUCT_CODE_VAL); > > - return -EINVAL; > > - } > > - > > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value); > > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value); > > if (ret) { > > dev_err(dev, "Failed to read revision register (ret=%i)\n", > > ret); > > return ret; > > } > > > > - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff); > > + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff); > > > > return 0; > > } > > @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) > > static int bd9571mwv_probe(struct i2c_client *client, > > const struct i2c_device_id *ids) > > { > > - struct bd9571mwv *bd; > > - int ret; > > - > > - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL); > > - if (!bd) > > - return -ENOMEM; > > - > > - i2c_set_clientdata(client, bd); > > - bd->dev = &client->dev; > > - bd->irq = client->irq; > > + const struct bd957x_data *data; > > ddata I'll change it. > > + struct device *dev = &client->dev; > > + struct regmap *regmap; > > + struct regmap_irq_chip_data *irq_data; > > + int ret, irq = client->irq; > > + > > + /* Read the PMIC product code */ > > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE); > > + if (ret < 0) { > > + dev_err(dev, "failed reading at 0x%02x\n", > > + BD9571MWV_PRODUCT_CODE); > > "Failed to read product code" is more user friendly. I got it. Thank you for your suggestion. > > + return ret; > > + } > > + switch (ret) { > > + case BD9571MWV_PRODUCT_CODE_VAL: > > Suggest: > > s/BD9571MWV_PRODUCT_CODE/BD9571MWV_PRODUCT_CODE_CMD/ > then > s/BD9571MWV_PRODUCT_CODE_VAL/BD9571MWV_PRODUCT_CODE/ Hmm, if we use "BD9571MWV_PRODUCT_CODE_CMD", this causes inconsistence other registers' definitions. So, perhaps, BD9571MWV_PRODUCT_CODE_BD9571MWV (and BD9571MWV_PRODUCT_CODE_BD9574MWF in the patch 12/12) instead of "_VAL" are better. > > + data = &bd9571mwv_data; > > + break; > > + default: > > + dev_err(dev, "Unsupported device 0x%x\n", ret); > > + return -ENOENT; > > ENOENT == "No such file or directory" > > I think you mean -ENODEV. Oops. I'll fix it. > > + } > > > > - bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config); > > - if (IS_ERR(bd->regmap)) { > > - dev_err(bd->dev, "Failed to initialize register map\n"); > > - return PTR_ERR(bd->regmap); > > + regmap = devm_regmap_init_i2c(client, data->regmap_config); > > + if (IS_ERR(regmap)) { > > + dev_err(dev, "Failed to initialize register map\n"); > > + return PTR_ERR(regmap); > > } > > > > - ret = bd9571mwv_identify(bd); > > + ret = bd9571mwv_identify(dev, regmap, data->part_name); > > Just pass ddata, then you'll have 'dev' and 'regmap'. Now "bd9571mwv_ddata" is const and doesn't have 'dev' and 'regmap'. Does this means I should not use const and add device and regmap into struct bd957x_ddata? > I'd remove 'part_name' completely. I got it. Best regards, Yoshihiro Shimoda
Hi Lee, Thank you for your review! > From: Lee Jones, Sent: Thursday, December 17, 2020 12:38 AM > > On Wed, 16 Dec 2020, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > The new PMIC BD9574MWF inherits features from BD9571MWV. > > Add the support of new PMIC to existing bd9571mwv driver. > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > [shimoda: rebase and refactor] > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/mfd/bd9571mwv.c | 86 ++++++++++++++++++++++++++++++++++++++++++- > > include/linux/mfd/bd9571mwv.h | 18 +++++++-- > > 2 files changed, 99 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > > index ccf1a60..f660de6 100644 > > --- a/drivers/mfd/bd9571mwv.c > > +++ b/drivers/mfd/bd9571mwv.c > > @@ -1,6 +1,6 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > /* > > - * ROHM BD9571MWV-M MFD driver > > + * ROHM BD9571MWV-M and BD9574MVF-M MFD driver > > While you're at it, please remove "MFD". > > Maybe replace with 'core' or something? I got it. I'll replace with 'core'. > > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > > * Copyright (C) 2020 Renesas Electronics Corporation > > @@ -11,6 +11,7 @@ > > #include <linux/i2c.h> > > #include <linux/interrupt.h> > > #include <linux/mfd/core.h> > > +#include <linux/mfd/rohm-generic.h> > > #include <linux/module.h> > > > > #include <linux/mfd/bd9571mwv.h> > > @@ -28,6 +29,7 @@ struct bd957x_data { > > int num_cells; > > }; > > > > +/* For BD9571MWV */ > > Don't think this is required? I'll remove it. Best regards, Yoshihiro Shimoda