| Message ID | 20221009064431.18839-2-lvjianmin@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip: Support to set irq type for ACPI path | expand |
In the subject and the commit log below, figure out whether you want to use "irq" or "IRQ" and do it consistently. I vote for "IRQ". Also consider subject lines for the other patches. Stuff like this is trivial, but it's an excuse for whoever will handle this (not me in this case) to put it off. I also add "()" after function names for clarity. On Sun, Oct 09, 2022 at 02:44:28PM +0800, Jianmin Lv wrote: > On LoongArch ACPI based systems, the irq trigger type of PCI devices > is high level, so high level triggered type is required to pass > to acpi_register_gsi when create irq mapping for PCI devices. This isn't worded quite right. The trigger type of PCI devices doesn't change just because you plug them into a LoongArch system instead of an x86 system. The comment in the code reads better: The IRQs are active low from the perspective of PCI, but are inverted before the interrupt controller. Including a reference here to the spec that mentions this inversion would help a lot. s/when create mapping/when creating mappings/ > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> > --- > drivers/acpi/pci_irq.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > index 08e15774fb9f..ff30ceca2203 100644 > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -387,13 +387,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > u8 pin; > int triggering = ACPI_LEVEL_SENSITIVE; > /* > - * On ARM systems with the GIC interrupt model, level interrupts > + * On ARM systems with the GIC interrupt model, or LoongArch > + * systems with the LPIC interrupt model, level interrupts > * are always polarity high by specification; PCI legacy > * IRQs lines are inverted before reaching the interrupt > * controller and must therefore be considered active high > * as default. > */ > - int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC || > + acpi_irq_model == ACPI_IRQ_MODEL_LPIC ? > ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > char *link = NULL; > char link_desc[16]; > -- > 2.31.1 >
Ok, thanks, I'll improve them as your suggestion. On 2022/10/20 上午12:05, Bjorn Helgaas wrote: > In the subject and the commit log below, figure out whether you want > to use "irq" or "IRQ" and do it consistently. I vote for "IRQ". Also > consider subject lines for the other patches. Stuff like this is > trivial, but it's an excuse for whoever will handle this (not me in > this case) to put it off. I also add "()" after function names for > clarity. > > On Sun, Oct 09, 2022 at 02:44:28PM +0800, Jianmin Lv wrote: >> On LoongArch ACPI based systems, the irq trigger type of PCI devices >> is high level, so high level triggered type is required to pass >> to acpi_register_gsi when create irq mapping for PCI devices. > > This isn't worded quite right. The trigger type of PCI devices > doesn't change just because you plug them into a LoongArch system > instead of an x86 system. The comment in the code reads better: The > IRQs are active low from the perspective of PCI, but are inverted > before the interrupt controller. > > Including a reference here to the spec that mentions this inversion > would help a lot. > > s/when create mapping/when creating mappings/ > >> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> >> --- >> drivers/acpi/pci_irq.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c >> index 08e15774fb9f..ff30ceca2203 100644 >> --- a/drivers/acpi/pci_irq.c >> +++ b/drivers/acpi/pci_irq.c >> @@ -387,13 +387,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev) >> u8 pin; >> int triggering = ACPI_LEVEL_SENSITIVE; >> /* >> - * On ARM systems with the GIC interrupt model, level interrupts >> + * On ARM systems with the GIC interrupt model, or LoongArch >> + * systems with the LPIC interrupt model, level interrupts >> * are always polarity high by specification; PCI legacy >> * IRQs lines are inverted before reaching the interrupt >> * controller and must therefore be considered active high >> * as default. >> */ >> - int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? >> + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC || >> + acpi_irq_model == ACPI_IRQ_MODEL_LPIC ? >> ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; >> char *link = NULL; >> char link_desc[16]; >> -- >> 2.31.1 >>
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index 08e15774fb9f..ff30ceca2203 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -387,13 +387,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev) u8 pin; int triggering = ACPI_LEVEL_SENSITIVE; /* - * On ARM systems with the GIC interrupt model, level interrupts + * On ARM systems with the GIC interrupt model, or LoongArch + * systems with the LPIC interrupt model, level interrupts * are always polarity high by specification; PCI legacy * IRQs lines are inverted before reaching the interrupt * controller and must therefore be considered active high * as default. */ - int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC || + acpi_irq_model == ACPI_IRQ_MODEL_LPIC ? ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; char *link = NULL; char link_desc[16];
On LoongArch ACPI based systems, the irq trigger type of PCI devices is high level, so high level triggered type is required to pass to acpi_register_gsi when create irq mapping for PCI devices. Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> --- drivers/acpi/pci_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)