diff mbox series

[RFC] dm: add cells_count parameter in *_count_phandle_with_args

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

Commit Message

Patrick Delaunay Sept. 10, 2020, 4:43 p.m. UTC
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(-)

Comments

Simon Glass Sept. 22, 2020, 6:48 p.m. UTC | #1
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.
Patrick Delaunay Sept. 23, 2020, 3:06 p.m. UTC | #2
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
Simon Glass Sept. 24, 2020, 4:08 p.m. UTC | #3
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 mbox series

Patch

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)