| Message ID | 1384853593-32202-2-git-send-email-hdoyu@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > The following pattern of code is tempting: > > for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> That's a very minimal commit message. Can you elaborate please. > --- > v5: > New patch for v5. > --- > include/linux/of.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/of.h b/include/linux/of.h > index 276c546..131fef5 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -613,6 +613,9 @@ static inline int of_property_read_u32(const struct device_node *np, > s; \ > s = of_prop_next_string(prop, s)) > > +#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \ > + for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) > + That works, but pretty darn inefficient. We need an iterator version of of_parse_phandle_with_args() to loop over all the entries. > #if defined(CONFIG_PROC_FS) && defined(CONFIG_PROC_DEVICETREE) > extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *); > extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop); > -- > 1.8.1.5 >
On Thu, 21 Nov 2013 13:43:28 +0100 Grant Likely <grant.likely@linaro.org> wrote: > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > > The following pattern of code is tempting: > > > > for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > That's a very minimal commit message. Can you elaborate please. The above can be: " The following pattern of code is tempting to add a new member for of_property_for_each_*() family as an idiom. for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) <do something with "args">; " Actual usage is here: int i; struct of_phandle_args args; of_property_for_each_phandle_with_args(dev->of_node, "iommus", "#iommu-cells", i, &args) { pr_debug("%s(i=%d) %s\n", __func__, i, dev_name(dev)); if (!of_find_iommu_by_node(args.np)) return -EPROBE_DEFER; Is this acceptable?
On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > On Thu, 21 Nov 2013 13:43:28 +0100 > Grant Likely <grant.likely@linaro.org> wrote: > > > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > > > The following pattern of code is tempting: > > > > > > for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) > > > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > > > That's a very minimal commit message. Can you elaborate please. > > The above can be: > > " > The following pattern of code is tempting to add a new member for > of_property_for_each_*() family as an idiom. > > for (i = 0; > !of_parse_phandle_with_args(np, list, cells, i, args); i++) > <do something with "args">; > " I really do like commit messages to be full enough that a future reader can figure out why a patch was written. ie: "Iterating over a property containing a list of phandles with arguments is a common operation for device drivers. This patch adds a new of_property_for_each_phandle_with_args() macro to make the iteration simpler." g. > > Actual usage is here: > > int i; > struct of_phandle_args args; > > of_property_for_each_phandle_with_args(dev->of_node, "iommus", > "#iommu-cells", i, &args) { > pr_debug("%s(i=%d) %s\n", __func__, i, dev_name(dev)); > > if (!of_find_iommu_by_node(args.np)) > return -EPROBE_DEFER; > > Is this acceptable?
Grant Likely <grant.likely@linaro.org> wrote @ Thu, 21 Nov 2013 16:56:49 +0100: > On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > > On Thu, 21 Nov 2013 13:43:28 +0100 > > Grant Likely <grant.likely@linaro.org> wrote: > > > > > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > > > > The following pattern of code is tempting: > > > > > > > > for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) > > > > > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > > > > > That's a very minimal commit message. Can you elaborate please. > > > > The above can be: > > > > " > > The following pattern of code is tempting to add a new member for > > of_property_for_each_*() family as an idiom. > > > > for (i = 0; > > !of_parse_phandle_with_args(np, list, cells, i, args); i++) > > <do something with "args">; > > " > > I really do like commit messages to be full enough that a future reader > can figure out why a patch was written. ie: Updated as: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args() http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007063.html This doesn't depend on anything and this can be merged independetly. Thanks for your help.
On 11/21/2013 10:20 AM, Hiroshi Doyu wrote: > Grant Likely <grant.likely@linaro.org> wrote @ Thu, 21 Nov 2013 16:56:49 +0100: > >> On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: >>> On Thu, 21 Nov 2013 13:43:28 +0100 >>> Grant Likely <grant.likely@linaro.org> wrote: >>> >>>> On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: >>>>> The following pattern of code is tempting: >>>>> >>>>> for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) >>>>> >>>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> >>>> >>>> That's a very minimal commit message. Can you elaborate please. >>> >>> The above can be: >>> >>> " >>> The following pattern of code is tempting to add a new member for >>> of_property_for_each_*() family as an idiom. >>> >>> for (i = 0; >>> !of_parse_phandle_with_args(np, list, cells, i, args); i++) >>> <do something with "args">; >>> " >> >> I really do like commit messages to be full enough that a future reader >> can figure out why a patch was written. ie: > > Updated as: > > [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args() > http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007063.html > > This doesn't depend on anything and this can be merged > independetly. Thanks for your help. Well, that patch doesn't depend /on/ anything, but something in the rest of the series does depend /on it/. As such, this patch can't be merged completely independently; it has to either: a) Go into whatever branch the rest of the series goes into. b) Go into a topic branch in the DT tree, which is then both merged into the main/regular DT tree /and/ used as a base for the rest of this series. Dependencies work two ways! (That is, unless this 1 patch gets merged into 3.14, and the rest of series doesn't get merged until 3.15. In that case, we can ignore the dependencies).
On Thu, Nov 21, 2013 at 11:20 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > Grant Likely <grant.likely@linaro.org> wrote @ Thu, 21 Nov 2013 16:56:49 +0100: > >> On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: >> > On Thu, 21 Nov 2013 13:43:28 +0100 >> > Grant Likely <grant.likely@linaro.org> wrote: >> > >> > > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote: >> > > > The following pattern of code is tempting: >> > > > >> > > > for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) >> > > > >> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> >> > > >> > > That's a very minimal commit message. Can you elaborate please. >> > >> > The above can be: >> > >> > " >> > The following pattern of code is tempting to add a new member for >> > of_property_for_each_*() family as an idiom. >> > >> > for (i = 0; >> > !of_parse_phandle_with_args(np, list, cells, i, args); i++) >> > <do something with "args">; >> > " >> >> I really do like commit messages to be full enough that a future reader >> can figure out why a patch was written. ie: > > Updated as: > > [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args() You also have a typo in the subject. s/earch/each/ Rob
diff --git a/include/linux/of.h b/include/linux/of.h index 276c546..131fef5 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -613,6 +613,9 @@ static inline int of_property_read_u32(const struct device_node *np, s; \ s = of_prop_next_string(prop, s)) +#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \ + for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) + #if defined(CONFIG_PROC_FS) && defined(CONFIG_PROC_DEVICETREE) extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *); extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop);
The following pattern of code is tempting: for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- v5: New patch for v5. --- include/linux/of.h | 3 +++ 1 file changed, 3 insertions(+)