Re: [PATCH v3 01/11] RISC-V: Add riscv_fw_parent_hartid() function

[Anup Patel]

On Wed, May 10, 2023 at 6:15 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> On Mon, May 08, 2023 at 07:58:32PM +0530, Anup Patel wrote:
> > We add common riscv_fw_parent_hartid() which help device drivers
> > to get parent hartid of the INTC (i.e. local interrupt controller)
> > fwnode. Currently, this new function only supports device tree
> > but it can be extended to support ACPI as well.
> >
> > Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/processor.h |  3 +++
> >  arch/riscv/kernel/cpu.c            | 12 ++++++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index 94a0590c6971..6fb8bbec8459 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -77,6 +77,9 @@ struct device_node;
> >  int riscv_of_processor_hartid(struct device_node *node, unsigned long *hartid);
> >  int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid);
> >
> > +struct fwnode_handle;
> > +int riscv_fw_parent_hartid(struct fwnode_handle *node, unsigned long *hartid);
> > +
> >  extern void riscv_fill_hwcap(void);
> >  extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
> >
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 5de6fb703cc2..1adbe48b2b58 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -73,6 +73,18 @@ int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid)
> >       return -1;
> >  }
> >
> > +/* Find hart ID of the CPU fwnode under which given fwnode falls. */
> > +int riscv_fw_parent_hartid(struct fwnode_handle *node, unsigned long *hartid)
> > +{
> > +     /*
> > +      * Currently, this function only supports DT but it can be
> > +      * extended to support ACPI as well.
> > +      */
>
> Statement of the obvious here, no?
> Although, it seems a little odd to read this comment & the corresponding
> statement in the commit message, when the series appears to have been
> based on the ACPI?
>
> Perhaps by the time v4 comes around, ACPI support will have been merged
> & that'll be moot.

Yes, I was anyway going to update this in v4 to support both DT and ACPI.

>
> > +     if (!is_of_node(node))
> > +             return -EINVAL;
> > +     return riscv_of_parent_hartid(to_of_node(node), hartid);
>
> nit: blank line before the return here please.

Okay, I will update.

>
> Thanks,
> Conor.

Regards,
Anup