Re: [Minios-devel] [UNIKRAFT PATCHv4 8/9] plat/common: Introduce fdt_get_interrupt helper

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Jia he,

Please find the comment inline

Thanks & Regards
Sharan

On 3/27/19 3:34 AM, Jia He wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
> This helper will be used very frequently for devices to
> get their interrupts.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   plat/common/fdt.c         | 29 +++++++++++++++++++++++++++++
>   plat/common/include/fdt.h | 13 +++++++++++++
>   2 files changed, 42 insertions(+)
>
> diff --git a/plat/common/fdt.c b/plat/common/fdt.c
> index 1707ef2..931c7de 100644
> --- a/plat/common/fdt.c
> +++ b/plat/common/fdt.c
> @@ -342,3 +342,32 @@ int fdt_node_offset_by_compatible_list(const void *fdt, 
> int startoffset,
>   
>   	return FDT_ERR_NOTFOUND;
>   }
> +
Is there a reason why the APIs for fdt_get_interrupt is different from 
fdt_get_address? Here we return the address as a pointer to address and 
NULL for error and in other case we return as a reference to the addr 
parameter and return error code. Can we make these function API 
consistent? I prefer the one with error code.
> +const void *fdt_get_interrupt(const void *fdt, int nodeoffset,
> +                       int index, int *size)
> +{
> +       int nintr, len, term_size;
> +       const void *regs;
> +
UK_ASSERT(size);

> +       nintr = fdt_interrupt_cells(fdt, nodeoffset);
> +       if (nintr < 0 || nintr >= FDT_MAX_NCELLS)
> +               return NULL;
> +
> +       /* "interrupts-extended" is not supported */
> +       regs = fdt_getprop(fdt, nodeoffset, "interrupts-extended", &len);
> +       if (regs) {
> +               uk_pr_warn("interrupts multiple parents is not supported\n");
> +               return NULL;
> +       }
> +       /*
> +        * Interrupt content must cover the index specific irq information.
> +        */
> +       regs = fdt_getprop(fdt, nodeoffset, "interrupts", &len);
> +       term_size = sizeof(fdt32_t) * nintr;
> +       if (regs == NULL || len < term_size * (index + 1))
> +               return NULL;
> +
> +       *size = nintr;
> +
> +       return regs + term_size * index;
> +}
> diff --git a/plat/common/include/fdt.h b/plat/common/include/fdt.h
> index be6b265..3490f71 100644
> --- a/plat/common/include/fdt.h
> +++ b/plat/common/include/fdt.h
> @@ -154,4 +154,17 @@ int fdt_get_address(const void *fdt, int nodeoffset, int 
> index,
>   int fdt_node_offset_by_compatible_list(const void *fdt, int startoffset,
>                                         const char *compatibles[]);
>   
> +/**
> + * fdt_get_interrupt - retrieve device interrupt of a given index
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node to find the address for
> + * @index: the index of interrupt we want to retrieve
> + * @size: interrupt cell size in fdt32_t
> + *
> + * returns:
> + *      NULL on failed, non-NULL on success
> + */
> +const void *fdt_get_interrupt(const void *fdt, int nodeoffset,
> +                               int index, int *size);
> +
>   #endif
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel