Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1273757 > unrolled thread
| Started by | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| First post | 2015-11-20 08:40 +0100 |
| Last post | 2015-12-01 01:50 +0100 |
| Articles | 6 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Masahiro Yamada <yamada.masahiro@socionext.com> - 2015-11-20 08:40 +0100
Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Stephen Boyd <sboyd@codeaurora.org> - 2015-11-20 18:50 +0100
Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Masahiro Yamada <yamada.masahiro@socionext.com> - 2015-11-22 07:10 +0100
Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Stephen Boyd <sboyd@codeaurora.org> - 2015-11-24 02:00 +0100
Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Masahiro Yamada <yamada.masahiro@socionext.com> - 2015-11-30 09:40 +0100
Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Stephen Boyd <sboyd@codeaurora.org> - 2015-12-01 01:50 +0100
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2015-11-20 08:40 +0100 |
| Subject | [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices |
| Message-ID | <qwOsx-4aA-1@gated-at.bofh.it> |
Currently, of_clk_get_parent_name() returns a wrong parent clock name
when "clock-indices" property exists and the given index is not found
in the property. In this case, NULL should be returned.
For example,
oscillator {
compatible = "myclocktype";
#clock-cells = <1>;
clock-indices = <1>, <3>;
clock-output-names = "clka", "clkb";
};
Currently, of_clk_get_parent_name(np, 0) returns "clka", but should
return NULL because "clock-indices" does not contain <0>.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/clk/clk.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 20d8e07..8698074 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3054,12 +3054,9 @@ EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
const char *of_clk_get_parent_name(struct device_node *np, int index)
{
struct of_phandle_args clkspec;
- struct property *prop;
const char *clk_name;
- const __be32 *vp;
- u32 pv;
- int rc;
- int count;
+ const __be32 *list;
+ int rc, len, i;
struct clk *clk;
rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
@@ -3068,17 +3065,20 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
return NULL;
index = clkspec.args_count ? clkspec.args[0] : 0;
- count = 0;
/* if there is an indices property, use it to transfer the index
* specified into an array offset for the clock-output-names property.
*/
- of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
- if (index == pv) {
- index = count;
- break;
- }
- count++;
+ list = of_get_property(clkspec.np, "clock-indices", &len);
+ if (list) {
+ len /= sizeof(*list);
+ for (i = 0; i < len; i++)
+ if (index == be32_to_cpup(list++)) {
+ index = i;
+ break;
+ }
+ if (i == len)
+ return NULL;
}
if (of_property_read_string_index(clkspec.np, "clock-output-names",
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-11-20 18:50 +0100 |
| Subject | Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices |
| Message-ID | <qwXYR-1UO-5@gated-at.bofh.it> |
| In reply to | #1273757 |
On 11/20, Masahiro Yamada wrote:
> Currently, of_clk_get_parent_name() returns a wrong parent clock name
> when "clock-indices" property exists and the given index is not found
> in the property. In this case, NULL should be returned.
>
> For example,
>
> oscillator {
> compatible = "myclocktype";
> #clock-cells = <1>;
> clock-indices = <1>, <3>;
> clock-output-names = "clka", "clkb";
> };
>
> Currently, of_clk_get_parent_name(np, 0) returns "clka", but should
> return NULL because "clock-indices" does not contain <0>.
What is np pointing at? Something like:
consumer {
clocks = <&oscillator 0>;
};
Which would be invalid DT because oscillator doesn't have an
output for index 0?
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> @@ -3068,17 +3065,20 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> return NULL;
>
> index = clkspec.args_count ? clkspec.args[0] : 0;
> - count = 0;
>
> /* if there is an indices property, use it to transfer the index
> * specified into an array offset for the clock-output-names property.
> */
> - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> - if (index == pv) {
> - index = count;
> - break;
> - }
> - count++;
> + list = of_get_property(clkspec.np, "clock-indices", &len);
> + if (list) {
> + len /= sizeof(*list);
> + for (i = 0; i < len; i++)
> + if (index == be32_to_cpup(list++)) {
> + index = i;
> + break;
> + }
> + if (i == len)
> + return NULL;
> }
Why can't we leave everything in place and check count == len at
the end? i.e.
of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
if (index == pv) {
index = count;
break;
}
count++;
}
if (count == of_property_count_u32_elems(clkspec.np, "clock-indices"))
return NULL
?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2015-11-22 07:10 +0100 |
| Message-ID | <qxw0x-8at-3@gated-at.bofh.it> |
| In reply to | #1274314 |
Hi Stephen,
2015-11-21 2:45 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 11/20, Masahiro Yamada wrote:
>> Currently, of_clk_get_parent_name() returns a wrong parent clock name
>> when "clock-indices" property exists and the given index is not found
>> in the property. In this case, NULL should be returned.
>>
>> For example,
>>
>> oscillator {
>> compatible = "myclocktype";
>> #clock-cells = <1>;
>> clock-indices = <1>, <3>;
>> clock-output-names = "clka", "clkb";
>> };
>>
>> Currently, of_clk_get_parent_name(np, 0) returns "clka", but should
>> return NULL because "clock-indices" does not contain <0>.
>
> What is np pointing at? Something like:
>
> consumer {
> clocks = <&oscillator 0>;
> };
>
> Which would be invalid DT because oscillator doesn't have an
> output for index 0?
You are right. My example was confusing.
oscillator: oscillator {
compatible = "myclocktype";
#clock-cells = <1>;
clock-indices = <1>, <3>;
clock-output-names = "clka", "clkb";
};
consumer {
compatible = "myclockconsumer";
clocks = <&oscillator 0>;
};
Currently, of_clk_get_parent_name(consumer_np, 0) returns "clks", but
should return NULL;
I will rephrase the git-log in v2.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> @@ -3068,17 +3065,20 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>> return NULL;
>>
>> index = clkspec.args_count ? clkspec.args[0] : 0;
>> - count = 0;
>>
>> /* if there is an indices property, use it to transfer the index
>> * specified into an array offset for the clock-output-names property.
>> */
>> - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
>> - if (index == pv) {
>> - index = count;
>> - break;
>> - }
>> - count++;
>> + list = of_get_property(clkspec.np, "clock-indices", &len);
>> + if (list) {
>> + len /= sizeof(*list);
>> + for (i = 0; i < len; i++)
>> + if (index == be32_to_cpup(list++)) {
>> + index = i;
>> + break;
>> + }
>> + if (i == len)
>> + return NULL;
>> }
>
> Why can't we leave everything in place and check count == len at
> the end? i.e.
>
> of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> if (index == pv) {
> index = count;
> break;
> }
> count++;
> }
>
> if (count == of_property_count_u32_elems(clkspec.np, "clock-indices"))
> return NULL
>
Of course we can, although we have to mention "clock-indices" twice.
A good thing for of_get_property() is that we can get both the value
and the length
at the same time.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-11-24 02:00 +0100 |
| Subject | Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices |
| Message-ID | <qya7E-Sp-7@gated-at.bofh.it> |
| In reply to | #1274796 |
On 11/22, Masahiro Yamada wrote:
> 2015-11-21 2:45 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> >
> > What is np pointing at? Something like:
> >
> > consumer {
> > clocks = <&oscillator 0>;
> > };
> >
> > Which would be invalid DT because oscillator doesn't have an
> > output for index 0?
>
>
> You are right. My example was confusing.
>
>
>
> oscillator: oscillator {
> compatible = "myclocktype";
> #clock-cells = <1>;
> clock-indices = <1>, <3>;
> clock-output-names = "clka", "clkb";
> };
>
> consumer {
> compatible = "myclockconsumer";
> clocks = <&oscillator 0>;
> };
>
> Currently, of_clk_get_parent_name(consumer_np, 0) returns "clks", but
> should return NULL;
>
> I will rephrase the git-log in v2.
>
Ok. Thanks.
> > Why can't we leave everything in place and check count == len at
> > the end? i.e.
> >
> > of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> > if (index == pv) {
> > index = count;
> > break;
> > }
> > count++;
> > }
> >
> > if (count == of_property_count_u32_elems(clkspec.np, "clock-indices"))
> > return NULL
> >
>
>
> Of course we can, although we have to mention "clock-indices" twice.
>
> A good thing for of_get_property() is that we can get both the value
> and the length
> at the same time.
>
Ok. Well if we don't want to count them again, perhaps a goto
jump over an unconditional return NULL would be better?
of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
if (index == pv) {
index = count;
goto found;
}
count++;
}
return NULL;
found:
Or since the macro for of_property_for_each_u32() tests the vp
poitner for NULL, we can check that pointer too...
of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
if (index == pv) {
index = count;
break;
}
count++;
}
/* We didn't find anything */
if (!vp)
return NULL;
I guess I prefer the latter approach here.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2015-11-30 09:40 +0100 |
| Message-ID | <qAsa7-1cU-31@gated-at.bofh.it> |
| In reply to | #1275995 |
Hi Stephen,
>>
>> Of course we can, although we have to mention "clock-indices" twice.
>>
>> A good thing for of_get_property() is that we can get both the value
>> and the length
>> at the same time.
>>
>
> Ok. Well if we don't want to count them again, perhaps a goto
> jump over an unconditional return NULL would be better?
>
> of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> if (index == pv) {
> index = count;
> goto found;
> }
> count++;
> }
>
> return NULL;
> found:
>
> Or since the macro for of_property_for_each_u32() tests the vp
> poitner for NULL, we can check that pointer too...
>
> of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> if (index == pv) {
> index = count;
> break;
> }
> count++;
> }
>
> /* We didn't find anything */
> if (!vp)
> return NULL;
>
> I guess I prefer the latter approach here.
>
No.
Neither of your two suggestions works because they are false positive.
With your way, if "clock-indices" does not exist, of_clk_get_parent_name()
would return NULL; in this case it should just parse "clock-output-names",
assuming that the clock names are simply indexed from zero.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-12-01 01:50 +0100 |
| Subject | Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices |
| Message-ID | <qAHiN-2re-1@gated-at.bofh.it> |
| In reply to | #1279683 |
On 11/30, Masahiro Yamada wrote:
> Hi Stephen,
>
>
>
> >>
> >> Of course we can, although we have to mention "clock-indices" twice.
> >>
> >> A good thing for of_get_property() is that we can get both the value
> >> and the length
> >> at the same time.
> >>
> >
> > Ok. Well if we don't want to count them again, perhaps a goto
> > jump over an unconditional return NULL would be better?
> >
> > of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> > if (index == pv) {
> > index = count;
> > goto found;
> > }
> > count++;
> > }
> >
> > return NULL;
> > found:
> >
> > Or since the macro for of_property_for_each_u32() tests the vp
> > poitner for NULL, we can check that pointer too...
> >
> > of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> > if (index == pv) {
> > index = count;
> > break;
> > }
> > count++;
> > }
> >
> > /* We didn't find anything */
> > if (!vp)
> > return NULL;
> >
> > I guess I prefer the latter approach here.
> >
>
> No.
>
> Neither of your two suggestions works because they are false positive.
>
So if (!vp && count) then?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web