From: Richard Fitzgerald <rf@opensource.cirrus.com>
To: Robin Murphy <robin.murphy@arm.com>, Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org,
Linux-ALSA <alsa-devel@alsa-project.org>,
- <patches@opensource.cirrus.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Mark Brown <broonie@kernel.org>,
Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
"moderated list:BROADCOM BCM2835 ARM ARCHITECTURE"
<linux-rpi-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/7] of: base: Add of_count_phandle_with_fixed_args()
Date: Fri, 16 Oct 2020 10:10:16 +0100 [thread overview]
Message-ID: <008108c4-952b-a8a3-af81-af9e40643f36@opensource.cirrus.com> (raw)
In-Reply-To: <90600a67-25e4-7933-35c3-f515deaee94f@arm.com>
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
>>
next prev parent reply other threads:[~2020-10-16 9:11 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-14 14:54 [PATCH 0/7] Add dts for Rpi4 + Cirrus Lochnagar and codecs Richard Fitzgerald
2020-10-14 14:54 ` [PATCH 1/7] of: base: Add of_count_phandle_with_fixed_args() Richard Fitzgerald
2020-10-14 18:39 ` Rob Herring
2020-10-15 16:52 ` Robin Murphy
2020-10-16 9:10 ` Richard Fitzgerald [this message]
2020-10-16 13:31 ` Rob Herring
2020-10-16 15:14 ` Richard Fitzgerald
2020-10-14 14:54 ` [PATCH 2/7] ASoC: simple-card: Add plls and sysclks DT schema Richard Fitzgerald
2020-10-14 14:54 ` [PATCH 3/7] ASoC: simple-card: Support setting component plls and sysclks Richard Fitzgerald
2020-10-14 14:54 ` [PATCH 4/7] ASoC: arizona: Allow codecs to be selected from kernel config Richard Fitzgerald
2020-10-15 0:19 ` kernel test robot
2020-10-14 14:54 ` [PATCH 5/7] ASoC: madera: " Richard Fitzgerald
2020-10-14 14:54 ` [PATCH 6/7] ARM: dts: Add dts for Raspberry Pi 4 + Cirrus Logic Lochnagar2 Richard Fitzgerald
2020-10-15 10:25 ` Nicolas Saenz Julienne
2020-10-15 11:14 ` Richard Fitzgerald
2020-10-15 15:12 ` Nicolas Saenz Julienne
2020-10-15 17:32 ` Mark Brown
2020-10-16 9:01 ` Richard Fitzgerald
2020-10-14 14:54 ` [PATCH 7/7] MAINTAINERS: Add dts for Cirrus Logic Lochnagar on RPi4 Richard Fitzgerald
2020-10-14 18:56 ` [PATCH 0/7] Add dts for Rpi4 + Cirrus Lochnagar and codecs Mark Brown
2020-10-16 13:30 ` Richard Fitzgerald
2020-10-16 15:18 ` Mark Brown
2020-10-19 20:48 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=008108c4-952b-a8a3-af81-af9e40643f36@opensource.cirrus.com \
--to=rf@opensource.cirrus.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=nsaenzjulienne@suse.de \
--cc=patches@opensource.cirrus.com \
--cc=robh+dt@kernel.org \
--cc=robin.murphy@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).