[PATCH 0/2] Introduce QTI I2C PMIC controller

[PATCH 0/2] Introduce QTI I2C PMIC controller

[Guru Das Srinagesh]

Add devicetree bindings and code for the Qualcomm Technologies, Inc. I2C PMIC
controller.

Guru Das Srinagesh (2):
  dt-bindings: mfd: Document QTI I2C PMIC controller
  mfd: Introduce QTI I2C PMIC controller

 .../devicetree/bindings/mfd/qcom,i2c-pmic.yaml     |  89 +++
 drivers/mfd/Kconfig                                |  11 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/qcom-i2c-pmic.c                        | 737 +++++++++++++++++++++
 4 files changed, 838 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
 create mode 100644 drivers/mfd/qcom-i2c-pmic.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/2] dt-bindings: mfd: Document QTI I2C PMIC controller

[Guru Das Srinagesh]

The Qualcomm Technologies, Inc. I2C PMIC Controller is used by
multi-function PMIC devices which communicate over the I2C bus.  The
controller enumerates all child nodes as platform devices, and
instantiates a regmap interface for them to communicate over the I2C
bus.

The controller also controls interrupts for all of the children platform
devices.  The controller handles the summary interrupt by deciphering
which peripheral triggered the interrupt, and which of the peripheral
interrupts were triggered.  Finally, it calls the interrupt handlers for
each of the virtual interrupts that were registered.

Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 .../devicetree/bindings/mfd/qcom,i2c-pmic.yaml     | 89 ++++++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml

diff --git a/Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml b/Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
new file mode 100644
index 0000000..893552c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/qcom,i2c-pmic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. I2C PMIC Interrupt Controller Platform Independent Bindings
+
+description: |
+  The I2C PMIC Controller is used by multi-function PMIC devices which
+  communicate over the I2C bus. An I2C PMIC controller node typically contains
+  one or more child nodes representing the device's peripherals. Each of the
+  peripherals typically has its own driver on the platform bus and will be
+  enumerated by this controller. The controller exposes a regmap to the
+  peripherals to communicate over the I2C bus.
+
+  The controller also controls interrupts for all of the peripherals on the
+  bus. The controller takes a summary interrupt, deciphers which peripheral
+  triggered the interrupt, and which of the peripheral's interrupts were
+  triggered. Finally, it calls the handlers for each of the virtual interrupts
+  that were registered.
+
+  This document describes the common platform independent bindings that apply
+  to all I2C PMIC interrupt controllers.
+
+maintainers:
+  - Guru Das Srinagesh <gurus@codeaurora.org>
+
+properties:
+  compatible:
+    const: qcom,i2c-pmic
+
+  reg:
+    description: 7-bit I2C address of the device.
+    maxItems: 1
+
+  interrupts:
+    description: Summary interrupt specifier.
+
+  interrupt-controller:
+    description: Flag indicating that this device is an interrupt controller.
+
+  "#interrupt-cells":
+    description: Number of cells to encode an interrupt source.
+
+  qcom,periph-map:
+    description: |
+      A contiguous list of u32 arrays where each element specifies the base
+      address of a single peripheral within the chip. This provides a mapping
+      between the summary status register bits and peripheral addresses as each
+      bit in the summary status register represents a peripheral.
+
+      The number of arrays should match the number of summary registers with up
+      to 8 elements each. Within each array, One element per bit of the summary
+      status register in order from the least sigificant bit to the most
+      significant bit.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  pinctrl-names:
+    const: default
+
+  pinctrl-0:
+    description: phandle of the pin configuration.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    &i2c_3 {
+      status = "ok";
+        qcom,smb138x@8 {
+          compatible = "qcom,i2c-pmic";
+          reg = <0x8>;
+          interrupt-parent = <&tlmm_pinmux>;
+          interrupts = <83 0>;
+          interrupt-controller;
+          #interrupt-cells = <3>;
+          pinctrl-names = "default";
+          pinctrl-0 = <&smb_stat_active>;
+          #address-cells = <1>;
+          #size-cells = <0>;
+          qcom,periph-map = <0x10 0x11 0x12 0x13 0x14 0x16 0x36>;
+        };
+    };
+
+...
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] mfd: Introduce QTI I2C PMIC controller

[Guru Das Srinagesh]

The Qualcomm Technologies, Inc. I2C PMIC Controller is used by
multi-function PMIC devices which communicate over the I2C bus.  The
controller enumerates all child nodes as platform devices, and
instantiates a regmap interface for them to communicate over the I2C
bus.

The controller also controls interrupts for all of the children platform
devices.  The controller handles the summary interrupt by deciphering
which peripheral triggered the interrupt, and which of the peripheral
interrupts were triggered.  Finally, it calls the interrupt handlers for
each of the virtual interrupts that were registered.

Nicholas Troast is the original author of this driver.

Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 drivers/mfd/Kconfig         |  11 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/qcom-i2c-pmic.c | 737 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 749 insertions(+)
 create mode 100644 drivers/mfd/qcom-i2c-pmic.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 54b6aa4..bf112eb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1002,6 +1002,17 @@ config MFD_PM8XXX
 	  Say M here if you want to include support for PM8xxx chips as a
 	  module. This will build a module called "pm8xxx-core".
 
+config MFD_I2C_PMIC
+	tristate "QTI I2C PMIC support"
+	depends on I2C && OF
+	select IRQ_DOMAIN
+	select REGMAP_I2C
+	help
+	  This enables support for controlling Qualcomm Technologies, Inc.
+	  PMICs over I2C. The driver controls interrupts, and provides register
+	  access for all of the device's peripherals.  Some QTI PMIC chips
+	  support communication over both I2C and SPMI.
+
 config MFD_QCOM_RPM
 	tristate "Qualcomm Resource Power Manager (RPM)"
 	depends on ARCH_QCOM && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 7761f84..26f0b80 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -199,6 +199,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
 obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
 obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
 obj-$(CONFIG_MFD_PM8XXX) 	+= qcom-pm8xxx.o ssbi.o
+obj-$(CONFIG_MFD_I2C_PMIC)     += qcom-i2c-pmic.o
 obj-$(CONFIG_MFD_QCOM_RPM)	+= qcom_rpm.o
 obj-$(CONFIG_MFD_SPMI_PMIC)	+= qcom-spmi-pmic.o
 obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
diff --git a/drivers/mfd/qcom-i2c-pmic.c b/drivers/mfd/qcom-i2c-pmic.c
new file mode 100644
index 0000000..d0f600a
--- /dev/null
+++ b/drivers/mfd/qcom-i2c-pmic.c
@@ -0,0 +1,737 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "I2C PMIC: %s: " fmt, __func__
+
+#include <linux/bitops.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define I2C_INTR_STATUS_BASE	0x0550
+#define INT_RT_STS_OFFSET	0x10
+#define INT_SET_TYPE_OFFSET	0x11
+#define INT_POL_HIGH_OFFSET	0x12
+#define INT_POL_LOW_OFFSET	0x13
+#define INT_LATCHED_CLR_OFFSET	0x14
+#define INT_EN_SET_OFFSET	0x15
+#define INT_EN_CLR_OFFSET	0x16
+#define INT_LATCHED_STS_OFFSET	0x18
+#define INT_PENDING_STS_OFFSET	0x19
+#define INT_MID_SEL_OFFSET	0x1A
+#define INT_MID_SEL_MASK	GENMASK(1, 0)
+#define INT_PRIORITY_OFFSET	0x1B
+#define INT_PRIORITY_BIT	BIT(0)
+
+enum {
+	IRQ_SET_TYPE = 0,
+	IRQ_POL_HIGH,
+	IRQ_POL_LOW,
+	IRQ_LATCHED_CLR, /* not needed but makes life easy */
+	IRQ_EN_SET,
+	IRQ_MAX_REGS,
+};
+
+struct i2c_pmic_periph {
+	void		*data;
+	u16		addr;
+	u8		cached[IRQ_MAX_REGS];
+	u8		synced[IRQ_MAX_REGS];
+	u8		wake;
+	struct mutex	lock;
+};
+
+struct i2c_pmic {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct irq_domain	*domain;
+	struct i2c_pmic_periph	*periph;
+	struct pinctrl		*pinctrl;
+	struct mutex		irq_complete;
+	const char		*pinctrl_name;
+	int			num_periphs;
+	int			summary_irq;
+	bool			resume_completed;
+	bool			irq_waiting;
+};
+
+static void i2c_pmic_irq_bus_lock(struct irq_data *d)
+{
+	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
+
+	mutex_lock(&periph->lock);
+}
+
+static void i2c_pmic_sync_type_polarity(struct i2c_pmic *chip,
+			       struct i2c_pmic_periph *periph)
+{
+	int rc;
+
+	/* did any irq type change? */
+	if (periph->cached[IRQ_SET_TYPE] ^ periph->synced[IRQ_SET_TYPE]) {
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_SET_TYPE_OFFSET,
+				  periph->cached[IRQ_SET_TYPE]);
+		if (rc < 0) {
+			pr_err("Couldn't set periph 0x%04x irqs 0x%02x type rc=%d\n",
+				periph->addr, periph->cached[IRQ_SET_TYPE], rc);
+			return;
+		}
+
+		periph->synced[IRQ_SET_TYPE] = periph->cached[IRQ_SET_TYPE];
+	}
+
+	/* did any polarity high change? */
+	if (periph->cached[IRQ_POL_HIGH] ^ periph->synced[IRQ_POL_HIGH]) {
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_POL_HIGH_OFFSET,
+				  periph->cached[IRQ_POL_HIGH]);
+		if (rc < 0) {
+			pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity high rc=%d\n",
+				periph->addr, periph->cached[IRQ_POL_HIGH], rc);
+			return;
+		}
+
+		periph->synced[IRQ_POL_HIGH] = periph->cached[IRQ_POL_HIGH];
+	}
+
+	/* did any polarity low change? */
+	if (periph->cached[IRQ_POL_LOW] ^ periph->synced[IRQ_POL_LOW]) {
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_POL_LOW_OFFSET,
+				  periph->cached[IRQ_POL_LOW]);
+		if (rc < 0) {
+			pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity low rc=%d\n",
+				periph->addr, periph->cached[IRQ_POL_LOW], rc);
+			return;
+		}
+
+		periph->synced[IRQ_POL_LOW] = periph->cached[IRQ_POL_LOW];
+	}
+}
+
+static void i2c_pmic_sync_enable(struct i2c_pmic *chip,
+				 struct i2c_pmic_periph *periph)
+{
+	u8 en_set, en_clr;
+	int rc;
+
+	/* determine which irqs were enabled and which were disabled */
+	en_clr = periph->synced[IRQ_EN_SET] & ~periph->cached[IRQ_EN_SET];
+	en_set = ~periph->synced[IRQ_EN_SET] & periph->cached[IRQ_EN_SET];
+
+	/* were any irqs disabled? */
+	if (en_clr) {
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_EN_CLR_OFFSET, en_clr);
+		if (rc < 0) {
+			pr_err("Couldn't disable periph 0x%04x irqs 0x%02x rc=%d\n",
+				periph->addr, en_clr, rc);
+			return;
+		}
+	}
+
+	/* were any irqs enabled? */
+	if (en_set) {
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_EN_SET_OFFSET, en_set);
+		if (rc < 0) {
+			pr_err("Couldn't enable periph 0x%04x irqs 0x%02x rc=%d\n",
+				periph->addr, en_set, rc);
+			return;
+		}
+	}
+
+	/* irq enabled status was written to hardware */
+	periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
+}
+
+static void i2c_pmic_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
+	struct i2c_pmic *chip = periph->data;
+
+	i2c_pmic_sync_type_polarity(chip, periph);
+	i2c_pmic_sync_enable(chip, periph);
+	mutex_unlock(&periph->lock);
+}
+
+static void i2c_pmic_irq_disable(struct irq_data *d)
+{
+	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
+
+	periph->cached[IRQ_EN_SET] &= ~d->hwirq & 0xFF;
+}
+
+static void i2c_pmic_irq_enable(struct irq_data *d)
+{
+	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
+
+	periph->cached[IRQ_EN_SET] |= d->hwirq & 0xFF;
+}
+
+static int i2c_pmic_irq_set_type(struct irq_data *d, unsigned int irq_type)
+{
+	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
+
+	switch (irq_type) {
+	case IRQ_TYPE_EDGE_RISING:
+		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
+		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
+		break;
+	default:
+		pr_err("irq type 0x%04x is not supported\n", irq_type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int i2c_pmic_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
+
+	if (on)
+		periph->wake |= d->hwirq & 0xFF;
+	else
+		periph->wake &= ~d->hwirq & 0xFF;
+
+	return 0;
+}
+#else
+#define i2c_pmic_irq_set_wake NULL
+#endif
+
+static struct irq_chip i2c_pmic_irq_chip = {
+	.name			= "i2c_pmic_irq_chip",
+	.irq_bus_lock		= i2c_pmic_irq_bus_lock,
+	.irq_bus_sync_unlock	= i2c_pmic_irq_bus_sync_unlock,
+	.irq_disable		= i2c_pmic_irq_disable,
+	.irq_enable		= i2c_pmic_irq_enable,
+	.irq_set_type		= i2c_pmic_irq_set_type,
+	.irq_set_wake		= i2c_pmic_irq_set_wake,
+};
+
+static struct i2c_pmic_periph *i2c_pmic_find_periph(struct i2c_pmic *chip,
+						    irq_hw_number_t hwirq)
+{
+	int i;
+
+	for (i = 0; i < chip->num_periphs; i++)
+		if (chip->periph[i].addr == (hwirq & 0xFF00))
+			return &chip->periph[i];
+
+	pr_err_ratelimited("Couldn't find periph struct for hwirq 0x%04lx\n",
+			   hwirq);
+	return NULL;
+}
+
+static int i2c_pmic_domain_map(struct irq_domain *d, unsigned int virq,
+			irq_hw_number_t hwirq)
+{
+	struct i2c_pmic *chip = d->host_data;
+	struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
+
+	if (!periph)
+		return -ENODEV;
+
+	irq_set_chip_data(virq, periph);
+	irq_set_chip_and_handler(virq, &i2c_pmic_irq_chip, handle_level_irq);
+	irq_set_nested_thread(virq, 1);
+	irq_set_noprobe(virq);
+	return 0;
+}
+
+static int i2c_pmic_domain_xlate(struct irq_domain *d,
+				 struct device_node *ctrlr, const u32 *intspec,
+				 unsigned int intsize, unsigned long *out_hwirq,
+				 unsigned int *out_type)
+{
+	if (intsize != 3)
+		return -EINVAL;
+
+	if (intspec[0] > 0xFF || intspec[1] > 0x7 ||
+					intspec[2] > IRQ_TYPE_SENSE_MASK)
+		return -EINVAL;
+
+	/*
+	 * Interrupt specifiers are triplets
+	 * <peripheral-address, irq-number, IRQ_TYPE_*>
+	 *
+	 * peripheral-address - The base address of the peripheral
+	 * irq-number	      - The zero based bit position of the peripheral's
+	 *			interrupt registers corresponding to the irq
+	 *			where the LSB is 0 and the MSB is 7
+	 * IRQ_TYPE_*	      - Please refer to linux/irq.h
+	 */
+	*out_hwirq = intspec[0] << 8 | BIT(intspec[1]);
+	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static const struct irq_domain_ops i2c_pmic_domain_ops = {
+	.map	= i2c_pmic_domain_map,
+	.xlate	= i2c_pmic_domain_xlate,
+};
+
+static void i2c_pmic_irq_ack_now(struct i2c_pmic *chip, u16 hwirq)
+{
+	int rc;
+
+	rc = regmap_write(chip->regmap,
+			  (hwirq & 0xFF00) | INT_LATCHED_CLR_OFFSET,
+			  hwirq & 0xFF);
+	if (rc < 0)
+		pr_err_ratelimited("Couldn't ack 0x%04x rc=%d\n", hwirq, rc);
+}
+
+static void i2c_pmic_irq_disable_now(struct i2c_pmic *chip, u16 hwirq)
+{
+	struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
+	int rc;
+
+	if (!periph)
+		return;
+
+	mutex_lock(&periph->lock);
+	periph->cached[IRQ_EN_SET] &= ~hwirq & 0xFF;
+
+	rc = regmap_write(chip->regmap,
+			  (hwirq & 0xFF00) | INT_EN_CLR_OFFSET,
+			  hwirq & 0xFF);
+	if (rc < 0) {
+		pr_err_ratelimited("Couldn't disable irq 0x%04x rc=%d\n",
+				   hwirq, rc);
+		goto unlock;
+	}
+
+	periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
+
+unlock:
+	mutex_unlock(&periph->lock);
+}
+
+static void i2c_pmic_periph_status_handler(struct i2c_pmic *chip,
+					   u16 periph_address, u8 periph_status)
+{
+	unsigned int hwirq, virq;
+	int i;
+
+	while (periph_status) {
+		i = ffs(periph_status) - 1;
+		periph_status &= ~BIT(i);
+		hwirq = periph_address | BIT(i);
+		virq = irq_find_mapping(chip->domain, hwirq);
+		if (virq == 0) {
+			pr_err_ratelimited("Couldn't find mapping; disabling 0x%04x\n",
+					   hwirq);
+			i2c_pmic_irq_disable_now(chip, hwirq);
+			continue;
+		}
+
+		handle_nested_irq(virq);
+		i2c_pmic_irq_ack_now(chip, hwirq);
+	}
+}
+
+static void i2c_pmic_summary_status_handler(struct i2c_pmic *chip,
+					    struct i2c_pmic_periph *periph,
+					    u8 summary_status)
+{
+	unsigned int periph_status;
+	int rc, i;
+
+	while (summary_status) {
+		i = ffs(summary_status) - 1;
+		summary_status &= ~BIT(i);
+
+		rc = regmap_read(chip->regmap,
+				 periph[i].addr | INT_LATCHED_STS_OFFSET,
+				 &periph_status);
+		if (rc < 0) {
+			pr_err_ratelimited("Couldn't read 0x%04x | INT_LATCHED_STS rc=%d\n",
+					   periph[i].addr, rc);
+			continue;
+		}
+
+		i2c_pmic_periph_status_handler(chip, periph[i].addr,
+					       periph_status);
+	}
+}
+
+static irqreturn_t i2c_pmic_irq_handler(int irq, void *dev_id)
+{
+	struct i2c_pmic *chip = dev_id;
+	struct i2c_pmic_periph *periph;
+	unsigned int summary_status;
+	int rc, i;
+
+	mutex_lock(&chip->irq_complete);
+	chip->irq_waiting = true;
+	if (!chip->resume_completed) {
+		pr_debug("IRQ triggered before device-resume\n");
+		disable_irq_nosync(irq);
+		mutex_unlock(&chip->irq_complete);
+		return IRQ_HANDLED;
+	}
+	chip->irq_waiting = false;
+
+	for (i = 0; i < DIV_ROUND_UP(chip->num_periphs, BITS_PER_BYTE); i++) {
+		rc = regmap_read(chip->regmap, I2C_INTR_STATUS_BASE + i,
+				&summary_status);
+		if (rc < 0) {
+			pr_err_ratelimited("Couldn't read I2C_INTR_STATUS%d rc=%d\n",
+					   i, rc);
+			continue;
+		}
+
+		if (summary_status == 0)
+			continue;
+
+		periph = &chip->periph[i * 8];
+		i2c_pmic_summary_status_handler(chip, periph, summary_status);
+	}
+
+	mutex_unlock(&chip->irq_complete);
+
+	return IRQ_HANDLED;
+}
+
+static int i2c_pmic_parse_dt(struct i2c_pmic *chip)
+{
+	struct device_node *node = chip->dev->of_node;
+	int rc, i;
+	u32 temp;
+
+	if (!node) {
+		pr_err("missing device tree\n");
+		return -EINVAL;
+	}
+
+	chip->num_periphs = of_property_count_u32_elems(node,
+							"qcom,periph-map");
+	if (chip->num_periphs < 0) {
+		pr_err("missing qcom,periph-map property rc=%d\n",
+			chip->num_periphs);
+		return chip->num_periphs;
+	}
+
+	if (chip->num_periphs == 0) {
+		pr_err("qcom,periph-map must contain at least one address\n");
+		return -EINVAL;
+	}
+
+	chip->periph = devm_kcalloc(chip->dev, chip->num_periphs,
+				     sizeof(*chip->periph), GFP_KERNEL);
+	if (!chip->periph)
+		return -ENOMEM;
+
+	for (i = 0; i < chip->num_periphs; i++) {
+		rc = of_property_read_u32_index(node, "qcom,periph-map",
+						i, &temp);
+		if (rc < 0) {
+			pr_err("Couldn't read qcom,periph-map[%d] rc=%d\n",
+			       i, rc);
+			return rc;
+		}
+
+		chip->periph[i].addr = (u16)(temp << 8);
+		chip->periph[i].data = chip;
+		mutex_init(&chip->periph[i].lock);
+	}
+
+	of_property_read_string(node, "pinctrl-names", &chip->pinctrl_name);
+
+	return rc;
+}
+
+#define MAX_I2C_RETRIES	3
+static int i2c_pmic_read(struct regmap *map, unsigned int reg, void *val,
+			size_t val_count)
+{
+	int rc, retries = 0;
+
+	do {
+		rc = regmap_bulk_read(map, reg, val, val_count);
+	} while (rc == -ENOTCONN && retries++ < MAX_I2C_RETRIES);
+
+	if (retries > 1)
+		pr_err("i2c_pmic_read failed for %d retries, rc = %d\n",
+			retries - 1, rc);
+
+	return rc;
+}
+
+static int i2c_pmic_determine_initial_status(struct i2c_pmic *chip)
+{
+	int rc, i;
+
+	for (i = 0; i < chip->num_periphs; i++) {
+		rc = i2c_pmic_read(chip->regmap,
+				chip->periph[i].addr | INT_SET_TYPE_OFFSET,
+				chip->periph[i].cached, IRQ_MAX_REGS);
+		if (rc < 0) {
+			pr_err("Couldn't read irq data rc=%d\n", rc);
+			return rc;
+		}
+
+		memcpy(chip->periph[i].synced, chip->periph[i].cached,
+		       IRQ_MAX_REGS * sizeof(*chip->periph[i].synced));
+	}
+
+	return 0;
+}
+
+static struct regmap_config i2c_pmic_regmap_config = {
+	.reg_bits	= 16,
+	.val_bits	= 8,
+	.max_register	= 0xFFFF,
+};
+
+static int i2c_pmic_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct i2c_pmic *chip;
+	int rc = 0;
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &client->dev;
+	chip->regmap = devm_regmap_init_i2c(client, &i2c_pmic_regmap_config);
+	if (!chip->regmap)
+		return -ENODEV;
+
+	i2c_set_clientdata(client, chip);
+	if (!of_property_read_bool(chip->dev->of_node, "interrupt-controller"))
+		goto probe_children;
+
+	chip->domain = irq_domain_add_tree(client->dev.of_node,
+					   &i2c_pmic_domain_ops, chip);
+	if (!chip->domain) {
+		rc = -ENOMEM;
+		goto cleanup;
+	}
+
+	rc = i2c_pmic_parse_dt(chip);
+	if (rc < 0) {
+		pr_err("Couldn't parse device tree rc=%d\n", rc);
+		goto cleanup;
+	}
+
+	rc = i2c_pmic_determine_initial_status(chip);
+	if (rc < 0) {
+		pr_err("Couldn't determine initial status rc=%d\n", rc);
+		goto cleanup;
+	}
+
+	if (chip->pinctrl_name) {
+		chip->pinctrl = devm_pinctrl_get_select(chip->dev,
+							chip->pinctrl_name);
+		if (IS_ERR(chip->pinctrl)) {
+			pr_err("Couldn't select %s pinctrl rc=%ld\n",
+				chip->pinctrl_name, PTR_ERR(chip->pinctrl));
+			rc = PTR_ERR(chip->pinctrl);
+			goto cleanup;
+		}
+	}
+
+	chip->resume_completed = true;
+	mutex_init(&chip->irq_complete);
+
+	rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+				       i2c_pmic_irq_handler,
+				       IRQF_ONESHOT | IRQF_SHARED,
+				       "i2c_pmic_stat_irq", chip);
+	if (rc < 0) {
+		pr_err("Couldn't request irq %d rc=%d\n", client->irq, rc);
+		goto cleanup;
+	}
+
+	chip->summary_irq = client->irq;
+	enable_irq_wake(client->irq);
+
+probe_children:
+	of_platform_populate(chip->dev->of_node, NULL, NULL, chip->dev);
+	pr_info("I2C PMIC probe successful\n");
+	return rc;
+
+cleanup:
+	if (chip->domain)
+		irq_domain_remove(chip->domain);
+	i2c_set_clientdata(client, NULL);
+	return rc;
+}
+
+static int i2c_pmic_remove(struct i2c_client *client)
+{
+	struct i2c_pmic *chip = i2c_get_clientdata(client);
+
+	of_platform_depopulate(chip->dev);
+	if (chip->domain)
+		irq_domain_remove(chip->domain);
+	i2c_set_clientdata(client, NULL);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int i2c_pmic_suspend_noirq(struct device *dev)
+{
+	struct i2c_pmic *chip = dev_get_drvdata(dev);
+
+	if (chip->irq_waiting) {
+		pr_err_ratelimited("Aborting suspend, an interrupt was detected while suspending\n");
+		return -EBUSY;
+	}
+	return 0;
+}
+
+static int i2c_pmic_suspend(struct device *dev)
+{
+	struct i2c_pmic *chip = dev_get_drvdata(dev);
+	struct i2c_pmic_periph *periph;
+	int rc = 0, i;
+
+	for (i = 0; i < chip->num_periphs; i++) {
+		periph = &chip->periph[i];
+
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
+		if (rc < 0) {
+			pr_err_ratelimited("Couldn't clear 0x%04x irqs rc=%d\n",
+				periph->addr, rc);
+			continue;
+		}
+
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_EN_SET_OFFSET,
+				  periph->wake);
+		if (rc < 0)
+			pr_err_ratelimited("Couldn't enable 0x%04x wake irqs 0x%02x rc=%d\n",
+			       periph->addr, periph->wake, rc);
+	}
+	if (!rc) {
+		mutex_lock(&chip->irq_complete);
+		chip->resume_completed = false;
+		mutex_unlock(&chip->irq_complete);
+	}
+
+	return rc;
+}
+
+static int i2c_pmic_resume(struct device *dev)
+{
+	struct i2c_pmic *chip = dev_get_drvdata(dev);
+	struct i2c_pmic_periph *periph;
+	int rc = 0, i;
+
+	for (i = 0; i < chip->num_periphs; i++) {
+		periph = &chip->periph[i];
+
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
+		if (rc < 0) {
+			pr_err("Couldn't clear 0x%04x irqs rc=%d\n",
+				periph->addr, rc);
+			continue;
+		}
+
+		rc = regmap_write(chip->regmap,
+				  periph->addr | INT_EN_SET_OFFSET,
+				  periph->synced[IRQ_EN_SET]);
+		if (rc < 0)
+			pr_err("Couldn't restore 0x%04x synced irqs 0x%02x rc=%d\n",
+			       periph->addr, periph->synced[IRQ_EN_SET], rc);
+	}
+
+	mutex_lock(&chip->irq_complete);
+	chip->resume_completed = true;
+	if (chip->irq_waiting) {
+		mutex_unlock(&chip->irq_complete);
+		/* irq was pending, call the handler */
+		i2c_pmic_irq_handler(chip->summary_irq, chip);
+		enable_irq(chip->summary_irq);
+	} else {
+		mutex_unlock(&chip->irq_complete);
+	}
+
+	return rc;
+}
+#else
+static int i2c_pmic_suspend(struct device *dev)
+{
+	return 0;
+}
+static int i2c_pmic_resume(struct device *dev)
+{
+	return 0;
+}
+static int i2c_pmic_suspend_noirq(struct device *dev)
+{
+	return 0
+}
+#endif
+static const struct dev_pm_ops i2c_pmic_pm_ops = {
+	.suspend	= i2c_pmic_suspend,
+	.suspend_noirq	= i2c_pmic_suspend_noirq,
+	.resume		= i2c_pmic_resume,
+};
+
+static const struct of_device_id i2c_pmic_match_table[] = {
+	{ .compatible = "qcom,i2c-pmic", },
+	{ },
+};
+
+static const struct i2c_device_id i2c_pmic_id[] = {
+	{ "i2c-pmic", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, i2c_pmic_id);
+
+static struct i2c_driver i2c_pmic_driver = {
+	.driver		= {
+		.name		= "i2c_pmic",
+		.pm		= &i2c_pmic_pm_ops,
+		.of_match_table	= i2c_pmic_match_table,
+	},
+	.probe		= i2c_pmic_probe,
+	.remove		= i2c_pmic_remove,
+	.id_table	= i2c_pmic_id,
+};
+
+module_i2c_driver(i2c_pmic_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("i2c:i2c_pmic");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] dt-bindings: mfd: Document QTI I2C PMIC controller

[Rob Herring]

On Mon, 27 Apr 2020 19:18:01 -0700, Guru Das Srinagesh wrote:
> The Qualcomm Technologies, Inc. I2C PMIC Controller is used by
> multi-function PMIC devices which communicate over the I2C bus.  The
> controller enumerates all child nodes as platform devices, and
> instantiates a regmap interface for them to communicate over the I2C
> bus.
> 
> The controller also controls interrupts for all of the children platform
> devices.  The controller handles the summary interrupt by deciphering
> which peripheral triggered the interrupt, and which of the peripheral
> interrupts were triggered.  Finally, it calls the interrupt handlers for
> each of the virtual interrupts that were registered.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  .../devicetree/bindings/mfd/qcom,i2c-pmic.yaml     | 89 ++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Error: Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dts:17.1-7 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1278158

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH 2/2] mfd: Introduce QTI I2C PMIC controller

[kbuild test robot]

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

Hi Guru,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v5.7-rc3 next-20200501]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Guru-Das-Srinagesh/Introduce-QTI-I2C-PMIC-controller/20200428-175503
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/mfd/qcom-i2c-pmic.c: In function 'i2c_pmic_suspend_noirq':
>> drivers/mfd/qcom-i2c-pmic.c:703:10: error: expected ';' before '}' token
     703 |  return 0
         |          ^
         |          ;
     704 | }
         | ~         

vim +703 drivers/mfd/qcom-i2c-pmic.c

   653	
   654	static int i2c_pmic_resume(struct device *dev)
   655	{
   656		struct i2c_pmic *chip = dev_get_drvdata(dev);
   657		struct i2c_pmic_periph *periph;
   658		int rc = 0, i;
   659	
   660		for (i = 0; i < chip->num_periphs; i++) {
   661			periph = &chip->periph[i];
   662	
   663			rc = regmap_write(chip->regmap,
   664					  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
   665			if (rc < 0) {
   666				pr_err("Couldn't clear 0x%04x irqs rc=%d\n",
   667					periph->addr, rc);
   668				continue;
   669			}
   670	
   671			rc = regmap_write(chip->regmap,
   672					  periph->addr | INT_EN_SET_OFFSET,
   673					  periph->synced[IRQ_EN_SET]);
   674			if (rc < 0)
   675				pr_err("Couldn't restore 0x%04x synced irqs 0x%02x rc=%d\n",
   676				       periph->addr, periph->synced[IRQ_EN_SET], rc);
   677		}
   678	
   679		mutex_lock(&chip->irq_complete);
   680		chip->resume_completed = true;
   681		if (chip->irq_waiting) {
   682			mutex_unlock(&chip->irq_complete);
   683			/* irq was pending, call the handler */
   684			i2c_pmic_irq_handler(chip->summary_irq, chip);
   685			enable_irq(chip->summary_irq);
   686		} else {
   687			mutex_unlock(&chip->irq_complete);
   688		}
   689	
   690		return rc;
   691	}
   692	#else
   693	static int i2c_pmic_suspend(struct device *dev)
   694	{
   695		return 0;
   696	}
   697	static int i2c_pmic_resume(struct device *dev)
   698	{
   699		return 0;
   700	}
   701	static int i2c_pmic_suspend_noirq(struct device *dev)
   702	{
 > 703		return 0
   704	}
   705	#endif
   706	static const struct dev_pm_ops i2c_pmic_pm_ops = {
   707		.suspend	= i2c_pmic_suspend,
   708		.suspend_noirq	= i2c_pmic_suspend_noirq,
   709		.resume		= i2c_pmic_resume,
   710	};
   711	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56845 bytes --]

Re: [PATCH 2/2] mfd: Introduce QTI I2C PMIC controller

[Lee Jones]

[Adding IRQ chaps]

Seeing as a vast portion of this driver is IRQ domain related, I see
good reason to separate it out into a driver in its own right.  At the
very least it will require an IRQ *-by tag.

On Mon, 27 Apr 2020, Guru Das Srinagesh wrote:

> The Qualcomm Technologies, Inc. I2C PMIC Controller is used by
> multi-function PMIC devices which communicate over the I2C bus.  The
> controller enumerates all child nodes as platform devices, and
> instantiates a regmap interface for them to communicate over the I2C
> bus.
> 
> The controller also controls interrupts for all of the children platform
> devices.  The controller handles the summary interrupt by deciphering
> which peripheral triggered the interrupt, and which of the peripheral
> interrupts were triggered.  Finally, it calls the interrupt handlers for
> each of the virtual interrupts that were registered.

> Nicholas Troast is the original author of this driver.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  drivers/mfd/Kconfig         |  11 +
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/qcom-i2c-pmic.c | 737 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 749 insertions(+)
>  create mode 100644 drivers/mfd/qcom-i2c-pmic.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 54b6aa4..bf112eb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1002,6 +1002,17 @@ config MFD_PM8XXX
>  	  Say M here if you want to include support for PM8xxx chips as a
>  	  module. This will build a module called "pm8xxx-core".
>  
> +config MFD_I2C_PMIC

"I2C PMIC" is too generic.

> +	tristate "QTI I2C PMIC support"

I don't see QTI used very often.

What's the difference between "QTI" and "QCOM"?

> +	depends on I2C && OF
> +	select IRQ_DOMAIN
> +	select REGMAP_I2C
> +	help
> +	  This enables support for controlling Qualcomm Technologies, Inc.
> +	  PMICs over I2C. The driver controls interrupts, and provides register
> +	  access for all of the device's peripherals.  Some QTI PMIC chips
> +	  support communication over both I2C and SPMI.
> +
>  config MFD_QCOM_RPM
>  	tristate "Qualcomm Resource Power Manager (RPM)"
>  	depends on ARCH_QCOM && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7761f84..26f0b80 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -199,6 +199,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
>  obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
>  obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
>  obj-$(CONFIG_MFD_PM8XXX) 	+= qcom-pm8xxx.o ssbi.o
> +obj-$(CONFIG_MFD_I2C_PMIC)     += qcom-i2c-pmic.o
>  obj-$(CONFIG_MFD_QCOM_RPM)	+= qcom_rpm.o
>  obj-$(CONFIG_MFD_SPMI_PMIC)	+= qcom-spmi-pmic.o
>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
> diff --git a/drivers/mfd/qcom-i2c-pmic.c b/drivers/mfd/qcom-i2c-pmic.c
> new file mode 100644
> index 0000000..d0f600a
> --- /dev/null
> +++ b/drivers/mfd/qcom-i2c-pmic.c
> @@ -0,0 +1,737 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.

This is out of date.

No author tag?

> + */
> +
> +#define pr_fmt(fmt) "I2C PMIC: %s: " fmt, __func__

Outside of in-development code I don't see a reason for adding the
function name to system log entries.  IMHO, it just dirties the log on
production/released H/W.  Please considering removing this
altogether in from plain device drivers.

> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>

I'm going to skip all of the IRQ code until we know what we're doing
with it.

 #########
<-- IRQ -->
 #########

> +#define I2C_INTR_STATUS_BASE	0x0550
> +#define INT_RT_STS_OFFSET	0x10
> +#define INT_SET_TYPE_OFFSET	0x11
> +#define INT_POL_HIGH_OFFSET	0x12
> +#define INT_POL_LOW_OFFSET	0x13
> +#define INT_LATCHED_CLR_OFFSET	0x14
> +#define INT_EN_SET_OFFSET	0x15
> +#define INT_EN_CLR_OFFSET	0x16
> +#define INT_LATCHED_STS_OFFSET	0x18
> +#define INT_PENDING_STS_OFFSET	0x19
> +#define INT_MID_SEL_OFFSET	0x1A
> +#define INT_MID_SEL_MASK	GENMASK(1, 0)
> +#define INT_PRIORITY_OFFSET	0x1B
> +#define INT_PRIORITY_BIT	BIT(0)
> +
> +enum {
> +	IRQ_SET_TYPE = 0,
> +	IRQ_POL_HIGH,
> +	IRQ_POL_LOW,
> +	IRQ_LATCHED_CLR, /* not needed but makes life easy */
> +	IRQ_EN_SET,
> +	IRQ_MAX_REGS,
> +};
> +
> +struct i2c_pmic_periph {
> +	void		*data;
> +	u16		addr;
> +	u8		cached[IRQ_MAX_REGS];
> +	u8		synced[IRQ_MAX_REGS];
> +	u8		wake;
> +	struct mutex	lock;
> +};
> +
> +struct i2c_pmic {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	struct irq_domain	*domain;
> +	struct i2c_pmic_periph	*periph;
> +	struct pinctrl		*pinctrl;
> +	struct mutex		irq_complete;
> +	const char		*pinctrl_name;
> +	int			num_periphs;
> +	int			summary_irq;
> +	bool			resume_completed;
> +	bool			irq_waiting;
> +};
> +
> +static void i2c_pmic_irq_bus_lock(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	mutex_lock(&periph->lock);
> +}
> +
> +static void i2c_pmic_sync_type_polarity(struct i2c_pmic *chip,
> +			       struct i2c_pmic_periph *periph)
> +{
> +	int rc;
> +
> +	/* did any irq type change? */
> +	if (periph->cached[IRQ_SET_TYPE] ^ periph->synced[IRQ_SET_TYPE]) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_SET_TYPE_OFFSET,
> +				  periph->cached[IRQ_SET_TYPE]);
> +		if (rc < 0) {
> +			pr_err("Couldn't set periph 0x%04x irqs 0x%02x type rc=%d\n",
> +				periph->addr, periph->cached[IRQ_SET_TYPE], rc);
> +			return;
> +		}
> +
> +		periph->synced[IRQ_SET_TYPE] = periph->cached[IRQ_SET_TYPE];
> +	}
> +
> +	/* did any polarity high change? */
> +	if (periph->cached[IRQ_POL_HIGH] ^ periph->synced[IRQ_POL_HIGH]) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_POL_HIGH_OFFSET,
> +				  periph->cached[IRQ_POL_HIGH]);
> +		if (rc < 0) {
> +			pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity high rc=%d\n",
> +				periph->addr, periph->cached[IRQ_POL_HIGH], rc);
> +			return;
> +		}
> +
> +		periph->synced[IRQ_POL_HIGH] = periph->cached[IRQ_POL_HIGH];
> +	}
> +
> +	/* did any polarity low change? */
> +	if (periph->cached[IRQ_POL_LOW] ^ periph->synced[IRQ_POL_LOW]) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_POL_LOW_OFFSET,
> +				  periph->cached[IRQ_POL_LOW]);
> +		if (rc < 0) {
> +			pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity low rc=%d\n",
> +				periph->addr, periph->cached[IRQ_POL_LOW], rc);
> +			return;
> +		}
> +
> +		periph->synced[IRQ_POL_LOW] = periph->cached[IRQ_POL_LOW];
> +	}
> +}
> +
> +static void i2c_pmic_sync_enable(struct i2c_pmic *chip,
> +				 struct i2c_pmic_periph *periph)
> +{
> +	u8 en_set, en_clr;
> +	int rc;
> +
> +	/* determine which irqs were enabled and which were disabled */
> +	en_clr = periph->synced[IRQ_EN_SET] & ~periph->cached[IRQ_EN_SET];
> +	en_set = ~periph->synced[IRQ_EN_SET] & periph->cached[IRQ_EN_SET];
> +
> +	/* were any irqs disabled? */
> +	if (en_clr) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_CLR_OFFSET, en_clr);
> +		if (rc < 0) {
> +			pr_err("Couldn't disable periph 0x%04x irqs 0x%02x rc=%d\n",
> +				periph->addr, en_clr, rc);
> +			return;
> +		}
> +	}
> +
> +	/* were any irqs enabled? */
> +	if (en_set) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_SET_OFFSET, en_set);
> +		if (rc < 0) {
> +			pr_err("Couldn't enable periph 0x%04x irqs 0x%02x rc=%d\n",
> +				periph->addr, en_set, rc);
> +			return;
> +		}
> +	}
> +
> +	/* irq enabled status was written to hardware */
> +	periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
> +}
> +
> +static void i2c_pmic_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +	struct i2c_pmic *chip = periph->data;
> +
> +	i2c_pmic_sync_type_polarity(chip, periph);
> +	i2c_pmic_sync_enable(chip, periph);
> +	mutex_unlock(&periph->lock);
> +}
> +
> +static void i2c_pmic_irq_disable(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	periph->cached[IRQ_EN_SET] &= ~d->hwirq & 0xFF;
> +}
> +
> +static void i2c_pmic_irq_enable(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	periph->cached[IRQ_EN_SET] |= d->hwirq & 0xFF;
> +}
> +
> +static int i2c_pmic_irq_set_type(struct irq_data *d, unsigned int irq_type)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	switch (irq_type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> +		break;
> +	default:
> +		pr_err("irq type 0x%04x is not supported\n", irq_type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_pmic_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	if (on)
> +		periph->wake |= d->hwirq & 0xFF;
> +	else
> +		periph->wake &= ~d->hwirq & 0xFF;
> +
> +	return 0;
> +}
> +#else
> +#define i2c_pmic_irq_set_wake NULL
> +#endif
> +
> +static struct irq_chip i2c_pmic_irq_chip = {
> +	.name			= "i2c_pmic_irq_chip",
> +	.irq_bus_lock		= i2c_pmic_irq_bus_lock,
> +	.irq_bus_sync_unlock	= i2c_pmic_irq_bus_sync_unlock,
> +	.irq_disable		= i2c_pmic_irq_disable,
> +	.irq_enable		= i2c_pmic_irq_enable,
> +	.irq_set_type		= i2c_pmic_irq_set_type,
> +	.irq_set_wake		= i2c_pmic_irq_set_wake,
> +};
> +
> +static struct i2c_pmic_periph *i2c_pmic_find_periph(struct i2c_pmic *chip,
> +						    irq_hw_number_t hwirq)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->num_periphs; i++)
> +		if (chip->periph[i].addr == (hwirq & 0xFF00))
> +			return &chip->periph[i];
> +
> +	pr_err_ratelimited("Couldn't find periph struct for hwirq 0x%04lx\n",
> +			   hwirq);
> +	return NULL;
> +}
> +
> +static int i2c_pmic_domain_map(struct irq_domain *d, unsigned int virq,
> +			irq_hw_number_t hwirq)
> +{
> +	struct i2c_pmic *chip = d->host_data;
> +	struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
> +
> +	if (!periph)
> +		return -ENODEV;
> +
> +	irq_set_chip_data(virq, periph);
> +	irq_set_chip_and_handler(virq, &i2c_pmic_irq_chip, handle_level_irq);
> +	irq_set_nested_thread(virq, 1);
> +	irq_set_noprobe(virq);
> +	return 0;
> +}
> +
> +static int i2c_pmic_domain_xlate(struct irq_domain *d,
> +				 struct device_node *ctrlr, const u32 *intspec,
> +				 unsigned int intsize, unsigned long *out_hwirq,
> +				 unsigned int *out_type)
> +{
> +	if (intsize != 3)
> +		return -EINVAL;
> +
> +	if (intspec[0] > 0xFF || intspec[1] > 0x7 ||
> +					intspec[2] > IRQ_TYPE_SENSE_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * Interrupt specifiers are triplets
> +	 * <peripheral-address, irq-number, IRQ_TYPE_*>
> +	 *
> +	 * peripheral-address - The base address of the peripheral
> +	 * irq-number	      - The zero based bit position of the peripheral's
> +	 *			interrupt registers corresponding to the irq
> +	 *			where the LSB is 0 and the MSB is 7
> +	 * IRQ_TYPE_*	      - Please refer to linux/irq.h
> +	 */
> +	*out_hwirq = intspec[0] << 8 | BIT(intspec[1]);
> +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops i2c_pmic_domain_ops = {
> +	.map	= i2c_pmic_domain_map,
> +	.xlate	= i2c_pmic_domain_xlate,
> +};
> +
> +static void i2c_pmic_irq_ack_now(struct i2c_pmic *chip, u16 hwirq)
> +{
> +	int rc;
> +
> +	rc = regmap_write(chip->regmap,
> +			  (hwirq & 0xFF00) | INT_LATCHED_CLR_OFFSET,
> +			  hwirq & 0xFF);
> +	if (rc < 0)
> +		pr_err_ratelimited("Couldn't ack 0x%04x rc=%d\n", hwirq, rc);
> +}
> +
> +static void i2c_pmic_irq_disable_now(struct i2c_pmic *chip, u16 hwirq)
> +{
> +	struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
> +	int rc;
> +
> +	if (!periph)
> +		return;
> +
> +	mutex_lock(&periph->lock);
> +	periph->cached[IRQ_EN_SET] &= ~hwirq & 0xFF;
> +
> +	rc = regmap_write(chip->regmap,
> +			  (hwirq & 0xFF00) | INT_EN_CLR_OFFSET,
> +			  hwirq & 0xFF);
> +	if (rc < 0) {
> +		pr_err_ratelimited("Couldn't disable irq 0x%04x rc=%d\n",
> +				   hwirq, rc);
> +		goto unlock;
> +	}
> +
> +	periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
> +
> +unlock:
> +	mutex_unlock(&periph->lock);
> +}
> +
> +static void i2c_pmic_periph_status_handler(struct i2c_pmic *chip,
> +					   u16 periph_address, u8 periph_status)
> +{
> +	unsigned int hwirq, virq;
> +	int i;
> +
> +	while (periph_status) {
> +		i = ffs(periph_status) - 1;
> +		periph_status &= ~BIT(i);
> +		hwirq = periph_address | BIT(i);
> +		virq = irq_find_mapping(chip->domain, hwirq);
> +		if (virq == 0) {
> +			pr_err_ratelimited("Couldn't find mapping; disabling 0x%04x\n",
> +					   hwirq);
> +			i2c_pmic_irq_disable_now(chip, hwirq);
> +			continue;
> +		}
> +
> +		handle_nested_irq(virq);
> +		i2c_pmic_irq_ack_now(chip, hwirq);
> +	}
> +}
> +
> +static void i2c_pmic_summary_status_handler(struct i2c_pmic *chip,
> +					    struct i2c_pmic_periph *periph,
> +					    u8 summary_status)
> +{
> +	unsigned int periph_status;
> +	int rc, i;
> +
> +	while (summary_status) {
> +		i = ffs(summary_status) - 1;
> +		summary_status &= ~BIT(i);
> +
> +		rc = regmap_read(chip->regmap,
> +				 periph[i].addr | INT_LATCHED_STS_OFFSET,
> +				 &periph_status);
> +		if (rc < 0) {
> +			pr_err_ratelimited("Couldn't read 0x%04x | INT_LATCHED_STS rc=%d\n",
> +					   periph[i].addr, rc);
> +			continue;
> +		}
> +
> +		i2c_pmic_periph_status_handler(chip, periph[i].addr,
> +					       periph_status);
> +	}
> +}
> +
> +static irqreturn_t i2c_pmic_irq_handler(int irq, void *dev_id)
> +{
> +	struct i2c_pmic *chip = dev_id;
> +	struct i2c_pmic_periph *periph;
> +	unsigned int summary_status;
> +	int rc, i;
> +
> +	mutex_lock(&chip->irq_complete);
> +	chip->irq_waiting = true;
> +	if (!chip->resume_completed) {
> +		pr_debug("IRQ triggered before device-resume\n");
> +		disable_irq_nosync(irq);
> +		mutex_unlock(&chip->irq_complete);
> +		return IRQ_HANDLED;
> +	}
> +	chip->irq_waiting = false;
> +
> +	for (i = 0; i < DIV_ROUND_UP(chip->num_periphs, BITS_PER_BYTE); i++) {
> +		rc = regmap_read(chip->regmap, I2C_INTR_STATUS_BASE + i,
> +				&summary_status);
> +		if (rc < 0) {
> +			pr_err_ratelimited("Couldn't read I2C_INTR_STATUS%d rc=%d\n",
> +					   i, rc);
> +			continue;
> +		}
> +
> +		if (summary_status == 0)
> +			continue;
> +
> +		periph = &chip->periph[i * 8];
> +		i2c_pmic_summary_status_handler(chip, periph, summary_status);
> +	}
> +
> +	mutex_unlock(&chip->irq_complete);
> +
> +	return IRQ_HANDLED;
> +}

 #########
<-- IRQ -->
 #########

> +static int i2c_pmic_parse_dt(struct i2c_pmic *chip)
> +{
> +	struct device_node *node = chip->dev->of_node;

s/node/np/

> +	int rc, i;
> +	u32 temp;

Temp is seldom good nomenclature.

"periph_addr"

> +	if (!node) {
> +		pr_err("missing device tree\n");

Why are you using pr_err() over dev_err()?

Please use proper grammar (less full-stops) in prints and comments.

The above goes for all - I won't mention it again.

> +		return -EINVAL;
> +	}

Can this happen.

> +	chip->num_periphs = of_property_count_u32_elems(node,
> +							"qcom,periph-map");

Prefer the break after the '='.

If you change 's/node/np' you don't need to break at all.

> +	if (chip->num_periphs < 0) {
> +		pr_err("missing qcom,periph-map property rc=%d\n",

Prefer it if you use plain English for user facing comments.

  "Peripheral map not defined"

> +			chip->num_periphs);
> +		return chip->num_periphs;
> +	}
> +
> +	if (chip->num_periphs == 0) {
> +		pr_err("qcom,periph-map must contain at least one address\n");

"Peripheral map is empty"

> +		return -EINVAL;
> +	}
> +
> +	chip->periph = devm_kcalloc(chip->dev, chip->num_periphs,
> +				     sizeof(*chip->periph), GFP_KERNEL);
> +	if (!chip->periph)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		rc = of_property_read_u32_index(node, "qcom,periph-map",
> +						i, &temp);
> +		if (rc < 0) {
> +			pr_err("Couldn't read qcom,periph-map[%d]
> rc=%d\n",

"Peripheral map entry %d is invalid"

> +			       i, rc);
> +			return rc;
> +		}
> +
> +		chip->periph[i].addr = (u16)(temp << 8);

Worth a comment I think.

> +		chip->periph[i].data = chip;

'data' is also not a great variable name, if you can avoid it.

> +		mutex_init(&chip->periph[i].lock);
> +	}
> +
> +	of_property_read_string(node, "pinctrl-names", &chip->pinctrl_name);
> +
> +	return rc;

Why return rc here?

> +}
> +
> +#define MAX_I2C_RETRIES	3

Why 3?

> +static int i2c_pmic_read(struct regmap *map, unsigned int reg, void *val,
> +			size_t val_count)
> +{
> +	int rc, retries = 0;
> +
> +	do {
> +		rc = regmap_bulk_read(map, reg, val, val_count);
> +	} while (rc == -ENOTCONN && retries++ < MAX_I2C_RETRIES);
> +
> +	if (retries > 1)
> +		pr_err("i2c_pmic_read failed for %d retries, rc = %d\n",
> +			retries - 1, rc);

Is this always an error?

Why would the user care if it failed once, then succeeded?

> +	return rc;
> +}
> +
> +static int i2c_pmic_determine_initial_status(struct i2c_pmic *chip)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		rc = i2c_pmic_read(chip->regmap,
> +				chip->periph[i].addr | INT_SET_TYPE_OFFSET,
> +				chip->periph[i].cached, IRQ_MAX_REGS);
> +		if (rc < 0) {
> +			pr_err("Couldn't read irq data rc=%d\n", rc);

"IRQ"

> +			return rc;
> +		}
> +
> +		memcpy(chip->periph[i].synced, chip->periph[i].cached,
> +		       IRQ_MAX_REGS * sizeof(*chip->periph[i].synced));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct regmap_config i2c_pmic_regmap_config = {
> +	.reg_bits	= 16,
> +	.val_bits	= 8,
> +	.max_register	= 0xFFFF,
> +};
> +
> +static int i2c_pmic_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct i2c_pmic *chip;

We usually call this ddata.  Especially relevant as you are storing it
in *driver_data for reclaim in the functions above.

> +	int rc = 0;

'ret' is by far the more common name for this.

$ git grep "int ret[;\|,]" | wc -l
39365
$ git grep "int rc[;\|,]" | wc -l
6761

Let's keep things as consistent as possible please.

> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->dev = &client->dev;
> +	chip->regmap = devm_regmap_init_i2c(client, &i2c_pmic_regmap_config);
> +	if (!chip->regmap)
> +		return -ENODEV;
> +
> +	i2c_set_clientdata(client, chip);

'\n'

> +	if (!of_property_read_bool(chip->dev->of_node, "interrupt-controller"))
> +		goto probe_children;
> +
> +	chip->domain = irq_domain_add_tree(client->dev.of_node,
> +					   &i2c_pmic_domain_ops, chip);
> +	if (!chip->domain) {
> +		rc = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	rc = i2c_pmic_parse_dt(chip);
> +	if (rc < 0) {

If you return an error or 0 here, you can change this to:

if (rc)

> +		pr_err("Couldn't parse device tree rc=%d\n", rc);

Looks like you already have prints in i2c_pmic_parse_dt().

No need for an additional one.

> +		goto cleanup;
> +	}
> +
> +	rc = i2c_pmic_determine_initial_status(chip);
> +	if (rc < 0) {
> +		pr_err("Couldn't determine initial status rc=%d\n", rc);

As above.

> +		goto cleanup;
> +	}
> +
> +	if (chip->pinctrl_name) {
> +		chip->pinctrl = devm_pinctrl_get_select(chip->dev,
> +							chip->pinctrl_name);
> +		if (IS_ERR(chip->pinctrl)) {
> +			pr_err("Couldn't select %s pinctrl rc=%ld\n",
> +				chip->pinctrl_name, PTR_ERR(chip->pinctrl));
> +			rc = PTR_ERR(chip->pinctrl);

Do this before the print, then you only need to do it once.

> +			goto cleanup;
> +		}
> +	}
> +
> +	chip->resume_completed = true;

Why can't you disable interrupts before suspending?

> +	mutex_init(&chip->irq_complete);
> +
> +	rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +				       i2c_pmic_irq_handler,
> +				       IRQF_ONESHOT | IRQF_SHARED,
> +				       "i2c_pmic_stat_irq", chip);
> +	if (rc < 0) {
> +		pr_err("Couldn't request irq %d rc=%d\n", client->irq, rc);

"IRQ"

> +		goto cleanup;
> +	}
> +
> +	chip->summary_irq = client->irq;
> +	enable_irq_wake(client->irq);
> +
> +probe_children:
> +	of_platform_populate(chip->dev->of_node, NULL, NULL, chip->dev);

devm_*?

> +	pr_info("I2C PMIC probe successful\n");

This is superfluous, please remove it.

'\n' here.

> +	return rc;
> +
> +cleanup:
> +	if (chip->domain)
> +		irq_domain_remove(chip->domain);
> +	i2c_set_clientdata(client, NULL);

'\n' here.

> +	return rc;
> +}
> +
> +static int i2c_pmic_remove(struct i2c_client *client)
> +{
> +	struct i2c_pmic *chip = i2c_get_clientdata(client);
> +
> +	of_platform_depopulate(chip->dev);

If you use devm_* this becomes superfluous.

> +	if (chip->domain)
> +		irq_domain_remove(chip->domain);

'\n' here.

> +	i2c_set_clientdata(client, NULL);

'\n' here.

> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_pmic_suspend_noirq(struct device *dev)
> +{
> +	struct i2c_pmic *chip = dev_get_drvdata(dev);
> +
> +	if (chip->irq_waiting) {
> +		pr_err_ratelimited("Aborting suspend, an interrupt was detected while suspending\n");
> +		return -EBUSY;

I haven't seen this before.  Why does this platform require it?

What happens when you return early from .suspend_noirq?

Does this system just resume?

> +	}

'\n' here.

> +	return 0;
> +}
> +
> +static int i2c_pmic_suspend(struct device *dev)
> +{
> +	struct i2c_pmic *chip = dev_get_drvdata(dev);
> +	struct i2c_pmic_periph *periph;
> +	int rc = 0, i;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		periph = &chip->periph[i];
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
> +		if (rc < 0) {
> +			pr_err_ratelimited("Couldn't clear 0x%04x irqs rc=%d\n",

"IRQs"

Same goes for all the other mentions of it.

> +				periph->addr, rc);
> +			continue;
> +		}
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_SET_OFFSET,
> +				  periph->wake);
> +		if (rc < 0)
> +			pr_err_ratelimited("Couldn't enable 0x%04x wake irqs 0x%02x rc=%d\n",
> +			       periph->addr, periph->wake, rc);
> +	}

'\n' here.

> +	if (!rc) {
> +		mutex_lock(&chip->irq_complete);
> +		chip->resume_completed = false;
> +		mutex_unlock(&chip->irq_complete);
> +	}
> +
> +	return rc;
> +}
> +
> +static int i2c_pmic_resume(struct device *dev)
> +{
> +	struct i2c_pmic *chip = dev_get_drvdata(dev);
> +	struct i2c_pmic_periph *periph;
> +	int rc = 0, i;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		periph = &chip->periph[i];
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
> +		if (rc < 0) {
> +			pr_err("Couldn't clear 0x%04x irqs rc=%d\n",
> +				periph->addr, rc);
> +			continue;
> +		}
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_SET_OFFSET,
> +				  periph->synced[IRQ_EN_SET]);
> +		if (rc < 0)
> +			pr_err("Couldn't restore 0x%04x synced irqs 0x%02x rc=%d\n",
> +			       periph->addr, periph->synced[IRQ_EN_SET], rc);
> +	}
> +
> +	mutex_lock(&chip->irq_complete);
> +	chip->resume_completed = true;

'\n' here.

> +	if (chip->irq_waiting) {
> +		mutex_unlock(&chip->irq_complete);

Move this above the if(), then you can remove it from the else too.

> +		/* irq was pending, call the handler */
> +		i2c_pmic_irq_handler(chip->summary_irq, chip);
> +		enable_irq(chip->summary_irq);
> +	} else {
> +		mutex_unlock(&chip->irq_complete);
> +	}
> +
> +	return rc;
> +}
> +#else
> +static int i2c_pmic_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +static int i2c_pmic_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +static int i2c_pmic_suspend_noirq(struct device *dev)
> +{
> +	return 0
> +}
> +#endif
> +static const struct dev_pm_ops i2c_pmic_pm_ops = {
> +	.suspend	= i2c_pmic_suspend,
> +	.suspend_noirq	= i2c_pmic_suspend_noirq,
> +	.resume		= i2c_pmic_resume,
> +};

See how drivers/mfd/arizona-core.c removes all of this boilerplate.

> +static const struct of_device_id i2c_pmic_match_table[] = {
> +	{ .compatible = "qcom,i2c-pmic", },
> +	{ },
> +};
> +static const struct i2c_device_id i2c_pmic_id[] = {
> +	{ "i2c-pmic", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, i2c_pmic_id);

If you use probe_new, you can remove this table altogether.

> +static struct i2c_driver i2c_pmic_driver = {
> +	.driver		= {
> +		.name		= "i2c_pmic",
> +		.pm		= &i2c_pmic_pm_ops,
> +		.of_match_table	= i2c_pmic_match_table,
> +	},
> +	.probe		= i2c_pmic_probe,
> +	.remove		= i2c_pmic_remove,
> +	.id_table	= i2c_pmic_id,
> +};
> +

Remove this line.

> +module_i2c_driver(i2c_pmic_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("i2c:i2c_pmic");

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog