| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <5037318E.2040903@samsung.com>
Date: Fri, 24 Aug 2012 16:47:26 +0900
From: jonghwa3.lee@...sung.com
To: Mike Turquette <mturquette@...aro.org>
Cc: Jonghwa Lee <jonghwa3.lee@...sung.com>,
linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
H Hartley Sweeten <hsweeten@...ionengravers.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH v4] clock: max77686: Add driver for Maxim 77686 32KHz
crystal oscillator
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@...r.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 majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists