| Message ID | 1376300994-1679-2-git-send-email-padma.v@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Padmavathi, On Monday 12 of August 2013 15:19:51 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. > > Signed-off-by: Padmavathi Venna <padma.v@samsung.com> > --- > .../devicetree/bindings/sound/samsung-i2s.txt | 18 ++---- > sound/soc/samsung/i2s.c | 62 > +++++++++++-------- 2 files changed, 42 insertions(+), 38 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt > b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index > 025e66b..25a0024 100644 > --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt > +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt > @@ -2,7 +2,11 @@ > > Required SoC Specific Properties: > > -- compatible : "samsung,i2s-v5" > +- compatible : should be one of the following. > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. > + - 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 +25,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. > @@ -36,7 +33,7 @@ Optional SoC Specific Properties: > Example: > > i2s0: i2s@03830000 { > - compatible = "samsung,i2s-v5"; > + compatible = "samsung,s5pv210-i2s"; > reg = <0x03830000 0x100>; > dmas = <&pdma0 10 > &pdma0 9 > @@ -46,9 +43,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 47e08dd..1671d9b 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 { > @@ -1032,18 +1033,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 const 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 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 > @@ -1074,13 +1075,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; > + const 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"); > @@ -1129,15 +1130,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) { > @@ -1250,27 +1243,44 @@ static int samsung_i2s_remove(struct > platform_device *pdev) return 0; > } > > +static const struct samsung_i2s_dai_data i2sv3_dai_type = { > + .dai_type = TYPE_PRI, > + .quirks = QUIRK_NO_MUXPSR, > +}; > + > +static const struct samsung_i2s_dai_data i2sv5_dai_type = { > + .dai_type = TYPE_PRI, > + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR, > +}; > + > +static const struct samsung_i2s_dai_data samsung_dai_type_pri = { > + .dai_type = TYPE_PRI, > +}; > + > +static const 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, > + .driver_data = (kernel_ulong_t)&samsung_dai_type_pri, > }, { > .name = "samsung-i2s-sec", > - .driver_data = TYPE_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, There is still a hole in this series, between patch 1/4 and 4/4, where existing platforms are broken. This will break bisects. I have commented on this in previous version of this series and suggested two possible solutions, one preferred and one low-effort. Otherwise looks good. Best regards, Tomasz
(CCing DT maintainers...) On 08/12/2013 03:49 AM, 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. > diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt > index 025e66b..25a0024 100644 > --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt > +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt > @@ -2,7 +2,11 @@ > > Required SoC Specific Properties: > > -- compatible : "samsung,i2s-v5" > +- compatible : should be one of the following. > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. > + - 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. Those descriptions seem a little odd. If I have an SoC that isn't s5pv210, yet supports "8/16/24bit multichannel(5.1) I2S with secondary fifo, s/w reset control and internal mux for root clk src", will compatible="samsung,s5pv210-i2s" work for my HW? I wonder if you should instead include the IP block version in the compatible value? Alternatively, perhaps simply removing the description of the capabilities of each SoC would make the binding document more typical. Although all that said, given the semantic meaning of compatible; to describe which specific SW-visible interface is available (which include the feature-set), I feel my comments are a little odd:-) > - 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 +25,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). Is it likely that a driver that fully implements those properties can run on future HW without modification, perhaps adding some extra properties to enable any new features? In other words, I don't think we have an answer to the question: Should differences between similar HW blocks be encoded into DT properties, or should the driver encode them into some table, and look them up from compatible value? This patch changes between those two options. I'm not sure if there's consensus that one option is better than the other, and hence whether this patch is actually necessary or useful. (although I dare say that at least samsung,supports-rstclr should be modified to use the new reset controller bindings) > - pinctrl-0: Should specify pin control groups used for this controller. > @@ -36,7 +33,7 @@ Optional SoC Specific Properties: > Example: > > i2s0: i2s@03830000 { > - compatible = "samsung,i2s-v5"; > + compatible = "samsung,s5pv210-i2s"; > reg = <0x03830000 0x100>; > dmas = <&pdma0 10 > &pdma0 9 > @@ -46,9 +43,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>;
On Mon, Aug 12, 2013 at 04:57:53PM -0600, Stephen Warren wrote: > On 08/12/2013 03:49 AM, Padmavathi Venna wrote: > > +- compatible : should be one of the following. > > + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. > > + - 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. > Those descriptions seem a little odd. If I have an SoC that isn't > s5pv210, yet supports "8/16/24bit multichannel(5.1) I2S with secondary > fifo, s/w reset control and internal mux for root clk src", will > compatible="samsung,s5pv210-i2s" work for my HW? > I wonder if you should instead include the IP block version in the > compatible value? We've been round this loop several times, I'd prefer the IP block versions too but they're at best patchily documented and so as a general policy the Samsung bindings use the name of the SoC an IP first appeared in as the version. > In other words, I don't think we have an answer to the question: Should > differences between similar HW blocks be encoded into DT properties, or > should the driver encode them into some table, and look them up from > compatible value? For usability it seems better to just be able to say which IP you've got, this also makes it easier to implement support for new IP features later on without having to go back and add new properties which would be sad. > (although I dare say that at least samsung,supports-rstclr should be > modified to use the new reset controller bindings) Really? That doesn't seem terribly sane - I had thought that was for bodging resets on the side of things that don't normally have them or need board specific logic. Also note that this is actually a magic register write done to reset the IP on some specific IPs.
On 08/12/2013 05:13 PM, Mark Brown wrote: > On Mon, Aug 12, 2013 at 04:57:53PM -0600, Stephen Warren wrote: >> On 08/12/2013 03:49 AM, Padmavathi Venna wrote: > >>> +- compatible : should be one of the following. + - >>> samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. + - >>> 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. > >> Those descriptions seem a little odd. If I have an SoC that >> isn't s5pv210, yet supports "8/16/24bit multichannel(5.1) I2S >> with secondary fifo, s/w reset control and internal mux for root >> clk src", will compatible="samsung,s5pv210-i2s" work for my HW? > >> I wonder if you should instead include the IP block version in >> the compatible value? > > We've been round this loop several times, I'd prefer the IP block > versions too but they're at best patchily documented and so as a > general policy the Samsung bindings use the name of the SoC an IP > first appeared in as the version. > >> In other words, I don't think we have an answer to the question: >> Should differences between similar HW blocks be encoded into DT >> properties, or should the driver encode them into some table, and >> look them up from compatible value? > > For usability it seems better to just be able to say which IP > you've got, this also makes it easier to implement support for new > IP features later on without having to go back and add new > properties which would be sad. That seems quite reasonable, but I don't think everyone involved in DT has come out and agreed on that. I'm quite happy with the approach of looking up everything based on compatible. >> (although I dare say that at least samsung,supports-rstclr should >> be modified to use the new reset controller bindings) > > Really? That doesn't seem terribly sane - I had thought that was > for bodging resets on the side of things that don't normally have > them or need board specific logic. Also note that this is actually > a magic register write done to reset the IP on some specific IPs. I believe that's exactly what the reset subsystem and associated DT bindings were designed for.
On Mon, Aug 12, 2013 at 05:18:34PM -0600, Stephen Warren wrote: > On 08/12/2013 05:13 PM, Mark Brown wrote: > >> (although I dare say that at least samsung,supports-rstclr should > >> be modified to use the new reset controller bindings) > > Really? That doesn't seem terribly sane - I had thought that was > > for bodging resets on the side of things that don't normally have > > them or need board specific logic. Also note that this is actually > > a magic register write done to reset the IP on some specific IPs. > I believe that's exactly what the reset subsystem and associated DT > bindings were designed for. That seems... interesting. It seems like this is fairly core device functionality I'd expect the driver to just be able to understand; the main thing this property was doing was deciding if the reset was needed. I'm not sure I see the benefit here?
On 08/12/2013 05:46 PM, Mark Brown wrote: > On Mon, Aug 12, 2013 at 05:18:34PM -0600, Stephen Warren wrote: >> On 08/12/2013 05:13 PM, Mark Brown wrote: > >>>> (although I dare say that at least samsung,supports-rstclr >>>> should be modified to use the new reset controller bindings) > >>> Really? That doesn't seem terribly sane - I had thought that >>> was for bodging resets on the side of things that don't >>> normally have them or need board specific logic. Also note >>> that this is actually a magic register write done to reset the >>> IP on some specific IPs. > >> I believe that's exactly what the reset subsystem and associated >> DT bindings were designed for. > > That seems... interesting. It seems like this is fairly core > device functionality I'd expect the driver to just be able to > understand; the main thing this property was doing was deciding if > the reset was needed. I'm not sure I see the benefit here? Sorry, I'm confused here. In this case, the reset is something internal to the IP module, not sourced by an external reset controller, so there's no need to involve the reset controller bindings or subsystem.
diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index 025e66b..25a0024 100644 --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt @@ -2,7 +2,11 @@ Required SoC Specific Properties: -- compatible : "samsung,i2s-v5" +- compatible : should be one of the following. + - samsung,s3c6410-i2s: for 8/16/24bit stereo I2S. + - 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 +25,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. @@ -36,7 +33,7 @@ Optional SoC Specific Properties: Example: i2s0: i2s@03830000 { - compatible = "samsung,i2s-v5"; + compatible = "samsung,s5pv210-i2s"; reg = <0x03830000 0x100>; dmas = <&pdma0 10 &pdma0 9 @@ -46,9 +43,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 47e08dd..1671d9b 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 { @@ -1032,18 +1033,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 const 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 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 @@ -1074,13 +1075,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; + const 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"); @@ -1129,15 +1130,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) { @@ -1250,27 +1243,44 @@ static int samsung_i2s_remove(struct platform_device *pdev) return 0; } +static const struct samsung_i2s_dai_data i2sv3_dai_type = { + .dai_type = TYPE_PRI, + .quirks = QUIRK_NO_MUXPSR, +}; + +static const struct samsung_i2s_dai_data i2sv5_dai_type = { + .dai_type = TYPE_PRI, + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR, +}; + +static const struct samsung_i2s_dai_data samsung_dai_type_pri = { + .dai_type = TYPE_PRI, +}; + +static const 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, + .driver_data = (kernel_ulong_t)&samsung_dai_type_pri, }, { .name = "samsung-i2s-sec", - .driver_data = TYPE_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 | 18 ++---- sound/soc/samsung/i2s.c | 62 +++++++++++-------- 2 files changed, 42 insertions(+), 38 deletions(-)