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 | 66 +++++++++++++++------------------------
1 file changed, 25 insertions(+), 41 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 3ac3742f438b..d53c987b0d49 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -56,15 +56,27 @@ 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), \
+ }, \
+ }
+
+#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)
+
+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_ALL),
+ _IRQ(PM8008_IRQ_GPIO1, PM8008_GPIO1, BIT(0), _IRQ_TYPE_ALL),
+ _IRQ(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0), _IRQ_TYPE_ALL),
};
static const unsigned int pm8008_periph_base[] = {
@@ -143,38 +155,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 +170,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.43.2
Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> The regmap irq array is potentially shared between multiple PMICs and
IRQ
> should only contain static data.
>
> Use a custom macro to initialise also the type fields and drop the
> unnecessary updates on each probe.
..
> +#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)
This is repetition of IRQ_TYPE_DEFAULT.
..
> - dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> + dev_err(dev, "failed to add IRQ chip: %d\n", rc);
dev_err_probe(...); ?
--
With Best Regards,
Andy Shevchenko
On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > The regmap irq array is potentially shared between multiple PMICs and
>
> IRQ
I'm referring to an array of struct regmap_irq. Perhaps I can add an
underscore.
> > should only contain static data.
> >
> > Use a custom macro to initialise also the type fields and drop the
> > unnecessary updates on each probe.
>
> ...
>
> > +#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)
>
> This is repetition of IRQ_TYPE_DEFAULT.
Thanks, I guess I should use IRQ_TYPE_SENSE_MASK here even.
> ...
>
> > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > + dev_err(dev, "failed to add IRQ chip: %d\n", rc);
>
> dev_err_probe(...); ?
This function won't return -EPROBE_DEFER, and that would be a separate
change in any case.
Johan
On Tue, May 7, 2024 at 6:01 PM Johan Hovold <[email protected]> wrote:
> On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > > The regmap irq array is potentially shared between multiple PMICs and
..
> > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> >
> > dev_err_probe(...); ?
>
> This function won't return -EPROBE_DEFER,
This is not an argument for a long time (since documentation of
dev_err_probe() had been amended to encourage its use for any error
cases in probe).
> and that would be a separate
> change in any case.
Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?
--
With Best Regards,
Andy Shevchenko
On Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:01 PM Johan Hovold <[email protected]> wrote:
> > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > > > The regmap irq array is potentially shared between multiple PMICs and
>
> ...
>
> > > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> > >
> > > dev_err_probe(...); ?
> >
> > This function won't return -EPROBE_DEFER,
>
> This is not an argument for a long time (since documentation of
> dev_err_probe() had been amended to encourage its use for any error
> cases in probe).
There was apparently a kernel doc update made in December 2023:
532888a59505 ("driver core: Better advertise dev_err_probe()")
to clarify that people are *allowed* to use it also for functions not
returning -EPROBE_DEFER. That's hardly a long time ago and, importantly,
this is of course still nothing that is *required*.
> > and that would be a separate
> > change in any case.
>
> Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?
This is not in any way technical debt.
Johan
Thu, May 09, 2024 at 10:49:28AM +0200, Johan Hovold kirjoitti:
> On Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote:
> > On Tue, May 7, 2024 at 6:01 PM Johan Hovold <[email protected]> wrote:
> > > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> > > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > > > > The regmap irq array is potentially shared between multiple PMICs and
..
> > > > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > > > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> > > >
> > > > dev_err_probe(...); ?
> > >
> > > This function won't return -EPROBE_DEFER,
> >
> > This is not an argument for a long time (since documentation of
> > dev_err_probe() had been amended to encourage its use for any error
> > cases in probe).
>
> There was apparently a kernel doc update made in December 2023:
>
> 532888a59505 ("driver core: Better advertise dev_err_probe()")
>
> to clarify that people are *allowed* to use it also for functions not
> returning -EPROBE_DEFER. That's hardly a long time ago and, importantly,
> this is of course still nothing that is *required*.
Fair enough.
> > > and that would be a separate
> > > change in any case.
> >
> > Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?
>
> This is not in any way technical debt.
OK.
--
With Best Regards,
Andy Shevchenko