| Message ID | 20170705055832.4893-1-jcsing.lee@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> > + 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
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.
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; > 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, > }; > >
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, };
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(-)