[PATCH v4 0/3] mfd: Intel SoC Power Management IC

[PATCH v4 0/3] 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.
v4:
- Remove all exported APIs which are wrappers of regmap API, export
  the regmap in data structure instead.
- Combine intel_soc_pmic_core.c and intel_soc_pmic_i2c.c
- Clean up include files.
- Remove useless members of struct intel_soc_pmic_config.
- Fix various coding style issues.

Zhu, Lejun (3):
  mfd: intel_soc_pmic: Core driver
  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  | 164 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_core.h  |  32 ++++++++
 drivers/mfd/intel_soc_pmic_crc.c   | 160 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic.h |  30 +++++++
 6 files changed, 401 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 include/linux/mfd/intel_soc_pmic.h

-- 
1.8.3.2

[PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Zhu, Lejun]

This patch provides the common I2C driver code for Intel SoC PMICs.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
v2:
- Use regmap instead of creating our own I2C read/write callbacks.
- Add one missing EXPORT_SYMBOL.
- Remove 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() and intel_soc_pmic_set_pdata().
- Use EXPORT_SYMBOL_GPL for exposed APIs.
- Use gpiod interface instead of gpio numbers.
- Remove redundant I2C IDs.
- Use managed allocations.
v4:
- Remove all exported APIs which are wrappers of regmap API, export
  the regmap in data structure instead.
- Combine I2C and core .c files.
- Clean up include files.
- Use intel_soc_pmic_ prefix to replace pmic_ and intel_pmic_.
- Fix various coding style issues.
---
 drivers/mfd/intel_soc_pmic_core.c  | 164 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_core.h  |  32 ++++++++
 include/linux/mfd/intel_soc_pmic.h |  30 +++++++
 3 files changed, 226 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..664eaef
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -0,0 +1,164 @@
+/*
+ * intel_soc_pmic_core.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>
+ * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/gpio/consumer.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include "intel_soc_pmic_core.h"
+
+static int intel_soc_pmic_find_gpio_irq(struct device *dev)
+{
+	struct gpio_desc *desc;
+	int irq;
+
+	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
+	if (IS_ERR(desc)) {
+		dev_dbg(dev, "Not using GPIO as interrupt.\n");
+		return -ENOENT;
+	}
+
+	irq = gpiod_to_irq(desc);
+	if (irq < 0)
+		dev_warn(dev, "Can't get irq: %d\n", irq);
+
+	return irq;
+}
+
+static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
+				    const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct intel_soc_pmic_config *config =
+		(struct intel_soc_pmic_config *)id->driver_data;
+	struct intel_soc_pmic *pmic;
+	int ret;
+	int irq;
+
+	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+	dev_set_drvdata(dev, pmic);
+
+	pmic->regmap = devm_regmap_init_i2c(i2c, config->regmap_config);
+
+	irq = intel_soc_pmic_find_gpio_irq(dev);
+	if (irq < 0)
+		pmic->irq = i2c->irq;
+	else
+		pmic->irq = irq;
+
+	ret = regmap_add_irq_chip(pmic->regmap, pmic->irq,
+				  config->irq_flags | IRQF_ONESHOT,
+				  0, config->irq_chip,
+				  &pmic->irq_chip_data);
+	if (ret)
+		goto err;
+
+	ret = enable_irq_wake(pmic->irq);
+	if (ret)
+		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
+
+	ret = mfd_add_devices(dev, -1, config->cell_dev,
+			      config->n_cell_devs, NULL, 0,
+			      regmap_irq_get_domain(pmic->irq_chip_data));
+	if (ret)
+		goto err_del_irq_chip;
+
+	return 0;
+
+err_del_irq_chip:
+	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
+err:
+	return ret;
+}
+
+static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(&i2c->dev);
+
+	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
+
+	mfd_remove_devices(&i2c->dev);
+
+	return 0;
+}
+
+static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(&i2c->dev);
+
+	disable_irq(pmic->irq);
+
+	return;
+}
+
+static int intel_soc_pmic_suspend(struct device *dev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+	disable_irq(pmic->irq);
+
+	return 0;
+}
+
+static int intel_soc_pmic_resume(struct device *dev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+	enable_irq(pmic->irq);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(intel_soc_pmic_pm_ops, intel_soc_pmic_suspend,
+			 intel_soc_pmic_resume);
+
+static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
+	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
+
+static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
+	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
+
+static struct i2c_driver intel_soc_pmic_i2c_driver = {
+	.driver = {
+		.name = "intel_soc_pmic_i2c",
+		.owner = THIS_MODULE,
+		.pm = &intel_soc_pmic_pm_ops,
+		.acpi_match_table = ACPI_PTR(intel_soc_pmic_acpi_match),
+	},
+	.probe = intel_soc_pmic_i2c_probe,
+	.remove = intel_soc_pmic_i2c_remove,
+	.id_table = intel_soc_pmic_i2c_id,
+	.shutdown = intel_soc_pmic_shutdown,
+};
+
+module_i2c_driver(intel_soc_pmic_i2c_driver);
+
+MODULE_DESCRIPTION("I2C driver for Intel SoC PMIC");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
+MODULE_AUTHOR("Zhu, Lejun <lejun.zhu@linux.intel.com>");
diff --git a/drivers/mfd/intel_soc_pmic_core.h b/drivers/mfd/intel_soc_pmic_core.h
new file mode 100644
index 0000000..33aacd9
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.h
@@ -0,0 +1,32 @@
+/*
+ * 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>
+ * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
+ */
+
+#ifndef __INTEL_SOC_PMIC_CORE_H__
+#define __INTEL_SOC_PMIC_CORE_H__
+
+struct intel_soc_pmic_config {
+	unsigned long irq_flags;
+	struct mfd_cell *cell_dev;
+	int n_cell_devs;
+	struct regmap_config *regmap_config;
+	struct regmap_irq_chip *irq_chip;
+};
+
+extern struct intel_soc_pmic_config intel_soc_pmic_config_crc;
+
+#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..abcbfcf
--- /dev/null
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -0,0 +1,30 @@
+/*
+ * 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>
+ * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
+ */
+
+#ifndef __INTEL_SOC_PMIC_H__
+#define __INTEL_SOC_PMIC_H__
+
+#include <linux/regmap.h>
+
+struct intel_soc_pmic {
+	int irq;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *irq_chip_data;
+};
+
+#endif	/* __INTEL_SOC_PMIC_H__ */
-- 
1.8.3.2

[PATCH v4 2/3] mfd: intel_soc_pmic: Crystal Cove support

[Zhu, Lejun]

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

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
v2:
- Add regmap_config for Crystal Cove.
v3:
- Convert IRQ config to regmap_irq_chip.
v4:
- Cleanup include files.
- Remove useless init() function.
- Remove useless .label and .init from struct intel_soc_pmic_config.
- Fix various coding style issues.
---
 drivers/mfd/intel_soc_pmic_crc.c | 160 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 160 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..43dbfcd
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -0,0 +1,160 @@
+/*
+ * 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>
+ * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
+ */
+
+#include <linux/mfd/core.h>
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include "intel_soc_pmic_core.h"
+
+#define CRYSTAL_COVE_MAX_REGISTER	0xC6
+
+#define REG_IRQLVL1			0x02
+#define REG_MIRQLVL1			0x0E
+
+enum crystal_cove_irq {
+	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",
+		.num_resources = ARRAY_SIZE(pwrsrc_resources),
+		.resources = pwrsrc_resources,
+	},
+	{
+		.name = "crystal_cove_adc",
+		.num_resources = ARRAY_SIZE(adc_resources),
+		.resources = adc_resources,
+	},
+	{
+		.name = "crystal_cove_thermal",
+		.num_resources = ARRAY_SIZE(thermal_resources),
+		.resources = thermal_resources,
+	},
+	{
+		.name = "crystal_cove_bcu",
+		.num_resources = ARRAY_SIZE(bcu_resources),
+		.resources = bcu_resources,
+	},
+	{
+		.name = "crystal_cove_gpio",
+		.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,
+};
+
+static const struct regmap_irq crystal_cove_irqs[] = {
+	[PWRSRC_IRQ] = {
+		.mask = BIT(PWRSRC_IRQ),
+	},
+	[THRM_IRQ] = {
+		.mask = BIT(THRM_IRQ),
+	},
+	[BCU_IRQ] = {
+		.mask = BIT(BCU_IRQ),
+	},
+	[ADC_IRQ] = {
+		.mask = BIT(ADC_IRQ),
+	},
+	[CHGR_IRQ] = {
+		.mask = BIT(CHGR_IRQ),
+	},
+	[GPIO_IRQ] = {
+		.mask = BIT(GPIO_IRQ),
+	},
+	[VHDMIOCP_IRQ] = {
+		.mask = BIT(VHDMIOCP_IRQ),
+	},
+};
+
+static struct regmap_irq_chip crystal_cove_irq_chip = {
+	.name = "Crystal Cove",
+	.irqs = crystal_cove_irqs,
+	.num_irqs = ARRAY_SIZE(crystal_cove_irqs),
+	.num_regs = 1,
+	.status_base = REG_IRQLVL1,
+	.mask_base = REG_MIRQLVL1,
+};
+
+struct intel_soc_pmic_config intel_soc_pmic_config_crc = {
+	.irq_flags = IRQF_TRIGGER_RISING,
+	.cell_dev = crystal_cove_dev,
+	.n_cell_devs = ARRAY_SIZE(crystal_cove_dev),
+	.regmap_config = &crystal_cove_regmap_config,
+	.irq_chip = &crystal_cove_irq_chip,
+};
-- 
1.8.3.2

[PATCH v4 3/3] mfd: intel_soc_pmic: Build files

[Zhu, Lejun]

This patch adds Intel SoC PMIC support to the build files.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
v2:
- Add select REGMAP_I2C.
v3:
- Add select REGMAP_IRQ.
v4:
- No change.
---
 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..36dda4c 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
+obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
-- 
1.8.3.2

Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Lee Jones]

> This patch provides the common I2C driver code for Intel SoC PMICs.
> 
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
> v2:
> - Use regmap instead of creating our own I2C read/write callbacks.
> - Add one missing EXPORT_SYMBOL.
> - Remove 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() and intel_soc_pmic_set_pdata().
> - Use EXPORT_SYMBOL_GPL for exposed APIs.
> - Use gpiod interface instead of gpio numbers.
> - Remove redundant I2C IDs.
> - Use managed allocations.
> v4:
> - Remove all exported APIs which are wrappers of regmap API, export
>   the regmap in data structure instead.
> - Combine I2C and core .c files.
> - Clean up include files.
> - Use intel_soc_pmic_ prefix to replace pmic_ and intel_pmic_.
> - Fix various coding style issues.
> ---
>  drivers/mfd/intel_soc_pmic_core.c  | 164 +++++++++++++++++++++++++++++++++++++
>  drivers/mfd/intel_soc_pmic_core.h  |  32 ++++++++
>  include/linux/mfd/intel_soc_pmic.h |  30 +++++++
>  3 files changed, 226 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..664eaef
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_core.c
> @@ -0,0 +1,164 @@

[...]

> +static int intel_soc_pmic_find_gpio_irq(struct device *dev)
> +{
> +	struct gpio_desc *desc;
> +	int irq;
> +
> +	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);

What does "KBUILD_MODNAME" translate to?

> +	if (IS_ERR(desc)) {
> +		dev_dbg(dev, "Not using GPIO as interrupt.\n");

You can't have a debug print, then return an err - use dev_err().

[...]

> +static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
> +				    const struct i2c_device_id *id)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct intel_soc_pmic_config *config =
> +		(struct intel_soc_pmic_config *)id->driver_data;
> +	struct intel_soc_pmic *pmic;
> +	int ret;
> +	int irq;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	dev_set_drvdata(dev, pmic);
> +
> +	pmic->regmap = devm_regmap_init_i2c(i2c, config->regmap_config);
> +
> +	irq = intel_soc_pmic_find_gpio_irq(dev);
> +	if (irq < 0)
> +		pmic->irq = i2c->irq;
> +	else
> +		pmic->irq = irq;

pmic->irq = (irq < 0) ? i2c->irq : irq;

> +	ret = regmap_add_irq_chip(pmic->regmap, pmic->irq,
> +				  config->irq_flags | IRQF_ONESHOT,
> +				  0, config->irq_chip,
> +				  &pmic->irq_chip_data);
> +	if (ret)
> +		goto err;

Just return ret here and remove the 'err:' label.

[...]

> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
> +
> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);

Does ACPI have a match function to extact it's .driver_data attribute?

If so, are you using it here? If not, why not?

[...]

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

Re: [PATCH v4 3/3] mfd: intel_soc_pmic: Build files

[Lee Jones]

> This patch adds Intel SoC PMIC support to the build files.
> 
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
> v2:
> - Add select REGMAP_I2C.
> v3:
> - Add select REGMAP_IRQ.
> v4:
> - No change.
> ---
>  drivers/mfd/Kconfig  | 12 ++++++++++++
>  drivers/mfd/Makefile |  3 +++
>  2 files changed, 15 insertions(+)

These changes shouldn't really be in a separate patch.

> 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..36dda4c 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
> +obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o

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

Re: [PATCH v4 2/3] mfd: intel_soc_pmic: Crystal Cove support

[Lee Jones]

On Thu, 29 May 2014, Zhu, Lejun wrote:

> This patch provides chip-specific support for Crystal Cove. Crystal
> Cove is the PMIC in Baytrail-T platform.
> 
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
> v2:
> - Add regmap_config for Crystal Cove.
> v3:
> - Convert IRQ config to regmap_irq_chip.
> v4:
> - Cleanup include files.
> - Remove useless init() function.
> - Remove useless .label and .init from struct intel_soc_pmic_config.
> - Fix various coding style issues.
> ---
>  drivers/mfd/intel_soc_pmic_crc.c | 160 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 160 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..43dbfcd
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_crc.c
> @@ -0,0 +1,160 @@
> +/*
> + * 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>
> + * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include "intel_soc_pmic_core.h"
> +
> +#define CRYSTAL_COVE_MAX_REGISTER	0xC6
> +
> +#define REG_IRQLVL1			0x02
> +#define REG_MIRQLVL1			0x0E
> +
> +enum crystal_cove_irq {
> +	PWRSRC_IRQ = 0,
> +	THRM_IRQ,
> +	BCU_IRQ,
> +	ADC_IRQ,
> +	CHGR_IRQ,
> +	GPIO_IRQ,
> +	VHDMIOCP_IRQ
> +};

I can't help thinking that these should be nice clear #defines

#define CRYSTAL_COVE_IRQ_PWSRC 		0
...
#define CRYSTAL_COVE_IRQ_VHDMIOCP	6

[...]

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

Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Zhu, Lejun]

On 5/29/2014 7:40 PM, Lee Jones wrote:
> [...]
> 
>> +static int intel_soc_pmic_find_gpio_irq(struct device *dev)
>> +{
>> +	struct gpio_desc *desc;
>> +	int irq;
>> +
>> +	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
> 
> What does "KBUILD_MODNAME" translate to?

It translates into "intel_soc_pmic".

> 
>> +	if (IS_ERR(desc)) {
>> +		dev_dbg(dev, "Not using GPIO as interrupt.\n");
> 
> You can't have a debug print, then return an err - use dev_err().

Actually returning ENOENT here is just a hardware difference. On some
boards the PMIC interrupt is from a GPIO line exposed by the CPU, on the
rest (e.g. Asus T100TA) it's not. When -ENOENT is returned, probe() will
simply use the IRQ provided by the I2C.

I will remove this line completely, and put a comment before the function.

> 
> [...]
> 
>> +static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>> +				    const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &i2c->dev;
>> +	struct intel_soc_pmic_config *config =
>> +		(struct intel_soc_pmic_config *)id->driver_data;
>> +	struct intel_soc_pmic *pmic;
>> +	int ret;
>> +	int irq;
>> +
>> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
>> +	dev_set_drvdata(dev, pmic);
>> +
>> +	pmic->regmap = devm_regmap_init_i2c(i2c, config->regmap_config);
>> +
>> +	irq = intel_soc_pmic_find_gpio_irq(dev);
>> +	if (irq < 0)
>> +		pmic->irq = i2c->irq;
>> +	else
>> +		pmic->irq = irq;
> 
> pmic->irq = (irq < 0) ? i2c->irq : irq;

I'll fix it.

>> +	ret = regmap_add_irq_chip(pmic->regmap, pmic->irq,
>> +				  config->irq_flags | IRQF_ONESHOT,
>> +				  0, config->irq_chip,
>> +				  &pmic->irq_chip_data);
>> +	if (ret)
>> +		goto err;
> 
> Just return ret here and remove the 'err:' label.

I'll fix it.

> 
> [...]
> 
>> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
>> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
>> +
>> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
>> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
> 
> Does ACPI have a match function to extact it's .driver_data attribute?
> 
> If so, are you using it here? If not, why not?
> 

The ACPI table is used in i2c_device_match(), and the i2c table is used
in i2c_device_probe(), so the id in the i2c table is actually fed to
intel_soc_pmic_probe(). But I only found out now that having the i2c
table alone is enough, because i2c_device_match will fallback to the i2c
table if there's no ACPI table. So to keep it simple, I'll remove the
ACPI table completely.

By the way, the GPIO child driver got reviewed-by from Linus Walleij,
but can't be merged because it depends on intel_soc_pmic.h. May I
include it in next version of the patch set and have it merged along
with the MFD driver?

Best Regards
Lejun

Re: [PATCH v4 3/3] mfd: intel_soc_pmic: Build files

[Zhu, Lejun]

On 5/29/2014 7:43 PM, Lee Jones wrote:
>> This patch adds Intel SoC PMIC support to the build files.
> These changes shouldn't really be in a separate patch.
I'll move them into the first (core.c) patch.

Best Regards
Lejun

Re: [PATCH v4 2/3] mfd: intel_soc_pmic: Crystal Cove support

[Zhu, Lejun]

On 5/29/2014 7:49 PM, Lee Jones wrote:
> On Thu, 29 May 2014, Zhu, Lejun wrote:
> 
>> This patch provides chip-specific support for Crystal Cove. Crystal
>> Cove is the PMIC in Baytrail-T platform.
(...)
>> +enum crystal_cove_irq {
>> +	PWRSRC_IRQ = 0,
>> +	THRM_IRQ,
>> +	BCU_IRQ,
>> +	ADC_IRQ,
>> +	CHGR_IRQ,
>> +	GPIO_IRQ,
>> +	VHDMIOCP_IRQ
>> +};
> 
> I can't help thinking that these should be nice clear #defines
> 
> #define CRYSTAL_COVE_IRQ_PWSRC 		0
> ...
> #define CRYSTAL_COVE_IRQ_VHDMIOCP	6
> 

Sure. I'll fix it.

Best Regards
Lejun

Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Lee Jones]

> >> +static int intel_soc_pmic_find_gpio_irq(struct device *dev)
> >> +{
> >> +	struct gpio_desc *desc;
> >> +	int irq;
> >> +
> >> +	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
> > 
> > What does "KBUILD_MODNAME" translate to?
> 
> It translates into "intel_soc_pmic".

Can you just put that instead?

> >> +	if (IS_ERR(desc)) {
> >> +		dev_dbg(dev, "Not using GPIO as interrupt.\n");
> > 
> > You can't have a debug print, then return an err - use dev_err().
> 
> Actually returning ENOENT here is just a hardware difference. On some
> boards the PMIC interrupt is from a GPIO line exposed by the CPU, on the
> rest (e.g. Asus T100TA) it's not. When -ENOENT is returned, probe() will
> simply use the IRQ provided by the I2C.
> 
> I will remove this line completely, and put a comment before the function.

That'll do, thanks.

> >> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
> >> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> >> +	{ }
> >> +};
> >> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
> >> +
> >> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
> >> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> >> +	{ },
> >> +};
> >> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
> > 
> > Does ACPI have a match function to extact it's .driver_data attribute?
> > 
> > If so, are you using it here? If not, why not?
> > 
> 
> The ACPI table is used in i2c_device_match(), and the i2c table is used
> in i2c_device_probe(), so the id in the i2c table is actually fed to
> intel_soc_pmic_probe(). But I only found out now that having the i2c
> table alone is enough, because i2c_device_match will fallback to the i2c
> table if there's no ACPI table. So to keep it simple, I'll remove the
> ACPI table completely.

Actually, can you do it the other way round?  Minimise the i2c table
and populate the ACPI one.  I'm just about to work on a separate
patch-set which deprecates the use of the i2c table on DT and/or ACPI
only registered devices.

> By the way, the GPIO child driver got reviewed-by from Linus Walleij,
> but can't be merged because it depends on intel_soc_pmic.h. May I
> include it in next version of the patch set and have it merged along
> with the MFD driver?

Yes, if it's okay with Linus and you aapply his Ack.

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

Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Zhu, Lejun]

On 2014/5/30 16:08, Lee Jones wrote:
>>>> +static int intel_soc_pmic_find_gpio_irq(struct device *dev)
>>>> +{
>>>> +	struct gpio_desc *desc;
>>>> +	int irq;
>>>> +
>>>> +	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
>>>
>>> What does "KBUILD_MODNAME" translate to?
>>
>> It translates into "intel_soc_pmic".
> 
> Can you just put that instead?

Sure. I'll fix it.

(...)

>>>> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
>>>> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
>>>> +
>>>> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
>>>> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
>>>> +	{ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
>>>
>>> Does ACPI have a match function to extact it's .driver_data attribute?
>>>
>>> If so, are you using it here? If not, why not?
>>>
>>
>> The ACPI table is used in i2c_device_match(), and the i2c table is used
>> in i2c_device_probe(), so the id in the i2c table is actually fed to
>> intel_soc_pmic_probe(). But I only found out now that having the i2c
>> table alone is enough, because i2c_device_match will fallback to the i2c
>> table if there's no ACPI table. So to keep it simple, I'll remove the
>> ACPI table completely.
> 
> Actually, can you do it the other way round?  Minimise the i2c table
> and populate the ACPI one.  I'm just about to work on a separate
> patch-set which deprecates the use of the i2c table on DT and/or ACPI
> only registered devices.

Current i2c_device_probe will only feed driver_data from i2c_device_id
table to intel_soc_pmic_probe(), because it uses i2c_match_id(). So if I
remove "&intel_soc_pmic_config_crc" from the i2c table, I will get NULL
from id->driver_data until your new patch fixes it.

So for the driver to work for the i2c code both today and in the future,
I think it's best to keep the driver_data populated in both tables. What
do you think?

Best Regards
Lejun

Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Lee Jones]

> >>>> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
> >>>> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> >>>> +	{ }
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
> >>>> +
> >>>> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
> >>>> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> >>>> +	{ },
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
> >>>
> >>> Does ACPI have a match function to extact it's .driver_data attribute?
> >>>
> >>> If so, are you using it here? If not, why not?
> >>>
> >>
> >> The ACPI table is used in i2c_device_match(), and the i2c table is used
> >> in i2c_device_probe(), so the id in the i2c table is actually fed to
> >> intel_soc_pmic_probe(). But I only found out now that having the i2c
> >> table alone is enough, because i2c_device_match will fallback to the i2c
> >> table if there's no ACPI table. So to keep it simple, I'll remove the
> >> ACPI table completely.
> > 
> > Actually, can you do it the other way round?  Minimise the i2c table
> > and populate the ACPI one.  I'm just about to work on a separate
> > patch-set which deprecates the use of the i2c table on DT and/or ACPI
> > only registered devices.
> 
> Current i2c_device_probe will only feed driver_data from i2c_device_id
> table to intel_soc_pmic_probe(), because it uses i2c_match_id(). So if I
> remove "&intel_soc_pmic_config_crc" from the i2c table, I will get NULL
> from id->driver_data until your new patch fixes it.

Right, which is why I asked if ACPI has a match function - I just
looked and it does.  So what you need to do is supply a very simple
i2c_device_id struct (just until my patch lands, then there'll be no
reason to supply one at all) and use  acpi_match_device() instead of
using id->driver_data.

> So for the driver to work for the i2c code both today and in the future,
> I think it's best to keep the driver_data populated in both tables. What
> do you think?


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

Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Zhu, Lejun]

On 2014/5/30 17:28, Lee Jones wrote:
>>>>>> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
>>>>>> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
>>>>>> +	{ }
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
>>>>>> +
>>>>>> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
>>>>>> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
>>>>>> +	{ },
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
>>>>>
>>>>> Does ACPI have a match function to extact it's .driver_data attribute?
>>>>>
>>>>> If so, are you using it here? If not, why not?
>>>>>
>>>>
>>>> The ACPI table is used in i2c_device_match(), and the i2c table is used
>>>> in i2c_device_probe(), so the id in the i2c table is actually fed to
>>>> intel_soc_pmic_probe(). But I only found out now that having the i2c
>>>> table alone is enough, because i2c_device_match will fallback to the i2c
>>>> table if there's no ACPI table. So to keep it simple, I'll remove the
>>>> ACPI table completely.
>>>
>>> Actually, can you do it the other way round?  Minimise the i2c table
>>> and populate the ACPI one.  I'm just about to work on a separate
>>> patch-set which deprecates the use of the i2c table on DT and/or ACPI
>>> only registered devices.
>>
>> Current i2c_device_probe will only feed driver_data from i2c_device_id
>> table to intel_soc_pmic_probe(), because it uses i2c_match_id(). So if I
>> remove "&intel_soc_pmic_config_crc" from the i2c table, I will get NULL
>> from id->driver_data until your new patch fixes it.
> 
> Right, which is why I asked if ACPI has a match function - I just
> looked and it does.  So what you need to do is supply a very simple
> i2c_device_id struct (just until my patch lands, then there'll be no
> reason to supply one at all) and use  acpi_match_device() instead of
> using id->driver_data.
> 

Oh I see. You mean calling acpi_match_device() in my own probe(). I'll
change my code to do that in next version.

Best Regards
Lejun