| Message ID | 20200910164341.29613-1-patrick.delaunay@st.com |
|---|---|
| State | RFC |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [RFC] dm: add cells_count parameter in *_count_phandle_with_args | expand |
On Thu, 10 Sep 2020 at 10:44, Patrick Delaunay <patrick.delaunay@st.com> wrote: > > The cell_count argument is required when cells_name is NULL. > > This patch adds this parameter in live tree API > - of_count_phandle_with_args > - ofnode_count_phandle_with_args > - dev_count_phandle_with_args > > This parameter solves issue when these API is used to count > the number of element of a cell without cell name. This parameter > allow to force the size cell. > > For example: > count = dev_count_phandle_with_args(dev, "array", NULL, 3); > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > --- > I push today this RFC. > > It is linked to previous serie [1] but it is not a blocking point today > as no user use this API with cells_name = NULL > + dev_count_phandle_with_args > + ofnode_count_phandle_with_args > > But I think it is the good time to modify these functions as they are not > hugely used: it is the proposition in this RFC. > > It is just a RFC because I don't sure if I can modify the existing API > even if parameters are aligned with *_parse_phandle_with_args. > > I can also to add new APIs to use when cells_name is NULL: > + dev_count_phandle_with_cell_count(node, list_name, cell_count); > + ofnode_count_phandle_with_cell_count(node, list_name, cell_count); > > and raise a error if cells_name == NULL in existing function > + dev_count_phandle_with_args > + ofnode_count_phandle_with_args > > [1] http://patchwork.ozlabs.org/project/uboot/list/?series=200899 > "dm: add cells_count parameter in live DT APIs of_parse_phandle_with_args" > > > board/st/stm32mp1/stm32mp1.c | 2 +- > drivers/clk/clk-uclass.c | 4 ++-- > drivers/core/of_access.c | 7 ++++--- > drivers/core/ofnode.c | 6 +++--- > drivers/core/read.c | 5 +++-- > drivers/phy/phy-uclass.c | 2 +- > drivers/reset/reset-uclass.c | 2 +- > drivers/usb/host/ehci-generic.c | 4 ++-- > include/dm/of_access.h | 4 +++- > include/dm/ofnode.h | 3 ++- > include/dm/read.h | 8 +++++--- > 11 files changed, 27 insertions(+), 20 deletions(-)' Reviewed-by: Simon Glass <sjg@chromium.org> A test would go a long way here.
Hi Simon, > From: Simon Glass <sjg@chromium.org> > Sent: mardi 22 septembre 2020 20:49 > > On Thu, 10 Sep 2020 at 10:44, Patrick Delaunay <patrick.delaunay@st.com> > wrote: > > > > The cell_count argument is required when cells_name is NULL. > > > > This patch adds this parameter in live tree API > > - of_count_phandle_with_args > > - ofnode_count_phandle_with_args > > - dev_count_phandle_with_args > > > > This parameter solves issue when these API is used to count the number > > of element of a cell without cell name. This parameter allow to force > > the size cell. > > > > For example: > > count = dev_count_phandle_with_args(dev, "array", NULL, 3); > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > > --- > > I push today this RFC. > > > > It is linked to previous serie [1] but it is not a blocking point > > today as no user use this API with cells_name = NULL > > + dev_count_phandle_with_args > > + ofnode_count_phandle_with_args > > > > But I think it is the good time to modify these functions as they are > > not hugely used: it is the proposition in this RFC. > > > > It is just a RFC because I don't sure if I can modify the existing API > > even if parameters are aligned with *_parse_phandle_with_args. > > > > I can also to add new APIs to use when cells_name is NULL: > > + dev_count_phandle_with_cell_count(node, list_name, cell_count); > > + ofnode_count_phandle_with_cell_count(node, list_name, cell_count); > > > > and raise a error if cells_name == NULL in existing function > > + dev_count_phandle_with_args > > + ofnode_count_phandle_with_args > > > > [1] http://patchwork.ozlabs.org/project/uboot/list/?series=200899 > > "dm: add cells_count parameter in live DT APIs of_parse_phandle_with_args" > > > > > > board/st/stm32mp1/stm32mp1.c | 2 +- > > drivers/clk/clk-uclass.c | 4 ++-- > > drivers/core/of_access.c | 7 ++++--- > > drivers/core/ofnode.c | 6 +++--- > > drivers/core/read.c | 5 +++-- > > drivers/phy/phy-uclass.c | 2 +- > > drivers/reset/reset-uclass.c | 2 +- > > drivers/usb/host/ehci-generic.c | 4 ++-- > > include/dm/of_access.h | 4 +++- > > include/dm/ofnode.h | 3 ++- > > include/dm/read.h | 8 +++++--- > > 11 files changed, 27 insertions(+), 20 deletions(-)' > > Reviewed-by: Simon Glass <sjg@chromium.org> > > A test would go a long way here. Sure, I will add a test in the real patch... I send RFC without test just to be sure that adding parameter in *_count_phandle_with_args() is a better solution than adding a new API. Proposition 1 (it is the RFC content): add argument in current API Example: of_count_phandle_with_args(node, "clocks", "#clock-cells", 0); ofnode_count_phandle_with_args(node, "resets", "#reset-cells", 0); dev_count_phandle_with_args(node, "phys", "#phy-cells", 0); dev_count_phandle_with_args(node, "test", NULL, 3); ofnode_count_phandle_with_args(node, "test", NULL, 3); Proposition 2: new API *count_phandle_with_cell_count of_count_phandle_with_args(node, "clocks", "#clock-cells"); ofnode_count_phandle_with_args(node, "resets", "#reset-cells"); dev_count_phandle_with_args(node, "phys", "#phy-cells"); dev_count_phandle_with_fixed_args(node, "test", 3); ofnode_count_phandle_with_fixed_args(node, "test", 3); I think that Proposition 1 (this RFC) is more clear because parameters are aligned with other API *read_phandle_with_args But proposition 2 is align with Linux API - of_count_phandle_with_args - of_parse_phandle_with_fixed_args And avoid to change all the current users of *count_phandle_with_args Patrick
Hi Patrick, On Wed, 23 Sep 2020 at 09:06, Patrick DELAUNAY <patrick.delaunay@st.com> wrote: > > Hi Simon, > > > From: Simon Glass <sjg@chromium.org> > > Sent: mardi 22 septembre 2020 20:49 > > > > On Thu, 10 Sep 2020 at 10:44, Patrick Delaunay <patrick.delaunay@st.com> > > wrote: > > > > > > The cell_count argument is required when cells_name is NULL. > > > > > > This patch adds this parameter in live tree API > > > - of_count_phandle_with_args > > > - ofnode_count_phandle_with_args > > > - dev_count_phandle_with_args > > > > > > This parameter solves issue when these API is used to count the number > > > of element of a cell without cell name. This parameter allow to force > > > the size cell. > > > > > > For example: > > > count = dev_count_phandle_with_args(dev, "array", NULL, 3); > > > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > > > --- > > > I push today this RFC. > > > > > > It is linked to previous serie [1] but it is not a blocking point > > > today as no user use this API with cells_name = NULL > > > + dev_count_phandle_with_args > > > + ofnode_count_phandle_with_args > > > > > > But I think it is the good time to modify these functions as they are > > > not hugely used: it is the proposition in this RFC. > > > > > > It is just a RFC because I don't sure if I can modify the existing API > > > even if parameters are aligned with *_parse_phandle_with_args. > > > > > > I can also to add new APIs to use when cells_name is NULL: > > > + dev_count_phandle_with_cell_count(node, list_name, cell_count); > > > + ofnode_count_phandle_with_cell_count(node, list_name, cell_count); > > > > > > and raise a error if cells_name == NULL in existing function > > > + dev_count_phandle_with_args > > > + ofnode_count_phandle_with_args > > > > > > [1] http://patchwork.ozlabs.org/project/uboot/list/?series=200899 > > > "dm: add cells_count parameter in live DT APIs of_parse_phandle_with_args" > > > > > > > > > board/st/stm32mp1/stm32mp1.c | 2 +- > > > drivers/clk/clk-uclass.c | 4 ++-- > > > drivers/core/of_access.c | 7 ++++--- > > > drivers/core/ofnode.c | 6 +++--- > > > drivers/core/read.c | 5 +++-- > > > drivers/phy/phy-uclass.c | 2 +- > > > drivers/reset/reset-uclass.c | 2 +- > > > drivers/usb/host/ehci-generic.c | 4 ++-- > > > include/dm/of_access.h | 4 +++- > > > include/dm/ofnode.h | 3 ++- > > > include/dm/read.h | 8 +++++--- > > > 11 files changed, 27 insertions(+), 20 deletions(-)' > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > A test would go a long way here. > > Sure, I will add a test in the real patch... > > I send RFC without test just to be sure that adding parameter in *_count_phandle_with_args() > is a better solution than adding a new API. > > Proposition 1 (it is the RFC content): add argument in current API I think that is best. It's only one more parameter onto a call that already has lots of parameters. It reduced the number of separate functions. > > Example: > > of_count_phandle_with_args(node, "clocks", "#clock-cells", 0); > ofnode_count_phandle_with_args(node, "resets", "#reset-cells", 0); > dev_count_phandle_with_args(node, "phys", "#phy-cells", 0); > > dev_count_phandle_with_args(node, "test", NULL, 3); > ofnode_count_phandle_with_args(node, "test", NULL, 3); > > > Proposition 2: new API *count_phandle_with_cell_count > > of_count_phandle_with_args(node, "clocks", "#clock-cells"); > ofnode_count_phandle_with_args(node, "resets", "#reset-cells"); > dev_count_phandle_with_args(node, "phys", "#phy-cells"); > > dev_count_phandle_with_fixed_args(node, "test", 3); > ofnode_count_phandle_with_fixed_args(node, "test", 3); > > I think that Proposition 1 (this RFC) is more clear because parameters are aligned > with other API *read_phandle_with_args > > But proposition 2 is align with Linux API > - of_count_phandle_with_args > - of_parse_phandle_with_fixed_args > And avoid to change all the current users of *count_phandle_with_args > > Patrick > - Simon
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 3b677d339b..03a19af930 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -314,7 +314,7 @@ static int board_check_usb_power(void) * for each of them */ adc_count = ofnode_count_phandle_with_args(node, "st,adc_usb_pd", - "#io-channel-cells"); + "#io-channel-cells", 0); if (adc_count < 0) { if (adc_count == -ENOENT) return 0; diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 934cd5787a..b1d8f1adaf 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -160,7 +160,7 @@ int clk_get_bulk(struct udevice *dev, struct clk_bulk *bulk) bulk->count = 0; - count = dev_count_phandle_with_args(dev, "clocks", "#clock-cells"); + count = dev_count_phandle_with_args(dev, "clocks", "#clock-cells", 0); if (count < 1) return count; @@ -195,7 +195,7 @@ static int clk_set_default_parents(struct udevice *dev, int stage) int ret; num_parents = dev_count_phandle_with_args(dev, "assigned-clock-parents", - "#clock-cells"); + "#clock-cells", 0); if (num_parents < 0) { debug("%s: could not read assigned-clock-parents for %p\n", __func__, dev); diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c index bcf1644d05..0a12e9b26f 100644 --- a/drivers/core/of_access.c +++ b/drivers/core/of_access.c @@ -756,10 +756,11 @@ int of_parse_phandle_with_args(const struct device_node *np, } int of_count_phandle_with_args(const struct device_node *np, - const char *list_name, const char *cells_name) + const char *list_name, const char *cells_name, + int cell_count) { - return __of_parse_phandle_with_args(np, list_name, cells_name, 0, - -1, NULL); + return __of_parse_phandle_with_args(np, list_name, cells_name, + cell_count, -1, NULL); } static void of_alias_add(struct alias_prop *ap, struct device_node *np, diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 79fcdf5ce2..7d1b89514c 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -432,15 +432,15 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name, } int ofnode_count_phandle_with_args(ofnode node, const char *list_name, - const char *cells_name) + const char *cells_name, int cell_count) { if (ofnode_is_np(node)) return of_count_phandle_with_args(ofnode_to_np(node), - list_name, cells_name); + list_name, cells_name, cell_count); else return fdtdec_parse_phandle_with_args(gd->fdt_blob, ofnode_to_offset(node), list_name, cells_name, - 0, -1, NULL); + cell_count, -1, NULL); } ofnode ofnode_path(const char *path) diff --git a/drivers/core/read.c b/drivers/core/read.c index 86f3f88170..076125824c 100644 --- a/drivers/core/read.c +++ b/drivers/core/read.c @@ -214,10 +214,11 @@ int dev_read_phandle_with_args(const struct udevice *dev, const char *list_name, } int dev_count_phandle_with_args(const struct udevice *dev, - const char *list_name, const char *cells_name) + const char *list_name, const char *cells_name, + int cell_count) { return ofnode_count_phandle_with_args(dev_ofnode(dev), list_name, - cells_name); + cells_name, cell_count); } int dev_read_addr_cells(const struct udevice *dev) diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c index db7f39cd0b..8713b8e7b7 100644 --- a/drivers/phy/phy-uclass.c +++ b/drivers/phy/phy-uclass.c @@ -179,7 +179,7 @@ int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) if (!dev_read_prop(dev, "phys", NULL)) return 0; - count = dev_count_phandle_with_args(dev, "phys", "#phy-cells"); + count = dev_count_phandle_with_args(dev, "phys", "#phy-cells", 0); if (count < 1) return count; diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c index 5e38ce5c06..411ad649ca 100644 --- a/drivers/reset/reset-uclass.c +++ b/drivers/reset/reset-uclass.c @@ -106,7 +106,7 @@ int reset_get_bulk(struct udevice *dev, struct reset_ctl_bulk *bulk) bulk->count = 0; - count = dev_count_phandle_with_args(dev, "resets", "#reset-cells"); + count = dev_count_phandle_with_args(dev, "resets", "#reset-cells", 0); if (count < 1) return count; diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index 304a3437d5..c93a7051a7 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -86,7 +86,7 @@ static int ehci_usb_probe(struct udevice *dev) err = 0; priv->clock_count = 0; clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks", - "#clock-cells"); + "#clock-cells", 0); if (clock_nb > 0) { priv->clocks = devm_kcalloc(dev, clock_nb, sizeof(struct clk), GFP_KERNEL); @@ -116,7 +116,7 @@ static int ehci_usb_probe(struct udevice *dev) priv->reset_count = 0; reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets", - "#reset-cells"); + "#reset-cells", 0); if (reset_nb > 0) { priv->resets = devm_kcalloc(dev, reset_nb, sizeof(struct reset_ctl), diff --git a/include/dm/of_access.h b/include/dm/of_access.h index 2fa65c9332..cc382b1671 100644 --- a/include/dm/of_access.h +++ b/include/dm/of_access.h @@ -450,6 +450,7 @@ int of_parse_phandle_with_args(const struct device_node *np, * @np: pointer to a device tree node containing a list * @list_name: property name that contains a list * @cells_name: property name that specifies phandles' arguments count + * @cells_count: Cell count to use if @cells_name is NULL * @return number of phandle found, -ENOENT if * @list_name does not exist, -EINVAL if a phandle was not found, * @cells_name could not be found, the arguments were truncated or there @@ -460,7 +461,8 @@ int of_parse_phandle_with_args(const struct device_node *np, * */ int of_count_phandle_with_args(const struct device_node *np, - const char *list_name, const char *cells_name); + const char *list_name, const char *cells_name, + int cells_count); /** * of_alias_scan() - Scan all properties of the 'aliases' node diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 8df2facf99..1726b6b2f9 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -555,12 +555,13 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name, * @node: device tree node containing a list * @list_name: property name that contains a list * @cells_name: property name that specifies phandles' arguments count + * @cells_count: Cell count to use if @cells_name is NULL * @return number of phandle on success, -ENOENT if @list_name does not * exist, -EINVAL if a phandle was not found, @cells_name could not * be found. */ int ofnode_count_phandle_with_args(ofnode node, const char *list_name, - const char *cells_name); + const char *cells_name, int cell_count); /** * ofnode_path() - find a node by full path diff --git a/include/dm/read.h b/include/dm/read.h index 67db94adfc..0585eb1228 100644 --- a/include/dm/read.h +++ b/include/dm/read.h @@ -429,12 +429,14 @@ int dev_read_phandle_with_args(const struct udevice *dev, const char *list_name, * @dev: device whose node containing a list * @list_name: property name that contains a list * @cells_name: property name that specifies phandles' arguments count + * @cells_count: Cell count to use if @cells_name is NULL * @Returns number of phandle found on success, on error returns appropriate * errno value. */ int dev_count_phandle_with_args(const struct udevice *dev, - const char *list_name, const char *cells_name); + const char *list_name, const char *cells_name, + int cell_count); /** * dev_read_addr_cells() - Get the number of address cells for a device's node @@ -880,10 +882,10 @@ static inline int dev_read_phandle_with_args(const struct udevice *dev, } static inline int dev_count_phandle_with_args(const struct udevice *dev, - const char *list_name, const char *cells_name) + const char *list_name, const char *cells_name, int cell_count) { return ofnode_count_phandle_with_args(dev_ofnode(dev), list_name, - cells_name); + cells_name, cell_count); } static inline int dev_read_addr_cells(const struct udevice *dev)
The cell_count argument is required when cells_name is NULL. This patch adds this parameter in live tree API - of_count_phandle_with_args - ofnode_count_phandle_with_args - dev_count_phandle_with_args This parameter solves issue when these API is used to count the number of element of a cell without cell name. This parameter allow to force the size cell. For example: count = dev_count_phandle_with_args(dev, "array", NULL, 3); Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- I push today this RFC. It is linked to previous serie [1] but it is not a blocking point today as no user use this API with cells_name = NULL + dev_count_phandle_with_args + ofnode_count_phandle_with_args But I think it is the good time to modify these functions as they are not hugely used: it is the proposition in this RFC. It is just a RFC because I don't sure if I can modify the existing API even if parameters are aligned with *_parse_phandle_with_args. I can also to add new APIs to use when cells_name is NULL: + dev_count_phandle_with_cell_count(node, list_name, cell_count); + ofnode_count_phandle_with_cell_count(node, list_name, cell_count); and raise a error if cells_name == NULL in existing function + dev_count_phandle_with_args + ofnode_count_phandle_with_args [1] http://patchwork.ozlabs.org/project/uboot/list/?series=200899 "dm: add cells_count parameter in live DT APIs of_parse_phandle_with_args" board/st/stm32mp1/stm32mp1.c | 2 +- drivers/clk/clk-uclass.c | 4 ++-- drivers/core/of_access.c | 7 ++++--- drivers/core/ofnode.c | 6 +++--- drivers/core/read.c | 5 +++-- drivers/phy/phy-uclass.c | 2 +- drivers/reset/reset-uclass.c | 2 +- drivers/usb/host/ehci-generic.c | 4 ++-- include/dm/of_access.h | 4 +++- include/dm/ofnode.h | 3 ++- include/dm/read.h | 8 +++++--- 11 files changed, 27 insertions(+), 20 deletions(-)