Re: [PATCH v6 17/23] mfd: max77686: Add Maxim 77802 PMIC support

Hello Krzysztof,

On 07/04/2014 01:30 PM, Krzysztof Kozlowski wrote:

On pią, 2014-07-04 at 11:55 +0200, Javier Martinez Canillas wrote:

quoted

Maxim MAX77802 is a power management chip that contains 10 high
efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
to power up application processors and peripherals, a 2-channel
32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
to program the individual regulators, clocks outputs and the RTC.

This patch adds support for MAX77802 to the MAX77686 driver and is
based on a driver added to the Chrome OS kernel 3.8 by Simon Glass.

Signed-off-by: Javier Martinez Canillas <redacted>
---

    NOTE: I didn't carry previous {Review,Acked,Tested}-by tags since
    this patch extending MAX77686 is quite different than the old one
    adding a new mfd driver. So review and test is highly appreciated.

    Changes since v5:
     - Extend the 77686 driver to support 77802 instead of adding a new driver.
       Suggested by Lee Jones.

    Changes since v4:
     - Use consistent expressions when checking for NULL values.
       Suggested by Krzysztof Kozlowski.
     - Remove unused defines. Suggested by Krzysztof Kozlowski.
     - Explain why IRQ is disabled on suspend. Suggested by Krzysztof Kozlowski.

    Changes since v3:
     - Remove unnecessary OOM error message since the mm subsystem already logs it.

    Changes since v2:
     - Split the DT binding docs in a separate patch and improve the documentation.
       Suggested by Mark Brown.
     - Add all the devices in the MFD driver instead of doing in separate patches.
       Suggested by Mark Brown.

    Changes since v1:
     - Convert max77{686,802} to regmap irq API and get rid of max77{686,802}-irq.c
       Suggested by Krzysztof Kozlowski.
     - Don't protect max77802 mfd_cells using Kconfig options since mfd core omits
       devices that don't match. Suggested by Lee Jones.
     - Change mfd driver to be tristate instead of boolean. Suggested by Mark Brown.
     - Change binding "voltage-regulators" property to "regulators" to be consistent
       with other PMIC drivers. Suggested by Mark Brown.
     - Use regulators node names instead of the deprecated "regulator-compatible"
       property. Suggested by Mark Brown.
     - Use the new descriptor-based GPIO interface instead of the deprecated
---
 drivers/mfd/Kconfig                  |   6 +-
 drivers/mfd/max77686.c               | 187 ++++++++++++++++++++++++++-----
 include/linux/mfd/max77686-private.h | 208 ++++++++++++++++++++++++++++++++++-
 include/linux/mfd/max77686.h         |  60 +++++++++-
 4 files changed, 428 insertions(+), 33 deletions(-)

Three comments below, after fixing:
Reviewed-by: Krzysztof Kozlowski <redacted>

(...)

quoted

@@ -55,6 +123,17 @@ static struct regmap_config max77686_rtc_regmap_config = {
 	.val_bits = 8,
 };
 
+static struct regmap_config max77802_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.writeable_reg = max77802_is_accessible_reg,
+	.readable_reg = max77802_is_accessible_reg,
+	.precious_reg = max77802_is_precious_reg,
+	.volatile_reg = max77802_is_volatile_reg,
+	.name = "max77802-pmic",
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static const struct regmap_irq max77686_irqs[] = {
 	/* INT1 interrupts */
 	{ .reg_offset = 0, .mask = MAX77686_INT1_PWRONF_MSK, },

@@ -98,9 +177,34 @@ static const struct regmap_irq_chip max77686_rtc_irq_chip = {
 	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
 };
 
+static const struct regmap_irq_chip max77802_irq_chip = {
+	.name			= "max77802-pmic",
+	.status_base		= MAX77802_REG_INT1,
+	.mask_base		= MAX77802_REG_INT1MSK,
+	.num_regs		= 2,
+	.irqs			= max77686_irqs, /* same masks than 77686 */

"same masks as 77686"

Ok

quoted

+	.num_irqs		= ARRAY_SIZE(max77686_irqs),
+};
+
+static const struct regmap_irq_chip max77802_rtc_irq_chip = {
+	.name			= "max77802-rtc",
+	.status_base		= MAX77802_RTC_INT,
+	.mask_base		= MAX77802_RTC_INTM,
+	.num_regs		= 1,
+	.irqs			= max77686_rtc_irqs, /* same masks than 77686 */

Ditto

Ok

quoted

+	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
+};
+
 static const struct of_device_id max77686_pmic_dt_match[] = {
-	{.compatible = "maxim,max77686", .data = NULL},
-	{},
+	{
+		.compatible = "maxim,max77686",
+		.data = (void *)TYPE_MAX77686,
+	},
+	{
+		.compatible = "maxim,max77802",
+		.data = (void *)TYPE_MAX77802,
+	},
+	{ },
 };
 
 static void max77686_dt_parse_dvs_gpio(struct device *dev)

@@ -236,6 +340,12 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	struct max77686_platform_data *pdata = dev_get_platdata(&i2c->dev);
 	unsigned int data;
 	int ret = 0;
+	const struct regmap_config *config;
+	const struct regmap_irq_chip *irq_chip;
+	const struct regmap_irq_chip *rtc_irq_chip;
+	struct regmap **rtc_regmap;
+	const struct mfd_cell *cells;
+	int n_devs;
 
 	if (IS_ENABLED(CONFIG_OF) && i2c->dev.of_node && !pdata)
 		pdata = max77686_i2c_parse_dt_pdata(&i2c->dev);

@@ -258,56 +368,77 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	max77686->wakeup = pdata->wakeup;
 	max77686->irq = i2c->irq;
 
-	max77686->regmap = devm_regmap_init_i2c(i2c, &max77686_regmap_config);
+	if (max77686->type == TYPE_MAX77686) {
+		config = &max77686_regmap_config;
+		irq_chip = &max77686_irq_chip;
+		rtc_irq_chip = &max77686_rtc_irq_chip;
+		rtc_regmap = &max77686->rtc_regmap;
+		cells =  max77686_devs;
+		n_devs = ARRAY_SIZE(max77686_devs);
+	} else {
+		config = &max77802_regmap_config;
+		irq_chip = &max77802_irq_chip;
+		rtc_irq_chip = &max77802_rtc_irq_chip;
+		rtc_regmap = &max77686->regmap;
+		cells =  max77802_devs;
+		n_devs = ARRAY_SIZE(max77802_devs);
+	}
+
+	max77686->regmap = devm_regmap_init_i2c(i2c, config);
 	if (IS_ERR(max77686->regmap)) {
 		ret = PTR_ERR(max77686->regmap);
 		dev_err(max77686->dev, "Failed to allocate register map: %d\n",
-				ret);
+			ret);
 		return ret;
 	}
 
 	ret = regmap_read(max77686->regmap, MAX77686_REG_DEVICE_ID, &data);
 	if (ret < 0) {
 		dev_err(max77686->dev,
-			"device not found on this channel (this is not an error)\n");
-		return -ENODEV;
-	}
-
-	max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
-	if (!max77686->rtc) {
-		dev_err(max77686->dev, "Failed to allocate I2C device for RTC\n");
+			"device not found on this channel\n");
 		return -ENODEV;
 	}
-	i2c_set_clientdata(max77686->rtc, max77686);
 
-	max77686->rtc_regmap = devm_regmap_init_i2c(max77686->rtc,
-						    &max77686_rtc_regmap_config);
-	if (IS_ERR(max77686->rtc_regmap)) {
-		ret = PTR_ERR(max77686->rtc_regmap);
-		dev_err(max77686->dev, "failed to allocate RTC regmap: %d\n",
-			ret);
-		goto err_unregister_i2c;
+	if (max77686->type == TYPE_MAX77686) {
+		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
+		if (!max77686->rtc) {
+			dev_err(max77686->dev,
+				"Failed to allocate I2C device for RTC\n");
+			return -ENODEV;
+		}
+		i2c_set_clientdata(max77686->rtc, max77686);
+
+		max77686->rtc_regmap =
+			devm_regmap_init_i2c(max77686->rtc,
+					     &max77686_rtc_regmap_config);
+		if (IS_ERR(max77686->rtc_regmap)) {
+			ret = PTR_ERR(max77686->rtc_regmap);
+			dev_err(max77686->dev,
+				"failed to allocate RTC regmap: %d\n",
+				ret);
+			goto err_unregister_i2c;
+		}
 	}
 
 	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
 				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
-				  IRQF_SHARED, 0, &max77686_irq_chip,
+				  IRQF_SHARED, 0, irq_chip,
 				  &max77686->irq_data);
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
 		goto err_unregister_i2c;
 	}
-	ret = regmap_add_irq_chip(max77686->rtc_regmap, max77686->irq,
+
+	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
 				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
-				  IRQF_SHARED, 0, &max77686_rtc_irq_chip,
+				  IRQF_SHARED, 0, rtc_irq_chip,
 				  &max77686->rtc_irq_data);
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
 		goto err_del_irqc;
 	}
 
-	ret = mfd_add_devices(max77686->dev, -1, max77686_devs,
-			      ARRAY_SIZE(max77686_devs), NULL, 0, NULL);
+	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
 		goto err_del_rtc_irqc;

@@ -320,7 +451,8 @@ err_del_rtc_irqc:
 err_del_irqc:
 	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
 err_unregister_i2c:
-	i2c_unregister_device(max77686->rtc);
+	if (max77686->type == TYPE_MAX77686)
+		i2c_unregister_device(max77686->rtc);
 
 	return ret;
 }

@@ -341,6 +473,7 @@ static int max77686_i2c_remove(struct i2c_client *i2c)

In remove() you should unregister dummy RTC I2C device only on MAX77686
(77802 does not add it).

Yes, I added the check to the probe()'s error path but forgot about the remove()
function. Thanks a lot for pointing this out.

Best regards,
Krzysztof

Best regards,
Javier