Re: [PATCH 11/24] RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support

Conor Dooley
2023-02-08T21:49:49Z

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

Hey Sunil,

On Mon, Jan 30, 2023 at 11:52:12PM +0530, Sunil V L wrote:
> Add support for initializing the RISC-V INTC driver on ACPI based
> platforms.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

> +static int __init
> +riscv_intc_acpi_init(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + int rc;
> + struct fwnode_handle *fn;
> + struct acpi_madt_rintc *rintc;
> +
> + rintc = (struct acpi_madt_rintc *)header;
> +
> + /*
> + * The ACPI MADT will have one INTC for each CPU (or HART)
> + * so riscv_intc_acpi_init() function will be called once
> + * for each INTC. We only need to do INTC initialization
> + * for the INTC belonging to the boot CPU (or boot HART).
> + */
> + if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
> + return 0;
> +
> + fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
> + WARN_ON(fn == NULL);

Is there a reason that you do not just check this as !fn?

> + if (!fn) {

This is a repeated check from the WARN_ON(), no?

> + pr_err("unable to allocate INTC FW node\n");

Why do you need a WARN_ON() & the pr_err() here?

> + return -1;

Why not an actual ERRNO?

Cheers,
Conor.

> + }
> +
> + rc = riscv_intc_init_common(fn);
> + if (rc) {
> + pr_err("failed to initialize INTC\n");
> + return rc;
> + }
>
> return 0;
> }
>
> -IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> +IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC,
> + NULL, 1, riscv_intc_acpi_init);
> +#endif
> --
> 2.38.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]