* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-07 8:45 ` Padmavathi Venna
0 siblings, 0 replies; 24+ messages in thread
From: Padmavathi Venna @ 2013-08-07 8:45 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
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 at 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..8a5504c 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 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
@@ -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;
+ 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,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-i2s-multi",
+ .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,
},
{},
};
--
1.7.4.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
2013-08-07 8:45 ` Padmavathi Venna
@ 2013-08-07 11:02 ` Tomasz Figa
-1 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-08-07 11:02 UTC (permalink / raw)
To: Padmavathi Venna
Cc: linux-samsung-soc, linux-arm-kernel, alsa-devel, devicetree,
padma.kvr, broonie, kgene.kim, tomasz.figa, abrestic, swarren,
mark.rutland, pawel.moll, ian.campbell, rob.herring
Hi Padmavathi,
[Ccing DT maintainers with a little comment about contents of this patch
for them:
This is a rework of Samsung i2s bindings to make them more reasonable than
they are at the moment. I know this was supposed to be stable, fixed, ABI,
etc., but as we discussed a lot the whole topic of DT bindings, we need to
sanitize existing bindings and this patch is a part of this work.]
On Wednesday 07 of August 2013 14:15:21 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 | 21 +++---
> sound/soc/samsung/i2s.c | 82
> +++++++++++++------- 2 files changed, 64 insertions(+), 39 deletions(-)
>
> 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.
The comment about previous versions seems a bit enigmatic.
If there is another variant supported by this driver that supports only
8/16bit audio data, then it should have a separate compatible value.
> + - 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 {
The example has also a compatible value set to "samsung,i2s-v5". I don't
think this is compliant to the new bindings.
> <&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..8a5504c 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 {
const 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 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;
If you make the dai_data const this cast will be no longer necessary.
> } 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;
> + struct samsung_i2s_dai_data *i2s_dai_data;
const
> 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,60 @@ static int samsung_i2s_remove(struct
> platform_device *pdev) return 0;
> }
>
> +static struct samsung_i2s_dai_data i2sv3_dai_type = {
const
> + .dai_type = TYPE_PRI,
> + .quirks = QUIRK_NO_MUXPSR,
> +};
> +
> +static struct samsung_i2s_dai_data i2sv4_dai_type = {
ditto
> + .dai_type = TYPE_PRI,
> + .quirks = QUIRK_PRI_6CHAN | QUIRK_NO_MUXPSR,
> +};
> +
> +static struct samsung_i2s_dai_data i2sv5_c100_dai_type = {
ditto
> + .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 = {
ditto
> + .dai_type = TYPE_PRI,
> + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR,
> +};
> +
> +static struct samsung_i2s_dai_data samsung_dai_type_sec = {
ditto
> + .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-i2s-multi",
> + .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,
Hmm, documentation mentions 4 compatible values (and possibly one is
missing there), but this patch adds only 2 of them. I don't think this is
correct, especially that the driver already supports handling all of them.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-07 11:02 ` Tomasz Figa
0 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-08-07 11:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi Padmavathi,
[Ccing DT maintainers with a little comment about contents of this patch
for them:
This is a rework of Samsung i2s bindings to make them more reasonable than
they are at the moment. I know this was supposed to be stable, fixed, ABI,
etc., but as we discussed a lot the whole topic of DT bindings, we need to
sanitize existing bindings and this patch is a part of this work.]
On Wednesday 07 of August 2013 14:15:21 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 | 21 +++---
> sound/soc/samsung/i2s.c | 82
> +++++++++++++------- 2 files changed, 64 insertions(+), 39 deletions(-)
>
> 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.
The comment about previous versions seems a bit enigmatic.
If there is another variant supported by this driver that supports only
8/16bit audio data, then it should have a separate compatible value.
> + - 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 at 03830000 {
The example has also a compatible value set to "samsung,i2s-v5". I don't
think this is compliant to the new bindings.
> <&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..8a5504c 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 {
const 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 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;
If you make the dai_data const this cast will be no longer necessary.
> } 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;
> + struct samsung_i2s_dai_data *i2s_dai_data;
const
> 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,60 @@ static int samsung_i2s_remove(struct
> platform_device *pdev) return 0;
> }
>
> +static struct samsung_i2s_dai_data i2sv3_dai_type = {
const
> + .dai_type = TYPE_PRI,
> + .quirks = QUIRK_NO_MUXPSR,
> +};
> +
> +static struct samsung_i2s_dai_data i2sv4_dai_type = {
ditto
> + .dai_type = TYPE_PRI,
> + .quirks = QUIRK_PRI_6CHAN | QUIRK_NO_MUXPSR,
> +};
> +
> +static struct samsung_i2s_dai_data i2sv5_c100_dai_type = {
ditto
> + .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 = {
ditto
> + .dai_type = TYPE_PRI,
> + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR,
> +};
> +
> +static struct samsung_i2s_dai_data samsung_dai_type_sec = {
ditto
> + .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-i2s-multi",
> + .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,
Hmm, documentation mentions 4 compatible values (and possibly one is
missing there), but this patch adds only 2 of them. I don't think this is
correct, especially that the driver already supports handling all of them.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
2013-08-07 11:02 ` Tomasz Figa
@ 2013-08-07 12:03 ` Padma Venkat
-1 siblings, 0 replies; 24+ messages in thread
From: Padma Venkat @ 2013-08-07 12:03 UTC (permalink / raw)
To: Tomasz Figa
Cc: Padmavathi Venna, linux-samsung-soc,
linux-arm-kernel@lists.infradead.org, alsa-devel, devicetree,
broonie@kernel.org, Kukjin Kim, Tomasz Figa, abrestic,
Stephen Warren, mark.rutland, pawel.moll, ian.campbell,
Rob Herring
Hi Tomasz,
On Wed, Aug 7, 2013 at 4:32 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Padmavathi,
>
> [Ccing DT maintainers with a little comment about contents of this patch
> for them:
>
> This is a rework of Samsung i2s bindings to make them more reasonable than
> they are at the moment. I know this was supposed to be stable, fixed, ABI,
> etc., but as we discussed a lot the whole topic of DT bindings, we need to
> sanitize existing bindings and this patch is a part of this work.]
>
[snip]
>>
>> 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.
>
> The comment about previous versions seems a bit enigmatic.
>
> If there is another variant supported by this driver that supports only
> 8/16bit audio data, then it should have a separate compatible value.
For previous i2s controllers there are two other i2s drivers
available. I think I can delete this comment here.
>
>> + - 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 {
>
> The example has also a compatible value set to "samsung,i2s-v5". I don't
> think this is compliant to the new bindings.
Yes. It's my mistake. Will change this to new one.
>
>> <&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..8a5504c 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 {
>
> const struct samsung_i2s_dai_data
OK.
>
>> 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 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;
>
> If you make the dai_data const this cast will be no longer necessary.
>
>> } 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;
>> + struct samsung_i2s_dai_data *i2s_dai_data;
>
> const
OK.
>
>> 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,60 @@ static int samsung_i2s_remove(struct
>> platform_device *pdev) return 0;
>> }
>>
>> +static struct samsung_i2s_dai_data i2sv3_dai_type = {
>
> const
OK.
>
>> + .dai_type = TYPE_PRI,
>> + .quirks = QUIRK_NO_MUXPSR,
>> +};
>> +
>> +static struct samsung_i2s_dai_data i2sv4_dai_type = {
>
> ditto
>
>> + .dai_type = TYPE_PRI,
>> + .quirks = QUIRK_PRI_6CHAN | QUIRK_NO_MUXPSR,
>> +};
>> +
>> +static struct samsung_i2s_dai_data i2sv5_c100_dai_type = {
>
> ditto
>
>> + .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 = {
>
> ditto
>
>> + .dai_type = TYPE_PRI,
>> + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR,
>> +};
>> +
>> +static struct samsung_i2s_dai_data samsung_dai_type_sec = {
>
> ditto
>
>> + .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-i2s-multi",
>> + .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,
>
> Hmm, documentation mentions 4 compatible values (and possibly one is
> missing there), but this patch adds only 2 of them. I don't think this is
> correct, especially that the driver already supports handling all of them.
Documentation mentions about all the compatible strings which are all
supported by this driver for reference. But some of the platforms
haven't converted to DT yet so here I added the compatible strings for
DT compliant platforms and compatible strings for non-DT compliant
platforms I added in the platform_device_id structure. Any way we have
to make all legacy platforms DT compliant so we need this
"samsung,s3c6410-i2s-multi" in future.
>
> Best regards,
> Tomasz
>
Thanks
Padma
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-07 12:03 ` Padma Venkat
0 siblings, 0 replies; 24+ messages in thread
From: Padma Venkat @ 2013-08-07 12:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tomasz,
On Wed, Aug 7, 2013 at 4:32 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Padmavathi,
>
> [Ccing DT maintainers with a little comment about contents of this patch
> for them:
>
> This is a rework of Samsung i2s bindings to make them more reasonable than
> they are at the moment. I know this was supposed to be stable, fixed, ABI,
> etc., but as we discussed a lot the whole topic of DT bindings, we need to
> sanitize existing bindings and this patch is a part of this work.]
>
[snip]
>>
>> 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.
>
> The comment about previous versions seems a bit enigmatic.
>
> If there is another variant supported by this driver that supports only
> 8/16bit audio data, then it should have a separate compatible value.
For previous i2s controllers there are two other i2s drivers
available. I think I can delete this comment here.
>
>> + - 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 at 03830000 {
>
> The example has also a compatible value set to "samsung,i2s-v5". I don't
> think this is compliant to the new bindings.
Yes. It's my mistake. Will change this to new one.
>
>> <&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..8a5504c 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 {
>
> const struct samsung_i2s_dai_data
OK.
>
>> 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 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;
>
> If you make the dai_data const this cast will be no longer necessary.
>
>> } 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;
>> + struct samsung_i2s_dai_data *i2s_dai_data;
>
> const
OK.
>
>> 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,60 @@ static int samsung_i2s_remove(struct
>> platform_device *pdev) return 0;
>> }
>>
>> +static struct samsung_i2s_dai_data i2sv3_dai_type = {
>
> const
OK.
>
>> + .dai_type = TYPE_PRI,
>> + .quirks = QUIRK_NO_MUXPSR,
>> +};
>> +
>> +static struct samsung_i2s_dai_data i2sv4_dai_type = {
>
> ditto
>
>> + .dai_type = TYPE_PRI,
>> + .quirks = QUIRK_PRI_6CHAN | QUIRK_NO_MUXPSR,
>> +};
>> +
>> +static struct samsung_i2s_dai_data i2sv5_c100_dai_type = {
>
> ditto
>
>> + .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 = {
>
> ditto
>
>> + .dai_type = TYPE_PRI,
>> + .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR,
>> +};
>> +
>> +static struct samsung_i2s_dai_data samsung_dai_type_sec = {
>
> ditto
>
>> + .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-i2s-multi",
>> + .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,
>
> Hmm, documentation mentions 4 compatible values (and possibly one is
> missing there), but this patch adds only 2 of them. I don't think this is
> correct, especially that the driver already supports handling all of them.
Documentation mentions about all the compatible strings which are all
supported by this driver for reference. But some of the platforms
haven't converted to DT yet so here I added the compatible strings for
DT compliant platforms and compatible strings for non-DT compliant
platforms I added in the platform_device_id structure. Any way we have
to make all legacy platforms DT compliant so we need this
"samsung,s3c6410-i2s-multi" in future.
>
> Best regards,
> Tomasz
>
Thanks
Padma
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
2013-08-07 8:45 ` Padmavathi Venna
@ 2013-08-07 11:13 ` Tomasz Figa
-1 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-08-07 11:13 UTC (permalink / raw)
To: Padmavathi Venna
Cc: linux-samsung-soc, linux-arm-kernel, alsa-devel, devicetree,
padma.kvr, broonie, kgene.kim, tomasz.figa, abrestic
Ahh, one more thing inline.
On Wednesday 07 of August 2013 14:15:21 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 | 21 +++---
> sound/soc/samsung/i2s.c | 82
> +++++++++++++------- 2 files changed, 64 insertions(+), 39 deletions(-)
>
> 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 47e08dd..8a5504c 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 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
> @@ -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;
> + 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,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-i2s-multi",
> + .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,
I don't think you need to change the legacy platform IDs at all.
IMHO it would be better to keep the old way of quirk retrieval from
platform_data (which I think this patch does anyway, because the probe path
without DT is unchanged), as it will be dropped in some point in time
anyway.
This would also eliminate the need for patch 1.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-07 11:13 ` Tomasz Figa
0 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-08-07 11:13 UTC (permalink / raw)
To: linux-arm-kernel
Ahh, one more thing inline.
On Wednesday 07 of August 2013 14:15:21 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 | 21 +++---
> sound/soc/samsung/i2s.c | 82
> +++++++++++++------- 2 files changed, 64 insertions(+), 39 deletions(-)
>
> 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 at 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..8a5504c 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 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
> @@ -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;
> + 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,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-i2s-multi",
> + .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,
I don't think you need to change the legacy platform IDs at all.
IMHO it would be better to keep the old way of quirk retrieval from
platform_data (which I think this patch does anyway, because the probe path
without DT is unchanged), as it will be dropped in some point in time
anyway.
This would also eliminate the need for patch 1.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
2013-08-07 11:13 ` Tomasz Figa
@ 2013-08-08 7:43 ` Padma Venkat
-1 siblings, 0 replies; 24+ messages in thread
From: Padma Venkat @ 2013-08-08 7:43 UTC (permalink / raw)
To: Tomasz Figa
Cc: Padmavathi Venna, linux-samsung-soc,
linux-arm-kernel@lists.infradead.org, alsa-devel, devicetree,
broonie@kernel.org, Kukjin Kim, Tomasz Figa, abrestic
Hi Tomasz,
On Wed, Aug 7, 2013 at 4:43 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Ahh, one more thing inline.
>
[snip]
>>
>> +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-i2s-multi",
>> + .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,
>
> I don't think you need to change the legacy platform IDs at all.
If legacy platforms not required to change then I need to introduce a
new samsung_i2s_dai_data structure which holds only dai_type for
non-dt platforms. If I change legacy platforms it breaks the older
platforms now. Is it okay adding a samsung_i2s_dai_data structure for
non-dt platforms which will be removed later?
>
> IMHO it would be better to keep the old way of quirk retrieval from
> platform_data (which I think this patch does anyway, because the probe path
> without DT is unchanged), as it will be dropped in some point in time
> anyway.
>
> This would also eliminate the need for patch 1.
>
> Best regards,
> Tomasz
>
Thanks
Padma
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-08 7:43 ` Padma Venkat
0 siblings, 0 replies; 24+ messages in thread
From: Padma Venkat @ 2013-08-08 7:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tomasz,
On Wed, Aug 7, 2013 at 4:43 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Ahh, one more thing inline.
>
[snip]
>>
>> +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-i2s-multi",
>> + .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,
>
> I don't think you need to change the legacy platform IDs at all.
If legacy platforms not required to change then I need to introduce a
new samsung_i2s_dai_data structure which holds only dai_type for
non-dt platforms. If I change legacy platforms it breaks the older
platforms now. Is it okay adding a samsung_i2s_dai_data structure for
non-dt platforms which will be removed later?
>
> IMHO it would be better to keep the old way of quirk retrieval from
> platform_data (which I think this patch does anyway, because the probe path
> without DT is unchanged), as it will be dropped in some point in time
> anyway.
>
> This would also eliminate the need for patch 1.
>
> Best regards,
> Tomasz
>
Thanks
Padma
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
2013-08-08 7:43 ` Padma Venkat
@ 2013-08-08 8:10 ` Tomasz Figa
-1 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-08-08 8:10 UTC (permalink / raw)
To: Padma Venkat
Cc: devicetree, alsa-devel, linux-samsung-soc, Padmavathi Venna,
abrestic, Tomasz Figa, broonie@kernel.org, Kukjin Kim,
linux-arm-kernel@lists.infradead.org
On Thursday 08 of August 2013 13:13:02 Padma Venkat wrote:
> Hi Tomasz,
>
> On Wed, Aug 7, 2013 at 4:43 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Ahh, one more thing inline.
>
> [snip]
>
> >> +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-i2s-multi",
> >> + .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,
> >
> > I don't think you need to change the legacy platform IDs at all.
>
> If legacy platforms not required to change then I need to introduce a
> new samsung_i2s_dai_data structure which holds only dai_type for
> non-dt platforms. If I change legacy platforms it breaks the older
> platforms now. Is it okay adding a samsung_i2s_dai_data structure for
> non-dt platforms which will be removed later?
Sorry, I don't understand the problem here.
Original contents of samsung_i2s_driver_ids[] array already conveyed
information about dai type inside driver_data field of each
platform_device_id entry.
If you really need to have samsung_i2s_dai_data struct for both DT and
non-DT cases you can create dummy dai_data for both platform ID entries,
which would have only dai type field initialized. This would be fine,
because the non-DT probe path takes anything else from platform_data and
would not need samsung_i2s_dai_data struct in any other way than getting
dai_type in probe.
Best regards,
Tomasz
> > IMHO it would be better to keep the old way of quirk retrieval from
> > platform_data (which I think this patch does anyway, because the probe
> > path without DT is unchanged), as it will be dropped in some point in
> > time anyway.
> >
> > This would also eliminate the need for patch 1.
> >
> > Best regards,
> > Tomasz
>
> Thanks
> Padma
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-08 8:10 ` Tomasz Figa
0 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-08-08 8:10 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 08 of August 2013 13:13:02 Padma Venkat wrote:
> Hi Tomasz,
>
> On Wed, Aug 7, 2013 at 4:43 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Ahh, one more thing inline.
>
> [snip]
>
> >> +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-i2s-multi",
> >> + .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,
> >
> > I don't think you need to change the legacy platform IDs at all.
>
> If legacy platforms not required to change then I need to introduce a
> new samsung_i2s_dai_data structure which holds only dai_type for
> non-dt platforms. If I change legacy platforms it breaks the older
> platforms now. Is it okay adding a samsung_i2s_dai_data structure for
> non-dt platforms which will be removed later?
Sorry, I don't understand the problem here.
Original contents of samsung_i2s_driver_ids[] array already conveyed
information about dai type inside driver_data field of each
platform_device_id entry.
If you really need to have samsung_i2s_dai_data struct for both DT and
non-DT cases you can create dummy dai_data for both platform ID entries,
which would have only dai type field initialized. This would be fine,
because the non-DT probe path takes anything else from platform_data and
would not need samsung_i2s_dai_data struct in any other way than getting
dai_type in probe.
Best regards,
Tomasz
> > IMHO it would be better to keep the old way of quirk retrieval from
> > platform_data (which I think this patch does anyway, because the probe
> > path without DT is unchanged), as it will be dropped in some point in
> > time anyway.
> >
> > This would also eliminate the need for patch 1.
> >
> > Best regards,
> > Tomasz
>
> Thanks
> Padma
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
2013-08-08 7:43 ` Padma Venkat
@ 2013-08-08 10:31 ` Mark Brown
-1 siblings, 0 replies; 24+ messages in thread
From: Mark Brown @ 2013-08-08 10:31 UTC (permalink / raw)
To: Padma Venkat
Cc: devicetree, alsa-devel, linux-samsung-soc, Padmavathi Venna,
abrestic, Tomasz Figa, Tomasz Figa, Kukjin Kim,
linux-arm-kernel@lists.infradead.org
[-- Attachment #1.1: Type: text/plain, Size: 616 bytes --]
On Thu, Aug 08, 2013 at 01:13:02PM +0530, Padma Venkat wrote:
> If legacy platforms not required to change then I need to introduce a
> new samsung_i2s_dai_data structure which holds only dai_type for
> non-dt platforms. If I change legacy platforms it breaks the older
> platforms now. Is it okay adding a samsung_i2s_dai_data structure for
> non-dt platforms which will be removed later?
This is totally fine and looks much nicer - you're not creating DT style
names in the platform bus, nor making names that are so long that we
need to expand the maximum length of a plaform device name (which isn't
awesome).
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH V3 2/3] ASoC: Samsung: I2S: Add quirks as driver data in I2S
@ 2013-08-08 10:31 ` Mark Brown
0 siblings, 0 replies; 24+ messages in thread
From: Mark Brown @ 2013-08-08 10:31 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 08, 2013 at 01:13:02PM +0530, Padma Venkat wrote:
> If legacy platforms not required to change then I need to introduce a
> new samsung_i2s_dai_data structure which holds only dai_type for
> non-dt platforms. If I change legacy platforms it breaks the older
> platforms now. Is it okay adding a samsung_i2s_dai_data structure for
> non-dt platforms which will be removed later?
This is totally fine and looks much nicer - you're not creating DT style
names in the platform bus, nor making names that are so long that we
need to expand the maximum length of a plaform device name (which isn't
awesome).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130808/3311f5b7/attachment.sig>
^ permalink raw reply [flat|nested] 24+ messages in thread