| Message ID | 1403806546-31122-6-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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__ */