diff mbox

[v5,05/14] clk: Add generic driver for Maxim PMIC clocks

Message ID 1403806546-31122-6-git-send-email-javier.martinez@collabora.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Javier Martinez Canillas June 26, 2014, 6:15 p.m. UTC
Maxim Integrated Power Management ICs are very similar with
regard to their clock outputs. Most of the clock drivers for
these chips are duplicating code and are simpler enough that
can be converted to use a generic driver to consolidate code
and avoid duplication.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---

Changes since v4:
 - Return recalc 0 if clock isn't enabled in Suggested by Yadwinder Singh Brar.

Changes since v3:
 - Add current copyright information. Suggested by Krzysztof Kozlowski
 - Do a single allocation for struct max_gen_clk. Suggested by Krzysztof Kozlowski
 - Add EXPORT_SYMBOL() for exported symbols. Suggested by Krzysztof Kozlowski

 drivers/clk/Kconfig       |   3 +
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-max-gen.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-max-gen.h |  32 ++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 drivers/clk/clk-max-gen.c
 create mode 100644 drivers/clk/clk-max-gen.h

Comments

Yadwinder Singh Brar June 30, 2014, 4:01 a.m. UTC | #1
Hi Javier,

On Thu, Jun 26, 2014 at 11:45 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Maxim Integrated Power Management ICs are very similar with
> regard to their clock outputs. Most of the clock drivers for
> these chips are duplicating code and are simpler enough that
> can be converted to use a generic driver to consolidate code
> and avoid duplication.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>
> Changes since v4:
>  - Return recalc 0 if clock isn't enabled in Suggested by Yadwinder Singh Brar.
>

It seems you didn't implement or posted same patch again :) .

> Changes since v3:
>  - Add current copyright information. Suggested by Krzysztof Kozlowski
>  - Do a single allocation for struct max_gen_clk. Suggested by Krzysztof Kozlowski
>  - Add EXPORT_SYMBOL() for exported symbols. Suggested by Krzysztof Kozlowski
>
>  drivers/clk/Kconfig       |   3 +
>  drivers/clk/Makefile      |   1 +
>  drivers/clk/clk-max-gen.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk-max-gen.h |  32 ++++++++
>  4 files changed, 231 insertions(+)
>  create mode 100644 drivers/clk/clk-max-gen.c
>  create mode 100644 drivers/clk/clk-max-gen.h
>

[ .. ]

> +
> +static unsigned long max_gen_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       return 32768;
> +}

Its still same here.

> +
> +struct clk_ops max_gen_clk_ops = {
> +       .prepare        = max_gen_clk_prepare,
> +       .unprepare      = max_gen_clk_unprepare,
> +       .is_prepared    = max_gen_clk_is_prepared,
> +       .recalc_rate    = max_gen_recalc_rate,
> +};
> +EXPORT_SYMBOL_GPL(max_gen_clk_ops);
> +
> +static struct clk *max_gen_clk_register(struct device *dev,
> +                                       struct max_gen_clk *max_gen)
> +{
> +       struct clk *clk;
> +       struct clk_hw *hw = &max_gen->hw;
> +
> +       clk = clk_register(dev, hw);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       max_gen->lookup = kzalloc(sizeof(struct clk_lookup), GFP_KERNEL);

As I suggested in other patch[1] also, its better to use
clkdev_alloc() instead of kzalloc() here.

> +       if (!max_gen->lookup)
> +               return ERR_PTR(-ENOMEM);
> +
> +       max_gen->lookup->con_id = hw->init->name;

Also IMO,  init->name should be over-written if name is provided in DT,
otherwise generic "clock-output-names" property will go futile,
perhaps it should be done before clk_register.

Regards,
Yadwinder

[1] : https://lkml.org/lkml/2014/6/27/197

> +       max_gen->lookup->clk = clk;
> +
> +       clkdev_add(max_gen->lookup);
> +
> +       return clk;
> +}
> +
> +int max_gen_clk_probe(struct platform_device *pdev, struct regmap *regmap,
> +                     u32 reg, struct clk_init_data *clks_init, int num_init)
> +{
> +       int i, ret;
> +       struct max_gen_clk *max_gen_clks;
> +       struct clk **clocks;
> +       struct device *dev = &pdev->dev;
> +
> +       clocks = devm_kzalloc(dev, sizeof(struct clk *) * num_init, GFP_KERNEL);
> +       if (!clocks)
> +               return -ENOMEM;
> +
> +       max_gen_clks = devm_kzalloc(dev, sizeof(struct max_gen_clk)
> +                                   * num_init, GFP_KERNEL);
> +       if (!max_gen_clks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < num_init; i++) {
> +               max_gen_clks[i].regmap = regmap;
> +               max_gen_clks[i].mask = 1 << i;
> +               max_gen_clks[i].reg = reg;
> +               max_gen_clks[i].hw.init = &clks_init[i];
> +
> +               clocks[i] = max_gen_clk_register(dev, &max_gen_clks[i]);
> +               if (IS_ERR(clocks[i])) {
> +                       ret = PTR_ERR(clocks[i]);
> +                       dev_err(dev, "failed to register %s\n",
> +                               max_gen_clks[i].hw.init->name);
> +                       goto err_clocks;
> +               }
> +       }
> +
> +       platform_set_drvdata(pdev, clocks);
> +
> +       if (dev->of_node) {
> +               struct clk_onecell_data *of_data;
> +
> +               of_data = devm_kzalloc(dev, sizeof(*of_data), GFP_KERNEL);
> +               if (!of_data) {
> +                       ret = -ENOMEM;
> +                       goto err_clocks;
> +               }
> +
> +               of_data->clks = clocks;
> +               of_data->clk_num = num_init;
> +               ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
> +                                         of_data);
> +
> +               if (ret) {
> +                       dev_err(dev, "failed to register OF clock provider\n");
> +                       goto err_clocks;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_clocks:
> +       for (--i; i >= 0; --i) {
> +               clkdev_drop(max_gen_clks[i].lookup);
> +               clk_unregister(max_gen_clks[i].hw.clk);
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(max_gen_clk_probe);
> +
> +int max_gen_clk_remove(struct platform_device *pdev, int num_init)
> +{
> +       struct clk **clocks = platform_get_drvdata(pdev);
> +       struct device *dev = pdev->dev.parent;
> +       int i;
> +
> +       if (dev->of_node)
> +               of_clk_del_provider(dev->of_node);
> +
> +       for (i = 0; i < num_init; i++) {
> +               struct clk_hw *hw = __clk_get_hw(clocks[i]);
> +               struct max_gen_clk *max_gen = to_max_gen_clk(hw);
> +
> +               clkdev_drop(max_gen->lookup);
> +               clk_unregister(clocks[i]);
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(max_gen_clk_remove);
> diff --git a/drivers/clk/clk-max-gen.h b/drivers/clk/clk-max-gen.h
> new file mode 100644
> index 0000000..997e86f
> --- /dev/null
> +++ b/drivers/clk/clk-max-gen.h
> @@ -0,0 +1,32 @@
> +/*
> + * clk-max-gen.h - Generic clock driver for Maxim PMICs clocks
> + *
> + * Copyright (C) 2014 Google, Inc
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __CLK_MAX_GEN_H__
> +#define __CLK_MAX_GEN_H__
> +
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/clkdev.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_device.h>
> +
> +int max_gen_clk_probe(struct platform_device *pdev, struct regmap *regmap,
> +                     u32 reg, struct clk_init_data *clks_init, int num_init);
> +int max_gen_clk_remove(struct platform_device *pdev, int num_init);
> +extern struct clk_ops max_gen_clk_ops;
> +
> +#endif /* __CLK_MAX_GEN_H__ */
> --
> 2.0.0.rc2
>
Javier Martinez Canillas June 30, 2014, 10:58 a.m. UTC | #2
Hello Yadwinder,

Thanks a lot for your feedback.

On 06/30/2014 06:01 AM, Yadwinder Singh Brar wrote:
> Hi Javier,
> 
> On Thu, Jun 26, 2014 at 11:45 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> Maxim Integrated Power Management ICs are very similar with
>> regard to their clock outputs. Most of the clock drivers for
>> these chips are duplicating code and are simpler enough that
>> can be converted to use a generic driver to consolidate code
>> and avoid duplication.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
>>
>> Changes since v4:
>>  - Return recalc 0 if clock isn't enabled in Suggested by Yadwinder Singh Brar.
>>
> 
> It seems you didn't implement or posted same patch again :) .
> 

Yeah, I did implement it but seems I was sleepy when I posted the series since I
managed to completely screw up the patch-set... More on that below.

>> Changes since v3:
>>  - Add current copyright information. Suggested by Krzysztof Kozlowski
>>  - Do a single allocation for struct max_gen_clk. Suggested by Krzysztof Kozlowski
>>  - Add EXPORT_SYMBOL() for exported symbols. Suggested by Krzysztof Kozlowski
>>
>>  drivers/clk/Kconfig       |   3 +
>>  drivers/clk/Makefile      |   1 +
>>  drivers/clk/clk-max-gen.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/clk-max-gen.h |  32 ++++++++
>>  4 files changed, 231 insertions(+)
>>  create mode 100644 drivers/clk/clk-max-gen.c
>>  create mode 100644 drivers/clk/clk-max-gen.h
>>
> 
> [ .. ]
> 
>> +
>> +static unsigned long max_gen_recalc_rate(struct clk_hw *hw,
>> +                                        unsigned long parent_rate)
>> +{
>> +       return 32768;
>> +}
> 
> Its still same here.
> 

Instead of squashing the delta in this patch I did on "[PATCH v4 05/14] clk: Add
generic driver for Maxim PMIC clocks" [0] so you can look the
max_gen_recalc_rate() on that patch.

I made the same mistake when squashing the mfd changes into the patch adding the
regulator driver [1] :-(

Sorry for the mess... I'll fix that for the next version.

>> +
>> +struct clk_ops max_gen_clk_ops = {
>> +       .prepare        = max_gen_clk_prepare,
>> +       .unprepare      = max_gen_clk_unprepare,
>> +       .is_prepared    = max_gen_clk_is_prepared,
>> +       .recalc_rate    = max_gen_recalc_rate,
>> +};
>> +EXPORT_SYMBOL_GPL(max_gen_clk_ops);
>> +
>> +static struct clk *max_gen_clk_register(struct device *dev,
>> +                                       struct max_gen_clk *max_gen)
>> +{
>> +       struct clk *clk;
>> +       struct clk_hw *hw = &max_gen->hw;
>> +
>> +       clk = clk_register(dev, hw);
>> +       if (IS_ERR(clk))
>> +               return clk;
>> +
>> +       max_gen->lookup = kzalloc(sizeof(struct clk_lookup), GFP_KERNEL);
> 
> As I suggested in other patch[1] also, its better to use
> clkdev_alloc() instead of kzalloc() here.
> 

Perfect, I'll do it on the next version.

>> +       if (!max_gen->lookup)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       max_gen->lookup->con_id = hw->init->name;
> 
> Also IMO,  init->name should be over-written if name is provided in DT,
> otherwise generic "clock-output-names" property will go futile,
> perhaps it should be done before clk_register.
> 

Even though Documentation/devicetree/bindings/clock/clock-bindings.txt says that
the "clock-output-names" property is optional I agree with you that will be
better to support it. So I'll add it on the next version as well.

> Regards,
> Yadwinder
> 

Best regards,
Javier

[0]: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg33085.html
[1]: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg33168.html
Russell King - ARM Linux June 30, 2014, 11:35 a.m. UTC | #3
On Mon, Jun 30, 2014 at 12:58:57PM +0200, Javier Martinez Canillas wrote:
> >> +       if (!max_gen->lookup)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       max_gen->lookup->con_id = hw->init->name;
> > 
> > Also IMO,  init->name should be over-written if name is provided in DT,
> > otherwise generic "clock-output-names" property will go futile,
> > perhaps it should be done before clk_register.
> > 
> 
> Even though Documentation/devicetree/bindings/clock/clock-bindings.txt says that
> the "clock-output-names" property is optional I agree with you that will be
> better to support it. So I'll add it on the next version as well.

However, remember that con_id is the _DEVICE_ specific connection name,
not the _CLOCK_ name.  You will get a NAK from me if you violate this
rule.
Javier Martinez Canillas June 30, 2014, 4 p.m. UTC | #4
Hello Russell,

Thanks a lot for your suggestion.

On 06/30/2014 01:35 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 30, 2014 at 12:58:57PM +0200, Javier Martinez Canillas wrote:
>> >> +       if (!max_gen->lookup)
>> >> +               return ERR_PTR(-ENOMEM);
>> >> +
>> >> +       max_gen->lookup->con_id = hw->init->name;
>> > 
>> > Also IMO,  init->name should be over-written if name is provided in DT,
>> > otherwise generic "clock-output-names" property will go futile,
>> > perhaps it should be done before clk_register.
>> > 
>> 
>> Even though Documentation/devicetree/bindings/clock/clock-bindings.txt says that
>> the "clock-output-names" property is optional I agree with you that will be
>> better to support it. So I'll add it on the next version as well.
> 
> However, remember that con_id is the _DEVICE_ specific connection name,
> not the _CLOCK_ name.  You will get a NAK from me if you violate this
> rule.
> 

Yes I know that con_id is the device specific connection name that is set by the
consumers using the "clock-names" property. But AFAIU the clk_lookup structs
added by clockdev_add() are only used by legacy non-DT drivers which don't set
their clock names and instead is the clock driver the one that set con_id to the
struct clk_init_data .name field.

I see that other drivers do the same, is that wrong as well? What value should
have con_id when registering clkdevs for legacy non-DT drivers?

DT enabled drivers whose devices define their "clock-names" are not affected by
this since the clock lookup is made by matching OF "clock-names" property + and
index.

Best regards,
Javier
Mike Turquette July 1, 2014, 5:26 p.m. UTC | #5
Quoting Yadwinder Singh Brar (2014-06-29 21:01:36)
> Hi Javier,
> 
> On Thu, Jun 26, 2014 at 11:45 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
> > Maxim Integrated Power Management ICs are very similar with
> > regard to their clock outputs. Most of the clock drivers for
> > these chips are duplicating code and are simpler enough that
> > can be converted to use a generic driver to consolidate code
> > and avoid duplication.
> >
> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >
> > Changes since v4:
> >  - Return recalc 0 if clock isn't enabled in Suggested by Yadwinder Singh Brar.
> >
> 
> It seems you didn't implement or posted same patch again :) .
> 
> > Changes since v3:
> >  - Add current copyright information. Suggested by Krzysztof Kozlowski
> >  - Do a single allocation for struct max_gen_clk. Suggested by Krzysztof Kozlowski
> >  - Add EXPORT_SYMBOL() for exported symbols. Suggested by Krzysztof Kozlowski
> >
> >  drivers/clk/Kconfig       |   3 +
> >  drivers/clk/Makefile      |   1 +
> >  drivers/clk/clk-max-gen.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/clk-max-gen.h |  32 ++++++++
> >  4 files changed, 231 insertions(+)
> >  create mode 100644 drivers/clk/clk-max-gen.c
> >  create mode 100644 drivers/clk/clk-max-gen.h
> >
> 
> [ .. ]
> 
> > +
> > +static unsigned long max_gen_recalc_rate(struct clk_hw *hw,
> > +                                        unsigned long parent_rate)
> > +{
> > +       return 32768;
> > +}
> 
> Its still same here.

Changing this would be a new behavior. I do not know of any other clock
drivers that conditionally returns a rate of 0 based on whether or not
the clock is gated.

It is also buggy since calls to clk_enable and clk_disable do not invoke
.recalc_rate, so the rate of your clock would not be updated from the
framework's perspective until some later point where you call
clk_set_rate or something.

If your driver needs to know whether or not the clock is enabled then we
could introduce a new bool clk_is_enabled(struct clk *clk); to clk.h,
but I'd rather not do that. Instead if a driver needs a clock then it
calls clk_enable on it without any knowledge about the internal state of
the clock enable_count.

Regards,
Mike

> 
> > +
> > +struct clk_ops max_gen_clk_ops = {
> > +       .prepare        = max_gen_clk_prepare,
> > +       .unprepare      = max_gen_clk_unprepare,
> > +       .is_prepared    = max_gen_clk_is_prepared,
> > +       .recalc_rate    = max_gen_recalc_rate,
> > +};
> > +EXPORT_SYMBOL_GPL(max_gen_clk_ops);
> > +
> > +static struct clk *max_gen_clk_register(struct device *dev,
> > +                                       struct max_gen_clk *max_gen)
> > +{
> > +       struct clk *clk;
> > +       struct clk_hw *hw = &max_gen->hw;
> > +
> > +       clk = clk_register(dev, hw);
> > +       if (IS_ERR(clk))
> > +               return clk;
> > +
> > +       max_gen->lookup = kzalloc(sizeof(struct clk_lookup), GFP_KERNEL);
> 
> As I suggested in other patch[1] also, its better to use
> clkdev_alloc() instead of kzalloc() here.
> 
> > +       if (!max_gen->lookup)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       max_gen->lookup->con_id = hw->init->name;
> 
> Also IMO,  init->name should be over-written if name is provided in DT,
> otherwise generic "clock-output-names" property will go futile,
> perhaps it should be done before clk_register.
> 
> Regards,
> Yadwinder
> 
> [1] : https://lkml.org/lkml/2014/6/27/197
> 
> > +       max_gen->lookup->clk = clk;
> > +
> > +       clkdev_add(max_gen->lookup);
> > +
> > +       return clk;
> > +}
> > +
> > +int max_gen_clk_probe(struct platform_device *pdev, struct regmap *regmap,
> > +                     u32 reg, struct clk_init_data *clks_init, int num_init)
> > +{
> > +       int i, ret;
> > +       struct max_gen_clk *max_gen_clks;
> > +       struct clk **clocks;
> > +       struct device *dev = &pdev->dev;
> > +
> > +       clocks = devm_kzalloc(dev, sizeof(struct clk *) * num_init, GFP_KERNEL);
> > +       if (!clocks)
> > +               return -ENOMEM;
> > +
> > +       max_gen_clks = devm_kzalloc(dev, sizeof(struct max_gen_clk)
> > +                                   * num_init, GFP_KERNEL);
> > +       if (!max_gen_clks)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < num_init; i++) {
> > +               max_gen_clks[i].regmap = regmap;
> > +               max_gen_clks[i].mask = 1 << i;
> > +               max_gen_clks[i].reg = reg;
> > +               max_gen_clks[i].hw.init = &clks_init[i];
> > +
> > +               clocks[i] = max_gen_clk_register(dev, &max_gen_clks[i]);
> > +               if (IS_ERR(clocks[i])) {
> > +                       ret = PTR_ERR(clocks[i]);
> > +                       dev_err(dev, "failed to register %s\n",
> > +                               max_gen_clks[i].hw.init->name);
> > +                       goto err_clocks;
> > +               }
> > +       }
> > +
> > +       platform_set_drvdata(pdev, clocks);
> > +
> > +       if (dev->of_node) {
> > +               struct clk_onecell_data *of_data;
> > +
> > +               of_data = devm_kzalloc(dev, sizeof(*of_data), GFP_KERNEL);
> > +               if (!of_data) {
> > +                       ret = -ENOMEM;
> > +                       goto err_clocks;
> > +               }
> > +
> > +               of_data->clks = clocks;
> > +               of_data->clk_num = num_init;
> > +               ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
> > +                                         of_data);
> > +
> > +               if (ret) {
> > +                       dev_err(dev, "failed to register OF clock provider\n");
> > +                       goto err_clocks;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +
> > +err_clocks:
> > +       for (--i; i >= 0; --i) {
> > +               clkdev_drop(max_gen_clks[i].lookup);
> > +               clk_unregister(max_gen_clks[i].hw.clk);
> > +       }
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(max_gen_clk_probe);
> > +
> > +int max_gen_clk_remove(struct platform_device *pdev, int num_init)
> > +{
> > +       struct clk **clocks = platform_get_drvdata(pdev);
> > +       struct device *dev = pdev->dev.parent;
> > +       int i;
> > +
> > +       if (dev->of_node)
> > +               of_clk_del_provider(dev->of_node);
> > +
> > +       for (i = 0; i < num_init; i++) {
> > +               struct clk_hw *hw = __clk_get_hw(clocks[i]);
> > +               struct max_gen_clk *max_gen = to_max_gen_clk(hw);
> > +
> > +               clkdev_drop(max_gen->lookup);
> > +               clk_unregister(clocks[i]);
> > +       }
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(max_gen_clk_remove);
> > diff --git a/drivers/clk/clk-max-gen.h b/drivers/clk/clk-max-gen.h
> > new file mode 100644
> > index 0000000..997e86f
> > --- /dev/null
> > +++ b/drivers/clk/clk-max-gen.h
> > @@ -0,0 +1,32 @@
> > +/*
> > + * clk-max-gen.h - Generic clock driver for Maxim PMICs clocks
> > + *
> > + * Copyright (C) 2014 Google, Inc
> > + *
> > + * This program is free software; you can redistribute  it and/or modify it
> > + * under  the terms of  the GNU General  Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#ifndef __CLK_MAX_GEN_H__
> > +#define __CLK_MAX_GEN_H__
> > +
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/regmap.h>
> > +#include <linux/platform_device.h>
> > +
> > +int max_gen_clk_probe(struct platform_device *pdev, struct regmap *regmap,
> > +                     u32 reg, struct clk_init_data *clks_init, int num_init);
> > +int max_gen_clk_remove(struct platform_device *pdev, int num_init);
> > +extern struct clk_ops max_gen_clk_ops;
> > +
> > +#endif /* __CLK_MAX_GEN_H__ */
> > --
> > 2.0.0.rc2
> >
Javier Martinez Canillas July 2, 2014, 10:13 a.m. UTC | #6
Hello Mike,

On 07/01/2014 07:26 PM, Mike Turquette wrote:
> Quoting Yadwinder Singh Brar (2014-06-29 21:01:36)
>> Hi Javier,
>> 
>> On Thu, Jun 26, 2014 at 11:45 PM, Javier Martinez Canillas
>> <javier.martinez@collabora.co.uk> wrote:
>> > Maxim Integrated Power Management ICs are very similar with
>> > regard to their clock outputs. Most of the clock drivers for
>> > these chips are duplicating code and are simpler enough that
>> > can be converted to use a generic driver to consolidate code
>> > and avoid duplication.
>> >
>> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> > ---
>> >
>> > Changes since v4:
>> >  - Return recalc 0 if clock isn't enabled in Suggested by Yadwinder Singh Brar.
>> >
>> 
>> It seems you didn't implement or posted same patch again :) .
>> 
>> > Changes since v3:
>> >  - Add current copyright information. Suggested by Krzysztof Kozlowski
>> >  - Do a single allocation for struct max_gen_clk. Suggested by Krzysztof Kozlowski
>> >  - Add EXPORT_SYMBOL() for exported symbols. Suggested by Krzysztof Kozlowski
>> >
>> >  drivers/clk/Kconfig       |   3 +
>> >  drivers/clk/Makefile      |   1 +
>> >  drivers/clk/clk-max-gen.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
>> >  drivers/clk/clk-max-gen.h |  32 ++++++++
>> >  4 files changed, 231 insertions(+)
>> >  create mode 100644 drivers/clk/clk-max-gen.c
>> >  create mode 100644 drivers/clk/clk-max-gen.h
>> >
>> 
>> [ .. ]
>> 
>> > +
>> > +static unsigned long max_gen_recalc_rate(struct clk_hw *hw,
>> > +                                        unsigned long parent_rate)
>> > +{
>> > +       return 32768;
>> > +}
>> 
>> Its still same here.
> 
> Changing this would be a new behavior. I do not know of any other clock
> drivers that conditionally returns a rate of 0 based on whether or not
> the clock is gated.
> 

After Yadwinder feedback I searched for clock drivers that returned 0 when the
clock was not enabled/prepared and found for example drivers/clk/clk-s2mps11.c:


static unsigned long s2mps11_clk_recalc_rate(struct clk_hw *hw,
                                             unsigned long parent_rate)
{
        struct s2mps11_clk *s2mps11 = to_s2mps11_clk(hw);
        if (s2mps11->enabled)
                return 32768;
        else
                return 0;
}

> It is also buggy since calls to clk_enable and clk_disable do not invoke
> .recalc_rate, so the rate of your clock would not be updated from the
> framework's perspective until some later point where you call
> clk_set_rate or something.
>

s2ps11->enabled is set in the driver's clk_ops .prepare and .unprepare function
handlers and calls to clk_prepare and clk_unprepare also don't seems to invoke
.recalc_rate so I guess that driver is wrong as well and should just return the
clock rate unconditionally?

> If your driver needs to know whether or not the clock is enabled then we
> could introduce a new bool clk_is_enabled(struct clk *clk); to clk.h,
> but I'd rather not do that. Instead if a driver needs a clock then it
> calls clk_enable on it without any knowledge about the internal state of
> the clock enable_count.
> 
> Regards,
> Mike
>

Thanks a lot for the explanation, I'll revert that change then and return the
clock rate unconditionally on the next version of the patch-set.

Best regards,
Javier
Krzysztof Kozlowski July 2, 2014, 10:19 a.m. UTC | #7
On ?ro, 2014-07-02 at 12:13 +0200, Javier Martinez Canillas wrote:
> Hello Mike,
> 
> On 07/01/2014 07:26 PM, Mike Turquette wrote:
> > Quoting Yadwinder Singh Brar (2014-06-29 21:01:36)
> >> Hi Javier,
> >> 
> >> On Thu, Jun 26, 2014 at 11:45 PM, Javier Martinez Canillas
> >> <javier.martinez@collabora.co.uk> wrote:
> >> > Maxim Integrated Power Management ICs are very similar with
> >> > regard to their clock outputs. Most of the clock drivers for
> >> > these chips are duplicating code and are simpler enough that
> >> > can be converted to use a generic driver to consolidate code
> >> > and avoid duplication.
> >> >
> >> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> >> > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >> > ---
> >> >
> >> > Changes since v4:
> >> >  - Return recalc 0 if clock isn't enabled in Suggested by Yadwinder Singh Brar.
> >> >
> >> 
> >> It seems you didn't implement or posted same patch again :) .
> >> 
> >> > Changes since v3:
> >> >  - Add current copyright information. Suggested by Krzysztof Kozlowski
> >> >  - Do a single allocation for struct max_gen_clk. Suggested by Krzysztof Kozlowski
> >> >  - Add EXPORT_SYMBOL() for exported symbols. Suggested by Krzysztof Kozlowski
> >> >
> >> >  drivers/clk/Kconfig       |   3 +
> >> >  drivers/clk/Makefile      |   1 +
> >> >  drivers/clk/clk-max-gen.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
> >> >  drivers/clk/clk-max-gen.h |  32 ++++++++
> >> >  4 files changed, 231 insertions(+)
> >> >  create mode 100644 drivers/clk/clk-max-gen.c
> >> >  create mode 100644 drivers/clk/clk-max-gen.h
> >> >
> >> 
> >> [ .. ]
> >> 
> >> > +
> >> > +static unsigned long max_gen_recalc_rate(struct clk_hw *hw,
> >> > +                                        unsigned long parent_rate)
> >> > +{
> >> > +       return 32768;
> >> > +}
> >> 
> >> Its still same here.
> > 
> > Changing this would be a new behavior. I do not know of any other clock
> > drivers that conditionally returns a rate of 0 based on whether or not
> > the clock is gated.
> > 
> 
> After Yadwinder feedback I searched for clock drivers that returned 0 when the
> clock was not enabled/prepared and found for example drivers/clk/clk-s2mps11.c:
> 
> 
> static unsigned long s2mps11_clk_recalc_rate(struct clk_hw *hw,
>                                              unsigned long parent_rate)
> {
>         struct s2mps11_clk *s2mps11 = to_s2mps11_clk(hw);
>         if (s2mps11->enabled)
>                 return 32768;
>         else
>                 return 0;
> }
> 
> > It is also buggy since calls to clk_enable and clk_disable do not invoke
> > .recalc_rate, so the rate of your clock would not be updated from the
> > framework's perspective until some later point where you call
> > clk_set_rate or something.
> >
> 
> s2ps11->enabled is set in the driver's clk_ops .prepare and .unprepare function
> handlers and calls to clk_prepare and clk_unprepare also don't seems to invoke
> .recalc_rate so I guess that driver is wrong as well and should just return the
> clock rate unconditionally?

The s2mps11 may be not a best example for proper clock driver :).
Karol Wrona already sent a patch for s2mps11:
https://lkml.org/lkml/2014/7/1/389


Best regards,
Krzysztof

> 
> > If your driver needs to know whether or not the clock is enabled then we
> > could introduce a new bool clk_is_enabled(struct clk *clk); to clk.h,
> > but I'd rather not do that. Instead if a driver needs a clock then it
> > calls clk_enable on it without any knowledge about the internal state of
> > the clock enable_count.
> > 
> > Regards,
> > Mike
> >
> 
> Thanks a lot for the explanation, I'll revert that change then and return the
> clock rate unconditionally on the next version of the patch-set.
> 
> Best regards,
> Javier
diff mbox

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9f9c5ae..73f78e8 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -32,6 +32,9 @@  config COMMON_CLK_WM831X
 
 source "drivers/clk/versatile/Kconfig"
 
+config COMMON_CLK_MAX_GEN
+        bool
+
 config COMMON_CLK_MAX77686
 	tristate "Clock driver for Maxim 77686 MFD"
 	depends on MFD_MAX77686
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 567f102..6c1aff6 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -18,6 +18,7 @@  obj-$(CONFIG_ARCH_BCM2835)		+= clk-bcm2835.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
 obj-$(CONFIG_MACH_LOONGSON1)		+= clk-ls1x.o
+obj-$(CONFIG_COMMON_CLK_MAX_GEN)	+= clk-max-gen.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
 obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
 obj-$(CONFIG_ARCH_NOMADIK)		+= clk-nomadik.o
diff --git a/drivers/clk/clk-max-gen.c b/drivers/clk/clk-max-gen.c
new file mode 100644
index 0000000..3e9fa7e
--- /dev/null
+++ b/drivers/clk/clk-max-gen.c
@@ -0,0 +1,195 @@ 
+/*
+ * clk-max-gen.c - Generic clock driver for Maxim PMICs clocks
+ *
+ * Copyright (C) 2014 Google, Inc
+ *
+ * Copyright (C) 2012 Samsung Electornics
+ * Jonghwa Lee <jonghwa3.lee@samsung.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This driver is based on clk-max77686.c
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include <linux/clk-provider.h>
+#include <linux/mutex.h>
+#include <linux/clkdev.h>
+
+struct max_gen_clk {
+	struct regmap *regmap;
+	u32 mask;
+	u32 reg;
+	struct clk_hw hw;
+	struct clk_lookup *lookup;
+};
+
+static struct max_gen_clk *to_max_gen_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct max_gen_clk, hw);
+}
+
+static int max_gen_clk_prepare(struct clk_hw *hw)
+{
+	struct max_gen_clk *max_gen = to_max_gen_clk(hw);
+
+	return regmap_update_bits(max_gen->regmap, max_gen->reg,
+				  max_gen->mask, max_gen->mask);
+}
+
+static void max_gen_clk_unprepare(struct clk_hw *hw)
+{
+	struct max_gen_clk *max_gen = to_max_gen_clk(hw);
+
+	regmap_update_bits(max_gen->regmap, max_gen->reg,
+			   max_gen->mask, ~max_gen->mask);
+}
+
+static int max_gen_clk_is_prepared(struct clk_hw *hw)
+{
+	struct max_gen_clk *max_gen = to_max_gen_clk(hw);
+	int ret;
+	u32 val;
+
+	ret = regmap_read(max_gen->regmap, max_gen->reg, &val);
+
+	if (ret < 0)
+		return -EINVAL;
+
+	return val & max_gen->mask;
+}
+
+static unsigned long max_gen_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	return 32768;
+}
+
+struct clk_ops max_gen_clk_ops = {
+	.prepare	= max_gen_clk_prepare,
+	.unprepare	= max_gen_clk_unprepare,
+	.is_prepared	= max_gen_clk_is_prepared,
+	.recalc_rate	= max_gen_recalc_rate,
+};
+EXPORT_SYMBOL_GPL(max_gen_clk_ops);
+
+static struct clk *max_gen_clk_register(struct device *dev,
+					struct max_gen_clk *max_gen)
+{
+	struct clk *clk;
+	struct clk_hw *hw = &max_gen->hw;
+
+	clk = clk_register(dev, hw);
+	if (IS_ERR(clk))
+		return clk;
+
+	max_gen->lookup = kzalloc(sizeof(struct clk_lookup), GFP_KERNEL);
+	if (!max_gen->lookup)
+		return ERR_PTR(-ENOMEM);
+
+	max_gen->lookup->con_id = hw->init->name;
+	max_gen->lookup->clk = clk;
+
+	clkdev_add(max_gen->lookup);
+
+	return clk;
+}
+
+int max_gen_clk_probe(struct platform_device *pdev, struct regmap *regmap,
+		      u32 reg, struct clk_init_data *clks_init, int num_init)
+{
+	int i, ret;
+	struct max_gen_clk *max_gen_clks;
+	struct clk **clocks;
+	struct device *dev = &pdev->dev;
+
+	clocks = devm_kzalloc(dev, sizeof(struct clk *) * num_init, GFP_KERNEL);
+	if (!clocks)
+		return -ENOMEM;
+
+	max_gen_clks = devm_kzalloc(dev, sizeof(struct max_gen_clk)
+				    * num_init, GFP_KERNEL);
+	if (!max_gen_clks)
+		return -ENOMEM;
+
+	for (i = 0; i < num_init; i++) {
+		max_gen_clks[i].regmap = regmap;
+		max_gen_clks[i].mask = 1 << i;
+		max_gen_clks[i].reg = reg;
+		max_gen_clks[i].hw.init = &clks_init[i];
+
+		clocks[i] = max_gen_clk_register(dev, &max_gen_clks[i]);
+		if (IS_ERR(clocks[i])) {
+			ret = PTR_ERR(clocks[i]);
+			dev_err(dev, "failed to register %s\n",
+				max_gen_clks[i].hw.init->name);
+			goto err_clocks;
+		}
+	}
+
+	platform_set_drvdata(pdev, clocks);
+
+	if (dev->of_node) {
+		struct clk_onecell_data *of_data;
+
+		of_data = devm_kzalloc(dev, sizeof(*of_data), GFP_KERNEL);
+		if (!of_data) {
+			ret = -ENOMEM;
+			goto err_clocks;
+		}
+
+		of_data->clks = clocks;
+		of_data->clk_num = num_init;
+		ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
+					  of_data);
+
+		if (ret) {
+			dev_err(dev, "failed to register OF clock provider\n");
+			goto err_clocks;
+		}
+	}
+
+	return 0;
+
+err_clocks:
+	for (--i; i >= 0; --i) {
+		clkdev_drop(max_gen_clks[i].lookup);
+		clk_unregister(max_gen_clks[i].hw.clk);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(max_gen_clk_probe);
+
+int max_gen_clk_remove(struct platform_device *pdev, int num_init)
+{
+	struct clk **clocks = platform_get_drvdata(pdev);
+	struct device *dev = pdev->dev.parent;
+	int i;
+
+	if (dev->of_node)
+		of_clk_del_provider(dev->of_node);
+
+	for (i = 0; i < num_init; i++) {
+		struct clk_hw *hw = __clk_get_hw(clocks[i]);
+		struct max_gen_clk *max_gen = to_max_gen_clk(hw);
+
+		clkdev_drop(max_gen->lookup);
+		clk_unregister(clocks[i]);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(max_gen_clk_remove);
diff --git a/drivers/clk/clk-max-gen.h b/drivers/clk/clk-max-gen.h
new file mode 100644
index 0000000..997e86f
--- /dev/null
+++ b/drivers/clk/clk-max-gen.h
@@ -0,0 +1,32 @@ 
+/*
+ * clk-max-gen.h - Generic clock driver for Maxim PMICs clocks
+ *
+ * Copyright (C) 2014 Google, Inc
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __CLK_MAX_GEN_H__
+#define __CLK_MAX_GEN_H__
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/clkdev.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+
+int max_gen_clk_probe(struct platform_device *pdev, struct regmap *regmap,
+		      u32 reg, struct clk_init_data *clks_init, int num_init);
+int max_gen_clk_remove(struct platform_device *pdev, int num_init);
+extern struct clk_ops max_gen_clk_ops;
+
+#endif /* __CLK_MAX_GEN_H__ */