[RESEND,v2,1/4] mfd: intel_soc_pmic: Core driver
diff mbox series

Message ID 1400805629-10322-2-git-send-email-lejun.zhu@linux.intel.com
State New, archived
Headers show
Series
  • mfd: Intel SoC Power Management IC
Related show

Commit Message

Zhu, Lejun May 23, 2014, 12:40 a.m. UTC
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

Comments

Mark Brown May 23, 2014, 5:49 p.m. UTC | #1
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.
Zhu, Lejun May 26, 2014, 6:01 a.m. UTC | #2
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown May 26, 2014, 2:51 p.m. UTC | #3
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.
Zhu, Lejun May 27, 2014, 12:48 a.m. UTC | #4
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown May 27, 2014, 11:20 a.m. UTC | #5
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.
Zhu, Lejun May 28, 2014, 12:55 a.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown May 28, 2014, 11:19 a.m. UTC | #7
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.

Patch
diff mbox series

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__ */