[PATCH 1/3] clk: remove redundant negative index check in of_clk_get_parent_name()

[PATCH 1/3] clk: remove redundant negative index check in of_clk_get_parent_name()

[Masahiro Yamada]

This if-block can be dropped because the of_parse_phandle_with_args()
in the following line returns -EINVAL for negative index.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 764aca2..20d8e07 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3062,9 +3062,6 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 	int count;
 	struct clk *clk;
 
-	if (index < 0)
-		return NULL;
-
 	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
 					&clkspec);
 	if (rc)
-- 
1.9.1

[PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

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

[PATCH 3/3] clk: split of_clk_get_parent_name() into two functions

[Masahiro Yamada]

Currently, there is no function to get the clock name of the given
node.  Create a new helper function, of_clk_get_name().  This is
useful to get the clock name where "clock-indices" property is used.

  of_clk_get_name(): get the clock name in the given node
  of_clk_get_parent_name(): get the name of the parent clock

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

I want to use of_clk_get_name() for my clk drivers for my SoCs,
which I will submit later.

I found this helper function is useful.


 drivers/clk/clk.c            | 45 +++++++++++++++++++++++++++-----------------
 include/linux/clk-provider.h |  1 +
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8698074..1788ec7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3051,25 +3051,17 @@ int of_clk_get_parent_count(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
 
-const char *of_clk_get_parent_name(struct device_node *np, int index)
+const char *of_clk_get_name(struct device_node *np, int index)
 {
-	struct of_phandle_args clkspec;
 	const char *clk_name;
 	const __be32 *list;
-	int rc, len, i;
-	struct clk *clk;
-
-	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
-					&clkspec);
-	if (rc)
-		return NULL;
-
-	index = clkspec.args_count ? clkspec.args[0] : 0;
+	int len, i, rc;
 
-	/* if there is an indices property, use it to transfer the index
+	/*
+	 * if there is an indices property, use it to transfer the index
 	 * specified into an array offset for the clock-output-names property.
 	 */
-	list = of_get_property(clkspec.np, "clock-indices", &len);
+	list = of_get_property(np, "clock-indices", &len);
 	if (list) {
 		len /= sizeof(*list);
 		for (i = 0; i < len; i++)
@@ -3081,9 +3073,29 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 			return NULL;
 	}
 
-	if (of_property_read_string_index(clkspec.np, "clock-output-names",
-					  index,
-					  &clk_name) < 0) {
+	rc = of_property_read_string_index(np, "clock-output-names", index,
+					   &clk_name);
+
+	return rc ? NULL : clk_name;
+}
+EXPORT_SYMBOL_GPL(of_clk_get_name);
+
+const char *of_clk_get_parent_name(struct device_node *np, int index)
+{
+	struct of_phandle_args clkspec;
+	const char *clk_name;
+	struct clk *clk;
+	int rc;
+
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
+					&clkspec);
+	if (rc)
+		return NULL;
+
+	index = clkspec.args_count ? clkspec.args[0] : 0;
+
+	clk_name = of_clk_get_name(clkspec.np, index);
+	if (!clk_name) {
 		/*
 		 * Best effort to get the name if the clock has been
 		 * registered with the framework. If the clock isn't
@@ -3102,7 +3114,6 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 		}
 	}
 
-
 	of_node_put(clkspec.np);
 	return clk_name;
 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index e7bd710..c6ff498 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -702,6 +702,7 @@ struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data);
 int of_clk_get_parent_count(struct device_node *np);
 int of_clk_parent_fill(struct device_node *np, const char **parents,
 		       unsigned int size);
+const char *of_clk_get_name(struct device_node *np, int index);
 const char *of_clk_get_parent_name(struct device_node *np, int index);
 
 void of_clk_init(const struct of_device_id *matches);
-- 
1.9.1

Re: [PATCH 1/3] clk: remove redundant negative index check in of_clk_get_parent_name()

[Stephen Boyd]

On 11/20, Masahiro Yamada wrote:
> This if-block can be dropped because the of_parse_phandle_with_args()
> in the following line returns -EINVAL for negative index.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Stephen Boyd]

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

Re: [PATCH 3/3] clk: split of_clk_get_parent_name() into two functions

[Stephen Boyd]

On 11/20, Masahiro Yamada wrote:
> Currently, there is no function to get the clock name of the given
> node.  Create a new helper function, of_clk_get_name().  This is
> useful to get the clock name where "clock-indices" property is used.
> 
>   of_clk_get_name(): get the clock name in the given node
>   of_clk_get_parent_name(): get the name of the parent clock
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> I want to use of_clk_get_name() for my clk drivers for my SoCs,
> which I will submit later.
> 
> I found this helper function is useful.

I don't see how this is useful. Is the new driver so generic it
doesn't know what clocks it's outputting? We've been trying to
move people away from using clock-output-names, so most likely
this sort of information should be conveyed from DT via the
compatible string and a table in the driver that matches up the
compatible string with the list of clock names.

> @@ -3102,7 +3114,6 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  		}
>  	}
>  
> -

Noise. Please remove.

>  	of_node_put(clkspec.np);
>  	return clk_name;
>  }

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] clk: split of_clk_get_parent_name() into two functions

[Masahiro Yamada]

Hi Stephen,


2015-11-21 9:37 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 11/20, Masahiro Yamada wrote:
>> Currently, there is no function to get the clock name of the given
>> node.  Create a new helper function, of_clk_get_name().  This is
>> useful to get the clock name where "clock-indices" property is used.
>>
>>   of_clk_get_name(): get the clock name in the given node
>>   of_clk_get_parent_name(): get the name of the parent clock
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> I want to use of_clk_get_name() for my clk drivers for my SoCs,
>> which I will submit later.
>>
>> I found this helper function is useful.
>
> I don't see how this is useful. Is the new driver so generic it
> doesn't know what clocks it's outputting? We've been trying to
> move people away from using clock-output-names, so most likely
> this sort of information should be conveyed from DT via the
> compatible string and a table in the driver that matches up the
> compatible string with the list of clock names.


What I implemented in my driver was:

[1] Use the clock names built in the driver, if "clock-output-names"
    is missing in the DT.

[2] If "clock-output-names" is specified, the driver overrides
    the clock names, respecting the "clock-output-names".


The following is a snippet from my driver code.


    /*
     * update the clock name with the one provided by
     * clock-output-names property, if exists
     */
    new_name = of_clk_get_name(np, index);
    if (new_name)
            init_data[i].name = new_name;
    else
            pr_info("DT does not specify output name for %s[%d]\n",
                    np->name, index);


I read the Documentation/devicetree/bindings/clock-bindings.txt
before I stared my driver development.

I did not know we are deprecating the "clock-output-names".
(Should it be mentioned in the clock-bindings.txt?)


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

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

Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Stephen Boyd]

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

Re: [PATCH 3/3] clk: split of_clk_get_parent_name() into two functions

[Masahiro Yamada]

Hi Stephen,


2015-11-22 14:44 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Stephen,
>
>
> 2015-11-21 9:37 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> On 11/20, Masahiro Yamada wrote:
>>> Currently, there is no function to get the clock name of the given
>>> node.  Create a new helper function, of_clk_get_name().  This is
>>> useful to get the clock name where "clock-indices" property is used.
>>>
>>>   of_clk_get_name(): get the clock name in the given node
>>>   of_clk_get_parent_name(): get the name of the parent clock
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>> I want to use of_clk_get_name() for my clk drivers for my SoCs,
>>> which I will submit later.
>>>
>>> I found this helper function is useful.
>>
>> I don't see how this is useful. Is the new driver so generic it
>> doesn't know what clocks it's outputting? We've been trying to
>> move people away from using clock-output-names, so most likely
>> this sort of information should be conveyed from DT via the
>> compatible string and a table in the driver that matches up the
>> compatible string with the list of clock names.
>
>
> What I implemented in my driver was:
>
> [1] Use the clock names built in the driver, if "clock-output-names"
>     is missing in the DT.
>
> [2] If "clock-output-names" is specified, the driver overrides
>     the clock names, respecting the "clock-output-names".
>
>
> The following is a snippet from my driver code.
>
>
>     /*
>      * update the clock name with the one provided by
>      * clock-output-names property, if exists
>      */
>     new_name = of_clk_get_name(np, index);
>     if (new_name)
>             init_data[i].name = new_name;
>     else
>             pr_info("DT does not specify output name for %s[%d]\n",
>                     np->name, index);
>
>
> I read the Documentation/devicetree/bindings/clock-bindings.txt
> before I stared my driver development.
>
> I did not know we are deprecating the "clock-output-names".
> (Should it be mentioned in the clock-bindings.txt?)




Please let me clarify the driver implementation guideline.

[1] Do not add "clock-output-names" in new device trees.
[2] New clock drivers should not rely on "clock-output-names" at all.


Is this correct?




-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

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

Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Stephen Boyd]

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

Re: [PATCH 3/3] clk: split of_clk_get_parent_name() into two functions

[Stephen Boyd]

On 11/24, Masahiro Yamada wrote:
> Hi Stephen,
> 
> 
> 2015-11-22 14:44 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> > Hi Stephen,
> >
> >
> > 2015-11-21 9:37 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> >> On 11/20, Masahiro Yamada wrote:
> >>> Currently, there is no function to get the clock name of the given
> >>> node.  Create a new helper function, of_clk_get_name().  This is
> >>> useful to get the clock name where "clock-indices" property is used.
> >>>
> >>>   of_clk_get_name(): get the clock name in the given node
> >>>   of_clk_get_parent_name(): get the name of the parent clock
> >>>
> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >>> ---
> >>>
> >>> I want to use of_clk_get_name() for my clk drivers for my SoCs,
> >>> which I will submit later.
> >>>
> >>> I found this helper function is useful.
> >>
> >> I don't see how this is useful. Is the new driver so generic it
> >> doesn't know what clocks it's outputting? We've been trying to
> >> move people away from using clock-output-names, so most likely
> >> this sort of information should be conveyed from DT via the
> >> compatible string and a table in the driver that matches up the
> >> compatible string with the list of clock names.
> >
> >
> > What I implemented in my driver was:
> >
> > [1] Use the clock names built in the driver, if "clock-output-names"
> >     is missing in the DT.
> >
> > [2] If "clock-output-names" is specified, the driver overrides
> >     the clock names, respecting the "clock-output-names".
> >
> >
> > The following is a snippet from my driver code.
> >
> >
> >     /*
> >      * update the clock name with the one provided by
> >      * clock-output-names property, if exists
> >      */
> >     new_name = of_clk_get_name(np, index);
> >     if (new_name)
> >             init_data[i].name = new_name;
> >     else
> >             pr_info("DT does not specify output name for %s[%d]\n",
> >                     np->name, index);
> >
> >
> > I read the Documentation/devicetree/bindings/clock-bindings.txt
> > before I stared my driver development.
> >
> > I did not know we are deprecating the "clock-output-names".
> > (Should it be mentioned in the clock-bindings.txt?)
> 
> 
> 
> 
> Please let me clarify the driver implementation guideline.
> 
> [1] Do not add "clock-output-names" in new device trees.
> [2] New clock drivers should not rely on "clock-output-names" at all.
> 
> 
> Is this correct?
> 

Seems a little extreme, but yes, we would like to see clock
provider drivers stop using clock-output-names. The binding isn't
deprecated, so we shouldn't say that in the binding document, but
perhaps we could add a comment that it's strongly suggested that
another way be found.

The only use I can think of is for something like fixed rate
clocks or other pure DT described clocks that want to tell us
their name. But the end goal is to make that irrelevant by using
something besides strings (or at least, user definded strings) to
describe the linkages between clocks in the clock tree. When this
is done, the clock-output-names property will be unused.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] clk: split of_clk_get_parent_name() into two functions

[Masahiro Yamada]

2015-12-01 9:49 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 11/24, Masahiro Yamada wrote:
>> Hi Stephen,
>>
>>
>> 2015-11-22 14:44 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>> > Hi Stephen,
>> >
>> >
>> > 2015-11-21 9:37 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> >> On 11/20, Masahiro Yamada wrote:
>> >>> Currently, there is no function to get the clock name of the given
>> >>> node.  Create a new helper function, of_clk_get_name().  This is
>> >>> useful to get the clock name where "clock-indices" property is used.
>> >>>
>> >>>   of_clk_get_name(): get the clock name in the given node
>> >>>   of_clk_get_parent_name(): get the name of the parent clock
>> >>>
>> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >>> ---
>> >>>
>> >>> I want to use of_clk_get_name() for my clk drivers for my SoCs,
>> >>> which I will submit later.
>> >>>
>> >>> I found this helper function is useful.
>> >>
>> >> I don't see how this is useful. Is the new driver so generic it
>> >> doesn't know what clocks it's outputting? We've been trying to
>> >> move people away from using clock-output-names, so most likely
>> >> this sort of information should be conveyed from DT via the
>> >> compatible string and a table in the driver that matches up the
>> >> compatible string with the list of clock names.
>> >
>> >
>> > What I implemented in my driver was:
>> >
>> > [1] Use the clock names built in the driver, if "clock-output-names"
>> >     is missing in the DT.
>> >
>> > [2] If "clock-output-names" is specified, the driver overrides
>> >     the clock names, respecting the "clock-output-names".
>> >
>> >
>> > The following is a snippet from my driver code.
>> >
>> >
>> >     /*
>> >      * update the clock name with the one provided by
>> >      * clock-output-names property, if exists
>> >      */
>> >     new_name = of_clk_get_name(np, index);
>> >     if (new_name)
>> >             init_data[i].name = new_name;
>> >     else
>> >             pr_info("DT does not specify output name for %s[%d]\n",
>> >                     np->name, index);
>> >
>> >
>> > I read the Documentation/devicetree/bindings/clock-bindings.txt
>> > before I stared my driver development.
>> >
>> > I did not know we are deprecating the "clock-output-names".
>> > (Should it be mentioned in the clock-bindings.txt?)
>>
>>
>>
>>
>> Please let me clarify the driver implementation guideline.
>>
>> [1] Do not add "clock-output-names" in new device trees.
>> [2] New clock drivers should not rely on "clock-output-names" at all.
>>
>>
>> Is this correct?
>>
>
> Seems a little extreme, but yes, we would like to see clock
> provider drivers stop using clock-output-names. The binding isn't
> deprecated, so we shouldn't say that in the binding document, but
> perhaps we could add a comment that it's strongly suggested that
> another way be found.
>
> The only use I can think of is for something like fixed rate
> clocks or other pure DT described clocks that want to tell us
> their name. But the end goal is to make that irrelevant by using
> something besides strings (or at least, user definded strings) to
> describe the linkages between clocks in the clock tree. When this
> is done, the clock-output-names property will be unused.

Right.

What is weird about clk_register() is that
it takes the parent name(s), not pointers of the parent(s).

The current way allows us to add clock providers in any order,
but I am not sure if this flexibility is really needed.




-- 
Best Regards
Masahiro Yamada