Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 19 Oct 2022 11:05:29 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Jianmin Lv <lvjianmin@loongson.cn>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>, linux-kernel@vger.kernel.org,
        loongarch@lists.linux.dev, Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Huacai Chen <chenhuacai@loongson.cn>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Len Brown <lenb@kernel.org>, rafael@kernel.org,
        linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH V3 1/4] ACPI / PCI: fix LPIC irq model default PCI IRQ
 polarity
Message-ID: <20221019160529.GA18372@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221009064431.18839-2-lvjianmin@loongson.cn>
Precedence: bulk

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
>