[PATCH v2 00/12] regmap-irq cleanups and refactoring

[PATCH v2 00/12] regmap-irq cleanups and refactoring

[Aidan MacDonald]

This series is an attempt at cleaning up the regmap-irq API in order
to simplify things and consolidate existing features, while at the
same time generalizing it to support a wider range of hardware.

There is a new system for IRQ type configuration, some tweaks to
unmask registers so they're more intuitive and useful, and a new
callback for calculating register addresses. There's also a few
minor code cleanups in here.

In v2 I've taken the approach of adding new features and deprecating
existing ones rather than removing them aggressively. Warnings will
be issued for any drivers that use deprecated features, but they'll
otherwise continue to function normally.

One important caveat: not all of these changes are tested beyond
compile testing, since I don't have hardware to exercise all of
the features.

Note that this series only applies cleanly on top of two patches from v1,

[01/49] regmap-irq: Fix a bug in regmap_irq_enable() for type_in_mask chips
[02/49] regmap-irq: Fix offset/index mismatch in read_sub_irq_data()

which are already in

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

(NB. I'm not too sure if I should be including them here or not.)

v2 changelog

* Drop driver patches, these will be sent as separate series to the
  appropriate subsystem maintainers.
* Drop patches that remove deprecated features, that should be done
  in a separate series.
* Various fixups to address Andy Shevchenko's v1 review comments.
* Drop patches that changed the behavior of mask_writeonly; instead
  just remove the flag.

Aidan MacDonald (12):
  regmap-irq: Convert bool bitfields to unsigned int
  regmap-irq: Remove unused type_reg_stride field
  regmap-irq: Cleanup sizeof(...) use in memory allocation
  regmap-irq: Remove an unnecessary restriction on type_in_mask
  regmap-irq: Remove inappropriate uses of regmap_irq_update_bits()
  regmap-irq: Remove mask_writeonly and regmap_irq_update_bits()
  regmap-irq: Refactor checks for status bulk read support
  regmap-irq: Introduce config registers for irq types
  regmap-irq: Deprecate type registers and virtual registers
  regmap-irq: Fix inverted handling of unmask registers
  regmap-irq: Add get_irq_reg() callback
  regmap-irq: Deprecate the not_fixed_stride flag

 drivers/base/regmap/regmap-irq.c | 432 +++++++++++++++++++++----------
 include/linux/regmap.h           | 104 +++++---
 2 files changed, 367 insertions(+), 169 deletions(-)

-- 
2.35.1

[PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Aidan MacDonald]

Use 'unsigned int' for bitfields for consistency with most other
kernel code.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c |  2 +-
 include/linux/regmap.h           | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index a6db605707b0..a58b29e9c7c7 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -43,7 +43,7 @@ struct regmap_irq_chip_data {
 	unsigned int irq_reg_stride;
 	unsigned int type_reg_stride;
 
-	bool clear_status:1;
+	unsigned int clear_status:1;
 };
 
 static int sub_irq_reg(struct regmap_irq_chip_data *data,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 8952fa3d0d59..7c5e4a20e9cf 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1518,19 +1518,19 @@ struct regmap_irq_chip {
 	unsigned int type_base;
 	unsigned int *virt_reg_base;
 	unsigned int irq_reg_stride;
-	bool mask_writeonly:1;
-	bool init_ack_masked:1;
-	bool mask_invert:1;
-	bool use_ack:1;
-	bool ack_invert:1;
-	bool clear_ack:1;
-	bool wake_invert:1;
-	bool runtime_pm:1;
-	bool type_invert:1;
-	bool type_in_mask:1;
-	bool clear_on_unmask:1;
-	bool not_fixed_stride:1;
-	bool status_invert:1;
+	unsigned int mask_writeonly:1;
+	unsigned int init_ack_masked:1;
+	unsigned int mask_invert:1;
+	unsigned int use_ack:1;
+	unsigned int ack_invert:1;
+	unsigned int clear_ack:1;
+	unsigned int wake_invert:1;
+	unsigned int runtime_pm:1;
+	unsigned int type_invert:1;
+	unsigned int type_in_mask:1;
+	unsigned int clear_on_unmask:1;
+	unsigned int not_fixed_stride:1;
+	unsigned int status_invert:1;
 
 	int num_regs;
 
-- 
2.35.1

[PATCH v2 02/12] regmap-irq: Remove unused type_reg_stride field

[Aidan MacDonald]

It appears that no chip ever required a nonzero type_reg_stride
and commit 1066cfbdfa3f ("regmap-irq: Extend sub-irq to support
non-fixed reg strides") broke support. Just remove the field.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 6 ------
 include/linux/regmap.h           | 3 ---
 2 files changed, 9 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index a58b29e9c7c7..475a959e2b8b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -41,7 +41,6 @@ struct regmap_irq_chip_data {
 	unsigned int **virt_buf;
 
 	unsigned int irq_reg_stride;
-	unsigned int type_reg_stride;
 
 	unsigned int clear_status:1;
 };
@@ -743,11 +742,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	else
 		d->irq_reg_stride = 1;
 
-	if (chip->type_reg_stride)
-		d->type_reg_stride = chip->type_reg_stride;
-	else
-		d->type_reg_stride = 1;
-
 	if (!map->use_single_read && map->reg_stride == 1 &&
 	    d->irq_reg_stride == 1) {
 		d->status_reg_buf = kmalloc_array(chip->num_regs,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 7c5e4a20e9cf..f75911239977 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1487,8 +1487,6 @@ struct regmap_irq_sub_irq_map {
  * @num_type_reg:    Number of type registers.
  * @num_virt_regs:   Number of non-standard irq configuration registers.
  *		     If zero unsupported.
- * @type_reg_stride: Stride to use for chips where type registers are not
- *			contiguous.
  * @handle_pre_irq:  Driver specific callback to handle interrupt from device
  *		     before regmap_irq_handler process the interrupts.
  * @handle_post_irq: Driver specific callback to handle interrupt from device
@@ -1539,7 +1537,6 @@ struct regmap_irq_chip {
 
 	int num_type_reg;
 	int num_virt_regs;
-	unsigned int type_reg_stride;
 
 	int (*handle_pre_irq)(void *irq_drv_data);
 	int (*handle_post_irq)(void *irq_drv_data);
-- 
2.35.1

[PATCH v2 03/12] regmap-irq: Cleanup sizeof(...) use in memory allocation

[Aidan MacDonald]

Instead of mentioning unsigned int directly, use a sizeof(...)
involving the buffer we're allocating to ensure the types don't
get out of sync.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 475a959e2b8b..dca27b4e29d3 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -670,30 +670,30 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 
 	if (chip->num_main_regs) {
 		d->main_status_buf = kcalloc(chip->num_main_regs,
-					     sizeof(unsigned int),
+					     sizeof(*d->main_status_buf),
 					     GFP_KERNEL);
 
 		if (!d->main_status_buf)
 			goto err_alloc;
 	}
 
-	d->status_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
+	d->status_buf = kcalloc(chip->num_regs, sizeof(*d->status_buf),
 				GFP_KERNEL);
 	if (!d->status_buf)
 		goto err_alloc;
 
-	d->mask_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
+	d->mask_buf = kcalloc(chip->num_regs, sizeof(*d->mask_buf),
 			      GFP_KERNEL);
 	if (!d->mask_buf)
 		goto err_alloc;
 
-	d->mask_buf_def = kcalloc(chip->num_regs, sizeof(unsigned int),
+	d->mask_buf_def = kcalloc(chip->num_regs, sizeof(*d->mask_buf_def),
 				  GFP_KERNEL);
 	if (!d->mask_buf_def)
 		goto err_alloc;
 
 	if (chip->wake_base) {
-		d->wake_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
+		d->wake_buf = kcalloc(chip->num_regs, sizeof(*d->wake_buf),
 				      GFP_KERNEL);
 		if (!d->wake_buf)
 			goto err_alloc;
@@ -702,11 +702,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg;
 	if (num_type_reg) {
 		d->type_buf_def = kcalloc(num_type_reg,
-					  sizeof(unsigned int), GFP_KERNEL);
+					  sizeof(*d->type_buf_def), GFP_KERNEL);
 		if (!d->type_buf_def)
 			goto err_alloc;
 
-		d->type_buf = kcalloc(num_type_reg, sizeof(unsigned int),
+		d->type_buf = kcalloc(num_type_reg, sizeof(*d->type_buf),
 				      GFP_KERNEL);
 		if (!d->type_buf)
 			goto err_alloc;
@@ -723,7 +723,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 
 		for (i = 0; i < chip->num_virt_regs; i++) {
 			d->virt_buf[i] = kcalloc(chip->num_regs,
-						 sizeof(unsigned int),
+						 sizeof(**d->virt_buf),
 						 GFP_KERNEL);
 			if (!d->virt_buf[i])
 				goto err_alloc;
-- 
2.35.1

[PATCH v2 04/12] regmap-irq: Remove an unnecessary restriction on type_in_mask

[Aidan MacDonald]

Check types_supported instead of checking type_rising/falling_val
when using type_in_mask interrupts. This makes the intent clearer
and allows a type_in_mask irq to support level or edge triggers,
rather than only edge triggers.

Update the documentation and comments to reflect the new behavior.

This shouldn't affect existing drivers, because if they didn't
set types_supported properly the type buffer wouldn't be updated.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 19 ++++++++-----------
 include/linux/regmap.h           |  8 +++++---
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index dca27b4e29d3..fd7c4315d16b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -252,22 +252,19 @@ static void regmap_irq_enable(struct irq_data *data)
 	struct regmap *map = d->map;
 	const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
 	unsigned int reg = irq_data->reg_offset / map->reg_stride;
-	unsigned int mask, type;
-
-	type = irq_data->type.type_falling_val | irq_data->type.type_rising_val;
+	unsigned int mask;
 
 	/*
 	 * The type_in_mask flag means that the underlying hardware uses
-	 * separate mask bits for rising and falling edge interrupts, but
-	 * we want to make them into a single virtual interrupt with
-	 * configurable edge.
+	 * separate mask bits for each interrupt trigger type, but we want
+	 * to have a single logical interrupt with a configurable type.
 	 *
-	 * If the interrupt we're enabling defines the falling or rising
-	 * masks then instead of using the regular mask bits for this
-	 * interrupt, use the value previously written to the type buffer
-	 * at the corresponding offset in regmap_irq_set_type().
+	 * If the interrupt we're enabling defines any supported types
+	 * then instead of using the regular mask bits for this interrupt,
+	 * use the value previously written to the type buffer at the
+	 * corresponding offset in regmap_irq_set_type().
 	 */
-	if (d->chip->type_in_mask && type)
+	if (d->chip->type_in_mask && irq_data->type.types_supported)
 		mask = d->type_buf[reg] & irq_data->mask;
 	else
 		mask = irq_data->mask;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f75911239977..106ca1172d3d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1468,9 +1468,11 @@ struct regmap_irq_sub_irq_map {
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_invert: Invert the type flags.
- * @type_in_mask: Use the mask registers for controlling irq type. For
- *                interrupts defining type_rising/falling_mask use mask_base
- *                for edge configuration and never update bits in type_base.
+ * @type_in_mask: Use the mask registers for controlling irq type. Use this if
+ *		  the hardware provides separate bits for rising/falling edge
+ *		  or low/high level interrupts and they should be combined into
+ *		  a single logical interrupt. Use &struct regmap_irq_type data
+ *		  to define the mask bit for each irq type.
  * @clear_on_unmask: For chips with interrupts cleared on read: read the status
  *                   registers before unmasking interrupts to clear any bits
  *                   set when they were masked.
-- 
2.35.1

[PATCH v2 05/12] regmap-irq: Remove inappropriate uses of regmap_irq_update_bits()

[Aidan MacDonald]

regmap_irq_update_bits() is misnamed and should only be used for
updating mask registers, since it checks the mask_writeonly flag.
However, it was also used for updating wake and type registers.

It's safe to replace these uses with regmap_update_bits() because
there are no users of the mask_writeonly flag.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index fd7c4315d16b..cb20ce6f91e7 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -158,11 +158,11 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		reg = sub_irq_reg(d, d->chip->wake_base, i);
 		if (d->wake_buf) {
 			if (d->chip->wake_invert)
-				ret = regmap_irq_update_bits(d, reg,
+				ret = regmap_update_bits(d->map, reg,
 							 d->mask_buf_def[i],
 							 ~d->wake_buf[i]);
 			else
-				ret = regmap_irq_update_bits(d, reg,
+				ret = regmap_update_bits(d->map, reg,
 							 d->mask_buf_def[i],
 							 d->wake_buf[i]);
 			if (ret != 0)
@@ -205,10 +205,10 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 				continue;
 			reg = sub_irq_reg(d, d->chip->type_base, i);
 			if (d->chip->type_invert)
-				ret = regmap_irq_update_bits(d, reg,
+				ret = regmap_update_bits(d->map, reg,
 					d->type_buf_def[i], ~d->type_buf[i]);
 			else
-				ret = regmap_irq_update_bits(d, reg,
+				ret = regmap_update_bits(d->map, reg,
 					d->type_buf_def[i], d->type_buf[i]);
 			if (ret != 0)
 				dev_err(d->map->dev, "Failed to sync type in %x\n",
@@ -825,11 +825,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			reg = sub_irq_reg(d, d->chip->wake_base, i);
 
 			if (chip->wake_invert)
-				ret = regmap_irq_update_bits(d, reg,
+				ret = regmap_update_bits(d->map, reg,
 							 d->mask_buf_def[i],
 							 0);
 			else
-				ret = regmap_irq_update_bits(d, reg,
+				ret = regmap_update_bits(d->map, reg,
 							 d->mask_buf_def[i],
 							 d->wake_buf[i]);
 			if (ret != 0) {
-- 
2.35.1

[PATCH v2 06/12] regmap-irq: Remove mask_writeonly and regmap_irq_update_bits()

[Aidan MacDonald]

Commit a71411dbf6c8 ("regmap: irq: add chip option mask_writeonly")
introduced the mask_writeonly option, but it isn't used now and it
appears it's never been used by any in-tree drivers. The motivation
for the option is mentioned in the commit message,

    Some irq controllers have writeonly/multipurpose register
    layouts. In those cases we read invalid data back. [...]

The option causes mask register updates to use regmap_write_bits()
instead of regmap_update_bits().

However, regmap_write_bits() doesn't solve the reading invalid data
problem. It's still a read-modify-write op like regmap_update_bits().
The difference is that 'update bits' will only write the new value
if it is different from the current value, while 'write bits' will
write the new value unconditionally, even if it's the same as the
current value.

This seems like a bit of a specialized use case and probably isn't
that useful for regmap-irq, so let's just remove the option and go
back to using an 'update bits' op for the mask registers. We can
always add the option back if some driver ends up needing it in the
future.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 24 +++++++-----------------
 include/linux/regmap.h           |  2 --
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index cb20ce6f91e7..7e93dd8af56b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -80,16 +80,6 @@ static void regmap_irq_lock(struct irq_data *data)
 	mutex_lock(&d->lock);
 }
 
-static int regmap_irq_update_bits(struct regmap_irq_chip_data *d,
-				  unsigned int reg, unsigned int mask,
-				  unsigned int val)
-{
-	if (d->chip->mask_writeonly)
-		return regmap_write_bits(d->map, reg, mask, val);
-	else
-		return regmap_update_bits(d->map, reg, mask, val);
-}
-
 static void regmap_irq_sync_unlock(struct irq_data *data)
 {
 	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
@@ -130,11 +120,11 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 
 		reg = sub_irq_reg(d, d->chip->mask_base, i);
 		if (d->chip->mask_invert) {
-			ret = regmap_irq_update_bits(d, reg,
+			ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf_def[i], ~d->mask_buf[i]);
 		} else if (d->chip->unmask_base) {
 			/* set mask with mask_base register */
-			ret = regmap_irq_update_bits(d, reg,
+			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], ~d->mask_buf[i]);
 			if (ret < 0)
 				dev_err(d->map->dev,
@@ -143,12 +133,12 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 			unmask_offset = d->chip->unmask_base -
 							d->chip->mask_base;
 			/* clear mask with unmask_base register */
-			ret = regmap_irq_update_bits(d,
+			ret = regmap_update_bits(d->map,
 					reg + unmask_offset,
 					d->mask_buf_def[i],
 					d->mask_buf[i]);
 		} else {
-			ret = regmap_irq_update_bits(d, reg,
+			ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf_def[i], d->mask_buf[i]);
 		}
 		if (ret != 0)
@@ -763,17 +753,17 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		reg = sub_irq_reg(d, d->chip->mask_base, i);
 
 		if (chip->mask_invert)
-			ret = regmap_irq_update_bits(d, reg,
+			ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf[i], ~d->mask_buf[i]);
 		else if (d->chip->unmask_base) {
 			unmask_offset = d->chip->unmask_base -
 					d->chip->mask_base;
-			ret = regmap_irq_update_bits(d,
+			ret = regmap_update_bits(d->map,
 					reg + unmask_offset,
 					d->mask_buf[i],
 					d->mask_buf[i]);
 		} else
-			ret = regmap_irq_update_bits(d, reg,
+			ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf[i], d->mask_buf[i]);
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 106ca1172d3d..d21eb8ad2675 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1452,7 +1452,6 @@ struct regmap_irq_sub_irq_map {
  *
  * @status_base: Base status register address.
  * @mask_base:   Base mask register address.
- * @mask_writeonly: Base mask register is write only.
  * @unmask_base:  Base unmask register address. for chips who have
  *                separate mask and unmask registers
  * @ack_base:    Base ack address. If zero then the chip is clear on read.
@@ -1518,7 +1517,6 @@ struct regmap_irq_chip {
 	unsigned int type_base;
 	unsigned int *virt_reg_base;
 	unsigned int irq_reg_stride;
-	unsigned int mask_writeonly:1;
 	unsigned int init_ack_masked:1;
 	unsigned int mask_invert:1;
 	unsigned int use_ack:1;
-- 
2.35.1

[PATCH v2 07/12] regmap-irq: Refactor checks for status bulk read support

[Aidan MacDonald]

There are several conditions that must be satisfied to support
bulk read of status registers. Move the check into a function
to avoid duplicating it in two places.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 7e93dd8af56b..5f9a5856c45e 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -73,6 +73,14 @@ struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data,
 	return &data->chip->irqs[irq];
 }
 
+static bool regmap_irq_can_bulk_read_status(struct regmap_irq_chip_data *data)
+{
+	struct regmap *map = data->map;
+
+	return data->irq_reg_stride == 1 && map->reg_stride == 1 &&
+	       !map->use_single_read;
+}
+
 static void regmap_irq_lock(struct irq_data *data)
 {
 	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
@@ -467,8 +475,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 			}
 
 		}
-	} else if (!map->use_single_read && map->reg_stride == 1 &&
-		   data->irq_reg_stride == 1) {
+	} else if (regmap_irq_can_bulk_read_status(data)) {
 
 		u8 *buf8 = data->status_reg_buf;
 		u16 *buf16 = data->status_reg_buf;
@@ -729,8 +736,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	else
 		d->irq_reg_stride = 1;
 
-	if (!map->use_single_read && map->reg_stride == 1 &&
-	    d->irq_reg_stride == 1) {
+	if (regmap_irq_can_bulk_read_status(d)) {
 		d->status_reg_buf = kmalloc_array(chip->num_regs,
 						  map->format.val_bytes,
 						  GFP_KERNEL);
-- 
2.35.1

[PATCH v2 08/12] regmap-irq: Introduce config registers for irq types

[Aidan MacDonald]

Config registers provide a more uniform approach to handling irq type
registers. They are essentially an extension of the virtual registers
used by the qcom-pm8008 driver.

Config registers can be represented as a 2D array:

    config_base[0]      reg0,0      reg0,1      reg0,2      reg0,3
    config_base[1]      reg1,0      reg1,1      reg1,2      reg1,3
    config_base[2]      reg2,0      reg2,1      reg2,2      reg2,3

There are 'num_config_bases' base registers, each of which is used to
address 'num_config_regs' registers. The addresses are calculated in
the same way as for other bases. It is assumed that an irq's type is
controlled by one column of registers; that column is identified by
the irq's 'type_reg_offset'.

The set_type_config() callback is responsible for updating the config
register contents. It receives an array of buffers (each represents a
row of registers) and the index of the column to update, along with
the 'struct regmap_irq' description and requested irq type.

Buffered values are written to registers in regmap_irq_sync_unlock().
Note that the entire register contents are overwritten, which is a
minor change in behavior from type registers via 'type_base'.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 115 +++++++++++++++++++++++++++++--
 include/linux/regmap.h           |  12 ++++
 2 files changed, 122 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 5f9a5856c45e..e3dbf55a561f 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -39,6 +39,7 @@ struct regmap_irq_chip_data {
 	unsigned int *type_buf;
 	unsigned int *type_buf_def;
 	unsigned int **virt_buf;
+	unsigned int **config_buf;
 
 	unsigned int irq_reg_stride;
 
@@ -228,6 +229,17 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		}
 	}
 
+	for (i = 0; i < d->chip->num_config_bases; i++) {
+		for (j = 0; j < d->chip->num_config_regs; j++) {
+			reg = sub_irq_reg(d, d->chip->config_base[i], j);
+			ret = regmap_write(map, reg, d->config_buf[i][j]);
+			if (ret)
+				dev_err(d->map->dev,
+					"Failed to write config %x: %d\n",
+					reg, ret);
+		}
+	}
+
 	if (d->chip->runtime_pm)
 		pm_runtime_put(map->dev);
 
@@ -287,7 +299,7 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
 	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
 	struct regmap *map = d->map;
 	const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
-	int reg;
+	int reg, ret;
 	const struct regmap_irq_type *t = &irq_data->type;
 
 	if ((t->types_supported & type) != type)
@@ -327,9 +339,19 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
 		return -EINVAL;
 	}
 
-	if (d->chip->set_type_virt)
-		return d->chip->set_type_virt(d->virt_buf, type, data->hwirq,
-					      reg);
+	if (d->chip->set_type_virt) {
+		ret = d->chip->set_type_virt(d->virt_buf, type, data->hwirq,
+					     reg);
+		if (ret)
+			return ret;
+	}
+
+	if (d->chip->set_type_config) {
+		ret = d->chip->set_type_config(d->config_buf, type,
+					       irq_data, reg);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -599,6 +621,61 @@ static const struct irq_domain_ops regmap_domain_ops = {
 	.xlate	= irq_domain_xlate_onetwocell,
 };
 
+/**
+ * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback.
+ * @buf: Buffer containing configuration register values, this is a 2D array of
+ *       `num_config_bases` rows, each of `num_config_regs` elements.
+ * @type: The requested IRQ type.
+ * @irq_data: The IRQ being configured.
+ * @idx: Index of the irq's config registers within each array `buf[i]`
+ *
+ * This is a &struct regmap_irq_chip->set_type_config callback suitable for
+ * chips with one config register. Register values are updated according to
+ * the &struct regmap_irq_type data associated with an IRQ.
+ */
+int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
+				      const struct regmap_irq *irq_data, int idx)
+{
+	const struct regmap_irq_type *t = &irq_data->type;
+
+	if (t->type_reg_mask)
+		buf[0][idx] &= ~t->type_reg_mask;
+	else
+		buf[0][idx] &= ~(t->type_falling_val |
+				 t->type_rising_val |
+				 t->type_level_low_val |
+				 t->type_level_high_val);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_FALLING:
+		buf[0][idx] |= t->type_falling_val;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		buf[0][idx] |= t->type_rising_val;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		buf[0][idx] |= (t->type_falling_val |
+				t->type_rising_val);
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		buf[0][idx] |= t->type_level_high_val;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		buf[0][idx] |= t->type_level_low_val;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(regmap_irq_set_type_config_simple);
+
 /**
  * regmap_add_irq_chip_fwnode() - Use standard regmap IRQ controller handling
  *
@@ -724,6 +801,24 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 	}
 
+	if (chip->num_config_bases && chip->num_config_regs) {
+		/*
+		 * Create config_buf[num_config_bases][num_config_regs]
+		 */
+		d->config_buf = kcalloc(chip->num_config_bases,
+					sizeof(*d->config_buf), GFP_KERNEL);
+		if (!d->config_buf)
+			goto err_alloc;
+
+		for (i = 0; i < chip->num_config_regs; i++) {
+			d->config_buf[i] = kcalloc(chip->num_config_regs,
+						   sizeof(**d->config_buf),
+						   GFP_KERNEL);
+			if (!d->config_buf[i])
+				goto err_alloc;
+		}
+	}
+
 	d->irq_chip = regmap_irq_chip;
 	d->irq_chip.name = chip->name;
 	d->irq = irq;
@@ -894,6 +989,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			kfree(d->virt_buf[i]);
 		kfree(d->virt_buf);
 	}
+	if (d->config_buf) {
+		for (i = 0; i < chip->num_config_bases; i++)
+			kfree(d->config_buf[i]);
+		kfree(d->config_buf);
+	}
 	kfree(d);
 	return ret;
 }
@@ -934,7 +1034,7 @@ EXPORT_SYMBOL_GPL(regmap_add_irq_chip);
 void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 {
 	unsigned int virq;
-	int hwirq;
+	int i, hwirq;
 
 	if (!d)
 		return;
@@ -964,6 +1064,11 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 	kfree(d->mask_buf);
 	kfree(d->status_reg_buf);
 	kfree(d->status_buf);
+	if (d->config_buf) {
+		for (i = 0; i < d->chip->num_config_bases; i++)
+			kfree(d->config_buf[i]);
+		kfree(d->config_buf);
+	}
 	kfree(d);
 }
 EXPORT_SYMBOL_GPL(regmap_del_irq_chip);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index d21eb8ad2675..432449f318cb 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1459,6 +1459,7 @@ struct regmap_irq_sub_irq_map {
  * @wake_base:   Base address for wake enables.  If zero unsupported.
  * @type_base:   Base address for irq type.  If zero unsupported.
  * @virt_reg_base:   Base addresses for extra config regs.
+ * @config_base: Base address for IRQ type config regs. If null unsupported.
  * @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.
@@ -1488,12 +1489,15 @@ struct regmap_irq_sub_irq_map {
  * @num_type_reg:    Number of type registers.
  * @num_virt_regs:   Number of non-standard irq configuration registers.
  *		     If zero unsupported.
+ * @num_config_bases:	Number of config base registers.
+ * @num_config_regs:	Number of config registers for each config base register.
  * @handle_pre_irq:  Driver specific callback to handle interrupt from device
  *		     before regmap_irq_handler process the interrupts.
  * @handle_post_irq: Driver specific callback to handle interrupt from device
  *		     after handling the interrupts in regmap_irq_handler().
  * @set_type_virt:   Driver specific callback to extend regmap_irq_set_type()
  *		     and configure virt regs.
+ * @set_type_config: Callback used for configuring irq types.
  * @irq_drv_data:    Driver specific IRQ data which is passed as parameter when
  *		     driver specific pre/post interrupt handler is called.
  *
@@ -1516,6 +1520,7 @@ struct regmap_irq_chip {
 	unsigned int wake_base;
 	unsigned int type_base;
 	unsigned int *virt_reg_base;
+	const unsigned int *config_base;
 	unsigned int irq_reg_stride;
 	unsigned int init_ack_masked:1;
 	unsigned int mask_invert:1;
@@ -1537,16 +1542,23 @@ struct regmap_irq_chip {
 
 	int num_type_reg;
 	int num_virt_regs;
+	int num_config_bases;
+	int num_config_regs;
 
 	int (*handle_pre_irq)(void *irq_drv_data);
 	int (*handle_post_irq)(void *irq_drv_data);
 	int (*set_type_virt)(unsigned int **buf, unsigned int type,
 			     unsigned long hwirq, int reg);
+	int (*set_type_config)(unsigned int **buf, unsigned int type,
+			       const struct regmap_irq *irq_data, int idx);
 	void *irq_drv_data;
 };
 
 struct regmap_irq_chip_data;
 
+int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
+				      const struct regmap_irq *irq_data, int idx);
+
 int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 			int irq_base, const struct regmap_irq_chip *chip,
 			struct regmap_irq_chip_data **data);
-- 
2.35.1

[PATCH v2 09/12] regmap-irq: Deprecate type registers and virtual registers

[Aidan MacDonald]

Config registers can be used to replace both type and virtual
registers, so mark both features are deprecated and issue a
warning if they're used.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c |  6 ++++++
 include/linux/regmap.h           | 18 ++++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index e3dbf55a561f..8cbc62c3d638 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -726,6 +726,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 				return -EINVAL;
 	}
 
+	if (chip->num_type_reg)
+		dev_warn(map->dev, "type registers are deprecated; use config registers instead");
+
+	if (chip->num_virt_regs || chip->virt_reg_base || chip->set_type_virt)
+		dev_warn(map->dev, "virtual registers are deprecated; use config registers instead");
+
 	if (irq_base) {
 		irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0);
 		if (irq_base < 0) {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 432449f318cb..2b5b07f85cc0 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1457,8 +1457,10 @@ struct regmap_irq_sub_irq_map {
  * @ack_base:    Base ack address. If zero then the chip is clear on read.
  *               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.
- * @virt_reg_base:   Base addresses for extra config regs.
+ * @type_base:   Base address for irq type.  If zero unsupported.  Deprecated,
+ *		 use @config_base instead.
+ * @virt_reg_base:   Base addresses for extra config regs. Deprecated, use
+ *		     @config_base instead.
  * @config_base: Base address for IRQ type config regs. If null unsupported.
  * @irq_reg_stride:  Stride to use for chips where registers are not contiguous.
  * @init_ack_masked: Ack all masked interrupts once during initalization.
@@ -1467,7 +1469,8 @@ struct regmap_irq_sub_irq_map {
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
- * @type_invert: Invert the type flags.
+ * @type_invert: Invert the type flags. Deprecated, use config registers
+ *		 instead.
  * @type_in_mask: Use the mask registers for controlling irq type. Use this if
  *		  the hardware provides separate bits for rising/falling edge
  *		  or low/high level interrupts and they should be combined into
@@ -1486,9 +1489,11 @@ struct regmap_irq_sub_irq_map {
  * @irqs:        Descriptors for individual IRQs.  Interrupt numbers are
  *               assigned based on the index in the array of the interrupt.
  * @num_irqs:    Number of descriptors.
- * @num_type_reg:    Number of type registers.
+ * @num_type_reg:    Number of type registers. Deprecated, use config registers
+ *		     instead.
  * @num_virt_regs:   Number of non-standard irq configuration registers.
- *		     If zero unsupported.
+ *		     If zero unsupported. Deprecated, use config registers
+ *		     instead.
  * @num_config_bases:	Number of config base registers.
  * @num_config_regs:	Number of config registers for each config base register.
  * @handle_pre_irq:  Driver specific callback to handle interrupt from device
@@ -1496,7 +1501,8 @@ struct regmap_irq_sub_irq_map {
  * @handle_post_irq: Driver specific callback to handle interrupt from device
  *		     after handling the interrupts in regmap_irq_handler().
  * @set_type_virt:   Driver specific callback to extend regmap_irq_set_type()
- *		     and configure virt regs.
+ *		     and configure virt regs. Deprecated, use @set_type_config
+ *		     callback and config registers instead.
  * @set_type_config: Callback used for configuring irq types.
  * @irq_drv_data:    Driver specific IRQ data which is passed as parameter when
  *		     driver specific pre/post interrupt handler is called.
-- 
2.35.1

[PATCH v2 10/12] regmap-irq: Fix inverted handling of unmask registers

[Aidan MacDonald]

To me "unmask" suggests that we write 1s to the register when
an interrupt is enabled. This also makes sense because it's the
opposite of what the "mask" register does (write 1s to disable
an interrupt).

But regmap-irq does the opposite: for a disabled interrupt, it
writes 1s to "unmask" and 0s to "mask". This is surprising and
deviates from the usual way mask registers are handled.

Additionally, mask_invert didn't interact with unmask registers
properly -- it caused them to be ignored entirely.

Fix this by making mask and unmask registers orthogonal, using
the following behavior:

* Mask registers are written with 1s for disabled interrupts.
* Unmask registers are written with 1s for enabled interrupts.

This behavior supports both normal or inverted mask registers
and separate set/clear registers via different combinations of
mask_base/unmask_base.

The old unmask register behavior is deprecated. Drivers need to
opt-in to the new behavior by setting mask_unmask_non_inverted.
Warnings are issued if the driver relies on deprecated behavior.
Chips that only set one of mask_base/unmask_base don't have to
use the mask_unmask_non_inverted flag because that use case was
previously not supported.

The mask_invert flag is also deprecated in favor of describing
inverted mask registers as unmask registers.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 114 +++++++++++++++++++------------
 include/linux/regmap.h           |  18 ++++-
 2 files changed, 84 insertions(+), 48 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 8cbc62c3d638..2c724ae185c4 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -30,6 +30,9 @@ struct regmap_irq_chip_data {
 	int irq;
 	int wake_count;
 
+	unsigned int mask_base;
+	unsigned int unmask_base;
+
 	void *status_reg_buf;
 	unsigned int *main_status_buf;
 	unsigned int *status_buf;
@@ -95,7 +98,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	struct regmap *map = d->map;
 	int i, j, ret;
 	u32 reg;
-	u32 unmask_offset;
 	u32 val;
 
 	if (d->chip->runtime_pm) {
@@ -124,35 +126,23 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	 * suppress pointless writes.
 	 */
 	for (i = 0; i < d->chip->num_regs; i++) {
-		if (!d->chip->mask_base)
-			continue;
-
-		reg = sub_irq_reg(d, d->chip->mask_base, i);
-		if (d->chip->mask_invert) {
+		if (d->mask_base) {
+			reg = sub_irq_reg(d, d->mask_base, i);
 			ret = regmap_update_bits(d->map, reg,
-					 d->mask_buf_def[i], ~d->mask_buf[i]);
-		} else if (d->chip->unmask_base) {
-			/* set mask with mask_base register */
+					d->mask_buf_def[i], d->mask_buf[i]);
+			if (ret)
+				dev_err(d->map->dev, "Failed to sync masks in %x\n",
+					reg);
+		}
+
+		if (d->unmask_base) {
+			reg = sub_irq_reg(d, d->unmask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], ~d->mask_buf[i]);
-			if (ret < 0)
-				dev_err(d->map->dev,
-					"Failed to sync unmasks in %x\n",
+			if (ret)
+				dev_err(d->map->dev, "Failed to sync masks in %x\n",
 					reg);
-			unmask_offset = d->chip->unmask_base -
-							d->chip->mask_base;
-			/* clear mask with unmask_base register */
-			ret = regmap_update_bits(d->map,
-					reg + unmask_offset,
-					d->mask_buf_def[i],
-					d->mask_buf[i]);
-		} else {
-			ret = regmap_update_bits(d->map, reg,
-					 d->mask_buf_def[i], d->mask_buf[i]);
 		}
-		if (ret != 0)
-			dev_err(d->map->dev, "Failed to sync masks in %x\n",
-				reg);
 
 		reg = sub_irq_reg(d, d->chip->wake_base, i);
 		if (d->wake_buf) {
@@ -704,7 +694,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	int ret = -ENOMEM;
 	int num_type_reg;
 	u32 reg;
-	u32 unmask_offset;
 
 	if (chip->num_regs <= 0)
 		return -EINVAL;
@@ -832,6 +821,42 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	d->chip = chip;
 	d->irq_base = irq_base;
 
+	if (chip->mask_base && chip->unmask_base &&
+	    !chip->mask_unmask_non_inverted) {
+		/*
+		 * Chips that specify both mask_base and unmask_base used to
+		 * get inverted mask behavior by default, with no way to ask
+		 * for the normal, non-inverted behavior. This "inverted by
+		 * default" behavior is deprecated, but we have to support it
+		 * until existing drivers have been fixed.
+		 *
+		 * Existing drivers should be updated by swapping mask_base
+		 * and unmask_base and setting mask_unmask_non_inverted=true.
+		 * New drivers should always set the flag.
+		 */
+		dev_warn(map->dev, "mask_base and unmask_base are inverted, please fix it");
+
+		/* Might as well warn about mask_invert while we're at it... */
+		if (chip->mask_invert)
+			dev_warn(map->dev, "mask_invert=true ignored");
+
+		d->mask_base = chip->unmask_base;
+		d->unmask_base = chip->mask_base;
+	} else if (chip->mask_invert) {
+		/*
+		 * Swap the roles of mask_base and unmask_base if the bits are
+		 * inverted. This is deprecated, drivers should use unmask_base
+		 * directly.
+		 */
+		dev_warn(map->dev, "mask_invert=true is deprecated; please switch to unmask_base");
+
+		d->mask_base = chip->unmask_base;
+		d->unmask_base = chip->mask_base;
+	} else {
+		d->mask_base = chip->mask_base;
+		d->unmask_base = chip->unmask_base;
+	}
+
 	if (chip->irq_reg_stride)
 		d->irq_reg_stride = chip->irq_reg_stride;
 	else
@@ -854,28 +879,27 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	/* Mask all the interrupts by default */
 	for (i = 0; i < chip->num_regs; i++) {
 		d->mask_buf[i] = d->mask_buf_def[i];
-		if (!chip->mask_base)
-			continue;
-
-		reg = sub_irq_reg(d, d->chip->mask_base, i);
 
-		if (chip->mask_invert)
+		if (d->mask_base) {
+			reg = sub_irq_reg(d, d->mask_base, i);
 			ret = regmap_update_bits(d->map, reg,
-					 d->mask_buf[i], ~d->mask_buf[i]);
-		else if (d->chip->unmask_base) {
-			unmask_offset = d->chip->unmask_base -
-					d->chip->mask_base;
-			ret = regmap_update_bits(d->map,
-					reg + unmask_offset,
-					d->mask_buf[i],
-					d->mask_buf[i]);
-		} else
+					d->mask_buf_def[i], d->mask_buf[i]);
+			if (ret) {
+				dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
+					reg, ret);
+				goto err_alloc;
+			}
+		}
+
+		if (d->unmask_base) {
+			reg = sub_irq_reg(d, d->unmask_base, i);
 			ret = regmap_update_bits(d->map, reg,
-					 d->mask_buf[i], d->mask_buf[i]);
-		if (ret != 0) {
-			dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
-				reg, ret);
-			goto err_alloc;
+					d->mask_buf_def[i], ~d->mask_buf[i]);
+			if (ret) {
+				dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
+					reg, ret);
+				goto err_alloc;
+			}
 		}
 
 		if (!chip->init_ack_masked)
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 2b5b07f85cc0..708f36dfaeda 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1451,9 +1451,10 @@ struct regmap_irq_sub_irq_map {
  *		   main_status set.
  *
  * @status_base: Base status register address.
- * @mask_base:   Base mask register address.
- * @unmask_base:  Base unmask register address. for chips who have
- *                separate mask and unmask registers
+ * @mask_base:   Base mask register address. Mask bits are set to 1 when an
+ *               interrupt is masked, 0 when unmasked.
+ * @unmask_base:  Base unmask register address. Unmask bits are set to 1 when
+ *                an interrupt is unmasked and 0 when masked.
  * @ack_base:    Base ack address. If zero then the chip is clear on read.
  *               Using zero value is possible with @use_ack bit.
  * @wake_base:   Base address for wake enables.  If zero unsupported.
@@ -1465,6 +1466,16 @@ struct regmap_irq_sub_irq_map {
  * @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.
+ *		 Deprecated; prefer describing an inverted mask register as
+ *		 an unmask register.
+ * @mask_unmask_non_inverted: Controls mask bit inversion for chips that set
+ *	both @mask_base and @unmask_base. If false, mask and unmask bits are
+ *	inverted (which is deprecated behavior); if true, bits will not be
+ *	inverted and the registers keep their normal behavior. Note that if
+ *	you use only one of @mask_base or @unmask_base, this flag has no
+ *	effect and is unnecessary. Any new drivers that set both @mask_base
+ *	and @unmask_base should set this to true to avoid relying on the
+ *	deprecated behavior.
  * @use_ack:     Use @ack register even if it is zero.
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
@@ -1530,6 +1541,7 @@ struct regmap_irq_chip {
 	unsigned int irq_reg_stride;
 	unsigned int init_ack_masked:1;
 	unsigned int mask_invert:1;
+	unsigned int mask_unmask_non_inverted:1;
 	unsigned int use_ack:1;
 	unsigned int ack_invert:1;
 	unsigned int clear_ack:1;
-- 
2.35.1

[PATCH v2 11/12] regmap-irq: Add get_irq_reg() callback

[Aidan MacDonald]

Replace the internal sub_irq_reg() function with a public callback
that drivers can use when they have more complex register layouts.
The default implementation is regmap_irq_get_irq_reg_linear(), used
if the chip doesn't provide its own callback.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 126 ++++++++++++++++++++-----------
 include/linux/regmap.h           |  15 +++-
 2 files changed, 93 insertions(+), 48 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 2c724ae185c4..71de097847a7 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -46,30 +46,12 @@ struct regmap_irq_chip_data {
 
 	unsigned int irq_reg_stride;
 
+	unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data,
+				    unsigned int base, int index);
+
 	unsigned int clear_status:1;
 };
 
-static int sub_irq_reg(struct regmap_irq_chip_data *data,
-		       unsigned int base_reg, int i)
-{
-	const struct regmap_irq_chip *chip = data->chip;
-	struct regmap *map = data->map;
-	struct regmap_irq_sub_irq_map *subreg;
-	unsigned int offset;
-	int reg = 0;
-
-	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
-		/* Assume linear mapping */
-		reg = base_reg + (i * map->reg_stride * data->irq_reg_stride);
-	} else {
-		subreg = &chip->sub_reg_offsets[i];
-		offset = subreg->offset[0];
-		reg = base_reg + offset;
-	}
-
-	return reg;
-}
-
 static inline const
 struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data,
 				     int irq)
@@ -81,7 +63,13 @@ static bool regmap_irq_can_bulk_read_status(struct regmap_irq_chip_data *data)
 {
 	struct regmap *map = data->map;
 
+	/*
+	 * While possible that a user-defined ->get_irq_reg() callback might
+	 * be linear enough to support bulk reads, most of the time it won't.
+	 * Therefore only allow them if the default callback is being used.
+	 */
 	return data->irq_reg_stride == 1 && map->reg_stride == 1 &&
+	       data->get_irq_reg == regmap_irq_get_irq_reg_linear &&
 	       !map->use_single_read;
 }
 
@@ -109,7 +97,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 
 	if (d->clear_status) {
 		for (i = 0; i < d->chip->num_regs; i++) {
-			reg = sub_irq_reg(d, d->chip->status_base, i);
+			reg = d->get_irq_reg(d, d->chip->status_base, i);
 
 			ret = regmap_read(map, reg, &val);
 			if (ret)
@@ -127,7 +115,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	 */
 	for (i = 0; i < d->chip->num_regs; i++) {
 		if (d->mask_base) {
-			reg = sub_irq_reg(d, d->mask_base, i);
+			reg = d->get_irq_reg(d, d->mask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], d->mask_buf[i]);
 			if (ret)
@@ -136,7 +124,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		}
 
 		if (d->unmask_base) {
-			reg = sub_irq_reg(d, d->unmask_base, i);
+			reg = d->get_irq_reg(d, d->unmask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], ~d->mask_buf[i]);
 			if (ret)
@@ -144,7 +132,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 					reg);
 		}
 
-		reg = sub_irq_reg(d, d->chip->wake_base, i);
+		reg = d->get_irq_reg(d, d->chip->wake_base, i);
 		if (d->wake_buf) {
 			if (d->chip->wake_invert)
 				ret = regmap_update_bits(d->map, reg,
@@ -168,7 +156,7 @@ 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 = sub_irq_reg(d, d->chip->ack_base, i);
+			reg = d->get_irq_reg(d, d->chip->ack_base, i);
 
 			/* some chips ack by write 0 */
 			if (d->chip->ack_invert)
@@ -192,7 +180,7 @@ 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 = sub_irq_reg(d, d->chip->type_base, i);
+			reg = d->get_irq_reg(d, d->chip->type_base, i);
 			if (d->chip->type_invert)
 				ret = regmap_update_bits(d->map, reg,
 					d->type_buf_def[i], ~d->type_buf[i]);
@@ -208,8 +196,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	if (d->chip->num_virt_regs) {
 		for (i = 0; i < d->chip->num_virt_regs; i++) {
 			for (j = 0; j < d->chip->num_regs; j++) {
-				reg = sub_irq_reg(d, d->chip->virt_reg_base[i],
-						  j);
+				reg = d->get_irq_reg(d, d->chip->virt_reg_base[i],
+						     j);
 				ret = regmap_write(map, reg, d->virt_buf[i][j]);
 				if (ret != 0)
 					dev_err(d->map->dev,
@@ -221,7 +209,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 
 	for (i = 0; i < d->chip->num_config_bases; i++) {
 		for (j = 0; j < d->chip->num_config_regs; j++) {
-			reg = sub_irq_reg(d, d->chip->config_base[i], j);
+			reg = d->get_irq_reg(d, d->chip->config_base[i], j);
 			ret = regmap_write(map, reg, d->config_buf[i][j]);
 			if (ret)
 				dev_err(d->map->dev,
@@ -382,14 +370,17 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
 	const struct regmap_irq_chip *chip = data->chip;
 	struct regmap *map = data->map;
 	struct regmap_irq_sub_irq_map *subreg;
+	unsigned int reg;
 	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]);
+		reg = data->get_irq_reg(data, chip->status_base, b);
+		ret = regmap_read(map, reg, &data->status_buf[b]);
 	} else {
+		/*
+		 * Note we can't use ->get_irq_reg() here because the offsets
+		 * in 'subreg' are *not* interchangeable with indices.
+		 */
 		subreg = &chip->sub_reg_offsets[b];
 		for (i = 0; i < subreg->num_regs; i++) {
 			unsigned int offset = subreg->offset[i];
@@ -455,10 +446,18 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		 * sake of simplicity. and add bulk reads only if needed
 		 */
 		for (i = 0; i < chip->num_main_regs; i++) {
-			ret = regmap_read(map, chip->main_status +
-				  (i * map->reg_stride
-				   * data->irq_reg_stride),
-				  &data->main_status_buf[i]);
+			/*
+			 * For not_fixed_stride, don't use ->get_irq_reg().
+			 * It would produce an incorrect result.
+			 */
+			if (data->chip->not_fixed_stride)
+				reg = chip->main_status +
+					i * map->reg_stride * data->irq_reg_stride;
+			else
+				reg = data->get_irq_reg(data,
+							chip->main_status, i);
+
+			ret = regmap_read(map, reg, &data->main_status_buf[i]);
 			if (ret) {
 				dev_err(map->dev,
 					"Failed to read IRQ status %d\n",
@@ -523,7 +522,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 
 	} else {
 		for (i = 0; i < data->chip->num_regs; i++) {
-			unsigned int reg = sub_irq_reg(data,
+			unsigned int reg = data->get_irq_reg(data,
 					data->chip->status_base, i);
 			ret = regmap_read(map, reg, &data->status_buf[i]);
 
@@ -551,7 +550,7 @@ 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 = sub_irq_reg(data, data->chip->ack_base, i);
+			reg = data->get_irq_reg(data, data->chip->ack_base, i);
 
 			if (chip->ack_invert)
 				ret = regmap_write(map, reg,
@@ -611,6 +610,36 @@ static const struct irq_domain_ops regmap_domain_ops = {
 	.xlate	= irq_domain_xlate_onetwocell,
 };
 
+/**
+ * regmap_irq_get_irq_reg_linear() - Linear IRQ register mapping callback.
+ * @data: Data for the &struct regmap_irq_chip
+ * @base: Base register
+ * @index: Register index
+ *
+ * Returns the register address corresponding to the given @base and @index
+ * by the formula ``base + index * regmap_stride * irq_reg_stride``.
+ */
+unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
+					   unsigned int base, int index)
+{
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+
+	/*
+	 * FIXME: This is for backward compatibility and should be removed
+	 * when not_fixed_stride is dropped (it's only used by qcom-pm8008).
+	 */
+	if (chip->not_fixed_stride && chip->sub_reg_offsets) {
+		struct regmap_irq_sub_irq_map *subreg;
+
+		subreg = &chip->sub_reg_offsets[0];
+		return base + subreg->offset[0];
+	}
+
+	return base + index * map->reg_stride * chip->irq_reg_stride;
+}
+EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear);
+
 /**
  * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback.
  * @buf: Buffer containing configuration register values, this is a 2D array of
@@ -862,6 +891,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	else
 		d->irq_reg_stride = 1;
 
+	if (chip->get_irq_reg)
+		d->get_irq_reg = chip->get_irq_reg;
+	else
+		d->get_irq_reg = regmap_irq_get_irq_reg_linear;
+
 	if (regmap_irq_can_bulk_read_status(d)) {
 		d->status_reg_buf = kmalloc_array(chip->num_regs,
 						  map->format.val_bytes,
@@ -881,7 +915,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		d->mask_buf[i] = d->mask_buf_def[i];
 
 		if (d->mask_base) {
-			reg = sub_irq_reg(d, d->mask_base, i);
+			reg = d->get_irq_reg(d, d->mask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], d->mask_buf[i]);
 			if (ret) {
@@ -892,7 +926,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 
 		if (d->unmask_base) {
-			reg = sub_irq_reg(d, d->unmask_base, i);
+			reg = d->get_irq_reg(d, d->unmask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], ~d->mask_buf[i]);
 			if (ret) {
@@ -906,7 +940,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			continue;
 
 		/* Ack masked but set interrupts */
-		reg = sub_irq_reg(d, d->chip->status_base, i);
+		reg = d->get_irq_reg(d, d->chip->status_base, i);
 		ret = regmap_read(map, reg, &d->status_buf[i]);
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
@@ -918,7 +952,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			d->status_buf[i] = ~d->status_buf[i];
 
 		if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
-			reg = sub_irq_reg(d, d->chip->ack_base, i);
+			reg = d->get_irq_reg(d, d->chip->ack_base, i);
 			if (chip->ack_invert)
 				ret = regmap_write(map, reg,
 					~(d->status_buf[i] & d->mask_buf[i]));
@@ -943,7 +977,7 @@ 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 = sub_irq_reg(d, d->chip->wake_base, i);
+			reg = d->get_irq_reg(d, d->chip->wake_base, i);
 
 			if (chip->wake_invert)
 				ret = regmap_update_bits(d->map, reg,
@@ -963,7 +997,7 @@ 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 = sub_irq_reg(d, d->chip->type_base, i);
+			reg = d->get_irq_reg(d, d->chip->type_base, i);
 
 			ret = regmap_read(map, reg, &d->type_buf_def[i]);
 
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 708f36dfaeda..ae5f1f7d4b5a 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1424,6 +1424,8 @@ struct regmap_irq_sub_irq_map {
 	unsigned int *offset;
 };
 
+struct regmap_irq_chip_data;
+
 /**
  * struct regmap_irq_chip - Description of a generic regmap irq_chip.
  *
@@ -1515,6 +1517,13 @@ struct regmap_irq_sub_irq_map {
  *		     and configure virt regs. Deprecated, use @set_type_config
  *		     callback and config registers instead.
  * @set_type_config: Callback used for configuring irq types.
+ * @get_irq_reg: Callback for mapping (base register, index) pairs to register
+ *		 addresses. The base register will be one of @status_base,
+ *		 @mask_base, etc., @main_status, or any of @config_base.
+ *		 The index will be in the range [0, num_main_regs[ for the
+ *		 main status base, [0, num_type_settings[ for any config
+ *		 register base, and [0, num_regs[ for any other base.
+ *		 If unspecified then regmap_irq_get_irq_reg_linear() is used.
  * @irq_drv_data:    Driver specific IRQ data which is passed as parameter when
  *		     driver specific pre/post interrupt handler is called.
  *
@@ -1569,11 +1578,13 @@ struct regmap_irq_chip {
 			     unsigned long hwirq, int reg);
 	int (*set_type_config)(unsigned int **buf, unsigned int type,
 			       const struct regmap_irq *irq_data, int idx);
+	unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data,
+				    unsigned int base, int index);
 	void *irq_drv_data;
 };
 
-struct regmap_irq_chip_data;
-
+unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
+					   unsigned int base, int index);
 int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
 				      const struct regmap_irq *irq_data, int idx);
 
-- 
2.35.1

[PATCH v2 12/12] regmap-irq: Deprecate the not_fixed_stride flag

[Aidan MacDonald]

This flag is a bit of a hack and the same thing can be accomplished
using a custom ->get_irq_reg() callback. Add a warning to catch any
use of the flag.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 2 ++
 include/linux/regmap.h           | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 71de097847a7..a691553a0d6f 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -739,6 +739,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	}
 
 	if (chip->not_fixed_stride) {
+		dev_warn(map->dev, "not_fixed_stride is deprecated; use ->get_irq_reg() instead");
+
 		for (i = 0; i < chip->num_regs; i++)
 			if (chip->sub_reg_offsets[i].num_regs != 1)
 				return -EINVAL;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index ae5f1f7d4b5a..84ab1c32271f 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1493,8 +1493,10 @@ struct regmap_irq_chip_data;
  *                   registers before unmasking interrupts to clear any bits
  *                   set when they were masked.
  * @not_fixed_stride: Used when chip peripherals are not laid out with fixed
- * 		      stride. Must be used with sub_reg_offsets containing the
- * 		      offsets to each peripheral.
+ *		      stride. Must be used with sub_reg_offsets containing the
+ *		      offsets to each peripheral. Deprecated; the same thing
+ *		      can be accomplished with a @get_irq_reg callback, without
+ *		      the need for a @sub_reg_offsets table.
  * @status_invert: Inverted status register: cleared bits are active interrupts.
  * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  *
-- 
2.35.1

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 11:13 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:
>
> Use 'unsigned int' for bitfields for consistency with most other
> kernel code.

...

There is no point to convert the fields you are about to remove.

So, either don't touch them or make this patch closer to the end of the series.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Mark Brown]

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

On Thu, Jun 23, 2022 at 11:26:10PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 11:13 PM Aidan MacDonald

> > Use 'unsigned int' for bitfields for consistency with most other
> > kernel code.

> There is no point to convert the fields you are about to remove.

> So, either don't touch them or make this patch closer to the end of the series.

It costs us nothing to convert them, this isn't a difficult or hard to
understand refactoring - the patch is fine the way it is.

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

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Joe Perches]

On Fri, 2022-06-24 at 13:11 +0100, Mark Brown wrote:
> On Thu, Jun 23, 2022 at 11:26:10PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 11:13 PM Aidan MacDonald
> 
> > > Use 'unsigned int' for bitfields for consistency with most other
> > > kernel code.
> 
> > There is no point to convert the fields you are about to remove.
> 
> > So, either don't touch them or make this patch closer to the end of the series.
> 
> It costs us nothing to convert them, this isn't a difficult or hard to
> understand refactoring - the patch is fine the way it is.

Modulo the defects that might be introduced if an overflow occurs.

struct foo {
	unsigned int a:1;
	bool b:1;
}

Assign a non-zero int without bit 0 set to each and see if
a and b differ.

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Mark Brown]

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

On Fri, Jun 24, 2022 at 05:46:10AM -0700, Joe Perches wrote:
> On Fri, 2022-06-24 at 13:11 +0100, Mark Brown wrote:
> > On Thu, Jun 23, 2022 at 11:26:10PM +0200, Andy Shevchenko wrote:

> > > There is no point to convert the fields you are about to remove.
> > 
> > > So, either don't touch them or make this patch closer to the end of the series.

> > It costs us nothing to convert them, this isn't a difficult or hard to
> > understand refactoring - the patch is fine the way it is.

> Modulo the defects that might be introduced if an overflow occurs.

This won't be an issue if the fields are removed which was what Andy was
complaining about.

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

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Aidan MacDonald]

Joe Perches <joe@perches.com> writes:

> On Fri, 2022-06-24 at 13:11 +0100, Mark Brown wrote:
>> On Thu, Jun 23, 2022 at 11:26:10PM +0200, Andy Shevchenko wrote:
>> > On Thu, Jun 23, 2022 at 11:13 PM Aidan MacDonald
>> 
>> > > Use 'unsigned int' for bitfields for consistency with most other
>> > > kernel code.
>> 
>> > There is no point to convert the fields you are about to remove.
>> 
>> > So, either don't touch them or make this patch closer to the end of the series.
>> 
>> It costs us nothing to convert them, this isn't a difficult or hard to
>> understand refactoring - the patch is fine the way it is.
>
> Modulo the defects that might be introduced if an overflow occurs.
>
> struct foo {
> 	unsigned int a:1;
> 	bool b:1;
> }
>
> Assign a non-zero int without bit 0 set to each and see if
> a and b differ.

Bool permits implicit pointer-to-bool conversions, so it isn't free
of pitfalls either. Overflow is probably more dangerous in general,
but here there's little chance of pointers or overflow getting involved.
These are const flags that describe properties of the hardware, nothing
should change them once the irq chip is created, and the vast majority
of chips are static const.

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Joe Perches]

On Fri, 2022-06-24 at 14:05 +0100, Aidan MacDonald wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Fri, 2022-06-24 at 13:11 +0100, Mark Brown wrote:
> > > On Thu, Jun 23, 2022 at 11:26:10PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jun 23, 2022 at 11:13 PM Aidan MacDonald
> > > 
> > > > > Use 'unsigned int' for bitfields for consistency with most other
> > > > > kernel code.
> > > 
> > > > There is no point to convert the fields you are about to remove.
> > > 
> > > > So, either don't touch them or make this patch closer to the end of the series.
> > > 
> > > It costs us nothing to convert them, this isn't a difficult or hard to
> > > understand refactoring - the patch is fine the way it is.
> > 
> > Modulo the defects that might be introduced if an overflow occurs.
> > 
> > struct foo {
> > 	unsigned int a:1;
> > 	bool b:1;
> > }
> > 
> > Assign a non-zero int without bit 0 set to each and see if
> > a and b differ.
> 
> Bool permits implicit pointer-to-bool conversions, so it isn't free
> of pitfalls either.

Care to describe some of those pitfalls?
I can't think of any off the top of my head.

> Overflow is probably more dangerous in general,
> but here there's little chance of pointers or overflow getting involved.

I don't know _this_ code at all, nor have I read it.

If all the conversions are just deleted later, then of course
it should not be converted at all.

I'm just commenting on the proposed refactoring.

I'm trying to show that conversions of bool:1->unsigned int:1
as being trivial are not so trivial after all.

It's fairly common to have code like:

	[bool] foo.bar = some_value & SETTING;

where some value is tested for a mask/bit and a non-zero is true.

So conversions of foo.bar from bool:1 to unsigned int:1 are not
wise unless all possible side effects are known.

RE: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[David Laight]

From: Joe Perches 
> Sent: 24 June 2022 14:45
...
> I'm trying to show that conversions of bool:1->unsigned int:1
> as being trivial are not so trivial after all.
> 
> It's fairly common to have code like:
> 
> 	[bool] foo.bar = some_value & SETTING;
> 
> where some value is tested for a mask/bit and a non-zero is true.
> 
> So conversions of foo.bar from bool:1 to unsigned int:1 are not
> wise unless all possible side effects are known.

I can never decide whether:
	bool_c = bool_a & bool_b;
can be compiled to a simple 'and' instruction, or requires a
load of instructions in case bool_a and/or bool_b has a non-zero
value without the bottom bit set.

All other C types are cpu register types - so you know what happens.
IMHO bool is an abomination and should not be used :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 01/12] regmap-irq: Convert bool bitfields to unsigned int

[Aidan MacDonald]

Joe Perches <joe@perches.com> writes:

> On Fri, 2022-06-24 at 14:05 +0100, Aidan MacDonald wrote:
>> Joe Perches <joe@perches.com> writes:
>> 
>> > On Fri, 2022-06-24 at 13:11 +0100, Mark Brown wrote:
>> > > On Thu, Jun 23, 2022 at 11:26:10PM +0200, Andy Shevchenko wrote:
>> > > > On Thu, Jun 23, 2022 at 11:13 PM Aidan MacDonald
>> > > 
>> > > > > Use 'unsigned int' for bitfields for consistency with most other
>> > > > > kernel code.
>> > > 
>> > > > There is no point to convert the fields you are about to remove.
>> > > 
>> > > > So, either don't touch them or make this patch closer to the end of the series.
>> > > 
>> > > It costs us nothing to convert them, this isn't a difficult or hard to
>> > > understand refactoring - the patch is fine the way it is.
>> > 
>> > Modulo the defects that might be introduced if an overflow occurs.
>> > 
>> > struct foo {
>> > 	unsigned int a:1;
>> > 	bool b:1;
>> > }
>> > 
>> > Assign a non-zero int without bit 0 set to each and see if
>> > a and b differ.
>> 
>> Bool permits implicit pointer-to-bool conversions, so it isn't free
>> of pitfalls either.
>
> Care to describe some of those pitfalls?
> I can't think of any off the top of my head.
>

I just listed the pitfall. I don't consider silently converting a
pointer to a bool value desirable, outside of contexts where that
is made obvious, ie: while(...), if(...), and so on.

>> Overflow is probably more dangerous in general,
>> but here there's little chance of pointers or overflow getting involved.
>
> I don't know _this_ code at all, nor have I read it.
>
> If all the conversions are just deleted later, then of course
> it should not be converted at all.

Only _some_ of the flags are being removed, not all of them.

>
> I'm just commenting on the proposed refactoring.
>
> I'm trying to show that conversions of bool:1->unsigned int:1
> as being trivial are not so trivial after all.
>
> It's fairly common to have code like:
>
> 	[bool] foo.bar = some_value & SETTING;
>
> where some value is tested for a mask/bit and a non-zero is true.
>
> So conversions of foo.bar from bool:1 to unsigned int:1 are not
> wise unless all possible side effects are known.

Good point. I didn't take that into account, because I expect all
users are using literal true/false values.

Anyhow, Andy asked for the flags to be converted to unsigned int since
he thought bool was strange for a bitfield, and grepping showed it was
much less common than unsigned int. I personally don't mind either way,
so maybe it's better to leave them as bools.

Re: [PATCH v2 00/12] regmap-irq cleanups and refactoring

[Mark Brown]

On Thu, 23 Jun 2022 22:14:08 +0100, Aidan MacDonald wrote:
> This series is an attempt at cleaning up the regmap-irq API in order
> to simplify things and consolidate existing features, while at the
> same time generalizing it to support a wider range of hardware.
> 
> There is a new system for IRQ type configuration, some tweaks to
> unmask registers so they're more intuitive and useful, and a new
> callback for calculating register addresses. There's also a few
> minor code cleanups in here.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[01/12] regmap-irq: Convert bool bitfields to unsigned int
        commit: 445cbd219ac3b8f451153c210aaf97adcbf4bd02
[02/12] regmap-irq: Remove unused type_reg_stride field
        commit: 53a1a16dcc972163bd5816192d5d63ae433c9e56
[03/12] regmap-irq: Cleanup sizeof(...) use in memory allocation
        commit: cffc2be30288786e242bceb9fedde4dfe6ce442d
[04/12] regmap-irq: Remove an unnecessary restriction on type_in_mask
        commit: 610fdd668e6af48fcae7908161d14eee3a95ec92
[05/12] regmap-irq: Remove inappropriate uses of regmap_irq_update_bits()
        commit: 6b0c31747722936101d56e71e809bfd7a6a440b7
[06/12] regmap-irq: Remove mask_writeonly and regmap_irq_update_bits()
        commit: ad22b3e98f9430896bd4bd8f4fbff4667f02a0c8
[07/12] regmap-irq: Refactor checks for status bulk read support
        commit: f7cc5062d6e5ca439708e8403b1a622cca75adf7
[08/12] regmap-irq: Introduce config registers for irq types
        commit: faa87ce9196dbb074d75bd4aecb8bacf18f19b4e
[09/12] regmap-irq: Deprecate type registers and virtual registers
        commit: 9edd4f5aee8470dcfd0db04005908f61fbfae8e0
[10/12] regmap-irq: Fix inverted handling of unmask registers
        commit: e8ffb12e7f065db616a3eba79ff138bececf0825
[11/12] regmap-irq: Add get_irq_reg() callback
        commit: bdf9b86cd3adbbcf590ab82b74ab8554534c9b6e
[12/12] regmap-irq: Deprecate the not_fixed_stride flag
        commit: 48e014ee9a61e8f4700987b82f7cb1dc3c89fa76

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 11/12] regmap-irq: Add get_irq_reg() callback

[Marek Szyprowski]

Hi All,

On 23.06.2022 23:14, Aidan MacDonald wrote:
> Replace the internal sub_irq_reg() function with a public callback
> that drivers can use when they have more complex register layouts.
> The default implementation is regmap_irq_get_irq_reg_linear(), used
> if the chip doesn't provide its own callback.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

This patch landed in today's linux next-20220701 as commit bdf9b86cd3ad 
("regmap-irq: Add get_irq_reg() callback"). I've noticed that it causes 
a regression on my test systems: the RTC alarm stopped working on all 
boards with Samsung PMICs (drivers/mfd/sec*.c). There are no 
warnings/oopses/etc. Waitng for the RTC alarm lasts forever, so it looks 
that something is broken with interrupts. Reverting it on top of 
linux-next fixes the issue.

Unfortunately I'm going for a holidays for the whole next week and I'm 
not able to analyze this issue further today. Krzysztof: maybe You will 
be able to provide some more hints which regmap irq variant is broken?


>   drivers/base/regmap/regmap-irq.c | 126 ++++++++++++++++++++-----------
>   include/linux/regmap.h           |  15 +++-
>   2 files changed, 93 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index 2c724ae185c4..71de097847a7 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -46,30 +46,12 @@ struct regmap_irq_chip_data {
>   
>   	unsigned int irq_reg_stride;
>   
> +	unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data,
> +				    unsigned int base, int index);
> +
>   	unsigned int clear_status:1;
>   };
>   
> -static int sub_irq_reg(struct regmap_irq_chip_data *data,
> -		       unsigned int base_reg, int i)
> -{
> -	const struct regmap_irq_chip *chip = data->chip;
> -	struct regmap *map = data->map;
> -	struct regmap_irq_sub_irq_map *subreg;
> -	unsigned int offset;
> -	int reg = 0;
> -
> -	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
> -		/* Assume linear mapping */
> -		reg = base_reg + (i * map->reg_stride * data->irq_reg_stride);
> -	} else {
> -		subreg = &chip->sub_reg_offsets[i];
> -		offset = subreg->offset[0];
> -		reg = base_reg + offset;
> -	}
> -
> -	return reg;
> -}
> -
>   static inline const
>   struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data,
>   				     int irq)
> @@ -81,7 +63,13 @@ static bool regmap_irq_can_bulk_read_status(struct regmap_irq_chip_data *data)
>   {
>   	struct regmap *map = data->map;
>   
> +	/*
> +	 * While possible that a user-defined ->get_irq_reg() callback might
> +	 * be linear enough to support bulk reads, most of the time it won't.
> +	 * Therefore only allow them if the default callback is being used.
> +	 */
>   	return data->irq_reg_stride == 1 && map->reg_stride == 1 &&
> +	       data->get_irq_reg == regmap_irq_get_irq_reg_linear &&
>   	       !map->use_single_read;
>   }
>   
> @@ -109,7 +97,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>   
>   	if (d->clear_status) {
>   		for (i = 0; i < d->chip->num_regs; i++) {
> -			reg = sub_irq_reg(d, d->chip->status_base, i);
> +			reg = d->get_irq_reg(d, d->chip->status_base, i);
>   
>   			ret = regmap_read(map, reg, &val);
>   			if (ret)
> @@ -127,7 +115,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>   	 */
>   	for (i = 0; i < d->chip->num_regs; i++) {
>   		if (d->mask_base) {
> -			reg = sub_irq_reg(d, d->mask_base, i);
> +			reg = d->get_irq_reg(d, d->mask_base, i);
>   			ret = regmap_update_bits(d->map, reg,
>   					d->mask_buf_def[i], d->mask_buf[i]);
>   			if (ret)
> @@ -136,7 +124,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>   		}
>   
>   		if (d->unmask_base) {
> -			reg = sub_irq_reg(d, d->unmask_base, i);
> +			reg = d->get_irq_reg(d, d->unmask_base, i);
>   			ret = regmap_update_bits(d->map, reg,
>   					d->mask_buf_def[i], ~d->mask_buf[i]);
>   			if (ret)
> @@ -144,7 +132,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>   					reg);
>   		}
>   
> -		reg = sub_irq_reg(d, d->chip->wake_base, i);
> +		reg = d->get_irq_reg(d, d->chip->wake_base, i);
>   		if (d->wake_buf) {
>   			if (d->chip->wake_invert)
>   				ret = regmap_update_bits(d->map, reg,
> @@ -168,7 +156,7 @@ 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 = sub_irq_reg(d, d->chip->ack_base, i);
> +			reg = d->get_irq_reg(d, d->chip->ack_base, i);
>   
>   			/* some chips ack by write 0 */
>   			if (d->chip->ack_invert)
> @@ -192,7 +180,7 @@ 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 = sub_irq_reg(d, d->chip->type_base, i);
> +			reg = d->get_irq_reg(d, d->chip->type_base, i);
>   			if (d->chip->type_invert)
>   				ret = regmap_update_bits(d->map, reg,
>   					d->type_buf_def[i], ~d->type_buf[i]);
> @@ -208,8 +196,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>   	if (d->chip->num_virt_regs) {
>   		for (i = 0; i < d->chip->num_virt_regs; i++) {
>   			for (j = 0; j < d->chip->num_regs; j++) {
> -				reg = sub_irq_reg(d, d->chip->virt_reg_base[i],
> -						  j);
> +				reg = d->get_irq_reg(d, d->chip->virt_reg_base[i],
> +						     j);
>   				ret = regmap_write(map, reg, d->virt_buf[i][j]);
>   				if (ret != 0)
>   					dev_err(d->map->dev,
> @@ -221,7 +209,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>   
>   	for (i = 0; i < d->chip->num_config_bases; i++) {
>   		for (j = 0; j < d->chip->num_config_regs; j++) {
> -			reg = sub_irq_reg(d, d->chip->config_base[i], j);
> +			reg = d->get_irq_reg(d, d->chip->config_base[i], j);
>   			ret = regmap_write(map, reg, d->config_buf[i][j]);
>   			if (ret)
>   				dev_err(d->map->dev,
> @@ -382,14 +370,17 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
>   	const struct regmap_irq_chip *chip = data->chip;
>   	struct regmap *map = data->map;
>   	struct regmap_irq_sub_irq_map *subreg;
> +	unsigned int reg;
>   	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]);
> +		reg = data->get_irq_reg(data, chip->status_base, b);
> +		ret = regmap_read(map, reg, &data->status_buf[b]);
>   	} else {
> +		/*
> +		 * Note we can't use ->get_irq_reg() here because the offsets
> +		 * in 'subreg' are *not* interchangeable with indices.
> +		 */
>   		subreg = &chip->sub_reg_offsets[b];
>   		for (i = 0; i < subreg->num_regs; i++) {
>   			unsigned int offset = subreg->offset[i];
> @@ -455,10 +446,18 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>   		 * sake of simplicity. and add bulk reads only if needed
>   		 */
>   		for (i = 0; i < chip->num_main_regs; i++) {
> -			ret = regmap_read(map, chip->main_status +
> -				  (i * map->reg_stride
> -				   * data->irq_reg_stride),
> -				  &data->main_status_buf[i]);
> +			/*
> +			 * For not_fixed_stride, don't use ->get_irq_reg().
> +			 * It would produce an incorrect result.
> +			 */
> +			if (data->chip->not_fixed_stride)
> +				reg = chip->main_status +
> +					i * map->reg_stride * data->irq_reg_stride;
> +			else
> +				reg = data->get_irq_reg(data,
> +							chip->main_status, i);
> +
> +			ret = regmap_read(map, reg, &data->main_status_buf[i]);
>   			if (ret) {
>   				dev_err(map->dev,
>   					"Failed to read IRQ status %d\n",
> @@ -523,7 +522,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>   
>   	} else {
>   		for (i = 0; i < data->chip->num_regs; i++) {
> -			unsigned int reg = sub_irq_reg(data,
> +			unsigned int reg = data->get_irq_reg(data,
>   					data->chip->status_base, i);
>   			ret = regmap_read(map, reg, &data->status_buf[i]);
>   
> @@ -551,7 +550,7 @@ 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 = sub_irq_reg(data, data->chip->ack_base, i);
> +			reg = data->get_irq_reg(data, data->chip->ack_base, i);
>   
>   			if (chip->ack_invert)
>   				ret = regmap_write(map, reg,
> @@ -611,6 +610,36 @@ static const struct irq_domain_ops regmap_domain_ops = {
>   	.xlate	= irq_domain_xlate_onetwocell,
>   };
>   
> +/**
> + * regmap_irq_get_irq_reg_linear() - Linear IRQ register mapping callback.
> + * @data: Data for the &struct regmap_irq_chip
> + * @base: Base register
> + * @index: Register index
> + *
> + * Returns the register address corresponding to the given @base and @index
> + * by the formula ``base + index * regmap_stride * irq_reg_stride``.
> + */
> +unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
> +					   unsigned int base, int index)
> +{
> +	const struct regmap_irq_chip *chip = data->chip;
> +	struct regmap *map = data->map;
> +
> +	/*
> +	 * FIXME: This is for backward compatibility and should be removed
> +	 * when not_fixed_stride is dropped (it's only used by qcom-pm8008).
> +	 */
> +	if (chip->not_fixed_stride && chip->sub_reg_offsets) {
> +		struct regmap_irq_sub_irq_map *subreg;
> +
> +		subreg = &chip->sub_reg_offsets[0];
> +		return base + subreg->offset[0];
> +	}
> +
> +	return base + index * map->reg_stride * chip->irq_reg_stride;
> +}
> +EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear);
> +
>   /**
>    * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback.
>    * @buf: Buffer containing configuration register values, this is a 2D array of
> @@ -862,6 +891,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>   	else
>   		d->irq_reg_stride = 1;
>   
> +	if (chip->get_irq_reg)
> +		d->get_irq_reg = chip->get_irq_reg;
> +	else
> +		d->get_irq_reg = regmap_irq_get_irq_reg_linear;
> +
>   	if (regmap_irq_can_bulk_read_status(d)) {
>   		d->status_reg_buf = kmalloc_array(chip->num_regs,
>   						  map->format.val_bytes,
> @@ -881,7 +915,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>   		d->mask_buf[i] = d->mask_buf_def[i];
>   
>   		if (d->mask_base) {
> -			reg = sub_irq_reg(d, d->mask_base, i);
> +			reg = d->get_irq_reg(d, d->mask_base, i);
>   			ret = regmap_update_bits(d->map, reg,
>   					d->mask_buf_def[i], d->mask_buf[i]);
>   			if (ret) {
> @@ -892,7 +926,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>   		}
>   
>   		if (d->unmask_base) {
> -			reg = sub_irq_reg(d, d->unmask_base, i);
> +			reg = d->get_irq_reg(d, d->unmask_base, i);
>   			ret = regmap_update_bits(d->map, reg,
>   					d->mask_buf_def[i], ~d->mask_buf[i]);
>   			if (ret) {
> @@ -906,7 +940,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>   			continue;
>   
>   		/* Ack masked but set interrupts */
> -		reg = sub_irq_reg(d, d->chip->status_base, i);
> +		reg = d->get_irq_reg(d, d->chip->status_base, i);
>   		ret = regmap_read(map, reg, &d->status_buf[i]);
>   		if (ret != 0) {
>   			dev_err(map->dev, "Failed to read IRQ status: %d\n",
> @@ -918,7 +952,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>   			d->status_buf[i] = ~d->status_buf[i];
>   
>   		if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
> -			reg = sub_irq_reg(d, d->chip->ack_base, i);
> +			reg = d->get_irq_reg(d, d->chip->ack_base, i);
>   			if (chip->ack_invert)
>   				ret = regmap_write(map, reg,
>   					~(d->status_buf[i] & d->mask_buf[i]));
> @@ -943,7 +977,7 @@ 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 = sub_irq_reg(d, d->chip->wake_base, i);
> +			reg = d->get_irq_reg(d, d->chip->wake_base, i);
>   
>   			if (chip->wake_invert)
>   				ret = regmap_update_bits(d->map, reg,
> @@ -963,7 +997,7 @@ 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 = sub_irq_reg(d, d->chip->type_base, i);
> +			reg = d->get_irq_reg(d, d->chip->type_base, i);
>   
>   			ret = regmap_read(map, reg, &d->type_buf_def[i]);
>   
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 708f36dfaeda..ae5f1f7d4b5a 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1424,6 +1424,8 @@ struct regmap_irq_sub_irq_map {
>   	unsigned int *offset;
>   };
>   
> +struct regmap_irq_chip_data;
> +
>   /**
>    * struct regmap_irq_chip - Description of a generic regmap irq_chip.
>    *
> @@ -1515,6 +1517,13 @@ struct regmap_irq_sub_irq_map {
>    *		     and configure virt regs. Deprecated, use @set_type_config
>    *		     callback and config registers instead.
>    * @set_type_config: Callback used for configuring irq types.
> + * @get_irq_reg: Callback for mapping (base register, index) pairs to register
> + *		 addresses. The base register will be one of @status_base,
> + *		 @mask_base, etc., @main_status, or any of @config_base.
> + *		 The index will be in the range [0, num_main_regs[ for the
> + *		 main status base, [0, num_type_settings[ for any config
> + *		 register base, and [0, num_regs[ for any other base.
> + *		 If unspecified then regmap_irq_get_irq_reg_linear() is used.
>    * @irq_drv_data:    Driver specific IRQ data which is passed as parameter when
>    *		     driver specific pre/post interrupt handler is called.
>    *
> @@ -1569,11 +1578,13 @@ struct regmap_irq_chip {
>   			     unsigned long hwirq, int reg);
>   	int (*set_type_config)(unsigned int **buf, unsigned int type,
>   			       const struct regmap_irq *irq_data, int idx);
> +	unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data,
> +				    unsigned int base, int index);
>   	void *irq_drv_data;
>   };
>   
> -struct regmap_irq_chip_data;
> -
> +unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
> +					   unsigned int base, int index);
>   int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
>   				      const struct regmap_irq *irq_data, int idx);
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 11/12] regmap-irq: Add get_irq_reg() callback

[Aidan MacDonald]

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Hi All,
>
> On 23.06.2022 23:14, Aidan MacDonald wrote:
>> Replace the internal sub_irq_reg() function with a public callback
>> that drivers can use when they have more complex register layouts.
>> The default implementation is regmap_irq_get_irq_reg_linear(), used
>> if the chip doesn't provide its own callback.
>>
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>
> This patch landed in today's linux next-20220701 as commit bdf9b86cd3ad 
> ("regmap-irq: Add get_irq_reg() callback"). I've noticed that it causes 
> a regression on my test systems: the RTC alarm stopped working on all 
> boards with Samsung PMICs (drivers/mfd/sec*.c). There are no 
> warnings/oopses/etc. Waitng for the RTC alarm lasts forever, so it looks 
> that something is broken with interrupts. Reverting it on top of 
> linux-next fixes the issue.
>
> Unfortunately I'm going for a holidays for the whole next week and I'm 
> not able to analyze this issue further today. Krzysztof: maybe You will 
> be able to provide some more hints which regmap irq variant is broken?

Fortunately it's nothing complicated -- just a typo. Thanks for testing.

>>   drivers/base/regmap/regmap-irq.c | 126 ++++++++++++++++++++-----------
>>   include/linux/regmap.h           |  15 +++-
>>   2 files changed, 93 insertions(+), 48 deletions(-)
>> [..]
>> +unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
>> +					   unsigned int base, int index)
>> +{
>> +	const struct regmap_irq_chip *chip = data->chip;
>> +	struct regmap *map = data->map;
>> +
>> +	/*
>> +	 * FIXME: This is for backward compatibility and should be removed
>> +	 * when not_fixed_stride is dropped (it's only used by qcom-pm8008).
>> +	 */
>> +	if (chip->not_fixed_stride && chip->sub_reg_offsets) {
>> +		struct regmap_irq_sub_irq_map *subreg;
>> +
>> +		subreg = &chip->sub_reg_offsets[0];
>> +		return base + subreg->offset[0];
>> +	}
>> +
>> +	return base + index * map->reg_stride * chip->irq_reg_stride;

chip->irq_reg_stride is usually 0. This should be data->irq_reg_stride,
which will be corrected to the default value of 1.

>> +}
>> +EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear);
>> +
>>   /**
>>    * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback.
>>    * @buf: Buffer containing configuration register values, this is a 2D array of
>> @@ -862,6 +891,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>
> Best regards