| Message ID | 1374845812-7803-3-git-send-email-padma.v@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: > -- compatible : "samsung,i2s-v5" > +- compatible : should be one of the following. > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions > + has only 8/16bit support. > + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) I2S. > + Introduced in s3c6410. This also applicable for s5p64x0 platforms. > + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 channel) I2S > + with secondary fifo and s/w reset control. > + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with > + secondary fifo, s/w reset control and internal mux for root clk src. > + So what happens with your changes to everyone who is using a DT file with the "samsung,i2s-v5" compatible string?
Hi Russell, On Friday 26 of July 2013 15:06:18 Russell King - ARM Linux wrote: > On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: > > -- compatible : "samsung,i2s-v5" > > +- compatible : should be one of the following. > > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions > > + has only 8/16bit support. > > + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) > > I2S. + Introduced in s3c6410. This also applicable for s5p64x0 > > platforms. + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 > > channel) I2S + with secondary fifo and s/w reset control. > > + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with > > + secondary fifo, s/w reset control and internal mux for root clk > > src. + > > So what happens with your changes to everyone who is using a DT file with > the "samsung,i2s-v5" compatible string? AFAIK we decided that current bindings, if broken, can be redone correctly, without caring for compatibility with old DTBs and only then, after reviewing these new bindings by DT people, they can be stabilized. Other issue, though, is that this patch breaks things until they get fixed by patch 3. Support for new bindings should be added first, then users fixed and only then old bindings can be removed. Best regards, Tomasz
On Fri, Jul 26, 2013 at 04:21:16PM +0200, Tomasz Figa wrote: > Hi Russell, > > On Friday 26 of July 2013 15:06:18 Russell King - ARM Linux wrote: > > On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: > > > -- compatible : "samsung,i2s-v5" > > > +- compatible : should be one of the following. > > > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions > > > + has only 8/16bit support. > > > + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) > > > I2S. + Introduced in s3c6410. This also applicable for s5p64x0 > > > platforms. + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 > > > channel) I2S + with secondary fifo and s/w reset control. > > > + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with > > > + secondary fifo, s/w reset control and internal mux for root clk > > > src. + > > > > So what happens with your changes to everyone who is using a DT file with > > the "samsung,i2s-v5" compatible string? > > AFAIK we decided that current bindings, if broken, can be redone correctly, > without caring for compatibility with old DTBs and only then, after > reviewing these new bindings by DT people, they can be stabilized. > > Other issue, though, is that this patch breaks things until they get fixed > by patch 3. Support for new bindings should be added first, then users fixed > and only then old bindings can be removed. So, these bindings were merged in the v3.9 merge window, using the "samsung,i2s-v5" compatible string, and now for the v3.12 merge window, you're proposing to break the audio description in any DT file which has been used with v3.9..v3.11 inclusive? The way you should be doing this is to keep the existing stuff working and supplement it with the new method of describing the hardware.
On Friday 26 of July 2013 15:27:22 Russell King - ARM Linux wrote: > On Fri, Jul 26, 2013 at 04:21:16PM +0200, Tomasz Figa wrote: > > Hi Russell, > > > > On Friday 26 of July 2013 15:06:18 Russell King - ARM Linux wrote: > > > On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: > > > > -- compatible : "samsung,i2s-v5" > > > > +- compatible : should be one of the following. > > > > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous > > > > versions > > > > + has only 8/16bit support. > > > > + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 > > > > channel) > > > > I2S. + Introduced in s3c6410. This also applicable for s5p64x0 > > > > platforms. + - samsung,s5pc100-i2s: for 8/16/24bit > > > > multichannel(5.1 > > > > channel) I2S + with secondary fifo and s/w reset control. > > > > + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S > > > > with > > > > + secondary fifo, s/w reset control and internal mux for root > > > > clk > > > > src. + > > > > > > So what happens with your changes to everyone who is using a DT file > > > with the "samsung,i2s-v5" compatible string? > > > > AFAIK we decided that current bindings, if broken, can be redone > > correctly, without caring for compatibility with old DTBs and only > > then, after reviewing these new bindings by DT people, they can be > > stabilized. > > > > Other issue, though, is that this patch breaks things until they get > > fixed by patch 3. Support for new bindings should be added first, then > > users fixed and only then old bindings can be removed. > > So, these bindings were merged in the v3.9 merge window, using the > "samsung,i2s-v5" compatible string, and now for the v3.12 merge window, > you're proposing to break the audio description in any DT file which has > been used with v3.9..v3.11 inclusive? It depends whether we want to keep all the broken stuff. At current state dts is still pretty much coupled with kernel version from which it comes from, so there is not yet too late to replace the broken bindings with something acceptable. > > The way you should be doing this is to keep the existing stuff working > and supplement it with the new method of describing the hardware. Ideally yes and we are moving towards this with all the plans to separate stable and staging bindings. However we are currently half-drowning in a swamp of broken bindings, so we would rather try to clean the mess ASAP, before things stabilize. Best regards, Tomasz
On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: > Samsung has different versions of I2S introduced in different > platforms. Each version has some new support added for multichannel, > secondary fifo, s/w reset control and internal mux for rclk src clk. > Each newly added change has a quirk. So this patch adds all the > required quirks as driver data and based on compatible string from > dtsi fetches the quirks. As Russell indicated you should really keep the old name around, though marking them as deprecated is OK. However I'm not sure anyone will have deployed this so I'm not sure how much it matters - every downstream kernel I've seen was still using board files anyway. The actual meat of the patch changing to a quirk scheme does look good, though. > -- compatible : "samsung,i2s-v5" > +- compatible : should be one of the following. > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions > + has only 8/16bit support. > + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) I2S. > + Introduced in s3c6410. This also applicable for s5p64x0 platforms. > + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 channel) I2S > + with secondary fifo and s/w reset control. > + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with > + secondary fifo, s/w reset control and internal mux for root clk src. > + I think the -vN naming scheme was fine - I see where this came from but the main point was about having things identified by a string not switching the naming scheme. As you can see from the s3c6410 stuff the SoC isn't that helpful as a naming scheme as multiple IP versions appear on the same SoC.
On Friday 26 of July 2013 15:53:19 Mark Brown wrote: > On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: > > Samsung has different versions of I2S introduced in different > > platforms. Each version has some new support added for multichannel, > > secondary fifo, s/w reset control and internal mux for rclk src clk. > > Each newly added change has a quirk. So this patch adds all the > > required quirks as driver data and based on compatible string from > > dtsi fetches the quirks. > > As Russell indicated you should really keep the old name around, though > marking them as deprecated is OK. However I'm not sure anyone will have > deployed this so I'm not sure how much it matters - every downstream > kernel I've seen was still using board files anyway. > > The actual meat of the patch changing to a quirk scheme does look good, > though. > > > -- compatible : "samsung,i2s-v5" > > +- compatible : should be one of the following. > > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions > > + has only 8/16bit support. > > + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) > > I2S. + Introduced in s3c6410. This also applicable for s5p64x0 > > platforms. + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 > > channel) I2S + with secondary fifo and s/w reset control. > > + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with > > + secondary fifo, s/w reset control and internal mux for root clk > > src. + > > I think the -vN naming scheme was fine - I see where this came from but > the main point was about having things identified by a string not > switching the naming scheme. As you can see from the s3c6410 stuff the > SoC isn't that helpful as a naming scheme as multiple IP versions appear > on the same SoC. IMHO this SoC-based identification looks much better, especially considering the fact that IP version isn't something easily determinable, as even the documentation can sometimes be not really clear about that. However the s3c6410-i2sv4 string looks a bit unfortunate. AFAIK there were two types of I2S IPs on S3C6410 - normal I2S and I2S multichannel. What about having a compatible like s3c6410-i2s-multi? Best regards, Tomasz
On Fri, Jul 26, 2013 at 05:02:46PM +0200, Tomasz Figa wrote: > IMHO this SoC-based identification looks much better, especially considering > the fact that IP version isn't something easily determinable, as even the > documentation can sometimes be not really clear about that. Yeah, it's not terribly clever either way. We've been using the version numbers in audio for a long time partly because it is documented sometimes and partly because most of the SoCs tend to have one fully featured controller and a bunch of secondary controllers on older IP revisions. > However the s3c6410-i2sv4 string looks a bit unfortunate. AFAIK there were > two types of I2S IPs on S3C6410 - normal I2S and I2S multichannel. What > about having a compatible like s3c6410-i2s-multi? It was explicitly identified as I2Sv4 in the S3C6410 datasheet so no real issue there.
Hi Mark, On Fri, Jul 26, 2013 at 8:23 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Jul 26, 2013 at 07:06:46PM +0530, Padmavathi Venna wrote: >> Samsung has different versions of I2S introduced in different >> platforms. Each version has some new support added for multichannel, >> secondary fifo, s/w reset control and internal mux for rclk src clk. >> Each newly added change has a quirk. So this patch adds all the >> required quirks as driver data and based on compatible string from >> dtsi fetches the quirks. > > As Russell indicated you should really keep the old name around, though > marking them as deprecated is OK. However I'm not sure anyone will have > deployed this so I'm not sure how much it matters - every downstream > kernel I've seen was still using board files anyway. This is there only on exynos5250.dtsi, so changing this file alone is enough. patch3 in this series have the same. > > The actual meat of the patch changing to a quirk scheme does look good, > though. > >> -- compatible : "samsung,i2s-v5" >> +- compatible : should be one of the following. >> + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions >> + has only 8/16bit support. >> + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) I2S. >> + Introduced in s3c6410. This also applicable for s5p64x0 platforms. >> + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 channel) I2S >> + with secondary fifo and s/w reset control. >> + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with >> + secondary fifo, s/w reset control and internal mux for root clk src. >> + > > I think the -vN naming scheme was fine - I see where this came from but > the main point was about having things identified by a string not > switching the naming scheme. As you can see from the s3c6410 stuff the > SoC isn't that helpful as a naming scheme as multiple IP versions appear > on the same SoC. Having only the version info is confusing. When I posted my previous version of patches I was clear which version introduced in which platform and again if I come back today and see I again had to search each SoC datasheet. So I think this patch now clearly explains what new support introduced in which version of IP and which SoC platform. Thanks Padma
On Sat, Jul 27, 2013 at 06:38:02AM +0530, Padma Venkat wrote: > On Fri, Jul 26, 2013 at 8:23 PM, Mark Brown <broonie@kernel.org> wrote: > > As Russell indicated you should really keep the old name around, though > > marking them as deprecated is OK. However I'm not sure anyone will have > > deployed this so I'm not sure how much it matters - every downstream > > kernel I've seen was still using board files anyway. > This is there only on exynos5250.dtsi, so changing this file alone is > enough. patch3 in > this series have the same. The idea with DT is that you can bake the DT into a board firmware and then upgrade the kernel without upgrading the DT. > Having only the version info is confusing. When I posted my previous version of > patches I was clear which version introduced in which platform and > again if I come > back today and see I again had to search each SoC datasheet. So I think this > patch now clearly explains what new support introduced in which > version of IP and which > SoC platform. Like I keep saying the problem we've always had is that there's never been a 1:1 mapping between SoCs and IIS IPs, and of course nobody can get the documentation on the older SoCs outside of Samsung so... If it were a linear march forward in terms of IP it'd be a lot easier :(
On Friday 26 of July 2013 16:25:51 Mark Brown wrote: > On Fri, Jul 26, 2013 at 05:02:46PM +0200, Tomasz Figa wrote: > > IMHO this SoC-based identification looks much better, especially > > considering the fact that IP version isn't something easily > > determinable, as even the documentation can sometimes be not really > > clear about that. > > Yeah, it's not terribly clever either way. We've been using the version > numbers in audio for a long time partly because it is documented > sometimes and partly because most of the SoCs tend to have one fully > featured controller and a bunch of secondary controllers on older IP > revisions. > > > However the s3c6410-i2sv4 string looks a bit unfortunate. AFAIK there > > were two types of I2S IPs on S3C6410 - normal I2S and I2S > > multichannel. What about having a compatible like s3c6410-i2s-multi? > > It was explicitly identified as I2Sv4 in the S3C6410 datasheet so no > real issue there. Well, the datasheet I have calls it either "I2S V40" or "IIS MULTI AUDIO INTERFACE". I like the latter much more, because it actually says what's the difference compared to previous I2S IPs. I'm not strongly against using the v4 suffix, but since we decided to use more meaningful compatible values elsewhere, I think this way would be better for sound drivers as well. Best regards, Tomasz
diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index 025e66b..b3f6443 100644 --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt @@ -2,7 +2,16 @@ Required SoC Specific Properties: -- compatible : "samsung,i2s-v5" +- compatible : should be one of the following. + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. Previous versions + has only 8/16bit support. + - samsung,s3c6410-i2sv4: for 8/16/24bit multichannel(5.1 channel) I2S. + Introduced in s3c6410. This also applicable for s5p64x0 platforms. + - samsung,s5pc100-i2s: for 8/16/24bit multichannel(5.1 channel) I2S + with secondary fifo and s/w reset control. + - samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with + secondary fifo, s/w reset control and internal mux for root clk src. + - reg: physical base address of the controller and length of memory mapped region. - dmas: list of DMA controller phandle and DMA request line ordered pairs. @@ -21,13 +30,6 @@ Required SoC Specific Properties: Optional SoC Specific Properties: -- samsung,supports-6ch: If the I2S Primary sound source has 5.1 Channel - support, this flag is enabled. -- samsung,supports-rstclr: This flag should be set if I2S software reset bit - control is required. When this flag is set I2S software reset bit will be - enabled or disabled based on need. -- samsung,supports-secdai:If I2S block has a secondary FIFO and internal DMA, - then this flag is enabled. - samsung,idma-addr: Internal DMA register base address of the audio sub system(used in secondary sound source). - pinctrl-0: Should specify pin control groups used for this controller. @@ -46,9 +48,6 @@ i2s0: i2s@03830000 { <&clock_audss EXYNOS_I2S_BUS>, <&clock_audss EXYNOS_SCLK_I2S>; clock-names = "iis", "i2s_opclk0", "i2s_opclk1"; - samsung,supports-6ch; - samsung,supports-rstclr; - samsung,supports-secdai; samsung,idma-addr = <0x03000000>; pinctrl-names = "default"; pinctrl-0 = <&i2s0_bus>; diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index 959c702..f661a98 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -40,6 +40,7 @@ enum samsung_dai_type { struct samsung_i2s_dai_data { int dai_type; + u32 quirks; }; struct i2s_dai { @@ -1018,18 +1019,18 @@ static struct i2s_dai *i2s_alloc_dai(struct platform_device *pdev, bool sec) static const struct of_device_id exynos_i2s_match[]; -static inline int samsung_i2s_get_driver_data(struct platform_device *pdev) +static inline struct samsung_i2s_dai_data *samsung_i2s_get_driver_data( + struct platform_device *pdev) { #ifdef CONFIG_OF - struct samsung_i2s_dai_data *data; if (pdev->dev.of_node) { const struct of_device_id *match; match = of_match_node(exynos_i2s_match, pdev->dev.of_node); - data = (struct samsung_i2s_dai_data *) match->data; - return data->dai_type; + return (struct samsung_i2s_dai_data *) match->data; } else #endif - return platform_get_device_id(pdev)->driver_data; + return (struct samsung_i2s_dai_data *) + platform_get_device_id(pdev)->driver_data; } #ifdef CONFIG_PM_RUNTIME @@ -1060,13 +1061,13 @@ static int samsung_i2s_probe(struct platform_device *pdev) struct resource *res; u32 regs_base, quirks = 0, idma_addr = 0; struct device_node *np = pdev->dev.of_node; - enum samsung_dai_type samsung_dai_type; + struct samsung_i2s_dai_data *i2s_dai_data; int ret = 0; /* Call during Seconday interface registration */ - samsung_dai_type = samsung_i2s_get_driver_data(pdev); + i2s_dai_data = samsung_i2s_get_driver_data(pdev); - if (samsung_dai_type == TYPE_SEC) { + if (i2s_dai_data->dai_type == TYPE_SEC) { sec_dai = dev_get_drvdata(&pdev->dev); if (!sec_dai) { dev_err(&pdev->dev, "Unable to get drvdata\n"); @@ -1115,15 +1116,7 @@ static int samsung_i2s_probe(struct platform_device *pdev) idma_addr = i2s_cfg->idma_addr; } } else { - if (of_find_property(np, "samsung,supports-6ch", NULL)) - quirks |= QUIRK_PRI_6CHAN; - - if (of_find_property(np, "samsung,supports-secdai", NULL)) - quirks |= QUIRK_SEC_DAI; - - if (of_find_property(np, "samsung,supports-rstclr", NULL)) - quirks |= QUIRK_NEED_RSTCLR; - + quirks = i2s_dai_data->quirks; if (of_property_read_u32(np, "samsung,idma-addr", &idma_addr)) { if (quirks & QUIRK_SEC_DAI) { @@ -1236,27 +1229,60 @@ static int samsung_i2s_remove(struct platform_device *pdev) return 0; } +static struct samsung_i2s_dai_data i2sv3_dai_type = { + .dai_type = TYPE_PRI, + .quirks = QUIRK_NO_MUXPSR, +}; + +static struct samsung_i2s_dai_data i2sv4_dai_type = { + .dai_type = TYPE_PRI, + .quirks = QUIRK_PRI_6CHAN | QUIRK_NO_MUXPSR, +}; + +static struct samsung_i2s_dai_data i2sv5_c100_dai_type = { + .dai_type = TYPE_PRI, + .quirks = QUIRK_PRI_6CHAN | QUIRK_NO_MUXPSR | QUIRK_SEC_DAI | + QUIRK_NEED_RSTCLR, +}; + +static struct samsung_i2s_dai_data i2sv5_dai_type = { + .dai_type = TYPE_PRI, + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR, +}; + +static struct samsung_i2s_dai_data samsung_dai_type_sec = { + .dai_type = TYPE_SEC, +}; + static struct platform_device_id samsung_i2s_driver_ids[] = { { - .name = "samsung-i2s", - .driver_data = TYPE_PRI, + .name = "samsung,s3c6410-i2s", + .driver_data = (kernel_ulong_t)&i2sv3_dai_type, + }, { + .name = "samsung,s3c6410-i2sv4", + .driver_data = (kernel_ulong_t)&i2sv4_dai_type, + }, { + .name = "samsung,s5pc100-i2s", + .driver_data = (kernel_ulong_t)&i2sv5_c100_dai_type, }, { - .name = "samsung-i2s-sec", - .driver_data = TYPE_SEC, + .name = "samsung,s5pv210-i2s", + .driver_data = (kernel_ulong_t)&i2sv5_dai_type, + }, { + .name = "samsung-i2s-sec", + .driver_data = (kernel_ulong_t)&samsung_dai_type_sec, }, {}, }; MODULE_DEVICE_TABLE(platform, samsung_i2s_driver_ids); #ifdef CONFIG_OF -static struct samsung_i2s_dai_data samsung_i2s_dai_data_array[] = { - [TYPE_PRI] = { TYPE_PRI }, - [TYPE_SEC] = { TYPE_SEC }, -}; - static const struct of_device_id exynos_i2s_match[] = { - { .compatible = "samsung,i2s-v5", - .data = &samsung_i2s_dai_data_array[TYPE_PRI], + { + .compatible = "samsung,s3c6410-i2s", + .data = &i2sv3_dai_type, + }, { + .compatible = "samsung,s5pv210-i2s", + .data = &i2sv5_dai_type, }, {}, };
Samsung has different versions of I2S introduced in different platforms. Each version has some new support added for multichannel, secondary fifo, s/w reset control and internal mux for rclk src clk. Each newly added change has a quirk. So this patch adds all the required quirks as driver data and based on compatible string from dtsi fetches the quirks. Signed-off-by: Padmavathi Venna <padma.v@samsung.com> --- .../devicetree/bindings/sound/samsung-i2s.txt | 21 +++--- sound/soc/samsung/i2s.c | 82 +++++++++++++------- 2 files changed, 64 insertions(+), 39 deletions(-)