| Message ID | 1548839891-20932-1-git-send-email-hsin-hsiung.wang@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Support for MediaTek PMIC MT6358 MFD Core and Regulator | expand |
Hi Pi-Hsun, On Thu, 2019-01-31 at 11:56 +0800, Pi-Hsun Shih wrote: > On Wed, Jan 30, 2019 at 5:19 PM Hsin-Hsiung Wang > <hsin-hsiung.wang@mediatek.com> wrote: > > > > This adds support for the MediaTek MT6358 PMIC. This is a > > multifunction device with the following sub modules: > > > > - Regulator > > - RTC > > - Codec > > - Interrupt ... > > +static const struct mfd_cell mt6358_devs[] = { > > + { > > + .name = "mt6358-regulator", > > + .of_compatible = "mediatek,mt6358-regulator" > > + }, { > > + .name = "mt6397-rtc", > > Should this be "mt6358-rtc"? Because MT6358 rtc uses the same rtc driver as MT6397, the name should be "mt6397-rtc" for rtc driver probe. Thanks a lot. > > + .num_resources = ARRAY_SIZE(mt6358_rtc_resources), > > + .resources = mt6358_rtc_resources, > > + .of_compatible = "mediatek,mt6358-rtc", > > + }, { > > + .name = "mt6358-sound", > > + .of_compatible = "mediatek,mt6358-sound" > > + }, > > +}; ... > > -- > > 1.9.1 > >
On Thu, 31 Jan 2019, Pi-Hsun Shih wrote: > On Wed, Jan 30, 2019 at 5:19 PM Hsin-Hsiung Wang > <hsin-hsiung.wang@mediatek.com> wrote: > > > > This adds support for the MediaTek MT6358 PMIC. This is a > > multifunction device with the following sub modules: > > > > - Regulator > > - RTC > > - Codec > > - Interrupt > > > > It is interfaced to the host controller using SPI interface > > by a proprietary hardware called PMIC wrapper or pwrap. > > MT6358 MFD is a child device of the pwrap. > > > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com> > > --- > > drivers/mfd/Makefile | 2 +- > > drivers/mfd/mt6358-irq.c | 236 +++++ > > drivers/mfd/mt6397-core.c | 62 +- > > include/linux/mfd/mt6358/core.h | 158 +++ > > include/linux/mfd/mt6358/registers.h | 1926 ++++++++++++++++++++++++++++++++++ > > include/linux/mfd/mt6397/core.h | 3 + > > 6 files changed, 2385 insertions(+), 2 deletions(-) > > create mode 100644 drivers/mfd/mt6358-irq.c > > create mode 100644 include/linux/mfd/mt6358/core.h > > create mode 100644 include/linux/mfd/mt6358/registers.h [100's (hundreds) of lines of code cut] > > +static const struct mfd_cell mt6358_devs[] = { > > + { > > + .name = "mt6358-regulator", > > + .of_compatible = "mediatek,mt6358-regulator" > > + }, { > > + .name = "mt6397-rtc", > > Should this be "mt6358-rtc"? [1000's (thousands) of lines of code cut] When replying to a patch, especially one as large as this, you should cut all of the irrelevant quotes. Put another way, you should only quote what you are replying to, and maybe a small section before/after to keep the context.
Hi Lee, On 07/02/2019 09:34, Lee Jones wrote: > Thomas, et al., > > Please could you take a look at this? > > I need some IRQ related guidance. TIA. > > On Wed, 30 Jan 2019, Hsin-Hsiung Wang wrote: > >> This adds support for the MediaTek MT6358 PMIC. This is a >> multifunction device with the following sub modules: >> >> - Regulator >> - RTC >> - Codec >> - Interrupt >> >> It is interfaced to the host controller using SPI interface >> by a proprietary hardware called PMIC wrapper or pwrap. >> MT6358 MFD is a child device of the pwrap. >> >> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com> >> --- >> drivers/mfd/Makefile | 2 +- >> drivers/mfd/mt6358-irq.c | 236 +++++ >> drivers/mfd/mt6397-core.c | 62 +- >> include/linux/mfd/mt6358/core.h | 158 +++ >> include/linux/mfd/mt6358/registers.h | 1926 ++++++++++++++++++++++++++++++++++ >> include/linux/mfd/mt6397/core.h | 3 + >> 6 files changed, 2385 insertions(+), 2 deletions(-) >> create mode 100644 drivers/mfd/mt6358-irq.c >> create mode 100644 include/linux/mfd/mt6358/core.h >> create mode 100644 include/linux/mfd/mt6358/registers.h >> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 088e249..50be021 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -230,7 +230,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o >> obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o >> obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o >> obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o >> -obj-$(CONFIG_MFD_MT6397) += mt6397-core.o mt6397-irq.o >> +obj-$(CONFIG_MFD_MT6397) += mt6397-core.o mt6397-irq.o mt6358-irq.o >> >> obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o >> obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o >> diff --git a/drivers/mfd/mt6358-irq.c b/drivers/mfd/mt6358-irq.c >> new file mode 100644 >> index 0000000..b29fdc1 >> --- /dev/null >> +++ b/drivers/mfd/mt6358-irq.c >> @@ -0,0 +1,236 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// >> +// Copyright (c) 2019 MediaTek Inc. >> + >> +#include <linux/interrupt.h> >> +#include <linux/mfd/mt6358/core.h> >> +#include <linux/mfd/mt6358/registers.h> >> +#include <linux/mfd/mt6397/core.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/of_irq.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +static struct irq_top_t mt6358_ints[] = { >> + MT6358_TOP_GEN(BUCK), >> + MT6358_TOP_GEN(LDO), >> + MT6358_TOP_GEN(PSC), >> + MT6358_TOP_GEN(SCK), >> + MT6358_TOP_GEN(BM), >> + MT6358_TOP_GEN(HK), >> + MT6358_TOP_GEN(AUD), >> + MT6358_TOP_GEN(MISC), >> +}; > > What is a 'top' IRQ? > >> +static int parsing_hwirq_to_top_group(unsigned int hwirq) >> +{ >> + int top_group; >> + >> + for (top_group = 1; top_group < ARRAY_SIZE(mt6358_ints); top_group++) { >> + if (mt6358_ints[top_group].hwirq_base > hwirq) { >> + top_group--; >> + break; >> + } >> + } >> + return top_group; >> +} > > This function is going to need some comments. Why do you start at LDO > instead of the top entry, BUCK? It also begs the question: why isn't that directly associated to the irq_data structure? Something is fishy here. In general, if you have to iterate over anything, you're likely to be doing the wrong thing. > >> +static void pmic_irq_enable(struct irq_data *data) >> +{ >> + unsigned int hwirq = irqd_to_hwirq(data); >> + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); >> + struct pmic_irq_data *irq_data = chip->irq_data; >> + >> + irq_data->enable_hwirq[hwirq] = 1; >> +} > > I see that you're doing your own caching operations. Is that > required? I think I'm going to stop here and as for some IRQ guy's > input on this. Dunno either. I thought that's what regmap was for? > >> +static void pmic_irq_disable(struct irq_data *data) >> +{ >> + unsigned int hwirq = irqd_to_hwirq(data); >> + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); >> + struct pmic_irq_data *irq_data = chip->irq_data; >> + >> + irq_data->enable_hwirq[hwirq] = 0; >> +} >> + >> +static void pmic_irq_lock(struct irq_data *data) >> +{ >> + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); >> + >> + mutex_lock(&chip->irqlock); >> +} >> + >> +static void pmic_irq_sync_unlock(struct irq_data *data) >> +{ >> + unsigned int i, top_gp, en_reg, int_regs, shift; >> + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); >> + struct pmic_irq_data *irq_data = chip->irq_data; I really wish you kept the symbol "irq_data" for something that is a struct irq_data. This is making the code pointlessly obfuscated. >> + >> + for (i = 0; i < irq_data->num_pmic_irqs; i++) { >> + if (irq_data->enable_hwirq[i] == >> + irq_data->cache_hwirq[i]) >> + continue; Please explain what you are trying to do here. The unlock operation is supposed to affect exactly one interrupt. Instead, you seem to deal with a bunch of them at once. Operations are supposed to happen on the "leaf" IRQs, not on the multiplexing interrupt. Also, the whole cache thing seems pretty pointless. Why isn't regmap doing that for you? >> + >> + top_gp = parsing_hwirq_to_top_group(i); >> + int_regs = mt6358_ints[top_gp].num_int_bits / MT6358_REG_WIDTH; >> + en_reg = mt6358_ints[top_gp].en_reg + >> + mt6358_ints[top_gp].en_reg_shift * int_regs; >> + shift = (i - mt6358_ints[top_gp].hwirq_base) % MT6358_REG_WIDTH; >> + regmap_update_bits(chip->regmap, en_reg, 0x1 << shift, >> + irq_data->enable_hwirq[i] << shift); >> + irq_data->cache_hwirq[i] = irq_data->enable_hwirq[i]; >> + } >> + mutex_unlock(&chip->irqlock); >> +} >> + >> +static int pmic_irq_set_type(struct irq_data *data, unsigned int type) >> +{ >> + return 0; >> +} >> + >> +static struct irq_chip mt6358_irq_chip = { >> + .name = "mt6358-irq", >> + .irq_enable = pmic_irq_enable, >> + .irq_disable = pmic_irq_disable, >> + .irq_bus_lock = pmic_irq_lock, >> + .irq_bus_sync_unlock = pmic_irq_sync_unlock, >> + .irq_set_type = pmic_irq_set_type, >> +}; >> + >> +static void mt6358_irq_sp_handler(struct mt6397_chip *chip, >> + unsigned int top_gp) >> +{ >> + unsigned int sta_reg, int_status = 0; >> + unsigned int hwirq, virq; >> + int ret, i, j; >> + >> + for (i = 0; i < mt6358_ints[top_gp].num_int_regs; i++) { >> + sta_reg = mt6358_ints[top_gp].sta_reg + >> + mt6358_ints[top_gp].sta_reg_shift * i; >> + ret = regmap_read(chip->regmap, sta_reg, &int_status); >> + if (ret) { >> + dev_err(chip->dev, >> + "Failed to read irq status: %d\n", ret); >> + return; >> + } >> + >> + if (!int_status) >> + continue; >> + >> + for (j = 0; j < 16 ; j++) { >> + if ((int_status & BIT(j)) == 0) >> + continue; >> + hwirq = mt6358_ints[top_gp].hwirq_base + >> + MT6358_REG_WIDTH * i + j; >> + virq = irq_find_mapping(chip->irq_domain, hwirq); >> + if (virq) >> + handle_nested_irq(virq); >> + } >> + >> + regmap_write(chip->regmap, sta_reg, int_status); >> + } >> +} >> + >> +static irqreturn_t mt6358_irq_handler(int irq, void *data) >> +{ >> + struct mt6397_chip *chip = data; >> + struct pmic_irq_data *mt6358_irq_data = chip->irq_data; >> + unsigned int top_int_status = 0; >> + unsigned int i; >> + int ret; >> + >> + ret = regmap_read(chip->regmap, >> + mt6358_irq_data->top_int_status_reg, >> + &top_int_status); >> + if (ret) { >> + dev_err(chip->dev, "Can't read TOP_INT_STATUS ret=%d\n", ret); >> + return IRQ_NONE; >> + } >> + >> + for (i = 0; i < mt6358_irq_data->num_top; i++) { >> + if (top_int_status & BIT(mt6358_ints[i].top_offset)) >> + mt6358_irq_sp_handler(chip, i); >> + } >> + >> + return IRQ_HANDLED; >> +} Why isn't this a normal chained irq flow, instead of a homegrown irq handler? Is that because this is a threaded handler? >> + >> +static int pmic_irq_domain_map(struct irq_domain *d, unsigned int irq, >> + irq_hw_number_t hw) >> +{ >> + struct mt6397_chip *mt6397 = d->host_data; >> + >> + irq_set_chip_data(irq, mt6397); >> + irq_set_chip_and_handler(irq, &mt6358_irq_chip, handle_level_irq); >> + irq_set_nested_thread(irq, 1); >> + irq_set_noprobe(irq); >> + >> + return 0; >> +} >> + >> +static const struct irq_domain_ops mt6358_irq_domain_ops = { >> + .map = pmic_irq_domain_map, >> + .xlate = irq_domain_xlate_twocell, >> +}; >> + >> +int mt6358_irq_init(struct mt6397_chip *chip) >> +{ >> + int i, j, ret; >> + struct pmic_irq_data *irq_data; >> + >> + irq_data = devm_kzalloc(chip->dev, sizeof(struct pmic_irq_data *), >> + GFP_KERNEL); >> + if (!irq_data) >> + return -ENOMEM; >> + >> + chip->irq_data = irq_data; >> + >> + mutex_init(&chip->irqlock); >> + irq_data->top_int_status_reg = MT6358_TOP_INT_STATUS0; >> + irq_data->num_pmic_irqs = MT6358_IRQ_NR; >> + irq_data->num_top = ARRAY_SIZE(mt6358_ints); >> + >> + irq_data->enable_hwirq = devm_kcalloc(chip->dev, >> + irq_data->num_pmic_irqs, >> + sizeof(unsigned int), >> + GFP_KERNEL); >> + if (!irq_data->enable_hwirq) >> + return -ENOMEM; >> + >> + irq_data->cache_hwirq = devm_kcalloc(chip->dev, >> + irq_data->num_pmic_irqs, >> + sizeof(unsigned int), >> + GFP_KERNEL); >> + if (!irq_data->cache_hwirq) >> + return -ENOMEM; >> + >> + /* Disable all interrupt for initializing */ >> + for (i = 0; i < irq_data->num_top; i++) { >> + for (j = 0; j < mt6358_ints[i].num_int_regs; j++) >> + regmap_write(chip->regmap, >> + mt6358_ints[i].en_reg + >> + mt6358_ints[i].en_reg_shift * j, 0); >> + } >> + >> + chip->irq_domain = irq_domain_add_linear(chip->dev->of_node, >> + irq_data->num_pmic_irqs, >> + &mt6358_irq_domain_ops, chip); >> + if (!chip->irq_domain) { >> + dev_err(chip->dev, "could not create irq domain\n"); >> + return -ENODEV; >> + } >> + >> + ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, >> + mt6358_irq_handler, IRQF_ONESHOT, >> + mt6358_irq_chip.name, chip); >> + if (ret) { >> + dev_err(chip->dev, "failed to register irq=%d; err: %d\n", >> + chip->irq, ret); >> + return ret; >> + } >> + >> + enable_irq_wake(chip->irq); Why is that decided at probe time, from kernel space? >> + return ret; >> +} Anyway, I've stopped here. This definitely needs clarification. Thanks, M.
On Thu, Feb 07, 2019 at 10:04:50AM +0000, Marc Zyngier wrote: > On 07/02/2019 09:34, Lee Jones wrote: > >> +static struct irq_top_t mt6358_ints[] = { > >> + MT6358_TOP_GEN(BUCK), > >> + MT6358_TOP_GEN(LDO), > >> + MT6358_TOP_GEN(PSC), > >> + MT6358_TOP_GEN(SCK), > >> + MT6358_TOP_GEN(BM), > >> + MT6358_TOP_GEN(HK), > >> + MT6358_TOP_GEN(AUD), > >> + MT6358_TOP_GEN(MISC), > >> +}; > > What is a 'top' IRQ? It looks like it's an intermediate parent IRQ controller; that's quite a common design for MFDs on slow buses to cut down on the number of status registers to poll. IIRC there at least used to be some framework reason for not using normal chained interrupts for these but I can't remember it any more - possibly something to do with threaded handlers. This is all looking very famililar, I suspect it's based on other drivers dating back years rather than doing anything original. There's certainly a bunch of other drivers with very similar patterns in tree, this doesn't look like it's got any problems over what most similar drivers are doing. The patterns all predate drivers/irqchip and a lot of them will come from me. > >> +static void pmic_irq_enable(struct irq_data *data) > >> +{ > >> + unsigned int hwirq = irqd_to_hwirq(data); > >> + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); > >> + struct pmic_irq_data *irq_data = chip->irq_data; > >> + > >> + irq_data->enable_hwirq[hwirq] = 1; > >> +} > > I see that you're doing your own caching operations. Is that > > required? I think I'm going to stop here and as for some IRQ guy's > > input on this. > Dunno either. I thought that's what regmap was for? IIRC from when I wrote drivers for chips like this these operations are called with interrupts disabled so you can't write to interrupt driven buses so you need to cache the write and flush in sync_unlock(). You can't do this with regmap as it stands since on a device that can't be accessed with interrupts disabled you'd need to disable the writes before going into the interrupts off section and there's a risk that having the device cache disabled would disrupt some other function on the chip that was expecting writes to get posted immediately. It would be possible to add per-register cache only behaviour but someone would need to do that and it's not clear that it's worth the effort, especially since for slow buses we currently lock the entire regmap including the cache with mutexes so you can't actually access the cache with interrupts off at the minute. > >> + for (i = 0; i < irq_data->num_pmic_irqs; i++) { > >> + if (irq_data->enable_hwirq[i] == > >> + irq_data->cache_hwirq[i]) > >> + continue; > Please explain what you are trying to do here. The unlock operation is > supposed to affect exactly one interrupt. Instead, you seem to deal with > a bunch of them at once. Operations are supposed to happen on the "leaf" > IRQs, not on the multiplexing interrupt. IIRC it was done this way because it wasn't altogether clear if operations on multiple interrupts could ever be batched or not and given that we're dealing with slow buses the cost of the loop is negligable. > Also, the whole cache thing seems pretty pointless. Why isn't regmap > doing that for you? See above. > >> + for (i = 0; i < mt6358_irq_data->num_top; i++) { > >> + if (top_int_status & BIT(mt6358_ints[i].top_offset)) > >> + mt6358_irq_sp_handler(chip, i); > >> + } > >> + > >> + return IRQ_HANDLED; > >> +} > Why isn't this a normal chained irq flow, instead of a homegrown irq > handler? Is that because this is a threaded handler? I think that's it but like I say I can't remember clearly any more, it's been a decade. > >> + ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, > >> + mt6358_irq_handler, IRQF_ONESHOT, > >> + mt6358_irq_chip.name, chip); > >> + if (ret) { > >> + dev_err(chip->dev, "failed to register irq=%d; err: %d\n", > >> + chip->irq, ret); > >> + return ret; > >> + } > >> + > >> + enable_irq_wake(chip->irq); > Why is that decided at probe time, from kernel space? IIRC it's due to it being the main interrupt for the device and there being at some point issues with this getting propagated to parent interrupts so wake just got turned on all the time for the parent and we relied on controlling the children. Making things be proper chained interrupt controllers would solve that I think.
Hi, A few comments inline. On a general note I agree with others that this code would benefit from more comments. On Wed, Jan 30, 2019 at 05:18:09PM +0800, Hsin-Hsiung Wang wrote: > This adds support for the MediaTek MT6358 PMIC. This is a > multifunction device with the following sub modules: > > - Regulator > - RTC > - Codec > - Interrupt > > It is interfaced to the host controller using SPI interface > by a proprietary hardware called PMIC wrapper or pwrap. > MT6358 MFD is a child device of the pwrap. > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com> > --- > drivers/mfd/Makefile | 2 +- > drivers/mfd/mt6358-irq.c | 236 +++++ > drivers/mfd/mt6397-core.c | 62 +- > include/linux/mfd/mt6358/core.h | 158 +++ > include/linux/mfd/mt6358/registers.h | 1926 ++++++++++++++++++++++++++++++++++ > include/linux/mfd/mt6397/core.h | 3 + > 6 files changed, 2385 insertions(+), 2 deletions(-) > create mode 100644 drivers/mfd/mt6358-irq.c > create mode 100644 include/linux/mfd/mt6358/core.h > create mode 100644 include/linux/mfd/mt6358/registers.h > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 088e249..50be021 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -230,7 +230,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > -obj-$(CONFIG_MFD_MT6397) += mt6397-core.o mt6397-irq.o > +obj-$(CONFIG_MFD_MT6397) += mt6397-core.o mt6397-irq.o mt6358-irq.o > > obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o > obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o > diff --git a/drivers/mfd/mt6358-irq.c b/drivers/mfd/mt6358-irq.c > new file mode 100644 > index 0000000..b29fdc1 > --- /dev/null > +++ b/drivers/mfd/mt6358-irq.c > @@ -0,0 +1,236 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Copyright (c) 2019 MediaTek Inc. > + > +#include <linux/interrupt.h> > +#include <linux/mfd/mt6358/core.h> > +#include <linux/mfd/mt6358/registers.h> > +#include <linux/mfd/mt6397/core.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +static struct irq_top_t mt6358_ints[] = { > + MT6358_TOP_GEN(BUCK), > + MT6358_TOP_GEN(LDO), > + MT6358_TOP_GEN(PSC), > + MT6358_TOP_GEN(SCK), > + MT6358_TOP_GEN(BM), > + MT6358_TOP_GEN(HK), > + MT6358_TOP_GEN(AUD), > + MT6358_TOP_GEN(MISC), > +}; > + > +static int parsing_hwirq_to_top_group(unsigned int hwirq) why 'parsing'? IIUC the 'top's are interrupt groups for different functionalities. Could we get rid of the 'top' terminology in most of the code and just call it irq_grp or similar? Would be less obfuscated IMO. > +{ > + int top_group; > + > + for (top_group = 1; top_group < ARRAY_SIZE(mt6358_ints); top_group++) { > + if (mt6358_ints[top_group].hwirq_base > hwirq) { > + top_group--; > + break; > + } > + } > + return top_group; > +} > + > +static void pmic_irq_enable(struct irq_data *data) > +{ > + unsigned int hwirq = irqd_to_hwirq(data); > + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); > + struct pmic_irq_data *irq_data = chip->irq_data; > + > + irq_data->enable_hwirq[hwirq] = 1; enable_hwirq should be boolean or - probably better - a bitmap (less memory usage and no need for dynamic allocation). > +static void pmic_irq_sync_unlock(struct irq_data *data) > +{ > + unsigned int i, top_gp, en_reg, int_regs, shift; > + struct mt6397_chip *chip = irq_data_get_irq_chip_data(data); > + struct pmic_irq_data *irq_data = chip->irq_data; > + > + for (i = 0; i < irq_data->num_pmic_irqs; i++) { > + if (irq_data->enable_hwirq[i] == > + irq_data->cache_hwirq[i]) > + continue; > + > + top_gp = parsing_hwirq_to_top_group(i); > + int_regs = mt6358_ints[top_gp].num_int_bits / MT6358_REG_WIDTH; > + en_reg = mt6358_ints[top_gp].en_reg + > + mt6358_ints[top_gp].en_reg_shift * int_regs; > + shift = (i - mt6358_ints[top_gp].hwirq_base) % MT6358_REG_WIDTH; > + regmap_update_bits(chip->regmap, en_reg, 0x1 << shift, use BIT() > +static void mt6358_irq_sp_handler(struct mt6397_chip *chip, > + unsigned int top_gp) > +{ > + unsigned int sta_reg, int_status = 0; initialization of int_status not needed. nit: consider changing 'int_status' to just 'status' or 'irq_status' > + unsigned int hwirq, virq; > + int ret, i, j; > + > + for (i = 0; i < mt6358_ints[top_gp].num_int_regs; i++) { > + sta_reg = mt6358_ints[top_gp].sta_reg + > + mt6358_ints[top_gp].sta_reg_shift * i; > + ret = regmap_read(chip->regmap, sta_reg, &int_status); > + if (ret) { > + dev_err(chip->dev, > + "Failed to read irq status: %d\n", ret); > + return; > + } > + > + if (!int_status) > + continue; > + > + for (j = 0; j < 16 ; j++) { s/16/MT6358_REG_WIDTH/ > + if ((int_status & BIT(j)) == 0) if (!(int_status & BIT(j))) > + continue; > + hwirq = mt6358_ints[top_gp].hwirq_base + > + MT6358_REG_WIDTH * i + j; > + virq = irq_find_mapping(chip->irq_domain, hwirq); > + if (virq) > + handle_nested_irq(virq); > + } > + > + regmap_write(chip->regmap, sta_reg, int_status); > + } > +} > + > +static irqreturn_t mt6358_irq_handler(int irq, void *data) > +{ > + struct mt6397_chip *chip = data; > + struct pmic_irq_data *mt6358_irq_data = chip->irq_data; > + unsigned int top_int_status = 0; initialization not needed > + unsigned int i; > + int ret; > + > + ret = regmap_read(chip->regmap, > + mt6358_irq_data->top_int_status_reg, > + &top_int_status); > + if (ret) { > + dev_err(chip->dev, "Can't read TOP_INT_STATUS ret=%d\n", ret); > + return IRQ_NONE; > + } > + > + for (i = 0; i < mt6358_irq_data->num_top; i++) { > + if (top_int_status & BIT(mt6358_ints[i].top_offset)) > + mt6358_irq_sp_handler(chip, i); > + } > + > + return IRQ_HANDLED; > +} Cheers Matthias