Re: [XEN][PATCH v9 03/19] xen/arm/device: Remove __init from function type

[Julien Grall <julien@xxxxxxx>]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Remove __init from following function to access during runtime:
>      1. map_irq_to_domain()
>      2. handle_device_interrupts()
>      3. map_range_to_domain()
>      4. unflatten_dt_node()
We are at v9 now, so this is more a remark for the future. In general we 
are trying to avoid modifying the same code multiple time within the 
series because this makes it more difficult to review. In this case, you 
could have removed the __init in patch #4 where you also export it.
> Move map_irq_to_domain() prototype from domain_build.h to setup.h.
>
> To avoid breaking the build, following changes are also done:
I am guessing by "breaking the build", you mean that you will get an 
error because an non-init function is implemented in domain_build.c. 
Right? If so, I think this should be spelled out.
> 1. Move map_irq_to_domain(), handle_device_interrupts() and 
> map_range_to_domain()
>      to device.c. After removing __init type,  these functions are not specific
>      to domain building, so moving them out of domain_build.c to device.c.
> 2. Remove static type from handle_device_interrupt().
Typo: s/interrupt/interrupts/ to match the function name. But I don't 
link the name when it is exported as it leads to think this is dealing 
with real interrupt.
Looking at the overlay code, the caller (i.e. add_resources()) is very 
similar to handle_device(). AFAICT the only differences are:
   1/ add_resources() doesn't deal with mapping (you said it will in 
the future)
   2/ You need to update some rangeset

For 1/ it means this is a superset. For 2/, I think this could be 
abstracted by adding a pointer to the rangesets in map_range_data. They 
could be NULL for the domain build case.
So can you look at abstracting? This will make easier to maintain a 
single place to parse a device node and map it.
A possible name for the function would be dt_map_resources_to_domain().

Cheers,

--
Julien Grall