[PATCH v2 0/3] Add support for Qualcomm MFD PMIC register layout

[PATCH v2 0/3] Add support for Qualcomm MFD PMIC register layout

[Guru Das Srinagesh]

Changes from v1:
- Fixed YAML errors in dt binding document.

This is a follow-up as promised [1] to the earlier attempts [2] [3] to upstream
the driver that has been hitherto used to handle IRQs for Qualcomm's PMICs that
have multiple on-board peripherals when they are interfaced over the I2C
interface.

This series is a rewrite of that driver while making use of the regmap-irq
framework, which needs some modifications to handle the register layout of
Qualcomm's PMICs. This is an RFC because I would like to get feedback on my
general approach before submitting as a patch per se.

Upon inspection of the regmap-irq framework, it was observed that the
downstream driver was essentially replicating the framework's IRQ handling
logic (such as adding an IRQ domain, and the interrupt handler thread that
reads sub-irqs from a main status register). It was also observed that the
framework could not be used as-is because:
- Qualcomm's PMIC peripheral register layout does not follow a fixed
  irq_reg_stride, and
- The "IRQ TYPE" configuration register takes one bit per interrupt, which when
  set configures that interrupt as Edge triggered, and when cleared sets it to
  Level triggered.
- There are two IRQ configuration registers in addition to "IRQ TYPE" that
  further configure the IRQ type as triggered by rising-edge/level high or
  alternatively, falling-edge/level low that have no support in the regmap-irq
  framework currently.

This patch series has been tested on an internal platform using PM8008 as a
test MFD PMIC chip. PM8008 is a PMIC that contains 7 LDOs, 2 GPIOs, temperature
monitoring, and can be interfaced over I2C.

Both the framework modifications as well as the chip driver
have been submitted here for review. Some details about the specific
differences between the framework and QCOM PMICs' register layout are provided
below using PM8008 as an example.

[PM8008 peripheral register layout]

Of all the peripherals in PM8008, only a few need IRQ support. They are laid
out at the following base addresses (only four are added at the moment for
simplicity):

	0x0900, 0x2400, 0xC000, 0xC100

Each peripheral is allocated a uniform size of 0x100 bytes and its IRQs are
configured through a set of registers that are located at fixed offsets from
the above base addresses, uniformly:

	Register name	       Addr	regmap-irq equivalent	Comment
	-----------------------------------------------------------------------
	INT_RT_STS_OFFSET      0x10	(no equivalent)		See #1 below
	INT_SET_TYPE_OFFSET    0x11	type_base 		See #2 below
	INT_POL_HIGH_OFFSET    0x12	(no equivalent)		See #3 below
	INT_POL_LOW_OFFSET     0x13	(no equivalent)		See #3 below
	INT_LATCHED_CLR_OFFSET 0x14	ack_base
	INT_EN_SET_OFFSET      0x15	unmask_base		See #4 below
	INT_EN_CLR_OFFSET      0x16	mask_base		See #4 below
	INT_LATCHED_STS_OFFSET 0x18	status_base

Comments (all registers are one bit per interrupt):
1. INT_RT_STS_OFFSET is not used by the regmap-irq, so it may be ignored.
2. INT_SET_TYPE_OFFSET: 1 for edge trigger, 0 for level trigger.
3. Support needs to be added for writing to INT_POL_HIGH_OFFSET and
   INT_POL_LOW_OFFSET correctly in the framework. Set to 1 or 0 to enable or
   disable rising-edge/level high or falling-edge/level low.
4. Even though INT_EN_SET_OFFSET and INT_EN_CLR_OFFSET map to unmask_base and
   mask_base in the regmap-irq framework conceptually, they are swapped in the
   chip driver because `unmask_offset` in the framework expects unmask_base to
   be larger than mask_base numerically. This has to be kept in mind while
   reviewing the "mfd: Add PM8008 driver" patch below.

[Summary of framework changes]

The main thrust of the changes is to introduce an array of peripheral offset
values, which are to be added to the *_base addresses in order to arrive at the
correct register addresses per peripheral. In order to get at the first
peripheral's addresses, the first element of this array must be zero.

Since there are two new registers (INT_POL_HIGH_OFFSET and INT_POL_LOW_OFFSET),
add support for storing the per-peripheral values and also writing to them.
These will be used only if peripheral offsets are specified.

[1] https://lore.kernel.org/lkml/20200519185757.GA13992@codeaurora.org/
[2] https://lore.kernel.org/lkml/cover.1588037638.git.gurus@codeaurora.org/
[3] https://lore.kernel.org/lkml/cover.1588115326.git.gurus@codeaurora.org/


Guru Das Srinagesh (3):
  regmap-irq: Add support for peripheral offsets
  dt-bindings: mfd: Add QCOM PM8008 MFD bindings
  mfd: Add PM8008 driver

 .../bindings/mfd/qcom,pm8008-irqchip.yaml          | 102 +++++++++++
 drivers/base/regmap/regmap-irq.c                   | 191 ++++++++++++++++----
 drivers/mfd/Kconfig                                |  14 ++
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/qcom-pm8008.c                          | 197 +++++++++++++++++++++
 include/linux/regmap.h                             |   6 +
 6 files changed, 477 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
 create mode 100644 drivers/mfd/qcom-pm8008.c

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

[PATCH v2 1/3] regmap-irq: Add support for peripheral offsets

[Guru Das Srinagesh]

Some MFD chips do not have the register space for their peripherals
mapped out with a fixed stride. Add peripheral address offsets to the
framework to support such address spaces.

In this new scheme, the regmap-irq client registering with the framework
shall have to define the *_base registers (e.g. status_base, mask_base,
type_base, etc.) as those of the very first peripheral in the chip, and
then specify address offsets of each subsequent peripheral so that their
corresponding *_base registers may be calculated by the framework. The
first element of the periph_offs array must be zero so that the first
peripherals' addresses may be accessed.

Some MFD chips define two registers in addition to the IRQ type
registers: POLARITY_HI and POLARITY_LO, so add support to manage their
data as well as write to them.

Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 drivers/base/regmap/regmap-irq.c | 191 ++++++++++++++++++++++++++++++++-------
 include/linux/regmap.h           |   6 ++
 2 files changed, 163 insertions(+), 34 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index ad5c2de..dbf2c86 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -38,6 +38,8 @@ struct regmap_irq_chip_data {
 	unsigned int *wake_buf;
 	unsigned int *type_buf;
 	unsigned int *type_buf_def;
+	unsigned int *polarity_hi_buf;
+	unsigned int *polarity_lo_buf;
 
 	unsigned int irq_reg_stride;
 	unsigned int type_reg_stride;
@@ -87,8 +89,13 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 
 	if (d->clear_status) {
 		for (i = 0; i < d->chip->num_regs; i++) {
-			reg = d->chip->status_base +
-				(i * map->reg_stride * d->irq_reg_stride);
+			if (d->chip->periph_offs)
+				reg = d->chip->status_base +
+					d->chip->periph_offs[i];
+			else
+				reg = d->chip->status_base +
+					(i * map->reg_stride *
+					 d->irq_reg_stride);
 
 			ret = regmap_read(map, reg, &val);
 			if (ret)
@@ -108,8 +115,13 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		if (!d->chip->mask_base)
 			continue;
 
-		reg = d->chip->mask_base +
-			(i * map->reg_stride * d->irq_reg_stride);
+		if (d->chip->periph_offs)
+			reg = d->chip->mask_base +
+				d->chip->periph_offs[i];
+		else
+			reg = d->chip->mask_base +
+				(i * map->reg_stride * d->irq_reg_stride);
+
 		if (d->chip->mask_invert) {
 			ret = regmap_irq_update_bits(d, reg,
 					 d->mask_buf_def[i], ~d->mask_buf[i]);
@@ -136,8 +148,13 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 			dev_err(d->map->dev, "Failed to sync masks in %x\n",
 				reg);
 
-		reg = d->chip->wake_base +
-			(i * map->reg_stride * d->irq_reg_stride);
+		if (d->chip->periph_offs)
+			reg = d->chip->wake_base +
+				d->chip->periph_offs[i];
+		else
+			reg = d->chip->wake_base +
+				(i * map->reg_stride * d->irq_reg_stride);
+
 		if (d->wake_buf) {
 			if (d->chip->wake_invert)
 				ret = regmap_irq_update_bits(d, reg,
@@ -161,8 +178,14 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		 * it'll be ignored in irq handler, then may introduce irq storm
 		 */
 		if (d->mask_buf[i] && (d->chip->ack_base || d->chip->use_ack)) {
-			reg = d->chip->ack_base +
-				(i * map->reg_stride * d->irq_reg_stride);
+			if (d->chip->periph_offs)
+				reg = d->chip->ack_base +
+					d->chip->periph_offs[i];
+			else
+				reg = d->chip->ack_base +
+					(i * map->reg_stride *
+					 d->irq_reg_stride);
+
 			/* some chips ack by write 0 */
 			if (d->chip->ack_invert)
 				ret = regmap_write(map, reg, ~d->mask_buf[i]);
@@ -187,8 +210,14 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		for (i = 0; i < d->chip->num_type_reg; i++) {
 			if (!d->type_buf_def[i])
 				continue;
-			reg = d->chip->type_base +
-				(i * map->reg_stride * d->type_reg_stride);
+			if (d->chip->periph_offs)
+				reg = d->chip->type_base +
+					d->chip->periph_offs[i];
+			else
+				reg = d->chip->type_base +
+					(i * map->reg_stride *
+					 d->type_reg_stride);
+
 			if (d->chip->type_invert)
 				ret = regmap_irq_update_bits(d, reg,
 					d->type_buf_def[i], ~d->type_buf[i]);
@@ -198,6 +227,25 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 			if (ret != 0)
 				dev_err(d->map->dev, "Failed to sync type in %x\n",
 					reg);
+
+			if (!d->chip->periph_offs ||
+			    !d->chip->polarity_hi_base ||
+			    !d->chip->polarity_lo_base)
+				continue;
+
+			reg = d->chip->polarity_hi_base +
+				d->chip->periph_offs[i];
+			ret = regmap_write(map, reg, d->polarity_hi_buf[i]);
+			if (ret != 0)
+				dev_err(d->map->dev, "Failed to sync polarity hi in %x\n",
+					reg);
+
+			reg = d->chip->polarity_lo_base +
+				d->chip->periph_offs[i];
+			ret = regmap_write(map, reg, d->polarity_lo_buf[i]);
+			if (ret != 0)
+				dev_err(d->map->dev, "Failed to sync polarity lo in %x\n",
+					reg);
 		}
 	}
 
@@ -280,23 +328,49 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
 	switch (type) {
 	case IRQ_TYPE_EDGE_FALLING:
 		d->type_buf[reg] |= t->type_falling_val;
+		if (d->chip->periph_offs) {
+			d->polarity_hi_buf[reg] &= ~t->type_falling_val;
+			d->polarity_lo_buf[reg] |= t->type_falling_val;
+		}
 		break;
 
 	case IRQ_TYPE_EDGE_RISING:
 		d->type_buf[reg] |= t->type_rising_val;
+		if (d->chip->periph_offs) {
+			d->polarity_hi_buf[reg] |= t->type_rising_val;
+			d->polarity_lo_buf[reg] &= ~t->type_rising_val;
+		}
 		break;
 
 	case IRQ_TYPE_EDGE_BOTH:
 		d->type_buf[reg] |= (t->type_falling_val |
 					t->type_rising_val);
+		if (d->chip->periph_offs) {
+			d->polarity_hi_buf[reg] |= (t->type_falling_val |
+					t->type_rising_val);
+			d->polarity_lo_buf[reg] |= (t->type_falling_val |
+					t->type_rising_val);
+		}
 		break;
 
 	case IRQ_TYPE_LEVEL_HIGH:
-		d->type_buf[reg] |= t->type_level_high_val;
+		if (!d->chip->periph_offs) {
+			d->type_buf[reg] |= t->type_level_high_val;
+		} else {
+			d->type_buf[reg] &= ~t->type_level_high_val;
+			d->polarity_hi_buf[reg] |= t->type_level_high_val;
+			d->polarity_lo_buf[reg] &= ~t->type_level_high_val;
+		}
 		break;
 
 	case IRQ_TYPE_LEVEL_LOW:
-		d->type_buf[reg] |= t->type_level_low_val;
+		if (!d->chip->periph_offs) {
+			d->type_buf[reg] |= t->type_level_low_val;
+		} else {
+			d->type_buf[reg] &= ~t->type_level_low_val;
+			d->polarity_hi_buf[reg] &= ~t->type_level_low_val;
+			d->polarity_lo_buf[reg] |= t->type_level_low_val;
+		}
 		break;
 	default:
 		return -EINVAL;
@@ -342,12 +416,10 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
 	struct regmap_irq_sub_irq_map *subreg;
 	int i, ret = 0;
 
-	if (!chip->sub_reg_offsets) {
-		/* Assume linear mapping */
-		ret = regmap_read(map, chip->status_base +
-				  (b * map->reg_stride * data->irq_reg_stride),
-				   &data->status_buf[b]);
-	} else {
+	if (chip->periph_offs) {
+		ret = regmap_read(map, chip->status_base + chip->periph_offs[b],
+				&data->status_buf[b]);
+	} else if (chip->sub_reg_offsets) {
 		subreg = &chip->sub_reg_offsets[b];
 		for (i = 0; i < subreg->num_regs; i++) {
 			unsigned int offset = subreg->offset[i];
@@ -357,6 +429,11 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
 			if (ret)
 				break;
 		}
+	} else {
+		/* Assume linear mapping */
+		ret = regmap_read(map, chip->status_base +
+				  (b * map->reg_stride * data->irq_reg_stride),
+				   &data->status_buf[b]);
 	}
 	return ret;
 }
@@ -474,10 +551,14 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 
 	} else {
 		for (i = 0; i < data->chip->num_regs; i++) {
-			ret = regmap_read(map, chip->status_base +
-					  (i * map->reg_stride
-					   * data->irq_reg_stride),
-					  &data->status_buf[i]);
+			if (chip->periph_offs)
+				reg = chip->status_base + chip->periph_offs[i];
+			else
+				reg = chip->status_base +
+					(i * map->reg_stride *
+					 data->irq_reg_stride);
+
+			ret = regmap_read(map, reg, &data->status_buf[i]);
 
 			if (ret != 0) {
 				dev_err(map->dev,
@@ -499,8 +580,13 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		data->status_buf[i] &= ~data->mask_buf[i];
 
 		if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) {
-			reg = chip->ack_base +
-				(i * map->reg_stride * data->irq_reg_stride);
+			if (chip->periph_offs)
+				reg = chip->ack_base + chip->periph_offs[i];
+			else
+				reg = chip->ack_base +
+					(i * map->reg_stride *
+					 data->irq_reg_stride);
+
 			if (chip->ack_invert)
 				ret = regmap_write(map, reg,
 						~data->status_buf[i]);
@@ -662,6 +748,18 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			goto err_alloc;
 	}
 
+	if (chip->periph_offs) {
+		d->polarity_hi_buf = kcalloc(chip->num_regs,
+					     sizeof(unsigned int), GFP_KERNEL);
+		if (!d->polarity_hi_buf)
+			goto err_alloc;
+
+		d->polarity_lo_buf = kcalloc(chip->num_regs,
+					     sizeof(unsigned int), GFP_KERNEL);
+		if (!d->polarity_lo_buf)
+			goto err_alloc;
+	}
+
 	d->irq_chip = regmap_irq_chip;
 	d->irq_chip.name = chip->name;
 	d->irq = irq;
@@ -700,8 +798,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		if (!chip->mask_base)
 			continue;
 
-		reg = chip->mask_base +
-			(i * map->reg_stride * d->irq_reg_stride);
+		if (chip->periph_offs)
+			reg = chip->mask_base + chip->periph_offs[i];
+		else
+			reg = chip->mask_base +
+				(i * map->reg_stride * d->irq_reg_stride);
+
 		if (chip->mask_invert)
 			ret = regmap_irq_update_bits(d, reg,
 					 d->mask_buf[i], ~d->mask_buf[i]);
@@ -725,8 +827,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			continue;
 
 		/* Ack masked but set interrupts */
-		reg = chip->status_base +
-			(i * map->reg_stride * d->irq_reg_stride);
+		if (chip->periph_offs)
+			reg = chip->status_base + chip->periph_offs[i];
+		else
+			reg = chip->status_base +
+				(i * map->reg_stride * d->irq_reg_stride);
+
 		ret = regmap_read(map, reg, &d->status_buf[i]);
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
@@ -735,8 +841,13 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 
 		if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
-			reg = chip->ack_base +
-				(i * map->reg_stride * d->irq_reg_stride);
+			if (chip->periph_offs)
+				reg = chip->ack_base + chip->periph_offs[i];
+			else
+				reg = chip->ack_base +
+					(i * map->reg_stride *
+					 d->irq_reg_stride);
+
 			if (chip->ack_invert)
 				ret = regmap_write(map, reg,
 					~(d->status_buf[i] & d->mask_buf[i]));
@@ -765,8 +876,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	if (d->wake_buf) {
 		for (i = 0; i < chip->num_regs; i++) {
 			d->wake_buf[i] = d->mask_buf_def[i];
-			reg = chip->wake_base +
-				(i * map->reg_stride * d->irq_reg_stride);
+			if (chip->periph_offs)
+				reg = chip->wake_base + chip->periph_offs[i];
+			else
+				reg = chip->wake_base +
+					(i * map->reg_stride *
+					 d->irq_reg_stride);
 
 			if (chip->wake_invert)
 				ret = regmap_irq_update_bits(d, reg,
@@ -786,8 +901,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 
 	if (chip->num_type_reg && !chip->type_in_mask) {
 		for (i = 0; i < chip->num_type_reg; ++i) {
-			reg = chip->type_base +
-				(i * map->reg_stride * d->type_reg_stride);
+			if (chip->periph_offs)
+				reg = chip->type_base + chip->periph_offs[i];
+			else
+				reg = chip->type_base +
+					(i * map->reg_stride *
+					 d->type_reg_stride);
 
 			ret = regmap_read(map, reg, &d->type_buf_def[i]);
 
@@ -833,6 +952,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	/* Should really dispose of the domain but... */
 err_alloc:
 	kfree(d->type_buf);
+	kfree(d->polarity_hi_buf);
+	kfree(d->polarity_lo_buf);
 	kfree(d->type_buf_def);
 	kfree(d->wake_buf);
 	kfree(d->mask_buf_def);
@@ -903,6 +1024,8 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 
 	irq_domain_remove(d->domain);
 	kfree(d->type_buf);
+	kfree(d->polarity_hi_buf);
+	kfree(d->polarity_lo_buf);
 	kfree(d->type_buf_def);
 	kfree(d->wake_buf);
 	kfree(d->mask_buf_def);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e7834d9..6fb1090 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1338,6 +1338,7 @@ struct regmap_irq_sub_irq_map {
  *		     status_base. Should contain num_regs arrays.
  *		     Can be provided for chips with more complex mapping than
  *		     1.st bit to 1.st sub-reg, 2.nd bit to 2.nd sub-reg, ...
+ * @periph_offs:    Array of addresses of peripherals, should be num_reg long.
  * @num_main_regs: Number of 'main status' irq registers for chips which have
  *		   main_status set.
  *
@@ -1350,6 +1351,8 @@ struct regmap_irq_sub_irq_map {
  *               Using zero value is possible with @use_ack bit.
  * @wake_base:   Base address for wake enables.  If zero unsupported.
  * @type_base:   Base address for irq type.  If zero unsupported.
+ * @polarity_hi_base:   Base address for polarity high when periph_offs is used.
+ * @polarity_lo_base:   Base address for polarity low when periph_offs is used.
  * @irq_reg_stride:  Stride to use for chips where registers are not contiguous.
  * @init_ack_masked: Ack all masked interrupts once during initalization.
  * @mask_invert: Inverted mask register: cleared bits are masked out.
@@ -1390,6 +1393,7 @@ struct regmap_irq_chip {
 	unsigned int main_status;
 	unsigned int num_main_status_bits;
 	struct regmap_irq_sub_irq_map *sub_reg_offsets;
+	unsigned int *periph_offs;
 	int num_main_regs;
 
 	unsigned int status_base;
@@ -1398,6 +1402,8 @@ struct regmap_irq_chip {
 	unsigned int ack_base;
 	unsigned int wake_base;
 	unsigned int type_base;
+	unsigned int polarity_hi_base;
+	unsigned int polarity_lo_base;
 	unsigned int irq_reg_stride;
 	bool mask_writeonly:1;
 	bool init_ack_masked:1;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/3] dt-bindings: mfd: Add QCOM PM8008 MFD bindings

[Guru Das Srinagesh]

Add device tree bindings for the driver for Qualcomm Technology Inc.'s
PM8008 MFD PMIC.

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

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
new file mode 100644
index 0000000..31d7b68
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
@@ -0,0 +1,102 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/qcom,pm8008-irqchip.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. PM8008 Multi-Function Device PMIC
+
+maintainers:
+  - Guru Das Srinagesh <gurus@codeaurora.org>
+
+description: |
+  PM8008 is a PMIC that contains 7 LDOs, 2 GPIOs, temperature monitoring, and
+  can be interfaced over I2C.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,pm8008-irqchip
+
+  reg:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: pm8008
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#address-cells":
+    const: 1
+    description: Must be specified if child nodes are specified.
+
+  "#size-cells":
+    const: 0
+    description: Must be specified if child nodes are specified.
+
+  "#interrupt-cells":
+    const: 2
+    description: |
+      The first cell is the IRQ number, the second cell is the IRQ trigger flag.
+
+patternProperties:
+  "^.*@[0-9a-f]+$":
+    type: object
+    # Each peripheral in PM8008 must be represented as a child node with an
+    # optional label for referencing as phandle elsewhere. This is optional.
+    properties:
+      compatible:
+        description: The compatible string for the peripheral's driver.
+
+      reg:
+        maxItems: 1
+
+      interrupts:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+      - interrupts
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    qupv3_se13_i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            pm8008i@8 {
+                    compatible = "qcom,pm8008-irqchip";
+                    reg = <0x8>;
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+                    interrupt-controller;
+                    #interrupt-cells = <2>;
+
+                    interrupt-names = "pm8008";
+                    interrupt-parent = <&tlmm>;
+                    interrupts = <32 IRQ_TYPE_EDGE_RISING>;
+
+                    pm8008_tz: qcom,temp-alarm@2400 {
+                            compatible = "qcom,spmi-temp-alarm";
+                            reg = <0x2400>;
+                            interrupts = <0x5 IRQ_TYPE_EDGE_BOTH>;
+                            #thermal-sensor-cells = <0>;
+                    };
+            };
+    };
+
+...
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/3] mfd: Add PM8008 driver

[Guru Das Srinagesh]

PM8008 is a PMIC that contains 7 LDOs, 2 GPIOs, temperature monitoring,
and can be interfaced over I2C. This driver uses the regmap-irq
framework to handle interrupts, creates a regmap and uses it to
instantiate all the child nodes under it in the device tree.

Only four peripherals have been added at the moment.

Every peripheral that has its TYPE register's hw default value as
zero must have it set to an all-ones mask (to cover all interrupts it
supports) in pm8008_init(). This is as per commit 84267d1b18ab ("regmap:
regmap-irq: Remove default irq type setting from core").

Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 drivers/mfd/Kconfig       |  14 ++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/qcom-pm8008.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 drivers/mfd/qcom-pm8008.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df083..3d4e989 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1960,6 +1960,20 @@ config MFD_ROHM_BD70528
 	  10 bits SAR ADC for battery temperature monitor and 1S battery
 	  charger.
 
+config MFD_QCOM_PM8008
+	tristate "QCOM PM8008 Power Management IC"
+	depends on I2C && OF
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	help
+	  Select this option to get support for the PM8008 PMIC chip. PM8008 is
+	  a low-cost PMIC that contains 7 LDOs, 2 GPIOs, temperature
+	  monitoring, and can be interfaced over I2C. This driver provides
+	  common support for accessing the device by instantiating all the
+	  child nodes under it in the device tree and, therefore, additional
+	  drivers must be enabled in order to use the functionality of the
+	  device.
+
 config MFD_ROHM_BD71828
 	tristate "ROHM BD71828 Power Management IC"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f8..e316064 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -260,6 +260,7 @@ obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD70528)	+= rohm-bd70528.o
 obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
+obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o
 obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
new file mode 100644
index 0000000..56a5951
--- /dev/null
+++ b/drivers/mfd/qcom-pm8008.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#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/regmap.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 PM8008_NUM_PERIPHS	4
+
+#define PM8008_PERIPH_0_BASE	0x900
+#define PM8008_PERIPH_1_BASE	0x2400
+#define PM8008_PERIPH_2_BASE	0xC000
+#define PM8008_PERIPH_3_BASE	0xC100
+
+#define PM8008_TEMP_ALARM_ADDR	PM8008_PERIPH_1_BASE
+#define PM8008_TEMP_ALARM_EN	0x1
+
+#define PM8008_STATUS_BASE	(PM8008_PERIPH_0_BASE | INT_LATCHED_STS_OFFSET)
+#define PM8008_MASK_BASE	(PM8008_PERIPH_0_BASE | INT_EN_SET_OFFSET)
+#define PM8008_UNMASK_BASE	(PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET)
+#define PM8008_TYPE_BASE	(PM8008_PERIPH_0_BASE | INT_SET_TYPE_OFFSET)
+#define PM8008_ACK_BASE		(PM8008_PERIPH_0_BASE | INT_LATCHED_CLR_OFFSET)
+#define PM8008_POLARITY_HI_BASE	(PM8008_PERIPH_0_BASE | INT_POL_HIGH_OFFSET)
+#define PM8008_POLARITY_LO_BASE	(PM8008_PERIPH_0_BASE | INT_POL_LOW_OFFSET)
+
+#define ADDRESS_OFFSET(paddr, base)	(paddr - base)
+
+#define PM8008_PERIPH_OFFSET(paddr)	\
+	ADDRESS_OFFSET(paddr, PM8008_PERIPH_0_BASE)
+
+struct pm8008_data {
+	struct device *dev;
+	struct regmap *regmap;
+	int irq;
+	struct regmap_irq_chip_data *irq_data;
+};
+
+unsigned int pm8008_periph_offs[] = {
+	PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE),
+	PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE),
+	PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE),
+	PM8008_PERIPH_OFFSET(PM8008_PERIPH_3_BASE),
+};
+
+/* Need to define enums for the interrupt numbers and masks */
+static struct regmap_irq pm8008_irqs[] = {
+	/* MISC IRQs */
+	REGMAP_IRQ_REG(0,	0,	BIT(0)),
+	REGMAP_IRQ_REG(1,	0,	BIT(1)),
+	REGMAP_IRQ_REG(2,	0,	BIT(2)),
+	REGMAP_IRQ_REG(3,	0,	BIT(3)),
+	REGMAP_IRQ_REG(4,	0,	BIT(4)),
+	/* TEMP ALARM IRQs */
+	REGMAP_IRQ_REG(5,	1,	BIT(0)),
+	/* GPIO1 IRQs */
+	REGMAP_IRQ_REG(6,	2,	BIT(0)),
+	/* GPIO2 IRQs */
+	REGMAP_IRQ_REG(7,	3,	BIT(0)),
+};
+
+static struct regmap_irq_chip pm8008_irq_chip = {
+	.name			= "pm8008_irq",
+	.main_status		= I2C_INTR_STATUS_BASE,
+	.num_main_regs		= 1,
+	.irqs			= pm8008_irqs,
+	.num_irqs		= ARRAY_SIZE(pm8008_irqs),
+	.num_regs		= PM8008_NUM_PERIPHS,
+	.periph_offs		= pm8008_periph_offs,
+	.status_base		= PM8008_STATUS_BASE,
+	/*
+	 * mask_base and unmask_base are swapped (SET and CLR) because
+	 * "unmask_offset" in framework expects unmask_base to be larger than
+	 * mask_base.
+	 */
+	.mask_base		= PM8008_MASK_BASE,
+	.unmask_base		= PM8008_UNMASK_BASE,
+	.type_base		= PM8008_TYPE_BASE,
+	.ack_base		= PM8008_ACK_BASE,
+	.polarity_hi_base	= PM8008_POLARITY_HI_BASE,
+	.polarity_lo_base	= PM8008_POLARITY_LO_BASE,
+	.num_type_reg		= PM8008_NUM_PERIPHS,
+};
+
+static struct regmap_config i2c_pmic_regmap_config = {
+	.reg_bits	= 16,
+	.val_bits	= 8,
+	.max_register	= 0xFFFF,
+};
+
+static int pm8008_init(struct pm8008_data *chip)
+{
+	/*
+	 * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework
+	 * reads this as the default value instead of zero, the HW default.
+	 */
+
+	return regmap_write(chip->regmap,
+			(PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
+			PM8008_TEMP_ALARM_EN);
+}
+
+static int pm8008_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	int rc, i;
+	struct regmap_irq_type *type;
+	struct pm8008_data *chip;
+	struct regmap_irq_chip_data *irq_data;
+
+	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);
+
+	rc = pm8008_init(chip);
+	if (rc) {
+		dev_err(chip->dev, "Init failed: %d\n", rc);
+		return rc;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
+		type = &pm8008_irqs[i].type;
+
+		/* All IRQs support both edge and level triggers */
+		type->types_supported = (IRQ_TYPE_EDGE_BOTH |
+				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
+
+		type->type_reg_offset 	  = pm8008_irqs[i].reg_offset;
+		type->type_rising_val 	  = pm8008_irqs[i].mask;
+		type->type_falling_val 	  = pm8008_irqs[i].mask;
+		type->type_level_high_val = pm8008_irqs[i].mask;
+		type->type_level_low_val  = pm8008_irqs[i].mask;
+	}
+
+	rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client->irq,
+			IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
+	if (rc) {
+		dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
+		return rc;
+	}
+
+	return devm_of_platform_populate(chip->dev);
+}
+
+static int pm8008_remove(struct i2c_client *client)
+{
+	i2c_set_clientdata(client, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id pm8008_match[] = {
+	{ .compatible = "qcom,pm8008-irqchip", },
+	{ },
+};
+
+static const struct i2c_device_id i2c_pmic_id[] = {
+	{ "qcom-i2c-pmic", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, i2c_pmic_id);
+
+static struct i2c_driver pm8008_irq_driver = {
+	.driver = {
+		.name = "pm8008-irqchip",
+		.of_match_table = pm8008_match,
+	},
+	.probe 		= pm8008_probe,
+	.remove 	= pm8008_remove,
+	.id_table	= i2c_pmic_id,
+};
+module_i2c_driver(pm8008_irq_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("i2c:qcom-pm8008");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/3] dt-bindings: mfd: Add QCOM PM8008 MFD bindings

[Rob Herring]

On Thu, Oct 22, 2020 at 02:35:41PM -0700, Guru Das Srinagesh wrote:
> Add device tree bindings for the driver for Qualcomm Technology Inc.'s
> PM8008 MFD PMIC.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  .../bindings/mfd/qcom,pm8008-irqchip.yaml          | 102 +++++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
> new file mode 100644
> index 0000000..31d7b68
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/qcom,pm8008-irqchip.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. PM8008 Multi-Function Device PMIC
> +
> +maintainers:
> +  - Guru Das Srinagesh <gurus@codeaurora.org>
> +
> +description: |
> +  PM8008 is a PMIC that contains 7 LDOs, 2 GPIOs, temperature monitoring, and
> +  can be interfaced over I2C.

No bindings for all those functions? Bindings should be complete.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,pm8008-irqchip

Why irqchip?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    items:
> +      - const: pm8008
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#address-cells":
> +    const: 1
> +    description: Must be specified if child nodes are specified.
> +
> +  "#size-cells":
> +    const: 0
> +    description: Must be specified if child nodes are specified.
> +
> +  "#interrupt-cells":
> +    const: 2
> +    description: |
> +      The first cell is the IRQ number, the second cell is the IRQ trigger flag.
> +
> +patternProperties:
> +  "^.*@[0-9a-f]+$":

'^.*' can be dropped. That's redundant.

> +    type: object
> +    # Each peripheral in PM8008 must be represented as a child node with an
> +    # optional label for referencing as phandle elsewhere. This is optional.
> +    properties:
> +      compatible:
> +        description: The compatible string for the peripheral's driver.
> +
> +      reg:
> +        maxItems: 1

What does the address represent? It's non-standard, so it needs to be 
defined.

> +
> +      interrupts:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +      - interrupts
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#interrupt-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    qupv3_se13_i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            pm8008i@8 {
> +                    compatible = "qcom,pm8008-irqchip";
> +                    reg = <0x8>;
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                    interrupt-controller;
> +                    #interrupt-cells = <2>;
> +
> +                    interrupt-names = "pm8008";
> +                    interrupt-parent = <&tlmm>;
> +                    interrupts = <32 IRQ_TYPE_EDGE_RISING>;
> +
> +                    pm8008_tz: qcom,temp-alarm@2400 {

Must be documented.

And don't use vendor prefixes in node names. 

> +                            compatible = "qcom,spmi-temp-alarm";
> +                            reg = <0x2400>;
> +                            interrupts = <0x5 IRQ_TYPE_EDGE_BOTH>;
> +                            #thermal-sensor-cells = <0>;
> +                    };
> +            };
> +    };
> +
> +...
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 2/3] dt-bindings: mfd: Add QCOM PM8008 MFD bindings

[Guru Das Srinagesh]

On Fri, Oct 30, 2020 at 10:49:00AM -0500, Rob Herring wrote:
> On Thu, Oct 22, 2020 at 02:35:41PM -0700, Guru Das Srinagesh wrote:
> > Add device tree bindings for the driver for Qualcomm Technology Inc.'s
> > PM8008 MFD PMIC.
> > 
> > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > ---
> >  .../bindings/mfd/qcom,pm8008-irqchip.yaml          | 102 +++++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
> > new file mode 100644
> > index 0000000..31d7b68
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
> > @@ -0,0 +1,102 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/qcom,pm8008-irqchip.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies, Inc. PM8008 Multi-Function Device PMIC
> > +
> > +maintainers:
> > +  - Guru Das Srinagesh <gurus@codeaurora.org>
> > +
> > +description: |
> > +  PM8008 is a PMIC that contains 7 LDOs, 2 GPIOs, temperature monitoring, and
> > +  can be interfaced over I2C.
> 
> No bindings for all those functions? Bindings should be complete.

While pushing out this patchset, I accidentally dropped the "RFC" tag in
the mail subjects. This driver and binding document are meant to be just
an exemplar for how the framework changes would be used, and hence I
felt adding only a single node would suffice for illustration purposes.

> 
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: qcom,pm8008-irqchip
> 
> Why irqchip?

Since the driver's main functions are to register with the regmap-irq
framework and to pass a regmap to the child nodes it populates. Would
"qcom,pm8008-mfd" be more appropriate?

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: pm8008
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  "#address-cells":
> > +    const: 1
> > +    description: Must be specified if child nodes are specified.
> > +
> > +  "#size-cells":
> > +    const: 0
> > +    description: Must be specified if child nodes are specified.
> > +
> > +  "#interrupt-cells":
> > +    const: 2
> > +    description: |
> > +      The first cell is the IRQ number, the second cell is the IRQ trigger flag.
> > +
> > +patternProperties:
> > +  "^.*@[0-9a-f]+$":
> 
> '^.*' can be dropped. That's redundant.

Done.

> 
> > +    type: object
> > +    # Each peripheral in PM8008 must be represented as a child node with an
> > +    # optional label for referencing as phandle elsewhere. This is optional.
> > +    properties:
> > +      compatible:
> > +        description: The compatible string for the peripheral's driver.
> > +
> > +      reg:
> > +        maxItems: 1
> 
> What does the address represent? It's non-standard, so it needs to be 
> defined.

Will add description.

> 
> > +
> > +      interrupts:
> > +        maxItems: 1
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - interrupts
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - "#interrupt-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    qupv3_se13_i2c {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            pm8008i@8 {
> > +                    compatible = "qcom,pm8008-irqchip";
> > +                    reg = <0x8>;
> > +                    #address-cells = <1>;
> > +                    #size-cells = <0>;
> > +                    interrupt-controller;
> > +                    #interrupt-cells = <2>;
> > +
> > +                    interrupt-names = "pm8008";
> > +                    interrupt-parent = <&tlmm>;
> > +                    interrupts = <32 IRQ_TYPE_EDGE_RISING>;
> > +
> > +                    pm8008_tz: qcom,temp-alarm@2400 {
> 
> Must be documented.
> 
> And don't use vendor prefixes in node names. 

Done.

> 
> > +                            compatible = "qcom,spmi-temp-alarm";
> > +                            reg = <0x2400>;
> > +                            interrupts = <0x5 IRQ_TYPE_EDGE_BOTH>;
> > +                            #thermal-sensor-cells = <0>;
> > +                    };
> > +            };
> > +    };
> > +
> > +...
> > -- 
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> > 

Re: [PATCH v2 1/3] regmap-irq: Add support for peripheral offsets

[Mark Brown]

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

On Thu, Oct 22, 2020 at 02:35:40PM -0700, Guru Das Srinagesh wrote:

> Some MFD chips do not have the register space for their peripherals
> mapped out with a fixed stride. Add peripheral address offsets to the
> framework to support such address spaces.

> In this new scheme, the regmap-irq client registering with the framework
> shall have to define the *_base registers (e.g. status_base, mask_base,
> type_base, etc.) as those of the very first peripheral in the chip, and
> then specify address offsets of each subsequent peripheral so that their
> corresponding *_base registers may be calculated by the framework. The
> first element of the periph_offs array must be zero so that the first
> peripherals' addresses may be accessed.

> Some MFD chips define two registers in addition to the IRQ type
> registers: POLARITY_HI and POLARITY_LO, so add support to manage their
> data as well as write to them.

It is difficult to follow what this change is supposed to do, in part
because it looks like this is in fact two separate changes, one adding
the _base feature and another adding the polarity feature.  These should
each be in a separate patch if that is the case, and I think each needs
a clearer changelog - I'm not entirely sure what the polarity feature is
supposed to do.  Nothing here says what POLARITY_HI and POLARITY_LO are,
how they interact or anything.

For the address offsets I'm not sure that this is the best way to
represent things.  It looks like the hardware this is trying to describe
is essentially a bunch of separate interrupt controllers that happen to
share an upstream interrupt and I think that the code would be a lot
clearer if at least the implementation looked like this.  Instead of
having to check for this array of offsets at every use point (which is
going to be rarely used and hence prone to bugs) we'd have a set of
separate regmap-irqs and then we'd mostly only have to loop through them
on handling, the bulk of the implementation wouldn't have to worry about
this special case.

Historically genirq didn't support sharing threaded interrupts, if
that's not changed we'd need to open code everything inside regmap-irq
but it would be doable, or ideally genirq could grow this feature.  If
it's done inside regmap you'd have a separate API that took an array of
regmap-irq configurations instead of just one and then when an interrupt
is delivered just loops through all of them handling it.  A quick scan
through the interrupt code suggests it might be able to cope with shared
IRQs now though which would make life easier.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] regmap-irq: Add support for peripheral offsets

[Guru Das Srinagesh]

Hi Mark,

Sorry for the delay in my response.

On Thu, Nov 12, 2020 at 07:33:12PM +0000, Mark Brown wrote:
> It is difficult to follow what this change is supposed to do, in part
> because it looks like this is in fact two separate changes, one adding
> the _base feature and another adding the polarity feature.  These should
> each be in a separate patch if that is the case, and I think each needs
> a clearer changelog - I'm not entirely sure what the polarity feature is
> supposed to do.  Nothing here says what POLARITY_HI and POLARITY_LO are,
> how they interact or anything.

Sure, I can split this into two patches for easier review.

The POLARITY_HI and POLARITY_LO registers were described very briefly in
the cover letter. If an interrupt is already configured as either edge-
or level-triggered, setting the corresponding bit for it in the
POLARITY_HI register further configures it as rising-edge or level-high
triggered (as the case may be), while setting the same bit in
POLARITY_LO further configures it as falling-edge or level-low
triggered. I could certainly add this information to the commit message
as well.

> 
> For the address offsets I'm not sure that this is the best way to
> represent things.  It looks like the hardware this is trying to describe
> is essentially a bunch of separate interrupt controllers that happen to
> share an upstream interrupt

Sorry but isn't this essentially the same as what the framework already knows as
the "sub-irq" concept, with the key difference that the register stride
is not fixed? Everything else is the same (except for a couple of minor
points noted below) - a main IRQ register that indicates sub-irq blocks
that have unhandled interrupts, as well as interrupt handling and
servicing.

The two minor differences are:
  - type_buf handling in regmap_irq_set_type() for IRQ_TYPE_LEVEL_HIGH and
    IRQ_TYPE_LEVEL_LOW
  - Two extra registers: POLARITY_HI and POLARITY_LO

> clearer if at least the implementation looked like this.  Instead of
> having to check for this array of offsets at every use point (which is
> going to be rarely used and hence prone to bugs)

Well, using irq_reg_stride already does exactly this - calculating the
right register to access at every use point, as an offset from the _base
register (status, ack, type, et c.). Peripheral offsets would just be
another way of calculating the right register, that's all. And we could
have a macro as well.

> we'd have a set of separate regmap-irqs and then we'd mostly only have
> to loop through them on handling, the bulk of the implementation
> wouldn't have to worry about this special case.
> 
> Historically genirq didn't support sharing threaded interrupts, if
> that's not changed we'd need to open code everything inside regmap-irq
> but it would be doable, or ideally genirq could grow this feature.  If
> it's done inside regmap you'd have a separate API that took an array of
> regmap-irq configurations instead of just one and then when an interrupt
> is delivered just loops through all of them handling it.  A quick scan
> through the interrupt code suggests it might be able to cope with shared
> IRQs now though which would make life easier.

Sure, I can look into how this approach would look like, but given that
the QCOM register arrangement of main vs sub-irq is essentially the same
as what the framework currently understands, couldn't we simply have a
macro to change the way the right register offset is calculated
(irq_reg_stride vs. peripheral offsets)?

Also, could you elaborate more on the genirq route? I'm not sure where
to start looking to evaluate one route vs the other.

Thank you.

Guru Das.

Re: [PATCH v2 1/3] regmap-irq: Add support for peripheral offsets

[Mark Brown]

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

On Thu, Mar 04, 2021 at 10:27:35AM -0800, Guru Das Srinagesh wrote:
> On Thu, Nov 12, 2020 at 07:33:12PM +0000, Mark Brown wrote:

> > supposed to do.  Nothing here says what POLARITY_HI and POLARITY_LO are,
> > how they interact or anything.

> The POLARITY_HI and POLARITY_LO registers were described very briefly in
> the cover letter. If an interrupt is already configured as either edge-
> or level-triggered, setting the corresponding bit for it in the
> POLARITY_HI register further configures it as rising-edge or level-high
> triggered (as the case may be), while setting the same bit in
> POLARITY_LO further configures it as falling-edge or level-low
> triggered. I could certainly add this information to the commit message
> as well.

So this is just a trigger type control that's in two discontiguous bits
possibly in different registers or something?  This doesn't sound like
anything generic with the API you're describing, if that's what it is
the interface should also handle things like four bits (one for each
type) or having the different values mapped differently within the two
bits that are spread out (eg, you could have one bit for polarity and
another for edge/level).

> > For the address offsets I'm not sure that this is the best way to
> > represent things.  It looks like the hardware this is trying to describe
> > is essentially a bunch of separate interrupt controllers that happen to
> > share an upstream interrupt

> Sorry but isn't this essentially the same as what the framework already knows as
> the "sub-irq" concept, with the key difference that the register stride
> is not fixed? Everything else is the same (except for a couple of minor
> points noted below) - a main IRQ register that indicates sub-irq blocks
> that have unhandled interrupts, as well as interrupt handling and
> servicing.

Like I said in my original review it is extremely hard to tell from your
patch what you are trying to implement, and it's now been more than four
months since you sent it which isn't helping anything.  This means it is
also extremely hard to tell if the patch is doing the same thing as
sub_irq.

IIRC it appeared that there was no top level interrupt status register,
the point with sub_irq is that we don't need to read all the status
registers because there's a top level status register which says which
of them have signals in them (including the possibility that more than
one bit in the top level status might indicate the same leaf status
register).  If the device doesn't have that it doesn't have sub_irqs.
Note that sub_irq only affects status register reads, it doesn't affect
other things like acking or masking.

On the other hand if this *is* the same thing as sub_irqs then why is it
completely separate code and not sharing anything with it?

As I said at the time you need to split this into at least two distinct
patches with clear changelogs which explain what they are trying to
implement, I don't think it's useful to discuss this further without
that.  I can't give you any clearer advice on how to implement whatever
you are trying to implement without having some idea of what that is.

> > clearer if at least the implementation looked like this.  Instead of
> > having to check for this array of offsets at every use point (which is
> > going to be rarely used and hence prone to bugs)

> Well, using irq_reg_stride already does exactly this - calculating the
> right register to access at every use point, as an offset from the _base
> register (status, ack, type, et c.). Peripheral offsets would just be
> another way of calculating the right register, that's all. And we could
> have a macro as well.

The stride code is executed in all paths, it doesn't add conditional
statements all over the place.  This helps a lot, we know it's being run
all the time as opposed to being a lot of separate code paths that are
rarely run - the case without a stride is just a stride of 1.

> Sure, I can look into how this approach would look like, but given that
> the QCOM register arrangement of main vs sub-irq is essentially the same
> as what the framework currently understands, couldn't we simply have a
> macro to change the way the right register offset is calculated
> (irq_reg_stride vs. peripheral offsets)?

I'm not sure macros all over the place is going to be clearer than
conditional statements all over the place.  As with what you were saying
about sub_irq if you think the two things are equivalent then why is one
not implemented in terms of the other rather than doing conditional code
on every single use?

> Also, could you elaborate more on the genirq route? I'm not sure where
> to start looking to evaluate one route vs the other.

Register a separate regmap-irq for each of these perhiperals in your
list, using the same parent interrupt for all of them and setting
IRQF_SHARED.  They will then be handled like any other shared interrupt,
if the parent interrupt fires then genirq will go through and call each
of the handlers until one reports that it handled an interrupt.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]