[PATCH] clk: add LP8788 clock driver

[mturquette@linaro.org (Mike Turquette)]

Quoting Kim, Milo (2013-03-20 17:37:14)
> LP8788 PMU consists of regulator, battery charger, RTC, ADC, backlight driver
> and current sinks.
> Here is additional driver, clock.
> 
> * Clock source
> LP8788 clock is generated by two clock source.
> One is internal oscillator. The other is attached external crystal oscillator.
> LP8788 provides automatic clock source selection through the I2C register.
> This operation is executed in 'prepare' of common clock driver architecture.
> 
> * Clock rate
> LP8788 generates a fixed 32768Hz clock which is used for the system.
> 
> * Supported operation
> .prepare: Before the clock is enabled, automatic clock source selection is done
>           by register access.
> .unprepare: No register for disable or remove the clock source, so do nothing.
> .is_enabled
> .recalc_rate: Fixed clock rate, 32.768KHz.
> 
> * clk_register_fixed_rate() vs devm_clk_register() and clkdev_add()
> Fixed clock driver can be created by common clock helper function but
> but LP8788 should be built as a module.
> Using devm_clk_register() and clkdev_add() is more appropriate method to
> implement LP8788 clock driver.
> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
>  drivers/clk/Kconfig        |    7 ++
>  drivers/clk/Makefile       |    1 +
>  drivers/clk/clk-lp8788.c   |  186 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/lp8788.c       |    2 +
>  include/linux/mfd/lp8788.h |    1 +
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/clk/clk-lp8788.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index a64caef..4b5109c 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -71,6 +71,13 @@ config COMMON_CLK_AXI_CLKGEN
>           Support for the Analog Devices axi-clkgen pcore clock generator for Xilinx
>           FPGAs. It is commonly used in Analog Devices' reference designs.
>  
> +config CLK_LP8788
> +       tristate "Clock driver for TI LP8788 PMU"
> +       depends on MFD_LP8788
> +       ---help---
> +         Supports the clock subsystem of TI LP8788. It generates the 32KHz
> +         clock output.
> +
>  endmenu
>  
>  source "drivers/clk/mvebu/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 1c22f9d..06d0763 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -35,3 +35,4 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
>  obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
>  obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
>  obj-$(CONFIG_CLK_TWL6040)      += clk-twl6040.o
> +obj-$(CONFIG_CLK_LP8788)       += clk-lp8788.o
> diff --git a/drivers/clk/clk-lp8788.c b/drivers/clk/clk-lp8788.c
> new file mode 100644
> index 0000000..de56887
> --- /dev/null
> +++ b/drivers/clk/clk-lp8788.c
> @@ -0,0 +1,186 @@
> +/*
> + * TI LP8788 Clock Driver
> + *
> + * Copyright 2013 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/lp8788.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +
> +/* Registers */
> +#define LP8788_REG_STATUS              0x06
> +#define LP8788_OSC_WORKING             0x10
> +
> +#define LP8788_REG_CLKCTRL             0xA2
> +#define LP8788_CLKMODE_MASK            0x02
> +#define LP8788_CLKMODE_AUTO            0X02
> +
> +#define CLKOUT_32KHZ           32768
> +
> +struct lp8788_clk {
> +       struct lp8788 *lp;
> +       struct clk *clk;
> +       struct clk_hw hw;
> +       struct clk_lookup *lookup;
> +       bool enabled;
> +};
> +
> +static struct lp8788_clk *to_lp8788_clk(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct lp8788_clk, hw);
> +}

Hi Milo,

Just FYI most folks are simply using a macro to do the above.  E.g:

#define to_lp8788_clk(_hw) container_of(_hw, struct lp8788_clk, hw)

> +
> +static int lp8788_clk_prepare(struct clk_hw *hw)
> +{
> +       struct lp8788_clk *pclk = to_lp8788_clk(hw);
> +
> +       /* Automatic oscillator source selection */
> +       return lp8788_update_bits(pclk->lp, LP8788_REG_CLKCTRL,
> +                               LP8788_CLKMODE_MASK, LP8788_CLKMODE_AUTO);
> +}
> +
> +static void lp8788_clk_unprepare(struct clk_hw *hw)
> +{
> +       /* Do nothing */
> +       return;
> +}
> +
> +static int lp8788_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct lp8788_clk *pclk = to_lp8788_clk(hw);
> +
> +       /*
> +        * Do not use the LP8788 register access here, unsafe locking problem
> +        * may occur during loading the driver. So stored varible is prefered.
> +        */

What unsafe locking problem?

Also have you looked into using the new .is_prepare callback?  See:
https://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdiff;h=3d6ee287a3e341c88eafd0b4620b12d640b3736b;hp=30ee400614385ac49f4c9b4bc03d77ff8f07a61e

> +
> +       return pclk->enabled;

Why provide a .is_enabled callback when there are no .enable or .disable
callbacks?

Regards,
Mike