[PATCH v1 0/2] Introduce QTI I2C PMIC controller

[PATCH v1 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     |  86 +++
 drivers/mfd/Kconfig                                |  11 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/qcom-i2c-pmic.c                        | 737 +++++++++++++++++++++
 4 files changed, 835 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 v1 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>
---
Changes from v0:
- Fixed "FATAL ERROR: Unable to parse input tree" error thrown by `make
  dt_binding_check`.

 .../devicetree/bindings/mfd/qcom,i2c-pmic.yaml     | 86 ++++++++++++++++++++++
 1 file changed, 86 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..42482af
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
@@ -0,0 +1,86 @@
+# 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:
+  - |
+    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 v1 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 v1 2/2] mfd: Introduce QTI I2C PMIC controller

[Lee Jones]

On Tue, 28 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 ++++++++++++++++++++++++++++++++++++++++++++

The vast majority of this driver deals with IRQ handling.  Why can't
this be split out into its own IRQ Chip driver and moved to
drivers/irqchip?

>  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

Too generic.  This should identify the vendor too.

> +	tristate "QTI I2C PMIC support"

Why aren't you using QCOM?

Actually, this should be expanded here anyway.

> +	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 very out of date.

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

Please don't role your own debug helpers.

The ones the kernel provides are suitably proficient.

> +#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 */

"Not"

It doesn't matter if the value is not used.

I think you can drop the comment.

> +	IRQ_EN_SET,
> +	IRQ_MAX_REGS,
> +};

Going to stop here for a second, as the vast majority of the remainder
of the driver appears to surround IRQ management.

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

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

[Lee Jones]

On Tue, 28 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.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
> Changes from v0:
> - Fixed "FATAL ERROR: Unable to parse input tree" error thrown by `make
>   dt_binding_check`.
> 
>  .../devicetree/bindings/mfd/qcom,i2c-pmic.yaml     | 86 ++++++++++++++++++++++
>  1 file changed, 86 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..42482af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
> @@ -0,0 +1,86 @@
> +# 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:
> +  - |
> +    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>;

Please provide examples of this device's children.

> +    };
> +
> +...

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

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

[Rob Herring]

On Tue, 28 Apr 2020 17:30:11 -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>
> ---
> Changes from v0:
> - Fixed "FATAL ERROR: Unable to parse input tree" error thrown by `make
>   dt_binding_check`.
> 
>  .../devicetree/bindings/mfd/qcom,i2c-pmic.yaml     | 86 ++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.yaml
> 

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

Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dts:19.11-23: Warning (reg_format): /example-0/qcom,smb138x@8:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/mfd/qcom,i2c-pmic.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'

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

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 v1 2/2] mfd: Introduce QTI I2C PMIC controller

[Guru Das Srinagesh]

On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> On Tue, 28 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 ++++++++++++++++++++++++++++++++++++++++++++
> 
> The vast majority of this driver deals with IRQ handling.  Why can't
> this be split out into its own IRQ Chip driver and moved to
> drivers/irqchip?

There appear to be quite a few in-tree MFD drivers that register IRQ
controllers, like this driver does:

$ grep --exclude-dir=.git -rnE "irq_domain_(add|create).+\(" drivers/mfd | wc -l
23

As a further example, drivers/mfd/stpmic1.c closely resembles this
driver in that it uses both devm_regmap_add_irq_chip() as well as
devm_of_platform_populate().

As such, it seems like this driver is in line with some of the
architectural choices that have been accepted in already-merged drivers.
Could you please elaborate on your concerns?

> 
> >  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
> 
> Too generic.  This should identify the vendor too.

Will change to MFD_QCOM_I2C_PMIC.

> 
> > +	tristate "QTI I2C PMIC support"
> 
> Why aren't you using QCOM?
> 
> Actually, this should be expanded here anyway.

Will expand this to "Qualcomm Technologies, Inc. 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.
> 
> This is very out of date.

It is internal company policy to update the copyright year only for
those years when a change was made to the driver. Since this driver hasn't
been modified since 2018, the copyright year has also been static since
then. Otherwise, the driver is very much current functionality-wise.

What modification would you suggest for the copyright year?

> 
> > + */
> > +
> > +#define pr_fmt(fmt) "I2C PMIC: %s: " fmt, __func__
> 
> Please don't role your own debug helpers.
> 
> The ones the kernel provides are suitably proficient.

Sure. Would this be acceptable instead, with the custom string replaced by a
macro that the kernel provides?

	#define pr_fmt(fmt) "%s: %s: " fmt, KBUILD_MODNAME, __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 */
> 
> "Not"
> 
> It doesn't matter if the value is not used.
> 
> I think you can drop the comment.

Done.

> 
> > +	IRQ_EN_SET,
> > +	IRQ_MAX_REGS,
> > +};
> 
> Going to stop here for a second, as the vast majority of the remainder
> of the driver appears to surround IRQ management.

(Please see my first comment above.)

Thank you.

Guru Das.

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

[Joe Perches]

On Thu, 2020-04-30 at 18:13 -0700, Guru Das Srinagesh wrote:
> On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> > On Tue, 28 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.
[]
> > > diff --git a/drivers/mfd/qcom-i2c-pmic.c b/drivers/mfd/qcom-i2c-pmic.c
[]
> > Please don't role your own debug helpers.
> > 
> > The ones the kernel provides are suitably proficient.
> 
> Sure. Would this be acceptable instead, with the custom string replaced by a
> macro that the kernel provides?
> 
> 	#define pr_fmt(fmt) "%s: %s: " fmt, KBUILD_MODNAME, __func__

trivia:

It's almost always smaller object code to use
the KBUILD_MODNAME as a fixed string instead of
as a printf argument.

	#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__

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

[Guru Das Srinagesh]

On Thu, Apr 30, 2020 at 06:18:18PM -0700, Joe Perches wrote:
> On Thu, 2020-04-30 at 18:13 -0700, Guru Das Srinagesh wrote:
> > On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> > > On Tue, 28 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.
> []
> > > > diff --git a/drivers/mfd/qcom-i2c-pmic.c b/drivers/mfd/qcom-i2c-pmic.c
> []
> > > Please don't role your own debug helpers.
> > > 
> > > The ones the kernel provides are suitably proficient.
> > 
> > Sure. Would this be acceptable instead, with the custom string replaced by a
> > macro that the kernel provides?
> > 
> > 	#define pr_fmt(fmt) "%s: %s: " fmt, KBUILD_MODNAME, __func__
> 
> trivia:
> 
> It's almost always smaller object code to use
> the KBUILD_MODNAME as a fixed string instead of
> as a printf argument.
> 
> 	#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__

Thanks, duly noted :)

Thank you.

Guru Das.

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

[Lee Jones]

On Thu, 30 Apr 2020, Guru Das Srinagesh wrote:

> On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> > On Tue, 28 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 ++++++++++++++++++++++++++++++++++++++++++++
> > 
> > The vast majority of this driver deals with IRQ handling.  Why can't
> > this be split out into its own IRQ Chip driver and moved to
> > drivers/irqchip?
> 
> There appear to be quite a few in-tree MFD drivers that register IRQ
> controllers, like this driver does:
> 
> $ grep --exclude-dir=.git -rnE "irq_domain_(add|create).+\(" drivers/mfd | wc -l
> 23
> 
> As a further example, drivers/mfd/stpmic1.c closely resembles this
> driver in that it uses both devm_regmap_add_irq_chip() as well as
> devm_of_platform_populate().
> 
> As such, it seems like this driver is in line with some of the
> architectural choices that have been accepted in already-merged drivers.
> Could you please elaborate on your concerns?

It is true that *basic* IRQ domain support has been added to these
drivers in the past.  However, IMHO the support added to this driver
goes beyond those realms such that it would justify a driver of its
own.

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

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

[Guru Das Srinagesh]

On Fri, May 15, 2020 at 11:45:20AM +0100, Lee Jones wrote:
> On Thu, 30 Apr 2020, Guru Das Srinagesh wrote:
> 
> > On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> > > On Tue, 28 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 ++++++++++++++++++++++++++++++++++++++++++++
> > > 
> > > The vast majority of this driver deals with IRQ handling.  Why can't
> > > this be split out into its own IRQ Chip driver and moved to
> > > drivers/irqchip?
> > 
> > There appear to be quite a few in-tree MFD drivers that register IRQ
> > controllers, like this driver does:
> > 
> > $ grep --exclude-dir=.git -rnE "irq_domain_(add|create).+\(" drivers/mfd | wc -l
> > 23
> > 
> > As a further example, drivers/mfd/stpmic1.c closely resembles this
> > driver in that it uses both devm_regmap_add_irq_chip() as well as
> > devm_of_platform_populate().
> > 
> > As such, it seems like this driver is in line with some of the
> > architectural choices that have been accepted in already-merged drivers.
> > Could you please elaborate on your concerns?
> 
> It is true that *basic* IRQ domain support has been added to these
> drivers in the past.  However, IMHO the support added to this driver
> goes beyond those realms such that it would justify a driver of its
> own.

I am exploring an option to see if the regmap-irq APIs may be used in
this driver, similar to stpmic1.c. Just to let you know, it might be a
few days before I am able to post my next patchset as I'll have to make
the necessary changes and test them out first.

Thank you.

Guru Das.

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

[Lee Jones]

On Tue, 19 May 2020, Guru Das Srinagesh wrote:

> On Fri, May 15, 2020 at 11:45:20AM +0100, Lee Jones wrote:
> > On Thu, 30 Apr 2020, Guru Das Srinagesh wrote:
> > 
> > > On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> > > > On Tue, 28 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 ++++++++++++++++++++++++++++++++++++++++++++
> > > > 
> > > > The vast majority of this driver deals with IRQ handling.  Why can't
> > > > this be split out into its own IRQ Chip driver and moved to
> > > > drivers/irqchip?
> > > 
> > > There appear to be quite a few in-tree MFD drivers that register IRQ
> > > controllers, like this driver does:
> > > 
> > > $ grep --exclude-dir=.git -rnE "irq_domain_(add|create).+\(" drivers/mfd | wc -l
> > > 23
> > > 
> > > As a further example, drivers/mfd/stpmic1.c closely resembles this
> > > driver in that it uses both devm_regmap_add_irq_chip() as well as
> > > devm_of_platform_populate().
> > > 
> > > As such, it seems like this driver is in line with some of the
> > > architectural choices that have been accepted in already-merged drivers.
> > > Could you please elaborate on your concerns?
> > 
> > It is true that *basic* IRQ domain support has been added to these
> > drivers in the past.  However, IMHO the support added to this driver
> > goes beyond those realms such that it would justify a driver of its
> > own.
> 
> I am exploring an option to see if the regmap-irq APIs may be used in
> this driver, similar to stpmic1.c. Just to let you know, it might be a
> few days before I am able to post my next patchset as I'll have to make
> the necessary changes and test them out first.

Take your time.

The next release is due imminently, so you have as long as you need.

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