diff mbox series

[1/7] of: base: Add of_count_phandle_with_fixed_args()

Message ID 20201014145418.31838-2-rf@opensource.cirrus.com (mailing list archive)
State New, archived
Headers show
Series Add dts for Rpi4 + Cirrus Lochnagar and codecs | expand

Commit Message

Richard Fitzgerald Oct. 14, 2020, 2:54 p.m. UTC
Add an equivalent of of_count_phandle_with_args() for fixed argument
sets, to pair with of_parse_phandle_with_fixed_args().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/of/base.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h |  9 +++++++++
 2 files changed, 51 insertions(+)

Comments

Rob Herring Oct. 14, 2020, 6:39 p.m. UTC | #1
On Wed, Oct 14, 2020 at 9:54 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add an equivalent of of_count_phandle_with_args() for fixed argument
> sets, to pair with of_parse_phandle_with_fixed_args().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  drivers/of/base.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h |  9 +++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ea44fea99813..45d8b0e65345 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1772,6 +1772,48 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>  }
>  EXPORT_SYMBOL(of_count_phandle_with_args);
>
> +/**
> + * of_count_phandle_with_fixed_args() - Find the number of phandles references in a property
> + * @np:                pointer to a device tree node containing a list
> + * @list_name: property name that contains a list
> + * @cell_count: number of argument cells following the phandle
> + *
> + * Returns the number of phandle + argument tuples within a property. It
> + * is a typical pattern to encode a list of phandle and variable
> + * arguments into a single property.
> + */
> +int of_count_phandle_with_fixed_args(const struct device_node *np,
> +                                    const char *list_name,
> +                                    int cells_count)
> +{

Looks to me like you can refactor of_count_phandle_with_args to handle
both case and then make this and of_count_phandle_with_args simple
wrapper functions.

> +       struct of_phandle_iterator it;
> +       int rc, cur_index = 0;
> +
> +       if (!cells_count) {
> +               const __be32 *list;
> +               int size;
> +
> +               list = of_get_property(np, list_name, &size);
> +               if (!list)
> +                       return -ENOENT;
> +
> +               return size / sizeof(*list);
> +       }
> +
> +       rc = of_phandle_iterator_init(&it, np, list_name, NULL, cells_count);
> +       if (rc)
> +               return rc;
> +
> +       while ((rc = of_phandle_iterator_next(&it)) == 0)
> +               cur_index += 1;
> +
> +       if (rc != -ENOENT)
> +               return rc;
> +
> +       return cur_index;
> +}
> +EXPORT_SYMBOL(of_count_phandle_with_fixed_args);
> +
>  /**
>   * __of_add_property - Add a property to a node without lock operations
>   */
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 5cf7ae0465d1..9f315da4e9da 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -377,6 +377,8 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
>         struct of_phandle_args *out_args);
>  extern int of_count_phandle_with_args(const struct device_node *np,
>         const char *list_name, const char *cells_name);
> +extern int of_count_phandle_with_fixed_args(const struct device_node *np,
> +       const char *list_name, int cells_count);
>
>  /* phandle iterator functions */
>  extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
> @@ -886,6 +888,13 @@ static inline int of_count_phandle_with_args(struct device_node *np,
>         return -ENOSYS;
>  }
>
> +static inline int of_count_phandle_with_fixed_args(const struct device_node *np,
> +                                                  const char *list_name,
> +                                                  int cells_count)
> +{
> +       return -ENOSYS;
> +}
> +
>  static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
>                                            const struct device_node *np,
>                                            const char *list_name,
> --
> 2.20.1
>
Robin Murphy Oct. 15, 2020, 4:52 p.m. UTC | #2
On 2020-10-14 19:39, Rob Herring wrote:
> On Wed, Oct 14, 2020 at 9:54 AM Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Add an equivalent of of_count_phandle_with_args() for fixed argument
>> sets, to pair with of_parse_phandle_with_fixed_args().
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   drivers/of/base.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of.h |  9 +++++++++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index ea44fea99813..45d8b0e65345 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1772,6 +1772,48 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>>   }
>>   EXPORT_SYMBOL(of_count_phandle_with_args);
>>
>> +/**
>> + * of_count_phandle_with_fixed_args() - Find the number of phandles references in a property
>> + * @np:                pointer to a device tree node containing a list
>> + * @list_name: property name that contains a list
>> + * @cell_count: number of argument cells following the phandle
>> + *
>> + * Returns the number of phandle + argument tuples within a property. It
>> + * is a typical pattern to encode a list of phandle and variable
>> + * arguments into a single property.
>> + */
>> +int of_count_phandle_with_fixed_args(const struct device_node *np,
>> +                                    const char *list_name,
>> +                                    int cells_count)
>> +{
> 
> Looks to me like you can refactor of_count_phandle_with_args to handle
> both case and then make this and of_count_phandle_with_args simple
> wrapper functions.

Although for just counting the number of phandles each with n arguments 
that a property contains, isn't that simply a case of dividing the 
property length by n + 1? The phandles themselves will be validated by 
any subsequent of_parse_phandle*() call anyway, so there doesn't seem 
much point in doing more work then necessary here.

>> +       struct of_phandle_iterator it;
>> +       int rc, cur_index = 0;
>> +
>> +       if (!cells_count) {
>> +               const __be32 *list;
>> +               int size;
>> +
>> +               list = of_get_property(np, list_name, &size);
>> +               if (!list)
>> +                       return -ENOENT;
>> +
>> +               return size / sizeof(*list);

Case in point - if it's OK to do exactly that for n == 0, then clearly 
we're *aren't* fussed about validating anything, so the n > 0 code below 
is nothing more than a massively expensive way to check for a nonzero 
remainder :/

Robin.

>> +       }
>> +
>> +       rc = of_phandle_iterator_init(&it, np, list_name, NULL, cells_count);
>> +       if (rc)
>> +               return rc;
>> +
>> +       while ((rc = of_phandle_iterator_next(&it)) == 0)
>> +               cur_index += 1;
>> +
>> +       if (rc != -ENOENT)
>> +               return rc;
>> +
>> +       return cur_index;
>> +}
>> +EXPORT_SYMBOL(of_count_phandle_with_fixed_args);
>> +
>>   /**
>>    * __of_add_property - Add a property to a node without lock operations
>>    */
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 5cf7ae0465d1..9f315da4e9da 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -377,6 +377,8 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
>>          struct of_phandle_args *out_args);
>>   extern int of_count_phandle_with_args(const struct device_node *np,
>>          const char *list_name, const char *cells_name);
>> +extern int of_count_phandle_with_fixed_args(const struct device_node *np,
>> +       const char *list_name, int cells_count);
>>
>>   /* phandle iterator functions */
>>   extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
>> @@ -886,6 +888,13 @@ static inline int of_count_phandle_with_args(struct device_node *np,
>>          return -ENOSYS;
>>   }
>>
>> +static inline int of_count_phandle_with_fixed_args(const struct device_node *np,
>> +                                                  const char *list_name,
>> +                                                  int cells_count)
>> +{
>> +       return -ENOSYS;
>> +}
>> +
>>   static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
>>                                             const struct device_node *np,
>>                                             const char *list_name,
>> --
>> 2.20.1
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Richard Fitzgerald Oct. 16, 2020, 9:10 a.m. UTC | #3
On 15/10/2020 17:52, Robin Murphy wrote:
> On 2020-10-14 19:39, Rob Herring wrote:
>> On Wed, Oct 14, 2020 at 9:54 AM Richard Fitzgerald
>> <rf@opensource.cirrus.com> wrote:
>>>
>>> Add an equivalent of of_count_phandle_with_args() for fixed argument
>>> sets, to pair with of_parse_phandle_with_fixed_args().
>>>
>>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>>> ---
>>>   drivers/of/base.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/of.h |  9 +++++++++
>>>   2 files changed, 51 insertions(+)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index ea44fea99813..45d8b0e65345 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -1772,6 +1772,48 @@ int of_count_phandle_with_args(const struct 
>>> device_node *np, const char *list_na
>>>   }
>>>   EXPORT_SYMBOL(of_count_phandle_with_args);
>>>
>>> +/**
>>> + * of_count_phandle_with_fixed_args() - Find the number of phandles 
>>> references in a property
>>> + * @np:                pointer to a device tree node containing a list
>>> + * @list_name: property name that contains a list
>>> + * @cell_count: number of argument cells following the phandle
>>> + *
>>> + * Returns the number of phandle + argument tuples within a 
>>> property. It
>>> + * is a typical pattern to encode a list of phandle and variable
>>> + * arguments into a single property.
>>> + */
>>> +int of_count_phandle_with_fixed_args(const struct device_node *np,
>>> +                                    const char *list_name,
>>> +                                    int cells_count)
>>> +{
>>
>> Looks to me like you can refactor of_count_phandle_with_args to handle
>> both case and then make this and of_count_phandle_with_args simple
>> wrapper functions.
> 
> Although for just counting the number of phandles each with n arguments 
> that a property contains, isn't that simply a case of dividing the 
> property length by n + 1? The phandles themselves will be validated by 
> any subsequent of_parse_phandle*() call anyway, so there doesn't seem 
> much point in doing more work then necessary here.
> 

As I'm not a DT expert, I'm reluctant to change existing algorithms that
could break everything just for the trivial case of adding a fixed
arguments count. I have a re-worked patch as suggested by Rob that
re-uses the existing counting function for both cases.

>>> +       struct of_phandle_iterator it;
>>> +       int rc, cur_index = 0;
>>> +
>>> +       if (!cells_count) {
>>> +               const __be32 *list;
>>> +               int size;
>>> +
>>> +               list = of_get_property(np, list_name, &size);
>>> +               if (!list)
>>> +                       return -ENOENT;
>>> +
>>> +               return size / sizeof(*list);
> 
> Case in point - if it's OK to do exactly that for n == 0, then clearly 
> we're *aren't* fussed about validating anything, so the n > 0 code below 
> is nothing more than a massively expensive way to check for a nonzero 
> remainder :/
> 
> Robin.
> 
>>> +       }
>>> +
>>> +       rc = of_phandle_iterator_init(&it, np, list_name, NULL, 
>>> cells_count);
>>> +       if (rc)
>>> +               return rc;
>>> +
>>> +       while ((rc = of_phandle_iterator_next(&it)) == 0)
>>> +               cur_index += 1;
>>> +
>>> +       if (rc != -ENOENT)
>>> +               return rc;
>>> +
>>> +       return cur_index;
>>> +}
>>> +EXPORT_SYMBOL(of_count_phandle_with_fixed_args);
>>> +
>>>   /**
>>>    * __of_add_property - Add a property to a node without lock 
>>> operations
>>>    */
>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>> index 5cf7ae0465d1..9f315da4e9da 100644
>>> --- a/include/linux/of.h
>>> +++ b/include/linux/of.h
>>> @@ -377,6 +377,8 @@ extern int of_parse_phandle_with_fixed_args(const 
>>> struct device_node *np,
>>>          struct of_phandle_args *out_args);
>>>   extern int of_count_phandle_with_args(const struct device_node *np,
>>>          const char *list_name, const char *cells_name);
>>> +extern int of_count_phandle_with_fixed_args(const struct device_node 
>>> *np,
>>> +       const char *list_name, int cells_count);
>>>
>>>   /* phandle iterator functions */
>>>   extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
>>> @@ -886,6 +888,13 @@ static inline int 
>>> of_count_phandle_with_args(struct device_node *np,
>>>          return -ENOSYS;
>>>   }
>>>
>>> +static inline int of_count_phandle_with_fixed_args(const struct 
>>> device_node *np,
>>> +                                                  const char 
>>> *list_name,
>>> +                                                  int cells_count)
>>> +{
>>> +       return -ENOSYS;
>>> +}
>>> +
>>>   static inline int of_phandle_iterator_init(struct 
>>> of_phandle_iterator *it,
>>>                                             const struct device_node 
>>> *np,
>>>                                             const char *list_name,
>>> -- 
>>> 2.20.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
Rob Herring Oct. 16, 2020, 1:31 p.m. UTC | #4
On Thu, Oct 15, 2020 at 11:52 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-14 19:39, Rob Herring wrote:
> > On Wed, Oct 14, 2020 at 9:54 AM Richard Fitzgerald
> > <rf@opensource.cirrus.com> wrote:
> >>
> >> Add an equivalent of of_count_phandle_with_args() for fixed argument
> >> sets, to pair with of_parse_phandle_with_fixed_args().
> >>
> >> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> >> ---
> >>   drivers/of/base.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>   include/linux/of.h |  9 +++++++++
> >>   2 files changed, 51 insertions(+)
> >>
> >> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> index ea44fea99813..45d8b0e65345 100644
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -1772,6 +1772,48 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
> >>   }
> >>   EXPORT_SYMBOL(of_count_phandle_with_args);
> >>
> >> +/**
> >> + * of_count_phandle_with_fixed_args() - Find the number of phandles references in a property
> >> + * @np:                pointer to a device tree node containing a list
> >> + * @list_name: property name that contains a list
> >> + * @cell_count: number of argument cells following the phandle
> >> + *
> >> + * Returns the number of phandle + argument tuples within a property. It
> >> + * is a typical pattern to encode a list of phandle and variable
> >> + * arguments into a single property.
> >> + */
> >> +int of_count_phandle_with_fixed_args(const struct device_node *np,
> >> +                                    const char *list_name,
> >> +                                    int cells_count)
> >> +{
> >
> > Looks to me like you can refactor of_count_phandle_with_args to handle
> > both case and then make this and of_count_phandle_with_args simple
> > wrapper functions.
>
> Although for just counting the number of phandles each with n arguments
> that a property contains, isn't that simply a case of dividing the
> property length by n + 1? The phandles themselves will be validated by
> any subsequent of_parse_phandle*() call anyway, so there doesn't seem
> much point in doing more work then necessary here.
>
> >> +       struct of_phandle_iterator it;
> >> +       int rc, cur_index = 0;
> >> +
> >> +       if (!cells_count) {
> >> +               const __be32 *list;
> >> +               int size;
> >> +
> >> +               list = of_get_property(np, list_name, &size);
> >> +               if (!list)
> >> +                       return -ENOENT;
> >> +
> >> +               return size / sizeof(*list);
>
> Case in point - if it's OK to do exactly that for n == 0, then clearly
> we're *aren't* fussed about validating anything, so the n > 0 code below
> is nothing more than a massively expensive way to check for a nonzero
> remainder :/

Indeed. We should just generalize this. It can still be refactored to
shared code.

It's probably worthwhile to check for a remainder here IMO.

Rob
Richard Fitzgerald Oct. 16, 2020, 3:14 p.m. UTC | #5
On 16/10/2020 14:31, Rob Herring wrote:
> On Thu, Oct 15, 2020 at 11:52 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-14 19:39, Rob Herring wrote:
>>> On Wed, Oct 14, 2020 at 9:54 AM Richard Fitzgerald
>>> <rf@opensource.cirrus.com> wrote:
>>>>
>>>> Add an equivalent of of_count_phandle_with_args() for fixed argument
>>>> sets, to pair with of_parse_phandle_with_fixed_args().
>>>>
>>>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>>>> ---
>>>>    drivers/of/base.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/of.h |  9 +++++++++
>>>>    2 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index ea44fea99813..45d8b0e65345 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -1772,6 +1772,48 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>>>>    }
>>>>    EXPORT_SYMBOL(of_count_phandle_with_args);
>>>>
>>>> +/**
>>>> + * of_count_phandle_with_fixed_args() - Find the number of phandles references in a property
>>>> + * @np:                pointer to a device tree node containing a list
>>>> + * @list_name: property name that contains a list
>>>> + * @cell_count: number of argument cells following the phandle
>>>> + *
>>>> + * Returns the number of phandle + argument tuples within a property. It
>>>> + * is a typical pattern to encode a list of phandle and variable
>>>> + * arguments into a single property.
>>>> + */
>>>> +int of_count_phandle_with_fixed_args(const struct device_node *np,
>>>> +                                    const char *list_name,
>>>> +                                    int cells_count)
>>>> +{
>>>
>>> Looks to me like you can refactor of_count_phandle_with_args to handle
>>> both case and then make this and of_count_phandle_with_args simple
>>> wrapper functions.
>>
>> Although for just counting the number of phandles each with n arguments
>> that a property contains, isn't that simply a case of dividing the
>> property length by n + 1? The phandles themselves will be validated by
>> any subsequent of_parse_phandle*() call anyway, so there doesn't seem
>> much point in doing more work then necessary here.
>>
>>>> +       struct of_phandle_iterator it;
>>>> +       int rc, cur_index = 0;
>>>> +
>>>> +       if (!cells_count) {
>>>> +               const __be32 *list;
>>>> +               int size;
>>>> +
>>>> +               list = of_get_property(np, list_name, &size);
>>>> +               if (!list)
>>>> +                       return -ENOENT;
>>>> +
>>>> +               return size / sizeof(*list);
>>
>> Case in point - if it's OK to do exactly that for n == 0, then clearly
>> we're *aren't* fussed about validating anything, so the n > 0 code below
>> is nothing more than a massively expensive way to check for a nonzero
>> remainder :/
> 
> Indeed. We should just generalize this. It can still be refactored to
> shared code.
> 
> It's probably worthwhile to check for a remainder here IMO.
>

Ok, I looked at the implementation of of_phandle_iterator_next() and
it is in fact simply incrementing by 'count' 32-bit words. So as Robin
said the count_phandle_with_x_args()functions could simply divide the
length by count+1.

However, may I suggest that should be done in a separate patch after my
patch to add count_phandle_with_fixed_args()? That way, if replacing the
iteration with the simple length divide causes any unforeseen problems
the patch can just be reverted.

> Rob
>
diff mbox series

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ea44fea99813..45d8b0e65345 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1772,6 +1772,48 @@  int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
+/**
+ * of_count_phandle_with_fixed_args() - Find the number of phandles references in a property
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cell_count: number of argument cells following the phandle
+ *
+ * Returns the number of phandle + argument tuples within a property. It
+ * is a typical pattern to encode a list of phandle and variable
+ * arguments into a single property.
+ */
+int of_count_phandle_with_fixed_args(const struct device_node *np,
+				     const char *list_name,
+				     int cells_count)
+{
+	struct of_phandle_iterator it;
+	int rc, cur_index = 0;
+
+	if (!cells_count) {
+		const __be32 *list;
+		int size;
+
+		list = of_get_property(np, list_name, &size);
+		if (!list)
+			return -ENOENT;
+
+		return size / sizeof(*list);
+	}
+
+	rc = of_phandle_iterator_init(&it, np, list_name, NULL, cells_count);
+	if (rc)
+		return rc;
+
+	while ((rc = of_phandle_iterator_next(&it)) == 0)
+		cur_index += 1;
+
+	if (rc != -ENOENT)
+		return rc;
+
+	return cur_index;
+}
+EXPORT_SYMBOL(of_count_phandle_with_fixed_args);
+
 /**
  * __of_add_property - Add a property to a node without lock operations
  */
diff --git a/include/linux/of.h b/include/linux/of.h
index 5cf7ae0465d1..9f315da4e9da 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -377,6 +377,8 @@  extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 	struct of_phandle_args *out_args);
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
+extern int of_count_phandle_with_fixed_args(const struct device_node *np,
+	const char *list_name, int cells_count);
 
 /* phandle iterator functions */
 extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
@@ -886,6 +888,13 @@  static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_count_phandle_with_fixed_args(const struct device_node *np,
+						   const char *list_name,
+						   int cells_count)
+{
+	return -ENOSYS;
+}
+
 static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
 					   const struct device_node *np,
 					   const char *list_name,