Re: [PATCH V2] clk: Add composite clock type

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Hi Prashant,

Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Mon, 4 Feb 2013 09:11:22 +0100:

> +struct clk *clk_register_composite(struct device *dev, const char *name,
> +                       const char **parent_names, int num_parents,
> +                       struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> +                       struct clk_hw *div_hw, const struct clk_ops *div_ops,
> +                       struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> +                       unsigned long flags)
> +{
> +       struct clk *clk;
> +       struct clk_init_data init;
> +       struct clk_composite *composite;
> +       struct clk_ops *clk_composite_ops;
> +
> +       composite = kzalloc(sizeof(*composite), GFP_KERNEL);
> +       if (!composite) {
> +               pr_err("%s: could not allocate composite clk\n", __func__);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       init.name = name;
> +       init.flags = flags | CLK_IS_BASIC;
> +       init.parent_names = parent_names;
> +       init.num_parents = num_parents;
> +
> +       /* allocate the clock ops */
> +       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);

The members of "clk_composite_ops" seems to be always assigned
statically. Istead of dynamically allocating/assigning, can't we just
have "clk_composite_ops" statically as below?

static struct clk_ops clk_composite_ops = {
	.get_parent = clk_composite_get_parent;
	.set_parent = clk_composite_set_parent;
	.recalc_rate = clk_composite_recalc_rate;
	.round_rate = clk_composite_round_rate;
	.set_rate = clk_composite_set_rate;
	.is_enabled = clk_composite_is_enabled;
	.enable = clk_composite_enable;
	.disable = clk_composite_disable;
};

struct clk *clk_register_composite(struct device *dev, const char *name,
		       const char **parent_names, int num_parents,
		       struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
		       struct clk_hw *div_hw, const struct clk_ops *div_ops,
		       struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
		       unsigned long flags)
{
	.....

	init.ops = &clk_composite_ops;

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Monday 04 February 2013 03:07 PM, Hiroshi Doyu wrote:
> Hi Prashant,
>
> Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Mon, 4 Feb 2013 09:11:22 +0100:
>
>> +struct clk *clk_register_composite(struct device *dev, const char *name,
>> +                       const char **parent_names, int num_parents,
>> +                       struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
>> +                       struct clk_hw *div_hw, const struct clk_ops *div_ops,
>> +                       struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
>> +                       unsigned long flags)
>> +{
>> +       struct clk *clk;
>> +       struct clk_init_data init;
>> +       struct clk_composite *composite;
>> +       struct clk_ops *clk_composite_ops;
>> +
>> +       composite = kzalloc(sizeof(*composite), GFP_KERNEL);
>> +       if (!composite) {
>> +               pr_err("%s: could not allocate composite clk\n", __func__);
>> +               return ERR_PTR(-ENOMEM);
>> +       }
>> +
>> +       init.name = name;
>> +       init.flags = flags | CLK_IS_BASIC;
>> +       init.parent_names = parent_names;
>> +       init.num_parents = num_parents;
>> +
>> +       /* allocate the clock ops */
>> +       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> The members of "clk_composite_ops" seems to be always assigned
> statically. Istead of dynamically allocating/assigning, can't we just
> have "clk_composite_ops" statically as below?
>
> static struct clk_ops clk_composite_ops = {
> 	.get_parent = clk_composite_get_parent;
> 	.set_parent = clk_composite_set_parent;
> 	.recalc_rate = clk_composite_recalc_rate;
> 	.round_rate = clk_composite_round_rate;
> 	.set_rate = clk_composite_set_rate;
> 	.is_enabled = clk_composite_is_enabled;
> 	.enable = clk_composite_enable;
> 	.disable = clk_composite_disable;
> };
>
> struct clk *clk_register_composite(struct device *dev, const char *name,
> 		       const char **parent_names, int num_parents,
> 		       struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> 		       struct clk_hw *div_hw, const struct clk_ops *div_ops,
> 		       struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> 		       unsigned long flags)
> {
> 	.....
>
> 	init.ops = &clk_composite_ops;

No, clk_ops depends on the clocks you are using. There could be a clock 
with mux and gate while another one with mux and div.

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Tue, 5 Feb 2013 09:33:41 +0100:

> > The members of "clk_composite_ops" seems to be always assigned
> > statically. Istead of dynamically allocating/assigning, can't we just
> > have "clk_composite_ops" statically as below?
> >
> > static struct clk_ops clk_composite_ops = {
> > 	.get_parent = clk_composite_get_parent;
> > 	.set_parent = clk_composite_set_parent;
> > 	.recalc_rate = clk_composite_recalc_rate;
> > 	.round_rate = clk_composite_round_rate;
> > 	.set_rate = clk_composite_set_rate;
> > 	.is_enabled = clk_composite_is_enabled;
> > 	.enable = clk_composite_enable;
> > 	.disable = clk_composite_disable;
> > };
> >
> > struct clk *clk_register_composite(struct device *dev, const char *name,
> > 		       const char **parent_names, int num_parents,
> > 		       struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> > 		       struct clk_hw *div_hw, const struct clk_ops *div_ops,
> > 		       struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
> > 		       unsigned long flags)
> > {
> > 	.....
> >
> > 	init.ops = &clk_composite_ops;
> 
> No, clk_ops depends on the clocks you are using. There could be a clock 
> with mux and gate while another one with mux and div.

You are right. What about the following? We don't have to have similar
copy of clk_composite_ops for each instances.

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index f30fb4b..8f88805 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
        const struct clk_ops *mux_ops = composite->mux_ops;
        struct clk_hw *mux_hw = composite->mux_hw;
 
+       if (!mux_hw->clk)
+	       return -EINVAL;
+
        mux_hw->clk = hw->clk;
 
        return mux_ops->get_parent(mux_hw);
@@ -38,6 +41,9 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
        const struct clk_ops *mux_ops = composite->mux_ops;
        struct clk_hw *mux_hw = composite->mux_hw;
 
+       if (!mux_hw->clk)
+	       return -EINVAL;
+
        mux_hw->clk = hw->clk;
 
        return mux_ops->set_parent(mux_hw, index);
@@ -50,6 +56,9 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
        const struct clk_ops *div_ops = composite->div_ops;
        struct clk_hw *div_hw = composite->div_hw;
 
+       if (!div_hw->clk)
+	       return -EINVAL;
+
        div_hw->clk = hw->clk;
 
        return div_ops->recalc_rate(div_hw, parent_rate);
@@ -62,6 +71,9 @@ static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
        const struct clk_ops *div_ops = composite->div_ops;
        struct clk_hw *div_hw = composite->div_hw;
 
+       if (!div_hw->clk)
+	       return -EINVAL;
+
        div_hw->clk = hw->clk;
 
        return div_ops->round_rate(div_hw, rate, prate);
@@ -74,6 +86,9 @@ static int clk_composite_set_rate(struct clk_hw *hw, unsigned long rate,
        const struct clk_ops *div_ops = composite->div_ops;
        struct clk_hw *div_hw = composite->div_hw;
 
+       if (!div_hw->clk)
+	       return -EINVAL;
+
        div_hw->clk = hw->clk;
 
        return div_ops->set_rate(div_hw, rate, parent_rate);
@@ -85,6 +100,9 @@ static int clk_composite_is_enabled(struct clk_hw *hw)
        const struct clk_ops *gate_ops = composite->gate_ops;
        struct clk_hw *gate_hw = composite->gate_hw;
 
+       if (!gate_hw->clk)
+	       return -EINVAL;
+
        gate_hw->clk = hw->clk;
 
        return gate_ops->is_enabled(gate_hw);
@@ -96,6 +114,9 @@ static int clk_composite_enable(struct clk_hw *hw)
        const struct clk_ops *gate_ops = composite->gate_ops;
        struct clk_hw *gate_hw = composite->gate_hw;
 
+       if (!gate_hw->clk)
+	       return -EINVAL;
+
        gate_hw->clk = hw->clk;
 
        return gate_ops->enable(gate_hw);
@@ -107,11 +128,25 @@ static void clk_composite_disable(struct clk_hw *hw)
        const struct clk_ops *gate_ops = composite->gate_ops;
        struct clk_hw *gate_hw = composite->gate_hw;
 
+       if (!gate_hw->clk)
+	       return -EINVAL;
+
        gate_hw->clk = hw->clk;
 
        gate_ops->disable(gate_hw);
 }
 
+static struct clk_ops clk_composite_ops = {
+	.get_parent = clk_composite_get_parent,
+	.set_parent = clk_composite_set_parent,
+	.recalc_rate = clk_composite_recalc_rate,
+	.round_rate = clk_composite_round_rate,
+	.set_rate = clk_composite_set_rate,
+	.is_enabled = clk_composite_is_enabled,
+	.enable = clk_composite_enable,
+	.disable = clk_composite_disable,
+};
+
 struct clk *clk_register_composite(struct device *dev, const char *name,
                        const char **parent_names, int num_parents,
                        struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
@@ -135,14 +170,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
        init.parent_names = parent_names;
        init.num_parents = num_parents;
 
-       /* allocate the clock ops */
-       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
-       if (!clk_composite_ops) {
-               pr_err("%s: could not allocate clk ops\n", __func__);
-               kfree(composite);
-               return ERR_PTR(-ENOMEM);
-       }
-
        if (mux_hw && mux_ops) {
                if (!mux_ops->get_parent || !mux_ops->set_parent) {
                        clk = ERR_PTR(-EINVAL);
@@ -151,8 +178,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
                composite->mux_hw = mux_hw;
                composite->mux_ops = mux_ops;
-               clk_composite_ops->get_parent = clk_composite_get_parent;
-               clk_composite_ops->set_parent = clk_composite_set_parent;
        }
 
        if (div_hw && div_ops) {
@@ -164,9 +189,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
                composite->div_hw = div_hw;
                composite->div_ops = div_ops;
-               clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
-               clk_composite_ops->round_rate = clk_composite_round_rate;
-               clk_composite_ops->set_rate = clk_composite_set_rate;
        }
 
        if (gate_hw && gate_ops) {
@@ -178,12 +200,9 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
                composite->gate_hw = gate_hw;
                composite->gate_ops = gate_ops;
-               clk_composite_ops->is_enabled = clk_composite_is_enabled;
-               clk_composite_ops->enable = clk_composite_enable;
-               clk_composite_ops->disable = clk_composite_disable;
        }
 
-       init.ops = clk_composite_ops;
+       init.ops = &clk_composite_ops;
        composite->hw.init = &init;
 
        clk = clk_register(dev, &composite->hw);
@@ -202,7 +221,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
        return clk;
 
 err:
-       kfree(clk_composite_ops);
        kfree(composite);
        return clk;
 }

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

On Tuesday 05 of February 2013 11:22:52 Hiroshi Doyu wrote:
> Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Tue, 5 Feb 2013 09:33:41 
+0100:
> > > The members of "clk_composite_ops" seems to be always assigned
> > > statically. Istead of dynamically allocating/assigning, can't we
> > > just
> > > have "clk_composite_ops" statically as below?
> > > 
> > > static struct clk_ops clk_composite_ops = {
> > > 
> > > 	.get_parent = clk_composite_get_parent;
> > > 	.set_parent = clk_composite_set_parent;
> > > 	.recalc_rate = clk_composite_recalc_rate;
> > > 	.round_rate = clk_composite_round_rate;
> > > 	.set_rate = clk_composite_set_rate;
> > > 	.is_enabled = clk_composite_is_enabled;
> > > 	.enable = clk_composite_enable;
> > > 	.disable = clk_composite_disable;
> > > 
> > > };
> > > 
> > > struct clk *clk_register_composite(struct device *dev, const char
> > > *name,> > 
> > > 		       const char **parent_names, int num_parents,
> > > 		       struct clk_hw *mux_hw, const struct clk_ops 
*mux_ops,
> > > 		       struct clk_hw *div_hw, const struct clk_ops 
*div_ops,
> > > 		       struct clk_hw *gate_hw, const struct clk_ops 
*gate_ops,
> > > 		       unsigned long flags)
> > > 
> > > {
> > > 
> > > 	.....
> > > 	
> > > 	init.ops = &clk_composite_ops;
> > 
> > No, clk_ops depends on the clocks you are using. There could be a
> > clock
> > with mux and gate while another one with mux and div.
> 
> You are right. What about the following? We don't have to have similar
> copy of clk_composite_ops for each instances.
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..8f88805 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>         const struct clk_ops *mux_ops = composite->mux_ops;
>         struct clk_hw *mux_hw = composite->mux_hw;
> 
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +

Or maybe even:

		if (!mux_ops)

This would be more self-explanatory and save one dereference.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH V2] clk: Add composite clock type

[Russell King - ARM Linux]

On Tue, Feb 05, 2013 at 11:22:52AM +0100, Hiroshi Doyu wrote:
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..8f88805 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>         const struct clk_ops *mux_ops = composite->mux_ops;
>         struct clk_hw *mux_hw = composite->mux_hw;
>  
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +
>         mux_hw->clk = hw->clk;

That just looks totally wrong.

Firstly, according to the hunk, this function has the prototype:

static u8 clk_composite_get_parent(struct clk_hw *hw)

What do you think is the effect of passing -EINVAL back as a 'u8' ?

Secondly, the whole "check mux_hw->clk for NULL, and then overwrite it"
looks really really really wrong.  If it's already set, then why does it
need to be changed?  If it isn't set, why do you need to error out?

Thirdly, why is a function called "get_parent" modifying anything (isn't
it supposed to be _get_ing something?

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Tuesday 05 February 2013 03:52 PM, Hiroshi Doyu wrote:
> Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Tue, 5 Feb 2013 09:33:41 +0100:
>
>>> The members of "clk_composite_ops" seems to be always assigned
>>> statically. Istead of dynamically allocating/assigning, can't we just
>>> have "clk_composite_ops" statically as below?
>>>
>>> static struct clk_ops clk_composite_ops = {
>>> 	.get_parent = clk_composite_get_parent;
>>> 	.set_parent = clk_composite_set_parent;
>>> 	.recalc_rate = clk_composite_recalc_rate;
>>> 	.round_rate = clk_composite_round_rate;
>>> 	.set_rate = clk_composite_set_rate;
>>> 	.is_enabled = clk_composite_is_enabled;
>>> 	.enable = clk_composite_enable;
>>> 	.disable = clk_composite_disable;
>>> };
>>>
>>> struct clk *clk_register_composite(struct device *dev, const char *name,
>>> 		       const char **parent_names, int num_parents,
>>> 		       struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
>>> 		       struct clk_hw *div_hw, const struct clk_ops *div_ops,
>>> 		       struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
>>> 		       unsigned long flags)
>>> {
>>> 	.....
>>>
>>> 	init.ops = &clk_composite_ops;
>> No, clk_ops depends on the clocks you are using. There could be a clock
>> with mux and gate while another one with mux and div.
> You are right. What about the following? We don't have to have similar
> copy of clk_composite_ops for each instances.

Clock framework takes decision depending on the ops availability and it 
does not know if the clock is mux or gate.

For example,

                 if (clk->ops->enable) {
                         ret = clk->ops->enable(clk->hw);
                         if (ret) {
                                 __clk_disable(clk->parent);
                                 return ret;
                         }
                 }

in above case if clk_composite does not have gate clock then as per your 
suggestion if it returns error value then it will fail and it is wrong.

Hence clock ops are populated depending on the clock types.

> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..8f88805 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>          const struct clk_ops *mux_ops = composite->mux_ops;
>          struct clk_hw *mux_hw = composite->mux_hw;
>   
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +
>          mux_hw->clk = hw->clk;
>   

It is wrong.

>          return mux_ops->get_parent(mux_hw);
> @@ -38,6 +41,9 @@ static int clk_composite_set_parent(struct clk_hw *hw, u8 index)
>          const struct clk_ops *mux_ops = composite->mux_ops;
>          struct clk_hw *mux_hw = composite->mux_hw;
>   
> +       if (!mux_hw->clk)
> +	       return -EINVAL;
> +
>          mux_hw->clk = hw->clk;
>   
>          return mux_ops->set_parent(mux_hw, index);
> @@ -50,6 +56,9 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
>          const struct clk_ops *div_ops = composite->div_ops;
>          struct clk_hw *div_hw = composite->div_hw;
>   
> +       if (!div_hw->clk)
> +	       return -EINVAL;
> +
>          div_hw->clk = hw->clk;
>   
>          return div_ops->recalc_rate(div_hw, parent_rate);
> @@ -62,6 +71,9 @@ static long clk_composite_round_rate(struct clk_hw *hw, unsigned long rate,
>          const struct clk_ops *div_ops = composite->div_ops;
>          struct clk_hw *div_hw = composite->div_hw;
>   
> +       if (!div_hw->clk)
> +	       return -EINVAL;
> +
>          div_hw->clk = hw->clk;
>   
>          return div_ops->round_rate(div_hw, rate, prate);
> @@ -74,6 +86,9 @@ static int clk_composite_set_rate(struct clk_hw *hw, unsigned long rate,
>          const struct clk_ops *div_ops = composite->div_ops;
>          struct clk_hw *div_hw = composite->div_hw;
>   
> +       if (!div_hw->clk)
> +	       return -EINVAL;
> +
>          div_hw->clk = hw->clk;
>   
>          return div_ops->set_rate(div_hw, rate, parent_rate);
> @@ -85,6 +100,9 @@ static int clk_composite_is_enabled(struct clk_hw *hw)
>          const struct clk_ops *gate_ops = composite->gate_ops;
>          struct clk_hw *gate_hw = composite->gate_hw;
>   
> +       if (!gate_hw->clk)
> +	       return -EINVAL;
> +
>          gate_hw->clk = hw->clk;
>   
>          return gate_ops->is_enabled(gate_hw);
> @@ -96,6 +114,9 @@ static int clk_composite_enable(struct clk_hw *hw)
>          const struct clk_ops *gate_ops = composite->gate_ops;
>          struct clk_hw *gate_hw = composite->gate_hw;
>   
> +       if (!gate_hw->clk)
> +	       return -EINVAL;
> +
>          gate_hw->clk = hw->clk;
>   
>          return gate_ops->enable(gate_hw);
> @@ -107,11 +128,25 @@ static void clk_composite_disable(struct clk_hw *hw)
>          const struct clk_ops *gate_ops = composite->gate_ops;
>          struct clk_hw *gate_hw = composite->gate_hw;
>   
> +       if (!gate_hw->clk)
> +	       return -EINVAL;
> +
>          gate_hw->clk = hw->clk;
>   
>          gate_ops->disable(gate_hw);
>   }
>   
> +static struct clk_ops clk_composite_ops = {
> +	.get_parent = clk_composite_get_parent,
> +	.set_parent = clk_composite_set_parent,
> +	.recalc_rate = clk_composite_recalc_rate,
> +	.round_rate = clk_composite_round_rate,
> +	.set_rate = clk_composite_set_rate,
> +	.is_enabled = clk_composite_is_enabled,
> +	.enable = clk_composite_enable,
> +	.disable = clk_composite_disable,
> +};
> +
>   struct clk *clk_register_composite(struct device *dev, const char *name,
>                          const char **parent_names, int num_parents,
>                          struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
> @@ -135,14 +170,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>          init.parent_names = parent_names;
>          init.num_parents = num_parents;
>   
> -       /* allocate the clock ops */
> -       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> -       if (!clk_composite_ops) {
> -               pr_err("%s: could not allocate clk ops\n", __func__);
> -               kfree(composite);
> -               return ERR_PTR(-ENOMEM);
> -       }
> -
>          if (mux_hw && mux_ops) {
>                  if (!mux_ops->get_parent || !mux_ops->set_parent) {
>                          clk = ERR_PTR(-EINVAL);
> @@ -151,8 +178,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>   
>                  composite->mux_hw = mux_hw;
>                  composite->mux_ops = mux_ops;
> -               clk_composite_ops->get_parent = clk_composite_get_parent;
> -               clk_composite_ops->set_parent = clk_composite_set_parent;
>          }
>   
>          if (div_hw && div_ops) {
> @@ -164,9 +189,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>   
>                  composite->div_hw = div_hw;
>                  composite->div_ops = div_ops;
> -               clk_composite_ops->recalc_rate = clk_composite_recalc_rate;
> -               clk_composite_ops->round_rate = clk_composite_round_rate;
> -               clk_composite_ops->set_rate = clk_composite_set_rate;
>          }
>   
>          if (gate_hw && gate_ops) {
> @@ -178,12 +200,9 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>   
>                  composite->gate_hw = gate_hw;
>                  composite->gate_ops = gate_ops;
> -               clk_composite_ops->is_enabled = clk_composite_is_enabled;
> -               clk_composite_ops->enable = clk_composite_enable;
> -               clk_composite_ops->disable = clk_composite_disable;
>          }
>   
> -       init.ops = clk_composite_ops;
> +       init.ops = &clk_composite_ops;
>          composite->hw.init = &init;
>   
>          clk = clk_register(dev, &composite->hw);
> @@ -202,7 +221,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>          return clk;
>   
>   err:
> -       kfree(clk_composite_ops);
>          kfree(composite);
>          return clk;
>   }

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Wed, 6 Feb 2013 03:55:00 +0100:

> >> No, clk_ops depends on the clocks you are using. There could be a clock
> >> with mux and gate while another one with mux and div.
> > You are right. What about the following? We don't have to have similar
> > copy of clk_composite_ops for each instances.
> 
> Clock framework takes decision depending on the ops availability and it 
> does not know if the clock is mux or gate.
> 
> For example,
> 
>                  if (clk->ops->enable) {
>                          ret = clk->ops->enable(clk->hw);
>                          if (ret) {
>                                  __clk_disable(clk->parent);
>                                  return ret;
>                          }
>                  }
> 
> in above case if clk_composite does not have gate clock then as per your 
> suggestion if it returns error value then it will fail and it is wrong.

Ok, now I understand. Thank you for explanation.

We always need to allocate clk_composite_ops for each clk_composite,
right? If so what about having "struct clk_ops ops" in "struct
clk_composite"?

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index f30fb4b..5240e24 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -129,20 +129,13 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
                pr_err("%s: could not allocate composite clk\n", __func__);
                return ERR_PTR(-ENOMEM);
        }
+       clk_composite_ops = &composite->ops;
 
        init.name = name;
        init.flags = flags | CLK_IS_BASIC;
        init.parent_names = parent_names;
        init.num_parents = num_parents;
 
-       /* allocate the clock ops */
-       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
-       if (!clk_composite_ops) {
-               pr_err("%s: could not allocate clk ops\n", __func__);
-               kfree(composite);
-               return ERR_PTR(-ENOMEM);
-       }
-
        if (mux_hw && mux_ops) {
                if (!mux_ops->get_parent || !mux_ops->set_parent) {
                        clk = ERR_PTR(-EINVAL);
@@ -202,7 +195,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
        return clk;
 
 err:
-       kfree(clk_composite_ops);
        kfree(composite);
        return clk;
 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f0ac818..bb5d36a 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -346,6 +346,8 @@ struct clk_composite {
        const struct clk_ops    *mux_ops;
        const struct clk_ops    *div_ops;
        const struct clk_ops    *gate_ops;
+
+       const struct clk_ops    ops;
 };
 
 struct clk *clk_register_composite(struct device *dev, const char *name,


> > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> > index f30fb4b..8f88805 100644
> > --- a/drivers/clk/clk-composite.c
> > +++ b/drivers/clk/clk-composite.c
> > @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
> >          const struct clk_ops *mux_ops = composite->mux_ops;
> >          struct clk_hw *mux_hw = composite->mux_hw;
> >   
> > +       if (!mux_hw->clk)
> > +	       return -EINVAL;
> > +
> >          mux_hw->clk = hw->clk;
>
> It is wrong.

Will the above "mux_hw->clk = hw->clk" be removed from the original?

Re: [PATCH V2] clk: Add composite clock type

[Prashant Gaikwad]

On Wednesday 06 February 2013 11:40 AM, Hiroshi Doyu wrote:
> Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Wed, 6 Feb 2013 03:55:00 +0100:
>
>>>> No, clk_ops depends on the clocks you are using. There could be a clock
>>>> with mux and gate while another one with mux and div.
>>> You are right. What about the following? We don't have to have similar
>>> copy of clk_composite_ops for each instances.
>> Clock framework takes decision depending on the ops availability and it
>> does not know if the clock is mux or gate.
>>
>> For example,
>>
>>                   if (clk->ops->enable) {
>>                           ret = clk->ops->enable(clk->hw);
>>                           if (ret) {
>>                                   __clk_disable(clk->parent);
>>                                   return ret;
>>                           }
>>                   }
>>
>> in above case if clk_composite does not have gate clock then as per your
>> suggestion if it returns error value then it will fail and it is wrong.
> Ok, now I understand. Thank you for explanation.
>
> We always need to allocate clk_composite_ops for each clk_composite,
> right? If so what about having "struct clk_ops ops" in "struct
> clk_composite"?
>
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index f30fb4b..5240e24 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -129,20 +129,13 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>                  pr_err("%s: could not allocate composite clk\n", __func__);
>                  return ERR_PTR(-ENOMEM);
>          }
> +       clk_composite_ops = &composite->ops;
>   
>          init.name = name;
>          init.flags = flags | CLK_IS_BASIC;
>          init.parent_names = parent_names;
>          init.num_parents = num_parents;
>   
> -       /* allocate the clock ops */
> -       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops), GFP_KERNEL);
> -       if (!clk_composite_ops) {
> -               pr_err("%s: could not allocate clk ops\n", __func__);
> -               kfree(composite);
> -               return ERR_PTR(-ENOMEM);
> -       }
> -
>          if (mux_hw && mux_ops) {
>                  if (!mux_ops->get_parent || !mux_ops->set_parent) {
>                          clk = ERR_PTR(-EINVAL);
> @@ -202,7 +195,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>          return clk;
>   
>   err:
> -       kfree(clk_composite_ops);
>          kfree(composite);
>          return clk;
>   }
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index f0ac818..bb5d36a 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -346,6 +346,8 @@ struct clk_composite {
>          const struct clk_ops    *mux_ops;
>          const struct clk_ops    *div_ops;
>          const struct clk_ops    *gate_ops;
> +
> +       const struct clk_ops    ops;
>   };
>   
>   struct clk *clk_register_composite(struct device *dev, const char *name,

This will work, but there is no harm in allocating dynamically. What is 
preferred?

>
>>> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
>>> index f30fb4b..8f88805 100644
>>> --- a/drivers/clk/clk-composite.c
>>> +++ b/drivers/clk/clk-composite.c
>>> @@ -27,6 +27,9 @@ static u8 clk_composite_get_parent(struct clk_hw *hw)
>>>           const struct clk_ops *mux_ops = composite->mux_ops;
>>>           struct clk_hw *mux_hw = composite->mux_hw;
>>>    
>>> +       if (!mux_hw->clk)
>>> +	       return -EINVAL;
>>> +
>>>           mux_hw->clk = hw->clk;
>> It is wrong.
> Will the above "mux_hw->clk = hw->clk" be removed from the original?

Re: [PATCH V2] clk: Add composite clock type

[Hiroshi Doyu]

Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Wed, 6 Feb 2013 10:52:54 +0100:

> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index f0ac818..bb5d36a 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -346,6 +346,8 @@ struct clk_composite {
> >          const struct clk_ops    *mux_ops;
> >          const struct clk_ops    *div_ops;
> >          const struct clk_ops    *gate_ops;
> > +
> > +       const struct clk_ops    ops;
> >   };
> >   
> >   struct clk *clk_register_composite(struct device *dev, const char *name,
> 
> This will work, but there is no harm in allocating dynamically. What is 
> preferred?

If we've already know that this "ops" is necessary per "struct
clk_composite" in advance, there's no point to allocate
"clk_composite" and "ops" separately.

Re: [PATCH V2] clk: Add composite clock type

[Tomasz Figa]

On Wednesday 06 of February 2013 15:22:54 Prashant Gaikwad wrote:
> On Wednesday 06 February 2013 11:40 AM, Hiroshi Doyu wrote:
> > Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Wed, 6 Feb 2013 
03:55:00 +0100:
> >>>> No, clk_ops depends on the clocks you are using. There could be a
> >>>> clock
> >>>> with mux and gate while another one with mux and div.
> >>> 
> >>> You are right. What about the following? We don't have to have
> >>> similar
> >>> copy of clk_composite_ops for each instances.
> >> 
> >> Clock framework takes decision depending on the ops availability and
> >> it
> >> does not know if the clock is mux or gate.
> >> 
> >> For example,
> >> 
> >>                   if (clk->ops->enable) {
> >>                   
> >>                           ret = clk->ops->enable(clk->hw);
> >>                           if (ret) {
> >>                           
> >>                                   __clk_disable(clk->parent);
> >>                                   return ret;
> >>                           
> >>                           }
> >>                   
> >>                   }
> >> 
> >> in above case if clk_composite does not have gate clock then as per
> >> your suggestion if it returns error value then it will fail and it
> >> is wrong.> 
> > Ok, now I understand. Thank you for explanation.
> > 
> > We always need to allocate clk_composite_ops for each clk_composite,
> > right? If so what about having "struct clk_ops ops" in "struct
> > clk_composite"?
> > 
> > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> > index f30fb4b..5240e24 100644
> > --- a/drivers/clk/clk-composite.c
> > +++ b/drivers/clk/clk-composite.c
> > @@ -129,20 +129,13 @@ struct clk *clk_register_composite(struct device
> > *dev, const char *name,> 
> >                  pr_err("%s: could not allocate composite clk\n",
> >                  __func__);
> >                  return ERR_PTR(-ENOMEM);
> >          
> >          }
> > 
> > +       clk_composite_ops = &composite->ops;
> > 
> >          init.name = name;
> >          init.flags = flags | CLK_IS_BASIC;
> >          init.parent_names = parent_names;
> >          init.num_parents = num_parents;
> > 
> > -       /* allocate the clock ops */
> > -       clk_composite_ops = kzalloc(sizeof(*clk_composite_ops),
> > GFP_KERNEL); -       if (!clk_composite_ops) {
> > -               pr_err("%s: could not allocate clk ops\n", __func__);
> > -               kfree(composite);
> > -               return ERR_PTR(-ENOMEM);
> > -       }
> > -
> > 
> >          if (mux_hw && mux_ops) {
> >          
> >                  if (!mux_ops->get_parent || !mux_ops->set_parent) {
> >                  
> >                          clk = ERR_PTR(-EINVAL);
> > 
> > @@ -202,7 +195,6 @@ struct clk *clk_register_composite(struct device
> > *dev, const char *name,> 
> >          return clk;
> >   
> >   err:
> > -       kfree(clk_composite_ops);
> > 
> >          kfree(composite);
> >          return clk;
> >   
> >   }
> > 
> > diff --git a/include/linux/clk-provider.h
> > b/include/linux/clk-provider.h index f0ac818..bb5d36a 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -346,6 +346,8 @@ struct clk_composite {
> > 
> >          const struct clk_ops    *mux_ops;
> >          const struct clk_ops    *div_ops;
> >          const struct clk_ops    *gate_ops;
> > 
> > +
> > +       const struct clk_ops    ops;
> > 
> >   };
> >   
> >   struct clk *clk_register_composite(struct device *dev, const char
> >   *name,
> This will work, but there is no harm in allocating dynamically. What is
> preferred?

IMHO it is always better to allocate one bigger structure than several 
smaller if they are always needed together and one cannot exist without 
others.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform