Re: [PATCH v3 1/5] xen: dt: add dt_for_each_irq_map helper

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 20/04/15 13:16, Ian Campbell wrote:
> This function iterates over a nodes interrupt-map property and calls a
> callback for each interrupt. For now it only supplies the raw IRQ
> since my use case has no need of e.g. child unit address. These can be
> added as needed by any future users.
> 
> This follows much the same logic as dt_irq_map_raw when parsing the
> interrupt-map, but doesn't walk up the tree doing the actual
> translation and it iterates over all entries instead of just looking
> for the first match.
> 
> I looked into refactoring dt_irq_map_raw but I couldn't find a way
> which I was confident in, plus I was reluctant to diverge from the
> Linux roots of this function any further.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c      |  140 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h |   12 ++++
>  2 files changed, 152 insertions(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 02cae91..174cc1f 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -811,6 +811,146 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
>      return (psize / onesize);
>  }
>  
> +int dt_for_each_irq_map(const struct dt_device_node *dev,
> +                        int (*cb)(const struct dt_device_node *,
> +                                  const struct dt_raw_irq *,

Do you plan to have callers which require the raw IRQ? If not, I would
directly pass the translated IRQ (i.e dt_irq) to the callback.

> +                                  void *),
> +                        void *data)
> +{

[..]

The implementation looks good to me. Although, I didn't read closely.

> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 57eb3ee..91bd23a 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -528,6 +528,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,
>  int dt_irq_translate(const struct dt_raw_irq *raw, struct dt_irq *out_irq);
>  
>  /**
> + * dt_for_each_irq_map - Iterate over a nodes interrupt-map property
> + * @dev: The node whose interrupt-map property should be iterated over
> + * @cb: Call back to call for each entry
> + * @data: Caller data passed to callback
> + */
> +int dt_for_each_irq_map(const struct dt_device_node *dev,
> +                        int (*cb)(const struct dt_device_node *,
> +                                  const struct dt_raw_irq *,
> +                                  void *),

I would add a typedef for the callback.

> +                        void *data);
> +
> +/**
>   * dt_n_size_cells - Helper to retrieve the number of cell for the size
>   * @np: node to get the value
>   *
> 


-- 
Julien Grall