Question about devm_get_clk_from_child()

Question about devm_get_clk_from_child()

[Kuninori Morimoto]

Hi Stephen

I noticed that this commit breaks my ALSA Sound playback

	4472287a3b2f52f4aa53f294ccb74392dde4e07d
	("clk: Introduce of_clk_get_hw_from_clkspec()")
    
I debuged this issue, and noticed that devm_get_clk_from_child() can't get clk
my pseudo DT code is like this

	deviceA: devA {
		...
		device = <&devB>
	};

	deviceB: devB {
		clocks = <xxxx>
	};

I could get clock on deviceB driver

	// dev = deviceB
	clk = clk_get(dev, NULL); 

	clk is "clocks = <xxx>"

But, I couldn't get clock from deviceA driver

	// node = device = devB
	node = ...

	// dev = deviceA
	clk = devm_get_clk_from_child(dev, node, NULL);

My understanding is these can get same clock.
But, am I wrong ??

Best regards
---
Kuninori Morimoto

Re: Question about devm_get_clk_from_child()

[Kuninori Morimoto]

Hi Stephen, again

> I noticed that this commit breaks my ALSA Sound playback
> 
> 	4472287a3b2f52f4aa53f294ccb74392dde4e07d
> 	("clk: Introduce of_clk_get_hw_from_clkspec()")

I could solve this issue, but I'm not sure this is correct.

For ALSA SoC
At least if you are using "simple-card" or "audio-graph" driver,
you might got same issue.

------------------
>From ea035a378a24479aea8d00fa8463bc4ff0bc5926 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Wed, 6 Mar 2019 15:54:59 +0900
Subject: [PATCH] clk: fixup default index for of_clk_get_by_name()

of_clk_get_by_name() is using -1 for __of_clk_get() index.
It will goes to of_parse_clkspec(), and be used for
of_parse_phandle_with_args().
Here, if user doesn't specified clock name
(= of_clk_get_by_name(np, NULL)), this index is still -1,
and of_parse_phandle_with_args() will return -EINVAL
(This index will be updated if if it had clock name).
clk_get_by_name(np, NULL) should work, then, default index
should be 0 instead of -1.
This patch fixup it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 14cbf23..96053a9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 	if (!np)
 		return ERR_PTR(-ENOENT);
 
-	return __of_clk_get(np, -1, np->full_name, name);
+	return __of_clk_get(np, 0, np->full_name, name);
 }
 EXPORT_SYMBOL(of_clk_get_by_name);
 
-- 
2.7.4


Best regards
---
Kuninori Morimoto

Re: Question about devm_get_clk_from_child()

[Stephen Boyd]

Quoting Kuninori Morimoto (2019-03-05 23:18:28)
> 
> of_clk_get_by_name() is using -1 for __of_clk_get() index.
> It will goes to of_parse_clkspec(), and be used for
> of_parse_phandle_with_args().
> Here, if user doesn't specified clock name
> (= of_clk_get_by_name(np, NULL)), this index is still -1,
> and of_parse_phandle_with_args() will return -EINVAL
> (This index will be updated if if it had clock name).
> clk_get_by_name(np, NULL) should work, then, default index
> should be 0 instead of -1.
> This patch fixup it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 14cbf23..96053a9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>         if (!np)
>                 return ERR_PTR(-ENOENT);
>  
> -       return __of_clk_get(np, -1, np->full_name, name);
> +       return __of_clk_get(np, 0, np->full_name, name);
>  }

Yes this is correct. Thanks for debugging and fixing my thinko here. I
was thinking that nobody would call of_clk_get_by_name() unless they
wanted to find some clk that had a matching name, but it seems that we
also allow NULL to be passed as the name to mean the typical "wildcard
match" thing that clkdev has done for years. I'll throw this patch on
top of the merge commit so the breakage window is small as I'd rather
not rewrite the series just for this. Thanks.

Re: Question about devm_get_clk_from_child()

[Stephen Boyd]

Quoting Kuninori Morimoto (2019-03-05 23:18:28)
> 
> of_clk_get_by_name() is using -1 for __of_clk_get() index.
> It will goes to of_parse_clkspec(), and be used for
> of_parse_phandle_with_args().
> Here, if user doesn't specified clock name
> (= of_clk_get_by_name(np, NULL)), this index is still -1,
> and of_parse_phandle_with_args() will return -EINVAL
> (This index will be updated if if it had clock name).
> clk_get_by_name(np, NULL) should work, then, default index
> should be 0 instead of -1.
> This patch fixup it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 14cbf23..96053a9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>         if (!np)
>                 return ERR_PTR(-ENOENT);
>  
> -       return __of_clk_get(np, -1, np->full_name, name);
> +       return __of_clk_get(np, 0, np->full_name, name);
>  }

Yes this is correct. Thanks for debugging and fixing my thinko here. I
was thinking that nobody would call of_clk_get_by_name() unless they
wanted to find some clk that had a matching name, but it seems that we
also allow NULL to be passed as the name to mean the typical "wildcard
match" thing that clkdev has done for years. I'll throw this patch on
top of the merge commit so the breakage window is small as I'd rather
not rewrite the series just for this. Thanks.

Re: Question about devm_get_clk_from_child()

[Kuninori Morimoto]

Hi Stephen

> > index 14cbf23..96053a9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> >         if (!np)
> >                 return ERR_PTR(-ENOENT);
> >  
> > -       return __of_clk_get(np, -1, np->full_name, name);
> > +       return __of_clk_get(np, 0, np->full_name, name);
> >  }
> 
> Yes this is correct. Thanks for debugging and fixing my thinko here. I
> was thinking that nobody would call of_clk_get_by_name() unless they
> wanted to find some clk that had a matching name, but it seems that we
> also allow NULL to be passed as the name to mean the typical "wildcard
> match" thing that clkdev has done for years. I'll throw this patch on
> top of the merge commit so the breakage window is small as I'd rather
> not rewrite the series just for this. Thanks.

Ahh, OK.
So, how about this ? it is including both opinion :)
I'm not sure which one should be applied, but both can solve my issue.

--------------------
From e75755351963c9b70ff3484f3088c4ed76d9d196 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Thu, 7 Mar 2019 09:05:51 +0900
Subject: [PATCH] clk: fixup default index for of_clk_get_by_name()

of_clk_get_by_name() is using -1 for __of_clk_get() index.
It will goes to of_parse_clkspec(), and be used for
of_parse_phandle_with_args().
Here, if user doesn't specified clock name
(= of_clk_get_by_name(np, NULL)), this index is still -1,
and of_parse_phandle_with_args() will return -EINVAL
(This index will be updated if if it had clock name).

OTAH, nobody would call of_clk_get_by_name() unless they
wanted to find some clk that had a matching name.
But we also allow NULL to be passed as the name to mean the
typical "wildcard match" thing.

This means default index should be 0 instead of -1 if name was NULL.
This patch fixup it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/clk/clk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 14cbf23..66c71e9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4210,10 +4210,12 @@ EXPORT_SYMBOL(of_clk_get);
  */
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 {
+	int index = name ? -1 : 0;
+
 	if (!np)
 		return ERR_PTR(-ENOENT);
 
-	return __of_clk_get(np, -1, np->full_name, name);
+	return __of_clk_get(np, index, np->full_name, name);
 }
 EXPORT_SYMBOL(of_clk_get_by_name);
 
-- 
2.7.4

Re: Question about devm_get_clk_from_child()

[Kuninori Morimoto]

Hi Stephen

> > index 14cbf23..96053a9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> >         if (!np)
> >                 return ERR_PTR(-ENOENT);
> >  
> > -       return __of_clk_get(np, -1, np->full_name, name);
> > +       return __of_clk_get(np, 0, np->full_name, name);
> >  }
> 
> Yes this is correct. Thanks for debugging and fixing my thinko here. I
> was thinking that nobody would call of_clk_get_by_name() unless they
> wanted to find some clk that had a matching name, but it seems that we
> also allow NULL to be passed as the name to mean the typical "wildcard
> match" thing that clkdev has done for years. I'll throw this patch on
> top of the merge commit so the breakage window is small as I'd rather
> not rewrite the series just for this. Thanks.

Ahh, OK.
So, how about this ? it is including both opinion :)
I'm not sure which one should be applied, but both can solve my issue.

--------------------
>From e75755351963c9b70ff3484f3088c4ed76d9d196 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Thu, 7 Mar 2019 09:05:51 +0900
Subject: [PATCH] clk: fixup default index for of_clk_get_by_name()

of_clk_get_by_name() is using -1 for __of_clk_get() index.
It will goes to of_parse_clkspec(), and be used for
of_parse_phandle_with_args().
Here, if user doesn't specified clock name
(= of_clk_get_by_name(np, NULL)), this index is still -1,
and of_parse_phandle_with_args() will return -EINVAL
(This index will be updated if if it had clock name).

OTAH, nobody would call of_clk_get_by_name() unless they
wanted to find some clk that had a matching name.
But we also allow NULL to be passed as the name to mean the
typical "wildcard match" thing.

This means default index should be 0 instead of -1 if name was NULL.
This patch fixup it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/clk/clk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 14cbf23..66c71e9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4210,10 +4210,12 @@ EXPORT_SYMBOL(of_clk_get);
  */
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 {
+	int index = name ? -1 : 0;
+
 	if (!np)
 		return ERR_PTR(-ENOENT);
 
-	return __of_clk_get(np, -1, np->full_name, name);
+	return __of_clk_get(np, index, np->full_name, name);
 }
 EXPORT_SYMBOL(of_clk_get_by_name);
 
-- 
2.7.4

Re: Question about devm_get_clk_from_child()

[Stephen Boyd]

Quoting Kuninori Morimoto (2019-03-06 16:20:40)
> 
> Hi Stephen
> 
> > > index 14cbf23..96053a9 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> > >         if (!np)
> > >                 return ERR_PTR(-ENOENT);
> > >  
> > > -       return __of_clk_get(np, -1, np->full_name, name);
> > > +       return __of_clk_get(np, 0, np->full_name, name);
> > >  }
> > 
> > Yes this is correct. Thanks for debugging and fixing my thinko here. I
> > was thinking that nobody would call of_clk_get_by_name() unless they
> > wanted to find some clk that had a matching name, but it seems that we
> > also allow NULL to be passed as the name to mean the typical "wildcard
> > match" thing that clkdev has done for years. I'll throw this patch on
> > top of the merge commit so the breakage window is small as I'd rather
> > not rewrite the series just for this. Thanks.
> 
> Ahh, OK.
> So, how about this ? it is including both opinion :)
> I'm not sure which one should be applied, but both can solve my issue.
> 

No worries. I applied your original patch.

Re: Question about devm_get_clk_from_child()

[Kuninori Morimoto]

Hi Stephen

> > > Yes this is correct. Thanks for debugging and fixing my thinko here. I
> > > was thinking that nobody would call of_clk_get_by_name() unless they
> > > wanted to find some clk that had a matching name, but it seems that we
> > > also allow NULL to be passed as the name to mean the typical "wildcard
> > > match" thing that clkdev has done for years. I'll throw this patch on
> > > top of the merge commit so the breakage window is small as I'd rather
> > > not rewrite the series just for this. Thanks.
> > 
> > Ahh, OK.
> > So, how about this ? it is including both opinion :)
> > I'm not sure which one should be applied, but both can solve my issue.
> > 
> 
> No worries. I applied your original patch.

OK. Thanks a lot !!

Best regards
---
Kuninori Morimoto