[PATCH v3 1/4] extcon: arizona: Remove enable/disable of 32k clock

[PATCH v3 1/4] extcon: arizona: Remove enable/disable of 32k clock

[Charles Keepax]

The 32k clock is unconditionally enabled by the MFD core so there is no
need to control it from the extcon device, so this patch removes the
control of the 32k clock.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---

No changes since v2.

Thanks,
Charles

 drivers/extcon/extcon-arizona.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index e4890dd..52d041f 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -1571,7 +1571,6 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 		goto err_micdet;
 	}
 
-	arizona_clk32k_enable(arizona);
 	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_DEBOUNCE,
 			   ARIZONA_JD1_DB, ARIZONA_JD1_DB);
 	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_ANALOGUE,
@@ -1642,7 +1641,6 @@ static int arizona_extcon_remove(struct platform_device *pdev)
 	cancel_delayed_work_sync(&info->hpdet_work);
 	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_ANALOGUE,
 			   ARIZONA_JD1_ENA, 0);
-	arizona_clk32k_disable(arizona);
 
 	return 0;
 }
-- 
2.1.4

[PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

[Charles Keepax]

Add an initial clock driver for the Arizona series audio CODECs.
Currently this driver only provides support for parsing the two input
clocks (mclk1, mclk2) and providing the internally consumed 32k clock.

Signed-off-by: Charles Keepax <ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---

Changes since v2:
 - Added missing include of clk.h which was causing build issues on Xtensa.

Thanks,
Charles

 MAINTAINERS                       |   1 +
 drivers/clk/Kconfig               |   6 ++
 drivers/clk/Makefile              |   1 +
 drivers/clk/clk-arizona.c         | 193 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/arizona/pdata.h |   3 +
 5 files changed, 204 insertions(+)
 create mode 100644 drivers/clk/clk-arizona.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 233f834..29e161a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11681,6 +11681,7 @@ F:	Documentation/devicetree/bindings/regulator/arizona-regulator.txt
 F:	Documentation/devicetree/bindings/mfd/arizona.txt
 F:	arch/arm/mach-s3c64xx/mach-crag6410*
 F:	drivers/clk/clk-wm83*.c
+F:	drivers/clk/clk-arizona.c
 F:	drivers/extcon/extcon-arizona.c
 F:	drivers/leds/leds-wm83*.c
 F:	drivers/gpio/gpio-*wm*.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 41f6c7f..2a88046 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -25,6 +25,12 @@ config COMMON_CLK
 menu "Common Clock Framework"
 	depends on COMMON_CLK
 
+config COMMON_CLK_ARIZONA
+	tristate "Clock driver for Arizona devices"
+	depends on MFD_ARIZONA
+	---help---
+	  This driver supports the clocking on the Arizona devices.
+
 config COMMON_CLK_WM831X
 	tristate "Clock driver for WM831x/2x PMICs"
 	depends on MFD_WM831X
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index b038e36..2b9cd1e 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -17,6 +17,7 @@ endif
 
 # hardware specific clock types
 # please keep this section sorted lexicographically by file/directory path name
+obj-$(CONFIG_COMMON_CLK_ARIZONA)	+= clk-arizona.o
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
new file mode 100644
index 0000000..eaf2877
--- /dev/null
+++ b/drivers/clk/clk-arizona.c
@@ -0,0 +1,193 @@
+/*
+ * Arizona clock control
+ *
+ * Copyright 2016 Cirrus Logic, Inc.
+ *
+ * Author: Charles Keepax <ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
+ *
+ *  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.
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include <linux/mfd/arizona/core.h>
+#include <linux/mfd/arizona/pdata.h>
+#include <linux/mfd/arizona/registers.h>
+
+#define CLK32K_RATE 32768
+
+struct arizona_clk {
+	struct arizona *arizona;
+
+	struct clk_hw clk32k_hw;
+	struct clk *clk32k;
+};
+
+static inline struct arizona_clk *clk32k_to_arizona_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct arizona_clk, clk32k_hw);
+}
+
+static int arizona_32k_enable(struct clk_hw *hw)
+{
+	struct arizona_clk *clkdata = clk32k_to_arizona_clk(hw);
+	struct arizona *arizona = clkdata->arizona;
+	int ret;
+
+	switch (arizona->pdata.clk32k_src) {
+	case ARIZONA_32KZ_MCLK1:
+		ret = pm_runtime_get_sync(arizona->dev);
+		if (ret)
+			goto out;
+		break;
+	}
+
+	ret = regmap_update_bits_async(arizona->regmap, ARIZONA_CLOCK_32K_1,
+				       ARIZONA_CLK_32K_ENA,
+				       ARIZONA_CLK_32K_ENA);
+
+out:
+	return ret;
+}
+
+static void arizona_32k_disable(struct clk_hw *hw)
+{
+	struct arizona_clk *clkdata = clk32k_to_arizona_clk(hw);
+	struct arizona *arizona = clkdata->arizona;
+
+	regmap_update_bits_async(arizona->regmap, ARIZONA_CLOCK_32K_1,
+				 ARIZONA_CLK_32K_ENA, 0);
+
+	switch (arizona->pdata.clk32k_src) {
+	case ARIZONA_32KZ_MCLK1:
+		pm_runtime_put_sync(arizona->dev);
+		break;
+	}
+}
+
+static const struct clk_ops arizona_32k_ops = {
+	.prepare = arizona_32k_enable,
+	.unprepare = arizona_32k_disable,
+};
+
+static int arizona_clk_of_get_pdata(struct arizona *arizona)
+{
+	const char * const pins[] = { "mclk1", "mclk2" };
+	struct clk *mclk;
+	int i;
+
+	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
+		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
+		if (IS_ERR(mclk))
+			return PTR_ERR(mclk);
+
+		if (clk_get_rate(mclk) == CLK32K_RATE) {
+			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
+			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
+		}
+
+		clk_put(mclk);
+	}
+
+	return 0;
+}
+
+static int arizona_clk_probe(struct platform_device *pdev)
+{
+	struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
+	struct arizona_clk *clkdata;
+	int ret;
+
+	struct clk_init_data clk32k_init = {
+		.name = "arizona-32k",
+		.ops = &arizona_32k_ops,
+	};
+
+	if (IS_ENABLED(CONFIG_OF) && !dev_get_platdata(arizona->dev)) {
+		ret = arizona_clk_of_get_pdata(arizona);
+		if (ret) {
+			dev_err(arizona->dev, "Failed parsing clock DT: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	clkdata = devm_kzalloc(&pdev->dev, sizeof(*clkdata), GFP_KERNEL);
+	if (!clkdata)
+		return -ENOMEM;
+
+	clkdata->arizona = arizona;
+
+	switch (arizona->pdata.clk32k_src) {
+	case 0:
+		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
+		/* Fall through */
+	case ARIZONA_32KZ_MCLK1:
+	case ARIZONA_32KZ_MCLK2:
+	case ARIZONA_32KZ_NONE:
+		regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
+				   ARIZONA_CLK_32K_SRC_MASK,
+				   arizona->pdata.clk32k_src - 1);
+		break;
+	default:
+		dev_err(arizona->dev, "Invalid 32kHz clock source: %d\n",
+			arizona->pdata.clk32k_src);
+		return -EINVAL;
+	}
+
+	if (arizona->pdata.clk32k_parent) {
+		clk32k_init.num_parents = 1;
+		clk32k_init.parent_names = &arizona->pdata.clk32k_parent;
+	} else {
+		clk32k_init.flags |= CLK_IS_ROOT;
+	}
+
+	clkdata->clk32k_hw.init = &clk32k_init;
+	clkdata->clk32k = devm_clk_register(&pdev->dev, &clkdata->clk32k_hw);
+	if (IS_ERR(clkdata->clk32k)) {
+		ret = PTR_ERR(clkdata->clk32k);
+		dev_err(arizona->dev, "Failed to register 32k clock: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = clk_register_clkdev(clkdata->clk32k, "arizona-32k",
+				  dev_name(arizona->dev));
+	if (ret) {
+		dev_err(arizona->dev, "Failed to register 32k clock dev: %d\n",
+			ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, clkdata);
+
+	return 0;
+}
+
+static struct platform_driver arizona_clk_driver = {
+	.probe = arizona_clk_probe,
+	.driver		= {
+		.name	= "arizona-clk",
+	},
+};
+
+module_platform_driver(arizona_clk_driver);
+
+/* Module information */
+MODULE_AUTHOR("Charles Keepax <ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>");
+MODULE_DESCRIPTION("Clock driver for Arizona devices");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:arizona-clk");
diff --git a/include/linux/mfd/arizona/pdata.h b/include/linux/mfd/arizona/pdata.h
index 57b45ca..ddeee17 100644
--- a/include/linux/mfd/arizona/pdata.h
+++ b/include/linux/mfd/arizona/pdata.h
@@ -87,6 +87,9 @@ struct arizona_pdata {
 	/** If a direct 32kHz clock is provided on an MCLK specify it here */
 	int clk32k_src;
 
+	/** Name of the parent clock for the 32k clock */
+	const char *clk32k_parent;
+
 	/** Mode for primary IRQ (defaults to active low) */
 	unsigned int irq_flags;
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Charles Keepax]

Now we have a clock driver that can control the 32k clock use this
rather than directly controlling the 32k clock from the MFD device.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---

No changes since v2.

Thanks,
Charles

 drivers/mfd/arizona-core.c       | 104 +++++++++------------------------------
 include/linux/mfd/arizona/core.h |   7 +--
 2 files changed, 25 insertions(+), 86 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index d474732..5a55dd6 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -36,63 +36,6 @@ static const char * const wm5102_core_supplies[] = {
 	"DBVDD1",
 };
 
-int arizona_clk32k_enable(struct arizona *arizona)
-{
-	int ret = 0;
-
-	mutex_lock(&arizona->clk_lock);
-
-	arizona->clk32k_ref++;
-
-	if (arizona->clk32k_ref == 1) {
-		switch (arizona->pdata.clk32k_src) {
-		case ARIZONA_32KZ_MCLK1:
-			ret = pm_runtime_get_sync(arizona->dev);
-			if (ret != 0)
-				goto out;
-			break;
-		}
-
-		ret = regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
-					 ARIZONA_CLK_32K_ENA,
-					 ARIZONA_CLK_32K_ENA);
-	}
-
-out:
-	if (ret != 0)
-		arizona->clk32k_ref--;
-
-	mutex_unlock(&arizona->clk_lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(arizona_clk32k_enable);
-
-int arizona_clk32k_disable(struct arizona *arizona)
-{
-	mutex_lock(&arizona->clk_lock);
-
-	BUG_ON(arizona->clk32k_ref <= 0);
-
-	arizona->clk32k_ref--;
-
-	if (arizona->clk32k_ref == 0) {
-		regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
-				   ARIZONA_CLK_32K_ENA, 0);
-
-		switch (arizona->pdata.clk32k_src) {
-		case ARIZONA_32KZ_MCLK1:
-			pm_runtime_put_sync(arizona->dev);
-			break;
-		}
-	}
-
-	mutex_unlock(&arizona->clk_lock);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
-
 static irqreturn_t arizona_clkgen_err(int irq, void *data)
 {
 	struct arizona *arizona = data;
@@ -874,6 +817,7 @@ static inline int arizona_of_get_core_pdata(struct arizona *arizona)
 
 static const struct mfd_cell early_devs[] = {
 	{ .name = "arizona-ldo1" },
+	{ .name = "arizona-clk" },
 };
 
 static const char * const wm5102_supplies[] = {
@@ -970,7 +914,6 @@ int arizona_dev_init(struct arizona *arizona)
 	int n_subdevs, ret, i;
 
 	dev_set_drvdata(arizona->dev, arizona);
-	mutex_init(&arizona->clk_lock);
 
 	if (dev_get_platdata(arizona->dev))
 		memcpy(&arizona->pdata, dev_get_platdata(arizona->dev),
@@ -1261,28 +1204,6 @@ int arizona_dev_init(struct arizona *arizona)
 	}
 
 	/* Chip default */
-	if (!arizona->pdata.clk32k_src)
-		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
-
-	switch (arizona->pdata.clk32k_src) {
-	case ARIZONA_32KZ_MCLK1:
-	case ARIZONA_32KZ_MCLK2:
-		regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
-				   ARIZONA_CLK_32K_SRC_MASK,
-				   arizona->pdata.clk32k_src - 1);
-		arizona_clk32k_enable(arizona);
-		break;
-	case ARIZONA_32KZ_NONE:
-		regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
-				   ARIZONA_CLK_32K_SRC_MASK, 2);
-		break;
-	default:
-		dev_err(arizona->dev, "Invalid 32kHz clock source: %d\n",
-			arizona->pdata.clk32k_src);
-		ret = -EINVAL;
-		goto err_reset;
-	}
-
 	for (i = 0; i < ARIZONA_MAX_MICBIAS; i++) {
 		if (!arizona->pdata.micbias[i].mV &&
 		    !arizona->pdata.micbias[i].bypass)
@@ -1387,10 +1308,25 @@ int arizona_dev_init(struct arizona *arizona)
 	pm_runtime_set_active(arizona->dev);
 	pm_runtime_enable(arizona->dev);
 
+	arizona->clk32k = devm_clk_get(arizona->dev, "arizona-32k");
+	if (IS_ERR(arizona->clk32k)) {
+		ret = PTR_ERR(arizona->clk32k);
+		if (ret == -ENOENT)
+			ret = -EPROBE_DEFER;
+		dev_err(arizona->dev, "Failed to get 32k clock: %d\n", ret);
+		goto err_pm;
+	}
+
+	ret = clk_prepare_enable(arizona->clk32k);
+	if (ret < 0) {
+		dev_err(arizona->dev, "Failed to enable 32k clock: %d\n", ret);
+		goto err_pm;
+	}
+
 	/* Set up for interrupts */
 	ret = arizona_irq_init(arizona);
 	if (ret != 0)
-		goto err_reset;
+		goto err_clock;
 
 	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
 	pm_runtime_use_autosuspend(arizona->dev);
@@ -1414,6 +1350,10 @@ int arizona_dev_init(struct arizona *arizona)
 
 err_irq:
 	arizona_irq_exit(arizona);
+err_clock:
+	clk_disable_unprepare(arizona->clk32k);
+err_pm:
+	pm_runtime_disable(arizona->dev);
 err_reset:
 	arizona_enable_reset(arizona);
 	regulator_disable(arizona->dcvdd);
@@ -1430,6 +1370,8 @@ EXPORT_SYMBOL_GPL(arizona_dev_init);
 
 int arizona_dev_exit(struct arizona *arizona)
 {
+	clk_disable_unprepare(arizona->clk32k);
+
 	pm_runtime_disable(arizona->dev);
 
 	regulator_disable(arizona->dcvdd);
diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
index 79e607e..b9a1da4 100644
--- a/include/linux/mfd/arizona/core.h
+++ b/include/linux/mfd/arizona/core.h
@@ -13,6 +13,7 @@
 #ifndef _WM_ARIZONA_CORE_H
 #define _WM_ARIZONA_CORE_H
 
+#include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -133,8 +134,7 @@ struct arizona {
 	bool hpdet_clamp;
 	unsigned int hp_ena;
 
-	struct mutex clk_lock;
-	int clk32k_ref;
+	struct clk *clk32k;
 
 	bool ctrlif_error;
 
@@ -148,9 +148,6 @@ struct arizona {
 	struct mutex dac_comp_lock;
 };
 
-int arizona_clk32k_enable(struct arizona *arizona);
-int arizona_clk32k_disable(struct arizona *arizona);
-
 int arizona_request_irq(struct arizona *arizona, int irq, char *name,
 			irq_handler_t handler, void *data);
 void arizona_free_irq(struct arizona *arizona, int irq, void *data);
-- 
2.1.4

[PATCH v3 4/4] mfd: arizona: Add device tree binding documentation for new clock driver

[Charles Keepax]

Specify the device tree binding for the input clocks to Arizona devices.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Acked-by: Rob Herring <robh@kernel.org>
---

No changes since v2.

Thanks,
Charles

 Documentation/devicetree/bindings/mfd/arizona.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
index 18be0cb..39f76f8 100644
--- a/Documentation/devicetree/bindings/mfd/arizona.txt
+++ b/Documentation/devicetree/bindings/mfd/arizona.txt
@@ -45,6 +45,13 @@ Optional properties:
 
   - wlf,reset : GPIO specifier for the GPIO controlling /RESET
 
+  - clocks: Should reference the clocks supplied on MCLK1 and MCLK2
+  - clock-names: Should contains two strings:
+      "mclk1" for the clock supplied on MCLK1, recommended to be a high
+      quality audio reference clock
+      "mclk2" for the clock supplied on MCLK2, recommended to be an always on
+      32k clock
+
   - wlf,gpio-defaults : A list of GPIO configuration register values. Defines
     for the appropriate values can found in <dt-bindings/mfd/arizona.txt>. If
     absent, no configuration of these registers is performed. If any entry has
-- 
2.1.4

Re: [PATCH v3 4/4] mfd: arizona: Add device tree binding documentation for new clock driver

[Lee Jones]

On Fri, 08 Jan 2016, Charles Keepax wrote:

> Specify the device tree binding for the input clocks to Arizona devices.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> 
> No changes since v2.
> 
> Thanks,
> Charles
> 
>  Documentation/devicetree/bindings/mfd/arizona.txt | 7 +++++++
>  1 file changed, 7 insertions(+)

Applied, thanks.

> diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
> index 18be0cb..39f76f8 100644
> --- a/Documentation/devicetree/bindings/mfd/arizona.txt
> +++ b/Documentation/devicetree/bindings/mfd/arizona.txt
> @@ -45,6 +45,13 @@ Optional properties:
>  
>    - wlf,reset : GPIO specifier for the GPIO controlling /RESET
>  
> +  - clocks: Should reference the clocks supplied on MCLK1 and MCLK2
> +  - clock-names: Should contains two strings:
> +      "mclk1" for the clock supplied on MCLK1, recommended to be a high
> +      quality audio reference clock
> +      "mclk2" for the clock supplied on MCLK2, recommended to be an always on
> +      32k clock
> +
>    - wlf,gpio-defaults : A list of GPIO configuration register values. Defines
>      for the appropriate values can found in <dt-bindings/mfd/arizona.txt>. If
>      absent, no configuration of these registers is performed. If any entry has

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

Re: [PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Lee Jones]

On Fri, 08 Jan 2016, Charles Keepax wrote:

> Now we have a clock driver that can control the 32k clock use this
> rather than directly controlling the 32k clock from the MFD device.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> No changes since v2.
> 
> Thanks,
> Charles
> 
>  drivers/mfd/arizona-core.c       | 104 +++++++++------------------------------
>  include/linux/mfd/arizona/core.h |   7 +--
>  2 files changed, 25 insertions(+), 86 deletions(-)

I guess this needs to go in with the Clk changes, which are yet to be
reviewed, right?

For my own reference:
  Acked-by: Lee Jones <lee.jones@linaro.org>

[...]

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

Re: [PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Charles Keepax]

On Mon, Jan 11, 2016 at 10:25:46AM +0000, Lee Jones wrote:
> On Fri, 08 Jan 2016, Charles Keepax wrote:
> 
> > Now we have a clock driver that can control the 32k clock use this
> > rather than directly controlling the 32k clock from the MFD device.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> > 
> > No changes since v2.
> > 
> > Thanks,
> > Charles
> > 
> >  drivers/mfd/arizona-core.c       | 104 +++++++++------------------------------
> >  include/linux/mfd/arizona/core.h |   7 +--
> >  2 files changed, 25 insertions(+), 86 deletions(-)
> 
> I guess this needs to go in with the Clk changes, which are yet to be
> reviewed, right?
> 
> For my own reference:
>   Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> [...]

Yeah this needs to go in with the clock changes themselves.

Thanks,
Charles

Re: [PATCH v3 1/4] extcon: arizona: Remove enable/disable of 32k clock

[Chanwoo Choi]

On 2016년 01월 08일 20:29, Charles Keepax wrote:
> The 32k clock is unconditionally enabled by the MFD core so there is no
> need to control it from the extcon device, so this patch removes the
> control of the 32k clock.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> No changes since v2.
> 
> Thanks,
> Charles
> 
>  drivers/extcon/extcon-arizona.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index e4890dd..52d041f 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -1571,7 +1571,6 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>  		goto err_micdet;
>  	}
>  
> -	arizona_clk32k_enable(arizona);
>  	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_DEBOUNCE,
>  			   ARIZONA_JD1_DB, ARIZONA_JD1_DB);
>  	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_ANALOGUE,
> @@ -1642,7 +1641,6 @@ static int arizona_extcon_remove(struct platform_device *pdev)
>  	cancel_delayed_work_sync(&info->hpdet_work);
>  	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_ANALOGUE,
>  			   ARIZONA_JD1_ENA, 0);
> -	arizona_clk32k_disable(arizona);
>  
>  	return 0;
>  }
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

If I apply only this patch on extcon tree,
the kernel build error will happen on other tree.
I think that this patch will go on clk tree and
then send pull request of immutable branch.

Thanks,
Chanwoo Choi

Re: [PATCH v3 4/4] mfd: arizona: Add device tree binding documentation for new clock driver

[Stephen Boyd]

On 01/08/2016 03:29 AM, Charles Keepax wrote:
> Specify the device tree binding for the input clocks to Arizona devices.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>
> No changes since v2.
>
> Thanks,
> Charles
>
>  Documentation/devicetree/bindings/mfd/arizona.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
> index 18be0cb..39f76f8 100644
> --- a/Documentation/devicetree/bindings/mfd/arizona.txt
> +++ b/Documentation/devicetree/bindings/mfd/arizona.txt
> @@ -45,6 +45,13 @@ Optional properties:
>  
>    - wlf,reset : GPIO specifier for the GPIO controlling /RESET
>  
> +  - clocks: Should reference the clocks supplied on MCLK1 and MCLK2
> +  - clock-names: Should contains two strings:

Can't this be clock-output-names instead? That's standard and already
accepted per clock-bindings.txt

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

Re: [PATCH v3 4/4] mfd: arizona: Add device tree binding documentation for new clock driver

[Stephen Boyd]

On 05/06/2016 05:47 PM, Stephen Boyd wrote:
> On 01/08/2016 03:29 AM, Charles Keepax wrote:
>> Specify the device tree binding for the input clocks to Arizona devices.
>>
>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>
>> No changes since v2.
>>
>> Thanks,
>> Charles
>>
>>  Documentation/devicetree/bindings/mfd/arizona.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
>> index 18be0cb..39f76f8 100644
>> --- a/Documentation/devicetree/bindings/mfd/arizona.txt
>> +++ b/Documentation/devicetree/bindings/mfd/arizona.txt
>> @@ -45,6 +45,13 @@ Optional properties:
>>  
>>    - wlf,reset : GPIO specifier for the GPIO controlling /RESET
>>  
>> +  - clocks: Should reference the clocks supplied on MCLK1 and MCLK2
>> +  - clock-names: Should contains two strings:
> Can't this be clock-output-names instead? That's standard and already
> accepted per clock-bindings.txt
>

Oh this is input, nevermind.

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

Re: [PATCH v3 1/4] extcon: arizona: Remove enable/disable of 32k clock

[Stephen Boyd]

On 01/08, Charles Keepax wrote:
> The 32k clock is unconditionally enabled by the MFD core so there is no
> need to control it from the extcon device, so this patch removes the
> control of the 32k clock.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---

Applied to clk-next

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

Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

[Stephen Boyd]

I've applied this to clk-next but still have a question, see
below.

On 01/08, Charles Keepax wrote:
> diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
> new file mode 100644
> index 0000000..eaf2877
> --- /dev/null
> +++ b/drivers/clk/clk-arizona.c
> +
> +static int arizona_clk_of_get_pdata(struct arizona *arizona)
> +{
> +	const char * const pins[] = { "mclk1", "mclk2" };
> +	struct clk *mclk;
> +	int i;
> +
> +	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
> +		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
> +		if (IS_ERR(mclk))
> +			return PTR_ERR(mclk);
> +
> +		if (clk_get_rate(mclk) == CLK32K_RATE) {
> +			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
> +			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
> +		}
> +
> +		clk_put(mclk);

Could this be done through assigned parents instead of this rate
checking stuff? Presumably DT could tell us how the clk tree
should be configured.

> +	}
> +
> +	return 0;
> +}
> +
> +static int arizona_clk_probe(struct platform_device *pdev)
> +{
> +	struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
> +	struct arizona_clk *clkdata;
> +	int ret;
> +
> +	struct clk_init_data clk32k_init = {
> +		.name = "arizona-32k",
> +		.ops = &arizona_32k_ops,
> +	};
> +
> +	if (IS_ENABLED(CONFIG_OF) && !dev_get_platdata(arizona->dev)) {
> +		ret = arizona_clk_of_get_pdata(arizona);
> +		if (ret) {
> +			dev_err(arizona->dev, "Failed parsing clock DT: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	clkdata = devm_kzalloc(&pdev->dev, sizeof(*clkdata), GFP_KERNEL);
> +	if (!clkdata)
> +		return -ENOMEM;
> +
> +	clkdata->arizona = arizona;
> +
> +	switch (arizona->pdata.clk32k_src) {
> +	case 0:
> +		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
> +		/* Fall through */
> +	case ARIZONA_32KZ_MCLK1:
> +	case ARIZONA_32KZ_MCLK2:
> +	case ARIZONA_32KZ_NONE:
> +		regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
> +				   ARIZONA_CLK_32K_SRC_MASK,
> +				   arizona->pdata.clk32k_src - 1);
> +		break;
> +	default:
> +		dev_err(arizona->dev, "Invalid 32kHz clock source: %d\n",
> +			arizona->pdata.clk32k_src);
> +		return -EINVAL;
> +	}
> +
> +	if (arizona->pdata.clk32k_parent) {
> +		clk32k_init.num_parents = 1;
> +		clk32k_init.parent_names = &arizona->pdata.clk32k_parent;
> +	} else {
> +		clk32k_init.flags |= CLK_IS_ROOT;

This flag is going away so I deleted it.

> +	}
> +
> +	clkdata->clk32k_hw.init = &clk32k_init;
> +	clkdata->clk32k = devm_clk_register(&pdev->dev, &clkdata->clk32k_hw);
> +	if (IS_ERR(clkdata->clk32k)) {
> +		ret = PTR_ERR(clkdata->clk32k);
> +		dev_err(arizona->dev, "Failed to register 32k clock: %d\n",
> +			ret);
> +		return ret;
> +	}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Stephen Boyd]

On 01/08, Charles Keepax wrote:
> Now we have a clock driver that can control the 32k clock use this
> rather than directly controlling the 32k clock from the MFD device.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---

Applied to clk-next

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

Re: [PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Lee Jones]

On Fri, 06 May 2016, Stephen Boyd wrote:

> On 01/08, Charles Keepax wrote:
> > Now we have a clock driver that can control the 32k clock use this
> > rather than directly controlling the 32k clock from the MFD device.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> 
> Applied to clk-next

If you're going to take the set though the clk tree, don't forget to
sent out a pull-request of the immutable branch you created.  If
you're not happy to do that, drop the patch and I'll happily oblige.

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

Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

[Charles Keepax]

On Fri, May 06, 2016 at 05:55:01PM -0700, Stephen Boyd wrote:
> I've applied this to clk-next but still have a question, see
> below.
> 
> On 01/08, Charles Keepax wrote:
> > diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
> > new file mode 100644
> > index 0000000..eaf2877
> > --- /dev/null
> > +++ b/drivers/clk/clk-arizona.c
> > +
> > +static int arizona_clk_of_get_pdata(struct arizona *arizona)
> > +{
> > +	const char * const pins[] = { "mclk1", "mclk2" };
> > +	struct clk *mclk;
> > +	int i;
> > +
> > +	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
> > +		return 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
> > +		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
> > +		if (IS_ERR(mclk))
> > +			return PTR_ERR(mclk);
> > +
> > +		if (clk_get_rate(mclk) == CLK32K_RATE) {
> > +			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
> > +			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
> > +		}
> > +
> > +		clk_put(mclk);
> 
> Could this be done through assigned parents instead of this rate
> checking stuff? Presumably DT could tell us how the clk tree
> should be configured.
> 

Apologies, I have been working on a v4 that includes these
improvements. It does indeed look much nicer using assigned
parents etc. I think it might be best to drop these for now until
those are ready to send.

The only problem I really have left to sort out before I can send
it are some locking issues. It is quite tricky to get interaction
between the clocking and SPI frameworks to play nicely. The SPI
framework will sometimes punt the actually processing for the
transfer to a worker thread which will often perform operations
on clocks required for the SPI. Because this is a seperate
thread it isn't handled by the re-enterant locking in the clock
framework. I had been working around this using async transfers
for the SPI, but even then I have since found you can get lockdep
warnings because of the potential mutex inversion (SPI mutex and
the clock one).

Any suggestions on this front would be greatly appreciated?

Thanks, Charles

Re: [PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Stephen Boyd]

On 05/09, Lee Jones wrote:
> On Fri, 06 May 2016, Stephen Boyd wrote:
> 
> > On 01/08, Charles Keepax wrote:
> > > Now we have a clock driver that can control the 32k clock use this
> > > rather than directly controlling the 32k clock from the MFD device.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > 
> > Applied to clk-next
> 
> If you're going to take the set though the clk tree, don't forget to
> sent out a pull-request of the immutable branch you created.  If
> you're not happy to do that, drop the patch and I'll happily oblige.
> 

Oh sorry, I got confused. I thought you acked this mfd patch so I could
take it through the clk tree but it seems you acked them so you
could apply it later without re-reviewing it?

Either way, I'm going to drop these patches for now and wait for
a v4 because Charles has suggested there are some more issues to
work out.

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

Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

[Stephen Boyd]

On 05/09, Charles Keepax wrote:
> On Fri, May 06, 2016 at 05:55:01PM -0700, Stephen Boyd wrote:
> > I've applied this to clk-next but still have a question, see
> > below.
> > 
> > On 01/08, Charles Keepax wrote:
> > > diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
> > > new file mode 100644
> > > index 0000000..eaf2877
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-arizona.c
> > > +
> > > +static int arizona_clk_of_get_pdata(struct arizona *arizona)
> > > +{
> > > +	const char * const pins[] = { "mclk1", "mclk2" };
> > > +	struct clk *mclk;
> > > +	int i;
> > > +
> > > +	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
> > > +		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
> > > +		if (IS_ERR(mclk))
> > > +			return PTR_ERR(mclk);
> > > +
> > > +		if (clk_get_rate(mclk) == CLK32K_RATE) {
> > > +			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
> > > +			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
> > > +		}
> > > +
> > > +		clk_put(mclk);
> > 
> > Could this be done through assigned parents instead of this rate
> > checking stuff? Presumably DT could tell us how the clk tree
> > should be configured.
> > 
> 
> Apologies, I have been working on a v4 that includes these
> improvements. It does indeed look much nicer using assigned
> parents etc. I think it might be best to drop these for now until
> those are ready to send.

Ok sure. I've dropped them.

> 
> The only problem I really have left to sort out before I can send
> it are some locking issues. It is quite tricky to get interaction
> between the clocking and SPI frameworks to play nicely. The SPI
> framework will sometimes punt the actually processing for the
> transfer to a worker thread which will often perform operations
> on clocks required for the SPI. Because this is a seperate
> thread it isn't handled by the re-enterant locking in the clock
> framework. I had been working around this using async transfers
> for the SPI, but even then I have since found you can get lockdep
> warnings because of the potential mutex inversion (SPI mutex and
> the clock one).
> 
> Any suggestions on this front would be greatly appreciated?
> 

The fix is to always prepare the first clk right? That way we
avoid any deadlock scenario.

We've been slowly working our way toward an alternate solution,
which is to have one mutex per clk so that different parts of the
clk tree can be locked independently, but so far that's blocked
on drivers re-entering the clk framework with clk consumer APIs
from within the clk_ops callbacks. Hopefully coordinated clk rate
switches will allow us to get rid of those situations and then we
can go and make sure all drivers aren't relying on the big
prepare mutex to keep their drivers safe from concurrent accesses
and finally move to one mutex per clk. This is a long term goal
though so I wouldn't depend on this happening anytime soon.

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

Re: [PATCH v3 3/4] mfd: arizona: Switch to using clock driver for 32k clock

[Lee Jones]

On Mon, 09 May 2016, Stephen Boyd wrote:
> On 05/09, Lee Jones wrote:
> > On Fri, 06 May 2016, Stephen Boyd wrote:
> > 
> > > On 01/08, Charles Keepax wrote:
> > > > Now we have a clock driver that can control the 32k clock use this
> > > > rather than directly controlling the 32k clock from the MFD device.
> > > > 
> > > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > ---
> > > 
> > > Applied to clk-next
> > 
> > If you're going to take the set though the clk tree, don't forget to
> > sent out a pull-request of the immutable branch you created.  If
> > you're not happy to do that, drop the patch and I'll happily oblige.
> > 
> 
> Oh sorry, I got confused. I thought you acked this mfd patch so I could
> take it through the clk tree but it seems you acked them so you
> could apply it later without re-reviewing it?

Right, that's what I mean by "for my own reference".  Since MFD is
more often than not the central/parent device, it's usually easier to
tunnel patch sets through the MFD, and I will always sent out a
pull-requests out to the other maintainers concerned in order to
prevent merge conflicts for Linus.

> Either way, I'm going to drop these patches for now and wait for
> a v4 because Charles has suggested there are some more issues to
> work out.

Sure.

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

Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

[Charles Keepax]

On Mon, May 09, 2016 at 02:48:29PM -0700, Stephen Boyd wrote:
> On 05/09, Charles Keepax wrote:
> > On Fri, May 06, 2016 at 05:55:01PM -0700, Stephen Boyd wrote:
> > > I've applied this to clk-next but still have a question, see
> > > below.
> > > 
> > > On 01/08, Charles Keepax wrote:
> > Apologies, I have been working on a v4 that includes these
> > improvements. It does indeed look much nicer using assigned
> > parents etc. I think it might be best to drop these for now until
> > those are ready to send.
> 
> Ok sure. I've dropped them.

Cool thanks.

> 
> > 
> > The only problem I really have left to sort out before I can send
> > it are some locking issues. It is quite tricky to get interaction
> > between the clocking and SPI frameworks to play nicely. The SPI
> > framework will sometimes punt the actually processing for the
> > transfer to a worker thread which will often perform operations
> > on clocks required for the SPI. Because this is a seperate
> > thread it isn't handled by the re-enterant locking in the clock
> > framework. I had been working around this using async transfers
> > for the SPI, but even then I have since found you can get lockdep
> > warnings because of the potential mutex inversion (SPI mutex and
> > the clock one).
> > 
> > Any suggestions on this front would be greatly appreciated?
> > 
> 
> The fix is to always prepare the first clk right? That way we
> avoid any deadlock scenario.

Yeah, been looking at that, the problem is our parts get used
in a fairly wide array of places and it tends to be up to the
individual SPI driver how the clocks are controlled. So feels a
bit like SPI driver whack a mole, perhaps something could be done
through the SPI core itself through.

> 
> We've been slowly working our way toward an alternate solution,
> which is to have one mutex per clk so that different parts of the
> clk tree can be locked independently, but so far that's blocked
> on drivers re-entering the clk framework with clk consumer APIs
> from within the clk_ops callbacks. Hopefully coordinated clk rate
> switches will allow us to get rid of those situations and then we
> can go and make sure all drivers aren't relying on the big
> prepare mutex to keep their drivers safe from concurrent accesses
> and finally move to one mutex per clk. This is a long term goal
> though so I wouldn't depend on this happening anytime soon.

Thanks, really good to hear some thoughts on this.  I had been
coming to the conclusion here that individual prepare locks was
the right course of action. I had tried some changes to allow
individual clock drivers to specify prepare_lock and unlock
callbacks and then having the clock core fall back to the global
lock if the driver didn't have these. Which is a sort of half way
house to individual locks, but its still quite easy to run into
mutex inversions with all the other clocks still sharing a global
lock.

Thanks,
Charles