| 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: <20120611110958.GT6845@arwen.pp.htv.fi>
Date: Mon, 11 Jun 2012 14:09:59 +0300
From: Felipe Balbi <balbi@...com>
To: Jonghwa Lee <jonghwa3.lee@...sung.com>
Cc: linux-kernel@...r.kernel.org,
Mike Turquette <mturquette@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
Hartley Sweeten <hsweeten@...ionengravers.com>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH v3] clock: max77686: Add driver for Maxim 77686 32KHz
crystal oscillator
Hi,
On Mon, Jun 11, 2012 at 08:01:20PM +0900, Jonghwa Lee wrote:
> 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@...sung.com>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@...sung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.com>
> ---
> v3
> - Add mutex to the max77686_clk structure to make atomic execution of enable/disable function.
>
> v2
> - Modify Kconfig symbol to depend on COMMON_CLK and MFD_MAX77686
> - Fix max77686_clk structure and get_max77686_clk fuction to remove obscure points.
>
> drivers/clk/Kconfig | 8 ++
> drivers/clk/Makefile | 2 +
> drivers/clk/clk-max77686.c | 209 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 219 insertions(+), 0 deletions(-)
> create mode 100644 drivers/clk/clk-max77686.c
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 165e1fe..3f63330 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -45,3 +45,11 @@ config COMMON_CLK_DEBUG
> clk_notifier_count.
>
> endmenu
> +
> +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.
> 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..bf4d452
> --- /dev/null
> +++ b/drivers/clk/clk-max77686.c
> @@ -0,0 +1,209 @@
> +/*
> + * clk-max77686.c - Clock driver for Maxim 77686
> + *
> + * Copyright (C) 2012 Samsung Electornics
> + * Jonghwa Lee <jonghwa3.lee@...sung.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.
> + *
> + * 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>
> +
> +enum {
> + MAX77686_32KH_AP,
> + MAX77686_32KH_CP,
> + MAX77686_P32KH,
> + MAX77686_CLKS_NUM,
> +};
> +
> +struct max77686_clk {
> + struct max77686_dev *iodev;
> + u32 mask;
> + struct clk_hw hw;
> + struct mutex mutex;
> +};
> +
> +struct clk *clk32khz_ap;
static
> +struct clk *clk32khz_cp;
static
> +struct clk *clk32khz_pmic;
static
also, why don't you place these pointers inside max77686_clk structure ?
If you keep them here, you will prevent boards with multiple instances
of this device to work.
> +char *max77686_clks[] = {
static const
> + "32khap",
> + "32khcp",
> + "p32kh",
> +};
> +
> +static struct max77686_clk *get_max77686_clk(struct clk_hw *hw)
> +{
> + return container_of(hw, struct max77686_clk, hw);
> +}
> +
> +static int max77686_clk_enable(struct clk_hw *hw)
> +{
> + struct max77686_clk *max77686;
> + int ret;
> +
> + max77686 = get_max77686_clk(hw);
> + if (!max77686)
> + return -ENOMEM;
container_of() will never return NULL. You should check if hw is valid
before the container_of() instead.
> + mutex_lock(&max77686->mutex);
> + ret = regmap_update_bits(max77686->iodev->regmap,
> + MAX77686_REG_32KHZ, max77686->mask, max77686->mask);
> + mutex_unlock(&max77686->mutex);
> +
> + return ret;
> +}
> +
> +static void max77686_clk_disable(struct clk_hw *hw)
> +{
> + struct max77686_clk *max77686;
> +
> + max77686 = get_max77686_clk(hw);
> + if (!max77686)
> + return;
> +
> + mutex_lock(&max77686->mutex);
> + regmap_update_bits(max77686->iodev->regmap,
> + MAX77686_REG_32KHZ, max77686->mask, ~max77686->mask);
> + mutex_unlock(&max77686->mutex);
> +}
> +
> +static int max77686_clk_is_enabled(struct clk_hw *hw)
> +{
> + struct max77686_clk *max77686 = NULL;
> + int ret;
> + u32 val;
> +
> + max77686 = get_max77686_clk(hw);
> + if (!max77686)
> + return -ENOMEM;
> +
> + mutex_lock(&max77686->mutex);
> + ret = regmap_read(max77686->iodev->regmap,
> + MAX77686_REG_32KHZ, &val);
> + mutex_unlock(&max77686->mutex);
> +
> + if (ret < 0)
> + return -EINVAL;
> +
> + return val & max77686->mask;
> +}
> +
> +static struct clk_ops max77686_clk_ops = {
> + .enable = max77686_clk_enable,
> + .disable = max77686_clk_disable,
> + .is_enabled = max77686_clk_is_enabled,
> +};
> +
> +
one blank line is enough.
> +static __devinit int max77686_clk_probe(struct platform_device *pdev)
why platform_device ? Isn't this an i2c device ? So this should be
i2c-client driver...
> +{
> + struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> + struct max77686_clk *max77686[MAX77686_CLKS_NUM];
> + int i, ret;
> +
> + for (i = 0; i < MAX77686_CLKS_NUM; i++) {
> + max77686[i] = devm_kzalloc(&pdev->dev,
> + sizeof(struct max77686_clk), GFP_KERNEL);
> + if (!max77686[i])
> + return -ENOMEM;
> +
> + max77686[i]->iodev = iodev;
> + max77686[i]->mask = 1 << i;
> + mutex_init(&max77686[i]->mutex);
> + }
doesn't look like the right way to do this. What if a user doesn't use
all clk outputs ?
> +
> +
one blank line.
> + clk32khz_ap = clk_register(&pdev->dev, max77686_clks[MAX77686_32KH_AP],
> + &max77686_clk_ops,
> + &max77686[MAX77686_32KH_AP]->hw,
> + NULL, 0, CLK_IS_ROOT);
> + if (IS_ERR(clk32khz_ap)) {
> + ret = PTR_ERR(clk32khz_ap);
> + goto err;
> + }
> +
> + clk32khz_cp = clk_register(&pdev->dev, max77686_clks[MAX77686_32KH_CP],
> + &max77686_clk_ops,
> + &max77686[MAX77686_32KH_CP]->hw,
> + NULL, 0, CLK_IS_ROOT);
> + if (IS_ERR(clk32khz_cp)) {
> + ret = PTR_ERR(clk32khz_cp);
here you will leak clk32khz_ap.
> + goto err;
> + }
> +
> + clk32khz_pmic = clk_register(&pdev->dev, max77686_clks[MAX77686_P32KH],
> + &max77686_clk_ops,
> + &max77686[MAX77686_P32KH]->hw,
> + NULL, 0, CLK_IS_ROOT);
> + if (IS_ERR(clk32khz_pmic)) {
> + ret = PTR_ERR(clk32khz_pmic);
here you will leak clk32khz_cp and clk32khz_ap.
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + dev_err(&pdev->dev, "Fail to register clock\n");
> + return ret;
> +}
> +
> +static int __devexit max77686_clk_remove(struct platform_device *pdev)
> +{
> + kfree(clk32khz_ap);
> + kfree(clk32khz_cp);
> + kfree(clk32khz_pmic);
kfree() or clk_unregister() ??
--
balbi
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists