[PATCH v1 0/4] regmap-irq fixes for qcom-pm8008

[PATCH v1 0/4] regmap-irq fixes for qcom-pm8008

[Aidan MacDonald]

The PM8008 driver is the last one using a bunch of deprecated features
in regmap-irq. Replace these with more generally useful equivalents so
the deprecated stuff can finally be removed.

Note, I have not tested any of these patches.

Aidan MacDonald (4):
  mfd: qcom-pm8008: Fix swapped mask/unmask in irq chip
  mfd: qcom-pm8008: Convert irq chip to config regs
  mfd: qcom-pm8008: Use .get_irq_reg() for irq chip
  mfd: qcom-pm8008: Remove workaround for a regmap-irq quirk

 drivers/mfd/qcom-pm8008.c | 131 ++++++++++++++------------------------
 1 file changed, 48 insertions(+), 83 deletions(-)

-- 
2.39.2

[PATCH v1 1/4] mfd: qcom-pm8008: Fix swapped mask/unmask in irq chip

[Aidan MacDonald]

The usual behavior of mask registers is writing a '1' bit to
disable (mask) an interrupt; similarly, writing a '1' bit to
an unmask register enables (unmasks) an interrupt.

Due to a longstanding issue in regmap-irq, mask and unmask
registers were inverted when both kinds of registers were
present on the same chip, ie. regmap-irq actually wrote '1's
to the mask register to enable an IRQ and '1's to the unmask
register to disable an IRQ.

This was fixed by commit e8ffb12e7f06 ("regmap-irq: Fix
inverted handling of unmask registers") but the fix is opt-in
via mask_unmask_non_inverted = true because it requires manual
changes for each affected driver. The new behavior will become
the default once all drivers have been updated.

The PM8008 appears to rely on the inverted behavior. It has
separate set & clear registers for a register called INT_EN,
which presumably enables interrupts by writing '1's. Opt in
to the new non-inverted behavior & swap mask_base/unmask_base.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/mfd/qcom-pm8008.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 9f3c4a01b4c1..39fd2a792e73 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -45,8 +45,8 @@ enum {
 #define PM8008_GPIO2_ADDR	PM8008_PERIPH_3_BASE
 
 #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_MASK_BASE	(PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET)
+#define PM8008_UNMASK_BASE	(PM8008_PERIPH_0_BASE | INT_EN_SET_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)
@@ -131,6 +131,7 @@ static struct regmap_irq_chip pm8008_irq_chip = {
 	.status_base		= PM8008_STATUS_BASE,
 	.mask_base		= PM8008_MASK_BASE,
 	.unmask_base		= PM8008_UNMASK_BASE,
+	.mask_unmask_non_inverted = true,
 	.type_base		= PM8008_TYPE_BASE,
 	.ack_base		= PM8008_ACK_BASE,
 	.virt_reg_base		= pm8008_virt_regs,
-- 
2.39.2

Re: [PATCH v1 1/4] mfd: qcom-pm8008: Fix swapped mask/unmask in irq chip

[Lee Jones]

On Thu, 16 Feb 2023, Aidan MacDonald wrote:

> The usual behavior of mask registers is writing a '1' bit to
> disable (mask) an interrupt; similarly, writing a '1' bit to
> an unmask register enables (unmasks) an interrupt.
> 
> Due to a longstanding issue in regmap-irq, mask and unmask
> registers were inverted when both kinds of registers were
> present on the same chip, ie. regmap-irq actually wrote '1's
> to the mask register to enable an IRQ and '1's to the unmask
> register to disable an IRQ.
> 
> This was fixed by commit e8ffb12e7f06 ("regmap-irq: Fix
> inverted handling of unmask registers") but the fix is opt-in
> via mask_unmask_non_inverted = true because it requires manual
> changes for each affected driver. The new behavior will become
> the default once all drivers have been updated.
> 
> The PM8008 appears to rely on the inverted behavior. It has
> separate set & clear registers for a register called INT_EN,
> which presumably enables interrupts by writing '1's. Opt in
> to the new non-inverted behavior & swap mask_base/unmask_base.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/mfd/qcom-pm8008.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v1 2/4] mfd: qcom-pm8008: Convert irq chip to config regs

[Aidan MacDonald]

Replace type and virtual registers, which are both deprecated,
with config registers. This also simplifies the driver because
IRQ types are set in one place, the set_type_config() callback.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/mfd/qcom-pm8008.c | 50 +++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 39fd2a792e73..2b6763605cd7 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -66,15 +66,16 @@ static struct regmap_irq_sub_irq_map pm8008_sub_reg_offsets[] = {
 	REGMAP_IRQ_MAIN_REG_OFFSET(p3_offs),
 };
 
-static unsigned int pm8008_virt_regs[] = {
-	PM8008_POLARITY_HI_BASE,
-	PM8008_POLARITY_LO_BASE,
-};
-
 enum {
+	SET_TYPE_INDEX,
 	POLARITY_HI_INDEX,
 	POLARITY_LO_INDEX,
-	PM8008_NUM_VIRT_REGS,
+};
+
+static unsigned int pm8008_config_regs[] = {
+	PM8008_TYPE_BASE,
+	PM8008_POLARITY_HI_BASE,
+	PM8008_POLARITY_LO_BASE,
 };
 
 static struct regmap_irq pm8008_irqs[] = {
@@ -88,32 +89,36 @@ static struct regmap_irq pm8008_irqs[] = {
 	REGMAP_IRQ_REG(PM8008_IRQ_GPIO2,	PM8008_GPIO2,	BIT(0)),
 };
 
-static int pm8008_set_type_virt(unsigned int **virt_buf,
-				      unsigned int type, unsigned long hwirq,
-				      int reg)
+static int pm8008_set_type_config(unsigned int **buf, unsigned int type,
+				  const struct regmap_irq *irq_data, int idx)
 {
 	switch (type) {
 	case IRQ_TYPE_EDGE_FALLING:
 	case IRQ_TYPE_LEVEL_LOW:
-		virt_buf[POLARITY_HI_INDEX][reg] &= ~pm8008_irqs[hwirq].mask;
-		virt_buf[POLARITY_LO_INDEX][reg] |= pm8008_irqs[hwirq].mask;
+		buf[POLARITY_HI_INDEX][idx] &= ~irq_data->mask;
+		buf[POLARITY_LO_INDEX][idx] |= irq_data->mask;
 		break;
 
 	case IRQ_TYPE_EDGE_RISING:
 	case IRQ_TYPE_LEVEL_HIGH:
-		virt_buf[POLARITY_HI_INDEX][reg] |= pm8008_irqs[hwirq].mask;
-		virt_buf[POLARITY_LO_INDEX][reg] &= ~pm8008_irqs[hwirq].mask;
+		buf[POLARITY_HI_INDEX][idx] |= irq_data->mask;
+		buf[POLARITY_LO_INDEX][idx] &= ~irq_data->mask;
 		break;
 
 	case IRQ_TYPE_EDGE_BOTH:
-		virt_buf[POLARITY_HI_INDEX][reg] |= pm8008_irqs[hwirq].mask;
-		virt_buf[POLARITY_LO_INDEX][reg] |= pm8008_irqs[hwirq].mask;
+		buf[POLARITY_HI_INDEX][idx] |= irq_data->mask;
+		buf[POLARITY_LO_INDEX][idx] |= irq_data->mask;
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		buf[SET_TYPE_INDEX][idx] |= irq_data->mask;
+	else
+		buf[SET_TYPE_INDEX][idx] &= ~irq_data->mask;
+
 	return 0;
 }
 
@@ -121,21 +126,20 @@ static struct regmap_irq_chip pm8008_irq_chip = {
 	.name			= "pm8008_irq",
 	.main_status		= I2C_INTR_STATUS_BASE,
 	.num_main_regs		= 1,
-	.num_virt_regs		= PM8008_NUM_VIRT_REGS,
 	.irqs			= pm8008_irqs,
 	.num_irqs		= ARRAY_SIZE(pm8008_irqs),
 	.num_regs		= PM8008_NUM_PERIPHS,
 	.not_fixed_stride	= true,
 	.sub_reg_offsets	= pm8008_sub_reg_offsets,
-	.set_type_virt		= pm8008_set_type_virt,
 	.status_base		= PM8008_STATUS_BASE,
 	.mask_base		= PM8008_MASK_BASE,
 	.unmask_base		= PM8008_UNMASK_BASE,
 	.mask_unmask_non_inverted = true,
-	.type_base		= PM8008_TYPE_BASE,
 	.ack_base		= PM8008_ACK_BASE,
-	.virt_reg_base		= pm8008_virt_regs,
-	.num_type_reg		= PM8008_NUM_PERIPHS,
+	.config_base		= pm8008_config_regs,
+	.num_config_bases	= ARRAY_SIZE(pm8008_config_regs),
+	.num_config_regs	= PM8008_NUM_PERIPHS,
+	.set_type_config	= pm8008_set_type_config,
 };
 
 static struct regmap_config qcom_mfd_regmap_cfg = {
@@ -185,11 +189,7 @@ static int pm8008_probe_irq_peripherals(struct device *dev,
 	for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
 		type = &pm8008_irqs[i].type;
 
-		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 = 0;
-		type->type_level_low_val  = 0;
+		type->type_reg_offset = pm8008_irqs[i].reg_offset;
 
 		if (type->type_reg_offset == PM8008_MISC)
 			type->types_supported = IRQ_TYPE_EDGE_RISING;
-- 
2.39.2

Re: [PATCH v1 2/4] mfd: qcom-pm8008: Convert irq chip to config regs

[Lee Jones]

On Thu, 16 Feb 2023, Aidan MacDonald wrote:

> Replace type and virtual registers, which are both deprecated,
> with config registers. This also simplifies the driver because
> IRQ types are set in one place, the set_type_config() callback.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/mfd/qcom-pm8008.c | 50 +++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v1 3/4] mfd: qcom-pm8008: Use .get_irq_reg() for irq chip

[Aidan MacDonald]

Replace the deprecated not_fixed_stride flag and the associated
hierarchy of offsets with a .get_irq_reg() callback.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/mfd/qcom-pm8008.c | 56 +++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 2b6763605cd7..4bcdf0e50c40 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -44,28 +44,6 @@ enum {
 #define PM8008_GPIO1_ADDR	PM8008_PERIPH_2_BASE
 #define PM8008_GPIO2_ADDR	PM8008_PERIPH_3_BASE
 
-#define PM8008_STATUS_BASE	(PM8008_PERIPH_0_BASE | INT_LATCHED_STS_OFFSET)
-#define PM8008_MASK_BASE	(PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET)
-#define PM8008_UNMASK_BASE	(PM8008_PERIPH_0_BASE | INT_EN_SET_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 PM8008_PERIPH_OFFSET(paddr)	(paddr - PM8008_PERIPH_0_BASE)
-
-static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
-static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
-static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
-static unsigned int p3_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_3_BASE)};
-
-static struct regmap_irq_sub_irq_map pm8008_sub_reg_offsets[] = {
-	REGMAP_IRQ_MAIN_REG_OFFSET(p0_offs),
-	REGMAP_IRQ_MAIN_REG_OFFSET(p1_offs),
-	REGMAP_IRQ_MAIN_REG_OFFSET(p2_offs),
-	REGMAP_IRQ_MAIN_REG_OFFSET(p3_offs),
-};
-
 enum {
 	SET_TYPE_INDEX,
 	POLARITY_HI_INDEX,
@@ -73,9 +51,9 @@ enum {
 };
 
 static unsigned int pm8008_config_regs[] = {
-	PM8008_TYPE_BASE,
-	PM8008_POLARITY_HI_BASE,
-	PM8008_POLARITY_LO_BASE,
+	INT_SET_TYPE_OFFSET,
+	INT_POL_HIGH_OFFSET,
+	INT_POL_LOW_OFFSET,
 };
 
 static struct regmap_irq pm8008_irqs[] = {
@@ -89,6 +67,23 @@ static struct regmap_irq pm8008_irqs[] = {
 	REGMAP_IRQ_REG(PM8008_IRQ_GPIO2,	PM8008_GPIO2,	BIT(0)),
 };
 
+static const unsigned int pm8008_periph_base[] = {
+	PM8008_PERIPH_0_BASE,
+	PM8008_PERIPH_1_BASE,
+	PM8008_PERIPH_2_BASE,
+	PM8008_PERIPH_3_BASE,
+};
+
+static unsigned int pm8008_get_irq_reg(struct regmap_irq_chip_data *data,
+				       unsigned int base, int index)
+{
+	/* Simple linear addressing for the main status register */
+	if (base == I2C_INTR_STATUS_BASE)
+		return base + index;
+
+	return pm8008_periph_base[index] + base;
+}
+
 static int pm8008_set_type_config(unsigned int **buf, unsigned int type,
 				  const struct regmap_irq *irq_data, int idx)
 {
@@ -129,17 +124,16 @@ static struct regmap_irq_chip pm8008_irq_chip = {
 	.irqs			= pm8008_irqs,
 	.num_irqs		= ARRAY_SIZE(pm8008_irqs),
 	.num_regs		= PM8008_NUM_PERIPHS,
-	.not_fixed_stride	= true,
-	.sub_reg_offsets	= pm8008_sub_reg_offsets,
-	.status_base		= PM8008_STATUS_BASE,
-	.mask_base		= PM8008_MASK_BASE,
-	.unmask_base		= PM8008_UNMASK_BASE,
+	.status_base		= INT_LATCHED_STS_OFFSET,
+	.mask_base		= INT_EN_CLR_OFFSET,
+	.unmask_base		= INT_EN_SET_OFFSET,
 	.mask_unmask_non_inverted = true,
-	.ack_base		= PM8008_ACK_BASE,
+	.ack_base		= INT_LATCHED_CLR_OFFSET,
 	.config_base		= pm8008_config_regs,
 	.num_config_bases	= ARRAY_SIZE(pm8008_config_regs),
 	.num_config_regs	= PM8008_NUM_PERIPHS,
 	.set_type_config	= pm8008_set_type_config,
+	.get_irq_reg		= pm8008_get_irq_reg,
 };
 
 static struct regmap_config qcom_mfd_regmap_cfg = {
-- 
2.39.2

Re: [PATCH v1 3/4] mfd: qcom-pm8008: Use .get_irq_reg() for irq chip

[Lee Jones]

On Thu, 16 Feb 2023, Aidan MacDonald wrote:

> Replace the deprecated not_fixed_stride flag and the associated
> hierarchy of offsets with a .get_irq_reg() callback.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/mfd/qcom-pm8008.c | 56 +++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v1 4/4] mfd: qcom-pm8008: Remove workaround for a regmap-irq quirk

[Aidan MacDonald]

Remove pm8008_init(), which according to the comments exists only
as a workaround for regmap-irq's odd treatment of type registers.
This workaround shouldn't be needed anymore because this driver
uses config registers, which are always programmed by regmap-irq
no matter what the initial register state is.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/mfd/qcom-pm8008.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 4bcdf0e50c40..a33fbc42ac8e 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -142,30 +142,6 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
 	.max_register	= 0xFFFF,
 };
 
-static int pm8008_init(struct regmap *regmap)
-{
-	int rc;
-
-	/*
-	 * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework
-	 * reads this as the default value instead of zero, the HW default.
-	 * This is required to enable the writing of TYPE registers in
-	 * regmap_irq_sync_unlock().
-	 */
-	rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
-	if (rc)
-		return rc;
-
-	/* Do the same for GPIO1 and GPIO2 peripherals */
-	rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
-	if (rc)
-		return rc;
-
-	rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
-
-	return rc;
-}
-
 static int pm8008_probe_irq_peripherals(struct device *dev,
 					struct regmap *regmap,
 					int client_irq)
@@ -174,12 +150,6 @@ static int pm8008_probe_irq_peripherals(struct device *dev,
 	struct regmap_irq_type *type;
 	struct regmap_irq_chip_data *irq_data;
 
-	rc = pm8008_init(regmap);
-	if (rc) {
-		dev_err(dev, "Init failed: %d\n", rc);
-		return rc;
-	}
-
 	for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
 		type = &pm8008_irqs[i].type;
 
-- 
2.39.2

Re: [PATCH v1 4/4] mfd: qcom-pm8008: Remove workaround for a regmap-irq quirk

[Lee Jones]

On Thu, 16 Feb 2023, Aidan MacDonald wrote:

> Remove pm8008_init(), which according to the comments exists only
> as a workaround for regmap-irq's odd treatment of type registers.
> This workaround shouldn't be needed anymore because this driver
> uses config registers, which are always programmed by regmap-irq
> no matter what the initial register state is.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/mfd/qcom-pm8008.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]