[PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[Daniel Lezcano]

The hi655x multi function device is a PMIC providing regulators.

The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
this clock in order to add it in the hi655x MFD and allow proper wireless
initialization.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clk/Kconfig      |   8 +++
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk-hi655x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)
 create mode 100644 drivers/clk/clk-hi655x.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9356ab4..471a433 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -47,6 +47,14 @@ config COMMON_CLK_RK808
 	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
 	  by control register.
 
+config COMMON_CLK_HI655X
+	tristate "Clock driver for Hi655x"
+	depends on MFD_HI655X_PMIC
+	---help---
+	  This driver supports the hi655x PMIC clock. This
+	  multi-function device has one fixed-rate oscillator, clocked
+	  at 32KHz.
+
 config COMMON_CLK_SCPI
 	tristate "Clock driver controlled via SCPI interface"
 	depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 92c12b8..c19983a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
+obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
 obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
 obj-$(CONFIG_COMMON_CLK_SCPI)           += clk-scpi.o
 obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c
new file mode 100644
index 0000000..f827d76
--- /dev/null
+++ b/drivers/clk/clk-hi655x.c
@@ -0,0 +1,145 @@
+/* Clock driver for Hi655x
+ *
+ * Copyright (c) 2016, Linaro Ltd.
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/hi655x-pmic.h>
+
+#define HI655X_CLK_BASE	HI655X_BUS_ADDR(0x1c)
+#define HI655X_CLK_SET	BIT(6)
+
+struct hi655x_clk {
+	struct hi655x_pmic *hi655x;
+	struct clk_hw       clk_hw;
+};
+
+static unsigned long hi655x_clk_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	return 32768;
+}
+
+static int hi655x_clk_enable(struct clk_hw *hw, bool enable)
+{
+	struct hi655x_clk *hi655x_clk =
+		container_of(hw, struct hi655x_clk, clk_hw);
+
+	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
+
+	return regmap_update_bits(hi655x->regmap, HI655X_CLK_BASE,
+				  HI655X_CLK_SET, enable ? HI655X_CLK_SET : 0);
+}
+
+static int hi655x_clk_prepare(struct clk_hw *hw)
+{
+	return hi655x_clk_enable(hw, true);
+}
+
+static void hi655x_clk_unprepare(struct clk_hw *hw)
+{
+	hi655x_clk_enable(hw, false);
+}
+
+static int hi655x_clk_is_prepared(struct clk_hw *hw)
+{
+	struct hi655x_clk *hi655x_clk =
+		container_of(hw, struct hi655x_clk, clk_hw);
+	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
+	int ret;
+	uint32_t val;
+
+	ret = regmap_read(hi655x->regmap, HI655X_CLK_BASE, &val);
+	if (ret < 0)
+		return ret;
+
+	return (val & HI655X_CLK_BASE);
+}
+
+static const struct clk_ops hi655x_clk_ops = {
+	.prepare     = hi655x_clk_prepare,
+	.unprepare   = hi655x_clk_unprepare,
+	.is_prepared = hi655x_clk_is_prepared,
+	.recalc_rate = hi655x_clk_recalc_rate,
+};
+
+static struct clk_hw *of_clk_hi655x_get(struct of_phandle_args *clkspec,
+					void *data)
+{
+	struct hi655x_clk *hi655x_clk = data;
+
+	return &hi655x_clk->clk_hw;
+}
+
+static int hi655x_clk_probe(struct platform_device *pdev)
+{
+	struct device *parent = pdev->dev.parent;
+	struct hi655x_pmic *hi655x = dev_get_drvdata(parent);
+	struct clk_init_data *hi655x_clk_init;
+	struct hi655x_clk *hi655x_clk;
+	const char *clk_name = "hi655x-clk";
+	int ret;
+
+	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
+	if (!hi655x_clk)
+		return -ENOMEM;
+
+	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init), GFP_KERNEL);
+	if (!hi655x_clk_init)
+		return -ENOMEM;
+
+	ret = of_property_read_string_index(parent->of_node, "clock-output-names",
+					    0, &clk_name);
+	if (ret)
+		return ret;
+
+	hi655x_clk_init->name	= clk_name;
+	hi655x_clk_init->ops	= &hi655x_clk_ops;
+
+	hi655x_clk->clk_hw.init	= hi655x_clk_init;
+	hi655x_clk->hi655x	= hi655x;
+
+	platform_set_drvdata(pdev, hi655x_clk);
+
+	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
+	if (ret)
+		return ret;
+
+	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
+	if (ret)
+		return ret;
+
+	return of_clk_add_hw_provider(parent->of_node,
+				      of_clk_hi655x_get, hi655x_clk);
+}
+
+static struct platform_driver hi655x_clk_driver = {
+	.probe =  hi655x_clk_probe,
+	.driver		= {
+		.name	= "hi655x-clk",
+	},
+};
+
+module_platform_driver(hi655x_clk_driver);
+
+MODULE_DESCRIPTION("Clk driver for the hi655x series PMICs");
+MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@linaro.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:hi655x-clk");
-- 
1.9.1

[PATCH 2/2] mfd: hi655x: Add the clock cell to provide WiFi and Bluetooth

[Daniel Lezcano]

The hi655x is a PMIC with regulator but also provides a clock for the WiFi
and the bluetooth which is missing in the current implementation.

Add the clock cell so it can be used in the next patch via the dts.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/mfd/hi655x-pmic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
index ba706ad..c37ccbf 100644
--- a/drivers/mfd/hi655x-pmic.c
+++ b/drivers/mfd/hi655x-pmic.c
@@ -77,7 +77,8 @@
 		.num_resources	= ARRAY_SIZE(pwrkey_resources),
 		.resources	= &pwrkey_resources[0],
 	},
-	{	.name		= "hi655x-regulator", },
+	{	.name		= "hi655x-regulator",	},
+	{	.name		= "hi655x-clk",		},
 };
 
 static void hi655x_local_irq_clear(struct regmap *map)
-- 
1.9.1

Re: [PATCH 2/2] mfd: hi655x: Add the clock cell to provide WiFi and Bluetooth

[Lee Jones]

On Fri, 17 Mar 2017, Daniel Lezcano wrote:

> The hi655x is a PMIC with regulator but also provides a clock for the WiFi
> and the bluetooth which is missing in the current implementation.
> 
> Add the clock cell so it can be used in the next patch via the dts.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/mfd/hi655x-pmic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied, thanks.

> diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
> index ba706ad..c37ccbf 100644
> --- a/drivers/mfd/hi655x-pmic.c
> +++ b/drivers/mfd/hi655x-pmic.c
> @@ -77,7 +77,8 @@
>  		.num_resources	= ARRAY_SIZE(pwrkey_resources),
>  		.resources	= &pwrkey_resources[0],
>  	},
> -	{	.name		= "hi655x-regulator", },
> +	{	.name		= "hi655x-regulator",	},
> +	{	.name		= "hi655x-clk",		},
>  };
>  
>  static void hi655x_local_irq_clear(struct regmap *map)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[Daniel Lezcano]

On Fri, Mar 17, 2017 at 08:58:49AM +0100, Daniel Lezcano wrote:
> The hi655x multi function device is a PMIC providing regulators.
> 
> The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
> this clock in order to add it in the hi655x MFD and allow proper wireless
> initialization.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Hi Mike, Stephane,

is there any comment on this driver?

Thanks.

  -- Daniel

> ---
>  drivers/clk/Kconfig      |   8 +++
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk-hi655x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)
>  create mode 100644 drivers/clk/clk-hi655x.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9356ab4..471a433 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -47,6 +47,14 @@ config COMMON_CLK_RK808
>  	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>  	  by control register.
>  
> +config COMMON_CLK_HI655X
> +	tristate "Clock driver for Hi655x"
> +	depends on MFD_HI655X_PMIC
> +	---help---
> +	  This driver supports the hi655x PMIC clock. This
> +	  multi-function device has one fixed-rate oscillator, clocked
> +	  at 32KHz.
> +
>  config COMMON_CLK_SCPI
>  	tristate "Clock driver controlled via SCPI interface"
>  	depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 92c12b8..c19983a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
>  obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
>  obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
>  obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
> +obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
>  obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
>  obj-$(CONFIG_COMMON_CLK_SCPI)           += clk-scpi.o
>  obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
> diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c
> new file mode 100644
> index 0000000..f827d76
> --- /dev/null
> +++ b/drivers/clk/clk-hi655x.c
> @@ -0,0 +1,145 @@
> +/* Clock driver for Hi655x
> + *
> + * Copyright (c) 2016, Linaro Ltd.
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/hi655x-pmic.h>
> +
> +#define HI655X_CLK_BASE	HI655X_BUS_ADDR(0x1c)
> +#define HI655X_CLK_SET	BIT(6)
> +
> +struct hi655x_clk {
> +	struct hi655x_pmic *hi655x;
> +	struct clk_hw       clk_hw;
> +};
> +
> +static unsigned long hi655x_clk_recalc_rate(struct clk_hw *hw,
> +					    unsigned long parent_rate)
> +{
> +	return 32768;
> +}
> +
> +static int hi655x_clk_enable(struct clk_hw *hw, bool enable)
> +{
> +	struct hi655x_clk *hi655x_clk =
> +		container_of(hw, struct hi655x_clk, clk_hw);
> +
> +	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
> +
> +	return regmap_update_bits(hi655x->regmap, HI655X_CLK_BASE,
> +				  HI655X_CLK_SET, enable ? HI655X_CLK_SET : 0);
> +}
> +
> +static int hi655x_clk_prepare(struct clk_hw *hw)
> +{
> +	return hi655x_clk_enable(hw, true);
> +}
> +
> +static void hi655x_clk_unprepare(struct clk_hw *hw)
> +{
> +	hi655x_clk_enable(hw, false);
> +}
> +
> +static int hi655x_clk_is_prepared(struct clk_hw *hw)
> +{
> +	struct hi655x_clk *hi655x_clk =
> +		container_of(hw, struct hi655x_clk, clk_hw);
> +	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
> +	int ret;
> +	uint32_t val;
> +
> +	ret = regmap_read(hi655x->regmap, HI655X_CLK_BASE, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (val & HI655X_CLK_BASE);
> +}
> +
> +static const struct clk_ops hi655x_clk_ops = {
> +	.prepare     = hi655x_clk_prepare,
> +	.unprepare   = hi655x_clk_unprepare,
> +	.is_prepared = hi655x_clk_is_prepared,
> +	.recalc_rate = hi655x_clk_recalc_rate,
> +};
> +
> +static struct clk_hw *of_clk_hi655x_get(struct of_phandle_args *clkspec,
> +					void *data)
> +{
> +	struct hi655x_clk *hi655x_clk = data;
> +
> +	return &hi655x_clk->clk_hw;
> +}
> +
> +static int hi655x_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct hi655x_pmic *hi655x = dev_get_drvdata(parent);
> +	struct clk_init_data *hi655x_clk_init;
> +	struct hi655x_clk *hi655x_clk;
> +	const char *clk_name = "hi655x-clk";
> +	int ret;
> +
> +	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
> +	if (!hi655x_clk)
> +		return -ENOMEM;
> +
> +	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init), GFP_KERNEL);
> +	if (!hi655x_clk_init)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_string_index(parent->of_node, "clock-output-names",
> +					    0, &clk_name);
> +	if (ret)
> +		return ret;
> +
> +	hi655x_clk_init->name	= clk_name;
> +	hi655x_clk_init->ops	= &hi655x_clk_ops;
> +
> +	hi655x_clk->clk_hw.init	= hi655x_clk_init;
> +	hi655x_clk->hi655x	= hi655x;
> +
> +	platform_set_drvdata(pdev, hi655x_clk);
> +
> +	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return of_clk_add_hw_provider(parent->of_node,
> +				      of_clk_hi655x_get, hi655x_clk);
> +}
> +
> +static struct platform_driver hi655x_clk_driver = {
> +	.probe =  hi655x_clk_probe,
> +	.driver		= {
> +		.name	= "hi655x-clk",
> +	},
> +};
> +
> +module_platform_driver(hi655x_clk_driver);
> +
> +MODULE_DESCRIPTION("Clk driver for the hi655x series PMICs");
> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@linaro.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:hi655x-clk");
> -- 
> 1.9.1
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[Stephen Boyd]

On 03/17, Daniel Lezcano wrote:
> The hi655x multi function device is a PMIC providing regulators.
> 
> The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
> this clock in order to add it in the hi655x MFD and allow proper wireless
> initialization.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Is there a binding patch for the PMIC?

> ---
>  drivers/clk/Kconfig      |   8 +++
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk-hi655x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)
>  create mode 100644 drivers/clk/clk-hi655x.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9356ab4..471a433 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -47,6 +47,14 @@ config COMMON_CLK_RK808
>  	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>  	  by control register.
>  
> +config COMMON_CLK_HI655X
> +	tristate "Clock driver for Hi655x"
> +	depends on MFD_HI655X_PMIC

Plus an || COMPILE_TEST? Or would it not compile without some
sort of PMIC define?

> +	---help---
> +	  This driver supports the hi655x PMIC clock. This
> +	  multi-function device has one fixed-rate oscillator, clocked
> +	  at 32KHz.
> +
>  config COMMON_CLK_SCPI
>  	tristate "Clock driver controlled via SCPI interface"
>  	depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
> diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c
> new file mode 100644
> index 0000000..f827d76
> --- /dev/null
> +++ b/drivers/clk/clk-hi655x.c
> @@ -0,0 +1,145 @@
> +/* Clock driver for Hi655x
> + *
> + * Copyright (c) 2016, Linaro Ltd.
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/hi655x-pmic.h>
> +
> +#define HI655X_CLK_BASE	HI655X_BUS_ADDR(0x1c)
> +#define HI655X_CLK_SET	BIT(6)
> +
> +struct hi655x_clk {
> +	struct hi655x_pmic *hi655x;
> +	struct clk_hw       clk_hw;
> +};
> +
> +static unsigned long hi655x_clk_recalc_rate(struct clk_hw *hw,
> +					    unsigned long parent_rate)
> +{
> +	return 32768;
> +}
> +
> +static int hi655x_clk_enable(struct clk_hw *hw, bool enable)
> +{
> +	struct hi655x_clk *hi655x_clk =
> +		container_of(hw, struct hi655x_clk, clk_hw);
> +
> +	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
> +
> +	return regmap_update_bits(hi655x->regmap, HI655X_CLK_BASE,
> +				  HI655X_CLK_SET, enable ? HI655X_CLK_SET : 0);
> +}
> +
> +static int hi655x_clk_prepare(struct clk_hw *hw)
> +{
> +	return hi655x_clk_enable(hw, true);
> +}
> +
> +static void hi655x_clk_unprepare(struct clk_hw *hw)
> +{
> +	hi655x_clk_enable(hw, false);
> +}
> +
> +static int hi655x_clk_is_prepared(struct clk_hw *hw)
> +{
> +	struct hi655x_clk *hi655x_clk =
> +		container_of(hw, struct hi655x_clk, clk_hw);
> +	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
> +	int ret;
> +	uint32_t val;
> +
> +	ret = regmap_read(hi655x->regmap, HI655X_CLK_BASE, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (val & HI655X_CLK_BASE);

Useless parenthesis.

> +}
> +
> +static const struct clk_ops hi655x_clk_ops = {
> +	.prepare     = hi655x_clk_prepare,
> +	.unprepare   = hi655x_clk_unprepare,
> +	.is_prepared = hi655x_clk_is_prepared,
> +	.recalc_rate = hi655x_clk_recalc_rate,
> +};
> +
> +static struct clk_hw *of_clk_hi655x_get(struct of_phandle_args *clkspec,
> +					void *data)
> +{
> +	struct hi655x_clk *hi655x_clk = data;
> +
> +	return &hi655x_clk->clk_hw;
> +}

Just use of_clk_hw_simple_get()?

> +
> +static int hi655x_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct hi655x_pmic *hi655x = dev_get_drvdata(parent);
> +	struct clk_init_data *hi655x_clk_init;
> +	struct hi655x_clk *hi655x_clk;
> +	const char *clk_name = "hi655x-clk";

Why do we set it and then overwrite it?

> +	int ret;
> +
> +	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
> +	if (!hi655x_clk)
> +		return -ENOMEM;
> +
> +	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init), GFP_KERNEL);
> +	if (!hi655x_clk_init)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_string_index(parent->of_node, "clock-output-names",
> +					    0, &clk_name);
> +	if (ret)
> +		return ret;
> +
> +	hi655x_clk_init->name	= clk_name;
> +	hi655x_clk_init->ops	= &hi655x_clk_ops;
> +
> +	hi655x_clk->clk_hw.init	= hi655x_clk_init;
> +	hi655x_clk->hi655x	= hi655x;
> +
> +	platform_set_drvdata(pdev, hi655x_clk);
> +
> +	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return of_clk_add_hw_provider(parent->of_node,
> +				      of_clk_hi655x_get, hi655x_clk);

We forgot to drop the clkdev here if this fails.

> +}
> +
> +static struct platform_driver hi655x_clk_driver = {
> +	.probe =  hi655x_clk_probe,
> +	.driver		= {
> +		.name	= "hi655x-clk",
> +	},
> +};

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[Daniel Lezcano]

On Fri, Apr 07, 2017 at 09:48:51AM -0700, Stephen Boyd wrote:
> On 03/17, Daniel Lezcano wrote:
> > The hi655x multi function device is a PMIC providing regulators.
> > 
> > The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
> > this clock in order to add it in the hi655x MFD and allow proper wireless
> > initialization.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Is there a binding patch for the PMIC?

There is a binding for the hi655x at:

Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt

but I don't see what I should add there.
 
> > ---
> >  drivers/clk/Kconfig      |   8 +++
> >  drivers/clk/Makefile     |   1 +
> >  drivers/clk/clk-hi655x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 154 insertions(+)
> >  create mode 100644 drivers/clk/clk-hi655x.c
> > 
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 9356ab4..471a433 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -47,6 +47,14 @@ config COMMON_CLK_RK808
> >  	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
> >  	  by control register.
> >  
> > +config COMMON_CLK_HI655X
> > +	tristate "Clock driver for Hi655x"
> > +	depends on MFD_HI655X_PMIC
> 
> Plus an || COMPILE_TEST? Or would it not compile without some
> sort of PMIC define?

I don't know. I will add the COMPILE_TEST option and fix the issues in case.

> > +	---help---
> > +	  This driver supports the hi655x PMIC clock. This
> > +	  multi-function device has one fixed-rate oscillator, clocked
> > +	  at 32KHz.
> > +
> >  config COMMON_CLK_SCPI
> >  	tristate "Clock driver controlled via SCPI interface"
> >  	depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
> > diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c
> > new file mode 100644
> > index 0000000..f827d76
> > --- /dev/null
> > +++ b/drivers/clk/clk-hi655x.c
> > @@ -0,0 +1,145 @@
> > +/* Clock driver for Hi655x
> > + *
> > + * Copyright (c) 2016, Linaro Ltd.
> > + *
> > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/hi655x-pmic.h>
> > +
> > +#define HI655X_CLK_BASE	HI655X_BUS_ADDR(0x1c)
> > +#define HI655X_CLK_SET	BIT(6)
> > +
> > +struct hi655x_clk {
> > +	struct hi655x_pmic *hi655x;
> > +	struct clk_hw       clk_hw;
> > +};
> > +
> > +static unsigned long hi655x_clk_recalc_rate(struct clk_hw *hw,
> > +					    unsigned long parent_rate)
> > +{
> > +	return 32768;
> > +}
> > +
> > +static int hi655x_clk_enable(struct clk_hw *hw, bool enable)
> > +{
> > +	struct hi655x_clk *hi655x_clk =
> > +		container_of(hw, struct hi655x_clk, clk_hw);
> > +
> > +	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
> > +
> > +	return regmap_update_bits(hi655x->regmap, HI655X_CLK_BASE,
> > +				  HI655X_CLK_SET, enable ? HI655X_CLK_SET : 0);
> > +}
> > +
> > +static int hi655x_clk_prepare(struct clk_hw *hw)
> > +{
> > +	return hi655x_clk_enable(hw, true);
> > +}
> > +
> > +static void hi655x_clk_unprepare(struct clk_hw *hw)
> > +{
> > +	hi655x_clk_enable(hw, false);
> > +}
> > +
> > +static int hi655x_clk_is_prepared(struct clk_hw *hw)
> > +{
> > +	struct hi655x_clk *hi655x_clk =
> > +		container_of(hw, struct hi655x_clk, clk_hw);
> > +	struct hi655x_pmic *hi655x = hi655x_clk->hi655x;
> > +	int ret;
> > +	uint32_t val;
> > +
> > +	ret = regmap_read(hi655x->regmap, HI655X_CLK_BASE, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (val & HI655X_CLK_BASE);
> 
> Useless parenthesis.

Ok.
 
> > +}
> > +
> > +static const struct clk_ops hi655x_clk_ops = {
> > +	.prepare     = hi655x_clk_prepare,
> > +	.unprepare   = hi655x_clk_unprepare,
> > +	.is_prepared = hi655x_clk_is_prepared,
> > +	.recalc_rate = hi655x_clk_recalc_rate,
> > +};
> > +
> > +static struct clk_hw *of_clk_hi655x_get(struct of_phandle_args *clkspec,
> > +					void *data)
> > +{
> > +	struct hi655x_clk *hi655x_clk = data;
> > +
> > +	return &hi655x_clk->clk_hw;
> > +}
> 
> Just use of_clk_hw_simple_get()?

Ah, yes :)

> > +
> > +static int hi655x_clk_probe(struct platform_device *pdev)
> > +{
> > +	struct device *parent = pdev->dev.parent;
> > +	struct hi655x_pmic *hi655x = dev_get_drvdata(parent);
> > +	struct clk_init_data *hi655x_clk_init;
> > +	struct hi655x_clk *hi655x_clk;
> > +	const char *clk_name = "hi655x-clk";
> 
> Why do we set it and then overwrite it?

Mmh, yeah. Actually, the clock name is not mandatory, so this name is the
default name in case it is not defined in the DT. However, if it is the case,
the function exits. 
The code should continue even if of_property_read_string_index fails.

> > +	int ret;
> > +
> > +	hi655x_clk = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk), GFP_KERNEL);
> > +	if (!hi655x_clk)
> > +		return -ENOMEM;
> > +
> > +	hi655x_clk_init = devm_kzalloc(&pdev->dev, sizeof(*hi655x_clk_init), GFP_KERNEL);
> > +	if (!hi655x_clk_init)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_string_index(parent->of_node, "clock-output-names",
> > +					    0, &clk_name);
> > +	if (ret)
> > +		return ret;
> > +
> > +	hi655x_clk_init->name	= clk_name;
> > +	hi655x_clk_init->ops	= &hi655x_clk_ops;
> > +
> > +	hi655x_clk->clk_hw.init	= hi655x_clk_init;
> > +	hi655x_clk->hi655x	= hi655x;
> > +
> > +	platform_set_drvdata(pdev, hi655x_clk);
> > +
> > +	ret = devm_clk_hw_register(&pdev->dev, &hi655x_clk->clk_hw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_hw_register_clkdev(&hi655x_clk->clk_hw, clk_name, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return of_clk_add_hw_provider(parent->of_node,
> > +				      of_clk_hi655x_get, hi655x_clk);
> 
> We forgot to drop the clkdev here if this fails.

Ok.

Thanks for the review.

  -- Daniel

> > +}
> > +
> > +static struct platform_driver hi655x_clk_driver = {
> > +	.probe =  hi655x_clk_probe,
> > +	.driver		= {
> > +		.name	= "hi655x-clk",
> > +	},
> > +};
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[Stephen Boyd]

On 04/07, Daniel Lezcano wrote:
> On Fri, Apr 07, 2017 at 09:48:51AM -0700, Stephen Boyd wrote:
> > On 03/17, Daniel Lezcano wrote:
> > > The hi655x multi function device is a PMIC providing regulators.
> > > 
> > > The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
> > > this clock in order to add it in the hi655x MFD and allow proper wireless
> > > initialization.
> > > 
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 
> > Is there a binding patch for the PMIC?
> 
> There is a binding for the hi655x at:
> 
> Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> 
> but I don't see what I should add there.

#clock-cells?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: hi6220: Add the hi655x's pmic clock

[Daniel Lezcano]

On Fri, Apr 07, 2017 at 10:23:02AM -0700, Stephen Boyd wrote:
> On 04/07, Daniel Lezcano wrote:
> > On Fri, Apr 07, 2017 at 09:48:51AM -0700, Stephen Boyd wrote:
> > > On 03/17, Daniel Lezcano wrote:
> > > > The hi655x multi function device is a PMIC providing regulators.
> > > > 
> > > > The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
> > > > this clock in order to add it in the hi655x MFD and allow proper wireless
> > > > initialization.
> > > > 
> > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > 
> > > Is there a binding patch for the PMIC?
> > 
> > There is a binding for the hi655x at:
> > 
> > Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> > 
> > but I don't see what I should add there.
> 
> #clock-cells?

Ok, thanks. I will have a look.



> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog