[PATCH 0/3] mfd: bd9571mwv: Add support for BD9574MWF

[PATCH 0/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Add BD574MWF support into bd9571mwv mfd driver. Latest Ebisu-4D
boards has this chip instead of BD9571MWV so that we need this
patch.

Khiem Nguyen (2):
  mfd: bd9571mwv: Make the driver more generic
  mfd: bd9571mwv: Add support for BD9574MWF

Yoshihiro Shimoda (1):
  mfd: bd9571mwv: Use the SPDX license identifier

 drivers/mfd/bd9571mwv.c       | 149 +++++++++++++++++++++++++++++++++++++-----
 include/linux/mfd/bd9571mwv.h | 109 +++++++++++++++++++++++++++---
 2 files changed, 231 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH 1/3] mfd: bd9571mwv: Use the SPDX license identifier

[Yoshihiro Shimoda]

Use the SPDX license identifier instead of a local description.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mfd/bd9571mwv.c       | 10 +---------
 include/linux/mfd/bd9571mwv.h | 10 +---------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index fab3cdc..80c6ef0 100644
--- a/drivers/mfd/bd9571mwv.c
+++ b/drivers/mfd/bd9571mwv.c
@@ -1,17 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * ROHM BD9571MWV-M MFD driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether expressed or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License version 2 for more details.
- *
  * Based on the TPS65086 driver
  */
 
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index eb05569..bcc7092 100644
--- a/include/linux/mfd/bd9571mwv.h
+++ b/include/linux/mfd/bd9571mwv.h
@@ -1,17 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
 /*
  * ROHM BD9571MWV-M driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether expressed or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License version 2 for more details.
- *
  * Based on the TPS65086 driver
  */
 
-- 
2.7.4

[PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

[Yoshihiro Shimoda]

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>
---
 drivers/mfd/bd9571mwv.c       | 47 ++++++++++++++++++++++++++++++++++---------
 include/linux/mfd/bd9571mwv.h | 19 +++++++++++++++++
 2 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index 80c6ef0..57bdb6a 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
  */
@@ -102,6 +103,15 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = {
 	.num_irqs	= ARRAY_SIZE(bd9571mwv_irqs),
 };
 
+static const struct bd957x_data bd9571mwv_data = {
+	.product_code_val = BD9571MWV_PRODUCT_CODE_VAL,
+	.part_number = BD9571MWV_PART_NUMBER,
+	.regmap_config = &bd9571mwv_regmap_config,
+	.irq_chip = &bd9571mwv_irq_chip,
+	.cells = bd9571mwv_cells,
+	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
+};
+
 static int bd9571mwv_identify(struct bd9571mwv *bd)
 {
 	struct device *dev = bd->dev;
@@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
 			ret);
 		return ret;
 	}
-
-	if (value != BD9571MWV_PRODUCT_CODE_VAL) {
+	/* Confirm the product code */
+	if (value != bd->data->product_code_val) {
 		dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
-			value, BD9571MWV_PRODUCT_CODE_VAL);
+			value, bd->data->product_code_val);
 		return -EINVAL;
 	}
-
 	ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value);
 	if (ret) {
 		dev_err(dev, "Failed to read revision register (ret=%i)\n",
@@ -141,7 +150,8 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
 		return ret;
 	}
 
-	dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
+	dev_info(dev, "Device: %s rev. %d\n", bd->data->part_number,
+		 value & 0xff);
 
 	return 0;
 }
@@ -150,6 +160,7 @@ static int bd9571mwv_probe(struct i2c_client *client,
 			  const struct i2c_device_id *ids)
 {
 	struct bd9571mwv *bd;
+	unsigned int product_code;
 	int ret;
 
 	bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
@@ -160,7 +171,25 @@ static int bd9571mwv_probe(struct i2c_client *client,
 	bd->dev = &client->dev;
 	bd->irq = client->irq;
 
-	bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config);
+	/* Read the PMIC product code */
+	ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed reading at 0x%02x\n",
+			BD9571MWV_PRODUCT_CODE);
+		return ret;
+	}
+
+	product_code = (unsigned int)ret;
+	if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
+		bd->data = &bd9571mwv_data;
+
+	if (!bd->data) {
+		dev_err(bd->dev, "No found supported device %d\n",
+			product_code);
+		return -ENOENT;
+	}
+
+	bd->regmap = devm_regmap_init_i2c(client, bd->data->regmap_config);
 	if (IS_ERR(bd->regmap)) {
 		dev_err(bd->dev, "Failed to initialize register map\n");
 		return PTR_ERR(bd->regmap);
@@ -171,14 +200,14 @@ static int bd9571mwv_probe(struct i2c_client *client,
 		return ret;
 
 	ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0,
-				  &bd9571mwv_irq_chip, &bd->irq_data);
+				  bd->data->irq_chip, &bd->irq_data);
 	if (ret) {
 		dev_err(bd->dev, "Failed to register IRQ chip\n");
 		return ret;
 	}
 
-	ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd9571mwv_cells,
-			      ARRAY_SIZE(bd9571mwv_cells), NULL, 0,
+	ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd->data->cells,
+			      bd->data->num_cells, NULL, 0,
 			      regmap_irq_get_domain(bd->irq_data));
 	if (ret) {
 		regmap_del_irq_chip(bd->irq, bd->irq_data);
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index bcc7092..0126b52 100644
--- a/include/linux/mfd/bd9571mwv.h
+++ b/include/linux/mfd/bd9571mwv.h
@@ -3,6 +3,7 @@
  * ROHM BD9571MWV-M driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
+ * Copyright (C) 2019 Renesas Electronics Corporation
  *
  * Based on the TPS65086 driver
  */
@@ -11,6 +12,7 @@
 #define __LINUX_MFD_BD9571MWV_H
 
 #include <linux/device.h>
+#include <linux/mfd/core.h>
 #include <linux/regmap.h>
 
 /* List of registers for BD9571MWV */
@@ -83,6 +85,8 @@
 
 #define BD9571MWV_ACCESS_KEY			0xff
 
+#define BD9571MWV_PART_NUMBER			"BD9571MWV"
+
 /* Define the BD9571MWV IRQ numbers */
 enum bd9571mwv_irqs {
 	BD9571MWV_IRQ_MD1,
@@ -96,6 +100,20 @@ enum bd9571mwv_irqs {
 };
 
 /**
+ * struct bd957x_data - internal data for the bd957x driver
+ *
+ * Internal data to distinguish bd9571mwv chip and bd9574mwf chip
+ */
+struct bd957x_data {
+	int product_code_val;
+	char *part_number;
+	const struct regmap_config *regmap_config;
+	const struct regmap_irq_chip *irq_chip;
+	const struct mfd_cell *cells;
+	int num_cells;
+};
+
+/**
  * struct bd9571mwv - state holder for the bd9571mwv driver
  *
  * Device data may be used to access the BD9571MWV chip
@@ -103,6 +121,7 @@ enum bd9571mwv_irqs {
 struct bd9571mwv {
 	struct device *dev;
 	struct regmap *regmap;
+	const struct bd957x_data *data;
 
 	/* IRQ Data */
 	int irq;
-- 
2.7.4

[PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

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       | 92 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/bd9571mwv.h | 80 +++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+)

diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index 57bdb6a..f8f0a87 100644
--- a/drivers/mfd/bd9571mwv.c
+++ b/drivers/mfd/bd9571mwv.c
@@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[] = {
 	{ .name = "bd9571mwv-gpio", },
 };
 
+/* Regmap for BD9571MWV */
 static const struct regmap_range bd9571mwv_readable_yes_ranges[] = {
 	regmap_reg_range(BD9571MWV_VENDOR_CODE, BD9571MWV_PRODUCT_REVISION),
 	regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT),
@@ -112,6 +113,95 @@ static const struct bd957x_data bd9571mwv_data = {
 	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
 };
 
+static const struct mfd_cell bd9574mwf_cells[] = {
+	{ .name = "bd9571mwv-gpio", },
+};
+
+/* Regmap for BD9574MWF */
+static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
+	regmap_reg_range(BD9574MWF_VENDOR_CODE, BD9574MWF_PRODUCT_REVISION),
+	regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
+	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
+	regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
+	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
+	regmap_reg_range(BD9574MWF_GPIO_INT_SET, BD9574MWF_GPIO_INTMASK),
+	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
+	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INT),
+	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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 const struct regmap_irq bd9574mwf_irqs[] = {
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_MD1, 0,
+		       BD9574MWF_INT_INTREQ_MD1_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E1, 0,
+		       BD9574MWF_INT_INTREQ_MD2_E1_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E2, 0,
+		       BD9574MWF_INT_INTREQ_MD2_E2_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_PROT_ERR, 0,
+		       BD9574MWF_INT_INTREQ_PROT_ERR_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_GP, 0,
+		       BD9574MWF_INT_INTREQ_GP_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_HOLD_OF, 0,
+		       BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_WDT_OF, 0,
+		       BD9574MWF_INT_INTREQ_WDT_OF_INT),
+	REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_TRG, 0,
+		       BD9574MWF_INT_INTREQ_BKUP_TRG_INT),
+};
+
+static struct regmap_irq_chip bd9574mwf_irq_chip = {
+	.name		= "bd9574mwf",
+	.status_base	= BD9574MWF_INT_INTREQ,
+	.mask_base	= BD9574MWF_INT_INTMASK,
+	.ack_base	= BD9574MWF_INT_INTREQ,
+	.init_ack_masked = true,
+	.num_regs	= 1,
+	.irqs		= bd9574mwf_irqs,
+	.num_irqs	= ARRAY_SIZE(bd9574mwf_irqs),
+};
+
+static const struct bd957x_data bd9574mwf_data = {
+	.product_code_val = BD9574MWF_PRODUCT_CODE_VAL,
+	.part_number = BD9574MWF_PART_NUMBER,
+	.regmap_config = &bd9574mwf_regmap_config,
+	.irq_chip = &bd9574mwf_irq_chip,
+	.cells = bd9574mwf_cells,
+	.num_cells = ARRAY_SIZE(bd9574mwf_cells),
+};
+
 static int bd9571mwv_identify(struct bd9571mwv *bd)
 {
 	struct device *dev = bd->dev;
@@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client *client,
 	product_code = (unsigned int)ret;
 	if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
 		bd->data = &bd9571mwv_data;
+	else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
+		bd->data = &bd9574mwf_data;
 
 	if (!bd->data) {
 		dev_err(bd->dev, "No found supported device %d\n",
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index 0126b52..e9e219b 100644
--- a/include/linux/mfd/bd9571mwv.h
+++ b/include/linux/mfd/bd9571mwv.h
@@ -99,6 +99,86 @@ enum bd9571mwv_irqs {
 	BD9571MWV_IRQ_BKUP_TRG,
 };
 
+/* List of registers for BD9574MWF */
+#define BD9574MWF_VENDOR_CODE			0x00
+#define BD9574MWF_VENDOR_CODE_VAL		0xdb
+#define BD9574MWF_PRODUCT_CODE			0x01
+#define BD9574MWF_PRODUCT_CODE_VAL		0x74
+#define BD9574MWF_PRODUCT_REVISION		0x02
+
+#define BD9574MWF_I2C_FUSA_MODE			0x10
+#define BD9574MWF_I2C_MD2_E1_BIT_1		0x11
+#define BD9574MWF_I2C_MD2_E1_BIT_2		0x12
+
+#define BD9574MWF_BKUP_MODE_CNT			0x20
+#define BD9574MWF_BKUP_MODE_STATUS		0x21
+#define BD9574MWF_BKUP_RECOVERY_CNT		0x22
+#define BD9574MWF_BKUP_CTRL_TIM_CNT		0x23
+#define BD9574MWF_WAITBKUP_WDT_CNT		0x24
+#define BD9574MWF_BKUP_HOLD_TIM_CNT1		0x26
+#define BD9574MWF_QLLM_CNT			0x27
+#define BD9574MWF_BKUP_HOLD_TIM_CNT2		0x28
+
+#define BD9574MWF_DCDC_FREQ			0x48
+
+#define BD9574MWF_VDCORE_VINIT			0x50
+#define BD9574MWF_VD09_VINIT			0x51
+#define BD9574MWF_VDCORE_SETVMAX		0x52
+#define BD9574MWF_VDCORE_SETVID			0x54
+#define BD9574MWF_VDCORE_MONIVDAC		0x55
+#define BD9574MWF_VDCORE_PGD_CNT		0x56
+
+#define BD9574MWF_GPIO_DIR			0x60
+#define BD9574MWF_GPIO_OUT			0x61
+#define BD9574MWF_GPIO_IN			0x62
+#define BD9574MWF_GPIO_DEB			0x63
+#define BD9574MWF_GPIO_INT_SET			0x64
+#define BD9574MWF_GPIO_INT			0x65
+#define BD9574MWF_GPIO_INTMASK			0x66
+#define BD9574MWF_GPIO_MUX			0x67
+
+#define BD9574MWF_REG_KEEP(n)			(0x70 + (n))
+
+#define BD9574MWF_PMIC_INTERNAL_STATUS		0x80
+#define BD9574MWF_PROT_ERROR_STATUS0		0x81
+#define BD9574MWF_PROT_ERROR_STATUS1		0x82
+#define BD9574MWF_PROT_ERROR_STATUS2		0x83
+#define BD9574MWF_PROT_ERROR_STATUS3		0x84
+#define BD9574MWF_PROT_ERROR_STATUS4		0x85
+#define BD9574MWF_PROT_ERROR_STATUS5		0x86
+#define BD9574MWF_SYS_ERROR_STATUS		0x87
+
+#define BD9574MWF_INT_INTREQ			0x90
+#define BD9574MWF_INT_INTREQ_MD1_INT		BIT(0)
+#define BD9574MWF_INT_INTREQ_MD2_E1_INT		BIT(1)
+#define BD9574MWF_INT_INTREQ_MD2_E2_INT		BIT(2)
+#define BD9574MWF_INT_INTREQ_PROT_ERR_INT	BIT(3)
+#define BD9574MWF_INT_INTREQ_GP_INT		BIT(4)
+#define BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT	BIT(5)
+#define BD9574MWF_INT_INTREQ_WDT_OF_INT		BIT(6)
+#define BD9574MWF_INT_INTREQ_BKUP_TRG_INT	BIT(7)
+#define BD9574MWF_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 BD9574MWF_PART_NUMBER			"BD9574MWF"
+
+/* Define the BD9574MWF IRQ numbers */
+enum bd9574mwf_irqs {
+	BD9574MWF_IRQ_MD1,
+	BD9574MWF_IRQ_MD2_E1,
+	BD9574MWF_IRQ_MD2_E2,
+	BD9574MWF_IRQ_PROT_ERR,
+	BD9574MWF_IRQ_GP,
+	BD9574MWF_IRQ_BKUP_HOLD_OF,
+	BD9574MWF_IRQ_WDT_OF,
+	BD9574MWF_IRQ_BKUP_TRG,
+};
+
 /**
  * struct bd957x_data - internal data for the bd957x driver
  *
-- 
2.7.4

Re: [PATCH 1/3] mfd: bd9571mwv: Use the SPDX license identifier

[Geert Uytterhoeven]

On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Use the SPDX license identifier instead of a local description.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

[Geert Uytterhoeven]

Hi Shimoda-san,

On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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>

Thanks for your patch!

> index 80c6ef0..57bdb6a 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c

> @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
>                         ret);
>                 return ret;
>         }
> -
> -       if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> +       /* Confirm the product code */
> +       if (value != bd->data->product_code_val) {
>                 dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
> -                       value, BD9571MWV_PRODUCT_CODE_VAL);
> +                       value, bd->data->product_code_val);
>                 return -EINVAL;
>         }

Reading the product code register, and checking the product code value
can be removed, as bd9571mwv_probe() has verified it already.

> @@ -150,6 +160,7 @@ static int bd9571mwv_probe(struct i2c_client *client,
>                           const struct i2c_device_id *ids)
>  {
>         struct bd9571mwv *bd;
> +       unsigned int product_code;

No need for a new variable...

>         int ret;
>
>         bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
> @@ -160,7 +171,25 @@ static int bd9571mwv_probe(struct i2c_client *client,
>         bd->dev = &client->dev;
>         bd->irq = client->irq;
>
> -       bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config);
> +       /* Read the PMIC product code */
> +       ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "failed reading at 0x%02x\n",
> +                       BD9571MWV_PRODUCT_CODE);
> +               return ret;
> +       }
> +
> +       product_code = (unsigned int)ret;
> +       if (product_code == BD9571MWV_PRODUCT_CODE_VAL)

... as you can just check if ret == BD9571MWV_PRODUCT_CODE_VAL here.

> +               bd->data = &bd9571mwv_data;
> +
> +       if (!bd->data) {
> +               dev_err(bd->dev, "No found supported device %d\n",

"Unsupported device 0x%x"?

> +                       product_code);
> +               return -ENOENT;
> +       }

The construct above may be easier to read and extend by using a switch()
statement, with a default case for unsupported devices.

> --- a/include/linux/mfd/bd9571mwv.h
> +++ b/include/linux/mfd/bd9571mwv.h

> @@ -83,6 +85,8 @@
>
>  #define BD9571MWV_ACCESS_KEY                   0xff
>
> +#define BD9571MWV_PART_NUMBER                  "BD9571MWV"

BD9571MWV_PART_NAME?

> +
>  /* Define the BD9571MWV IRQ numbers */
>  enum bd9571mwv_irqs {
>         BD9571MWV_IRQ_MD1,
> @@ -96,6 +100,20 @@ enum bd9571mwv_irqs {
>  };
>
>  /**
> + * struct bd957x_data - internal data for the bd957x driver
> + *
> + * Internal data to distinguish bd9571mwv chip and bd9574mwf chip

... distinguish bd957x variants?

> + */
> +struct bd957x_data {
> +       int product_code_val;

unsigned int?

> +       char *part_number;

part_name?

> +       const struct regmap_config *regmap_config;
> +       const struct regmap_irq_chip *irq_chip;
> +       const struct mfd_cell *cells;
> +       int num_cells;

I'd say "unsigned int", but mfd_add_devices() takes plain "int".

Please put the "product_code_val" and "num_cells" fields next to
each other, to avoid two 4-byte gaps on 64-bit platforms.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Geert Uytterhoeven]

Hi Shimoda-san,

CC Matti (BD9573/6 driver for R-Car platforms)

(I don't have the BD9574 datasheet, so I have to base my review on
 https://www.rohm.com/r-car-pmic)

On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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>

Thanks for your patch!

> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[] = {
>         { .name = "bd9571mwv-gpio", },
>  };
>
> +/* Regmap for BD9571MWV */

Note that bd9571mwv_cells[] above also applies to BD9571MWV.

>  static const struct regmap_range bd9571mwv_readable_yes_ranges[] = {
>         regmap_reg_range(BD9571MWV_VENDOR_CODE, BD9571MWV_PRODUCT_REVISION),
>         regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT),
> @@ -112,6 +113,95 @@ static const struct bd957x_data bd9571mwv_data = {
>         .num_cells = ARRAY_SIZE(bd9571mwv_cells),
>  };
>
> +static const struct mfd_cell bd9574mwf_cells[] = {
> +       { .name = "bd9571mwv-gpio", },

No regulator cell?

> +};
> +
> +/* Regmap for BD9574MWF */

Note that bd9574mwf_cells[] above also applies to BD9574MWF.
Perhaps the comments should be changed slightly, and moved up,
to serve as a separator between chip variants?

> +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> +       regmap_reg_range(BD9574MWF_VENDOR_CODE, BD9574MWF_PRODUCT_REVISION),

Missing BD9574MWF_BKUP_MODE_CNT and BD9574MWF_DVFS_*?

> +       regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> +       regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
> +       regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
> +       regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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[] = {

Missing BD9574MWF_BKUP_MODE_CNT and BD9574MWF_DVFS_*?

> +       regmap_reg_range(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
> +       regmap_reg_range(BD9574MWF_GPIO_INT_SET, BD9574MWF_GPIO_INTMASK),
> +       regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTMASK),
> +};

> @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client *client,
>         product_code = (unsigned int)ret;
>         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
>                 bd->data = &bd9571mwv_data;
> +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> +               bd->data = &bd9574mwf_data;
>
>         if (!bd->data) {
>                 dev_err(bd->dev, "No found supported device %d\n",

While BD9571MWV and BD9574MWF can be distinguished at runtime,
I think it would still be a good idea to document a "rohm,bd9574mwf"
compatible value in the DT bindings, and let the driver match on that.

> diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
> index 0126b52..e9e219b 100644
> --- a/include/linux/mfd/bd9571mwv.h
> +++ b/include/linux/mfd/bd9571mwv.h

> +#define BD9574MWF_VDCORE_VINIT                 0x50
> +#define BD9574MWF_VD09_VINIT                   0x51
> +#define BD9574MWF_VDCORE_SETVMAX               0x52
> +#define BD9574MWF_VDCORE_SETVID                        0x54
> +#define BD9574MWF_VDCORE_MONIVDAC              0x55
> +#define BD9574MWF_VDCORE_PGD_CNT               0x56

Some of the above are the same as the corresponding BD9571MWV
registers, so using the same define may simplify regulator support
(cfr. BD9571MWV_DVFS_SETVID and BD9571MWV_DVFS_MONIVDAC).

> +#define BD9574MWF_PART_NUMBER                  "BD9574MWF"

BD9574MWF_PART_NAME?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for your review!

> From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:26 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
<snip>
> > index 80c6ef0..57bdb6a 100644
> > --- a/drivers/mfd/bd9571mwv.c
> > +++ b/drivers/mfd/bd9571mwv.c
> 
> > @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
> >                         ret);
> >                 return ret;
> >         }
> > -
> > -       if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> > +       /* Confirm the product code */
> > +       if (value != bd->data->product_code_val) {
> >                 dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
> > -                       value, BD9571MWV_PRODUCT_CODE_VAL);
> > +                       value, bd->data->product_code_val);
> >                 return -EINVAL;
> >         }
> 
> Reading the product code register, and checking the product code value
> can be removed, as bd9571mwv_probe() has verified it already.

Indeed. I'll remove this.

> > @@ -150,6 +160,7 @@ static int bd9571mwv_probe(struct i2c_client *client,
> >                           const struct i2c_device_id *ids)
> >  {
> >         struct bd9571mwv *bd;
> > +       unsigned int product_code;
> 
> No need for a new variable...
> 
> >         int ret;
> >
> >         bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
> > @@ -160,7 +171,25 @@ static int bd9571mwv_probe(struct i2c_client *client,
> >         bd->dev = &client->dev;
> >         bd->irq = client->irq;
> >
> > -       bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config);
> > +       /* Read the PMIC product code */
> > +       ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
> > +       if (ret < 0) {
> > +               dev_err(&client->dev, "failed reading at 0x%02x\n",
> > +                       BD9571MWV_PRODUCT_CODE);
> > +               return ret;
> > +       }
> > +
> > +       product_code = (unsigned int)ret;
> > +       if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> 
> ... as you can just check if ret == BD9571MWV_PRODUCT_CODE_VAL here.

I got it.

> > +               bd->data = &bd9571mwv_data;
> > +
> > +       if (!bd->data) {
> > +               dev_err(bd->dev, "No found supported device %d\n",
> 
> "Unsupported device 0x%x"?

I'll fix it.

> > +                       product_code);
> > +               return -ENOENT;
> > +       }
> 
> The construct above may be easier to read and extend by using a switch()
> statement, with a default case for unsupported devices.

I think so. I'll fix it.

> > --- a/include/linux/mfd/bd9571mwv.h
> > +++ b/include/linux/mfd/bd9571mwv.h
> 
> > @@ -83,6 +85,8 @@
> >
> >  #define BD9571MWV_ACCESS_KEY                   0xff
> >
> > +#define BD9571MWV_PART_NUMBER                  "BD9571MWV"
> 
> BD9571MWV_PART_NAME?

I'll rename it.

> > +
> >  /* Define the BD9571MWV IRQ numbers */
> >  enum bd9571mwv_irqs {
> >         BD9571MWV_IRQ_MD1,
> > @@ -96,6 +100,20 @@ enum bd9571mwv_irqs {
> >  };
> >
> >  /**
> > + * struct bd957x_data - internal data for the bd957x driver
> > + *
> > + * Internal data to distinguish bd9571mwv chip and bd9574mwf chip
> 
> ... distinguish bd957x variants?

Thanks. I'll modify it.

> > + */
> > +struct bd957x_data {
> > +       int product_code_val;
> 
> unsigned int?

We can remove this member.
Or, keeping this member and then we check the product code by this member
instead of switch() like below?

/* No build test, JFYI */
struct bd957x_data *data_array[] = {
	&bd9571mwv_data,
	&bd9574mwf_data,
};

for (i = 0; I < ARRAY_SIZE(data_array); i++) {
	if (val == data_array[i].product_code_val) {
		bd->data = data_array[i];
		break;
	}
}

> > +       char *part_number;
> 
> part_name?

Yes, I'll fix it.

> > +       const struct regmap_config *regmap_config;
> > +       const struct regmap_irq_chip *irq_chip;
> > +       const struct mfd_cell *cells;
> > +       int num_cells;
> 
> I'd say "unsigned int", but mfd_add_devices() takes plain "int".
> 
> Please put the "product_code_val" and "num_cells" fields next to
> each other, to avoid two 4-byte gaps on 64-bit platforms.

I'll fix it if we kept "product_code_val".

Best regards,
Yoshihiro Shimoda

RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for your review!

> From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30 PM
<snip> 
> > --- a/drivers/mfd/bd9571mwv.c
> > +++ b/drivers/mfd/bd9571mwv.c
> > @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[] = {
> >         { .name = "bd9571mwv-gpio", },
> >  };
> >
> > +/* Regmap for BD9571MWV */
> 
> Note that bd9571mwv_cells[] above also applies to BD9571MWV.

Yes, so, I'll move this comment.

> >  static const struct regmap_range bd9571mwv_readable_yes_ranges[] = {
> >         regmap_reg_range(BD9571MWV_VENDOR_CODE, BD9571MWV_PRODUCT_REVISION),
> >         regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT),
> > @@ -112,6 +113,95 @@ static const struct bd957x_data bd9571mwv_data = {
> >         .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> >  };
> >
> > +static const struct mfd_cell bd9574mwf_cells[] = {
> > +       { .name = "bd9571mwv-gpio", },
> 
> No regulator cell?

Oops, we should add regulator for bd9574mwf to use backup_mode and DVFS.
However, since BD9574MWF doesn't have AVS, we should add
" bd9574mwf-regulator", not "bd9571mwv-regulator".
# Your indicated web site said BD9574MWF supports AVS feature.
# But, "Application Circuit" doesn't have any AVS pins.
# Of course, the datasheet doesn't mention about AVS :)

> > +};
> > +
> > +/* Regmap for BD9574MWF */
> 
> Note that bd9574mwf_cells[] above also applies to BD9574MWF.
> Perhaps the comments should be changed slightly, and moved up,
> to serve as a separator between chip variants?

I think so. I'll fix it.

> > +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> > +       regmap_reg_range(BD9574MWF_VENDOR_CODE, BD9574MWF_PRODUCT_REVISION),
> 
> Missing BD9574MWF_BKUP_MODE_CNT and BD9574MWF_DVFS_*?

Yes, I'll add these.

> > +       regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> > +       regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
> > +       regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
> > +       regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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[] = {
> 
> Missing BD9574MWF_BKUP_MODE_CNT and BD9574MWF_DVFS_*?

Yes, I'll add these.

> > +       regmap_reg_range(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
> > +       regmap_reg_range(BD9574MWF_GPIO_INT_SET, BD9574MWF_GPIO_INTMASK),
> > +       regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTMASK),
> > +};
> 
> > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client *client,
> >         product_code = (unsigned int)ret;
> >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> >                 bd->data = &bd9571mwv_data;
> > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > +               bd->data = &bd9574mwf_data;
> >
> >         if (!bd->data) {
> >                 dev_err(bd->dev, "No found supported device %d\n",
> 
> While BD9571MWV and BD9574MWF can be distinguished at runtime,
> I think it would still be a good idea to document a "rohm,bd9574mwf"
> compatible value in the DT bindings, and let the driver match on that.

In this driver point of view, we can use such the DT bindings,
however, in the board point of view, it's difficult to describe
which chip is installed on r8a77990-ebisu.dts. So, I'd like to
keep this runtime detection.

> > diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
> > index 0126b52..e9e219b 100644
> > --- a/include/linux/mfd/bd9571mwv.h
> > +++ b/include/linux/mfd/bd9571mwv.h
> 
> > +#define BD9574MWF_VDCORE_VINIT                 0x50
> > +#define BD9574MWF_VD09_VINIT                   0x51
> > +#define BD9574MWF_VDCORE_SETVMAX               0x52
> > +#define BD9574MWF_VDCORE_SETVID                        0x54
> > +#define BD9574MWF_VDCORE_MONIVDAC              0x55
> > +#define BD9574MWF_VDCORE_PGD_CNT               0x56
> 
> Some of the above are the same as the corresponding BD9571MWV
> registers, so using the same define may simplify regulator support
> (cfr. BD9571MWV_DVFS_SETVID and BD9571MWV_DVFS_MONIVDAC).

Indeed. I'll fix it.

> > +#define BD9574MWF_PART_NUMBER                  "BD9574MWF"
> 
> BD9574MWF_PART_NAME?

Yes, I'll rename it.

Best regards,
Yoshihiro Shimoda

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Vaittinen, Matti]

Hi deee Ho Yoshihiro-san, Geert, All,

On Wed, 2020-12-09 at 14:30 +0100, Geert Uytterhoeven wrote:
> Hi Shimoda-san,
> 
> CC Matti (BD9573/6 driver for R-Car platforms)

Thank Geert! I wouldn't have noticed this :)

> 
> (I don't have the BD9574 datasheet, so I have to base my review on
>  https://www.rohm.com/r-car-pmic)
> 
> On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> 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>
> 
> Thanks for your patch!
> 

Indeed! It's really nice to see other people working on upstreaming
drivers for ROHM PMICs! Thanks for all the good work!

Can you please add me to CC if you ever resend the series? I like to
know what kind of support there is added in-tree for ROHM PMICs :)

Best Regards
  Matti

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Vaittinen, Matti]

On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote:
> Hi Geert-san,
> 
> Thank you for your review!
> 
> > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30
> > PM
> <snip> 
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> > > 
> > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client
> > > *client,
> > >         product_code = (unsigned int)ret;
> > >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > >                 bd->data = &bd9571mwv_data;
> > > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > +               bd->data = &bd9574mwf_data;
> > > 
> > >         if (!bd->data) {
> > >                 dev_err(bd->dev, "No found supported device
> > > %d\n",
> > 
> > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > I think it would still be a good idea to document a
> > "rohm,bd9574mwf"
> > compatible value in the DT bindings, and let the driver match on
> > that.
> 
> In this driver point of view, we can use such the DT bindings,
> however, in the board point of view, it's difficult to describe
> which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> keep this runtime detection.

First of all - I don't want to insist changes here so my comment can be
ignored. I would definitely like seeing the support for BD9574 in-tree!

But as a 'nit':
I don't know what are the difficulties you are referring to so it's
hard for me to comment this. Without better understanding of board dts
files - I think BD9574MWF should really have own compatible as I
understood it is different from the BD9571MWV. Relying on product code
probing sounds like something that may easily break when/if new
variants are produced. ( I've seen new HW variants using the same
ID information being produced in previous companies I've worked. Sure
ROHM wouldn't do this but still... :] ). And producing boards where DTS
does not allow describing the correct components sounds like asking for
a nose-bleed to me... If probing of IC type fails AND there is devices
with wrong PMIC information burned in DT - then fixing it can be a
nightmare. So I would really try make DTS files such that they can be
changed to match the actual board. (Perhaps introduce the compatible
for BD9574MWF - make this driver to match both of the PMICs - leave the
runtime probing here for now - and in parallel work with the DTS files
so that eventually the probing can be removed(?) That was my 10 cents
on this topic :] )

Best Regards
    Matti Vaittinen

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Geert Uytterhoeven]

Hi Matti, Shimoda-san,

On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote:
> > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30
> > > PM
> > <snip>
> > > > --- a/drivers/mfd/bd9571mwv.c
> > > > +++ b/drivers/mfd/bd9571mwv.c
> > > >
> > > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client
> > > > *client,
> > > >         product_code = (unsigned int)ret;
> > > >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > > >                 bd->data = &bd9571mwv_data;
> > > > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > > +               bd->data = &bd9574mwf_data;
> > > >
> > > >         if (!bd->data) {
> > > >                 dev_err(bd->dev, "No found supported device
> > > > %d\n",
> > >
> > > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > > I think it would still be a good idea to document a
> > > "rohm,bd9574mwf"
> > > compatible value in the DT bindings, and let the driver match on
> > > that.
> >
> > In this driver point of view, we can use such the DT bindings,
> > however, in the board point of view, it's difficult to describe
> > which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> > keep this runtime detection.

To clarify: I meant to document and add the compatible value, but
treat both compatible values the same in the driver, and still do runtime
probing.

> First of all - I don't want to insist changes here so my comment can be
> ignored. I would definitely like seeing the support for BD9574 in-tree!
>
> But as a 'nit':
> I don't know what are the difficulties you are referring to so it's
> hard for me to comment this. Without better understanding of board dts
> files - I think BD9574MWF should really have own compatible as I
> understood it is different from the BD9571MWV. Relying on product code
> probing sounds like something that may easily break when/if new
> variants are produced. ( I've seen new HW variants using the same
> ID information being produced in previous companies I've worked. Sure

Yes, this happens from time to time, unfortunately.

> ROHM wouldn't do this but still... :] ). And producing boards where DTS
> does not allow describing the correct components sounds like asking for
> a nose-bleed to me... If probing of IC type fails AND there is devices
> with wrong PMIC information burned in DT - then fixing it can be a
> nightmare. So I would really try make DTS files such that they can be

The issue we're facing is that older Ebisu-4D boards have BD9571, while
newer boards have BD9574.  The schematics say "BD9574MWF-M (tentative
ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible
(ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574).
Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device node
compatible with "rohm,bd9571mwv".  If we have runtime probing, we can
keep on using that for both variants.

> changed to match the actual board. (Perhaps introduce the compatible
> for BD9574MWF - make this driver to match both of the PMICs - leave the
> runtime probing here for now - and in parallel work with the DTS files
> so that eventually the probing can be removed(?) That was my 10 cents
> on this topic :] )

Exactly. Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Dec 10, 2020 at 5:10 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:26 PM
> > On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> <snip>
> > > index 80c6ef0..57bdb6a 100644
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> >
> > > @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
> > >                         ret);
> > >                 return ret;
> > >         }
> > > -
> > > -       if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> > > +       /* Confirm the product code */
> > > +       if (value != bd->data->product_code_val) {
> > >                 dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
> > > -                       value, BD9571MWV_PRODUCT_CODE_VAL);
> > > +                       value, bd->data->product_code_val);
> > >                 return -EINVAL;
> > >         }
> >
> > Reading the product code register, and checking the product code value
> > can be removed, as bd9571mwv_probe() has verified it already.
>
> Indeed. I'll remove this.

OK.

> > > --- a/include/linux/mfd/bd9571mwv.h
> > > +++ b/include/linux/mfd/bd9571mwv.h

> > > + */
> > > +struct bd957x_data {
> > > +       int product_code_val;
> >
> > unsigned int?
>
> We can remove this member.

True.

> Or, keeping this member and then we check the product code by this member
> instead of switch() like below?
>
> /* No build test, JFYI */
> struct bd957x_data *data_array[] = {
>         &bd9571mwv_data,
>         &bd9574mwf_data,
> };
>
> for (i = 0; I < ARRAY_SIZE(data_array); i++) {
>         if (val == data_array[i].product_code_val) {
>                 bd->data = data_array[i];
>                 break;
>         }
> }

Given we probably won't have more than a handful variants, I'm
leaning towards the switch() approach.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Hi Geert-san, Matti,

Thank you for your comments!

> From: Geert Uytterhoeven, Sent: Thursday, December 10, 2020 5:20 PM
> 
> Hi Matti, Shimoda-san,
> 
> On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote:
> > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30
> > > > PM
> > > <snip>
> > > > > --- a/drivers/mfd/bd9571mwv.c
> > > > > +++ b/drivers/mfd/bd9571mwv.c
> > > > >
> > > > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client
> > > > > *client,
> > > > >         product_code = (unsigned int)ret;
> > > > >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > > > >                 bd->data = &bd9571mwv_data;
> > > > > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > > > +               bd->data = &bd9574mwf_data;
> > > > >
> > > > >         if (!bd->data) {
> > > > >                 dev_err(bd->dev, "No found supported device
> > > > > %d\n",
> > > >
> > > > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > > > I think it would still be a good idea to document a
> > > > "rohm,bd9574mwf"
> > > > compatible value in the DT bindings, and let the driver match on
> > > > that.
> > >
> > > In this driver point of view, we can use such the DT bindings,
> > > however, in the board point of view, it's difficult to describe
> > > which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> > > keep this runtime detection.
> 
> To clarify: I meant to document and add the compatible value, but
> treat both compatible values the same in the driver, and still do runtime
> probing.

Thank you! I understood it.

<snip>
> > ROHM wouldn't do this but still... :] ). And producing boards where DTS
> > does not allow describing the correct components sounds like asking for
> > a nose-bleed to me... If probing of IC type fails AND there is devices
> > with wrong PMIC information burned in DT - then fixing it can be a
> > nightmare. So I would really try make DTS files such that they can be
> 
> The issue we're facing is that older Ebisu-4D boards have BD9571, while
> newer boards have BD9574.  The schematics say "BD9574MWF-M (tentative
> ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible
> (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574).
> Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device node
> compatible with "rohm,bd9571mwv".  If we have runtime probing, we can
> keep on using that for both variants.

Thank you very much for explaining this! It's very clear :)

Best regards,
Yoshihiro Shimoda

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Vaittinen, Matti]

On Tue, 2020-12-08 at 17:04 +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>
> ---
>  drivers/mfd/bd9571mwv.c       | 92
> +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/bd9571mwv.h | 80
> +++++++++++++++++++++++++++++++++++++
>  2 files changed, 172 insertions(+)
> 
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 57bdb6a..f8f0a87 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[] = {
>  	{ .name = "bd9571mwv-gpio", },
>  };
>  
> +/* Regmap for BD9571MWV */
>  static const struct regmap_range bd9571mwv_readable_yes_ranges[] = {
>  	regmap_reg_range(BD9571MWV_VENDOR_CODE,
> BD9571MWV_PRODUCT_REVISION),
>  	regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> BD9571MWV_BKUP_MODE_CNT),
> @@ -112,6 +113,95 @@ static const struct bd957x_data bd9571mwv_data =
> {
>  	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
>  };
>  
> +static const struct mfd_cell bd9574mwf_cells[] = {
> +	{ .name = "bd9571mwv-gpio", },

Another 'nit' suggestion which you can ignore if it does not make sense
:)

Are the GPIO blocks 100% identical? If not, then I would suggest
changing this to:
{ .name = "bd9574mwf-gpio", },

and populating the platform_driver id_table for sub driver(s) using
something like:

static const struct platform_device_id bd957x_gpio_id[] = {
        { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
        { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
        { },
};

Then you can get the IC type using
platform_get_device_id(pdev)->driver_data.

Next, I think the parent data from MFD is only used to get the regmap
and dev in sub-devices, right? Maybe you could simplify this and get
rid of the whole MFD parent data structure? I think you can use

pdev->dev.parent to get the parent device and
dev_get_regmap(pdev->dev.parent, NULL);

to get the regmap?

(After this I wonder if you need the
struct bd9571mwv at all?)

> +};
> +
> +/* Regmap for BD9574MWF */
> +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> +	regmap_reg_range(BD9574MWF_VENDOR_CODE,
> BD9574MWF_PRODUCT_REVISION),
> +	regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> +	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
> +	regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
> +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
> +	regmap_reg_range(BD9574MWF_GPIO_INT_SET,
> BD9574MWF_GPIO_INTMASK),
> +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> +	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INT),
> +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTREQ),
> +};

Are you using the other interrupts/statuses or VDCORE MoniVDAC? Should
they be volatile too?

> +
> +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 const struct regmap_irq bd9574mwf_irqs[] = {
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_MD1, 0,
> +		       BD9574MWF_INT_INTREQ_MD1_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E1, 0,
> +		       BD9574MWF_INT_INTREQ_MD2_E1_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E2, 0,
> +		       BD9574MWF_INT_INTREQ_MD2_E2_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_PROT_ERR, 0,
> +		       BD9574MWF_INT_INTREQ_PROT_ERR_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_GP, 0,
> +		       BD9574MWF_INT_INTREQ_GP_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_HOLD_OF, 0,
> +		       BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_WDT_OF, 0,
> +		       BD9574MWF_INT_INTREQ_WDT_OF_INT),
> +	REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_TRG, 0,
> +		       BD9574MWF_INT_INTREQ_BKUP_TRG_INT),
> +};
> +
> +static struct regmap_irq_chip bd9574mwf_irq_chip = {
> +	.name		= "bd9574mwf",
> +	.status_base	= BD9574MWF_INT_INTREQ,
> +	.mask_base	= BD9574MWF_INT_INTMASK,
> +	.ack_base	= BD9574MWF_INT_INTREQ,
> +	.init_ack_masked = true,
> +	.num_regs	= 1,
> +	.irqs		= bd9574mwf_irqs,
> +	.num_irqs	= ARRAY_SIZE(bd9574mwf_irqs),
> +};
> +
> +static const struct bd957x_data bd9574mwf_data = {
> +	.product_code_val = BD9574MWF_PRODUCT_CODE_VAL,
> +	.part_number = BD9574MWF_PART_NUMBER,
> +	.regmap_config = &bd9574mwf_regmap_config,
> +	.irq_chip = &bd9574mwf_irq_chip,
> +	.cells = bd9574mwf_cells,
> +	.num_cells = ARRAY_SIZE(bd9574mwf_cells),
> +};
> +
>  static int bd9571mwv_identify(struct bd9571mwv *bd)
>  {
>  	struct device *dev = bd->dev;
> @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
>  	product_code = (unsigned int)ret;
>  	if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
>  		bd->data = &bd9571mwv_data;
> +	else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> +		bd->data = &bd9574mwf_data;
>  
>  	if (!bd->data) {
>  		dev_err(bd->dev, "No found supported device %d\n",
> diff --git a/include/linux/mfd/bd9571mwv.h
> b/include/linux/mfd/bd9571mwv.h
> index 0126b52..e9e219b 100644
> --- a/include/linux/mfd/bd9571mwv.h
> +++ b/include/linux/mfd/bd9571mwv.h
> @@ -99,6 +99,86 @@ enum bd9571mwv_irqs {
>  	BD9571MWV_IRQ_BKUP_TRG,
>  };
>  
> +/* List of registers for BD9574MWF */
> +#define BD9574MWF_VENDOR_CODE			0x00
> +#define BD9574MWF_VENDOR_CODE_VAL		0xdb
> +#define BD9574MWF_PRODUCT_CODE			0x01
> +#define BD9574MWF_PRODUCT_CODE_VAL		0x74
> +#define BD9574MWF_PRODUCT_REVISION		0x02
> +
> +#define BD9574MWF_I2C_FUSA_MODE			0x10
> +#define BD9574MWF_I2C_MD2_E1_BIT_1		0x11
> +#define BD9574MWF_I2C_MD2_E1_BIT_2		0x12
> +
> +#define BD9574MWF_BKUP_MODE_CNT			0x20
> +#define BD9574MWF_BKUP_MODE_STATUS		0x21
> +#define BD9574MWF_BKUP_RECOVERY_CNT		0x22
> +#define BD9574MWF_BKUP_CTRL_TIM_CNT		0x23
> +#define BD9574MWF_WAITBKUP_WDT_CNT		0x24
> +#define BD9574MWF_BKUP_HOLD_TIM_CNT1		0x26
> +#define BD9574MWF_QLLM_CNT			0x27
> +#define BD9574MWF_BKUP_HOLD_TIM_CNT2		0x28
> +
> +#define BD9574MWF_DCDC_FREQ			0x48
> +
> +#define BD9574MWF_VDCORE_VINIT			0x50
> +#define BD9574MWF_VD09_VINIT			0x51
> +#define BD9574MWF_VDCORE_SETVMAX		0x52
> +#define BD9574MWF_VDCORE_SETVID			0x54
> +#define BD9574MWF_VDCORE_MONIVDAC		0x55
> +#define BD9574MWF_VDCORE_PGD_CNT		0x56
> +
> +#define BD9574MWF_GPIO_DIR			0x60
> +#define BD9574MWF_GPIO_OUT			0x61
> +#define BD9574MWF_GPIO_IN			0x62
> +#define BD9574MWF_GPIO_DEB			0x63
> +#define BD9574MWF_GPIO_INT_SET			0x64
> +#define BD9574MWF_GPIO_INT			0x65
> +#define BD9574MWF_GPIO_INTMASK			0x66
> +#define BD9574MWF_GPIO_MUX			0x67
> +
> +#define BD9574MWF_REG_KEEP(n)			(0x70 + (n))
> +
> +#define BD9574MWF_PMIC_INTERNAL_STATUS		0x80
> +#define BD9574MWF_PROT_ERROR_STATUS0		0x81
> +#define BD9574MWF_PROT_ERROR_STATUS1		0x82
> +#define BD9574MWF_PROT_ERROR_STATUS2		0x83
> +#define BD9574MWF_PROT_ERROR_STATUS3		0x84
> +#define BD9574MWF_PROT_ERROR_STATUS4		0x85
> +#define BD9574MWF_PROT_ERROR_STATUS5		0x86
> +#define BD9574MWF_SYS_ERROR_STATUS		0x87
> +
> +#define BD9574MWF_INT_INTREQ			0x90
> +#define BD9574MWF_INT_INTREQ_MD1_INT		BIT(0)
> +#define BD9574MWF_INT_INTREQ_MD2_E1_INT		BIT(1)
> +#define BD9574MWF_INT_INTREQ_MD2_E2_INT		BIT(2)
> +#define BD9574MWF_INT_INTREQ_PROT_ERR_INT	BIT(3)
> +#define BD9574MWF_INT_INTREQ_GP_INT		BIT(4)
> +#define BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT	BIT(5)
> +#define BD9574MWF_INT_INTREQ_WDT_OF_INT		BIT(6)
> +#define BD9574MWF_INT_INTREQ_BKUP_TRG_INT	BIT(7)
> +#define BD9574MWF_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 BD9574MWF_PART_NUMBER			"BD9574MWF"
> +
> +/* Define the BD9574MWF IRQ numbers */
> +enum bd9574mwf_irqs {
> +	BD9574MWF_IRQ_MD1,
> +	BD9574MWF_IRQ_MD2_E1,
> +	BD9574MWF_IRQ_MD2_E2,
> +	BD9574MWF_IRQ_PROT_ERR,
> +	BD9574MWF_IRQ_GP,
> +	BD9574MWF_IRQ_BKUP_HOLD_OF,
> +	BD9574MWF_IRQ_WDT_OF,
> +	BD9574MWF_IRQ_BKUP_TRG,
> +};
> +
>  /**
>   * struct bd957x_data - internal data for the bd957x driver
>   *

Overall a good looking driver! Thanks a lot!

Best Regards
    Matti

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)

RE: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, December 10, 2020 5:23 PM
<snip>
> > Or, keeping this member and then we check the product code by this member
> > instead of switch() like below?
> >
> > /* No build test, JFYI */
> > struct bd957x_data *data_array[] = {
> >         &bd9571mwv_data,
> >         &bd9574mwf_data,
> > };
> >
> > for (i = 0; I < ARRAY_SIZE(data_array); i++) {
> >         if (val == data_array[i].product_code_val) {
> >                 bd->data = data_array[i];
> >                 break;
> >         }
> > }
> 
> Given we probably won't have more than a handful variants, I'm
> leaning towards the switch() approach.

OK. I got it. I'll use switch().

Best regards,
Yoshihiro Shimoda

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Vaittinen, Matti]

On Thu, 2020-12-10 at 09:19 +0100, Geert Uytterhoeven wrote:
> Hi Matti, Shimoda-san,
> 
> On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote:
> > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020
> > > > 10:30
> > > > PM
> > > <snip>
> > > > > --- a/drivers/mfd/bd9571mwv.c
> > > > > +++ b/drivers/mfd/bd9571mwv.c
> > > > > 
> > > > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct
> > > > > i2c_client
> > > > > *client,
> > > > >         product_code = (unsigned int)ret;
> > > > >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > > > >                 bd->data = &bd9571mwv_data;
> > > > > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > > > +               bd->data = &bd9574mwf_data;
> > > > > 
> > > > >         if (!bd->data) {
> > > > >                 dev_err(bd->dev, "No found supported device
> > > > > %d\n",
> > > > 
> > > > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > > > I think it would still be a good idea to document a
> > > > "rohm,bd9574mwf"
> > > > compatible value in the DT bindings, and let the driver match
> > > > on
> > > > that.
> > > 
> > > In this driver point of view, we can use such the DT bindings,
> > > however, in the board point of view, it's difficult to describe
> > > which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> > > keep this runtime detection.
> 
> To clarify: I meant to document and add the compatible value, but
> treat both compatible values the same in the driver, and still do
> runtime
> probing.
> 
> > First of all - I don't want to insist changes here so my comment
> > can be
> > ignored. I would definitely like seeing the support for BD9574 in-
> > tree!
> > 
> > But as a 'nit':
> > I don't know what are the difficulties you are referring to so it's
> > hard for me to comment this. Without better understanding of board
> > dts
> > files - I think BD9574MWF should really have own compatible as I
> > understood it is different from the BD9571MWV. Relying on product
> > code
> > probing sounds like something that may easily break when/if new
> > variants are produced. ( I've seen new HW variants using the same
> > ID information being produced in previous companies I've worked.
> > Sure
> 
> Yes, this happens from time to time, unfortunately.
> 
> > ROHM wouldn't do this but still... :] ). And producing boards where
> > DTS
> > does not allow describing the correct components sounds like asking
> > for
> > a nose-bleed to me... If probing of IC type fails AND there is
> > devices
> > with wrong PMIC information burned in DT - then fixing it can be a
> > nightmare. So I would really try make DTS files such that they can
> > be
> 
> The issue we're facing is that older Ebisu-4D boards have BD9571,
> while
> newer boards have BD9574.  The schematics say "BD9574MWF-M (tentative
> ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible
> (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574).
> Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device
> node
> compatible with "rohm,bd9571mwv".  If we have runtime probing, we can
> keep on using that for both variants.

Thank you for the explanation :) This is a nice learning experience for
me!

> > changed to match the actual board. (Perhaps introduce the
> > compatible
> > for BD9574MWF - make this driver to match both of the PMICs - leave
> > the
> > runtime probing here for now - and in parallel work with the DTS
> > files
> > so that eventually the probing can be removed(?) That was my 10
> > cents
> > on this topic :] )
> 
> Exactly. Thanks!

I am more than happy with this solution :)

--Matti

RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Hi Matti,

> From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 5:28 PM
> 
> On Tue, 2020-12-08 at 17:04 +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>
> > ---
> >  drivers/mfd/bd9571mwv.c       | 92
> > +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/bd9571mwv.h | 80
> > +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 172 insertions(+)
> >
> > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > index 57bdb6a..f8f0a87 100644
> > --- a/drivers/mfd/bd9571mwv.c
> > +++ b/drivers/mfd/bd9571mwv.c
> > @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[] = {
> >  	{ .name = "bd9571mwv-gpio", },
> >  };
> >
> > +/* Regmap for BD9571MWV */
> >  static const struct regmap_range bd9571mwv_readable_yes_ranges[] = {
> >  	regmap_reg_range(BD9571MWV_VENDOR_CODE,
> > BD9571MWV_PRODUCT_REVISION),
> >  	regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> > BD9571MWV_BKUP_MODE_CNT),
> > @@ -112,6 +113,95 @@ static const struct bd957x_data bd9571mwv_data =
> > {
> >  	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
> >  };
> >
> > +static const struct mfd_cell bd9574mwf_cells[] = {
> > +	{ .name = "bd9571mwv-gpio", },
> 
> Another 'nit' suggestion which you can ignore if it does not make sense
> :)
> 
> Are the GPIO blocks 100% identical? If not, then I would suggest
> changing this to:
> { .name = "bd9574mwf-gpio", },

The GPIO blocks are not 100% identical. BD9574MWF seems to have
an additional feature which GPIOx pin are used for other mode by
using gpio mux regisiter.

> and populating the platform_driver id_table for sub driver(s) using
> something like:
> 
> static const struct platform_device_id bd957x_gpio_id[] = {
>         { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
>         { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
>         { },
> };
> 
> Then you can get the IC type using
> platform_get_device_id(pdev)->driver_data.

I got it. So, perhaps, I should add these types into
include/linux/mfd/rohm-generic.h.

> Next, I think the parent data from MFD is only used to get the regmap
> and dev in sub-devices, right? Maybe you could simplify this and get
> rid of the whole MFD parent data structure? I think you can use
> 
> pdev->dev.parent to get the parent device and
> dev_get_regmap(pdev->dev.parent, NULL);
> 
> to get the regmap?

IIUC, these comments are related to gpio-bd9571mwv.c.
# Also, bd9571mwv-regulator.c?
If so, I didn't try this yet, but perhaps, we can modify such things.

> (After this I wonder if you need the
> struct bd9571mwv at all?)

I'm sorry, but I could not understand this.

> > +};
> > +
> > +/* Regmap for BD9574MWF */
> > +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> > +	regmap_reg_range(BD9574MWF_VENDOR_CODE,
> > BD9574MWF_PRODUCT_REVISION),
> > +	regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> > +	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
> > +	regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
> > +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
> > +	regmap_reg_range(BD9574MWF_GPIO_INT_SET,
> > BD9574MWF_GPIO_INTMASK),
> > +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_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(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> > +	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INT),
> > +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTREQ),
> > +};
> 
> Are you using the other interrupts/statuses or VDCORE MoniVDAC? Should
> they be volatile too?

At least, since BD9571MWV_DVFS_MONIVDAC is used on bd9571mfv-regulator.c,
I should add it into the volatile.

<snip>
> >  /**
> >   * struct bd957x_data - internal data for the bd957x driver
> >   *
> 
> Overall a good looking driver! Thanks a lot!

Thank you very much for your review!

By the way, I realized Linux kernel supports bd9576-regulator.c
and it has "bd957x", but it doesn't seem to be compatible with BD9571.
So, I wonder if I should not use "bd957x" in the bd9571mwv driver to avoid
confusion. But, what do you think?

Best regards,
Yoshihiro Shimoda

Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Vaittinen, Matti]

Hi Yoshihiro san,

On Thu, 2020-12-10 at 10:58 +0000, Yoshihiro Shimoda wrote:
> Hi Matti,
> 
> > From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 5:28 PM
> > 
> > On Tue, 2020-12-08 at 17:04 +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>
> > > ---
> > >  drivers/mfd/bd9571mwv.c       | 92
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/bd9571mwv.h | 80
> > > +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 172 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > > index 57bdb6a..f8f0a87 100644
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> > > @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[]
> > > = {
> > >  	{ .name = "bd9571mwv-gpio", },
> > >  };
> > > 
> > > +/* Regmap for BD9571MWV */
> > >  static const struct regmap_range bd9571mwv_readable_yes_ranges[]
> > > = {
> > >  	regmap_reg_range(BD9571MWV_VENDOR_CODE,
> > > BD9571MWV_PRODUCT_REVISION),
> > >  	regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> > > BD9571MWV_BKUP_MODE_CNT),
> > > @@ -112,6 +113,95 @@ static const struct bd957x_data
> > > bd9571mwv_data =
> > > {
> > >  	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > >  };
> > > 
> > > +static const struct mfd_cell bd9574mwf_cells[] = {
> > > +	{ .name = "bd9571mwv-gpio", },
> > 
> > Another 'nit' suggestion which you can ignore if it does not make
> > sense
> > :)
> > 
> > Are the GPIO blocks 100% identical? If not, then I would suggest
> > changing this to:
> > { .name = "bd9574mwf-gpio", },
> 
> The GPIO blocks are not 100% identical. BD9574MWF seems to have
> an additional feature which GPIOx pin are used for other mode by
> using gpio mux regisiter.
> 
> > and populating the platform_driver id_table for sub driver(s) using
> > something like:
> > 
> > static const struct platform_device_id bd957x_gpio_id[] = {
> >         { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> >         { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
> >         { },
> > };
> > 
> > Then you can get the IC type using
> > platform_get_device_id(pdev)->driver_data.
> 
> I got it. So, perhaps, I should add these types into
> include/linux/mfd/rohm-generic.h.

That would make sense to me. I like the idea of collecting the IDs in
same place - but on the other hand, the define in id-table is not
really visible outside the sub-driver - so you can probably also define
the type in sub-device driver if you wish. I don't have strong opinion
on that.
> 
> > Next, I think the parent data from MFD is only used to get the
> > regmap
> > and dev in sub-devices, right? Maybe you could simplify this and
> > get
> > rid of the whole MFD parent data structure? I think you can use
> > 
> > pdev->dev.parent to get the parent device and
> > dev_get_regmap(pdev->dev.parent, NULL);
> > 
> > to get the regmap?
> 
> IIUC, these comments are related to gpio-bd9571mwv.c.
> # Also, bd9571mwv-regulator.c?
> If so, I didn't try this yet, but perhaps, we can modify such things.

Correct. My suggestion was related to how regmap and dev pointers are
obtained in sub-devices. It is related to MFD because I think you could
remove the MFD driver data usage.

> 
> > (After this I wonder if you need the
> > struct bd9571mwv at all?)
> 
> I'm sorry, but I could not understand this.

I believe the struct bd9571mwv is defined only to collect all the MFD
driver data in one struct so that it can be passed to sub-drivers using
the MFD device private data. But as far as I can tell, the sub-devices
only use regmap and dev pointers from this data. If this is the case,
then I think there is no need to define this struct or populate the MFD
driver data (unless I am missing something).

(And as a further clen-up, one could probably switch from:
regmap_add_irq_chip
to
devm_regmap_add_irq_chip
and get rid of the remove function)

but as I said - these are only 'nit' comments and I am not insisting on
changing these. Especially since some of the comments are more related
to changing the original bd9571mwv than adding support for this new IC.
I just think one might be able to make this a little bit simpler :)

> <snip>
> > >  /**
> > >   * struct bd957x_data - internal data for the bd957x driver
> > >   *
> > 
> > Overall a good looking driver! Thanks a lot!
> 
> Thank you very much for your review!
> 
> By the way, I realized Linux kernel supports bd9576-regulator.c
> and it has "bd957x", but it doesn't seem to be compatible with
> BD9571.
> So, I wonder if I should not use "bd957x" in the bd9571mwv driver to
> avoid
> confusion. But, what do you think?

Valid point. I think BD9573 and BD9573 are close to each-others but
largely different from BD9571 and BD9574... Good example why wildcards
are so hard. I have previously attempted to use the wildcards in ROHM
PMIC software naming - and usually failed. The numbering does not
really seem to reflect the functionality... So maybe i am not the best
person to comment on this XD

On the other hand, sometimes we want to highlight that some of the
functions/defines are used by all IC versions a driver supports - while
others are IC specific. For example, inside the driver which supports
BD71837 and BD71847 I used BD718XX as a prefix for defines that were
common for both ICs. IC specific defines I named with BD71837 and
BD71847. I think that was quite clear inside the driver (until BD71850
was made - which uses same defines as BD71847... :| )

So... My suggestion - you can probably use wildcards inside a driver
(and comment things when wildcards do not match anymore). But I would
not add wildcards to any globally visible defines (in definitions
included from headers, file names, ...)

As Rob put it a while ago - "Naming is hard". :)

Best Regards
    Matti Vaittinen

RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Hi Matti-san,

> From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 8:56 PM
> 
> Hi Yoshihiro san,
> 
> On Thu, 2020-12-10 at 10:58 +0000, Yoshihiro Shimoda wrote:
> > Hi Matti,
> >
> > > From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 5:28 PM
> > >
> > > On Tue, 2020-12-08 at 17:04 +0900, Yoshihiro Shimoda wrote:
<snip>
> > > and populating the platform_driver id_table for sub driver(s) using
> > > something like:
> > >
> > > static const struct platform_device_id bd957x_gpio_id[] = {
> > >         { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> > >         { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
> > >         { },
> > > };
> > >
> > > Then you can get the IC type using
> > > platform_get_device_id(pdev)->driver_data.
> >
> > I got it. So, perhaps, I should add these types into
> > include/linux/mfd/rohm-generic.h.
> 
> That would make sense to me. I like the idea of collecting the IDs in
> same place - but on the other hand, the define in id-table is not
> really visible outside the sub-driver - so you can probably also define
> the type in sub-device driver if you wish. I don't have strong opinion
> on that.

I got it. ROHM_CHIP_TYPE_BD957[14] are used from both gpio and regulator
drivers so that adding IDs into rohm-generic.h is reasonable.

> > > Next, I think the parent data from MFD is only used to get the
> > > regmap
> > > and dev in sub-devices, right? Maybe you could simplify this and
> > > get
> > > rid of the whole MFD parent data structure? I think you can use
> > >
> > > pdev->dev.parent to get the parent device and
> > > dev_get_regmap(pdev->dev.parent, NULL);
> > >
> > > to get the regmap?
> >
> > IIUC, these comments are related to gpio-bd9571mwv.c.
> > # Also, bd9571mwv-regulator.c?
> > If so, I didn't try this yet, but perhaps, we can modify such things.
> 
> Correct. My suggestion was related to how regmap and dev pointers are
> obtained in sub-devices. It is related to MFD because I think you could
> remove the MFD driver data usage.

I understood it.

> > > (After this I wonder if you need the
> > > struct bd9571mwv at all?)
> >
> > I'm sorry, but I could not understand this.
> 
> I believe the struct bd9571mwv is defined only to collect all the MFD
> driver data in one struct so that it can be passed to sub-drivers using
> the MFD device private data. But as far as I can tell, the sub-devices
> only use regmap and dev pointers from this data. If this is the case,
> then I think there is no need to define this struct or populate the MFD
> driver data (unless I am missing something).
> 
> (And as a further clen-up, one could probably switch from:
> regmap_add_irq_chip
> to
> devm_regmap_add_irq_chip
> and get rid of the remove function)

Thank you for the detail. I understood it.

> but as I said - these are only 'nit' comments and I am not insisting on
> changing these. Especially since some of the comments are more related
> to changing the original bd9571mwv than adding support for this new IC.
> I just think one might be able to make this a little bit simpler :)

I got it :) For now, I would like to focus adding BD9574MWF support at first.
After that, I'll try to clean-up these drivers later.

> > <snip>
> > > >  /**
> > > >   * struct bd957x_data - internal data for the bd957x driver
> > > >   *
> > >
> > > Overall a good looking driver! Thanks a lot!
> >
> > Thank you very much for your review!
> >
> > By the way, I realized Linux kernel supports bd9576-regulator.c
> > and it has "bd957x", but it doesn't seem to be compatible with
> > BD9571.
> > So, I wonder if I should not use "bd957x" in the bd9571mwv driver to
> > avoid
> > confusion. But, what do you think?
> 
> Valid point. I think BD9573 and BD9573 are close to each-others but
> largely different from BD9571 and BD9574... Good example why wildcards
> are so hard. I have previously attempted to use the wildcards in ROHM
> PMIC software naming - and usually failed. The numbering does not
> really seem to reflect the functionality... So maybe i am not the best
> person to comment on this XD
> 
> On the other hand, sometimes we want to highlight that some of the
> functions/defines are used by all IC versions a driver supports - while
> others are IC specific. For example, inside the driver which supports
> BD71837 and BD71847 I used BD718XX as a prefix for defines that were
> common for both ICs. IC specific defines I named with BD71837 and
> BD71847. I think that was quite clear inside the driver (until BD71850
> was made - which uses same defines as BD71847... :| )
> 
> So... My suggestion - you can probably use wildcards inside a driver
> (and comment things when wildcards do not match anymore). But I would
> not add wildcards to any globally visible defines (in definitions
> included from headers, file names, ...)

Thank you for the suggestion! I think it's a good idea. So, I'll move
struct bd957x_data from the header to MFD driver.

> As Rob put it a while ago - "Naming is hard". :)

I think so :)

Best regards,
Yoshihiro Shimoda

RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Hi again,

> From: Yoshihiro Shimoda, Sent: Friday, December 11, 2020 9:49 AM
> <snip>
> > but as I said - these are only 'nit' comments and I am not insisting on
> > changing these. Especially since some of the comments are more related
> > to changing the original bd9571mwv than adding support for this new IC.
> > I just think one might be able to make this a little bit simpler :)
> 
> I got it :) For now, I would like to focus adding BD9574MWF support at first.
> After that, I'll try to clean-up these drivers later.

JFYI.
I submitted v2 patch series with this clean up because this is needed
to avoid "struct bd957x_data" in the include/linux/mfd/bd9571mwv.h.

Best regards,
Yoshihiro Shimoda