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

[PATCH RESEND v2 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.

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                |  11 +
 drivers/mfd/Makefile               |   3 +
 drivers/mfd/intel_soc_pmic_core.c  | 521 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_core.h  |  83 ++++++
 drivers/mfd/intel_soc_pmic_crc.c   | 165 ++++++++++++
 drivers/mfd/intel_soc_pmic_i2c.c   | 148 +++++++++++
 include/linux/mfd/intel_soc_pmic.h |  29 +++
 7 files changed, 960 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 RESEND v2 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.

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  | 521 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_core.h  |  83 ++++++
 include/linux/mfd/intel_soc_pmic.h |  29 +++
 3 files changed, 633 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..76d366e
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -0,0 +1,521 @@
+/*
+ * 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"
+
+struct cell_dev_pdata {
+	struct list_head	list;
+	const char		*name;
+	void			*data;
+	int			len;
+};
+static LIST_HEAD(pdata_list);
+
+static DEFINE_MUTEX(pmic_lock);	/* protect the following variables */
+static struct intel_soc_pmic *pmic;
+static int cache_offset = -1;
+static unsigned int cache_read_val;
+static unsigned int cache_write_val;
+static int cache_write_pending;
+static int cache_flags;
+
+struct device *intel_soc_pmic_dev(void)
+{
+	return pmic->dev;
+}
+EXPORT_SYMBOL(intel_soc_pmic_dev);
+
+/*
+ * 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(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(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(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(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(intel_soc_pmic_update);
+
+/*
+ * Set platform_data of the child devices. Needs to be called before
+ * the MFD device is added.
+ */
+int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
+{
+	struct cell_dev_pdata *pdata;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		pr_err("intel_pmic: can't set pdata!\n");
+		return -ENOMEM;
+	}
+
+	pdata->name = name;
+	pdata->data = data;
+	pdata->len = len;
+
+	list_add_tail(&pdata->list, &pdata_list);
+
+	return 0;
+}
+EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
+
+static void __pmic_regmap_flush(void)
+{
+	if (cache_write_pending)
+		WARN_ON(regmap_write(pmic->regmap, cache_offset,
+				     cache_write_val));
+	cache_offset = -1;
+	cache_write_pending = 0;
+}
+
+static void pmic_regmap_flush(void)
+{
+	mutex_lock(&pmic_lock);
+
+	if (pmic)
+		__pmic_regmap_flush();
+
+	mutex_unlock(&pmic_lock);
+}
+
+static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
+{
+	int ret;
+
+	if (cache_offset == map->offset) {
+		if (cache_flags != map->flags) {
+			dev_warn(pmic->dev, "Same reg with diff flags\n");
+			__pmic_regmap_flush();
+		}
+	}
+
+	if (cache_offset != map->offset) {
+		__pmic_regmap_flush();
+		if (IS_PMIC_REG_WO(map) || IS_PMIC_REG_W1C(map)) {
+			cache_write_val = 0;
+			ret = regmap_read(pmic->regmap, map->offset,
+					  &cache_read_val);
+		} else {
+			ret = regmap_read(pmic->regmap, map->offset,
+					  &cache_read_val);
+			cache_write_val = cache_read_val;
+		}
+		if (ret) {
+			dev_err(pmic->dev, "Register access error\n");
+			return ret;
+		}
+		cache_offset = map->offset;
+		cache_flags = map->flags;
+	}
+
+	return 0;
+}
+
+static int pmic_regmap_write(struct intel_pmic_regmap *map, int val)
+{
+	int ret = 0;
+
+	if (!IS_PMIC_REG_VALID(map))
+		return -ENXIO;
+
+	if (IS_PMIC_REG_INV(map))
+		val = ~val;
+
+	mutex_lock(&pmic_lock);
+
+	if (!pmic) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	ret = pmic_regmap_load_from_hw(map);
+	if (ret)
+		goto err;
+
+	val = ((val & map->mask) << map->shift);
+	cache_write_val &= ~(map->mask << map->shift);
+	cache_write_val |= val;
+	cache_write_pending = 1;
+
+	if (!IS_PMIC_REG_WO(map) && !IS_PMIC_REG_W1C(map))
+		cache_read_val = cache_write_val;
+
+err:
+	dev_dbg(pmic->dev, "offset=%x, shift=%x, mask=%x, flags=%x\n",
+		map->offset, map->shift, map->mask, map->flags);
+	dev_dbg(pmic->dev, "cache_read=%x, cache_write=%x, ret=%x\n",
+		cache_read_val, cache_write_val, ret);
+
+	mutex_unlock(&pmic_lock);
+
+	return ret;
+}
+
+static int pmic_regmap_read(struct intel_pmic_regmap *map)
+{
+	int ret = 0;
+
+	if (!IS_PMIC_REG_VALID(map))
+		return -ENXIO;
+
+	mutex_lock(&pmic_lock);
+
+	if (!pmic) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	ret = pmic_regmap_load_from_hw(map);
+	if (ret)
+		goto err;
+
+	if (IS_PMIC_REG_INV(map))
+		ret = ~cache_read_val;
+	else
+		ret = cache_read_val;
+
+	ret = (ret >> map->shift) & map->mask;
+	if (!IS_PMIC_REG_WO(map) && !IS_PMIC_REG_W1C(map))
+		cache_write_val = cache_read_val;
+
+err:
+	dev_dbg(pmic->dev, "offset=%x, shift=%x, mask=%x, flags=%x\n",
+		map->offset, map->shift, map->mask, map->flags);
+	dev_dbg(pmic->dev, "cache_read=%x, cache_write=%x, ret=%x\n",
+		cache_read_val, cache_write_val, ret);
+
+	mutex_unlock(&pmic_lock);
+
+	return ret;
+}
+
+static void pmic_irq_enable(struct irq_data *data)
+{
+	clear_bit(PMIC_IRQ_MASK_BIT(data->irq - pmic->irq_base),
+		  &PMIC_IRQ_MASK(pmic, data->irq - pmic->irq_base));
+	pmic->irq_need_update = 1;
+}
+
+static void pmic_irq_disable(struct irq_data *data)
+{
+	set_bit(PMIC_IRQ_MASK_BIT(data->irq - pmic->irq_base),
+		&PMIC_IRQ_MASK(pmic, data->irq - pmic->irq_base));
+	pmic->irq_need_update = 1;
+}
+
+static void pmic_irq_sync_unlock(struct irq_data *data)
+{
+	int val, irq_offset;
+	if (pmic->irq_need_update) {
+		irq_offset = data->irq - pmic->irq_base;
+		val = !!test_bit(PMIC_IRQ_MASK_BIT(irq_offset),
+				 &PMIC_IRQ_MASK(pmic, irq_offset));
+		pmic_regmap_write(&pmic->irq_regmap[irq_offset].mask, val);
+		pmic->irq_need_update = 0;
+		pmic_regmap_flush();
+	}
+	mutex_unlock(&pmic->irq_lock);
+}
+
+static void pmic_irq_lock(struct irq_data *data)
+{
+	mutex_lock(&pmic->irq_lock);
+}
+
+static irqreturn_t pmic_irq_isr(int irq, void *data)
+{
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t pmic_irq_thread(int irq, void *data)
+{
+	int i;
+
+	mutex_lock(&pmic->irq_lock);
+
+	for (i = 0; i < pmic->irq_num; i++) {
+		if (test_bit(PMIC_IRQ_MASK_BIT(i), &PMIC_IRQ_MASK(pmic, i)))
+			continue;
+
+		if (pmic_regmap_read(&pmic->irq_regmap[i].status)) {
+			pmic_regmap_write(&pmic->irq_regmap[i].ack, 1);
+			handle_nested_irq(pmic->irq_base + i);
+		}
+	}
+
+	pmic_regmap_flush();
+
+	mutex_unlock(&pmic->irq_lock);
+
+	return IRQ_HANDLED;
+}
+
+static struct irq_chip pmic_irq_chip = {
+	.name			= "intel_soc_pmic",
+	.irq_bus_lock		= pmic_irq_lock,
+	.irq_bus_sync_unlock	= pmic_irq_sync_unlock,
+	.irq_disable		= pmic_irq_disable,
+	.irq_enable		= pmic_irq_enable,
+};
+
+static int pmic_irq_init(void)
+{
+	int cur_irq;
+	int ret;
+	int i;
+
+	for (i = 0; i < pmic->irq_num; i++) {
+		pmic_regmap_write(&pmic->irq_regmap[i].mask, 1);
+		set_bit(PMIC_IRQ_MASK_BIT(i), &PMIC_IRQ_MASK(pmic, i));
+	}
+
+	for (i = 0; i < pmic->irq_num; i++)
+		pmic_regmap_write(&pmic->irq_regmap[i].ack, 1);
+
+	pmic_regmap_flush();
+
+	pmic->irq_base = irq_alloc_descs(-1, 0, pmic->irq_num, 0);
+	if (pmic->irq_base < 0) {
+		dev_warn(pmic->dev, "Failed to allocate IRQs: %d\n",
+			 pmic->irq_base);
+		pmic->irq_base = 0;
+		ret = -EINVAL;
+		goto out;
+	} else {
+		dev_dbg(pmic->dev, "PMIC IRQ Base:%d\n", pmic->irq_base);
+	}
+
+	for (cur_irq = pmic->irq_base;
+	     cur_irq < pmic->irq_num + pmic->irq_base;
+	     cur_irq++) {
+		irq_set_chip_data(cur_irq, pmic);
+		irq_set_chip_and_handler(cur_irq, &pmic_irq_chip,
+					 handle_edge_irq);
+		irq_set_nested_thread(cur_irq, 1);
+		irq_set_noprobe(cur_irq);
+	}
+
+	if (gpio_is_valid(pmic->pmic_int_gpio)) {
+		ret = gpio_request_one(pmic->pmic_int_gpio,
+				       GPIOF_DIR_IN, "PMIC Interupt");
+		if (ret) {
+			dev_err(pmic->dev, "Request PMIC_INT gpio error\n");
+			goto out_free_desc;
+		}
+
+		pmic->irq = gpio_to_irq(pmic->pmic_int_gpio);
+	}
+
+	ret = request_threaded_irq(pmic->irq, pmic_irq_isr, pmic_irq_thread,
+				   pmic->irq_flags, "intel_soc_pmic", pmic);
+	if (ret != 0) {
+		dev_err(pmic->dev, "Failed to request IRQ %d: %d\n",
+			pmic->irq, ret);
+		goto out_free_gpio;
+	}
+
+	ret = enable_irq_wake(pmic->irq);
+	if (ret != 0)
+		dev_warn(pmic->dev, "Can't enable IRQ as wake source: %d\n",
+			 ret);
+
+	return 0;
+
+out_free_gpio:
+	if (gpio_is_valid(pmic->pmic_int_gpio)) {
+		gpio_free(pmic->pmic_int_gpio);
+		pmic->irq = 0;
+	}
+out_free_desc:
+	irq_free_descs(pmic->irq_base, pmic->irq_num);
+out:
+	return ret;
+}
+
+int intel_pmic_add(struct intel_soc_pmic *chip)
+{
+	int i, ret;
+	struct cell_dev_pdata *pdata;
+
+	mutex_lock(&pmic_lock);
+
+	if (pmic != NULL) {
+		mutex_unlock(&pmic_lock);
+		return -EBUSY;
+	}
+
+	pmic = chip;
+
+	mutex_unlock(&pmic_lock);
+
+	mutex_init(&chip->irq_lock);
+
+	if (chip->init) {
+		ret = chip->init();
+		if (ret != 0)
+			goto err;
+	}
+
+	ret = pmic_irq_init();
+	if (ret != 0)
+		goto err;
+
+	for (i = 0; chip->cell_dev[i].name != NULL; i++) {
+		list_for_each_entry(pdata, &pdata_list, list) {
+			if (!strcmp(pdata->name, chip->cell_dev[i].name)) {
+				chip->cell_dev[i].platform_data = pdata->data;
+				chip->cell_dev[i].pdata_size = pdata->len;
+			}
+		}
+	}
+
+	return mfd_add_devices(chip->dev, -1, chip->cell_dev, i,
+			       NULL, chip->irq_base, NULL);
+
+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);
+
+	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..6b54962
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.h
@@ -0,0 +1,83 @@
+/*
+ * 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__
+
+#define INTEL_PMIC_IRQ_MAX	128
+#define INTEL_PMIC_REG_NULL	{-1,}
+
+#define INTEL_PMIC_REG_INV	BIT(0) /*value revert*/
+#define INTEL_PMIC_REG_WO	BIT(1) /*write only*/
+#define INTEL_PMIC_REG_RO	BIT(2) /*read only*/
+#define INTEL_PMIC_REG_W1C	BIT(3) /*write 1 clear*/
+#define INTEL_PMIC_REG_RC	BIT(4) /*read clear*/
+#define IS_PMIC_REG_INV(_map)	(_map->flags & INTEL_PMIC_REG_INV)
+#define IS_PMIC_REG_WO(_map)	(_map->flags & INTEL_PMIC_REG_WO)
+#define IS_PMIC_REG_RO(_map)	(_map->flags & INTEL_PMIC_REG_RO)
+#define IS_PMIC_REG_W1C(_map)	(_map->flags & INTEL_PMIC_REG_W1C)
+#define IS_PMIC_REG_RC(_map)	(_map->flags & INTEL_PMIC_REG_RC)
+#define IS_PMIC_REG_VALID(_map) \
+	((_map->mask != 0) && (_map->offset >= 0))
+
+#define PMIC_IRQREG_MASK	0
+#define PMIC_IRQREG_STATUS	1
+#define PMIC_IRQREG_ACK		2
+
+#define PMIC_IRQ_MASK_NBITS	32
+#define PMIC_IRQ_MASK_LEN	(INTEL_PMIC_IRQ_MAX / PMIC_IRQ_MASK_NBITS)
+#define PMIC_IRQ_MASK(pmic, offset)	\
+	(pmic->irq_mask[(offset) / PMIC_IRQ_MASK_NBITS])
+#define PMIC_IRQ_MASK_BIT(offset)	((offset) % PMIC_IRQ_MASK_NBITS)
+
+struct intel_pmic_regmap {
+	int				offset;
+	int				shift;
+	int				mask;
+	int				flags;
+};
+
+struct intel_pmic_irqregmap {
+	struct intel_pmic_regmap	mask;
+	struct intel_pmic_regmap	status;
+	struct intel_pmic_regmap	ack;
+};
+
+struct intel_soc_pmic {
+	const char			*label;
+	unsigned long			irq_flags;
+	int				irq_num;
+	struct mfd_cell			*cell_dev;
+	struct intel_pmic_irqregmap	*irq_regmap;
+	struct regmap_config		*regmap_cfg;
+	int				(*init)(void);
+	struct device			*dev;
+	struct mutex			irq_lock; /* irq_bus_lock */
+	int				irq_need_update;
+	int				irq;
+	int				irq_base;
+	unsigned long			irq_mask[PMIC_IRQ_MASK_LEN];
+	int				pmic_int_gpio;
+	struct regmap			*regmap;
+};
+
+int intel_pmic_add(struct intel_soc_pmic *chip);
+int intel_pmic_remove(struct intel_soc_pmic *chip);
+
+extern struct intel_soc_pmic 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..88c28d9
--- /dev/null
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -0,0 +1,29 @@
+/*
+ * 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);
+int intel_soc_pmic_set_pdata(const char *name, void *data, int len);
+struct device *intel_soc_pmic_dev(void);
+
+#endif	/* __INTEL_SOC_PMIC_H__ */
-- 
1.8.3.2

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]

On Fri, May 23, 2014 at 08:40:26AM +0800, Zhu, Lejun wrote:

> +struct device *intel_soc_pmic_dev(void)
> +{
> +	return pmic->dev;
> +}
> +EXPORT_SYMBOL(intel_soc_pmic_dev);

Why do you need to take a global reference to this?

> +/*
> + * 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(intel_soc_pmic_readb);

This is wrapping the EXPORT_SYMBOL_GPL() regmap API with an
EXPORT_SYMBOL() API.  Don't do that, if you really do need these
wrappers make them EXPORT_SYMBOL_GPL().

There should also be no need to add extra locking around regmap calls,
the regmap API has locking as standard.

It's also not clear why this API exists at all, surely all the
interaction with the device happens from the core or function drivers
for the device which ought to be able to get a direct reference to the
regmap anyway and only be instantiated when one is present.

> +/*
> + * Set platform_data of the child devices. Needs to be called before
> + * the MFD device is added.
> + */
> +int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
> +{
> +	struct cell_dev_pdata *pdata;
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		pr_err("intel_pmic: can't set pdata!\n");
> +		return -ENOMEM;
> +	}
> +
> +	pdata->name = name;
> +	pdata->data = data;
> +	pdata->len = len;
> +
> +	list_add_tail(&pdata->list, &pdata_list);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(intel_soc_pmic_set_pdata);

What is going on here, why aren't the normal ways of getting data to the
devices (such as reading the platform data of the parent device) OK?

> +static void __pmic_regmap_flush(void)
> +{
> +	if (cache_write_pending)
> +		WARN_ON(regmap_write(pmic->regmap, cache_offset,
> +				     cache_write_val));

> +static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
> +{
> +	int ret;

This all appears to be an open coded cache layer - there is already
cache support in regmap, just reuse that.

> +static irqreturn_t pmic_irq_isr(int irq, void *data)
> +{
> +	return IRQ_WAKE_THREAD;
> +}

Just register the IRQ as IRQF_ONESHOT and only provide the threaded
handler.

> +static irqreturn_t pmic_irq_thread(int irq, void *data)
> +{
> +	int i;
> +
> +	mutex_lock(&pmic->irq_lock);
> +
> +	for (i = 0; i < pmic->irq_num; i++) {
> +		if (test_bit(PMIC_IRQ_MASK_BIT(i), &PMIC_IRQ_MASK(pmic, i)))
> +			continue;
> +
> +		if (pmic_regmap_read(&pmic->irq_regmap[i].status)) {
> +			pmic_regmap_write(&pmic->irq_regmap[i].ack, 1);
> +			handle_nested_irq(pmic->irq_base + i);
> +		}
> +	}

This looks like you should be using regmap-irq, or at least like there's
some small additions needed to make it usable.

> +	if (gpio_is_valid(pmic->pmic_int_gpio)) {
> +		ret = gpio_request_one(pmic->pmic_int_gpio,
> +				       GPIOF_DIR_IN, "PMIC Interupt");
> +		if (ret) {
> +			dev_err(pmic->dev, "Request PMIC_INT gpio error\n");
> +			goto out_free_desc;
> +		}
> +
> +		pmic->irq = gpio_to_irq(pmic->pmic_int_gpio);
> +	}

There should be no need to do this, simply requesting the interrupt
should be sufficient to ensure the pin is in the correct mode.  If this
isn't the case the interrupt controller driver probably needs an update,
there's some support for helping with this in the GPIO framework IIRC.

You're also not using managed (devm) allocations for anything here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Zhu, Lejun]

On 5/24/2014 1:49 AM, Mark Brown wrote:
> On Fri, May 23, 2014 at 08:40:26AM +0800, Zhu, Lejun wrote:
> 
>> +struct device *intel_soc_pmic_dev(void)
>> +{
>> +	return pmic->dev;
>> +}
>> +EXPORT_SYMBOL(intel_soc_pmic_dev);
> 
> Why do you need to take a global reference to this?

It was used by the GPIO driver to get the parent device. The latest
patch use dev.parent instead, so the whole function can be removed.

>> +/*
>> + * 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(intel_soc_pmic_readb);
> 
> This is wrapping the EXPORT_SYMBOL_GPL() regmap API with an
> EXPORT_SYMBOL() API.  Don't do that, if you really do need these
> wrappers make them EXPORT_SYMBOL_GPL().

I'll change them to use EXPORT_SYMBOL_GPL().

> There should also be no need to add extra locking around regmap calls,
> the regmap API has locking as standard.

Actually it also protects the pmic variable, so it won't be set to NULL
while there's ongoing read/write.

> It's also not clear why this API exists at all, surely all the
> interaction with the device happens from the core or function drivers
> for the device which ought to be able to get a direct reference to the
> regmap anyway and only be instantiated when one is present.

We created these names to hide the implementation of how read/write is
done from other platform specific patches interacting with this driver.
So when we change the implementation, e.g. from I2C read/write to
regmap, we don't have to touch all these patches.

>> +/*
>> + * Set platform_data of the child devices. Needs to be called before
>> + * the MFD device is added.
>> + */
>> +int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
(...)
>> +EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
> 
> What is going on here, why aren't the normal ways of getting data to the
> devices (such as reading the platform data of the parent device) OK?

For this PMIC (Crystal Cove) it is not used. So I'll remove it.

>> +static void __pmic_regmap_flush(void)
>> +{
>> +	if (cache_write_pending)
>> +		WARN_ON(regmap_write(pmic->regmap, cache_offset,
>> +				     cache_write_val));
> 
>> +static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
>> +{
>> +	int ret;
> 
> This all appears to be an open coded cache layer - there is already
> cache support in regmap, just reuse that.
>
>> +static irqreturn_t pmic_irq_isr(int irq, void *data)
>> +{
>> +	return IRQ_WAKE_THREAD;
>> +}
> 
> Just register the IRQ as IRQF_ONESHOT and only provide the threaded
> handler.

I'll fix it.

>> +static irqreturn_t pmic_irq_thread(int irq, void *data)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&pmic->irq_lock);
>> +
>> +	for (i = 0; i < pmic->irq_num; i++) {
>> +		if (test_bit(PMIC_IRQ_MASK_BIT(i), &PMIC_IRQ_MASK(pmic, i)))
>> +			continue;
>> +
>> +		if (pmic_regmap_read(&pmic->irq_regmap[i].status)) {
>> +			pmic_regmap_write(&pmic->irq_regmap[i].ack, 1);
>> +			handle_nested_irq(pmic->irq_base + i);
>> +		}
>> +	}
> 
> This looks like you should be using regmap-irq, or at least like there's
> some small additions needed to make it usable.

I'll check if I can convert to regmap-irq. If it works, I won't need
this and the cache layer.

>> +	if (gpio_is_valid(pmic->pmic_int_gpio)) {
>> +		ret = gpio_request_one(pmic->pmic_int_gpio,
>> +				       GPIOF_DIR_IN, "PMIC Interupt");
>> +		if (ret) {
>> +			dev_err(pmic->dev, "Request PMIC_INT gpio error\n");
>> +			goto out_free_desc;
>> +		}
>> +
>> +		pmic->irq = gpio_to_irq(pmic->pmic_int_gpio);
>> +	}
> 
> There should be no need to do this, simply requesting the interrupt
> should be sufficient to ensure the pin is in the correct mode.  If this
> isn't the case the interrupt controller driver probably needs an update,
> there's some support for helping with this in the GPIO framework IIRC.

I'll remove this.

> You're also not using managed (devm) allocations for anything here.

Best Regards
Lejun

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

On Mon, May 26, 2014 at 02:01:11PM +0800, Zhu, Lejun wrote:
> On 5/24/2014 1:49 AM, Mark Brown wrote:

> > There should also be no need to add extra locking around regmap calls,
> > the regmap API has locking as standard.

> Actually it also protects the pmic variable, so it won't be set to NULL
> while there's ongoing read/write.

Righ, but there is no clear need for the pmic variable to exist in the
first place.

> > It's also not clear why this API exists at all, surely all the
> > interaction with the device happens from the core or function drivers
> > for the device which ought to be able to get a direct reference to the
> > regmap anyway and only be instantiated when one is present.

> We created these names to hide the implementation of how read/write is
> done from other platform specific patches interacting with this driver.
> So when we change the implementation, e.g. from I2C read/write to
> regmap, we don't have to touch all these patches.

This sort of HAL is frowned upon in the upstream kernel.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Zhu, Lejun]

On 5/26/2014 10:51 PM, Mark Brown wrote:
> On Mon, May 26, 2014 at 02:01:11PM +0800, Zhu, Lejun wrote:
>> On 5/24/2014 1:49 AM, Mark Brown wrote:
> 
>>> There should also be no need to add extra locking around regmap calls,
>>> the regmap API has locking as standard.
> 
>> Actually it also protects the pmic variable, so it won't be set to NULL
>> while there's ongoing read/write.
> 
> Righ, but there is no clear need for the pmic variable to exist in the
> first place.
>
>>> It's also not clear why this API exists at all, surely all the
>>> interaction with the device happens from the core or function drivers
>>> for the device which ought to be able to get a direct reference to the
>>> regmap anyway and only be instantiated when one is present.
> 
>> We created these names to hide the implementation of how read/write is
>> done from other platform specific patches interacting with this driver.
>> So when we change the implementation, e.g. from I2C read/write to
>> regmap, we don't have to touch all these patches.
> 
> This sort of HAL is frowned upon in the upstream kernel.

We want to do what other MFD drivers' been doing, and make it easier for
the callers. A couple of similar examples are intel_msic_reg_read() and
lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(),
and I don't think it's too odd.

Best Regards
Lejun

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Tue, May 27, 2014 at 08:48:58AM +0800, Zhu, Lejun wrote:
> On 5/26/2014 10:51 PM, Mark Brown wrote:

> >> We created these names to hide the implementation of how read/write is
> >> done from other platform specific patches interacting with this driver.
> >> So when we change the implementation, e.g. from I2C read/write to
> >> regmap, we don't have to touch all these patches.

> > This sort of HAL is frowned upon in the upstream kernel.

> We want to do what other MFD drivers' been doing, and make it easier for
> the callers. A couple of similar examples are intel_msic_reg_read() and
> lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(),
> and I don't think it's too odd.

The odd and problematic bit is the global variable part of things -
these wrappers are usually just doing lookup of the underlying I/O
handle in the struct for the device and can be implemented as static
inlines in the header.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Zhu, Lejun]

On 5/27/2014 7:20 PM, Mark Brown wrote:
> On Tue, May 27, 2014 at 08:48:58AM +0800, Zhu, Lejun wrote:
>> On 5/26/2014 10:51 PM, Mark Brown wrote:
> 
>>>> We created these names to hide the implementation of how read/write is
>>>> done from other platform specific patches interacting with this driver.
>>>> So when we change the implementation, e.g. from I2C read/write to
>>>> regmap, we don't have to touch all these patches.
> 
>>> This sort of HAL is frowned upon in the upstream kernel.
> 
>> We want to do what other MFD drivers' been doing, and make it easier for
>> the callers. A couple of similar examples are intel_msic_reg_read() and
>> lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(),
>> and I don't think it's too odd.
> 
> The odd and problematic bit is the global variable part of things -
> these wrappers are usually just doing lookup of the underlying I/O
> handle in the struct for the device and can be implemented as static
> inlines in the header.
> 

Oh I see. Sorry I missed your point. So you are saying "int
intel_soc_pmic_readb(int reg)" is bad, but if I have:

int intel_soc_pmic_readb(struct intel_soc_pmic *pmic, int reg)
{
	int ret;
	unsigned int val;

	ret = regmap_read(pmic->regmap, reg, &val);
	if (!ret)
		ret = val;

	return ret;
}

And have the caller (device or core) look up and pass *pmic in, this
will be OK?

Best Regards
Lejun

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Wed, May 28, 2014 at 08:55:11AM +0800, Zhu, Lejun wrote:

> Oh I see. Sorry I missed your point. So you are saying "int
> intel_soc_pmic_readb(int reg)" is bad, but if I have:

> int intel_soc_pmic_readb(struct intel_soc_pmic *pmic, int reg)
> {
> 	int ret;
> 	unsigned int val;
> 
> 	ret = regmap_read(pmic->regmap, reg, &val);
> 	if (!ret)
> 		ret = val;
> 
> 	return ret;
> }

> And have the caller (device or core) look up and pass *pmic in, this
> will be OK?

Yes, that's the more common pattern - normally the caller will need
*pmic for some other purpose anyway so it saves an additional regmap
lookup if that's desired.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH RESEND v2 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.

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 | 148 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 148 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..b6b8ee1
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_i2c.c
@@ -0,0 +1,148 @@
+/*
+ * 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 i2c_client *pmic_i2c_client;
+static struct intel_soc_pmic *pmic_i2c;
+
+static void pmic_shutdown(struct i2c_client *client)
+{
+	disable_irq(pmic_i2c_client->irq);
+	return;
+}
+
+static int pmic_suspend(struct device *dev)
+{
+	disable_irq(pmic_i2c_client->irq);
+	return 0;
+}
+
+static int pmic_resume(struct device *dev)
+{
+	enable_irq(pmic_i2c_client->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_lookup_gpio(struct device *dev, int acpi_index)
+{
+	struct gpio_desc *desc;
+	int gpio;
+
+	desc = gpiod_get_index(dev, KBUILD_MODNAME, acpi_index);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	gpio = desc_to_gpio(desc);
+
+	gpiod_put(desc);
+
+	return gpio;
+}
+
+static int pmic_i2c_probe(struct i2c_client *i2c,
+			  const struct i2c_device_id *id)
+{
+	if (pmic_i2c_client != NULL || pmic_i2c != NULL)
+		return -EBUSY;
+
+	pmic_i2c	= (struct intel_soc_pmic *)id->driver_data;
+	pmic_i2c_client	= i2c;
+	pmic_i2c->dev	= &i2c->dev;
+	pmic_i2c->irq	= i2c->irq;
+	pmic_i2c->pmic_int_gpio = pmic_i2c_lookup_gpio(pmic_i2c->dev, 0);
+	pmic_i2c->regmap = devm_regmap_init_i2c(i2c, pmic_i2c->regmap_cfg);
+
+	return intel_pmic_add(pmic_i2c);
+}
+
+static int pmic_i2c_remove(struct i2c_client *i2c)
+{
+	int ret = intel_pmic_remove(pmic_i2c);
+
+	pmic_i2c_client = NULL;
+	pmic_i2c = NULL;
+
+	return ret;
+}
+
+static const struct i2c_device_id pmic_i2c_id[] = {
+	{ "crystal_cove", (kernel_ulong_t)&crystal_cove_pmic},
+	{ "INT33FD", (kernel_ulong_t)&crystal_cove_pmic},
+	{ "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;
+}
+subsys_initcall(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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On Fri, May 23, 2014 at 08:40:27AM +0800, Zhu, Lejun wrote:

> +static int pmic_i2c_lookup_gpio(struct device *dev, int acpi_index)
> +{
> +	struct gpio_desc *desc;
> +	int gpio;
> +
> +	desc = gpiod_get_index(dev, KBUILD_MODNAME, acpi_index);
> +	if (IS_ERR(desc))
> +		return PTR_ERR(desc);
> +
> +	gpio = desc_to_gpio(desc);
> +
> +	gpiod_put(desc);
> +
> +	return gpio;
> +}

Why not just have the driver work with the gpiod API, is there any real
need to convert to a GPIO number?

> +static const struct i2c_device_id pmic_i2c_id[] = {
> +	{ "crystal_cove", (kernel_ulong_t)&crystal_cove_pmic},
> +	{ "INT33FD", (kernel_ulong_t)&crystal_cove_pmic},
> +	{ "INT33FD:00", (kernel_ulong_t)&crystal_cove_pmic},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pmic_i2c_id);

The INT33FD ones here look like they should only be in the ACPI table.

> +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;
> +}
> +subsys_initcall(pmic_i2c_init);

module_i2c_driver() - you shouldn't need subsys_initcall().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Zhu, Lejun]

On 5/24/2014 1:53 AM, Mark Brown wrote:
> On Fri, May 23, 2014 at 08:40:27AM +0800, Zhu, Lejun wrote:
> 
>> +static int pmic_i2c_lookup_gpio(struct device *dev, int acpi_index)
>> +{
>> +	struct gpio_desc *desc;
>> +	int gpio;
>> +
>> +	desc = gpiod_get_index(dev, KBUILD_MODNAME, acpi_index);
>> +	if (IS_ERR(desc))
>> +		return PTR_ERR(desc);
>> +
>> +	gpio = desc_to_gpio(desc);
>> +
>> +	gpiod_put(desc);
>> +
>> +	return gpio;
>> +}
> 
> Why not just have the driver work with the gpiod API, is there any real
> need to convert to a GPIO number?

The whole function will be removed since the gpio set code is removed
during probe.

>> +static const struct i2c_device_id pmic_i2c_id[] = {
>> +	{ "crystal_cove", (kernel_ulong_t)&crystal_cove_pmic},
>> +	{ "INT33FD", (kernel_ulong_t)&crystal_cove_pmic},
>> +	{ "INT33FD:00", (kernel_ulong_t)&crystal_cove_pmic},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, pmic_i2c_id);
> 
> The INT33FD ones here look like they should only be in the ACPI table.

You are right. Only INT33FD:00 should be 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;
>> +}
>> +subsys_initcall(pmic_i2c_init);
> 
> module_i2c_driver() - you shouldn't need subsys_initcall().

I'll change it to module_init.

Best Regards
Lejun

[PATCH RESEND v2 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.

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 | 165 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 165 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..6ab4cbf
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -0,0 +1,165 @@
+/*
+ * 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 CRYSTAL_COVE_IRQ_NUM		7
+#define CRYSTAL_COVE_MAX_REGISTER	0xBD
+
+#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,
+	},
+	{NULL, },
+};
+
+#define	CRC_IRQREGMAP_VALUE(irq)	{	\
+		{MIRQLVL1, irq, 1, 0},		\
+		{IRQLVL1,  irq, 1, 0},		\
+		INTEL_PMIC_REG_NULL,		\
+	}
+
+static struct intel_pmic_irqregmap crystal_cove_irqregmap[] = {
+	[PWRSRC_IRQ]	= CRC_IRQREGMAP_VALUE(PWRSRC_IRQ),
+	[THRM_IRQ]	= CRC_IRQREGMAP_VALUE(THRM_IRQ),
+	[BCU_IRQ]	= CRC_IRQREGMAP_VALUE(BCU_IRQ),
+	[ADC_IRQ]	= CRC_IRQREGMAP_VALUE(ADC_IRQ),
+	[CHGR_IRQ]	= CRC_IRQREGMAP_VALUE(CHGR_IRQ),
+	[GPIO_IRQ]	= CRC_IRQREGMAP_VALUE(GPIO_IRQ),
+	[VHDMIOCP_IRQ]	= CRC_IRQREGMAP_VALUE(VHDMIOCP_IRQ),
+};
+
+static struct regmap_config crystal_cove_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = CRYSTAL_COVE_MAX_REGISTER,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int crystal_cove_init(void)
+{
+	pr_debug("Crystal Cove: ID 0x%02X, VERSION 0x%02X\n",
+		 intel_soc_pmic_readb(CHIPID), intel_soc_pmic_readb(CHIPVER));
+	return 0;
+}
+
+struct intel_soc_pmic crystal_cove_pmic = {
+	.label		= "crystal cove",
+	.irq_flags	= IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+	.init		= crystal_cove_init,
+	.cell_dev	= crystal_cove_dev,
+	.irq_regmap	= crystal_cove_irqregmap,
+	.irq_num	= CRYSTAL_COVE_IRQ_NUM,
+	.regmap_cfg	= &crystal_cove_regmap_config,
+};
-- 
1.8.3.2

[PATCH RESEND v2 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.

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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3383412..2c9de8e0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -241,6 +241,17 @@ 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
+	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 RESEND v2 0/4] mfd: Intel SoC Power Management IC

[Lee Jones]

> 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.
> 
> 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                |  11 +
>  drivers/mfd/Makefile               |   3 +
>  drivers/mfd/intel_soc_pmic_core.c  | 521 +++++++++++++++++++++++++++++++++++++
>  drivers/mfd/intel_soc_pmic_core.h  |  83 ++++++
>  drivers/mfd/intel_soc_pmic_crc.c   | 165 ++++++++++++
>  drivers/mfd/intel_soc_pmic_i2c.c   | 148 +++++++++++
>  include/linux/mfd/intel_soc_pmic.h |  29 +++
>  7 files changed, 960 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

Why are you re-sending this?

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

Re: [PATCH RESEND v2 0/4] mfd: Intel SoC Power Management IC

[Zhu, Lejun]

On 5/23/2014 6:08 PM, Lee Jones wrote:
> 
> Why are you re-sending this?
> 
Hi,

My mail server reported that it failed to send [PATCH v2 1/4] to LKML,
so I resent the whole series, only to get it properly archived.

Sorry for the confusion.

Best Regards
Lejun