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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/