[PATCH v3 0/4] mfd: Intel SoC Power Management IC

[PATCH v3 0/4] mfd: Intel SoC Power Management IC

[Zhu, Lejun]

Devices based on Intel SoC products such as Baytrail have a Power Management IC. In the PMIC there are subsystems for voltage regulation, A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called Crystal Cove.

This series contains common code for these PMICs, and device specific support for Crystal Cove.

v2:
- Use regmap instead of creating our own I2C read/write callbacks.
- Add one missing EXPORT_SYMBOL.
- Remove some duplicate code and put them into pmic_regmap_load_from_hw.
v3:
- Use regmap-irq and remove lots of duplicate code.
- Remove 2 unused APIs.
- Some other cleanup.

Zhu, Lejun (4):
  mfd: intel_soc_pmic: Core driver
  mfd: intel_soc_pmic: I2C interface
  mfd: intel_soc_pmic: Crystal Cove support
  mfd: intel_soc_pmic: Build files

 drivers/mfd/Kconfig                |  12 +++
 drivers/mfd/Makefile               |   3 +
 drivers/mfd/intel_soc_pmic_core.c  | 212 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_core.h  |  44 ++++++++
 drivers/mfd/intel_soc_pmic_crc.c   | 175 ++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_i2c.c   | 150 ++++++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic.h |  27 +++++
 7 files changed, 623 insertions(+)
 create mode 100644 drivers/mfd/intel_soc_pmic_core.c
 create mode 100644 drivers/mfd/intel_soc_pmic_core.h
 create mode 100644 drivers/mfd/intel_soc_pmic_crc.c
 create mode 100644 drivers/mfd/intel_soc_pmic_i2c.c
 create mode 100644 include/linux/mfd/intel_soc_pmic.h

-- 
1.8.3.2

[PATCH v3 1/4] mfd: intel_soc_pmic: Core driver

[Zhu, Lejun]

This patch provides the common code for the intel_soc_pmic MFD driver, such as read/write register and set up IRQ.

v2:
- Use regmap instead of our own callbacks for read/write.
- Add one missing EXPORT_SYMBOL.
- Remove some duplicate code and put them into pmic_regmap_load_from_hw.
v3:
- Use regmap-irq. Remove our own pmic_regmap_* and IRQ handling code.
- Remove intel_soc_pmic_dev() because gpio driver no longer uses it.
- Remove intel_soc_pmic_set_pdata() because currently it's not used.
- Use EXPORT_SYMBOL_GPL for exposed APIs.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_core.c  | 212 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_core.h  |  44 ++++++++
 include/linux/mfd/intel_soc_pmic.h |  27 +++++
 3 files changed, 283 insertions(+)
 create mode 100644 drivers/mfd/intel_soc_pmic_core.c
 create mode 100644 drivers/mfd/intel_soc_pmic_core.h
 create mode 100644 include/linux/mfd/intel_soc_pmic.h

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
new file mode 100644
index 0000000..4f95a4a
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -0,0 +1,212 @@
+/*
+ * intel_soc_pmic_core.c - Intel SoC PMIC Core Functions
+ *
+ * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
+ *
+ * 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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/mfd/core.h>
+#include <linux/err.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/acpi.h>
+#include <linux/version.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include "intel_soc_pmic_core.h"
+
+static DEFINE_MUTEX(pmic_lock);	/* protect pmic */
+static struct intel_soc_pmic *pmic;
+
+/*
+ * Read from a PMIC register
+ */
+int intel_soc_pmic_readb(int reg)
+{
+	int ret;
+	unsigned int val;
+
+	mutex_lock(&pmic_lock);
+
+	if (!pmic) {
+		ret = -EIO;
+	} else {
+		ret = regmap_read(pmic->regmap, reg, &val);
+		if (!ret)
+			ret = val;
+	}
+
+	mutex_unlock(&pmic_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_soc_pmic_readb);
+
+/*
+ * Write to a PMIC register
+ */
+int intel_soc_pmic_writeb(int reg, u8 val)
+{
+	int ret;
+
+	mutex_lock(&pmic_lock);
+
+	if (!pmic)
+		ret = -EIO;
+	else
+		ret = regmap_write(pmic->regmap, reg, val);
+
+	mutex_unlock(&pmic_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_soc_pmic_writeb);
+
+/*
+ * Set 1 bit in a PMIC register
+ */
+int intel_soc_pmic_setb(int reg, u8 mask)
+{
+	int ret;
+
+	mutex_lock(&pmic_lock);
+
+	if (!pmic)
+		ret = -EIO;
+	else
+		ret = regmap_update_bits(pmic->regmap, reg, mask, mask);
+
+	mutex_unlock(&pmic_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_soc_pmic_setb);
+
+/*
+ * Clear 1 bit in a PMIC register
+ */
+int intel_soc_pmic_clearb(int reg, u8 mask)
+{
+	int ret;
+
+	mutex_lock(&pmic_lock);
+
+	if (!pmic)
+		ret = -EIO;
+	else
+		ret = regmap_update_bits(pmic->regmap, reg, mask, 0);
+
+	mutex_unlock(&pmic_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_soc_pmic_clearb);
+
+/*
+* Set and clear multiple bits of a PMIC register
+*/
+int intel_soc_pmic_update(int reg, u8 val, u8 mask)
+{
+	int ret;
+	
+	mutex_lock(&pmic_lock);
+	
+	if (!pmic)
+		ret = -EIO;
+	else
+		ret = regmap_update_bits(pmic->regmap, reg, mask, val);
+	
+	mutex_unlock(&pmic_lock);
+	
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_soc_pmic_update);
+
+int intel_pmic_add(struct intel_soc_pmic *chip)
+{
+	int ret;
+	struct intel_soc_pmic_config *cfg = chip->config;
+
+	mutex_lock(&pmic_lock);
+
+	if (pmic != NULL) {
+		mutex_unlock(&pmic_lock);
+		return -EBUSY;
+	}
+
+	pmic = chip;
+
+	mutex_unlock(&pmic_lock);
+
+	if (cfg->init) {
+		ret = cfg->init();
+		if (ret != 0)
+			goto err;
+	}
+
+	ret = regmap_add_irq_chip(chip->regmap, chip->irq,
+				  cfg->irq_flags | IRQF_ONESHOT,
+				  0, cfg->irq_chip,
+				  &chip->irq_chip_data);
+	if (ret != 0)
+		goto err;
+
+	ret = enable_irq_wake(chip->irq);
+	if (ret != 0)
+		dev_warn(chip->dev, "Can't enable IRQ as wake source: %d\n",
+			 ret);
+
+	ret = mfd_add_devices(chip->dev, -1, cfg->cell_dev,
+			       cfg->n_cell_devs, NULL, 0,
+			       regmap_irq_get_domain(chip->irq_chip_data));
+
+	if (ret)
+		goto err_del_irq_chip;
+
+	return 0;
+
+err_del_irq_chip:
+	regmap_del_irq_chip(chip->irq, chip->irq_chip_data);
+err:
+	mutex_lock(&pmic_lock);
+	if (pmic == chip)
+		pmic = NULL;
+	mutex_unlock(&pmic_lock);
+	return ret;
+}
+
+int intel_pmic_remove(struct intel_soc_pmic *chip)
+{
+	mutex_lock(&pmic_lock);
+
+	if (pmic != chip) {
+		mutex_unlock(&pmic_lock);
+		return -ENODEV;
+	}
+
+	pmic = NULL;
+
+	mutex_unlock(&pmic_lock);
+
+	regmap_del_irq_chip(chip->irq, chip->irq_chip_data);
+
+	mfd_remove_devices(chip->dev);
+
+	return 0;
+}
diff --git a/drivers/mfd/intel_soc_pmic_core.h b/drivers/mfd/intel_soc_pmic_core.h
new file mode 100644
index 0000000..9189ca5
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.h
@@ -0,0 +1,44 @@
+/*
+ * intel_soc_pmic_core.h - Intel SoC PMIC MFD Driver
+ *
+ * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
+ *
+ * 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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#ifndef __INTEL_SOC_PMIC_CORE_H__
+#define __INTEL_SOC_PMIC_CORE_H__
+
+struct intel_soc_pmic_config {
+	const char			*label;
+	unsigned long			irq_flags;
+	struct mfd_cell			*cell_dev;
+	int				n_cell_devs;
+	struct regmap_config		*regmap_cfg;
+	struct regmap_irq_chip		*irq_chip;
+	int				(*init)(void);
+};
+
+struct intel_soc_pmic {
+	struct intel_soc_pmic_config	*config;
+	struct device			*dev;
+	int				irq;
+	struct regmap			*regmap;
+	struct regmap_irq_chip_data	*irq_chip_data;
+};
+
+int intel_pmic_add(struct intel_soc_pmic *chip);
+int intel_pmic_remove(struct intel_soc_pmic *chip);
+
+extern struct intel_soc_pmic_config crystal_cove_pmic;
+
+#endif	/* __INTEL_SOC_PMIC_CORE_H__ */
diff --git a/include/linux/mfd/intel_soc_pmic.h b/include/linux/mfd/intel_soc_pmic.h
new file mode 100644
index 0000000..01270e4
--- /dev/null
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -0,0 +1,27 @@
+/*
+ * intel_soc_pmic.h - Intel SoC PMIC Driver
+ *
+ * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
+ *
+ * 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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#ifndef __INTEL_SOC_PMIC_H__
+#define __INTEL_SOC_PMIC_H__
+
+int intel_soc_pmic_readb(int reg);
+int intel_soc_pmic_writeb(int reg, u8 val);
+int intel_soc_pmic_setb(int reg, u8 mask);
+int intel_soc_pmic_clearb(int reg, u8 mask);
+int intel_soc_pmic_update(int reg, u8 val, u8 mask);
+
+#endif	/* __INTEL_SOC_PMIC_H__ */
-- 
1.8.3.2

[PATCH v3 2/4] mfd: intel_soc_pmic: I2C interface

[Zhu, Lejun]

The Intel SoC PMIC devices are connected to the CPU via the I2C interface. This patch provides support of the related I2C operations.

v2:
- Use regmap instead of creating our own I2C read/write callbacks.
v3:
- Use gpiod interface instead of gpio numbers.
- Remove redundant I2C IDs.
- Use managed allocations.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_i2c.c | 150 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)
 create mode 100644 drivers/mfd/intel_soc_pmic_i2c.c

diff --git a/drivers/mfd/intel_soc_pmic_i2c.c b/drivers/mfd/intel_soc_pmic_i2c.c
new file mode 100644
index 0000000..3b107d7
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_i2c.c
@@ -0,0 +1,150 @@
+/*
+ * intel_soc_pmic_i2c.c - Intel SoC PMIC MFD Driver
+ *
+ * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
+ *
+ * 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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/mfd/core.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/acpi.h>
+#include <linux/version.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include "intel_soc_pmic_core.h"
+
+static struct intel_soc_pmic *pmic_i2c;
+
+static void pmic_shutdown(struct i2c_client *client)
+{
+	disable_irq(pmic_i2c->irq);
+	return;
+}
+
+static int pmic_suspend(struct device *dev)
+{
+	disable_irq(pmic_i2c->irq);
+	return 0;
+}
+
+static int pmic_resume(struct device *dev)
+{
+	enable_irq(pmic_i2c->irq);
+	return 0;
+}
+
+static const struct dev_pm_ops pmic_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pmic_suspend, pmic_resume)
+};
+
+static int pmic_i2c_probe(struct i2c_client *i2c,
+			  const struct i2c_device_id *id)
+{
+	struct gpio_desc *desc;
+	struct device *dev = &i2c->dev;
+	int irq = i2c->irq;
+
+	if (pmic_i2c != NULL) {
+		dev_err(dev, "PMIC already exists.\n");
+		return -EBUSY;
+	}
+
+	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
+	if (IS_ERR(desc)) {
+		dev_dbg(dev, "Not using GPIO as interrupt.\n");
+	} else {
+		irq = gpiod_to_irq(desc);
+		if (irq < 0) {
+			dev_warn(dev, "Can't get irq: %d\n", irq);
+			irq = i2c->irq;
+		}
+	}
+
+	pmic_i2c = devm_kzalloc(dev, sizeof(*pmic_i2c), GFP_KERNEL);
+	pmic_i2c->config = (struct intel_soc_pmic_config *)id->driver_data;
+	pmic_i2c->dev = dev;
+	pmic_i2c->regmap = devm_regmap_init_i2c(i2c,
+						pmic_i2c->config->regmap_cfg);
+	pmic_i2c->irq = irq;
+
+	return intel_pmic_add(pmic_i2c);
+}
+
+static int pmic_i2c_remove(struct i2c_client *i2c)
+{
+	int ret;
+	
+	if (pmic_i2c == NULL)
+		return -ENODEV;
+
+	ret = intel_pmic_remove(pmic_i2c);
+
+	pmic_i2c = NULL;
+
+	return ret;
+}
+
+static const struct i2c_device_id pmic_i2c_id[] = {
+	{ "INT33FD:00", (kernel_ulong_t)&crystal_cove_pmic},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pmic_i2c_id);
+
+static struct acpi_device_id pmic_acpi_match[] = {
+	{ "INT33FD", (kernel_ulong_t)&crystal_cove_pmic},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, pmic_acpi_match);
+
+static struct i2c_driver pmic_i2c_driver = {
+	.driver = {
+		.name = "intel_soc_pmic_i2c",
+		.owner = THIS_MODULE,
+		.pm = &pmic_pm_ops,
+		.acpi_match_table = ACPI_PTR(pmic_acpi_match),
+	},
+	.probe = pmic_i2c_probe,
+	.remove = pmic_i2c_remove,
+	.id_table = pmic_i2c_id,
+	.shutdown = pmic_shutdown,
+};
+
+static int __init pmic_i2c_init(void)
+{
+	int ret;
+
+	ret = i2c_add_driver(&pmic_i2c_driver);
+	if (ret != 0)
+		pr_err("Failed to register pmic I2C driver: %d\n", ret);
+
+	return ret;
+}
+module_init(pmic_i2c_init);
+
+static void __exit pmic_i2c_exit(void)
+{
+	i2c_del_driver(&pmic_i2c_driver);
+}
+module_exit(pmic_i2c_exit);
+
+MODULE_DESCRIPTION("I2C driver for Intel SoC PMIC");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
-- 
1.8.3.2

[PATCH v3 3/4] mfd: intel_soc_pmic: Crystal Cove support

[Zhu, Lejun]

Crystal Cove is the PMIC in Baytrail-T platform. This patch provides chip-specific support for Crystal Cove.

v2:
- Add regmap_config for Crystal Cove.
v3:
- Convert IRQ config to regmap_irq_chip.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_crc.c | 175 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)
 create mode 100644 drivers/mfd/intel_soc_pmic_crc.c

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
new file mode 100644
index 0000000..341ab02
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -0,0 +1,175 @@
+/*
+ * intel_soc_pmic_crc.c - Device access for Crystal Cove PMIC
+ *
+ * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
+ *
+ * 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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/mfd/core.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/acpi.h>
+#include <linux/version.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include "intel_soc_pmic_core.h"
+
+#define	CHIP_NAME			"Crystal Cove"
+#define CRYSTAL_COVE_MAX_REGISTER	0xC6
+
+#define CHIPID		0x00
+#define CHIPVER		0x01
+#define IRQLVL1		0x02
+#define MIRQLVL1	0x0E
+
+enum {
+	PWRSRC_IRQ = 0,
+	THRM_IRQ,
+	BCU_IRQ,
+	ADC_IRQ,
+	CHGR_IRQ,
+	GPIO_IRQ,
+	VHDMIOCP_IRQ
+};
+
+static struct resource gpio_resources[] = {
+	{
+		.name	= "GPIO",
+		.start	= GPIO_IRQ,
+		.end	= GPIO_IRQ,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct resource pwrsrc_resources[] = {
+	{
+		.name  = "PWRSRC",
+		.start = PWRSRC_IRQ,
+		.end   = PWRSRC_IRQ,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+static struct resource adc_resources[] = {
+	{
+		.name  = "ADC",
+		.start = ADC_IRQ,
+		.end   = ADC_IRQ,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+static struct resource thermal_resources[] = {
+	{
+		.name  = "THERMAL",
+		.start = THRM_IRQ,
+		.end   = THRM_IRQ,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+static struct resource bcu_resources[] = {
+	{
+		.name  = "BCU",
+		.start = BCU_IRQ,
+		.end   = BCU_IRQ,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+static struct mfd_cell crystal_cove_dev[] = {
+	{
+		.name = "crystal_cove_pwrsrc",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(pwrsrc_resources),
+		.resources = pwrsrc_resources,
+	},
+	{
+		.name = "crystal_cove_adc",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(adc_resources),
+		.resources = adc_resources,
+	},
+	{
+		.name = "crystal_cove_thermal",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(thermal_resources),
+		.resources = thermal_resources,
+	},
+	{
+		.name = "crystal_cove_bcu",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(bcu_resources),
+		.resources = bcu_resources,
+	},
+	{
+		.name = "crystal_cove_gpio",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(gpio_resources),
+		.resources = gpio_resources,
+	},
+};
+
+static struct regmap_config crystal_cove_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = CRYSTAL_COVE_MAX_REGISTER,
+	.cache_type = REGCACHE_NONE,
+};
+
+#define	CRC_REGMAP_IRQ_VALUE(irq)	{	\
+		.reg_offset = 0,		\
+		.mask = BIT(irq),		\
+	}
+
+static const struct regmap_irq crystal_cove_irqs[] = {
+	[PWRSRC_IRQ]	= CRC_REGMAP_IRQ_VALUE(PWRSRC_IRQ),
+	[THRM_IRQ]	= CRC_REGMAP_IRQ_VALUE(THRM_IRQ),
+	[BCU_IRQ]	= CRC_REGMAP_IRQ_VALUE(BCU_IRQ),
+	[ADC_IRQ]	= CRC_REGMAP_IRQ_VALUE(ADC_IRQ),
+	[CHGR_IRQ]	= CRC_REGMAP_IRQ_VALUE(CHGR_IRQ),
+	[GPIO_IRQ]	= CRC_REGMAP_IRQ_VALUE(GPIO_IRQ),
+	[VHDMIOCP_IRQ]	= CRC_REGMAP_IRQ_VALUE(VHDMIOCP_IRQ),
+};
+
+static struct regmap_irq_chip crystal_cove_irq_chip = {
+	.name = CHIP_NAME,
+	.irqs = crystal_cove_irqs,
+	.num_irqs = ARRAY_SIZE(crystal_cove_irqs),
+	.num_regs = 1,
+	.status_base = IRQLVL1,
+	.mask_base = MIRQLVL1,
+};
+
+static int crystal_cove_init(void)
+{
+	pr_debug("%s: ID 0x%02X, VERSION 0x%02X\n", CHIP_NAME,
+		 intel_soc_pmic_readb(CHIPID), intel_soc_pmic_readb(CHIPVER));
+	return 0;
+}
+
+struct intel_soc_pmic_config crystal_cove_pmic = {
+	.label		= CHIP_NAME,
+	.irq_flags	= IRQF_TRIGGER_RISING,
+	.init		= crystal_cove_init,
+	.cell_dev	= crystal_cove_dev,
+	.n_cell_devs	= ARRAY_SIZE(crystal_cove_dev),
+	.regmap_cfg	= &crystal_cove_regmap_config,
+	.irq_chip	= &crystal_cove_irq_chip,
+};
-- 
1.8.3.2

[PATCH v3 4/4] mfd: intel_soc_pmic: Build files

[Zhu, Lejun]

Devices based on Intel SoC products such as Baytrail have a Power Management IC. This patch adds Intel SoC PMIC support to the build files.

v2:
- Add select REGMAP_I2C.
v3:
- Add select REGMAP_IRQ.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
 drivers/mfd/Kconfig  | 12 ++++++++++++
 drivers/mfd/Makefile |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3383412..d987b71 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -241,6 +241,18 @@ config LPC_SCH
 	  LPC bridge function of the Intel SCH provides support for
 	  System Management Bus and General Purpose I/O.
 
+config INTEL_SOC_PMIC
+	bool "Support for Intel Atom SoC PMIC"
+	depends on I2C=y
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	help
+	  Select this option to enable support for the PMIC device
+	  on some Intel SoC systems. The PMIC provides ADC, GPIO,
+	  thermal, charger and related power management functions
+	  on these systems.
+
 config MFD_INTEL_MSIC
 	bool "Intel MSIC"
 	depends on INTEL_SCU_IPC
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 2851275..1a18d8b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -166,3 +166,6 @@ obj-$(CONFIG_MFD_RETU)		+= retu-mfd.o
 obj-$(CONFIG_MFD_AS3711)	+= as3711.o
 obj-$(CONFIG_MFD_AS3722)	+= as3722.o
 obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
+
+intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o intel_soc_pmic_i2c.o
+obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
-- 
1.8.3.2

Re: [PATCH v3 1/4] mfd: intel_soc_pmic: Core driver

[Lee Jones]

> This patch provides the common code for the intel_soc_pmic MFD driver, such as read/write register and set up IRQ.
> 
> v2:
> - Use regmap instead of our own callbacks for read/write.
> - Add one missing EXPORT_SYMBOL.
> - Remove some duplicate code and put them into pmic_regmap_load_from_hw.
> v3:
> - Use regmap-irq. Remove our own pmic_regmap_* and IRQ handling code.
> - Remove intel_soc_pmic_dev() because gpio driver no longer uses it.
> - Remove intel_soc_pmic_set_pdata() because currently it's not used.
> - Use EXPORT_SYMBOL_GPL for exposed APIs.
> 
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
>  drivers/mfd/intel_soc_pmic_core.c  | 212 +++++++++++++++++++++++++++++++++++++
>  drivers/mfd/intel_soc_pmic_core.h  |  44 ++++++++
>  include/linux/mfd/intel_soc_pmic.h |  27 +++++
>  3 files changed, 283 insertions(+)
>  create mode 100644 drivers/mfd/intel_soc_pmic_core.c
>  create mode 100644 drivers/mfd/intel_soc_pmic_core.h
>  create mode 100644 include/linux/mfd/intel_soc_pmic.h
> 
> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
> new file mode 100644
> index 0000000..4f95a4a
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_core.c
> @@ -0,0 +1,212 @@
> +/*
> + * intel_soc_pmic_core.c - Intel SoC PMIC Core Functions
> + *
> + * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
> + *
> + * 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 in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Author: Yang, Bin <bin.yang@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/mfd/core.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/acpi.h>
> +#include <linux/version.h>
> +#include <linux/gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include "intel_soc_pmic_core.h"
> +
> +static DEFINE_MUTEX(pmic_lock);	/* protect pmic */
> +static struct intel_soc_pmic *pmic;
> +
> +/*
> + * Read from a PMIC register
> + */
> +int intel_soc_pmic_readb(int reg)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	mutex_lock(&pmic_lock);
> +
> +	if (!pmic) {
> +		ret = -EIO;
> +	} else {
> +		ret = regmap_read(pmic->regmap, reg, &val);
> +		if (!ret)
> +			ret = val;
> +	}
> +
> +	mutex_unlock(&pmic_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_soc_pmic_readb);
> +
> +/*
> + * Write to a PMIC register
> + */
> +int intel_soc_pmic_writeb(int reg, u8 val)
> +{
> +	int ret;
> +
> +	mutex_lock(&pmic_lock);
> +
> +	if (!pmic)
> +		ret = -EIO;
> +	else
> +		ret = regmap_write(pmic->regmap, reg, val);
> +
> +	mutex_unlock(&pmic_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_soc_pmic_writeb);
> +
> +/*
> + * Set 1 bit in a PMIC register
> + */
> +int intel_soc_pmic_setb(int reg, u8 mask)
> +{
> +	int ret;
> +
> +	mutex_lock(&pmic_lock);
> +
> +	if (!pmic)
> +		ret = -EIO;
> +	else
> +		ret = regmap_update_bits(pmic->regmap, reg, mask, mask);
> +
> +	mutex_unlock(&pmic_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_soc_pmic_setb);
> +
> +/*
> + * Clear 1 bit in a PMIC register
> + */
> +int intel_soc_pmic_clearb(int reg, u8 mask)
> +{
> +	int ret;
> +
> +	mutex_lock(&pmic_lock);
> +
> +	if (!pmic)
> +		ret = -EIO;
> +	else
> +		ret = regmap_update_bits(pmic->regmap, reg, mask, 0);
> +
> +	mutex_unlock(&pmic_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_soc_pmic_clearb);
> +
> +/*
> +* Set and clear multiple bits of a PMIC register
> +*/
> +int intel_soc_pmic_update(int reg, u8 val, u8 mask)
> +{
> +	int ret;
> +	
> +	mutex_lock(&pmic_lock);
> +	
> +	if (!pmic)
> +		ret = -EIO;
> +	else
> +		ret = regmap_update_bits(pmic->regmap, reg, mask, val);
> +	
> +	mutex_unlock(&pmic_lock);
> +	
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_soc_pmic_update);

I'm really not a fan of all these pointless agregation call-backs.  I
see them as unesersary overhead.  Just use the regmap API directly.

> +int intel_pmic_add(struct intel_soc_pmic *chip)
> +{
> +	int ret;
> +	struct intel_soc_pmic_config *cfg = chip->config;
> +
> +	mutex_lock(&pmic_lock);
> +
> +	if (pmic != NULL) {

if (pmic)

> +		mutex_unlock(&pmic_lock);
> +		return -EBUSY;
> +	}
> +
> +	pmic = chip;
> +
> +	mutex_unlock(&pmic_lock);
> +
> +	if (cfg->init) {
> +		ret = cfg->init();

All this does is invokes a dev_info() message.  Until this does
something useful, please remove it.

> +		if (ret != 0)

if (ret)

Same with all "!= 0" checks.

> +			goto err;
> +	}
> +
> +	ret = regmap_add_irq_chip(chip->regmap, chip->irq,
> +				  cfg->irq_flags | IRQF_ONESHOT,
> +				  0, cfg->irq_chip,
> +				  &chip->irq_chip_data);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = enable_irq_wake(chip->irq);
> +	if (ret != 0)
> +		dev_warn(chip->dev, "Can't enable IRQ as wake source: %d\n",
> +			 ret);
> +
> +	ret = mfd_add_devices(chip->dev, -1, cfg->cell_dev,
> +			       cfg->n_cell_devs, NULL, 0,
> +			       regmap_irq_get_domain(chip->irq_chip_data));
> +

Remove this line.

> +	if (ret)
> +		goto err_del_irq_chip;
> +
> +	return 0;
> +
> +err_del_irq_chip:
> +	regmap_del_irq_chip(chip->irq, chip->irq_chip_data);
> +err:
> +	mutex_lock(&pmic_lock);
> +	if (pmic == chip)
> +		pmic = NULL;
> +	mutex_unlock(&pmic_lock);

I don't think this locking is required here, and won't pmic == chip
always be true?

> +	return ret;
> +}
> +
> +int intel_pmic_remove(struct intel_soc_pmic *chip)
> +{
> +	mutex_lock(&pmic_lock);
> +
> +	if (pmic != chip) {
> +		mutex_unlock(&pmic_lock);
> +		return -ENODEV;
> +	}
> +
> +	pmic = NULL;
> +
> +	mutex_unlock(&pmic_lock);

I'm not sure the locks are required here either.

> +	regmap_del_irq_chip(chip->irq, chip->irq_chip_data);
> +
> +	mfd_remove_devices(chip->dev);
> +
> +	return 0;
> +}

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/4] mfd: intel_soc_pmic: I2C interface

[Lee Jones]

> The Intel SoC PMIC devices are connected to the CPU via the I2C interface. This patch provides support of the related I2C operations.

Please only use 72 chars for your commit log.

> v2:
> - Use regmap instead of creating our own I2C read/write callbacks.
> v3:
> - Use gpiod interface instead of gpio numbers.
> - Remove redundant I2C IDs.
> - Use managed allocations.

This shouldn't be in the commit log.

> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---

The change log information above should really be below the "---"
above.

Then we usually put another "---" below.

>  drivers/mfd/intel_soc_pmic_i2c.c | 150 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 drivers/mfd/intel_soc_pmic_i2c.c
> 
> diff --git a/drivers/mfd/intel_soc_pmic_i2c.c b/drivers/mfd/intel_soc_pmic_i2c.c
> new file mode 100644
> index 0000000..3b107d7
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_i2c.c
> @@ -0,0 +1,150 @@
> +/*
> + * intel_soc_pmic_i2c.c - Intel SoC PMIC MFD Driver
> + *
> + * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
> + *
> + * 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 in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Author: Yang, Bin <bin.yang@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/core.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/acpi.h>
> +#include <linux/version.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include "intel_soc_pmic_core.h"

Please review these, as they are not all in use.

> +static struct intel_soc_pmic *pmic_i2c;

I'm not really comfortable seeing global structures such as these.
Tuck it away in a driver data structure somewhere.

> +static void pmic_shutdown(struct i2c_client *client)
> +{
> +	disable_irq(pmic_i2c->irq);
> +	return;
> +}
> +
> +static int pmic_suspend(struct device *dev)
> +{
> +	disable_irq(pmic_i2c->irq);
> +	return 0;
> +}
> +
> +static int pmic_resume(struct device *dev)
> +{
> +	enable_irq(pmic_i2c->irq);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops pmic_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pmic_suspend, pmic_resume)
> +};

Place all of these below remove().

Use SIMPLE_DEV_PM_OPS instead of SET_SYSTEM_SLEEP_PM_OPS.

> +static int pmic_i2c_probe(struct i2c_client *i2c,

pmic_* is very generic.  Even intel_pmic_* is generic.  Are Intel only
ever going to make PMICs which can be supported by this driver?

> +			  const struct i2c_device_id *id)
> +{
> +	struct gpio_desc *desc;
> +	struct device *dev = &i2c->dev;
> +	int irq = i2c->irq;
> +
> +	if (pmic_i2c != NULL) {

if (pmic_i2c)

> +		dev_err(dev, "PMIC already exists.\n");
> +		return -EBUSY;
> +	}
> +
> +	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
> +	if (IS_ERR(desc)) {
> +		dev_dbg(dev, "Not using GPIO as interrupt.\n");
> +	} else {
> +		irq = gpiod_to_irq(desc);
> +		if (irq < 0) {
> +			dev_warn(dev, "Can't get irq: %d\n", irq);
> +			irq = i2c->irq;
> +		}
> +	}
> +
> +	pmic_i2c = devm_kzalloc(dev, sizeof(*pmic_i2c), GFP_KERNEL);
> +	pmic_i2c->config = (struct intel_soc_pmic_config *)id->driver_data;
> +	pmic_i2c->dev = dev;
> +	pmic_i2c->regmap = devm_regmap_init_i2c(i2c,
> +						pmic_i2c->config->regmap_cfg);

Neater if you break the line at the '='.

> +	pmic_i2c->irq = irq;
> +
> +	return intel_pmic_add(pmic_i2c);
> +}
> +
> +static int pmic_i2c_remove(struct i2c_client *i2c)
> +{
> +	int ret;
> +	
> +	if (pmic_i2c == NULL)

if (!pmic_i2c)

> +		return -ENODEV;
> +
> +	ret = intel_pmic_remove(pmic_i2c);
> +
> +	pmic_i2c = NULL;
> +
> +	return ret;
> +}
> +
> +static const struct i2c_device_id pmic_i2c_id[] = {
> +	{ "INT33FD:00", (kernel_ulong_t)&crystal_cove_pmic},

This name looks odd to me.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pmic_i2c_id);
> +
> +static struct acpi_device_id pmic_acpi_match[] = {
> +	{ "INT33FD", (kernel_ulong_t)&crystal_cove_pmic},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, pmic_acpi_match);
> +
> +static struct i2c_driver pmic_i2c_driver = {
> +	.driver = {
> +		.name = "intel_soc_pmic_i2c",
> +		.owner = THIS_MODULE,
> +		.pm = &pmic_pm_ops,
> +		.acpi_match_table = ACPI_PTR(pmic_acpi_match),
> +	},
> +	.probe = pmic_i2c_probe,
> +	.remove = pmic_i2c_remove,
> +	.id_table = pmic_i2c_id,
> +	.shutdown = pmic_shutdown,
> +};

Remove from here -------->

> +static int __init pmic_i2c_init(void)
> +{
> +	int ret;
> +
> +	ret = i2c_add_driver(&pmic_i2c_driver);
> +	if (ret != 0)
> +		pr_err("Failed to register pmic I2C driver: %d\n", ret);
> +
> +	return ret;
> +}
> +module_init(pmic_i2c_init);
> +
> +static void __exit pmic_i2c_exit(void)
> +{
> +	i2c_del_driver(&pmic_i2c_driver);
> +}
> +module_exit(pmic_i2c_exit);

To here --------->

And replace with:

module_i2c_driver(pmic_i2c_driver);

> +MODULE_DESCRIPTION("I2C driver for Intel SoC PMIC");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/4] mfd: intel_soc_pmic: Crystal Cove support

[Lee Jones]

On Tue, 27 May 2014, Zhu, Lejun wrote:

> Crystal Cove is the PMIC in Baytrail-T platform. This patch provides chip-specific support for Crystal Cove.
> 
> v2:
> - Add regmap_config for Crystal Cove.
> v3:
> - Convert IRQ config to regmap_irq_chip.

Same comments about the commit log as before.

> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
>  drivers/mfd/intel_soc_pmic_crc.c | 175 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644 drivers/mfd/intel_soc_pmic_crc.c
> 
> diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
> new file mode 100644
> index 0000000..341ab02
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_crc.c
> @@ -0,0 +1,175 @@
> +/*
> + * intel_soc_pmic_crc.c - Device access for Crystal Cove PMIC
> + *
> + * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
> + *
> + * 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 in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Author: Yang, Bin <bin.yang@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/core.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/acpi.h>
> +#include <linux/version.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include "intel_soc_pmic_core.h"

Please review these.

> +#define	CHIP_NAME			"Crystal Cove"

Don't do this.  Just use the name in the correct places.

> +#define CRYSTAL_COVE_MAX_REGISTER	0xC6
> +
> +#define CHIPID		0x00
> +#define CHIPVER		0x01
> +#define IRQLVL1		0x02
> +#define MIRQLVL1	0x0E
> +
> +enum {
> +	PWRSRC_IRQ = 0,
> +	THRM_IRQ,
> +	BCU_IRQ,
> +	ADC_IRQ,
> +	CHGR_IRQ,
> +	GPIO_IRQ,
> +	VHDMIOCP_IRQ
> +};
> +
> +static struct resource gpio_resources[] = {
> +	{
> +		.name	= "GPIO",
> +		.start	= GPIO_IRQ,
> +		.end	= GPIO_IRQ,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct resource pwrsrc_resources[] = {
> +	{
> +		.name  = "PWRSRC",
> +		.start = PWRSRC_IRQ,
> +		.end   = PWRSRC_IRQ,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct resource adc_resources[] = {
> +	{
> +		.name  = "ADC",
> +		.start = ADC_IRQ,
> +		.end   = ADC_IRQ,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct resource thermal_resources[] = {
> +	{
> +		.name  = "THERMAL",
> +		.start = THRM_IRQ,
> +		.end   = THRM_IRQ,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct resource bcu_resources[] = {
> +	{
> +		.name  = "BCU",
> +		.start = BCU_IRQ,
> +		.end   = BCU_IRQ,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct mfd_cell crystal_cove_dev[] = {
> +	{
> +		.name = "crystal_cove_pwrsrc",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(pwrsrc_resources),
> +		.resources = pwrsrc_resources,
> +	},
> +	{
> +		.name = "crystal_cove_adc",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +		.resources = adc_resources,
> +	},
> +	{
> +		.name = "crystal_cove_thermal",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(thermal_resources),
> +		.resources = thermal_resources,
> +	},
> +	{
> +		.name = "crystal_cove_bcu",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(bcu_resources),
> +		.resources = bcu_resources,
> +	},
> +	{
> +		.name = "crystal_cove_gpio",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(gpio_resources),
> +		.resources = gpio_resources,
> +	},
> +};

You can remove the id field.

> +static struct regmap_config crystal_cove_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = CRYSTAL_COVE_MAX_REGISTER,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +#define	CRC_REGMAP_IRQ_VALUE(irq)	{	\
> +		.reg_offset = 0,		\
> +		.mask = BIT(irq),		\
> +	}

Can you sort out the whitespace here?

> +static const struct regmap_irq crystal_cove_irqs[] = {
> +	[PWRSRC_IRQ]	= CRC_REGMAP_IRQ_VALUE(PWRSRC_IRQ),
> +	[THRM_IRQ]	= CRC_REGMAP_IRQ_VALUE(THRM_IRQ),
> +	[BCU_IRQ]	= CRC_REGMAP_IRQ_VALUE(BCU_IRQ),
> +	[ADC_IRQ]	= CRC_REGMAP_IRQ_VALUE(ADC_IRQ),
> +	[CHGR_IRQ]	= CRC_REGMAP_IRQ_VALUE(CHGR_IRQ),
> +	[GPIO_IRQ]	= CRC_REGMAP_IRQ_VALUE(GPIO_IRQ),
> +	[VHDMIOCP_IRQ]	= CRC_REGMAP_IRQ_VALUE(VHDMIOCP_IRQ),
> +};
> +
> +static struct regmap_irq_chip crystal_cove_irq_chip = {
> +	.name = CHIP_NAME,
> +	.irqs = crystal_cove_irqs,
> +	.num_irqs = ARRAY_SIZE(crystal_cove_irqs),
> +	.num_regs = 1,
> +	.status_base = IRQLVL1,
> +	.mask_base = MIRQLVL1,
> +};
> +
> +static int crystal_cove_init(void)
> +{
> +	pr_debug("%s: ID 0x%02X, VERSION 0x%02X\n", CHIP_NAME,
> +		 intel_soc_pmic_readb(CHIPID), intel_soc_pmic_readb(CHIPVER));
> +	return 0;
> +}

This seems pointless.

> +struct intel_soc_pmic_config crystal_cove_pmic = {
> +	.label		= CHIP_NAME,

Where is this used?

> +	.irq_flags	= IRQF_TRIGGER_RISING,
> +	.init		= crystal_cove_init,
> +	.cell_dev	= crystal_cove_dev,
> +	.n_cell_devs	= ARRAY_SIZE(crystal_cove_dev),
> +	.regmap_cfg	= &crystal_cove_regmap_config,
> +	.irq_chip	= &crystal_cove_irq_chip,
> +};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 4/4] mfd: intel_soc_pmic: Build files

[Lee Jones]

> Devices based on Intel SoC products such as Baytrail have a Power Management IC. This patch adds Intel SoC PMIC support to the build files.
> 
> v2:
> - Add select REGMAP_I2C.
> v3:
> - Add select REGMAP_IRQ.

Please fix this, same as before.

> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
>  drivers/mfd/Kconfig  | 12 ++++++++++++
>  drivers/mfd/Makefile |  3 +++
>  2 files changed, 15 insertions(+)

Patch looks okay:

  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 1/4] mfd: intel_soc_pmic: Core driver

[Zhu, Lejun]

On 5/27/2014 11:35 PM, Lee Jones wrote:
>> This patch provides the common code for the intel_soc_pmic MFD driver, such as read/write register and set up IRQ.
(...)
>> +/*
>> +* Set and clear multiple bits of a PMIC register
>> +*/
>> +int intel_soc_pmic_update(int reg, u8 val, u8 mask)
>> +{
>> +	int ret;
>> +	
>> +	mutex_lock(&pmic_lock);
>> +	
>> +	if (!pmic)
>> +		ret = -EIO;
>> +	else
>> +		ret = regmap_update_bits(pmic->regmap, reg, mask, val);
>> +	
>> +	mutex_unlock(&pmic_lock);
>> +	
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_soc_pmic_update);
> 
> I'm really not a fan of all these pointless agregation call-backs.  I
> see them as unesersary overhead.  Just use the regmap API directly.

OK. I'll remove these wrappers from the MFD driver, seems no one likes
them...

I'll fix the patch set as you suggested and resubmit. Thank you for
reviewing this.

Best Regards
Lejun