imx patches when I enable imx6q sabrelite audio
Subsystems (6)
cover imx patches when I enable imx6q sabrelite audio
[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
sdma device names vary for different SoC. So we just check whether it includes "sdma" substring.
--- arch/arm/plat-mxc/include/mach/dma.h | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
arch/arm/plat-mxc/include/mach/dma.h +1 -2
@@ -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" ); }
Signature · 1 line
1.7.5.4
Sascha Hauer
maintainerPATCH 1/6 ARM: mxc: make imx_dma_is_general_purpose more generic for sdma
Quoted 4 lines · Richard Zhao
sdma device names vary for different SoC. So we just check whether it includes "sdma" substring.
Signed-off-by Richard Zhao <[email protected]>
Quoted 23 lines
--- arch/arm/plat-mxc/include/mach/dma.h | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)arch/arm/plat-mxc/include/mach/dma.h +1
-2@@ -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" ); }Signature · 1 line
1.7.5.4
Signature · 4 lines
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 2/6 ARM: imx6q: add cko1 clock
In reply to 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.
--- arch/arm/mach-imx/clock-imx6q.c | 74 ++++++++++++++++++++++++++++++++++++++- 1 files changed, 73 insertions(+), 1 deletions(-)
arch/arm/mach-imx/clock-imx6q.c +73 -1
@@ -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);
Signature · 1 line
1.7.5.4
PATCH 3/6 arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators
In reply to Richard Zhao--- arch/arm/boot/dts/imx6q-sabrelite.dts | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
arch/arm/boot/dts/imx6q-sabrelite.dts +20 -0
@@ -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; + }; + }; };
Signature · 1 line
1.7.5.4
PATCH 3/6 arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators
For subject, I would suggest something like below to keep consistency with code patches.
ARM: dts: imx6q-sabrelite: ...
Quoted 10 lines · Richard Zhao
Signed-off-by Richard Zhao <[email protected]>--- arch/arm/boot/dts/imx6q-sabrelite.dts | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)arch/arm/boot/dts/imx6q-sabrelite.dts +0
-0
arch/arm/boot/dts/imx6q-sabrelite.dts line 50
Quoted 5 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0@@ -46,4 +46,24 @@ }; }; }; + + regulators {
Shouldn't this node be under node 'anatop at 020c8000'?
arch/arm/boot/dts/imx6q-sabrelite.dts line 52
Quoted 2 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0+ compatible = "simple-bus"; +
Hmm, do we really need this?
arch/arm/boot/dts/imx6q-sabrelite.dts line 53
Quoted 1 line
arch/arm/boot/dts/imx6q-sabrelite.dts +1
-0+ 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'.
arch/arm/boot/dts/imx6q-sabrelite.dts line 55
Quoted 2 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0+ compatible = "regulator-fixed"; + regulator-name = "2P5V";
It's fine to use capital letters for such case.
Regards,
Shawn
Quoted 16 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +13
-0+ 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; + }; + }; };Signature · 1 line
1.7.5.4
PATCH 3/6 arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators
Quoted 4 lines · Shawn Guo
For subject, I would suggest something like below to keep consistency with code patches.
ARM: dts: imx6q-sabrelite: ...
ok
Quoted 7 lines · Richard Zhao
Quoted 5 lines
Signed-off-by Richard Zhao <[email protected]>--- arch/arm/boot/dts/imx6q-sabrelite.dts | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
Quoted 12 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 50
Quoted 10 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0@@ -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?
Quoted 4 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 52
Quoted 3 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0+ compatible = "simple-bus"; +Hmm, do we really need this?
If I don't set it, the regulator devices will not be populated.
Quoted 6 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 53
Quoted 2 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +1
-0+ 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.
Quoted 3 lines
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.
Quoted 5 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 55
Quoted 3 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0+ compatible = "regulator-fixed"; + regulator-name = "2P5V";It's fine to use capital letters for such case.
ok
Thanks
Richard
Quoted 20 lines
Regards,
ShawnQuoted 16 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +13
-0+ 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; + }; + }; };Signature · 1 line
1.7.5.4
PATCH 3/6 arm/dts: imx6q-sabrelite: add 2P5V and 3P3V regulators
Quoted 25 lines · Richard Zhao
Quoted 4 lines · Shawn Guo
For subject, I would suggest something like below to keep consistency with code patches.
ARM: dts: imx6q-sabrelite: ...
ok
Quoted 7 lines · Richard Zhao
Quoted 5 lines
Signed-off-by Richard Zhao <[email protected]>--- arch/arm/boot/dts/imx6q-sabrelite.dts | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)Quoted 12 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 50
Quoted 10 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0@@ -46,4 +46,24 @@ }; }; }; + + regulators {Shouldn't this node be under node 'anatop at
020c8000'?
Quoted 1 line
It's LDOs on board. Why do I put it in anatop?
Sorry, I messed up here.
Quoted 5 lines
Quoted 4 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 52
Quoted 3 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0+ compatible = "simple-bus"; +Hmm, do we really need this?
If I don't set it, the regulator devices will not be populated.
ditto
Quoted 8 lines
Quoted 6 lines
arch/arm/boot/dts/imx6q-sabrelite.dts line 53
Quoted 2 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +1
-0+ 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.
Quoted 5 lines
Quoted 3 lines
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
In reply to Richard Zhao--- arch/arm/boot/dts/imx6q-sabrelite.dts | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
arch/arm/boot/dts/imx6q-sabrelite.dts +12 -0
@@ -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 = <®_2P5V>; + VDDIO-supply = <®_3P3V>; + }; + }; }; };
Signature · 1 line
1.7.5.4
PATCH 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
Quoted 13 lines · Richard Zhao
Signed-off-by Richard Zhao <[email protected]>--- arch/arm/boot/dts/imx6q-sabrelite.dts | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)arch/arm/boot/dts/imx6q-sabrelite.dts +0
-0@@ -44,6 +44,18 @@ uart2: uart at 021e8000 { status = "okay"; };
arch/arm/boot/dts/imx6q-sabrelite.dts line 56
Quoted 10 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +10
-0+ + i2c at 021a0000 { /* I2C1 */ + status = "okay"; + clock-frequency = <100000>; + + codec: sgtl5000 at 0a { + compatible = "fsl,sgtl5000"; + reg = <0x0a>; + VDDA-supply = <®_2P5V>; + VDDIO-supply = <®_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<---
drivers/regulator/core.c +6 -0
@@ -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
Quoted 9 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +2
-0+ }; + }; }; };Signature · 1 line
1.7.5.4
PATCH 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
Quoted 15 lines · Shawn Guo
Review
Quoted 2 lines · Richard Zhao
(quoted diff) +2
-0+ VDDA-supply = <®_2P5V>; + VDDIO-supply = <®_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 thesupply_namesin upper case, while unlikeof_node_cmp()isstrcasecmp(),of_prop_cmp()is juststrcmp().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);
drivers/regulator/core.c static struct device_node *of_get_regulator(struct device *dev, const char *supp · line 159
Quoted 4 lines
drivers/regulator/core.c +4
-0+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
Quoted 21 lines · Mark Brown
Quoted 15 lines · Shawn Guo
Review
Quoted 2 lines · Richard Zhao
(quoted diff) +2
-0+ VDDA-supply = <®_2P5V>; + VDDIO-supply = <®_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 thesupply_namesin upper case, while unlikeof_node_cmp()isstrcasecmp(),of_prop_cmp()is juststrcmp().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);Review
Quoted 4 lines
(quoted diff) +4
-0+ while (prop_name[i] && i < 32) { + prop_name[i] = tolower(prop_name[i]); + i++; + }
Quoted 5 lines
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?
include/linux/of.h +1 -1
@@ -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
Signature · 2 lines
Regards, Shawn
PATCH 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
Quoted 40 lines · Shawn Guo
Quoted 21 lines · Mark Brown
Quoted 15 lines · Shawn Guo
Review
Quoted 2 lines · Richard Zhao
(quoted diff) +2
-0+ VDDA-supply = <®_2P5V>; + VDDIO-supply = <®_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 thesupply_namesin upper case, while unlikeof_node_cmp()isstrcasecmp(),of_prop_cmp()is juststrcmp().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);Review
Quoted 4 lines
(quoted diff) +4
-0+ while (prop_name[i] && i < 32) { + prop_name[i] = tolower(prop_name[i]); + i++; + }Quoted 5 lines
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?
include/linux/of.h +0
-0
include/linux/of.h static inline unsigned long of_read_ulong(const __be32 *cell, int size) · line 152
Quoted 7 lines
include/linux/of.h +1
-1@@ -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
Quoted 11 lines
Quoted 7 lines
?/* Default string compare functions, Allow arch
asm/prom.hto override */ ?#if !defined(of_compat_cmp)
?#defineof_compat_cmp(s1, s2, l)? ? ?strcasecmp((s1), (s2))(quoted diff) +1
-1-#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)) ?#endifDevice-trees are case sensitive, so I don't think we want to go globally changing that behavior.
Quoted 2 lines
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 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
Quoted 11 lines · Eric Miao
Quoted 11 lines
Quoted 7 lines
?/* Default string compare functions, Allow arch
asm/prom.hto override */ ?#if !defined(of_compat_cmp)
?#defineof_compat_cmp(s1, s2, l)? ? ?strcasecmp((s1), (s2))(quoted diff) +1
-1-#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)) ?#endifDevice-trees are case sensitive, so I don't think we want to go globally changing that behavior.
Quoted 4 lines
Quoted 2 lines
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
Quoted 5 lines
_______________________________________________
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
In reply to Eric MiaoQuoted 4 lines · Eric Miao
Quoted 2 lines
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
Quoted 10 lines · Mark Brown
Quoted 4 lines · Eric Miao
Quoted 2 lines
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");
Signature · 2 lines
Regards, Shawn
PATCH 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
Quoted 11 lines · Shawn Guo
Quoted 3 lines · Mark Brown
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
Quoted 7 lines · Mark Brown
Quoted 5 lines · Shawn Guo
Quoted 3 lines · Mark Brown
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.
Quoted 7 lines
Quoted 5 lines
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.
Signature · 2 lines
Regards, Shawn
PATCH 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
Quoted 6 lines · Shawn Guo
Quoted 1 line · Mark Brown
It's purely an idiom, there's nothing enforcing it.
Actually, a rough calculation on 'git grep
regulator_getdrivers/' 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
Quoted 11 lines · Mark Brown
Quoted 6 lines · Shawn Guo
Quoted 1 line · Mark Brown
It's purely an idiom, there's nothing enforcing it.
Actually, a rough calculation on 'git grep
regulator_getdrivers/' 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
Quoted 6 lines
_______________________________________________
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
Quoted 2 lines · Richard Zhao
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
In reply to Rob HerringQuoted 15 lines · Rob Herring
include/linux/of.h static inline unsigned long of_read_ulong(const __be32 *cell, int size) · line 152
Quoted 12 lines · Shawn Guo
include/linux/of.h +1
-1@@ -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.
Quoted 6 lines
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.
Signature · 2 lines
Regards, Shawn
Fabio Estevam
reviewerPATCH 4/6 arm/dts: imx6q-sabrelite: add sgtl5000 audio codec
In reply to Richard ZhaoHi Richard,
Quoted 18 lines · Richard Zhao
Signed-off-by Richard Zhao <[email protected]>---
?arch/arm/boot/dts/imx6q-sabrelite.dts | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)arch/arm/boot/dts/imx6q-sabrelite.dts +5
-0@@ -44,6 +44,18 @@ ? ? ? ? ? ? ? ? ? ? ? ?uart2: uart at 021e8000 { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = "okay"; ? ? ? ? ? ? ? ? ? ? ? ?}; + + ? ? ? ? ? ? ? ? ? ? ? i2c at 021a0000 { /* I2C1 */ + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? status = "okay"; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clock-frequency = <100000>; +
arch/arm/boot/dts/imx6q-sabrelite.dts line 61
Quoted 10 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +7
-0+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? codec: sgtl5000 at 0a { + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compatible = "fsl,sgtl5000"; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg = <0x0a>; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDA-supply = <®_2P5V>; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDIO-supply = <®_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
Quoted 35 lines · Fabio Estevam
Hi Richard,
Quoted 18 lines · Richard Zhao
Signed-off-by Richard Zhao <[email protected]>---
?arch/arm/boot/dts/imx6q-sabrelite.dts | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)arch/arm/boot/dts/imx6q-sabrelite.dts +5
-0@@ -44,6 +44,18 @@ ? ? ? ? ? ? ? ? ? ? ? ?uart2: uart at 021e8000 { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = "okay"; ? ? ? ? ? ? ? ? ? ? ? ?}; + + ? ? ? ? ? ? ? ? ? ? ? i2c at 021a0000 { /* I2C1 */ + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? status = "okay"; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clock-frequency = <100000>; +arch/arm/boot/dts/imx6q-sabrelite.dts line 61
Quoted 10 lines
arch/arm/boot/dts/imx6q-sabrelite.dts +7
-0+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? codec: sgtl5000 at 0a { + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compatible = "fsl,sgtl5000"; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg = <0x0a>; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDA-supply = <®_2P5V>; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDDIO-supply = <®_3P3V>; + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }; + ? ? ? ? ? ? ? ? ? ? ? }; ? ? ? ? ? ? ? ?}; ? ? ? ?};I don't see a
sound/soc/imx/imx-sgtl5000.cfile yet, so I am wondering how did you test this?
Quoted 1 line
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
Quoted 8 lines
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
In reply to Richard ZhaoIf 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.
--- arch/arm/plat-mxc/audmux-v2.c | 59 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 59 insertions(+), 0 deletions(-)
arch/arm/plat-mxc/audmux-v2.c +59 -0
@@ -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);
Signature · 1 line
1.7.5.4
PATCH 5/6 ARM: mxc: add dt support for audmux-v2
Quoted 13 lines · Richard Zhao
arch/arm/plat-mxc/audmux-v2.c +13
-0+#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 ;
arch/arm/plat-mxc/audmux-v2.c int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr, · line 211
Quoted 10 lines
arch/arm/plat-mxc/audmux-v2.c +10
-0+ } + + 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 5/6 ARM: mxc: add dt support for audmux-v2
Quoted 18 lines · Russell King - ARM Linux
Quoted 17 lines · Richard Zhao
arch/arm/plat-mxc/audmux-v2.c +17
-0+#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) {
Quoted 8 lines
arch/arm/plat-mxc/audmux-v2.c int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr, · line 211
Quoted 6 lines
arch/arm/plat-mxc/audmux-v2.c +6
-0+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
Quoted 6 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sascha Hauer
maintainerPATCH 5/6 ARM: mxc: add dt support for audmux-v2
In reply to Richard ZhaoQuoted 78 lines · Richard Zhao
If
CONFIG_OF, we create a audmux device driver.For other platforms that don't set
CONFIG_OF, it still initialize everything inpostcore_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 <[email protected]>--- arch/arm/plat-mxc/audmux-v2.c | 59 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 59 insertions(+), 0 deletions(-)arch/arm/plat-mxc/audmux-v2.c +50
-0@@ -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,
arch/arm/plat-mxc/audmux-v2.c int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr, · line 246
Quoted 10 lines
arch/arm/plat-mxc/audmux-v2.c +8
-0+ }, +}; + +#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
Signature · 4 lines
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
Quoted 85 lines · Sascha Hauer
Quoted 84 lines · Richard Zhao
If
CONFIG_OF, we create a audmux device driver.For other platforms that don't set
CONFIG_OF, it still initialize everything inpostcore_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 <[email protected]>--- arch/arm/plat-mxc/audmux-v2.c | 59 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 59 insertions(+), 0 deletions(-)arch/arm/plat-mxc/audmux-v2.c +55
-0@@ -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 )
Quoted 8 lines
arch/arm/plat-mxc/audmux-v2.c int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr, · line 246
Quoted 4 lines
arch/arm/plat-mxc/audmux-v2.c +3
-0{ +#ifdef CONFIG_OF +return platform_driver_register (&audmux_v2_driver); +#else Don't do this. Just because
CONFIG_OFis 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
Quoted 15 lines
Sascha
Signature · 9 lines
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
Sascha Hauer
maintainerPATCH 5/6 ARM: mxc: add dt support for audmux-v2
Quoted 94 lines · Richard Zhao
Quoted 85 lines · Sascha Hauer
Quoted 84 lines · Richard Zhao
If
CONFIG_OF, we create a audmux device driver.For other platforms that don't set
CONFIG_OF, it still initialize everything inpostcore_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 <[email protected]>--- arch/arm/plat-mxc/audmux-v2.c | 59 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 59 insertions(+), 0 deletions(-)arch/arm/plat-mxc/audmux-v2.c +55
-0@@ -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 )Quoted 8 lines
arch/arm/plat-mxc/audmux-v2.c int mxc_audmux_v2_configure_port(unsigned int port, unsigned int ptcr, · line 246
Quoted 4 lines
arch/arm/plat-mxc/audmux-v2.c +3
-0{ +#ifdef CONFIG_OF +return platform_driver_register (&audmux_v2_driver); +#else Don't do this. Just because
CONFIG_OFis enabled does not mean that this kernel is only used for OF. This #else breaks starting the kernel the traditional machid way.
Quoted 2 lines
You are right. Look like I have to convert audmux to platform devices for all platforms.
I hoped you would say this :)
Sascha
Signature · 4 lines
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
In reply to Richard ZhaoQuoted 3 lines · Richard Zhao
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?
Signature · 2 lines
Regards, Shawn
PATCH 5/6 ARM: mxc: add dt support for audmux-v2
Quoted 6 lines · Shawn Guo
Quoted 3 lines · Richard Zhao
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
Quoted 5 lines
Signature · 3 lines
-- Regards, Shawn
PATCH 5/6 ARM: mxc: add dt support for audmux-v2
Quoted 7 lines · Richard Zhao
Quoted 2 lines · Shawn Guo
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
Quoted 10 lines · Mark Brown
Quoted 7 lines · Richard Zhao
Quoted 2 lines · Shawn Guo
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
PATCH 6/6 ARM: imx6q-sabrelite: add audmux support
In reply to Richard Zhao- enable audmux in dts
- enable audmux in Kconfig
--- 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(-)
arch/arm/boot/dts/imx6q-sabrelite.dts +4 -0
@@ -41,6 +41,10 @@ status = "okay"; }; + audmux at 021d8000 { + status = "okay"; + }; + uart2: uart at 021e8000 { status = "okay"; };
arch/arm/boot/dts/imx6q.dtsi +2 -0
@@ -527,7 +527,9 @@ }; audmux at 021d8000 { + compatible = "fsl,audmux-v2"; reg = <0x021d8000 0x4000>; + status = "disabled"; }; mipi at 021dc000 { /* MIPI-CSI */
arch/arm/mach-imx/Kconfig +1 -0
@@ -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
Signature · 1 line
1.7.5.4
PATCH 6/6 ARM: imx6q-sabrelite: add audmux support
arch/arm/boot/dts/imx6q.dtsi line 530
Quoted 7 lines · Richard Zhao
arch/arm/boot/dts/imx6q.dtsi +1
-0@@ -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.
Quoted 3 lines
arch/arm/boot/dts/imx6q.dtsi +1
-0reg = <0x021d8000 0x4000>; + status = "disabled"; };
Signature · 2 lines
Regards, Shawn
PATCH 6/6 ARM: imx6q-sabrelite: add audmux support
Quoted 6 lines · Shawn Guo
Quoted 5 lines · Richard Zhao
arch/arm/boot/dts/imx6q.dtsi +0
-0@@ -527,7 +527,9 @@ };
Quoted 7 lines
arch/arm/boot/dts/imx6q.dtsi line 530
Quoted 2 lines
arch/arm/boot/dts/imx6q.dtsi +1
-0audmux 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
Quoted 14 lines
Quoted 4 lines
arch/arm/boot/dts/imx6q.dtsi +1
-0reg = <0x021d8000 0x4000>; + status = "disabled"; };Signature · 7 lines
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
Quoted 14 lines · Richard Zhao
Quoted 6 lines · Shawn Guo
Quoted 5 lines · Richard Zhao
arch/arm/boot/dts/imx6q.dtsi +0
-0@@ -527,7 +527,9 @@ };Quoted 7 lines
arch/arm/boot/dts/imx6q.dtsi line 530
Quoted 2 lines
arch/arm/boot/dts/imx6q.dtsi +1
-0audmux 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.
Quoted 3 lines
compatible = "fsl,imx6q-audmux", "fsl,imx25-audmux"; I forgot imx25 or imx31 is oldest. Is it ok for you?
Yes.
Signature · 2 lines
Regards, Shawn
PATCH 6/6 ARM: imx6q-sabrelite: add audmux support
Quoted 15 lines · Shawn Guo
Quoted 14 lines · Richard Zhao
Quoted 13 lines · Shawn Guo
arch/arm/boot/dts/imx6q.dtsi line 530
Quoted 7 lines · Richard Zhao
arch/arm/boot/dts/imx6q.dtsi +1
-0@@ -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.
Quoted 4 lines
Quoted 3 lines
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";
Signature · 2 lines
Regards, Shawn