Re: Question about devm_get_clk_from_child()

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()

[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