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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-04-29 at 17:39 +0100, Julien Grall wrote:
> 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@xxxxxxxxxx>
> > ---
> >  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.

Sure.

> 
> > +                                  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.

It's only used in this one place, so I don't think it's really needed
and the obvious name dt_for_each_irq_map_cb is super clumsy.

Also you didn't say the same thing to the cb in the next patch ;-).

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel