Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1622219 > unrolled thread

Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock

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.

Contents

  Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock Stephen Boyd <sboyd@codeaurora.org> - 2017-04-12 17:10 +0200
    Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-04-16 23:00 +0200
      Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock Stephen Boyd <sboyd@codeaurora.org> - 2017-04-19 18:10 +0200
        Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-04-19 21:50 +0200
          Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock Stephen Boyd <sboyd@codeaurora.org> - 2017-04-22 04:20 +0200

#1622219 — Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock

On 04/08, Daniel Lezcano wrote:
>  
>  Example:
>  	pmic: pmic@f8000000 {
> @@ -24,4 +29,5 @@ Example:
>  		interrupt-controller;
>  		#interrupt-cells = <2>;
>  		pmic-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
> +		clock-cells = <0>;

Should be #clock-cells instead.

>  	}
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 92c12b8..c19983a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
>  obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
>  obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
>  obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
> +obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
>  obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
>  obj-$(CONFIG_COMMON_CLK_SCPI)           += clk-scpi.o
>  obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
> diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c
> new file mode 100644
> index 0000000..b2bea32
> --- /dev/null
> +++ b/drivers/clk/clk-hi655x.c
> @@ -0,0 +1,140 @@
> +
> +static int hi655x_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct hi655x_pmic *hi655x = dev_get_drvdata(parent);
> +	struct clk_init_data *hi655x_clk_init;

This can just go onto the stack? We don't need it around after
probe.

> +	struct hi655x_clk *hi655x_clk;
> +	const char *clk_name = "hi655x-clk";
> +	int ret;
> +
> +	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
> +	if (!hi655x_clk)
> +		return -ENOMEM;
> +
> +	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init),
> +				       GFP_KERNEL);
> +	if (!hi655x_clk_init)
> +		return -ENOMEM;
> +
> +	of_property_read_string_index(parent->of_node, "clock-output-names",
> +				      0, &clk_name);
> +
> +	hi655x_clk_init->name	= clk_name;
> +	hi655x_clk_init->ops	= &hi655x_clk_ops;
> +
> +	hi655x_clk->clk_hw.init	= hi655x_clk_init;
> +	hi655x_clk->hi655x	= hi655x;
> +
> +	platform_set_drvdata(pdev, hi655x_clk);
> +
> +	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
> +				     &hi655x_clk->clk_hw);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);

Missed this last time. Do you use this clkdev lookup? The name is
usually supposed to be based on what the device is expecting,
instead of clk_name, and we would want some device name for the
third argument here.

> +	if (ret)
> +		of_clk_del_provider(parent->of_node);
> +
> +	return ret;

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

[toc] | [next] | [standalone]

#1624461

On Wed, Apr 12, 2017 at 08:02:45AM -0700, Stephen Boyd wrote:
> On 04/08, Daniel Lezcano wrote:
> >  
> >  Example:
> >  	pmic: pmic@f8000000 {
> > @@ -24,4 +29,5 @@ Example:
> >  		interrupt-controller;
> >  		#interrupt-cells = <2>;
> >  		pmic-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
> > +		clock-cells = <0>;
> 
> Should be #clock-cells instead.

Ok.

> > +static int hi655x_clk_probe(struct platform_device *pdev)
> > +{
> > +	struct device *parent = pdev->dev.parent;
> > +	struct hi655x_pmic *hi655x = dev_get_drvdata(parent);
> > +	struct clk_init_data *hi655x_clk_init;
> 
> This can just go onto the stack? We don't need it around after
> probe.

Agree.
 
> > +	struct hi655x_clk *hi655x_clk;
> > +	const char *clk_name = "hi655x-clk";
> > +	int ret;
> > +
> > +	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
> > +	if (!hi655x_clk)
> > +		return -ENOMEM;
> > +
> > +	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init),
> > +				       GFP_KERNEL);
> > +	if (!hi655x_clk_init)
> > +		return -ENOMEM;
> > +
> > +	of_property_read_string_index(parent->of_node, "clock-output-names",
> > +				      0, &clk_name);
> > +
> > +	hi655x_clk_init->name	= clk_name;
> > +	hi655x_clk_init->ops	= &hi655x_clk_ops;
> > +
> > +	hi655x_clk->clk_hw.init	= hi655x_clk_init;
> > +	hi655x_clk->hi655x	= hi655x;
> > +
> > +	platform_set_drvdata(pdev, hi655x_clk);
> > +
> > +	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
> > +				     &hi655x_clk->clk_hw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> 
> Missed this last time. Do you use this clkdev lookup? The name is
> usually supposed to be based on what the device is expecting,
> instead of clk_name, and we would want some device name for the
> third argument here.

I'm not sure to get your comment. Are you saying the clk_name should be the
third argument?


-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1626428

On 04/16, Daniel Lezcano wrote:
> On Wed, Apr 12, 2017 at 08:02:45AM -0700, Stephen Boyd wrote:
> > On 04/08, Daniel Lezcano wrote:
>  
> > > +	struct hi655x_clk *hi655x_clk;
> > > +	const char *clk_name = "hi655x-clk";
> > > +	int ret;
> > > +
> > > +	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
> > > +	if (!hi655x_clk)
> > > +		return -ENOMEM;
> > > +
> > > +	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init),
> > > +				       GFP_KERNEL);
> > > +	if (!hi655x_clk_init)
> > > +		return -ENOMEM;
> > > +
> > > +	of_property_read_string_index(parent->of_node, "clock-output-names",
> > > +				      0, &clk_name);
> > > +
> > > +	hi655x_clk_init->name	= clk_name;
> > > +	hi655x_clk_init->ops	= &hi655x_clk_ops;
> > > +
> > > +	hi655x_clk->clk_hw.init	= hi655x_clk_init;
> > > +	hi655x_clk->hi655x	= hi655x;
> > > +
> > > +	platform_set_drvdata(pdev, hi655x_clk);
> > > +
> > > +	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
> > > +				     &hi655x_clk->clk_hw);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> > 
> > Missed this last time. Do you use this clkdev lookup? The name is
> > usually supposed to be based on what the device is expecting,
> > instead of clk_name, and we would want some device name for the
> > third argument here.
> 
> I'm not sure to get your comment. Are you saying the clk_name should be the
> third argument?
> 
> 

Sorry, no. I meant that con_id is typically something like "core"
or "ahb" or something like that, and dev_id is something like
"a456002.pmic_device" or whatever dev_name(pmic_dev) would return for
the consuming device. That way when we call clk_get(dev, "core")
it will find the lookup with "core" and "a456002.pmic_device" to
match up the clk lookup.

If anything, the clk_name should just go into the con_id for now,
and then it will need to be a globally unique identifier for the
clk. But that is going against how clkdev is supposed to be used.
Hence the question if you even need to use it. If not, just don't
add it. I can fix up v3 of this patch to put clk_name back at
con_id if you like. No need to resend.

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

[toc] | [prev] | [next] | [standalone]

#1626739

On Wed, Apr 19, 2017 at 09:00:05AM -0700, Stephen Boyd wrote:
> On 04/16, Daniel Lezcano wrote:
> > On Wed, Apr 12, 2017 at 08:02:45AM -0700, Stephen Boyd wrote:
> > > On 04/08, Daniel Lezcano wrote:

[ ... ]

> > > > +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> > > 
> > > Missed this last time. Do you use this clkdev lookup? The name is
> > > usually supposed to be based on what the device is expecting,
> > > instead of clk_name, and we would want some device name for the
> > > third argument here.
> > 
> > I'm not sure to get your comment. Are you saying the clk_name should be the
> > third argument?
> > 
> > 
> 
> Sorry, no. I meant that con_id is typically something like "core"
> or "ahb" or something like that, and dev_id is something like
> "a456002.pmic_device" or whatever dev_name(pmic_dev) would return for
> the consuming device. That way when we call clk_get(dev, "core")
> it will find the lookup with "core" and "a456002.pmic_device" to
> match up the clk lookup.
> 
> If anything, the clk_name should just go into the con_id for now,
> and then it will need to be a globally unique identifier for the
> clk. But that is going against how clkdev is supposed to be used.
> Hence the question if you even need to use it. If not, just don't
> add it. I can fix up v3 of this patch to put clk_name back at
> con_id if you like. No need to resend.

Ok, I'm not very used with the CCF, so perhaps clk_name is not needed at all. I
gave a try with the following combination:

 - con_id = NULL, dev_id = clk_name
 - con_id = clk_name, dev_id = NULL
 - con_id = NULL, dev_id = NULL

All worked.

And finally I removed the clk_hw_register_clkdev() call and it worked also.

So I'm not sure about this function.

Any idea ?

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

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1628737

On 04/19, Daniel Lezcano wrote:
> On Wed, Apr 19, 2017 at 09:00:05AM -0700, Stephen Boyd wrote:
> > On 04/16, Daniel Lezcano wrote:
> > > On Wed, Apr 12, 2017 at 08:02:45AM -0700, Stephen Boyd wrote:
> > > > On 04/08, Daniel Lezcano wrote:
> 
> [ ... ]
> 
> > > > > +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> > > > 
> > > > Missed this last time. Do you use this clkdev lookup? The name is
> > > > usually supposed to be based on what the device is expecting,
> > > > instead of clk_name, and we would want some device name for the
> > > > third argument here.
> > > 
> > > I'm not sure to get your comment. Are you saying the clk_name should be the
> > > third argument?
> > > 
> > > 
> > 
> > Sorry, no. I meant that con_id is typically something like "core"
> > or "ahb" or something like that, and dev_id is something like
> > "a456002.pmic_device" or whatever dev_name(pmic_dev) would return for
> > the consuming device. That way when we call clk_get(dev, "core")
> > it will find the lookup with "core" and "a456002.pmic_device" to
> > match up the clk lookup.
> > 
> > If anything, the clk_name should just go into the con_id for now,
> > and then it will need to be a globally unique identifier for the
> > clk. But that is going against how clkdev is supposed to be used.
> > Hence the question if you even need to use it. If not, just don't
> > add it. I can fix up v3 of this patch to put clk_name back at
> > con_id if you like. No need to resend.
> 
> Ok, I'm not very used with the CCF, so perhaps clk_name is not needed at all. I
> gave a try with the following combination:
> 
>  - con_id = NULL, dev_id = clk_name
>  - con_id = clk_name, dev_id = NULL
>  - con_id = NULL, dev_id = NULL
> 
> All worked.
> 
> And finally I removed the clk_hw_register_clkdev() call and it worked also.
> 
> So I'm not sure about this function.
> 

If you've removed it and it still works, then it means you're
using DT and you don't need clkdev at all. That's the of_clk
provider thing that you're using. So I'll go remove this clkdev
lookup because it's not used. If someone needs it they can add it
later.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web