* [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator
@ 2012-06-27 10:31 Jonghwa Lee
2012-08-24 2:07 ` Mike Turquette
0 siblings, 1 reply; 4+ messages in thread
From: Jonghwa Lee @ 2012-06-27 10:31 UTC (permalink / raw)
To: linux-kernel
Cc: Mike Turquette, Arnd Bergmann, H Hartley Sweeten, Jonghwa Lee,
MyungJoo Ham, Kyungmin Park
Maxim 77686 has three 32KHz clock outputs through the its crystal oscillator.
This driver can control those ouputs by I2C bus. The clocks are used to supply
to SOC and peripheral chips as a clock source. Clocks can be enabled/disabled only.
It uses regmap interface to communicate with I2C bus.
Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
v4
- Remove global definition of clk instances.
- Add max77686_clk_register() to simplify the registering clocks.
- Add support clkdev so user can get clocks with clkdev API.
- Replace enable/diable function to prepare/unprepare fuction to assure
that it excutes in non-atomic context.
drivers/clk/Kconfig | 8 ++
drivers/clk/Makefile | 2 +
drivers/clk/clk-max77686.c | 192 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 202 insertions(+), 0 deletions(-)
create mode 100644 drivers/clk/clk-max77686.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 165e1fe..936c27a 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -44,4 +44,12 @@ config COMMON_CLK_DEBUG
clk_flags, clk_prepare_count, clk_enable_count &
clk_notifier_count.
+config CLK_MAX77686
+ tristate "Maxim 77686 32KHz crystal oscillator driver"
+ depends on MFD_MAX77686 && COMMON_CLK
+ ---help---
+ This driver supports for Maxim 77686's crystal oscillator.
+ Maxim 77686 has three 32KHz buffered outputs and It can
+ controls them over I2C bus.
endmenu
+
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 1f736bc..a952555b 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,3 +2,5 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
obj-$(CONFIG_COMMON_CLK) += clk.o clk-fixed-rate.o clk-gate.o \
clk-mux.o clk-divider.o
+
+obj-$(CONFIG_CLK_MAX77686) += clk-max77686.o
diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c
new file mode 100644
index 0000000..fd3899e
--- /dev/null
+++ b/drivers/clk/clk-max77686.c
@@ -0,0 +1,192 @@
+/*
+ * clk-max77686.c - Clock driver for Maxim 77686
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/max77686.h>
+#include <linux/mfd/max77686-private.h>
+#include <linux/clk-private.h>
+#include <linux/mutex.h>
+#include <linux/clkdev.h>
+
+#define MAX77686_CLKS_NUM 3
+
+struct clk_max77686 {
+ struct max77686_dev *iodev;
+ u32 mask;
+ struct clk_hw hw;
+ struct clk_lookup *lookup;
+};
+
+static struct clk_max77686 *get_max77686_clk(struct clk_hw *hw)
+{
+ return container_of(hw, struct clk_max77686, hw);
+}
+
+static int max77686_clk_enable(struct clk_hw *hw)
+{
+ struct clk_max77686 *max77686;
+ int ret;
+
+ max77686 = get_max77686_clk(hw);
+ if (!max77686)
+ return -ENOMEM;
+
+ ret = regmap_update_bits(max77686->iodev->regmap,
+ MAX77686_REG_32KHZ, max77686->mask, max77686->mask);
+
+ return ret;
+}
+
+static void max77686_clk_disable(struct clk_hw *hw)
+{
+ struct clk_max77686 *max77686;
+
+ max77686 = get_max77686_clk(hw);
+ if (!max77686)
+ return;
+
+ regmap_update_bits(max77686->iodev->regmap,
+ MAX77686_REG_32KHZ, max77686->mask, ~max77686->mask);
+}
+
+static int max77686_clk_is_enabled(struct clk_hw *hw)
+{
+ struct clk_max77686 *max77686;
+ int ret;
+ u32 val;
+
+ max77686 = get_max77686_clk(hw);
+ if (!max77686)
+ return -ENOMEM;
+
+ ret = regmap_read(max77686->iodev->regmap,
+ MAX77686_REG_32KHZ, &val);
+
+ if (ret < 0)
+ return -EINVAL;
+
+ return val & max77686->mask;
+}
+
+static struct clk_ops clk_max77686_ops = {
+ .prepare = max77686_clk_enable,
+ .unprepare = max77686_clk_disable,
+ .is_enabled = max77686_clk_is_enabled,
+};
+
+static int max77686_clk_register(struct device *dev,
+ struct clk_max77686 *max77686, int clk_id)
+{
+ char clk_name[][10] = {"32khap", "32kcp", "p32kh"};
+ struct clk *clk;
+
+ clk = clk_register(dev, clk_name[clk_id], &clk_max77686_ops,
+ &max77686->hw, NULL, 0, CLK_IS_ROOT);
+
+ if (IS_ERR(clk))
+ return -ENOMEM;
+
+ max77686->lookup = devm_kzalloc(dev, sizeof(struct clk_lookup),
+ GFP_KERNEL);
+ if (IS_ERR(max77686->lookup))
+ return -ENOMEM;
+
+ max77686->lookup->con_id = clk_name[clk_id];
+ max77686->lookup->clk = clk;
+
+ clkdev_add(max77686->lookup);
+
+ return 0;
+}
+
+static __devinit int max77686_clk_probe(struct platform_device *pdev)
+{
+ struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+ struct clk_max77686 *max77686[MAX77686_CLKS_NUM];
+ int i, ret;
+
+ for (i = 0; i < MAX77686_CLKS_NUM; i++) {
+ max77686[i] = devm_kzalloc(&pdev->dev,
+ sizeof(struct clk_max77686), GFP_KERNEL);
+ if (!max77686[i])
+ return -ENOMEM;
+
+ max77686[i]->iodev = iodev;
+ max77686[i]->mask = 1 << i;
+
+ ret = max77686_clk_register(&pdev->dev, max77686[i], i);
+ if (ret) {
+ dev_err(&pdev->dev, "Fail to register clock\n");
+ return ret;
+ }
+ }
+
+ platform_set_drvdata(pdev, max77686);
+
+ return 0;
+}
+
+static int __devexit max77686_clk_remove(struct platform_device *pdev)
+{
+ struct clk_max77686 **max77686 = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < MAX77686_CLKS_NUM; i++) {
+ clkdev_drop(max77686[i]->lookup);
+ kfree(max77686[i]->hw.clk);
+ }
+ return 0;
+}
+
+static const struct platform_device_id max77686_clk_id[] = {
+ { "max77686-clk", 0},
+ { },
+};
+MODULE_DEVICE_TABLE(platform, max77686_clk_id);
+
+static struct platform_driver max77686_clk_driver = {
+ .driver = {
+ .name = "max77686-clk",
+ .owner = THIS_MODULE,
+ },
+ .probe = max77686_clk_probe,
+ .remove = __devexit_p(max77686_clk_remove),
+ .id_table = max77686_clk_id,
+};
+
+static int __init max77686_clk_init(void)
+{
+ return platform_driver_register(&max77686_clk_driver);
+}
+subsys_initcall(max77686_clk_init);
+
+static void __init max77686_clk_cleanup(void)
+{
+ platform_driver_unregister(&max77686_clk_driver);
+}
+module_exit(max77686_clk_cleanup);
+
+MODULE_DESCRIPTION("MAXIM 77686 Clock Driver");
+MODULE_AUTHOR("Jonghwa Lee <jonghwa3.lee@samsung.com>");
+MODULE_LICENSE("GPL");
--
1.7.4.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator
2012-06-27 10:31 [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator Jonghwa Lee
@ 2012-08-24 2:07 ` Mike Turquette
2012-08-24 7:47 ` jonghwa3.lee
0 siblings, 1 reply; 4+ messages in thread
From: Mike Turquette @ 2012-08-24 2:07 UTC (permalink / raw)
To: Jonghwa Lee, linux-kernel
Cc: Arnd Bergmann, H Hartley Sweeten, Jonghwa Lee, MyungJoo Ham,
Kyungmin Park
Hello Jonghwa,
Quoting Jonghwa Lee (2012-06-27 03:31:17)
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/max77686.h>
> +#include <linux/mfd/max77686-private.h>
> +#include <linux/clk-private.h>
Please use clk-provider.h. I don't see any reason for this code to need
clk-private.h.
> +#include <linux/mutex.h>
> +#include <linux/clkdev.h>
> +
> +#define MAX77686_CLKS_NUM 3
> +
> +struct clk_max77686 {
> + struct max77686_dev *iodev;
> + u32 mask;
> + struct clk_hw hw;
> + struct clk_lookup *lookup;
> +};
> +
> +static struct clk_max77686 *get_max77686_clk(struct clk_hw *hw)
> +{
> + return container_of(hw, struct clk_max77686, hw);
> +}
> +
> +static int max77686_clk_enable(struct clk_hw *hw)
Please rename this function to max77686_clk_prepare to reflect that is
is the .prepare callback.
> +{
> + struct clk_max77686 *max77686;
> + int ret;
> +
> + max77686 = get_max77686_clk(hw);
> + if (!max77686)
> + return -ENOMEM;
> +
> + ret = regmap_update_bits(max77686->iodev->regmap,
> + MAX77686_REG_32KHZ, max77686->mask, max77686->mask);
> +
> + return ret;
> +}
> +
> +static void max77686_clk_disable(struct clk_hw *hw)
Please rename this function to max77686_clk_unprepare to reflect that is
is the .unprepare callback.
> +{
> + struct clk_max77686 *max77686;
> +
> + max77686 = get_max77686_clk(hw);
> + if (!max77686)
> + return;
> +
> + regmap_update_bits(max77686->iodev->regmap,
> + MAX77686_REG_32KHZ, max77686->mask, ~max77686->mask);
> +}
> +
> +static int max77686_clk_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_max77686 *max77686;
> + int ret;
> + u32 val;
> +
> + max77686 = get_max77686_clk(hw);
> + if (!max77686)
> + return -ENOMEM;
> +
> + ret = regmap_read(max77686->iodev->regmap,
> + MAX77686_REG_32KHZ, &val);
> +
> + if (ret < 0)
> + return -EINVAL;
> +
> + return val & max77686->mask;
> +}
> +
> +static struct clk_ops clk_max77686_ops = {
> + .prepare = max77686_clk_enable,
> + .unprepare = max77686_clk_disable,
> + .is_enabled = max77686_clk_is_enabled,
> +};
> +
> +static int max77686_clk_register(struct device *dev,
> + struct clk_max77686 *max77686, int clk_id)
> +{
> + char clk_name[][10] = {"32khap", "32kcp", "p32kh"};
const?
> + struct clk *clk;
> +
> + clk = clk_register(dev, clk_name[clk_id], &clk_max77686_ops,
> + &max77686->hw, NULL, 0, CLK_IS_ROOT);
> +
> + if (IS_ERR(clk))
> + return -ENOMEM;
> +
> + max77686->lookup = devm_kzalloc(dev, sizeof(struct clk_lookup),
> + GFP_KERNEL);
> + if (IS_ERR(max77686->lookup))
> + return -ENOMEM;
> +
> + max77686->lookup->con_id = clk_name[clk_id];
> + max77686->lookup->clk = clk;
> +
> + clkdev_add(max77686->lookup);
> +
> + return 0;
> +}
> +
> +static __devinit int max77686_clk_probe(struct platform_device *pdev)
> +{
> + struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> + struct clk_max77686 *max77686[MAX77686_CLKS_NUM];
> + int i, ret;
> +
> + for (i = 0; i < MAX77686_CLKS_NUM; i++) {
> + max77686[i] = devm_kzalloc(&pdev->dev,
> + sizeof(struct clk_max77686), GFP_KERNEL);
> + if (!max77686[i])
> + return -ENOMEM;
> +
> + max77686[i]->iodev = iodev;
> + max77686[i]->mask = 1 << i;
> +
> + ret = max77686_clk_register(&pdev->dev, max77686[i], i);
> + if (ret) {
> + dev_err(&pdev->dev, "Fail to register clock\n");
> + return ret;
> + }
There is a memory leak if you successfully register any clocks and then
fail to register the others. Be sure to unwind the loop and kfree those
clocks in such a case (since this appears fatal).
You only have three clocks, so I think the loop could be completely
replaced with an array and three calls to clk_register and clkdev_add
(each).
Regards,
Mike
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator
2012-08-24 2:07 ` Mike Turquette
@ 2012-08-24 7:47 ` jonghwa3.lee
2012-08-24 8:12 ` jonghwa3.lee
0 siblings, 1 reply; 4+ messages in thread
From: jonghwa3.lee @ 2012-08-24 7:47 UTC (permalink / raw)
To: Mike Turquette
Cc: Jonghwa Lee, linux-kernel, Arnd Bergmann, H Hartley Sweeten,
MyungJoo Ham, Kyungmin Park
Hi Mike,
On 2012년 08월 24일 11:07, Mike Turquette wrote:
> Hello Jonghwa,
>
> Quoting Jonghwa Lee (2012-06-27 03:31:17)
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mfd/max77686.h>
>> +#include <linux/mfd/max77686-private.h>
>> +#include <linux/clk-private.h>
> Please use clk-provider.h. I don't see any reason for this code to need
> clk-private.h.
Yes, you're right. I missed to change it. I'll update it.
>> +#include <linux/mutex.h>
>> +#include <linux/clkdev.h>
>> +
>> +#define MAX77686_CLKS_NUM 3
>> +
>> +struct clk_max77686 {
>> + struct max77686_dev *iodev;
>> + u32 mask;
>> + struct clk_hw hw;
>> + struct clk_lookup *lookup;
>> +};
>> +
>> +static struct clk_max77686 *get_max77686_clk(struct clk_hw *hw)
>> +{
>> + return container_of(hw, struct clk_max77686, hw);
>> +}
>> +
>> +static int max77686_clk_enable(struct clk_hw *hw)
> Please rename this function to max77686_clk_prepare to reflect that is
> is the .prepare callback.
Okay.
>> +{
>> + struct clk_max77686 *max77686;
>> + int ret;
>> +
>> + max77686 = get_max77686_clk(hw);
>> + if (!max77686)
>> + return -ENOMEM;
>> +
>> + ret = regmap_update_bits(max77686->iodev->regmap,
>> + MAX77686_REG_32KHZ, max77686->mask, max77686->mask);
>> +
>> + return ret;
>> +}
>> +
>> +static void max77686_clk_disable(struct clk_hw *hw)
> Please rename this function to max77686_clk_unprepare to reflect that is
> is the .unprepare callback.
Okay.
>> +{
>> + struct clk_max77686 *max77686;
>> +
>> + max77686 = get_max77686_clk(hw);
>> + if (!max77686)
>> + return;
>> +
>> + regmap_update_bits(max77686->iodev->regmap,
>> + MAX77686_REG_32KHZ, max77686->mask, ~max77686->mask);
>> +}
>> +
>> +static int max77686_clk_is_enabled(struct clk_hw *hw)
>> +{
>> + struct clk_max77686 *max77686;
>> + int ret;
>> + u32 val;
>> +
>> + max77686 = get_max77686_clk(hw);
>> + if (!max77686)
>> + return -ENOMEM;
>> +
>> + ret = regmap_read(max77686->iodev->regmap,
>> + MAX77686_REG_32KHZ, &val);
>> +
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> + return val & max77686->mask;
>> +}
>> +
>> +static struct clk_ops clk_max77686_ops = {
>> + .prepare = max77686_clk_enable,
>> + .unprepare = max77686_clk_disable,
>> + .is_enabled = max77686_clk_is_enabled,
>> +};
>> +
>> +static int max77686_clk_register(struct device *dev,
>> + struct clk_max77686 *max77686, int clk_id)
>> +{
>> + char clk_name[][10] = {"32khap", "32kcp", "p32kh"};
> const?
Okay, I agree that it is better to change definition into const.
>
>> + struct clk *clk;
>> +
>> + clk = clk_register(dev, clk_name[clk_id], &clk_max77686_ops,
>> + &max77686->hw, NULL, 0, CLK_IS_ROOT);
>> +
>> + if (IS_ERR(clk))
>> + return -ENOMEM;
>> +
>> + max77686->lookup = devm_kzalloc(dev, sizeof(struct clk_lookup),
>> + GFP_KERNEL);
>> + if (IS_ERR(max77686->lookup))
>> + return -ENOMEM;
>> +
>> + max77686->lookup->con_id = clk_name[clk_id];
>> + max77686->lookup->clk = clk;
>> +
>> + clkdev_add(max77686->lookup);
>> +
>> + return 0;
>> +}
>> +
>> +static __devinit int max77686_clk_probe(struct platform_device *pdev)
>> +{
>> + struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>> + struct clk_max77686 *max77686[MAX77686_CLKS_NUM];
>> + int i, ret;
>> +
>> + for (i = 0; i < MAX77686_CLKS_NUM; i++) {
>> + max77686[i] = devm_kzalloc(&pdev->dev,
>> + sizeof(struct clk_max77686), GFP_KERNEL);
>> + if (!max77686[i])
>> + return -ENOMEM;
>> +
>> + max77686[i]->iodev = iodev;
>> + max77686[i]->mask = 1 << i;
>> +
>> + ret = max77686_clk_register(&pdev->dev, max77686[i], i);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Fail to register clock\n");
>> + return ret;
>> + }
> There is a memory leak if you successfully register any clocks and then
> fail to register the others. Be sure to unwind the loop and kfree those
> clocks in such a case (since this appears fatal).
I think it'll free memory allocation automatically, because It is used
devm_kzalloc. Am I wrong?
> You only have three clocks, so I think the loop could be completely
> replaced with an array and three calls to clk_register and clkdev_add
> (each).
Okay, I'll consider it.
Thanks and Best regard.
> Regards,
> Mike
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator
2012-08-24 7:47 ` jonghwa3.lee
@ 2012-08-24 8:12 ` jonghwa3.lee
0 siblings, 0 replies; 4+ messages in thread
From: jonghwa3.lee @ 2012-08-24 8:12 UTC (permalink / raw)
To: jonghwa3.lee
Cc: Mike Turquette, linux-kernel, Arnd Bergmann, H Hartley Sweeten,
MyungJoo Ham, Kyungmin Park
On 2012년 08월 24일 16:47, jonghwa3.lee@samsung.com wrote:
> On 2012년 08월 24일 11:07, Mike Turquette wrote:
>>> + struct clk *clk;
>>> +
>>> + clk = clk_register(dev, clk_name[clk_id], &clk_max77686_ops,
>>> + &max77686->hw, NULL, 0, CLK_IS_ROOT);
>>> +
>>> + if (IS_ERR(clk))
>>> + return -ENOMEM;
>>> +
>>> + max77686->lookup = devm_kzalloc(dev, sizeof(struct clk_lookup),
>>> + GFP_KERNEL);
>>> + if (IS_ERR(max77686->lookup))
>>> + return -ENOMEM;
>>> +
>>> + max77686->lookup->con_id = clk_name[clk_id];
>>> + max77686->lookup->clk = clk;
>>> +
>>> + clkdev_add(max77686->lookup);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static __devinit int max77686_clk_probe(struct platform_device *pdev)
>>> +{
>>> + struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>>> + struct clk_max77686 *max77686[MAX77686_CLKS_NUM];
>>> + int i, ret;
>>> +
>>> + for (i = 0; i < MAX77686_CLKS_NUM; i++) {
>>> + max77686[i] = devm_kzalloc(&pdev->dev,
>>> + sizeof(struct clk_max77686), GFP_KERNEL);
>>> + if (!max77686[i])
>>> + return -ENOMEM;
>>> +
>>> + max77686[i]->iodev = iodev;
>>> + max77686[i]->mask = 1 << i;
>>> +
>>> + ret = max77686_clk_register(&pdev->dev, max77686[i], i);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Fail to register clock\n");
>>> + return ret;
>>> + }
>> There is a memory leak if you successfully register any clocks and then
>> fail to register the others. Be sure to unwind the loop and kfree those
>> clocks in such a case (since this appears fatal).
> I think it'll free memory allocation automatically, because It is used
> devm_kzalloc. Am I wrong?
>
>
Sorry, I misunderstood. Yes, you're right. Since clk has been allocated
with kzalloc it would be released for the failing.
Thanks for advisement. I'll fix it.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-08-24 8:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-27 10:31 [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator Jonghwa Lee
2012-08-24 2:07 ` Mike Turquette
2012-08-24 7:47 ` jonghwa3.lee
2012-08-24 8:12 ` jonghwa3.lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox