[PATCH 0/6] imx patches when I enable imx6q sabrelite audio

[PATCH 0/6] imx patches when I enable imx6q sabrelite audio

[Richard Zhao]

[PATCH 1/6] ARM: mxc: make imx_dma_is_general_purpose more generic
[PATCH 2/6] ARM: imx6q: add cko1 clock
[PATCH 3/6] arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators
[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
[PATCH 5/6] ARM: mxc: add dt support for audmux-v2
[PATCH 6/6] ARM: imx6q-sabrelite: add audmux support

Thanks
Richard

[PATCH 1/6] ARM: mxc: make imx_dma_is_general_purpose more generic for sdma

[Richard Zhao]

sdma device names vary for different SoC. So we just check
whether it includes "sdma" substring.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/plat-mxc/include/mach/dma.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-mxc/include/mach/dma.h b/arch/arm/plat-mxc/include/mach/dma.h
index 233d0a5..1b90803 100644
--- a/arch/arm/plat-mxc/include/mach/dma.h
+++ b/arch/arm/plat-mxc/include/mach/dma.h
@@ -60,8 +60,7 @@ static inline int imx_dma_is_ipu(struct dma_chan *chan)
 
 static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
 {
-	return !strcmp(dev_name(chan->device->dev), "imx31-sdma") ||
-		!strcmp(dev_name(chan->device->dev), "imx35-sdma") ||
+	return strstr(dev_name(chan->device->dev), "sdma") ||
 		!strcmp(dev_name(chan->device->dev), "imx-dma");
 }
 
-- 
1.7.5.4

[PATCH 2/6] ARM: imx6q: add cko1 clock

[Richard Zhao]

- add DEF_CLK_1B to define clocks using one bit gate
- add cko1 clock and set ahb as the default parent

imx6q-sabrelite board use it as audio codec clock.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-imx/clock-imx6q.c |   74 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 73 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
index 039a7ab..82c7541 100644
--- a/arch/arm/mach-imx/clock-imx6q.c
+++ b/arch/arm/mach-imx/clock-imx6q.c
@@ -329,6 +329,12 @@
 #define BM_CLPCR_MASK_SCU_IDLE		(0x1 << 26)
 #define BM_CLPCR_MASK_L2CC_IDLE		(0x1 << 27)
 
+#define BP_CCOSR_CKO1_EN		7
+#define BP_CCOSR_CKO1_PODF		4
+#define BM_CCOSR_CKO1_PODF		(0x7 << 4)
+#define BP_CCOSR_CKO1_SEL		0
+#define BM_CCOSR_CKO1_SEL		(0xf << 0)
+
 #define FREQ_480M	480000000
 #define FREQ_528M	528000000
 #define FREQ_594M	594000000
@@ -393,6 +399,7 @@ static struct clk ipu1_di1_clk;
 static struct clk ipu2_di0_clk;
 static struct clk ipu2_di1_clk;
 static struct clk enfc_clk;
+static struct clk cko1_clk;
 static struct clk dummy_clk = {};
 
 static unsigned long external_high_reference;
@@ -928,6 +935,24 @@ static void _clk_disable(struct clk *clk)
 	writel_relaxed(reg, clk->enable_reg);
 }
 
+static int _clk_enable_1b(struct clk *clk)
+{
+	u32 reg;
+	reg = readl_relaxed(clk->enable_reg);
+	reg |= 0x1 << clk->enable_shift;
+	writel_relaxed(reg, clk->enable_reg);
+
+	return 0;
+}
+
+static void _clk_disable_1b(struct clk *clk)
+{
+	u32 reg;
+	reg = readl_relaxed(clk->enable_reg);
+	reg &= ~(0x1 << clk->enable_shift);
+	writel_relaxed(reg, clk->enable_reg);
+}
+
 struct divider {
 	struct clk *clk;
 	void __iomem *reg;
@@ -973,6 +998,7 @@ DEF_CLK_DIV1(ipu2_di0_pre_div,	&ipu2_di0_pre_clk,	CSCDR2,	IPU2_DI0_PRE);
 DEF_CLK_DIV1(ipu2_di1_pre_div,	&ipu2_di1_pre_clk,	CSCDR2,	IPU2_DI1_PRE);
 DEF_CLK_DIV1(ipu1_div,		&ipu1_clk,		CSCDR3,	IPU1_HSP);
 DEF_CLK_DIV1(ipu2_div,		&ipu2_clk,		CSCDR3,	IPU2_HSP);
+DEF_CLK_DIV1(cko1_div,		&cko1_clk,		CCOSR, CKO1);
 
 #define DEF_CLK_DIV2(d, c, r, b)				\
 	static struct divider d = {				\
@@ -1028,6 +1054,7 @@ static struct divider *dividers[] = {
 	&enfc_div,
 	&spdif_div,
 	&asrc_serial_div,
+	&cko1_div,
 };
 
 static unsigned long ldb_di_clk_get_rate(struct clk *clk)
@@ -1615,6 +1642,32 @@ DEF_IPU_DI_MUX(CSCDR2, 2, 1);
 DEF_IPU_MUX(1);
 DEF_IPU_MUX(2);
 
+static struct multiplexer cko1_mux = {
+	.clk = &cko1_clk,
+	.reg = CCOSR,
+	.bp = BP_CCOSR_CKO1_SEL,
+	.bm = BM_CCOSR_CKO1_SEL,
+	.parents = {
+		&pll3_usb_otg,
+		&pll2_bus,
+		&pll1_sys,
+		&pll5_video,
+		&dummy_clk,
+		&axi_clk,
+		&enfc_clk,
+		&ipu1_di0_clk,
+		&ipu1_di1_clk,
+		&ipu2_di0_clk,
+		&ipu2_di1_clk,
+		&ahb_clk,
+		&ipg_clk,
+		&ipg_perclk,
+		&ckil_clk,
+		&pll4_audio,
+		NULL
+	},
+};
+
 static struct multiplexer *multiplexers[] = {
 	&axi_mux,
 	&periph_mux,
@@ -1657,6 +1710,7 @@ static struct multiplexer *multiplexers[] = {
 	&ipu2_di1_mux,
 	&ipu1_mux,
 	&ipu2_mux,
+	&cko1_mux,
 };
 
 static int _clk_set_parent(struct clk *clk, struct clk *parent)
@@ -1680,7 +1734,7 @@ static int _clk_set_parent(struct clk *clk, struct clk *parent)
 			break;
 		i++;
 	}
-	if (!m->parents[i])
+	if (!m->parents[i] || m->parents[i] == &dummy_clk)
 		return -EINVAL;
 
 	val = readl_relaxed(m->reg);
@@ -1735,6 +1789,20 @@ DEF_NG_CLK(asrc_serial_clk,	&pll3_usb_otg);
 		.secondary	= s,			\
 	}
 
+#define DEF_CLK_1B(name, er, es, p, s)			\
+	static struct clk name = {			\
+		.enable_reg	= er,			\
+		.enable_shift	= es,			\
+		.enable		= _clk_enable_1b,	\
+		.disable	= _clk_disable_1b,	\
+		.get_rate	= _clk_get_rate,	\
+		.set_rate	= _clk_set_rate,	\
+		.round_rate	= _clk_round_rate,	\
+		.set_parent	= _clk_set_parent,	\
+		.parent		= p,			\
+		.secondary	= s,			\
+	}
+
 DEF_CLK(aips_tz1_clk,	  CCGR0, CG0,  &ahb_clk,	  NULL);
 DEF_CLK(aips_tz2_clk,	  CCGR0, CG1,  &ahb_clk,	  NULL);
 DEF_CLK(apbh_dma_clk,	  CCGR0, CG2,  &ahb_clk,	  NULL);
@@ -1801,6 +1869,7 @@ DEF_CLK(usdhc4_clk,	  CCGR6, CG4,  &pll2_pfd_400m,	  NULL);
 DEF_CLK(emi_slow_clk,	  CCGR6, CG5,  &axi_clk,	  NULL);
 DEF_CLK(vdo_axi_clk,	  CCGR6, CG6,  &axi_clk,	  NULL);
 DEF_CLK(vpu_clk,	  CCGR6, CG7,  &axi_clk,	  NULL);
+DEF_CLK_1B(cko1_clk,	  CCOSR, BP_CCOSR_CKO1_EN, &pll2_bus, NULL);
 
 static int pcie_clk_enable(struct clk *clk)
 {
@@ -1911,6 +1980,7 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK(NULL, "gpmi_io_clk", gpmi_io_clk),
 	_REGISTER_CLOCK(NULL, "usboh3_clk", usboh3_clk),
 	_REGISTER_CLOCK(NULL, "sata_clk", sata_clk),
+	_REGISTER_CLOCK(NULL, "cko1_clk", cko1_clk),
 };
 
 int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
@@ -2020,6 +2090,8 @@ int __init mx6q_clocks_init(void)
 	clk_set_rate(&usdhc3_clk, 49500000);
 	clk_set_rate(&usdhc4_clk, 49500000);
 
+	clk_set_parent(&cko1_clk, &ahb_clk);
+
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpt");
 	base = of_iomap(np, 0);
 	WARN_ON(!base);
-- 
1.7.5.4

[PATCH 3/6] arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 08d920d..3f4b45e 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -46,4 +46,24 @@
 			};
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+
+		reg_2P5V: regulator-2P5V {
+			compatible = "regulator-fixed";
+			regulator-name = "2P5V";
+			regulator-min-microvolt = <2500000>;
+			regulator-max-microvolt = <2500000>;
+			regulator-always-on;
+		};
+
+		reg_3P3V: regulator-3P3V {
+			compatible = "regulator-fixed";
+			regulator-name = "3P3V";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-always-on;
+		};
+	};
 };
-- 
1.7.5.4

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 3f4b45e..567b664 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -44,6 +44,18 @@
 			uart2: uart at 021e8000 {
 				status = "okay";
 			};
+
+			i2c at 021a0000 { /* I2C1 */
+				status = "okay";
+				clock-frequency = <100000>;
+
+				codec: sgtl5000 at 0a {
+					compatible = "fsl,sgtl5000";
+					reg = <0x0a>;
+					VDDA-supply = <&reg_2P5V>;
+					VDDIO-supply = <&reg_3P3V>;
+				};
+			};
 		};
 	};
 
-- 
1.7.5.4

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Richard Zhao]

If CONFIG_OF, we create a audmux device driver.

For other platforms that don't set CONFIG_OF, it still initialize
everything in postcore_initcall. I didn't create device driver for
them, because it would be big change and one day the platforms will
convert to device tree too.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/plat-mxc/audmux-v2.c |   59 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-mxc/audmux-v2.c b/arch/arm/plat-mxc/audmux-v2.c
index 8cced35..87e05e2 100644
--- a/arch/arm/plat-mxc/audmux-v2.c
+++ b/arch/arm/plat-mxc/audmux-v2.c
@@ -20,7 +20,9 @@
 #include <linux/io.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <mach/audmux.h>
 #include <mach/hardware.h>
 
@@ -184,8 +186,64 @@ int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr,
 }
 EXPORT_SYMBOL_GPL(mxc_audmux_v2_configure_port);
 
+#ifdef CONFIG_OF
+
+static int audmux_v2_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	int ret = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+	if (!request_mem_region(res->start, resource_size(res), "audmux_v2")) {
+		dev_err(&pdev->dev, "request_mem_region failed\n");
+		return -EBUSY;
+	}
+
+	audmux_base = ioremap(res->start, resource_size(res));
+	if (!audmux_base) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		ret = -ENODEV;
+		goto failed_ioremap;
+	}
+
+	audmux_clk = clk_get(NULL, "audmux");
+	if (IS_ERR(audmux_clk)) {
+			dev_warn(&pdev->dev, "%s: cannot get clock: %d\n",
+				__func__, ret);
+			audmux_clk = NULL;
+	}
+
+	audmux_debugfs_init();
+	return 0;
+
+failed_ioremap:
+	release_mem_region(res->start, resource_size(res));
+
+	return ret;
+}
+
+static const struct of_device_id audmux_v2_dt_ids[] = {
+	{ .compatible = "fsl,audmux-v2", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver audmux_v2_driver = {
+	.probe = audmux_v2_probe,
+	.driver = {
+		.name = "audmux_v2",
+		.of_match_table = audmux_v2_dt_ids,
+	},
+};
+
+#endif
+
 static int mxc_audmux_v2_init(void)
 {
+#ifdef CONFIG_OF
+	return platform_driver_register(&audmux_v2_driver);
+#else
 	int ret;
 	if (cpu_is_mx51()) {
 		audmux_base = MX51_IO_ADDRESS(MX51_AUDMUX_BASE_ADDR);
@@ -214,6 +272,7 @@ static int mxc_audmux_v2_init(void)
 	audmux_debugfs_init();
 
 	return 0;
+#endif
 }
 
 postcore_initcall(mxc_audmux_v2_init);
-- 
1.7.5.4

[PATCH 6/6] ARM: imx6q-sabrelite: add audmux support

[Richard Zhao]

- enable audmux in dts
- enable audmux in Kconfig

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |    4 ++++
 arch/arm/boot/dts/imx6q.dtsi          |    2 ++
 arch/arm/mach-imx/Kconfig             |    1 +
 3 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 567b664..cae7151 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -41,6 +41,10 @@
 				status = "okay";
 			};
 
+			audmux at 021d8000 {
+				status = "okay";
+			};
+
 			uart2: uart at 021e8000 {
 				status = "okay";
 			};
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..12ed20c 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -527,7 +527,9 @@
 			};
 
 			audmux at 021d8000 {
+				compatible = "fsl,audmux-v2";
 				reg = <0x021d8000 0x4000>;
+				status = "disabled";
 			};
 
 			mipi at 021dc000 { /* MIPI-CSI */
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 6538972..67f0fe7 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -836,6 +836,7 @@ comment "i.MX6 family:"
 config SOC_IMX6Q
 	bool "i.MX6 Quad support"
 	select ARM_CPU_SUSPEND if PM
+	select ARCH_MXC_AUDMUX_V2
 	select ARM_GIC
 	select CACHE_L2X0
 	select CPU_V7
-- 
1.7.5.4

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Russell King - ARM Linux]

On Fri, Jan 06, 2012 at 11:25:42AM +0800, Richard Zhao wrote:
> +#ifdef CONFIG_OF
> +
> +static int audmux_v2_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	int ret = 0;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	if (!request_mem_region(res->start, resource_size(res), "audmux_v2")) {
> +		dev_err(&pdev->dev, "request_mem_region failed\n");
> +		return -EBUSY;
> +	}
> +
> +	audmux_base = ioremap(res->start, resource_size(res));
> +	if (!audmux_base) {
> +		dev_err(&pdev->dev, "ioremap failed\n");
> +		ret = -ENODEV;
> +		goto failed_ioremap;
> +	}
> +
> +	audmux_clk = clk_get(NULL, "audmux");

NAK.  You have a struct device.  Use it.

[PATCH 1/6] ARM: mxc: make imx_dma_is_general_purpose more generic for sdma

[Sascha Hauer]

On Fri, Jan 06, 2012 at 11:25:38AM +0800, Richard Zhao wrote:
> sdma device names vary for different SoC. So we just check
> whether it includes "sdma" substring.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>

Acked-by: Sascha Hauer <s.hauer@pengutronix.de>


> ---
>  arch/arm/plat-mxc/include/mach/dma.h |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/include/mach/dma.h b/arch/arm/plat-mxc/include/mach/dma.h
> index 233d0a5..1b90803 100644
> --- a/arch/arm/plat-mxc/include/mach/dma.h
> +++ b/arch/arm/plat-mxc/include/mach/dma.h
> @@ -60,8 +60,7 @@ static inline int imx_dma_is_ipu(struct dma_chan *chan)
>  
>  static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
>  {
> -	return !strcmp(dev_name(chan->device->dev), "imx31-sdma") ||
> -		!strcmp(dev_name(chan->device->dev), "imx35-sdma") ||
> +	return strstr(dev_name(chan->device->dev), "sdma") ||
>  		!strcmp(dev_name(chan->device->dev), "imx-dma");
>  }
>  
> -- 
> 1.7.5.4
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Sascha Hauer]

On Fri, Jan 06, 2012 at 11:25:42AM +0800, Richard Zhao wrote:
> If CONFIG_OF, we create a audmux device driver.
> 
> For other platforms that don't set CONFIG_OF, it still initialize
> everything in postcore_initcall. I didn't create device driver for
> them, because it would be big change and one day the platforms will
> convert to device tree too.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/plat-mxc/audmux-v2.c |   59 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 59 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/audmux-v2.c b/arch/arm/plat-mxc/audmux-v2.c
> index 8cced35..87e05e2 100644
> --- a/arch/arm/plat-mxc/audmux-v2.c
> +++ b/arch/arm/plat-mxc/audmux-v2.c
> @@ -20,7 +20,9 @@
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  #include <mach/audmux.h>
>  #include <mach/hardware.h>
>  
> @@ -184,8 +186,64 @@ int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr,
>  }
>  EXPORT_SYMBOL_GPL(mxc_audmux_v2_configure_port);
>  
> +#ifdef CONFIG_OF
> +
> +static int audmux_v2_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	int ret = 0;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	if (!request_mem_region(res->start, resource_size(res), "audmux_v2")) {
> +		dev_err(&pdev->dev, "request_mem_region failed\n");
> +		return -EBUSY;
> +	}
> +
> +	audmux_base = ioremap(res->start, resource_size(res));
> +	if (!audmux_base) {
> +		dev_err(&pdev->dev, "ioremap failed\n");
> +		ret = -ENODEV;
> +		goto failed_ioremap;
> +	}
> +
> +	audmux_clk = clk_get(NULL, "audmux");
> +	if (IS_ERR(audmux_clk)) {
> +			dev_warn(&pdev->dev, "%s: cannot get clock: %d\n",
> +				__func__, ret);
> +			audmux_clk = NULL;
> +	}
> +
> +	audmux_debugfs_init();
> +	return 0;
> +
> +failed_ioremap:
> +	release_mem_region(res->start, resource_size(res));
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id audmux_v2_dt_ids[] = {
> +	{ .compatible = "fsl,audmux-v2", },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver audmux_v2_driver = {
> +	.probe = audmux_v2_probe,
> +	.driver = {
> +		.name = "audmux_v2",
> +		.of_match_table = audmux_v2_dt_ids,
> +	},
> +};
> +
> +#endif
> +
>  static int mxc_audmux_v2_init(void)
>  {
> +#ifdef CONFIG_OF
> +	return platform_driver_register(&audmux_v2_driver);
> +#else

Don't do this. Just because CONFIG_OF is enabled does not mean that this
kernel is only used for OF. This #else breaks starting the kernel the
traditional machid way.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Richard Zhao]

On Fri, Jan 06, 2012 at 08:56:17AM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 06, 2012 at 11:25:42AM +0800, Richard Zhao wrote:
> > +#ifdef CONFIG_OF
> > +
> > +static int audmux_v2_probe(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +	int ret = 0;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENODEV;
> > +	if (!request_mem_region(res->start, resource_size(res), "audmux_v2")) {
> > +		dev_err(&pdev->dev, "request_mem_region failed\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	audmux_base = ioremap(res->start, resource_size(res));
> > +	if (!audmux_base) {
> > +		dev_err(&pdev->dev, "ioremap failed\n");
> > +		ret = -ENODEV;
> > +		goto failed_ioremap;
> > +	}
> > +
> > +	audmux_clk = clk_get(NULL, "audmux");
> 
> NAK.  You have a struct device.  Use it.
Right, Thanks.

Richard
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Richard Zhao]

On Fri, Jan 06, 2012 at 10:13:15AM +0100, Sascha Hauer wrote:
> On Fri, Jan 06, 2012 at 11:25:42AM +0800, Richard Zhao wrote:
> > If CONFIG_OF, we create a audmux device driver.
> > 
> > For other platforms that don't set CONFIG_OF, it still initialize
> > everything in postcore_initcall. I didn't create device driver for
> > them, because it would be big change and one day the platforms will
> > convert to device tree too.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  arch/arm/plat-mxc/audmux-v2.c |   59 +++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 59 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/plat-mxc/audmux-v2.c b/arch/arm/plat-mxc/audmux-v2.c
> > index 8cced35..87e05e2 100644
> > --- a/arch/arm/plat-mxc/audmux-v2.c
> > +++ b/arch/arm/plat-mxc/audmux-v2.c
> > @@ -20,7 +20,9 @@
> >  #include <linux/io.h>
> >  #include <linux/clk.h>
> >  #include <linux/debugfs.h>
> > +#include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > +#include <linux/of.h>
> >  #include <mach/audmux.h>
> >  #include <mach/hardware.h>
> >  
> > @@ -184,8 +186,64 @@ int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr,
> >  }
> >  EXPORT_SYMBOL_GPL(mxc_audmux_v2_configure_port);
> >  
> > +#ifdef CONFIG_OF
> > +
> > +static int audmux_v2_probe(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +	int ret = 0;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENODEV;
> > +	if (!request_mem_region(res->start, resource_size(res), "audmux_v2")) {
> > +		dev_err(&pdev->dev, "request_mem_region failed\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	audmux_base = ioremap(res->start, resource_size(res));
> > +	if (!audmux_base) {
> > +		dev_err(&pdev->dev, "ioremap failed\n");
> > +		ret = -ENODEV;
> > +		goto failed_ioremap;
> > +	}
> > +
> > +	audmux_clk = clk_get(NULL, "audmux");
> > +	if (IS_ERR(audmux_clk)) {
> > +			dev_warn(&pdev->dev, "%s: cannot get clock: %d\n",
> > +				__func__, ret);
> > +			audmux_clk = NULL;
> > +	}
> > +
> > +	audmux_debugfs_init();
> > +	return 0;
> > +
> > +failed_ioremap:
> > +	release_mem_region(res->start, resource_size(res));
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id audmux_v2_dt_ids[] = {
> > +	{ .compatible = "fsl,audmux-v2", },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver audmux_v2_driver = {
> > +	.probe = audmux_v2_probe,
> > +	.driver = {
> > +		.name = "audmux_v2",
> > +		.of_match_table = audmux_v2_dt_ids,
> > +	},
> > +};
> > +
> > +#endif
> > +
> >  static int mxc_audmux_v2_init(void)
> >  {
> > +#ifdef CONFIG_OF
> > +	return platform_driver_register(&audmux_v2_driver);
> > +#else
> 
> Don't do this. Just because CONFIG_OF is enabled does not mean that this
> kernel is only used for OF. This #else breaks starting the kernel the
> traditional machid way.
You are right. Look like I have to convert audmux to platform devices
for all platforms.

Thanks
Richard
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Sascha Hauer]

On Fri, Jan 06, 2012 at 05:21:24PM +0800, Richard Zhao wrote:
> On Fri, Jan 06, 2012 at 10:13:15AM +0100, Sascha Hauer wrote:
> > On Fri, Jan 06, 2012 at 11:25:42AM +0800, Richard Zhao wrote:
> > > If CONFIG_OF, we create a audmux device driver.
> > > 
> > > For other platforms that don't set CONFIG_OF, it still initialize
> > > everything in postcore_initcall. I didn't create device driver for
> > > them, because it would be big change and one day the platforms will
> > > convert to device tree too.
> > > 
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  arch/arm/plat-mxc/audmux-v2.c |   59 +++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 59 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/arm/plat-mxc/audmux-v2.c b/arch/arm/plat-mxc/audmux-v2.c
> > > index 8cced35..87e05e2 100644
> > > --- a/arch/arm/plat-mxc/audmux-v2.c
> > > +++ b/arch/arm/plat-mxc/audmux-v2.c
> > > @@ -20,7 +20,9 @@
> > >  #include <linux/io.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/debugfs.h>
> > > +#include <linux/platform_device.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/of.h>
> > >  #include <mach/audmux.h>
> > >  #include <mach/hardware.h>
> > >  
> > > @@ -184,8 +186,64 @@ int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr,
> > >  }
> > >  EXPORT_SYMBOL_GPL(mxc_audmux_v2_configure_port);
> > >  
> > > +#ifdef CONFIG_OF
> > > +
> > > +static int audmux_v2_probe(struct platform_device *pdev)
> > > +{
> > > +	struct resource *res;
> > > +	int ret = 0;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res)
> > > +		return -ENODEV;
> > > +	if (!request_mem_region(res->start, resource_size(res), "audmux_v2")) {
> > > +		dev_err(&pdev->dev, "request_mem_region failed\n");
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	audmux_base = ioremap(res->start, resource_size(res));
> > > +	if (!audmux_base) {
> > > +		dev_err(&pdev->dev, "ioremap failed\n");
> > > +		ret = -ENODEV;
> > > +		goto failed_ioremap;
> > > +	}
> > > +
> > > +	audmux_clk = clk_get(NULL, "audmux");
> > > +	if (IS_ERR(audmux_clk)) {
> > > +			dev_warn(&pdev->dev, "%s: cannot get clock: %d\n",
> > > +				__func__, ret);
> > > +			audmux_clk = NULL;
> > > +	}
> > > +
> > > +	audmux_debugfs_init();
> > > +	return 0;
> > > +
> > > +failed_ioremap:
> > > +	release_mem_region(res->start, resource_size(res));
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct of_device_id audmux_v2_dt_ids[] = {
> > > +	{ .compatible = "fsl,audmux-v2", },
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > > +static struct platform_driver audmux_v2_driver = {
> > > +	.probe = audmux_v2_probe,
> > > +	.driver = {
> > > +		.name = "audmux_v2",
> > > +		.of_match_table = audmux_v2_dt_ids,
> > > +	},
> > > +};
> > > +
> > > +#endif
> > > +
> > >  static int mxc_audmux_v2_init(void)
> > >  {
> > > +#ifdef CONFIG_OF
> > > +	return platform_driver_register(&audmux_v2_driver);
> > > +#else
> > 
> > Don't do this. Just because CONFIG_OF is enabled does not mean that this
> > kernel is only used for OF. This #else breaks starting the kernel the
> > traditional machid way.
> You are right. Look like I have to convert audmux to platform devices
> for all platforms.

I hoped you would say this :)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/6] arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators

[Shawn Guo]

For subject, I would suggest something like below to keep consistency
with code patches.

ARM: dts: imx6q-sabrelite: ...

On Fri, Jan 06, 2012 at 11:25:40AM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/boot/dts/imx6q-sabrelite.dts |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 08d920d..3f4b45e 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -46,4 +46,24 @@
>  			};
>  		};
>  	};
> +
> +	regulators {

Shouldn't this node be under node 'anatop at 020c8000'?

> +		compatible = "simple-bus";
> +
Hmm, do we really need this?

> +		reg_2P5V: regulator-2P5V {

There is convention that we should try to have all kinds of names in dts
as lower case, even though hardware document generally names blocks in
capital letters.  It just looks odd to have mixed cases in the name.

Since the node is under node 'regulators', we may name the node just
as simple as '2p5v'.

> +			compatible = "regulator-fixed";
> +			regulator-name = "2P5V";

It's fine to use capital letters for such case.

Regards,
Shawn

> +			regulator-min-microvolt = <2500000>;
> +			regulator-max-microvolt = <2500000>;
> +			regulator-always-on;
> +		};
> +
> +		reg_3P3V: regulator-3P3V {
> +			compatible = "regulator-fixed";
> +			regulator-name = "3P3V";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +			regulator-always-on;
> +		};
> +	};
>  };
> -- 
> 1.7.5.4

[PATCH 3/6] arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators

[Richard Zhao]

On Sun, Jan 08, 2012 at 05:06:21PM +0800, Shawn Guo wrote:
> For subject, I would suggest something like below to keep consistency
> with code patches.
> 
> ARM: dts: imx6q-sabrelite: ...
ok
> 
> On Fri, Jan 06, 2012 at 11:25:40AM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  arch/arm/boot/dts/imx6q-sabrelite.dts |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > index 08d920d..3f4b45e 100644
> > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > @@ -46,4 +46,24 @@
> >  			};
> >  		};
> >  	};
> > +
> > +	regulators {
> 
> Shouldn't this node be under node 'anatop at 020c8000'?
It's LDOs on board. Why do I put it in anatop?
> 
> > +		compatible = "simple-bus";
> > +
> Hmm, do we really need this?
If I don't set it, the regulator devices will not be populated.
> 
> > +		reg_2P5V: regulator-2P5V {
> 
> There is convention that we should try to have all kinds of names in dts
> as lower case, even though hardware document generally names blocks in
> capital letters.  It just looks odd to have mixed cases in the name.
The convention looks weird. The dts is supposed to reflect hw as much
as possible. I'll change it if you insist.
> 
> Since the node is under node 'regulators', we may name the node just
> as simple as '2p5v'.
I was just not sure whether the name can start with digit. If yes, I'm
glad to make it short.
> 
> > +			compatible = "regulator-fixed";
> > +			regulator-name = "2P5V";
> 
> It's fine to use capital letters for such case.
ok

Thanks
Richard
> 
> Regards,
> Shawn
> 
> > +			regulator-min-microvolt = <2500000>;
> > +			regulator-max-microvolt = <2500000>;
> > +			regulator-always-on;
> > +		};
> > +
> > +		reg_3P3V: regulator-3P3V {
> > +			compatible = "regulator-fixed";
> > +			regulator-name = "3P3V";
> > +			regulator-min-microvolt = <3300000>;
> > +			regulator-max-microvolt = <3300000>;
> > +			regulator-always-on;
> > +		};
> > +	};
> >  };
> > -- 
> > 1.7.5.4

[PATCH 3/6] arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators

[Shawn Guo]

On Sun, Jan 08, 2012 at 05:14:05PM +0800, Richard Zhao wrote:
> On Sun, Jan 08, 2012 at 05:06:21PM +0800, Shawn Guo wrote:
> > For subject, I would suggest something like below to keep consistency
> > with code patches.
> > 
> > ARM: dts: imx6q-sabrelite: ...
> ok
> > 
> > On Fri, Jan 06, 2012 at 11:25:40AM +0800, Richard Zhao wrote:
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  arch/arm/boot/dts/imx6q-sabrelite.dts |   20 ++++++++++++++++++++
> > >  1 files changed, 20 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > > index 08d920d..3f4b45e 100644
> > > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > > @@ -46,4 +46,24 @@
> > >  			};
> > >  		};
> > >  	};
> > > +
> > > +	regulators {
> > 
> > Shouldn't this node be under node 'anatop at 020c8000'?
> It's LDOs on board. Why do I put it in anatop?

Sorry, I messed up here.

> > 
> > > +		compatible = "simple-bus";
> > > +
> > Hmm, do we really need this?
> If I don't set it, the regulator devices will not be populated.

ditto

> > 
> > > +		reg_2P5V: regulator-2P5V {
> > 
> > There is convention that we should try to have all kinds of names in dts
> > as lower case, even though hardware document generally names blocks in
> > capital letters.  It just looks odd to have mixed cases in the name.
> The convention looks weird. The dts is supposed to reflect hw as much
> as possible. I'll change it if you insist.

Yes, please.

> > 
> > Since the node is under node 'regulators', we may name the node just
> > as simple as '2p5v'.
> I was just not sure whether the name can start with digit. If yes, I'm
> glad to make it short.

I just gave a quick try, and it works.

Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Shawn Guo]

On Fri, Jan 06, 2012 at 11:25:41AM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/boot/dts/imx6q-sabrelite.dts |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 3f4b45e..567b664 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -44,6 +44,18 @@
>  			uart2: uart at 021e8000 {
>  				status = "okay";
>  			};
> +
> +			i2c at 021a0000 { /* I2C1 */
> +				status = "okay";
> +				clock-frequency = <100000>;
> +
> +				codec: sgtl5000 at 0a {
> +					compatible = "fsl,sgtl5000";
> +					reg = <0x0a>;
> +					VDDA-supply = <&reg_2P5V>;
> +					VDDIO-supply = <&reg_3P3V>;

I would prefer to have them named vdda-supply and vddio-supply.  But
I just learnt that they do not work, because sgtl5000 driver
(sound/soc/codecs/sgtl5000.c) has the supply_names in upper case, while
unlike of_node_cmp() is strcasecmp(), of_prop_cmp() is just strcmp().

But the convention on property name is really all using lower case,
and mixing cases there looks odd, so I'm thinking about the changes
below on of_get_regulator().

Mark, how do you think?

8<---
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ca86f39..b89eb43 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -17,6 +17,7 @@

 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/ctype.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/slab.h>
@@ -147,10 +148,15 @@ static struct device_node *of_get_regulator(struct device *dev, const char *supp
 {
        struct device_node *regnode = NULL;
        char prop_name[32]; /* 32 is max size of property name */
+       int i = 0;

        dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);

        snprintf(prop_name, 32, "%s-supply", supply);
+       while (prop_name[i] && i < 32) {
+               prop_name[i] = tolower(prop_name[i]);
+               i++;
+       }
        regnode = of_parse_phandle(dev->of_node, prop_name, 0);

        if (!regnode) {
--->8
 
Regards,
Shawn

> +				};
> +			};
>  		};
>  	};
>  
> -- 
> 1.7.5.4
> 
> 

[PATCH 6/6] ARM: imx6q-sabrelite: add audmux support

[Richard Zhao]

On Sun, Jan 08, 2012 at 11:02:36PM +0800, Shawn Guo wrote:
> On Fri, Jan 06, 2012 at 11:25:43AM +0800, Richard Zhao wrote:
> > --- a/arch/arm/boot/dts/imx6q.dtsi
> > +++ b/arch/arm/boot/dts/imx6q.dtsi
> > @@ -527,7 +527,9 @@
> >  			};
> >  
> >  			audmux at 021d8000 {
> > +				compatible = "fsl,audmux-v2";
> 
> This is not a good compatible string, because the version number is
> not defined by hardware.  Even it is defined by hardware, the version
> number is not as stable as soc to identify specific hardware block.
> You can look at uart, cspi, sdma etc. for examples.
compatible = "fsl,imx6q-audmux", "fsl,imx25-audmux";
I forgot imx25 or imx31 is oldest. Is it ok for you?

Thanks
Richard
> 
> >  				reg = <0x021d8000 0x4000>;
> > +				status = "disabled";
> >  			};
> 
> -- 
> Regards,
> Shawn
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 6/6] ARM: imx6q-sabrelite: add audmux support

[Shawn Guo]

On Fri, Jan 06, 2012 at 11:25:43AM +0800, Richard Zhao wrote:
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -527,7 +527,9 @@
>  			};
>  
>  			audmux at 021d8000 {
> +				compatible = "fsl,audmux-v2";

This is not a good compatible string, because the version number is
not defined by hardware.  Even it is defined by hardware, the version
number is not as stable as soc to identify specific hardware block.
You can look at uart, cspi, sdma etc. for examples.

>  				reg = <0x021d8000 0x4000>;
> +				status = "disabled";
>  			};

-- 
Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Mark Brown]

On Sun, Jan 08, 2012 at 10:52:56PM +0800, Shawn Guo wrote:
> On Fri, Jan 06, 2012 at 11:25:41AM +0800, Richard Zhao wrote:

> > +					VDDA-supply = <&reg_2P5V>;
> > +					VDDIO-supply = <&reg_3P3V>;

> I would prefer to have them named vdda-supply and vddio-supply.  But
> I just learnt that they do not work, because sgtl5000 driver
> (sound/soc/codecs/sgtl5000.c) has the supply_names in upper case, while
> unlike of_node_cmp() is strcasecmp(), of_prop_cmp() is just strcmp().

> But the convention on property name is really all using lower case,
> and mixing cases there looks odd, so I'm thinking about the changes
> below on of_get_regulator().

>         snprintf(prop_name, 32, "%s-supply", supply);
> +       while (prop_name[i] && i < 32) {
> +               prop_name[i] = tolower(prop_name[i]);
> +               i++;
> +       }

There's two big problems here.  One is that we clearly shouldn't be
open coding this here but adding a function for it.  The other is that
this is going to break any existing device tree which has upper cased
supplies.  If we were going to do something here I'd go with case
insensitve matching though I'm not sure it's a real problem.

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Shawn Guo]

On Sun, Jan 08, 2012 at 12:55:05PM -0800, Mark Brown wrote:
> On Sun, Jan 08, 2012 at 10:52:56PM +0800, Shawn Guo wrote:
> > On Fri, Jan 06, 2012 at 11:25:41AM +0800, Richard Zhao wrote:
> 
> > > +					VDDA-supply = <&reg_2P5V>;
> > > +					VDDIO-supply = <&reg_3P3V>;
> 
> > I would prefer to have them named vdda-supply and vddio-supply.  But
> > I just learnt that they do not work, because sgtl5000 driver
> > (sound/soc/codecs/sgtl5000.c) has the supply_names in upper case, while
> > unlike of_node_cmp() is strcasecmp(), of_prop_cmp() is just strcmp().
> 
> > But the convention on property name is really all using lower case,
> > and mixing cases there looks odd, so I'm thinking about the changes
> > below on of_get_regulator().
> 
> >         snprintf(prop_name, 32, "%s-supply", supply);
> > +       while (prop_name[i] && i < 32) {
> > +               prop_name[i] = tolower(prop_name[i]);
> > +               i++;
> > +       }
> 
> There's two big problems here.  One is that we clearly shouldn't be
> open coding this here but adding a function for it.  The other is that
> this is going to break any existing device tree which has upper cased
> supplies.  If we were going to do something here I'd go with case
> insensitve matching though I'm not sure it's a real problem.

Ok, let's test device tree maintainers.

Grant, Rob,

Could the problem we are seeing here be a good reason to make the
following change?

diff --git a/include/linux/of.h b/include/linux/of.h
index a75a831..c26c20f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -147,7 +147,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 /* Default string compare functions, Allow arch asm/prom.h to override */
 #if !defined(of_compat_cmp)
 #define of_compat_cmp(s1, s2, l)       strcasecmp((s1), (s2))
-#define of_prop_cmp(s1, s2)            strcmp((s1), (s2))
+#define of_prop_cmp(s1, s2)            strcasecmp((s1), (s2))
 #define of_node_cmp(s1, s2)            strcasecmp((s1), (s2))
 #endif

-- 
Regards,
Shawn

[PATCH 6/6] ARM: imx6q-sabrelite: add audmux support

[Shawn Guo]

On Sun, Jan 08, 2012 at 10:58:14PM +0800, Richard Zhao wrote:
> On Sun, Jan 08, 2012 at 11:02:36PM +0800, Shawn Guo wrote:
> > On Fri, Jan 06, 2012 at 11:25:43AM +0800, Richard Zhao wrote:
> > > --- a/arch/arm/boot/dts/imx6q.dtsi
> > > +++ b/arch/arm/boot/dts/imx6q.dtsi
> > > @@ -527,7 +527,9 @@
> > >  			};
> > >  
> > >  			audmux at 021d8000 {
> > > +				compatible = "fsl,audmux-v2";
> > 
> > This is not a good compatible string, because the version number is
> > not defined by hardware.  Even it is defined by hardware, the version
> > number is not as stable as soc to identify specific hardware block.
> > You can look at uart, cspi, sdma etc. for examples.
> compatible = "fsl,imx6q-audmux", "fsl,imx25-audmux";
> I forgot imx25 or imx31 is oldest. Is it ok for you?
> 
Yes.

-- 
Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Rob Herring]

On 01/08/2012 06:56 PM, Shawn Guo wrote:
> On Sun, Jan 08, 2012 at 12:55:05PM -0800, Mark Brown wrote:
>> On Sun, Jan 08, 2012 at 10:52:56PM +0800, Shawn Guo wrote:
>>> On Fri, Jan 06, 2012 at 11:25:41AM +0800, Richard Zhao wrote:
>>
>>>> +					VDDA-supply = <&reg_2P5V>;
>>>> +					VDDIO-supply = <&reg_3P3V>;
>>
>>> I would prefer to have them named vdda-supply and vddio-supply.  But
>>> I just learnt that they do not work, because sgtl5000 driver
>>> (sound/soc/codecs/sgtl5000.c) has the supply_names in upper case, while
>>> unlike of_node_cmp() is strcasecmp(), of_prop_cmp() is just strcmp().
>>
>>> But the convention on property name is really all using lower case,
>>> and mixing cases there looks odd, so I'm thinking about the changes
>>> below on of_get_regulator().
>>
>>>         snprintf(prop_name, 32, "%s-supply", supply);
>>> +       while (prop_name[i] && i < 32) {
>>> +               prop_name[i] = tolower(prop_name[i]);
>>> +               i++;
>>> +       }
>>
>> There's two big problems here.  One is that we clearly shouldn't be
>> open coding this here but adding a function for it.  The other is that
>> this is going to break any existing device tree which has upper cased
>> supplies.  If we were going to do something here I'd go with case
>> insensitve matching though I'm not sure it's a real problem.
> 
> Ok, let's test device tree maintainers.
> 
> Grant, Rob,
> 
> Could the problem we are seeing here be a good reason to make the
> following change?
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index a75a831..c26c20f 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -147,7 +147,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>  /* Default string compare functions, Allow arch asm/prom.h to override */
>  #if !defined(of_compat_cmp)
>  #define of_compat_cmp(s1, s2, l)       strcasecmp((s1), (s2))
> -#define of_prop_cmp(s1, s2)            strcmp((s1), (s2))
> +#define of_prop_cmp(s1, s2)            strcasecmp((s1), (s2))
>  #define of_node_cmp(s1, s2)            strcasecmp((s1), (s2))
>  #endif

Device-trees are case sensitive, so I don't think we want to go globally
changing that behavior.

If you want lower case names, then change the sgtl5000 code to lower
case names.

Rob

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Eric Miao]

>> ?/* Default string compare functions, Allow arch asm/prom.h to override */
>> ?#if !defined(of_compat_cmp)
>> ?#define of_compat_cmp(s1, s2, l) ? ? ? strcasecmp((s1), (s2))
>> -#define of_prop_cmp(s1, s2) ? ? ? ? ? ?strcmp((s1), (s2))
>> +#define of_prop_cmp(s1, s2) ? ? ? ? ? ?strcasecmp((s1), (s2))
>> ?#define of_node_cmp(s1, s2) ? ? ? ? ? ?strcasecmp((s1), (s2))
>> ?#endif
>
> Device-trees are case sensitive, so I don't think we want to go globally
> changing that behavior.
>
> If you want lower case names, then change the sgtl5000 code to lower
> case names.

+1, I'd vote for consistent lower case names.

[PATCH 6/6] ARM: imx6q-sabrelite: add audmux support

[Shawn Guo]

On Mon, Jan 09, 2012 at 09:19:50AM +0800, Shawn Guo wrote:
> On Sun, Jan 08, 2012 at 10:58:14PM +0800, Richard Zhao wrote:
> > On Sun, Jan 08, 2012 at 11:02:36PM +0800, Shawn Guo wrote:
> > > On Fri, Jan 06, 2012 at 11:25:43AM +0800, Richard Zhao wrote:
> > > > --- a/arch/arm/boot/dts/imx6q.dtsi
> > > > +++ b/arch/arm/boot/dts/imx6q.dtsi
> > > > @@ -527,7 +527,9 @@
> > > >  			};
> > > >  
> > > >  			audmux at 021d8000 {
> > > > +				compatible = "fsl,audmux-v2";
> > > 
> > > This is not a good compatible string, because the version number is
> > > not defined by hardware.  Even it is defined by hardware, the version
> > > number is not as stable as soc to identify specific hardware block.
> > > You can look at uart, cspi, sdma etc. for examples.
> > compatible = "fsl,imx6q-audmux", "fsl,imx25-audmux";
> > I forgot imx25 or imx31 is oldest. Is it ok for you?
> > 
> Yes.

Forgot mentioning that imx31 was born earlier than imx25, so I would
expect it be: 

	compatible = "fsl,imx6q-audmux", "fsl,imx31-audmux";

-- 
Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Richard Zhao]

On Mon, Jan 09, 2012 at 01:05:39PM +0800, Eric Miao wrote:
> >> ?/* Default string compare functions, Allow arch asm/prom.h to override */
> >> ?#if !defined(of_compat_cmp)
> >> ?#define of_compat_cmp(s1, s2, l) ? ? ? strcasecmp((s1), (s2))
> >> -#define of_prop_cmp(s1, s2) ? ? ? ? ? ?strcmp((s1), (s2))
> >> +#define of_prop_cmp(s1, s2) ? ? ? ? ? ?strcasecmp((s1), (s2))
> >> ?#define of_node_cmp(s1, s2) ? ? ? ? ? ?strcasecmp((s1), (s2))
> >> ?#endif
> >
> > Device-trees are case sensitive, so I don't think we want to go globally
> > changing that behavior.
> >
> > If you want lower case names, then change the sgtl5000 code to lower
> > case names.
> 
> +1, I'd vote for consistent lower case names.
sgtl5000 is used by many other platforms too. I doubt why we do the
big change just because it's convention or looks nice.
As I said in other mails, I don't feel good about the lower case
convention:
 - hw spec or sch may use upper case, lower case don't reflect exact.
 - the lower case rule might have to extend to other subsystems, for
   example, regulator. I bet it's not the only case when we get more
   and more DT bindings.

Thanks
Richard
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Mark Brown]

On Mon, Jan 09, 2012 at 01:05:39PM +0800, Eric Miao wrote:

> > If you want lower case names, then change the sgtl5000 code to lower
> > case names.

> +1, I'd vote for consistent lower case names.

That would be non-idiomatic for the regulators - the idiom for the
regulator API is to use upper case names as that's the idiom used to
label supplies in datasheets and schematics.

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Mark Brown]

On Mon, Jan 09, 2012 at 02:52:40PM +0800, Shawn Guo wrote:
> On Sun, Jan 08, 2012 at 10:25:12PM -0800, Mark Brown wrote:

> > That would be non-idiomatic for the regulators - the idiom for the
> > regulator API is to use upper case names as that's the idiom used to
> > label supplies in datasheets and schematics.

> Hmm, the idiom seems not so strong.

> drivers/mmc/host/sdhci.c:

> 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");

It's purely an idiom, there's nothing enforcing it.

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Shawn Guo]

On Sun, Jan 08, 2012 at 09:38:39PM -0600, Rob Herring wrote:
> On 01/08/2012 06:56 PM, Shawn Guo wrote:
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index a75a831..c26c20f 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -147,7 +147,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
> >  /* Default string compare functions, Allow arch asm/prom.h to override */
> >  #if !defined(of_compat_cmp)
> >  #define of_compat_cmp(s1, s2, l)       strcasecmp((s1), (s2))
> > -#define of_prop_cmp(s1, s2)            strcmp((s1), (s2))
> > +#define of_prop_cmp(s1, s2)            strcasecmp((s1), (s2))
> >  #define of_node_cmp(s1, s2)            strcasecmp((s1), (s2))
> >  #endif
> 
> Device-trees are case sensitive,

I do not believe this is true, as of_compat_cmp() and of_node_cmp()
are clearly defined as strcasecmp() above.

> so I don't think we want to go globally
> changing that behavior.
> 
> If you want lower case names, then change the sgtl5000 code to lower
> case names.
> 
I do not have a strong opinion about which is the one we should change
among OF, regulator and sgtl5000.  But I think it's the convention
that we should keep property name in dts all lower case.  I'm actually
even fine with this odd mixing case name, if you tell that it's okay
to override the convention for some cases.

-- 
Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Shawn Guo]

On Sun, Jan 08, 2012 at 10:25:12PM -0800, Mark Brown wrote:
> On Mon, Jan 09, 2012 at 01:05:39PM +0800, Eric Miao wrote:
> 
> > > If you want lower case names, then change the sgtl5000 code to lower
> > > case names.
> 
> > +1, I'd vote for consistent lower case names.
> 
> That would be non-idiomatic for the regulators - the idiom for the
> regulator API is to use upper case names as that's the idiom used to
> label supplies in datasheets and schematics.

Hmm, the idiom seems not so strong.

drivers/mmc/host/sdhci.c:

	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");

-- 
Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Mark Brown]

On Mon, Jan 09, 2012 at 03:17:16PM +0800, Shawn Guo wrote:
> On Sun, Jan 08, 2012 at 10:43:29PM -0800, Mark Brown wrote:

> > It's purely an idiom, there's nothing enforcing it.

> Actually, a rough calculation on 'git grep regulator_get drivers/'
> output tells 80% callers are using lower case.

Add in the ASoC stuff (which I actually actively review) and I suspect
the number changes.  Anyway, there is a good solid reason for wanting
capitals in the code so...

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Shawn Guo]

On Sun, Jan 08, 2012 at 10:43:29PM -0800, Mark Brown wrote:
> On Mon, Jan 09, 2012 at 02:52:40PM +0800, Shawn Guo wrote:
> > On Sun, Jan 08, 2012 at 10:25:12PM -0800, Mark Brown wrote:
> 
> > > That would be non-idiomatic for the regulators - the idiom for the
> > > regulator API is to use upper case names as that's the idiom used to
> > > label supplies in datasheets and schematics.
> 
> > Hmm, the idiom seems not so strong.
> 
> > drivers/mmc/host/sdhci.c:
> 
> > 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
> 
> It's purely an idiom, there's nothing enforcing it.

Actually, a rough calculation on 'git grep regulator_get drivers/'
output tells 80% callers are using lower case.

-- 
Regards,
Shawn

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Richard Zhao]

On Sun, Jan 08, 2012 at 11:12:56PM -0800, Mark Brown wrote:
> On Mon, Jan 09, 2012 at 03:17:16PM +0800, Shawn Guo wrote:
> > On Sun, Jan 08, 2012 at 10:43:29PM -0800, Mark Brown wrote:
> 
> > > It's purely an idiom, there's nothing enforcing it.
> 
> > Actually, a rough calculation on 'git grep regulator_get drivers/'
> > output tells 80% callers are using lower case.
> 
> Add in the ASoC stuff (which I actually actively review) and I suspect
> the number changes.  Anyway, there is a good solid reason for wanting
> capitals in the code so...
Shawn and Mark,

Could you get agreement on it?
I incline to not forcing upper case property names.

Thanks
Richard
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Mark Brown]

On Wed, Jan 11, 2012 at 08:57:54AM +0800, Richard Zhao wrote:

> Could you get agreement on it?
> I incline to not forcing upper case property names.

Personally I see no prolbme with the status quo.

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Fabio Estevam]

Hi Richard,

On Fri, Jan 6, 2012 at 1:25 AM, Richard Zhao <richard.zhao@linaro.org> wrote:
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
> ?arch/arm/boot/dts/imx6q-sabrelite.dts | ? 12 ++++++++++++
> ?1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 3f4b45e..567b664 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -44,6 +44,18 @@
> ? ? ? ? ? ? ? ? ? ? ? ?uart2: uart at 021e8000 {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = "okay";
> ? ? ? ? ? ? ? ? ? ? ? ?};
> +
> + ? ? ? ? ? ? ? ? ? ? ? i2c at 021a0000 { /* I2C1 */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? status = "okay";
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clock-frequency = <100000>;
> +
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? codec: sgtl5000 at 0a {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compatible = "fsl,sgtl5000";
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg = <0x0a>;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDA-supply = <&reg_2P5V>;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDIO-supply = <&reg_3P3V>;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? };
> + ? ? ? ? ? ? ? ? ? ? ? };
> ? ? ? ? ? ? ? ?};
> ? ? ? ?};
>

I don't see a  sound/soc/imx/imx-sgtl5000.c file yet, so I am
wondering how did you test this?

Maybe I missed a patch. Could you please clarify?

Regards,

Fabio Estevam

[PATCH 4/6] arm/dts: imx6q-sabrelite: add sgtl5000 audio codec

[Richard Zhao]

On Tue, Jan 10, 2012 at 11:33:10PM -0200, Fabio Estevam wrote:
> Hi Richard,
> 
> On Fri, Jan 6, 2012 at 1:25 AM, Richard Zhao <richard.zhao@linaro.org> wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> > ?arch/arm/boot/dts/imx6q-sabrelite.dts | ? 12 ++++++++++++
> > ?1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > index 3f4b45e..567b664 100644
> > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > @@ -44,6 +44,18 @@
> > ? ? ? ? ? ? ? ? ? ? ? ?uart2: uart at 021e8000 {
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = "okay";
> > ? ? ? ? ? ? ? ? ? ? ? ?};
> > +
> > + ? ? ? ? ? ? ? ? ? ? ? i2c at 021a0000 { /* I2C1 */
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? status = "okay";
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clock-frequency = <100000>;
> > +
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? codec: sgtl5000 at 0a {
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compatible = "fsl,sgtl5000";
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg = <0x0a>;
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDA-supply = <&reg_2P5V>;
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDIO-supply = <&reg_3P3V>;
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? };
> > + ? ? ? ? ? ? ? ? ? ? ? };
> > ? ? ? ? ? ? ? ?};
> > ? ? ? ?};
> >
> 
> I don't see a  sound/soc/imx/imx-sgtl5000.c file yet, so I am
> wondering how did you test this?
> 
> Maybe I missed a patch. Could you please clarify?
I test it using linaro freescale landing team kernel. Actually
I'm upstreaming the audio work I did for linaro. Because audio
involves too much subsystem, I split it into several set of patch.
Finally, you'll see imx-sgtl5000.c.

Thanks
Richard
> 
> Regards,
> 
> Fabio Estevam
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Shawn Guo]

On Fri, Jan 06, 2012 at 05:21:24PM +0800, Richard Zhao wrote:
> You are right. Look like I have to convert audmux to platform devices
> for all platforms.
> 
With doing so, it may be also good for us to move this driver into
sound/soc/imx/.  Any particular reason to keep it in platform?

-- 
Regards,
Shawn

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Richard Zhao]

On Wed, Jan 11, 2012 at 01:26:06PM +0800, Shawn Guo wrote:
> On Fri, Jan 06, 2012 at 05:21:24PM +0800, Richard Zhao wrote:
> > You are right. Look like I have to convert audmux to platform devices
> > for all platforms.
> > 
> With doing so, it may be also good for us to move this driver into
> sound/soc/imx/.  Any particular reason to keep it in platform?
Yes, we can move it out. But I'm not sure we can put it in asoc,
because it's not a standard part of ASoC. Mark?

Thanks
Richard
> 
> -- 
> Regards,
> Shawn
> 

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Mark Brown]

On Wed, Jan 11, 2012 at 09:02:44PM +0800, Richard Zhao wrote:
> On Wed, Jan 11, 2012 at 01:26:06PM +0800, Shawn Guo wrote:

> > With doing so, it may be also good for us to move this driver into
> > sound/soc/imx/.  Any particular reason to keep it in platform?

> Yes, we can move it out. But I'm not sure we can put it in asoc,
> because it's not a standard part of ASoC. Mark?

It should be supported using standard ASoC mechanisms.

[PATCH 5/6] ARM: mxc: add dt support for audmux-v2

[Richard Zhao]

On Wed, Jan 11, 2012 at 05:38:27PM +0000, Mark Brown wrote:
> On Wed, Jan 11, 2012 at 09:02:44PM +0800, Richard Zhao wrote:
> > On Wed, Jan 11, 2012 at 01:26:06PM +0800, Shawn Guo wrote:
> 
> > > With doing so, it may be also good for us to move this driver into
> > > sound/soc/imx/.  Any particular reason to keep it in platform?
> 
> > Yes, we can move it out. But I'm not sure we can put it in asoc,
> > because it's not a standard part of ASoC. Mark?
> 
> It should be supported using standard ASoC mechanisms.
Anyway, it needs another patch. I'll not do it in this patch series.

Thanks
Richard