| Message ID | 54FF7E13.4060801@maciej.szmigiero.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 11, 2015 at 12:28:19AM +0100, Maciej S. Szmigiero wrote: > Add ability to remove rate constraints from generic ASoC AC'97 CODEC > driver via passed platform data, make it selectable in config. Please use subject lines matching the style for the subsystem. This is helpful for identifying relevant patches and not getting your messages deleted unread... > This way this driver can be used for platforms which don't need > specialized AC'97 CODEC drivers while at the same avoiding > code duplication from implementing equivalent functionality in > a controller driver. I'm sorry but this just doesn't explain what this patch is intended to accomplish. If we can talk to the AC'97 CODEC at all we can already identify whatever constraints it has by looking at the ID registers so it's not clear when or why a platform would need to use this. It feels like there is some underlying problem that you're trying to address.
Thanks for looking into the patch. W dniu 22.03.2015 19:27, Mark Brown pisze: > On Wed, Mar 11, 2015 at 12:28:19AM +0100, Maciej S. Szmigiero wrote: >> Add ability to remove rate constraints from generic ASoC AC'97 CODEC >> driver via passed platform data, make it selectable in config. > > Please use subject lines matching the style for the subsystem. This is > helpful for identifying relevant patches and not getting your messages > deleted unread... I assume "[PATCH] ASoC: driver: subject" format would be right? >> This way this driver can be used for platforms which don't need >> specialized AC'97 CODEC drivers while at the same avoiding >> code duplication from implementing equivalent functionality in >> a controller driver. > > I'm sorry but this just doesn't explain what this patch is intended to > accomplish. If we can talk to the AC'97 CODEC at all we can already > identify whatever constraints it has by looking at the ID registers so > it's not clear when or why a platform would need to use this. It feels > like there is some underlying problem that you're trying to address. This patch itself is supposed to allow using this CODEC driver with CODECs that support other rates that these that were already hard-coded in the driver (8000, 11025, 22050, 44100, 48000). In general sense what I want to accomplish is to add sound support for UDOO board to the mainline kernel. It uses i.MX6 SSI core as controller with VT1613 AC'97 codec. While it would be possible to simply add ac97 bus enumeration to either fsl_ssi driver or fsl-asoc-card sound card driver it looks to me that even in this case a platform device for codec would need to be registered anyway. This is because as far I see ASoC CODEC DAIs in links are identified either by OF node or name of their platform device. I've already tried to accomplish all of this via adding OF node of AC'97 codec but such patch was rejected. If the ac97 bus enumeration is added to fsl_ssi driver then there is also a problem of communicating a name of ac97 platform device to sound card driver so it can then reference it in DAI links that it builds. If such name would be hardcoded then there would be a possibility to use only one such CODEC in the system (this SoC theoretically allows up to three). And, naturally, this would result in a small code duplication with regard to this generic driver. Best regards, Maciej Szmigiero
On Sun, Mar 29, 2015 at 06:00:33PM +0200, Maciej S. Szmigiero wrote: > W dniu 22.03.2015 19:27, Mark Brown pisze: > > Please use subject lines matching the style for the subsystem. This is > > helpful for identifying relevant patches and not getting your messages > > deleted unread... > I assume "[PATCH] ASoC: driver: subject" format would be right? Yes. > > I'm sorry but this just doesn't explain what this patch is intended to > > accomplish. If we can talk to the AC'97 CODEC at all we can already > > identify whatever constraints it has by looking at the ID registers so > > it's not clear when or why a platform would need to use this. It feels > > like there is some underlying problem that you're trying to address. > This patch itself is supposed to allow using this CODEC driver with > CODECs that support other rates that these that were already hard-coded > in the driver (8000, 11025, 22050, 44100, 48000). You're talking about Linux internal implementation details here but the change you are proposing a change to the device tree? Like I think I said in reply to a previous iteration of this patch we can clearly enumerate the required information from the hardware already. > While it would be possible to simply add ac97 bus enumeration to > either fsl_ssi driver or fsl-asoc-card sound card driver it looks to me > that even in this case a platform device for codec would need to be > registered anyway. That's the way the ASoC AC'97 support currently works, yes. > If the ac97 bus enumeration is added to fsl_ssi driver then there is > also a problem of communicating a name of ac97 platform device > to sound card driver so it can then reference it in DAI links > that it builds. ... > And, naturally, this would result in a small code duplication with > regard to this generic driver. This sounds like something that can easily be put in some generic code and either used as a helper library by the drivers or just done directly from the framework when it knows it's dealing with an AC'97 bus. Alternatively if you're just trying to open up the constraints why not just open them up by default and make sure that the AC'97 generic code is constraining things appropriately if it's not doing so already?
W dniu 30.03.2015 06:19, Mark Brown pisze: > On Sun, Mar 29, 2015 at 06:00:33PM +0200, Maciej S. Szmigiero wrote: >> W dniu 22.03.2015 19:27, Mark Brown pisze: > >>> Please use subject lines matching the style for the subsystem. This is >>> helpful for identifying relevant patches and not getting your messages >>> deleted unread... > >> I assume "[PATCH] ASoC: driver: subject" format would be right? > > Yes. Thanks for information. >>> I'm sorry but this just doesn't explain what this patch is intended to >>> accomplish. If we can talk to the AC'97 CODEC at all we can already >>> identify whatever constraints it has by looking at the ID registers so >>> it's not clear when or why a platform would need to use this. It feels >>> like there is some underlying problem that you're trying to address. > >> This patch itself is supposed to allow using this CODEC driver with >> CODECs that support other rates that these that were already hard-coded >> in the driver (8000, 11025, 22050, 44100, 48000). > > You're talking about Linux internal implementation details here but the > change you are proposing a change to the device tree? Like I think I > said in reply to a previous iteration of this patch we can clearly > enumerate the required information from the hardware already. > >> While it would be possible to simply add ac97 bus enumeration to >> either fsl_ssi driver or fsl-asoc-card sound card driver it looks to me >> that even in this case a platform device for codec would need to be >> registered anyway. > > That's the way the ASoC AC'97 support currently works, yes. > >> If the ac97 bus enumeration is added to fsl_ssi driver then there is >> also a problem of communicating a name of ac97 platform device >> to sound card driver so it can then reference it in DAI links >> that it builds. > > ... > >> And, naturally, this would result in a small code duplication with >> regard to this generic driver. > > This sounds like something that can easily be put in some generic code > and either used as a helper library by the drivers or just done directly > from the framework when it knows it's dealing with an AC'97 bus. > > Alternatively if you're just trying to open up the constraints why not > just open them up by default and make sure that the AC'97 generic code > is constraining things appropriately if it's not doing so already? By 'open them up by default' do you mean removing them altogether from ASOC AC'97 generic CODEC to let the AC'97 bus driver constrain rates on its own or do you mean retaining ability to constraint in ASOC AC'97 driver, just have it switched off by default? (that is, what the current patch did, just change the default setting). If the rate constraints are removed (or disabled) from the ASOC AC'97 generic CODEC driver then it can be used as-is in the (at least) fsl_ssi driver by registering AC'97 platform device there using index taken from DT property (cell-index) already defined as required in Documentation/devicetree/bindings/sound/fsl,ssi.txt. This way the AC'97 CODECs can be uniquely identified in case there is more than one in the system. Sadly, despite this property being marked as required in documentation no current DT file seems to define it. Now the question is what is more binding: the documentation which defines this property as required or DT files which don't define it at all? Best regards, Maciej Szmigiero
On Sun, Apr 05, 2015 at 12:16:40AM +0200, Maciej S. Szmigiero wrote: > W dniu 30.03.2015 06:19, Mark Brown pisze: > > On Sun, Mar 29, 2015 at 06:00:33PM +0200, Maciej S. Szmigiero wrote: > > Alternatively if you're just trying to open up the constraints why not > > just open them up by default and make sure that the AC'97 generic code > > is constraining things appropriately if it's not doing so already? > By 'open them up by default' do you mean removing them altogether > from ASOC AC'97 generic CODEC to let the AC'97 bus driver constrain > rates on its own or do you mean retaining ability to constraint in > ASOC AC'97 driver, just have it switched off by default? I mean make the default setting be the maximum possible set of rates and then allow the CODEC code to constrain based on what it finds on the link. Please also note that ASoC is spelt ASoC. > (that is, what the current patch did, just change the default setting). No, it added a device tree property for controlling things. > If the rate constraints are removed (or disabled) from the ASOC AC'97 > generic CODEC driver then it can be used as-is in the (at least) fsl_ssi > driver by registering AC'97 platform device there using index taken from > DT property (cell-index) already defined as required in > Documentation/devicetree/bindings/sound/fsl,ssi.txt. > Now the question is what is more binding: the documentation which > defines this property as required or DT files which don't define it at all? The driver really ought to continue to support existing DTs, it can complain about them and assume the default value if the property really is required for some reason though.
W dniu 06.04.2015 18:41, Mark Brown pisze: > On Sun, Apr 05, 2015 at 12:16:40AM +0200, Maciej S. Szmigiero wrote: >> W dniu 30.03.2015 06:19, Mark Brown pisze: >>> On Sun, Mar 29, 2015 at 06:00:33PM +0200, Maciej S. Szmigiero wrote: > >>> Alternatively if you're just trying to open up the constraints why not >>> just open them up by default and make sure that the AC'97 generic code >>> is constraining things appropriately if it's not doing so already? > >> By 'open them up by default' do you mean removing them altogether >> from ASOC AC'97 generic CODEC to let the AC'97 bus driver constrain >> rates on its own or do you mean retaining ability to constraint in >> ASOC AC'97 driver, just have it switched off by default? > > I mean make the default setting be the maximum possible set of rates and > then allow the CODEC code to constrain based on what it finds on the > link. > > Please also note that ASoC is spelt ASoC. > >> (that is, what the current patch did, just change the default setting). > > No, it added a device tree property for controlling things. I meant this patch (about which the Subject line is), which controlled rate constraints based on passed platform data, not the first one which added DT support to the generic ASoC AC'97 CODEC driver. >> If the rate constraints are removed (or disabled) from the ASOC AC'97 >> generic CODEC driver then it can be used as-is in the (at least) fsl_ssi >> driver by registering AC'97 platform device there using index taken from >> DT property (cell-index) already defined as required in >> Documentation/devicetree/bindings/sound/fsl,ssi.txt. > >> Now the question is what is more binding: the documentation which >> defines this property as required or DT files which don't define it at all? > > The driver really ought to continue to support existing DTs, it can > complain about them and assume the default value if the property really > is required for some reason though. Thanks for explanation. In this case this property would be only de facto required in DT of board that I am adding sound to (and in future boards that are also using this controller - CODEC arrangement), so there won't be any compatibility problems with existing board DT files. Best regards, Maciej Szmigiero
diff --git a/include/sound/soc-ac97.h b/include/sound/soc-ac97.h new file mode 100644 index 0000000..ceb4e2f --- /dev/null +++ b/include/sound/soc-ac97.h @@ -0,0 +1,17 @@ +/* + * Platform data for generic ASoC AC97 CODEC driver. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ +#ifndef __LINUX_SND__SOC_AC97_H +#define __LINUX_SND__SOC_AC97_H + +struct snd_soc_ac97_codec_platform_data { + bool playback_rate_constrained; + bool capture_rate_constrained; +}; + +#endif /* __LINUX_SND__SOC_AC97_H */ diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 0bddd92..92d6d39 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -16,7 +16,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_88PM860X if MFD_88PM860X select SND_SOC_L3 select SND_SOC_AB8500_CODEC if ABX500_CORE - select SND_SOC_AC97_CODEC if SND_SOC_AC97_BUS + select SND_SOC_AC97_CODEC select SND_SOC_AD1836 if SPI_MASTER select SND_SOC_AD193X_SPI if SPI_MASTER select SND_SOC_AD193X_I2C if I2C @@ -210,8 +210,9 @@ config SND_SOC_AB8500_CODEC tristate config SND_SOC_AC97_CODEC - tristate + tristate "Build generic ASoC AC97 CODEC driver" select SND_AC97_CODEC + select SND_SOC_AC97_BUS config SND_SOC_AD1836 tristate diff --git a/sound/soc/codecs/ac97.c b/sound/soc/codecs/ac97.c index d0ac723..c3fb845 100644 --- a/sound/soc/codecs/ac97.c +++ b/sound/soc/codecs/ac97.c @@ -20,9 +20,16 @@ #include <sound/core.h> #include <sound/pcm.h> #include <sound/ac97_codec.h> +#include <sound/soc-ac97.h> #include <sound/initval.h> #include <sound/soc.h> +struct ac97_private { + struct snd_ac97 *ac97; + bool playback_rate_constrained; + bool capture_rate_constrained; +}; + static const struct snd_soc_dapm_widget ac97_widgets[] = { SND_SOC_DAPM_INPUT("RX"), SND_SOC_DAPM_OUTPUT("TX"), @@ -33,22 +40,53 @@ static const struct snd_soc_dapm_route ac97_routes[] = { { "TX", NULL, "AC97 Playback" }, }; +static const unsigned int default_rates[] = { + 8000, 11025, 22050, 44100, 48000 +}; + +static const struct snd_pcm_hw_constraint_list default_rate_constraints = { + .count = ARRAY_SIZE(default_rates), + .list = default_rates, +}; + +static int ac97_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_codec *codec = dai->codec; + struct ac97_private *ac97 = snd_soc_codec_get_drvdata(codec); + bool constrained; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + constrained = ac97->playback_rate_constrained; + else + constrained = ac97->capture_rate_constrained; + + if (!constrained) + return 0; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + snd_pcm_hw_constraint_list(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, &default_rate_constraints); + else + snd_pcm_hw_constraint_list(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, &default_rate_constraints); + + return 0; +} + static int ac97_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_codec *codec = dai->codec; - struct snd_ac97 *ac97 = snd_soc_codec_get_drvdata(codec); + struct ac97_private *ac97 = snd_soc_codec_get_drvdata(codec); int reg = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ? AC97_PCM_FRONT_DAC_RATE : AC97_PCM_LR_ADC_RATE; - return snd_ac97_set_rate(ac97, reg, substream->runtime->rate); + return snd_ac97_set_rate(ac97->ac97, reg, substream->runtime->rate); } -#define STD_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\ - SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_44100 |\ - SNDRV_PCM_RATE_48000) - static const struct snd_soc_dai_ops ac97_dai_ops = { + .startup = ac97_startup, .prepare = ac97_prepare, }; @@ -58,20 +96,21 @@ static struct snd_soc_dai_driver ac97_dai = { .stream_name = "AC97 Playback", .channels_min = 1, .channels_max = 2, - .rates = STD_AC97_RATES, + .rates = SNDRV_PCM_RATE_KNOT, .formats = SND_SOC_STD_AC97_FMTS,}, .capture = { .stream_name = "AC97 Capture", .channels_min = 1, .channels_max = 2, - .rates = STD_AC97_RATES, + .rates = SNDRV_PCM_RATE_KNOT, .formats = SND_SOC_STD_AC97_FMTS,}, .ops = &ac97_dai_ops, }; static int ac97_soc_probe(struct snd_soc_codec *codec) { - struct snd_ac97 *ac97; + struct ac97_private *ac97 = snd_soc_codec_get_drvdata(codec); + struct snd_ac97_bus *ac97_bus; struct snd_ac97_template ac97_template; int ret; @@ -83,31 +122,28 @@ static int ac97_soc_probe(struct snd_soc_codec *codec) return ret; memset(&ac97_template, 0, sizeof(struct snd_ac97_template)); - ret = snd_ac97_mixer(ac97_bus, &ac97_template, &ac97); + ret = snd_ac97_mixer(ac97_bus, &ac97_template, &ac97->ac97); if (ret < 0) return ret; - snd_soc_codec_set_drvdata(codec, ac97); - return 0; } #ifdef CONFIG_PM static int ac97_soc_suspend(struct snd_soc_codec *codec) { - struct snd_ac97 *ac97 = snd_soc_codec_get_drvdata(codec); + struct ac97_private *ac97 = snd_soc_codec_get_drvdata(codec); - snd_ac97_suspend(ac97); + snd_ac97_suspend(ac97->ac97); return 0; } static int ac97_soc_resume(struct snd_soc_codec *codec) { + struct ac97_private *ac97 = snd_soc_codec_get_drvdata(codec); - struct snd_ac97 *ac97 = snd_soc_codec_get_drvdata(codec); - - snd_ac97_resume(ac97); + snd_ac97_resume(ac97->ac97); return 0; } @@ -129,6 +165,27 @@ static struct snd_soc_codec_driver soc_codec_dev_ac97 = { static int ac97_probe(struct platform_device *pdev) { + struct ac97_private *ac97 = + devm_kzalloc(&pdev->dev, sizeof(struct ac97_private), + GFP_KERNEL); + struct snd_soc_ac97_codec_platform_data *pdata = + pdev->dev.platform_data; + + if (!ac97) + return -ENOMEM; + + if (pdata) { + ac97->playback_rate_constrained = + pdata->playback_rate_constrained; + ac97->capture_rate_constrained = + pdata->capture_rate_constrained; + } else { + ac97->playback_rate_constrained = 1; + ac97->capture_rate_constrained = 1; + } + + platform_set_drvdata(pdev, ac97); + return snd_soc_register_codec(&pdev->dev, &soc_codec_dev_ac97, &ac97_dai, 1); }
Add ability to remove rate constraints from generic ASoC AC'97 CODEC driver via passed platform data, make it selectable in config. This way this driver can be used for platforms which don't need specialized AC'97 CODEC drivers while at the same avoiding code duplication from implementing equivalent functionality in a controller driver. There is no change in behavior when no platform data is passed. Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>