Messages in this thread | | | Date | Mon, 11 Jun 2012 14:09:59 +0300 | From | Felipe Balbi <> | 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@samsung.com> > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.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@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. > + * > + * 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 [unhandled content-type:application/pgp-signature] | |