2024-05-29 16:37:49

by Johan Hovold

Subject: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

The regmap irq array is potentially shared between multiple PMICs and
should only contain static data.

Use a custom macro to initialise also the type fields and drop the
unnecessary updates on each probe.

Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/mfd/qcom-pm8008.c | 64 ++++++++++++++-------------------------
1 file changed, 23 insertions(+), 41 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 3ac3742f438b..f71c490f25c8 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -56,15 +56,25 @@ static unsigned int pm8008_config_regs[] = {
INT_POL_LOW_OFFSET,
};

-static struct regmap_irq pm8008_irqs[] = {
- REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0)),
- REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1)),
- REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2)),
- REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3)),
- REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4)),
- REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM, BIT(0)),
- REGMAP_IRQ_REG(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0)),
- REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)),
+#define _IRQ(_irq, _off, _mask, _types) \
+ [_irq] = { \
+ .reg_offset = (_off), \
+ .mask = (_mask), \
+ .type = { \
+ .type_reg_offset = (_off), \
+ .types_supported = (_types), \
+ }, \
+ }
+
+static const struct regmap_irq pm8008_irqs[] = {
+ _IRQ(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0), IRQ_TYPE_EDGE_RISING),
+ _IRQ(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1), IRQ_TYPE_EDGE_RISING),
+ _IRQ(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2), IRQ_TYPE_EDGE_RISING),
+ _IRQ(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3), IRQ_TYPE_EDGE_RISING),
+ _IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4), IRQ_TYPE_EDGE_RISING),
+ _IRQ(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
+ _IRQ(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0), IRQ_TYPE_SENSE_MASK),
+ _IRQ(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0), IRQ_TYPE_SENSE_MASK),
};

static const unsigned int pm8008_periph_base[] = {
@@ -143,38 +153,9 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
.max_register = 0xFFFF,
};

-static int pm8008_probe_irq_peripherals(struct device *dev,
- struct regmap *regmap,
- int client_irq)
-{
- int rc, i;
- struct regmap_irq_type *type;
- struct regmap_irq_chip_data *irq_data;
-
- for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
- type = &pm8008_irqs[i].type;
-
- type->type_reg_offset = pm8008_irqs[i].reg_offset;
-
- if (type->type_reg_offset == PM8008_MISC)
- type->types_supported = IRQ_TYPE_EDGE_RISING;
- else
- type->types_supported = (IRQ_TYPE_EDGE_BOTH |
- IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
- }
-
- rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
- IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
- if (rc) {
- dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
- return rc;
- }
-
- return 0;
-}
-
static int pm8008_probe(struct i2c_client *client)
{
+ struct regmap_irq_chip_data *irq_data;
int rc;
struct device *dev;
struct regmap *regmap;
@@ -187,9 +168,10 @@ static int pm8008_probe(struct i2c_client *client)
i2c_set_clientdata(client, regmap);

if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
- rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
+ rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
+ IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
if (rc)
- dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
+ dev_err(dev, "failed to add IRQ chip: %d\n", rc);
}

return devm_of_platform_populate(dev);
--
2.44.1

2024-05-31 16:23:10

by Bryan O'Donoghue

Subject: Re: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

On 29/05/2024 17:29, Johan Hovold wrote:
> The regmap irq array is potentially shared between multiple PMICs and
> should only contain static data.
>
> Use a custom macro to initialise also the type fields and drop the
> unnecessary updates on each probe.
>
> Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/mfd/qcom-pm8008.c | 64 ++++++++++++++-------------------------
> 1 file changed, 23 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 3ac3742f438b..f71c490f25c8 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -56,15 +56,25 @@ static unsigned int pm8008_config_regs[] = {
> INT_POL_LOW_OFFSET,
> };
>
> -static struct regmap_irq pm8008_irqs[] = {
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4)),
> - REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM, BIT(0)),
> - REGMAP_IRQ_REG(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0)),
> - REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)),
> +#define _IRQ(_irq, _off, _mask, _types) \
> + [_irq] = { \
> + .reg_offset = (_off), \
> + .mask = (_mask), \
> + .type = { \
> + .type_reg_offset = (_off), \
> + .types_supported = (_types), \
> + }, \
> + }
> +
> +static const struct regmap_irq pm8008_irqs[] = {
> + _IRQ(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
> + _IRQ(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0), IRQ_TYPE_SENSE_MASK),
> + _IRQ(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0), IRQ_TYPE_SENSE_MASK),
> };
>
> static const unsigned int pm8008_periph_base[] = {
> @@ -143,38 +153,9 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> .max_register = 0xFFFF,
> };
>
> -static int pm8008_probe_irq_peripherals(struct device *dev,
> - struct regmap *regmap,
> - int client_irq)
> -{
> - int rc, i;
> - struct regmap_irq_type *type;
> - struct regmap_irq_chip_data *irq_data;
> -
> - for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
> - type = &pm8008_irqs[i].type;
> -
> - type->type_reg_offset = pm8008_irqs[i].reg_offset;
> -
> - if (type->type_reg_offset == PM8008_MISC)
> - type->types_supported = IRQ_TYPE_EDGE_RISING;
> - else
> - type->types_supported = (IRQ_TYPE_EDGE_BOTH |
> - IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> - }
ick

A no-brainer improvement.

Reviewed-by: Bryan O'Donoghue <[email protected]>

2024-05-31 17:04:09

by Lee Jones

Subject: Re: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

On Wed, 29 May 2024, Johan Hovold wrote:

> The regmap irq array is potentially shared between multiple PMICs and
> should only contain static data.
>
> Use a custom macro to initialise also the type fields and drop the
> unnecessary updates on each probe.
>
> Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/mfd/qcom-pm8008.c | 64 ++++++++++++++-------------------------
> 1 file changed, 23 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 3ac3742f438b..f71c490f25c8 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -56,15 +56,25 @@ static unsigned int pm8008_config_regs[] = {
> INT_POL_LOW_OFFSET,
> };
>
> -static struct regmap_irq pm8008_irqs[] = {
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3)),
> - REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4)),
> - REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM, BIT(0)),
> - REGMAP_IRQ_REG(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0)),
> - REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)),
> +#define _IRQ(_irq, _off, _mask, _types) \
> + [_irq] = { \
> + .reg_offset = (_off), \
> + .mask = (_mask), \
> + .type = { \
> + .type_reg_offset = (_off), \
> + .types_supported = (_types), \
> + }, \
> + }

Any reason why this can't be generic and be tucked away somewhere in a
header file?

> +static const struct regmap_irq pm8008_irqs[] = {
> + _IRQ(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4), IRQ_TYPE_EDGE_RISING),
> + _IRQ(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
> + _IRQ(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0), IRQ_TYPE_SENSE_MASK),
> + _IRQ(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0), IRQ_TYPE_SENSE_MASK),
> };
>
> static const unsigned int pm8008_periph_base[] = {
> @@ -143,38 +153,9 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> .max_register = 0xFFFF,
> };
>
> -static int pm8008_probe_irq_peripherals(struct device *dev,
> - struct regmap *regmap,
> - int client_irq)
> -{
> - int rc, i;
> - struct regmap_irq_type *type;
> - struct regmap_irq_chip_data *irq_data;
> -
> - for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
> - type = &pm8008_irqs[i].type;
> -
> - type->type_reg_offset = pm8008_irqs[i].reg_offset;
> -
> - if (type->type_reg_offset == PM8008_MISC)
> - type->types_supported = IRQ_TYPE_EDGE_RISING;
> - else
> - type->types_supported = (IRQ_TYPE_EDGE_BOTH |
> - IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> - }
> -
> - rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> - IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> - if (rc) {
> - dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> - return rc;
> - }
> -
> - return 0;
> -}
> -
> static int pm8008_probe(struct i2c_client *client)
> {
> + struct regmap_irq_chip_data *irq_data;
> int rc;
> struct device *dev;
> struct regmap *regmap;
> @@ -187,9 +168,10 @@ static int pm8008_probe(struct i2c_client *client)
> i2c_set_clientdata(client, regmap);
>
> if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> - rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
> + rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
> + IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> if (rc)
> - dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> + dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> }
>
> return devm_of_platform_populate(dev);
> --
> 2.44.1
>

--
Lee Jones [李琼斯]

2024-06-08 15:48:54

by Johan Hovold

Subject: Re: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

On Fri, May 31, 2024 at 06:03:53PM +0100, Lee Jones wrote:
> On Wed, 29 May 2024, Johan Hovold wrote:
>
> > The regmap irq array is potentially shared between multiple PMICs and
> > should only contain static data.
> >
> > Use a custom macro to initialise also the type fields and drop the
> > unnecessary updates on each probe.
> >
> > Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
> > Signed-off-by: Johan Hovold <[email protected]>

> > -static struct regmap_irq pm8008_irqs[] = {
> > - REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM, BIT(0)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0)),
> > - REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)),
> > +#define _IRQ(_irq, _off, _mask, _types) \
> > + [_irq] = { \
> > + .reg_offset = (_off), \
> > + .mask = (_mask), \
> > + .type = { \
> > + .type_reg_offset = (_off), \
> > + .types_supported = (_types), \
> > + }, \
> > + }
>
> Any reason why this can't be generic and be tucked away somewhere in a
> header file?

These macros tend to be quite driver specific so not sure it makes sense
to try to generalise beyond the basic ones already provided by regmap.

Either way, I don't think that should be a prerequisite for fixing this
driver.

I'm also considering replacing the current irq chip implementation as
part of unifying with the SPMI implementation.

> > +static const struct regmap_irq pm8008_irqs[] = {
> > + _IRQ(PM8008_IRQ_MISC_UVLO, PM8008_MISC, BIT(0), IRQ_TYPE_EDGE_RISING),
> > + _IRQ(PM8008_IRQ_MISC_OVLO, PM8008_MISC, BIT(1), IRQ_TYPE_EDGE_RISING),
> > + _IRQ(PM8008_IRQ_MISC_OTST2, PM8008_MISC, BIT(2), IRQ_TYPE_EDGE_RISING),
> > + _IRQ(PM8008_IRQ_MISC_OTST3, PM8008_MISC, BIT(3), IRQ_TYPE_EDGE_RISING),
> > + _IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC, BIT(4), IRQ_TYPE_EDGE_RISING),
> > + _IRQ(PM8008_IRQ_TEMP_ALARM, PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
> > + _IRQ(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0), IRQ_TYPE_SENSE_MASK),
> > + _IRQ(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0), IRQ_TYPE_SENSE_MASK),
> > };

Johan