[alsa-devel] [PATCH] ASoC: samsung: i2s: Supported high resolution rates
This driver can support more frequencies over 96KHz. There are no reasons to limit the frequency range below 96KHz. If codecs/amps or something else can't support high resolution rates, the constraints would be set rates properly because each drivers have its own limits.
Signed-off-by: Jaechul Lee jcsing.lee@samsung.com --- sound/soc/samsung/i2s.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index af3ba4d4ccc5..fc34af6a2c1e 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -50,6 +50,7 @@ struct samsung_i2s_variant_regs {
struct samsung_i2s_dai_data { u32 quirks; + unsigned int rates; const struct samsung_i2s_variant_regs *i2s_variant_regs; };
@@ -1076,20 +1077,25 @@ static const struct snd_soc_component_driver samsung_i2s_component = { .name = "samsung-i2s", };
-#define SAMSUNG_I2S_RATES SNDRV_PCM_RATE_8000_96000 - #define SAMSUNG_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \ SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S24_LE)
static struct i2s_dai *i2s_alloc_dai(struct platform_device *pdev, bool sec) { + const struct samsung_i2s_dai_data *i2s_dai_data; struct i2s_dai *i2s;
i2s = devm_kzalloc(&pdev->dev, sizeof(struct i2s_dai), GFP_KERNEL); if (i2s == NULL) return NULL;
+ if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) + i2s_dai_data = of_device_get_match_data(&pdev->dev); + else + i2s_dai_data = (struct samsung_i2s_dai_data *) + platform_get_device_id(pdev)->driver_data; + i2s->pdev = pdev; i2s->pri_dai = NULL; i2s->sec_dai = NULL; @@ -1101,13 +1107,13 @@ static struct i2s_dai *i2s_alloc_dai(struct platform_device *pdev, bool sec) i2s->i2s_dai_drv.resume = i2s_resume; i2s->i2s_dai_drv.playback.channels_min = 1; i2s->i2s_dai_drv.playback.channels_max = 2; - i2s->i2s_dai_drv.playback.rates = SAMSUNG_I2S_RATES; + i2s->i2s_dai_drv.playback.rates = i2s_dai_data->rates; i2s->i2s_dai_drv.playback.formats = SAMSUNG_I2S_FMTS;
if (!sec) { i2s->i2s_dai_drv.capture.channels_min = 1; i2s->i2s_dai_drv.capture.channels_max = 2; - i2s->i2s_dai_drv.capture.rates = SAMSUNG_I2S_RATES; + i2s->i2s_dai_drv.capture.rates = i2s_dai_data->rates; i2s->i2s_dai_drv.capture.formats = SAMSUNG_I2S_FMTS; } return i2s; @@ -1452,29 +1458,34 @@ static const struct samsung_i2s_variant_regs i2sv5_i2s1_regs = {
static const struct samsung_i2s_dai_data i2sv3_dai_type = { .quirks = QUIRK_NO_MUXPSR, + .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv3_regs, };
static const struct samsung_i2s_dai_data i2sv5_dai_type = { .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_IDMA, + .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv3_regs, };
static const struct samsung_i2s_dai_data i2sv6_dai_type = { .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_TDM | QUIRK_SUPPORTS_IDMA, + .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv6_regs, };
static const struct samsung_i2s_dai_data i2sv7_dai_type = { .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_TDM, + .rates = SNDRV_PCM_RATE_8000_192000, .i2s_variant_regs = &i2sv7_regs, };
static const struct samsung_i2s_dai_data i2sv5_dai_type_i2s1 = { .quirks = QUIRK_PRI_6CHAN | QUIRK_NEED_RSTCLR, + .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv5_i2s1_regs, };
Hi Jaechul,
This driver can support more frequencies over 96KHz. There are no reasons to limit the frequency range below 96KHz. If codecs/amps or something else can't support high resolution rates, the constraints would be set rates properly because each drivers have its own limits.
That's a good commit introduction, but you are not explaining what you actually did. Please explain better how you fixed the issue.
"> Supported high resolution rates"
In the patch title, please use the imperative form, not "supported" but "support".
- if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node)
I do not like drivers to check the Kernel's configuration, I think just checking "pdev->dev.of_node" is enough.
The rest looks good.
Andi
On Wed, Jul 05, 2017 at 03:13:23PM +0900, Andi Shyti wrote:
- if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node)
I do not like drivers to check the Kernel's configuration, I think just checking "pdev->dev.of_node" is enough.
This idiom gets used because it allows the compiler to optimize out the DT section if the kernel is built without DT support, it's fine.
- if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node)
i2s_dai_data = of_device_get_match_data(&pdev->dev);- else
i2s_dai_data = (struct samsung_i2s_dai_data *)platform_get_device_id(pdev)->driver_data;
BTW, as we discussed together already, you are duplicating some code here and it looks ugly. Please refactor.
Thanks, Andi
Hi Jaechul,
On 2017년 07월 05일 14:58, Jaechul Lee wrote:
This driver can support more frequencies over 96KHz. There are no reasons to limit the frequency range below 96KHz. If codecs/amps or something else can't support high resolution rates, the constraints would be set rates properly because each drivers have its own limits.
Signed-off-by: Jaechul Lee jcsing.lee@samsung.com
sound/soc/samsung/i2s.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index af3ba4d4ccc5..fc34af6a2c1e 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -50,6 +50,7 @@ struct samsung_i2s_variant_regs {
struct samsung_i2s_dai_data { u32 quirks;
- unsigned int rates; const struct samsung_i2s_variant_regs *i2s_variant_regs;
};
@@ -1076,20 +1077,25 @@ static const struct snd_soc_component_driver samsung_i2s_component = { .name = "samsung-i2s", };
-#define SAMSUNG_I2S_RATES SNDRV_PCM_RATE_8000_96000
#define SAMSUNG_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \ SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S24_LE)
static struct i2s_dai *i2s_alloc_dai(struct platform_device *pdev, bool sec) {
const struct samsung_i2s_dai_data *i2s_dai_data; struct i2s_dai *i2s;
i2s = devm_kzalloc(&pdev->dev, sizeof(struct i2s_dai), GFP_KERNEL); if (i2s == NULL) return NULL;
if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node)
i2s_dai_data = of_device_get_match_data(&pdev->dev);else
i2s_dai_data = (struct samsung_i2s_dai_data *)platform_get_device_id(pdev)->driver_data;
The samsung_i2s_probe() already gets the 'i2s_dai_data' instance. It is not good to add the duplicate code in the i2s_alloc_dai(). You better to reuse the i2s_dai_data in samsung_i2s_probe()
I think that you can choose the following two method. IMO, I prefer to use the 'method 1'.
method 1. Redefine the 'i2s_alloc_dai' function as following: static struct i2s_dai *i2s_alloc_dai(struct platform_device *pdev, bool sec struct samsung_i2s_dai_data *i2s_dai_data)
method 2. Add the 'i2s_dai_data' field to the 'struct i2s_dai'. @@ -98,6 +98,8 @@ struct i2s_dai { /* Below fields are only valid if this is the primary FIFO */ struct clk *clk_table[3]; struct clk_onecell_data clk_data; + + struct samsung_i2s_dai_data *i2s_dai_data; };
i2s->pdev = pdev; i2s->pri_dai = NULL; i2s->sec_dai = NULL; @@ -1101,13 +1107,13 @@ static struct i2s_dai *i2s_alloc_dai(struct platform_device *pdev, bool sec) i2s->i2s_dai_drv.resume = i2s_resume; i2s->i2s_dai_drv.playback.channels_min = 1; i2s->i2s_dai_drv.playback.channels_max = 2;
- i2s->i2s_dai_drv.playback.rates = SAMSUNG_I2S_RATES;
i2s->i2s_dai_drv.playback.rates = i2s_dai_data->rates; i2s->i2s_dai_drv.playback.formats = SAMSUNG_I2S_FMTS;
if (!sec) { i2s->i2s_dai_drv.capture.channels_min = 1; i2s->i2s_dai_drv.capture.channels_max = 2;
i2s->i2s_dai_drv.capture.rates = SAMSUNG_I2S_RATES;
i2s->i2s_dai_drv.capture.rates = i2s_dai_data->rates;@@ -1452,29 +1458,34 @@ static const struct samsung_i2s_variant_regs i2sv5_i2s1_regs = {
static const struct samsung_i2s_dai_data i2sv3_dai_type = { .quirks = QUIRK_NO_MUXPSR,
- .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv3_regs,
};
static const struct samsung_i2s_dai_data i2sv5_dai_type = { .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_IDMA,
- .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv3_regs,
};
static const struct samsung_i2s_dai_data i2sv6_dai_type = { .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_TDM | QUIRK_SUPPORTS_IDMA,
- .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv6_regs,
};
static const struct samsung_i2s_dai_data i2sv7_dai_type = { .quirks = QUIRK_PRI_6CHAN | QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_TDM,
- .rates = SNDRV_PCM_RATE_8000_192000, .i2s_variant_regs = &i2sv7_regs,
};
static const struct samsung_i2s_dai_data i2sv5_dai_type_i2s1 = { .quirks = QUIRK_PRI_6CHAN | QUIRK_NEED_RSTCLR,
- .rates = SNDRV_PCM_RATE_8000_96000, .i2s_variant_regs = &i2sv5_i2s1_regs,
};
participants (4)
-
Andi Shyti -
Chanwoo Choi -
Jaechul Lee -
Mark Brown