| Message ID | 20240506150830.23709-3-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic | expand |
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(...); ?
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 <johan@kernel.org> 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?
On Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote: > On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> 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 <johan@kernel.org> 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.
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);
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 <johan+linaro@kernel.org> --- drivers/mfd/qcom-pm8008.c | 66 +++++++++++++++------------------------ 1 file changed, 25 insertions(+), 41 deletions(-)