Skip to contentSkip to conversation

add 88pm80x mfd driver

PATCH Superseded Qiao Zhou 4 participants 22 messages Jun 28, 2012 – Jul 3, 2012

Subsystems (4)
I2C subsystem host drivers Input drivers Multifunction devices (MFD) Real time clock (RTC)

v1 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Qiao Zhou

author
#0 lore raw

cover add 88pm80x mfd driver

change log [v1]:
1, pm800 and pm805 are decoupled and probed separately;
2, re-used the most of API for pm800 and pm805 per Arnd's comments; 3, use regmap_irq, instead of previous 88pm80x_irq_data per Mark's comments. use regmap_add_irq_chip, and remove previous 88pm80x irq handling. 4, remove callback function in rtc pdata per Arnd's comments.
5, updated some coding style issue.

Qiao Zhou (3):
mfd: support 88pm80x in 80x driver
rtc: add rtc support to 88PM80X PMIC
input: add onkey support to 88PM80X PMIC

 drivers/input/misc/88pm80x_onkey.c |  194 ++++++++++
 drivers/input/misc/Kconfig         |   10 +
 drivers/input/misc/Makefile        |    1 +
 drivers/mfd/88pm80x-core.c         |  545 +++++++++++++++++++++++++++++
 drivers/mfd/88pm80x-i2c.c          |  274 +++++++++++++++
 drivers/mfd/Kconfig                |   12 +
 drivers/mfd/Makefile               |    2 +
 drivers/rtc/Kconfig                |   10 +
 drivers/rtc/Makefile               |    1 +
 drivers/rtc/rtc-88pm80x.c          |  370 ++++++++++++++++++++
 include/linux/mfd/88pm80x.h        |  678 ++++++++++++++++++++++++++++++++++++
 11 files changed, 2097 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/88pm80x_onkey.c
 create mode 100644 drivers/mfd/88pm80x-core.c
 create mode 100644 drivers/mfd/88pm80x-i2c.c
 create mode 100644 drivers/rtc/rtc-88pm80x.c
 create mode 100644 include/linux/mfd/88pm80x.h
author
#1 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

88PM800 and 88PM805 are two discrete chips used for power management. Hardware designer can use them together or only one of them according to requirement.

due to hardware design, there are some registers used by pm805, but actually defined in pm800 chip. so pm805 needs to access pm800 register in some occasion. A workaround is used in driver to solve such issue and would be removed when it's fixed in latter new chip version.

All I2C operations are accessed by 80x-i2c driver, and register access is via regmap interface.

The benefit is that client drivers only need one kind of read/write API. I2C and MFD driver can be shared in both 800 and 805.

Signed-off-by Qiao Zhou <[email protected]>
---
 drivers/mfd/88pm80x-core.c  |  545 ++++++++++++++++++++++++++++++++++
 drivers/mfd/88pm80x-i2c.c   |  274 +++++++++++++++++
 drivers/mfd/Kconfig         |   12 +
 drivers/mfd/Makefile        |    2 +
 include/linux/mfd/88pm80x.h |  678 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 1511 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/88pm80x-core.c
 create mode 100644 drivers/mfd/88pm80x-i2c.c
 create mode 100644 include/linux/mfd/88pm80x.h
drivers/mfd/88pm80x-core.c new file +545 -0
new file mode 100644
index 0000000..b836c0c
@@ -0,0 +1,545 @@
+/*
+ * Base driver for Marvell 88PM80X
+ *
+ * Copyright (C) 2012 Marvell International Ltd.
+ * Haojian Zhuang <[email protected]>
+ * Joseph(Yossi) Hanin <[email protected]>
+ * Qiao Zhou <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/88pm80x.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+
+static struct resource rtc_resources[] = {
+	{
+	 .name = "88pm80x-rtc",
+	 .start = PM800_IRQ_RTC,
+	 .end = PM800_IRQ_RTC,
+	 .flags = IORESOURCE_IRQ,
+	 },
+};
+
+static struct mfd_cell rtc_devs[] = {
+	{
+	 .name = "88pm80x-rtc",
+	 .num_resources = ARRAY_SIZE(rtc_resources),
+	 .resources = &rtc_resources[0],
+	 .id = -1,
+	 },
+};
+
+static struct resource onkey_resources[] = {
+	{
+	 .name = "88pm80x-onkey",
+	 .start = PM800_IRQ_ONKEY,
+	 .end = PM800_IRQ_ONKEY,
+	 .flags = IORESOURCE_IRQ,
+	 },
+};
+
+static struct mfd_cell onkey_devs[] = {
+	{
+	 .name = "88pm80x-onkey",
+	 .num_resources = 1,
+	 .resources = &onkey_resources[0],
+	 .id = -1,
+	 },
+};
+
+static struct resource codec_resources[] = {
+	{
+	 /* Headset microphone insertion or removal */
+	 .name = "micin",
+	 .start = PM805_IRQ_MIC_DET,
+	 .end = PM805_IRQ_MIC_DET,
+	 .flags = IORESOURCE_IRQ,
+	 },
+	{
+	 /* Audio short HP1 */
+	 .name = "audio-short1",
+	 .start = PM805_IRQ_HP1_SHRT,
+	 .end = PM805_IRQ_HP1_SHRT,
+	 .flags = IORESOURCE_IRQ,
+	 },
+	{
+	 /* Audio short HP2 */
+	 .name = "audio-short2",
+	 .start = PM805_IRQ_HP2_SHRT,
+	 .end = PM805_IRQ_HP2_SHRT,
+	 .flags = IORESOURCE_IRQ,
+	 },
+};
+
+static struct mfd_cell codec_devs[] = {
+	{
+	 .name = "88pm80x-codec",
+	 .num_resources = ARRAY_SIZE(codec_resources),
+	 .resources = &codec_resources[0],
+	 .id = -1,
+	 },
+};
+
+static const struct regmap_irq pm800_irqs[] = {
+	/* INT0 */
+	[PM800_IRQ_ONKEY] = {
+		.mask = PM800_ONKEY_INT_ENA1,
+	},
+	[PM800_IRQ_EXTON] = {
+		.mask = PM800_EXTON_INT_ENA1,
+	},
+	[PM800_IRQ_CHG] = {
+		.mask = PM800_CHG_INT_ENA1,
+	},
+	[PM800_IRQ_BAT] = {
+		.mask = PM800_BAT_INT_ENA1,
+	},
+	[PM800_IRQ_RTC] = {
+		.mask = PM800_RTC_INT_ENA1,
+	},
+	[PM800_IRQ_CLASSD] = {
+		.mask = PM800_CLASSD_OC_INT_ENA1,
+	},
+	/* INT1 */
+	[PM800_IRQ_VBAT] = {
+		.reg_offset = 1,
+		.mask = PM800_VBAT_INT_ENA2,
+	},
+	[PM800_IRQ_VSYS] = {
+		.reg_offset = 1,
+		.mask = PM800_VSYS_INT_ENA2,
+	},
+	[PM800_IRQ_VCHG] = {
+		.reg_offset = 1,
+		.mask = PM800_VCHG_INT_ENA2,
+	},
+	[PM800_IRQ_TINT] = {
+		.reg_offset = 1,
+		.mask = PM800_TINT_INT_ENA2,
+	},
+	/* INT2 */
+	[PM800_IRQ_GPADC0] = {
+		.reg_offset = 2,
+		.mask = PM800_GPADC0_INT_ENA3,
+	},
+	[PM800_IRQ_GPADC1] = {
+		.reg_offset = 2,
+		.mask = PM800_GPADC1_INT_ENA3,
+	},
+	[PM800_IRQ_GPADC2] = {
+		.reg_offset = 2,
+		.mask = PM800_GPADC2_INT_ENA3,
+	},
+	[PM800_IRQ_GPADC3] = {
+		.reg_offset = 2,
+		.mask = PM800_GPADC3_INT_ENA3,
+	},
+	[PM800_IRQ_GPADC4] = {
+		.reg_offset = 2,
+		.mask = PM800_GPADC4_INT_ENA3,
+	},
+	/* INT3 */
+	[PM800_IRQ_GPIO0] = {
+		.reg_offset = 3,
+		.mask = PM800_GPIO0_INT_ENA4,
+	},
+	[PM800_IRQ_GPIO1] = {
+		.reg_offset = 3,
+		.mask = PM800_GPIO1_INT_ENA4,
+	},
+	[PM800_IRQ_GPIO2] = {
+		.reg_offset = 3,
+		.mask = PM800_GPIO2_INT_ENA4,
+	},
+	[PM800_IRQ_GPIO3] = {
+		.reg_offset = 3,
+		.mask = PM800_GPIO3_INT_ENA4,
+	},
+	[PM800_IRQ_GPIO4] = {
+		.reg_offset = 3,
+		.mask = PM800_GPIO4_INT_ENA4,
+	},
+};
+
+static struct regmap_irq pm805_irqs[] = {
+	/* INT0 */
+	[PM805_IRQ_LDO_OFF] = {
+		.mask = PM805_INT1_HP1_SHRT,
+	},
+	[PM805_IRQ_SRC_DPLL_LOCK] = {
+		.mask = PM805_INT1_HP2_SHRT,
+	},
+	[PM805_IRQ_CLIP_FAULT] = {
+		.mask = PM805_INT1_MIC_CONFLICT,
+	},
+	[PM805_IRQ_MIC_CONFLICT] = {
+		.mask = PM805_INT1_CLIP_FAULT,
+	},
+	[PM805_IRQ_HP2_SHRT] = {
+		.mask = PM805_INT1_LDO_OFF,
+	},
+	[PM805_IRQ_HP1_SHRT] = {
+		.mask = PM805_INT1_SRC_DPLL_LOCK,
+	},
+	/* INT1 */
+	[PM805_IRQ_FINE_PLL_FAULT] = {
+		.reg_offset = 1,
+		.mask = PM805_INT2_MIC_DET,
+	},
+	[PM805_IRQ_RAW_PLL_FAULT] = {
+		.reg_offset = 1,
+		.mask = PM805_INT2_SHRT_BTN_DET,
+	},
+	[PM805_IRQ_VOLP_BTN_DET] = {
+		.reg_offset = 1,
+		.mask = PM805_INT2_VOLM_BTN_DET,
+	},
+	[PM805_IRQ_VOLM_BTN_DET] = {
+		.reg_offset = 1,
+		.mask = PM805_INT2_VOLP_BTN_DET,
+	},
+	[PM805_IRQ_SHRT_BTN_DET] = {
+		.reg_offset = 1,
+		.mask = PM805_INT2_RAW_PLL_FAULT,
+	},
+	[PM805_IRQ_MIC_DET] = {
+		.reg_offset = 1,
+		.mask = PM805_INT2_FINE_PLL_FAULT,
+	},
+};
+
+static int __devinit device_gpadc_init(struct pm80x_chip *chip,
+				       struct pm80x_platform_data *pdata)
+{
+	struct pm80x_subchip *subchip = chip->subchip;
+	struct i2c_client *i2c = subchip->gpadc_page;
+	struct regmap *map = dev_get_regmap(&i2c->dev, NULL);
+	int data = 0, mask = 0, ret = 0;
+
+	if (!map) {
+		dev_warn(chip->dev,
+			 "Warning: gpadc regmap is not available!\n");
+		return -EINVAL;
+	}
+	/* initialize GPADC without activating it turn on GPADC
+	 * measurments
+	 */
+	ret = regmap_update_bits(map,
+				 PM800_GPADC_MISC_CONFIG2,
+				 PM800_GPADC_MISC_GPFSM_EN,
+				 PM800_GPADC_MISC_GPFSM_EN);
+	if (ret < 0)
+		goto out;
+	/*
+	 * This function configures the ADC as requires for
+	 * CP implementation.CP does not "own" the ADC configuration
+	 * registers and relies on AP.
+	 * Reason: enable automatic ADC measurements needed
+	 * for CP to get VBAT and RF temperature readings.
+	 */
+	ret = regmap_update_bits(map, PM800_GPADC_MEAS_EN1,
+				 PM800_MEAS_EN1_VBAT, PM800_MEAS_EN1_VBAT);
+	if (ret < 0)
+		goto out;
+	ret = regmap_update_bits(map, PM800_GPADC_MEAS_EN2,
+				 (PM800_MEAS_EN2_RFTMP | PM800_MEAS_GP0_EN),
+				 (PM800_MEAS_EN2_RFTMP | PM800_MEAS_GP0_EN));
+	if (ret < 0)
+		goto out;
+
+	/* the defult of PM800 is GPADC operates@100Ks/s rate
+	 * and Number of GPADC slots with active current bias prior
+	 * to GPADC sampling = 1 slot for all GPADCs set for
+	 * Temprature mesurmants
+	 */
+	mask = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
+		PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
+
+	if (pdata && (pdata->batt_det == 0)) {
+		data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
+			PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
+	} else {
+		data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN2 |
+			PM800_GPADC_GP_BIAS_EN3);
+	}
+	ret = regmap_update_bits(map, PM800_GP_BIAS_ENA1, mask, data);
+	if (ret < 0)
+		goto out;
+
+	dev_info(chip->dev, "pm80x device_gpadc_init: Done\n");
+	return 0;
+
+out:
+	dev_info(chip->dev, "pm80x device_gpadc_init: Failed!\n");
+	return ret;
+}
+
+static int __devinit device_irq_init_80x(struct pm80x_chip *chip)
+{
+	struct i2c_client *i2c = chip->client;
+	struct regmap *map = dev_get_regmap(&i2c->dev, NULL);
+	unsigned long flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
+	int data, mask, ret = -EINVAL;
+
+	if (!map || !chip->irq) {
+		dev_err(chip->dev, "incorrect parameters\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * irq_mode defines the way of clearing interrupt. currently only
+	 * suport write 1 to clear status, which is aligned with regmap
+	 * implementation.
+	 */
+	if (chip->id == CHIP_PM800) {
+		flags |= IRQF_SHARED;
+		mask =
+		    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
+		    PM800_WAKEUP2_INT_MASK;
+
+		data = PM800_WAKEUP2_INT_CLEAR;
+		ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
+	} else if (chip->id == CHIP_PM805) {
+		mask =
+		    PM805_STATUS0_INT_CLEAR | PM805_STATUS0_INV_INT |
+		    PM800_STATUS0_INT_MASK;
+
+		data = PM805_STATUS0_INT_CLEAR;
+		ret = regmap_update_bits(map, PM805_INT_STATUS0, mask, data);
+		/* PM805_INT_STATUS is under 32K clock domain, so need to
+		 * add proper delay before the next I2C register access.
+		 */
+		msleep(1);
+	}
+
+	if (ret < 0)
+		goto out;
+
+	ret =
+	    regmap_add_irq_chip(chip->regmap, chip->irq, flags, chip->irq_base,
+				chip->regmap_irq_chip, &chip->irq_data);
+
+out:
+	return ret;
+}
+
+static void device_irq_exit_80x(struct pm80x_chip *chip)
+{
+	regmap_del_irq_chip(chip->irq, chip->irq_data);
+}
+
+static struct regmap_irq_chip pm805_irq_chip = {
+	.name = "88pm805",
+	.irqs = pm805_irqs,
+	.num_irqs = ARRAY_SIZE(pm805_irqs),
+
+	.num_regs = 2,
+	.status_base = PM805_INT_STATUS1,
+	.mask_base = PM805_INT_MASK1,
+	.ack_base = PM805_INT_STATUS1,
+};
+
+static struct regmap_irq_chip pm800_irq_chip = {
+	.name = "88pm800",
+	.irqs = pm800_irqs,
+	.num_irqs = ARRAY_SIZE(pm800_irqs),
+
+	.num_regs = 4,
+	.status_base = PM800_INT_STATUS1,
+	.mask_base = PM800_INT_ENA_1,
+	.ack_base = PM800_INT_STATUS1,
+};
+
+static int __devinit device_805_init(struct pm80x_chip *chip,
+				     struct i2c_client *i2c,
+				     struct pm80x_platform_data *pdata)
+{
+	int ret = 0;
+	struct regmap *map = dev_get_regmap(&i2c->dev, NULL);
+
+	if (!map) {
+		dev_err(chip->dev, "regmap is invalid\n");
+		return -EINVAL;
+	}
+
+	dev_info(chip->dev, "pm80x:%s slave addr[0x%x]\n", __func__, i2c->addr);
+
+	regmap_read(map, PM805_CHIP_ID, &ret);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to read CHIP ID: %d\n", ret);
+		goto out_irq_init;
+	}
+	chip->version = ret;
+
+	chip->regmap_irq_chip = &pm805_irq_chip;
+
+	ret = device_irq_init_80x(chip);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to init pm805 irq!\n");
+		goto out_irq_init;
+	}
+
+	ret = mfd_add_devices(chip->dev, 0, &codec_devs[0],
+			      ARRAY_SIZE(codec_devs), &codec_resources[0], 0);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to add codec subdev\n");
+		goto out_codec;
+	} else
+		dev_info(chip->dev, "[%s]:Added mfd codec_devs\n", __func__);
+
+	if (pdata->pm80x_plat_config)
+		pdata->pm80x_plat_config(chip, pdata);
+
+	return 0;
+
+out_codec:
+	device_irq_exit_80x(chip);
+out_irq_init:
+	return ret;
+}
+
+static int __devinit device_800_init(struct pm80x_chip *chip,
+				     struct i2c_client *i2c,
+				     struct pm80x_platform_data *pdata)
+{
+	struct regmap *map = dev_get_regmap(&i2c->dev, NULL);
+	int ret, data, pmic_id;
+
+	if (!map) {
+		dev_err(chip->dev, "regmap is invalid\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_read(map, PM800_CHIP_ID, &data);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to read CHIP ID: %d\n", ret);
+		goto out;
+	}
+
+	pmic_id = data & PM80X_VERSION_MASK;
+
+	if ((pmic_id >= PM800_CHIP_A0) && (pmic_id <= PM800_CHIP_END)) {
+		chip->version = data;
+		dev_info(chip->dev,
+			 "88PM80x:Marvell 88PM800 (ID:0x%x) detected\n", ret);
+	} else {
+		dev_err(chip->dev,
+			"Failed to detect Marvell 88PM800:ChipID[0x%x]\n", ret);
+		goto out;
+	}
+
+	/*
+	 * alarm wake up bit will be clear in device_irq_init(),
+	 * read before that
+	 */
+	ret = regmap_read(map, PM800_RTC_CONTROL, &data);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to read RTC register: %d\n", ret);
+		goto out;
+	}
+	if (data & PM800_ALARM_WAKEUP) {
+		if (pdata && pdata->rtc)
+			pdata->rtc->rtc_wakeup = 1;
+	}
+
+	ret = device_gpadc_init(chip, pdata);
+	if (ret < 0) {
+		dev_err(chip->dev, "[%s]Failed to init gpadc\n", __func__);
+		goto out;
+	}
+
+	chip->regmap_irq_chip = &pm800_irq_chip;
+
+	ret = device_irq_init_80x(chip);
+	if (ret < 0) {
+		dev_err(chip->dev, "[%s]Failed to init pm800 irq\n", __func__);
+		goto out;
+	}
+
+	ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
+			      ARRAY_SIZE(onkey_devs), &onkey_resources[0],
+			      chip->irq_base);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to add onkey subdev\n");
+		goto out_dev;
+	} else
+		dev_info(chip->dev, "[%s]:Added mfd onkey_devs\n", __func__);
+
+	if (pdata && pdata->rtc) {
+		rtc_devs[0].platform_data = pdata->rtc;
+		rtc_devs[0].pdata_size = sizeof(struct pm80x_rtc_pdata);
+		ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
+				      ARRAY_SIZE(rtc_devs), NULL,
+				      chip->irq_base);
+		if (ret < 0) {
+			dev_err(chip->dev, "Failed to add rtc subdev\n");
+			goto out_dev;
+		} else
+			dev_info(chip->dev,
+				 "[%s]:Added mfd rtc_devs\n", __func__);
+	}
+
+	if (pdata->pm80x_plat_config)
+		pdata->pm80x_plat_config(chip, pdata);
+
+	return 0;
+out_dev:
+	mfd_remove_devices(chip->dev);
+	device_irq_exit_80x(chip);
+out:
+	return ret;
+}
+
+int __devinit pm80x_device_init(struct pm80x_chip *chip,
+				struct pm80x_platform_data *pdata)
+{
+	int ret = 0;
+
+	switch (chip->id) {
+	case CHIP_PM800:
+		ret = device_800_init(chip, chip->client, pdata);
+		break;
+	case CHIP_PM805:
+		ret = device_805_init(chip, chip->client, pdata);
+		break;
+	default:
+		dev_err(chip->dev, "%s incorrect chip id!\n", __func__);
+		return -EINVAL;
+	}
+
+	if (ret)
+		dev_err(chip->dev, "%s id 0x%x failed!\n", __func__, chip->id);
+
+	return ret;
+}
+
+void __devexit pm80x_device_exit(struct pm80x_chip *chip)
+{
+	device_irq_exit_80x(chip);
+	mfd_remove_devices(chip->dev);
+}
+
+MODULE_DESCRIPTION("PMIC Driver for Marvell 88PM80x");
+MODULE_AUTHOR("Qiao Zhou <[email protected]>");
+MODULE_LICENSE("GPL");
drivers/mfd/88pm80x-i2c.c new file +274 -0

274 lines not on this page · raw message

drivers/mfd/Kconfig +12 -0
index e129c82..96dd2d7 100644
@@ -20,6 +20,18 @@ config MFD_88PM860X
 	  select individual components like voltage regulators, RTC and
 	  battery-charger under the corresponding menus.
 
+config MFD_88PM80X
+	bool "Support Marvell 88PM800/88PM805"
+	depends on I2C=y && GENERIC_HARDIRQS
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  This supports for Marvell 88PM800/88PM805 Power Management IC.
+	  This includes the I2C driver and the core APIs _only_, you have to
+	  select individual components like voltage regulators, RTC and
+	  battery-charger under the corresponding menus.
+
 config MFD_SM501
 	tristate "Support for Silicon Motion SM501"
 	 ---help---
drivers/mfd/Makefile +2 -0
index 75f6ed6..bf5de13 100644
@@ -3,7 +3,9 @@
 #
 
 88pm860x-objs			:= 88pm860x-core.o 88pm860x-i2c.o
+88pm80x-objs			:= 88pm80x-core.o 88pm80x-i2c.o
 obj-$(CONFIG_MFD_88PM860X)	+= 88pm860x.o
+obj-$(CONFIG_MFD_88PM80X)	+= 88pm80x.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
include/linux/mfd/88pm80x.h new file +678 -0

678 lines not on this page · raw message

Signature · 1 line
1.7.0.4
#2 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 4 lines · Qiao Zhou

88PM800 and 88PM805 are two discrete chips used for power management. Hardware designer can use them together or only one of them according to requirement.

This looks much better than the first version, great work!

However, there are a few things that you have not addressed from my review and that you did not explain either. Most importantly, you said that you'd add support for DT based booting, which is not in this version.

What platform is this driver for?
- If it's for an existing platform that does not have DT support, please include the platform changes for that platform so we can see what gets done in there.
- If it's for a new platform, you should not have any platform data in this patch, just device tree based probing because that platform will also have to use DT probing in order to get merged. The tree that carries such a platform out of the kernel can add back the platform data parsing, just don't submit it upstream with the main driver.

Quoted 4 lines

due to hardware design, there are some registers used by pm805, but actually defined in pm800 chip. so pm805 needs to access pm800 register in some occasion. A workaround is used in driver to solve such issue and would be removed when it's fixed in latter new chip version.

I don't see from the code what this is referring to. Can you be a little more specific? Ideally you would have one patch that adds the driver without this workaround, followed by a patch that just adds that workaround with an explanation why it's needed, and then you can later just revert that patch once it's no longer required.

Quoted 5 lines

All I2C operations are accessed by 80x-i2c driver, and register access is via regmap interface.

The benefit is that client drivers only need one kind of read/write API. I2C and MFD driver can be shared in both 800 and 805.

I'm not sure what the purpose of this split is. Usually you only separate out the i2c parts if the same driver has multiple host-side interfaces, e.g. i2c and spi. If your driver only has one of them, it's easier to just have one file. Using regmap however seems to be a good idea nonetheless.

I would also suggest you consider splitting the driver into three separate parts after moving the i2c file into the core file:

a) one "library" that contains the common code and exports symbols
b) one driver for 805 that contains just the 805 specific parts and registers the i2c driver for 805
c) one driver for 800 that contains just the 800 specific parts and registers the i2c driver for 800

Does that sound reasonable?

There is very little common that is actually part of the 88pm80x_common file, everything else is specific to just one of the variants. The only common parts I found are device_irq_init_80x and device_irq_exit_80x, and even for those I'm not sure if it wouldn't be easier to just
have separate functions.

The common parts seem to be mostly in your current _i2c.c file, so that could instead become the common file when you split 805 from 800. You basically just export the pm_ops and the probe/remove functions and let that be called from the individual drivers.

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (pdata->pm80x_plat_config)
+		pdata->pm80x_plat_config(chip, pdata);

You should definitely remove these callbacks, as I pointed out in the initial review.

Quoted 6 lines
drivers/mfd/88pm80x-core.c new file +6 -0
+int __devinit pm80x_device_init(struct pm80x_chip *chip,
+				struct pm80x_platform_data *pdata)
+{
+	int ret = 0;
+
+	switch (chip->id) {

drivers/mfd/88pm80x-core.c line 529

Quoted 10 lines
drivers/mfd/88pm80x-core.c new file +10 -0
+	case CHIP_PM800:
+		ret = device_800_init(chip, chip->client, pdata);
+		break;
+	case CHIP_PM805:
+		ret = device_805_init(chip, chip->client, pdata);
+		break;
+	default:
+		dev_err(chip->dev, "%s incorrect chip id!\n", __func__);
+		return -EINVAL;
+	}

It seems strange that you access the i2c_client in the common file, rather than just working with the regmap.

include/linux/mfd/88pm80x.h

Quoted 4 lines
include/linux/mfd/88pm80x.h new file +0 -1
new file mode 100644
index 0000000..9e2e11d
--- /dev/null

A lot of the contents of this file should not really be globally visible. Most of the register definitions are just used by the common mfd driver, so I would put them in a header file in the mfd directory or (even better) just into the .c file that uses them.

Arnd

#3 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 4 lines · Arnd Bergmann

What platform is this driver for?
- If it's for an existing platform that does not have DT support, please include the platform changes for that platform so we can see what gets done in there.

This is really not normal practice for device drivers, it'd be a serious
obstacle to getting drivers integrated if we have to have a board merged
with every driver.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120628/85bd8638/attachment.sig>

#4 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 11 lines · Mark Brown

Show Details

Quoted 4 lines · Arnd Bergmann

What platform is this driver for?
- If it's for an existing platform that does not have DT support, please include the platform changes for that platform so we can see what gets done in there.

This is really not normal practice for device drivers, it'd be a serious obstacle to getting drivers integrated if we have to have a board merged with every driver.

But it would be very helpful to see how the platform data is set, especially with the callback. If the callback is just there to set up a regulator or clock, then it should be changed to a more generic way.

Arnd

#5 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 13 lines · Arnd Bergmann
Quoted 11 lines · Mark Brown

Show Details

Quoted 4 lines · Arnd Bergmann

What platform is this driver for?
?- If it's for an existing platform that does not have DT support, please ? ?include the platform changes for that platform so we can see what gets ? ?done in there.

This is really not normal practice for device drivers, it'd be a serious obstacle to getting drivers integrated if we have to have a board merged with every driver.

Quoted 5 lines

But it would be very helpful to see how the platform data is set, especially with the callback. If the callback is just there to set up a regulator or clock, then it should be changed to a more generic way.

? ? ? ?Arnd

No, the callbacks is not used to set up a regulator or clock. They're used to configure the logic that are not integrated into drivers yet. For example, one special regulator needs active in sleep mode; some power saving configuration with board; accessing special register for fuse or OTP, .... Could we set up milestones for this? I agree that we need to move to DT support. But we have the interface of OF_DEV_AUXDATA in machine driver. We can use them to deliver platform data even with callback into drivers.

By the way, this PMIC is installed on new platforms that are not submitted patches
into mainline yet. They're used and verified in internal code without DT support. The
plan is submitting PMIC pathces before the new platform. If we only support DT, we
can't verify these code.

Regards
Haojian

#6 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 9 lines · Haojian Zhuang
Quoted 3 lines · Arnd Bergmann

But it would be very helpful to see how the platform data is set, especially with the callback. If the callback is just there to set up a regulator or clock, then it should be changed to a more generic way.

No, the callbacks is not used to set up a regulator or clock. They're used to configure the logic that are not integrated into drivers yet. For example, one special regulator needs active in sleep mode; some power saving configuration

This is a *totally* normal pattern for PMICs, normally the things that
are being configured by Linux would be configured prior to Linux
starting and the callback is basically there for overriding things so we
can fix up mistakes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120629/bd351af7/attachment.sig>

#7 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

In reply to Haojian Zhuang
Quoted 18 lines · Haojian Zhuang
Quoted 9 lines · Arnd Bergmann
Quoted 8 lines · Mark Brown

Show Details

Quoted 4 lines · Arnd Bergmann

What platform is this driver for?
- If it's for an existing platform that does not have DT support, please include the platform changes for that platform so we can see what gets done in there.

Quoted 7 lines
Quoted 3 lines

This is really not normal practice for device drivers, it'd be a serious obstacle to getting drivers integrated if we have to have a board merged with every driver.

But it would be very helpful to see how the platform data is set, especially with the callback. If the callback is just there to set up a regulator or clock, then it should be changed to a more generic way.

Quoted 7 lines

No, the callbacks is not used to set up a regulator or clock. They're used to configure the logic that are not integrated into drivers yet. For example, one special regulator needs active in sleep mode; some power saving configuration with board; accessing special register for fuse or OTP, .... Could we set up milestones for this? I agree that we need to move to DT support. But we have the interface of OF_DEV_AUXDATA in machine driver. We can use them to deliver platform data even with callback into drivers.

Yes, this is complex enough that using auxdata sounds like the right approach in this case for now, thanks for the explanation.

Quoted 7 lines

By the way, this PMIC is installed on new platforms that are not submitted patches
into mainline yet. They're used and verified in internal code without DT support. The
plan is submitting PMIC pathces before the new platform. If we only support DT, we
can't verify these code.

Well, as long as no platform in the mainline kernel supports this driver, it does not matter the upstream maintainers whether the code has been verified. The more important question is whether it is done the right way.

My usual recommendation for cases like this is to submit the driver in the way it would be written if all the other code using it was already perfect. In your internal verification, you already carry patches to add the new platform support, so you can add more patches to work around missing parts of the driver, like adding back the platform_data. What is more important for you is that you get as much as possible upstream in a version that has been reviewed and acked by the relevant maintainers.

I guess it's ok here to leave the platform data in for now, as it sounds that it can take a longer time (if ever) to fully eliminate it. However, I think it would be nice to also add preliminary DT bindings for the things that can be DT properties.

Arnd

author
#8 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

In reply to Arnd Bergmann
Quoted 12 lines · Arnd Bergmann
Quoted 4 lines · Qiao Zhou

88PM800 and 88PM805 are two discrete chips used for power management. Hardware designer can use them together or only one of them according to requirement.

This looks much better than the first version, great work!

However, there are a few things that you have not addressed from my review and that you did not explain either. Most importantly, you said that you'd add support for DT based booting, which is not in this version.

Quoted 9 lines

What platform is this driver for?
- If it's for an existing platform that does not have DT support, please include the platform changes for that platform so we can see what gets done in there.
- If it's for a new platform, you should not have any platform data in this patch, just device tree based probing because that platform will also have to use DT probing in order to get merged. The tree that carries such a platform out of the kernel can add back the platform data parsing, just don't submit it upstream with the main driver.

it's for a new platform as Haojian has commented. currently we can't only support DT for verifying issue.

Quoted 11 lines
Quoted 5 lines

due to hardware design, there are some registers used by pm805, but actually defined in pm800 chip. so pm805 needs to access pm800 register in some occasion. A workaround is used in driver to solve such issue and would be removed when it's fixed in latter new chip version.

I don't see from the code what this is referring to. Can you be a little more specific? Ideally you would have one patch that adds the driver without this workaround, followed by a patch that just adds that workaround with an explanation why it's needed, and then you can later just revert that patch once it's no longer required.

would update and give details to explain.

Quoted 29 lines
Quoted 6 lines

All I2C operations are accessed by 80x-i2c driver, and register access is via regmap interface.

The benefit is that client drivers only need one kind of read/write API. I2C and MFD driver can be shared in both 800 and 805.

I'm not sure what the purpose of this split is. Usually you only separate out the i2c parts if the same driver has multiple host-side interfaces, e.g. i2c and spi. If your driver only has one of them, it's easier to just have one file. Using regmap however seems to be a good idea nonetheless.

I would also suggest you consider splitting the driver into three separate parts after moving the i2c file into the core file:

a) one "library" that contains the common code and exports symbols
b) one driver for 805 that contains just the 805 specific parts and registers the i2c driver for 805
c) one driver for 800 that contains just the 800 specific parts and registers the i2c driver for 800

Does that sound reasonable?

There is very little common that is actually part of the 88pm80x_common file, everything else is specific to just one of the variants. The only common parts I found are device_irq_init_80x and device_irq_exit_80x, and even for those I'm not sure if it wouldn't be easier to just
have separate functions.

Quoted 4 lines

The common parts seem to be mostly in your current _i2c.c file, so that could instead become the common file when you split 805 from 800. You basically just export the pm_ops and the probe/remove functions and let that be called from the individual drivers.

Arnd, the comments is inaccurate for I2C operations. actually all I2C operations are handled by regmap in the new driver. and no more API needs to be exported by 88pm80x-i2c driver. driver which needs to access registers only uses uses regmap handle via dev_get_regmap, only i2c client needed. actually there is no split in I2C operations. please let me know if I miss your meaning.

in the 88pm80x_core.c, actually it only has five functions, irq_init_80x, irq_exit_80x, dev_init_800, dev_init_805, dev_exit_80x. there is indeed few common part. so I assume your meaning is that:
creating 88pm800_core.c and 88pm805_core.c separtely, which implement irq_init, irq_exit, dev_init, dev_exit for its own chip. is that correct?

Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+	if (pdata->pm80x_plat_config)
+		pdata->pm80x_plat_config(chip, pdata);

You should definitely remove these callbacks, as I pointed out in the initial review.

would remove it.

Quoted 12 lines
Quoted 12 lines
drivers/mfd/88pm80x-core.c new file +11 -0
+int __devinit pm80x_device_init(struct pm80x_chip *chip,
+				struct pm80x_platform_data *pdata)
+{
+	int ret = 0;
+
+	switch (chip->id) {
+	case CHIP_PM800:
+		ret = device_800_init(chip, chip->client, pdata);
+		break;
+	case CHIP_PM805:
+		ret = device_805_init(chip, chip->client, pdata);
Quoted 8 lines

drivers/mfd/88pm80x-core.c line 529

Quoted 5 lines
drivers/mfd/88pm80x-core.c new file +5 -0
+		break;
+	default:
+		dev_err(chip->dev, "%s incorrect chip id!\n", __func__);
+		return -EINVAL;
+	}

It seems strange that you access the i2c_client in the common file, rather than just working with the regmap.

would remove it.

Quoted 11 lines

include/linux/mfd/88pm80x.h

Quoted 6 lines
include/linux/mfd/88pm80x.h new file +0 -1
new file mode 100644
index 0000000..9e2e11d
--- /dev/null

A lot of the contents of this file should not really be globally visible. Most of the register definitions are just used by the common mfd driver, so I would put them in a header file in the mfd directory or (even better) just into the .c file that uses them.

yes, would category these registers. some registers which is used by regulator/rtc/onkey/codec/headset det/MISC would be kept in this header file, while others would be removed from globally visibility.

Quoted 4 lines

Arnd

Arnd,

thanks for your review & good suggestions!

Signature · 2 lines
Best Regards
Qiao
#9 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 35 lines · Qiao Zhou
Quoted 28 lines · Arnd Bergmann
Quoted 5 lines

All I2C operations are accessed by 80x-i2c driver, and register access is via regmap interface.

The benefit is that client drivers only need one kind of read/write API. I2C and MFD driver can be shared in both 800 and 805.

I'm not sure what the purpose of this split is. Usually you only separate out the i2c parts if the same driver has multiple host-side interfaces, e.g. i2c and spi. If your driver only has one of them, it's easier to just have one file. Using regmap however seems to be a good idea nonetheless.

I would also suggest you consider splitting the driver into three separate parts after moving the i2c file into the core file:

a) one "library" that contains the common code and exports symbols
b) one driver for 805 that contains just the 805 specific parts and registers the i2c driver for 805
c) one driver for 800 that contains just the 800 specific parts and registers the i2c driver for 800

Does that sound reasonable?

There is very little common that is actually part of the 88pm80x_common file, everything else is specific to just one of the variants. The only common parts I found are device_irq_init_80x and device_irq_exit_80x, and even for those I'm not sure if it wouldn't be easier to just
have separate functions.

Quoted 4 lines

The common parts seem to be mostly in your current _i2c.c file, so that could instead become the common file when you split 805 from 800. You basically just export the pm_ops and the probe/remove functions and let that be called from the individual drivers.

Quoted 6 lines

Arnd, the comments is inaccurate for I2C operations. actually all I2C operations are handled by regmap in the new driver. and no more API needs to be exported by 88pm80x-i2c driver. driver which needs to access registers only uses uses regmap handle via dev_get_regmap, only i2c client needed. actually there is no split in I2C operations. please let me know if I miss your meaning.

The point was something else: The regmap always uses i2c behind the covers, unlike drivers that have a common file using regmap to abstract the interface so that they can be used with either i2c or spi.

Quoted 5 lines

in the 88pm80x_core.c, actually it only has five functions, irq_init_80x, irq_exit_80x, dev_init_800, dev_init_805, dev_exit_80x. there is indeed few common part. so I assume your meaning is that:
creating 88pm800_core.c and 88pm805_core.c separtely, which implement irq_init, irq_exit, dev_init, dev_exit for its own chip. is that correct?

Right. However, I would go further and swap the layering of the driver
parts: Right now, a common pm08x_probe from the common i2c_driver structure calls the specific device_800_init and device_805_init through a global pm80x_device_init. When you do the split, I would move the i2c_driver structure to each of the two 88pm80?_core.c files and have an individual probe function in them that calls the common pm08x_probe and then its own device_init.

This is the layering that most other subsystems use when you have drivers sharing some common code.

Quoted 9 lines
Quoted 9 lines

include/linux/mfd/88pm80x.h

Quoted 4 lines
include/linux/mfd/88pm80x.h new file +0 -1
new file mode 100644
index 0000000..9e2e11d
--- /dev/null

A lot of the contents of this file should not really be globally visible. Most of the register definitions are just used by the common mfd driver, so I would put them in a header file in the mfd directory or (even better) just into the .c file that uses them.

Quoted 3 lines

yes, would category these registers. some registers which is used by regulator/rtc/onkey/codec/headset det/MISC would be kept in this header file, while others would be removed from globally visibility.

Ok, sounds good.

Arnd

author
#10 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 43 lines · Arnd Bergmann
Quoted 35 lines · Qiao Zhou
Quoted 28 lines · Arnd Bergmann
Quoted 5 lines

All I2C operations are accessed by 80x-i2c driver, and register access is via regmap interface.

The benefit is that client drivers only need one kind of read/write API. I2C and MFD driver can be shared in both 800 and 805.

I'm not sure what the purpose of this split is. Usually you only separate out the i2c parts if the same driver has multiple host-side interfaces, e.g. i2c and spi. If your driver only has one of them, it's easier to just have one file. Using regmap however seems to be a good idea nonetheless.

I would also suggest you consider splitting the driver into three separate parts after moving the i2c file into the core file:

a) one "library" that contains the common code and exports symbols
b) one driver for 805 that contains just the 805 specific parts and registers the i2c driver for 805
c) one driver for 800 that contains just the 800 specific parts and registers the i2c driver for 800

Does that sound reasonable?

There is very little common that is actually part of the 88pm80x_common file, everything else is specific to just one of the variants. The only common parts I found are device_irq_init_80x and device_irq_exit_80x, and even for those I'm not sure if it wouldn't be easier to just
have separate functions.

Quoted 4 lines

The common parts seem to be mostly in your current _i2c.c file, so that could instead become the common file when you split 805 from 800. You basically just export the pm_ops and the probe/remove functions and let that be called from the individual drivers.

Quoted 6 lines

Arnd, the comments is inaccurate for I2C operations. actually all I2C operations are handled by regmap in the new driver. and no more API needs to be exported by 88pm80x-i2c driver. driver which needs to access registers only uses uses regmap handle via dev_get_regmap, only i2c client needed. actually there is no split in I2C operations. please let me know if I miss your meaning.

Quoted 3 lines

The point was something else: The regmap always uses i2c behind the covers, unlike drivers that have a common file using regmap to abstract the interface so that they can be used with either i2c or spi.

Arnd, understand your meaning. driver just exports the API while callers doesn't need to know whether it's via i2c or spi in lower driver. It makes sense. thanks!

Here I have a question about the implementation specifically with 88pm800. this chip has 3 register pages, and 3 i2c address corresponding to each page. so it requires an additional parameter to point out the page(i2c) to be accessed. currently I didn't think of a good API to export for such purpose. seems to me, the 88pm800 chip is already bound to i2c interface, and using regmap directly is a better solution. could you give some suggestions?

Quoted 15 lines
Quoted 6 lines

in the 88pm80x_core.c, actually it only has five functions, irq_init_80x, irq_exit_80x, dev_init_800, dev_init_805, dev_exit_80x. there is indeed few common part. so I assume your meaning is that:
creating 88pm800_core.c and 88pm805_core.c separtely, which implement irq_init, irq_exit, dev_init, dev_exit for its own chip. is that correct?

Right. However, I would go further and swap the layering of the driver
parts: Right now, a common pm08x_probe from the common i2c_driver structure calls the specific device_800_init and device_805_init through a global pm80x_device_init. When you do the split, I would move the i2c_driver structure to each of the two 88pm80?_core.c files and have an individual probe function in them that calls the common pm08x_probe and then its own device_init.

Quoted 2 lines

This is the layering that most other subsystems use when you have drivers sharing some common code.

would update according to suggestion. thanks!

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

include/linux/mfd/88pm80x.h

Quoted 5 lines
include/linux/mfd/88pm80x.h new file +0 -1
new file mode 100644
index 0000000..9e2e11d
--- /dev/null

A lot of the contents of this file should not really be globally visible. Most of the register definitions are just used by the common mfd driver, so I would put them in a header file in the mfd directory or (even better) just into the .c file that uses them.

Quoted 8 lines
Quoted 3 lines

yes, would category these registers. some registers which is used by regulator/rtc/onkey/codec/headset det/MISC would be kept in this header file, while others would be removed from globally visibility.

Ok, sounds good.

Arnd

Arnd, thanks for your suggestions!

Signature · 2 lines
Best Regards
Qiao
author
#11 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 62 lines · Qiao Zhou
Quoted 53 lines · Arnd Bergmann
Quoted 45 lines · Qiao Zhou
Quoted 35 lines · Arnd Bergmann
Quoted 7 lines

All I2C operations are accessed by 80x-i2c driver, and register access is via
regmap interface.

The benefit is that client drivers only need one kind of read/write API. I2C
and MFD driver can be shared in both 800 and 805.

I'm not sure what the purpose of this split is. Usually you only separate out
the i2c parts if the same driver has multiple host-side interfaces, e.g. i2c
and spi. If your driver only has one of them, it's easier to just have one
file. Using regmap however seems to be a good idea nonetheless.

I would also suggest you consider splitting the driver into three separate
parts after moving the i2c file into the core file:

a) one "library" that contains the common code and exports symbols
b) one driver for 805 that contains just the 805 specific parts and registers the i2c driver for 805
c) one driver for 800 that contains just the 800 specific parts and registers the i2c driver for 800

Does that sound reasonable?

There is very little common that is actually part of the 88pm80x_common file, everything else is specific to just one of the variants. The only common
parts I found are device_irq_init_80x and device_irq_exit_80x, and even for those I'm not sure if it wouldn't be easier to just have separate functions.

Quoted 7 lines

The common parts seem to be mostly in your current _i2c.c file, so that could
instead become the common file when you split 805 from 800. You basically
just export the pm_ops and the probe/remove functions and let that be called
from the individual drivers.

Quoted 6 lines

Arnd, the comments is inaccurate for I2C operations. actually all I2C operations are handled by regmap in the new driver. and no more API needs to be exported by 88pm80x-i2c driver. driver which needs to access registers only uses uses regmap handle via dev_get_regmap, only i2c client needed. actually there is no split in I2C operations. please let me know if I miss your meaning.

Quoted 4 lines

The point was something else: The regmap always uses i2c behind the covers,
unlike drivers that have a common file using regmap to abstract the interface so that they can be used with either i2c or spi.

Arnd, understand your meaning. driver just exports the API while callers doesn't need to know whether it's via i2c or spi in lower driver. It makes sense. thanks!

Quoted 7 lines

Here I have a question about the implementation specifically with 88pm800. this chip has 3 register pages, and 3 i2c address corresponding to each page. so it requires an additional parameter to point out the page(i2c) to be accessed. currently I didn't think of a good API to export for such purpose. seems to me, the 88pm800 chip is already bound to i2c interface, and using regmap directly is a better solution. could you give some suggestions?

is it OK to export another two groups of r/w interface for gpadc and power page separately in 88pm800? such as pm800_gpadc_read_reg() / pm800_power_read_reg() etc? Appreciate any comments.

Quoted 43 lines
Quoted 17 lines
Quoted 7 lines

in the 88pm80x_core.c, actually it only has five functions, irq_init_80x, irq_exit_80x, dev_init_800, dev_init_805, dev_exit_80x. there is indeed few common part. so I assume your meaning is that:
creating 88pm800_core.c and 88pm805_core.c separtely, which implement irq_init, irq_exit, dev_init, dev_exit for its own chip. is that correct?

Right. However, I would go further and swap the layering of the driver
parts: Right now, a common pm08x_probe from the common i2c_driver structure
calls the specific device_800_init and device_805_init through a global pm80x_device_init. When you do the split, I would move the i2c_driver structure to each of the two 88pm80?_core.c files and have an individual probe function in them that calls the common pm08x_probe and then its own device_init.

Quoted 2 lines

This is the layering that most other subsystems use when you have drivers sharing some common code.

would update according to suggestion. thanks!

Quoted 13 lines
Quoted 13 lines
Quoted 13 lines

include/linux/mfd/88pm80x.h

Quoted 5 lines
include/linux/mfd/88pm80x.h new file +0 -1
new file mode 100644
index 0000000..9e2e11d
--- /dev/null

A lot of the contents of this file should not really be globally visible.
Most of the register definitions are just used by the common mfd driver,
so I would put them in a header file in the mfd directory or (even better)
just into the .c file that uses them.

Quoted 8 lines
Quoted 3 lines

yes, would category these registers. some registers which is used by regulator/rtc/onkey/codec/headset det/MISC would be kept in this header file, while others would be removed from globally visibility.

Ok, sounds good.

Arnd

Arnd, thanks for your suggestions!

Signature · 2 lines
Best Regards
Qiao
#12 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 10 lines · Qiao Zhou
Quoted 7 lines · Qiao Zhou

Here I have a question about the implementation specifically with 88pm800. this chip has 3 register pages, and 3 i2c address corresponding to each page. so it requires an additional parameter to point out the page(i2c) to be accessed. currently I didn't think of a good API to export for such purpose. seems to me, the 88pm800 chip is already bound to i2c interface, and using regmap directly is a better solution. could you give some suggestions?

Quoted 3 lines

is it OK to export another two groups of r/w interface for gpadc and power page separately in 88pm800? such as pm800_gpadc_read_reg() / pm800_power_read_reg() etc? Appreciate any comments.

What do you mean by pages? regmap has paging support which just maps
everything into a single flat register map from the point of view of
callers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120702/6fd60484/attachment.sig>

author
#13 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 11 lines · Mark Brown
Quoted 9 lines · Qiao Zhou
Quoted 7 lines · Qiao Zhou

Here I have a question about the implementation specifically with 88pm800. this chip has 3 register pages, and 3 i2c address corresponding to each page. so it requires an additional parameter to point out the page(i2c) to be accessed. currently I didn't think of a good API to export for such purpose. seems to me, the 88pm800 chip is already bound to i2c interface, and using regmap directly is a better solution. could you give some suggestions?

Quoted 8 lines
Quoted 3 lines

is it OK to export another two groups of r/w interface for gpadc and power page separately in 88pm800? such as pm800_gpadc_read_reg() / pm800_power_read_reg() etc? Appreciate any comments.

What do you mean by pages? regmap has paging support which just maps everything into a single flat register map from the point of view of callers.

Mark, let me explain: the 88pm800 chip has three i2c address internally, which we called different page instead. it confuses you with the register page_read/write operation. there are registers in each i2c address domain, and we need to use different i2c client to access reg in different domain. such as some common regs are in the page of i2c_addr = 0x30, and power related regs are in the page of i2c_addr = 0x31, and gpadc related regs are in the page of 0x32.

Signature · 2 lines
Best Regards
Qiao
#14 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 6 lines · Qiao Zhou
Quoted 3 lines · Mark Brown

What do you mean by pages? regmap has paging support which just maps everything into a single flat register map from the point of view of callers.

Quoted 7 lines

Mark, let me explain: the 88pm800 chip has three i2c address internally, which we called different page instead. it confuses you with the register page_read/write operation. there are registers in each i2c address domain, and we need to use different i2c client to access reg in different domain. such as some common regs are in the page of i2c_addr = 0x30, and power related regs are in the page of i2c_addr = 0x31, and gpadc related regs are in the page of 0x32.

These aren't what people normally call pages, those are just separate
I2C devices from a Linux point of view.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120702/7e53ff13/attachment.sig>

author
#15 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 15 lines · Mark Brown
Quoted 6 lines · Qiao Zhou
Quoted 3 lines · Mark Brown

What do you mean by pages? regmap has paging support which just maps everything into a single flat register map from the point of view of callers.

Quoted 7 lines

Mark, let me explain: the 88pm800 chip has three i2c address internally, which we called different page instead. it confuses you with the register page_read/write operation. there are registers in each i2c address domain, and we need to use different i2c client to access reg in different domain. such as some common regs are in the page of i2c_addr = 0x30, and power related regs are in the page of i2c_addr = 0x31, and gpadc related regs are in the page of 0x32.

Quoted 3 lines

These aren't what people normally call pages, those are just separate I2C devices from a Linux point of view.

Mark, surely I'll pay attention to the terms used. thanks! due to there separate I2C devices, does it make sense to export separate r/w interface for them? do you have suggestion in such case?

Signature · 2 lines
Best Regards
Qiao
#16 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 16 lines · Qiao Zhou
Quoted 15 lines · Mark Brown
Quoted 13 lines · Qiao Zhou
Quoted 3 lines · Mark Brown

What do you mean by pages? regmap has paging support which just maps everything into a single flat register map from the point of view of callers.

Mark, let me explain: the 88pm800 chip has three i2c address internally, which we called different page instead. it confuses you with the register page_read/write operation. there are registers in each i2c address domain, and we need to use different i2c client to access reg in different domain. such as some common regs are in the page of i2c_addr = 0x30, and power related regs are in the page of i2c_addr = 0x31, and gpadc related regs are in the page of 0x32.

Quoted 6 lines
Quoted 3 lines

These aren't what people normally call pages, those are just separate I2C devices from a Linux point of view.

Mark, surely I'll pay attention to the terms used. thanks! due to there separate I2C devices, does it make sense to export separate r/w interface for them? do you have suggestion in such case?

(adding the i2c mailing list to get more insight)

I think in case of device tree based probing, it would be straightforward to represent 88pm800 as a single device with three addresses in the "reg" property, while the natural linux representation would be one regular i2c_client device with two dummies. Do we or should we have any infrastructure to deal with this?

If this is a common scenario, we could probably let regmap handle it entirely internally and represent the i2c client with its dummies as a single regmap.

Arnd

author
#17 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 32 lines · Arnd Bergmann
Quoted 16 lines · Qiao Zhou
Quoted 15 lines · Mark Brown
Quoted 13 lines · Qiao Zhou
Quoted 3 lines · Mark Brown

What do you mean by pages? regmap has paging support which just maps everything into a single flat register map from the point of view of callers.

Mark, let me explain: the 88pm800 chip has three i2c address internally, which we called different page instead. it confuses you with the register page_read/write operation. there are registers in each i2c address domain, and we need to use different i2c client to access reg in different domain. such as some common regs are in the page of i2c_addr = 0x30, and power related regs are in the page of i2c_addr = 0x31, and gpadc related regs are in the page of 0x32.

Quoted 6 lines
Quoted 3 lines

These aren't what people normally call pages, those are just separate I2C devices from a Linux point of view.

Mark, surely I'll pay attention to the terms used. thanks! due to there separate I2C devices, does it make sense to export separate r/w interface for them? do you have suggestion in such case?

(adding the i2c mailing list to get more insight)

I think in case of device tree based probing, it would be straightforward to represent 88pm800 as a single device with three addresses in the "reg" property, while the natural linux representation would be one regular i2c_client device with two dummies. Do we or should we have any infrastructure to deal with this?

Quoted 3 lines

If this is a common scenario, we could probably let regmap handle it entirely internally and represent the i2c client with its dummies as a single regmap.

actually there are many drivers under mfd which have this common issue, which has i2c dummy devices, such as max77693.c, max8925-i2c.c, ab3100-core.c, max8997.c, max8998.c, s5m-core.c etc. some use regmap handle directly as param in exported r/w api, some add extra param to differentiate i2c dummy. it seems to be a common scenario. how do we handle the API in short term and long term?

Quoted 3 lines

Arnd

Signature · 2 lines
Best Regards
Qiao
#18 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

In reply to Qiao Zhou

drivers/mfd/88pm80x-core.c line 341

Quoted 3 lines · Qiao Zhou
drivers/mfd/88pm80x-core.c new file +3 -0
+       ret =
+           regmap_add_irq_chip(chip->regmap, chip->irq, flags, chip->irq_base,
+                               chip->regmap_irq_chip, &chip->irq_data);

One more question I have about this, mainly for Mark:

I understand that by passing -1 into regmap_add_irq_chip, it should allocate its own linear irq domain rather using a legacy domain. Does that require any other changes? Should the drivers that use the irq number just use regmap_irq_get_virq() to get the linux irq number, or is there more to do here?

Arnd

#19 lore raw

PATCH 1/3 mfd: support 88pm80x in 80x driver

Quoted 7 lines · Arnd Bergmann

One more question I have about this, mainly for Mark:

I understand that by passing -1 into regmap_add_irq_chip, it should allocate its own linear irq domain rather using a legacy domain. Does that require any other changes? Should the drivers that use the irq number just use regmap_irq_get_virq() to get the linux irq number, or is there more to do here?

That's it - the users don't need to worry about where the Linux IRQ
number came from, they can just call that function and it'll give them
something they can use regardless if it's a linear, legacy or something
else domain.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120630/315a6610/attachment.sig>

author
#20 lore raw

PATCH 2/3 rtc: add rtc support to 88PM80X PMIC

In reply to Qiao Zhou

add rtc driver for MARVELL 88PM80X PMIC and enable rtc function.

Signed-off-by Qiao Zhou <[email protected]>
---
 drivers/rtc/Kconfig       |   10 ++
 drivers/rtc/Makefile      |    1 +
 drivers/rtc/rtc-88pm80x.c |  370 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 381 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-88pm80x.c
drivers/rtc/Kconfig +10 -0
index 08cbdb9..f3b49f8 100644
@@ -135,6 +135,16 @@ config RTC_DRV_88PM860X
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-88pm860x.
 
+config RTC_DRV_88PM80X
+	tristate "Marvell 88PM80x"
+	depends on RTC_CLASS && I2C && MFD_88PM80X
+	help
+	  If you say yes here you get support for RTC function in Marvell
+	  88PM80x chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-88pm80x.
+
 config RTC_DRV_DS1307
 	tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025"
 	help
drivers/rtc/Makefile +1 -0
index 2973921..0d5b2b6 100644
@@ -16,6 +16,7 @@ rtc-core-$(CONFIG_RTC_INTF_SYSFS) += rtc-sysfs.o
 # Keep the list ordered.
 
 obj-$(CONFIG_RTC_DRV_88PM860X)  += rtc-88pm860x.o
+obj-$(CONFIG_RTC_DRV_88PM80X)	+= rtc-88pm80x.o
 obj-$(CONFIG_RTC_DRV_AB3100)	+= rtc-ab3100.o
 obj-$(CONFIG_RTC_DRV_AB8500)	+= rtc-ab8500.o
 obj-$(CONFIG_RTC_DRV_AT32AP700X)+= rtc-at32ap700x.o
drivers/rtc/rtc-88pm80x.c new file +370 -0

370 lines not on this page · raw message

Signature · 1 line
1.7.0.4
author
#21 lore raw

PATCH 3/3 input: add onkey support to 88PM80X PMIC

In reply to Qiao Zhou

add onkey support to MARVELL 88PM80X PMIC.

Signed-off-by Qiao Zhou <[email protected]>
---
 drivers/input/misc/88pm80x_onkey.c |  194 ++++++++++++++++++++++++++++++++++++
 drivers/input/misc/Kconfig         |   10 ++
 drivers/input/misc/Makefile        |    1 +
 3 files changed, 205 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/88pm80x_onkey.c
drivers/input/misc/88pm80x_onkey.c new file +194 -0
new file mode 100644
index 0000000..091bcf9
@@ -0,0 +1,194 @@
+/*
+ * 88pm80x_onkey.c - Marvell 88PM80x ONKEY driver
+ *
+ * Copyright (C) 2012 Marvell International Ltd.
+ * Haojian Zhuang <[email protected]>
+ * Qiao Zhou <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/88pm80x.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define PM800_LONG_ONKEY_EN		(1 << 0)
+#define PM800_LONG_KEY_DELAY		(8)	/* 1 .. 16 seconds */
+#define PM800_LONKEY_PRESS_TIME		((PM800_LONG_KEY_DELAY-1) << 4)
+#define PM800_LONKEY_PRESS_TIME_MASK	(0xF0)
+#define PM800_SW_PDOWN			(1 << 5)
+
+struct pm80x_onkey_info {
+	struct input_dev *idev;
+	struct pm80x_chip *pm80x;
+	struct i2c_client *i2c;
+	struct regmap *map;
+	int irq;
+};
+
+/* 88PM80x gives us an interrupt when ONKEY is held */
+static irqreturn_t pm80x_onkey_handler(int irq, void *data)
+{
+	struct pm80x_onkey_info *info = data;
+	int ret = 0;
+
+	regmap_read(info->map, PM800_STATUS_1, &ret);
+	ret &= PM800_ONKEY_STS1;
+
+	input_report_key(info->idev, KEY_POWER, ret);
+	input_sync(info->idev);
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_PM
+static int pm80x_onkey_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+
+	if (device_may_wakeup(dev))
+		chip->wu_flag |= (1 << PM800_IRQ_ONKEY);
+
+	return 0;
+}
+
+static int pm80x_onkey_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+
+	if (device_may_wakeup(dev))
+		chip->wu_flag &= ~(1 << PM800_IRQ_ONKEY);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pm80x_onkey_pm_ops, pm80x_onkey_suspend,
+			 pm80x_onkey_resume);
+
+static int __devinit pm80x_onkey_probe(struct platform_device *pdev)
+{
+
+	void *chip = dev_get_drvdata(pdev->dev.parent);
+	struct pm80x_onkey_info *info;
+	int ret;
+
+	info =
+	    devm_kzalloc(&pdev->dev, sizeof(struct pm80x_onkey_info),
+			 GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->pm80x = (struct pm80x_chip *)chip;
+	info->i2c = info->pm80x->client;
+
+	info->irq = platform_get_irq(pdev, 0);
+	if (info->irq < 0) {
+		dev_err(&pdev->dev, "No IRQ resource!\n");
+		return -EINVAL;
+	}
+
+
+	info->map = dev_get_regmap(&info->i2c->dev, NULL);
+	if (!info->map) {
+		dev_err(&pdev->dev, "no regmap!\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	info->idev = input_allocate_device();
+	if (!info->idev) {
+		dev_err(&pdev->dev, "Failed to allocate input dev\n");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	info->idev->name = "88pm80x_on";
+	info->idev->phys = "88pm80x_on/input0";
+	info->idev->id.bustype = BUS_I2C;
+	info->idev->dev.parent = &pdev->dev;
+	info->idev->evbit[0] = BIT_MASK(EV_KEY);
+	info->idev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
+
+	ret = input_register_device(info->idev);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't register input device: %d\n", ret);
+		goto out_reg;
+	}
+
+	ret = request_threaded_irq(info->irq, NULL, pm80x_onkey_handler,
+				   IRQF_ONESHOT, "onkey", info);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to request IRQ: #%d: %d\n",
+			info->irq, ret);
+		goto out_irq;
+	}
+
+	platform_set_drvdata(pdev, info);
+
+	/* Enable long onkey detection */
+	regmap_update_bits(info->map, PM800_RTC_MISC4, PM800_LONG_ONKEY_EN,
+			   PM800_LONG_ONKEY_EN);
+	/* Set 8-second interval */
+	regmap_update_bits(info->map, PM800_RTC_MISC3,
+			   PM800_LONKEY_PRESS_TIME_MASK,
+			   PM800_LONKEY_PRESS_TIME);
+
+	device_init_wakeup(&pdev->dev, 1);
+	return 0;
+
+out_irq:
+	input_unregister_device(info->idev);
+out_reg:
+	input_free_device(info->idev);
+out:
+	devm_kfree(&pdev->dev, info);
+	return ret;
+}
+
+static int __devexit pm80x_onkey_remove(struct platform_device *pdev)
+{
+	struct pm80x_onkey_info *info = platform_get_drvdata(pdev);
+
+	free_irq(info->irq, info);
+	input_unregister_device(info->idev);
+	input_free_device(info->idev);
+	devm_kfree(&pdev->dev, info);
+	return 0;
+}
+
+static struct platform_driver pm80x_onkey_driver = {
+	.driver = {
+		   .name = "88pm80x-onkey",
+		   .owner = THIS_MODULE,
+		   .pm = &pm80x_onkey_pm_ops,
+		   },
+	.probe = pm80x_onkey_probe,
+	.remove = __devexit_p(pm80x_onkey_remove),
+};
+
+module_platform_driver(pm80x_onkey_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Marvell 88PM80x ONKEY driver");
+MODULE_AUTHOR("Qiao Zhou <[email protected]>");
+MODULE_ALIAS("platform:88pm80x-onkey");
drivers/input/misc/Kconfig +10 -0
index 7faf4a7..8205a66 100644
@@ -22,6 +22,16 @@ config INPUT_88PM860X_ONKEY
 	  To compile this driver as a module, choose M here: the module
 	  will be called 88pm860x_onkey.
 
+config INPUT_88PM80X_ONKEY
+	tristate "88PM80x ONKEY support"
+	depends on MFD_88PM80X
+	help
+	  Support the ONKEY of Marvell 88PM80x PMICs as an input device
+	  reporting power button status.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called 88pm80x_onkey.
+
 config INPUT_AB8500_PONKEY
 	tristate "AB8500 Pon (PowerOn) Key"
 	depends on AB8500_CORE
drivers/input/misc/Makefile +1 -0
index f55cdf4..83fe6f5 100644
@@ -5,6 +5,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_INPUT_88PM860X_ONKEY)	+= 88pm860x_onkey.o
+obj-$(CONFIG_INPUT_88PM80X_ONKEY)	+= 88pm80x_onkey.o
 obj-$(CONFIG_INPUT_AB8500_PONKEY)	+= ab8500-ponkey.o
 obj-$(CONFIG_INPUT_AD714X)		+= ad714x.o
 obj-$(CONFIG_INPUT_AD714X_I2C)		+= ad714x-i2c.o
Signature · 1 line
1.7.0.4