diff mbox

[ASoC] Add DT bindings for generic ASoC AC97 CODEC driver

Message ID 54F9F81E.8080105@maciej.szmigiero.name
State Needs Review / ACK, archived
Headers show

Checks

Context Check Description
robh/checkpatch warning total: 1 errors, 0 warnings, 0 lines checked
robh/patch-applied success

Commit Message

Maciej S. Szmigiero March 6, 2015, 6:55 p.m. UTC 
Add and document DT bindings for generic ASoC AC97 CODEC driver,
make it selectable in config.

Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark Brown March 7, 2015, 10:52 a.m. UTC | #1 
On Fri, Mar 06, 2015 at 07:55:26PM +0100, Maciej S. Szmigiero wrote:

> Add and document DT bindings for generic ASoC AC97 CODEC driver,
> make it selectable in config.

AC'97 shouldn't need DT bindings for the CODEC, it's an enumerable bus.

> +  - playback-rates : A list of supported playback rates.
> +
> +  - capture-rates : A list of supported capture rates.

Why would we need these properties - the sample rate support is defined
by the standard, and any extensions can be enumerated from the device.
Maciej S. Szmigiero March 7, 2015, 1:58 p.m. UTC | #2 
Thanks for looking into the patch.

W dniu 07.03.2015 11:52, Mark Brown pisze:
> On Fri, Mar 06, 2015 at 07:55:26PM +0100, Maciej S. Szmigiero wrote:
> 
>> Add and document DT bindings for generic ASoC AC97 CODEC driver,
>> make it selectable in config.
> 
> AC'97 shouldn't need DT bindings for the CODEC, it's an enumerable bus.

What this driver did / does is essentially to allow attaching AC'97 bus
to ASoC AC'97 controllers which don't do it on its own (from what I can
see only board files do it).

> 
>> +  - playback-rates : A list of supported playback rates.
>> +
>> +  - capture-rates : A list of supported capture rates.
> 
> Why would we need these properties - the sample rate support is defined
> by the standard, and any extensions can be enumerated from the device.

This driver originally constrained rates to 8000, 11025, 22050, 44100,
48000.

An alternative would be to remove this constraint at all from this
driver and leave such constraining for controller and AC'97 bus code.

But since this driver was originally limited to these rates I think it
would be safer to do it only when instantiated via OF.

Best regards,
Maciej Szmigiero

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown March 7, 2015, 2:34 p.m. UTC | #3 
On Sat, Mar 07, 2015 at 02:58:53PM +0100, Maciej S. Szmigiero wrote:
> W dniu 07.03.2015 11:52, Mark Brown pisze:
> > On Fri, Mar 06, 2015 at 07:55:26PM +0100, Maciej S. Szmigiero wrote:

> > AC'97 shouldn't need DT bindings for the CODEC, it's an enumerable bus.

> What this driver did / does is essentially to allow attaching AC'97 bus
> to ASoC AC'97 controllers which don't do it on its own (from what I can
> see only board files do it).

That's true, and it's something it's easy to get away with with board
files, but that doesn't mean it's something we should be doing for
device tree where we're not just finding an expedient way to load things
but rather defining an ABI.

> This driver originally constrained rates to 8000, 11025, 22050, 44100,
> 48000.

> An alternative would be to remove this constraint at all from this
> driver and leave such constraining for controller and AC'97 bus code.

That's a much better approach.

> But since this driver was originally limited to these rates I think it
> would be safer to do it only when instantiated via OF.

Again, DT is defining an ABI.
Maciej S. Szmigiero March 7, 2015, 3:33 p.m. UTC | #4 
W dniu 07.03.2015 15:34, Mark Brown pisze:
> On Sat, Mar 07, 2015 at 02:58:53PM +0100, Maciej S. Szmigiero wrote:
>> W dniu 07.03.2015 11:52, Mark Brown pisze:
>>> On Fri, Mar 06, 2015 at 07:55:26PM +0100, Maciej S. Szmigiero wrote:
> 
>>> AC'97 shouldn't need DT bindings for the CODEC, it's an enumerable bus.
> 
>> What this driver did / does is essentially to allow attaching AC'97 bus
>> to ASoC AC'97 controllers which don't do it on its own (from what I can
>> see only board files do it).
> 
> That's true, and it's something it's easy to get away with with board
> files, but that doesn't mean it's something we should be doing for
> device tree where we're not just finding an expedient way to load things
> but rather defining an ABI.

Do you recommend then putting AC'97 bus/mixer attach in
simple-card/fsl-asoc-card or controller driver instead?

>> This driver originally constrained rates to 8000, 11025, 22050, 44100,
>> 48000.
> 
>> An alternative would be to remove this constraint at all from this
>> driver and leave such constraining for controller and AC'97 bus code.
> 
> That's a much better approach.
> 
>> But since this driver was originally limited to these rates I think it
>> would be safer to do it only when instantiated via OF.
> 
> Again, DT is defining an ABI.

Best regards,
Maciej Szmigiero

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown March 7, 2015, 4:31 p.m. UTC | #5 
On Sat, Mar 07, 2015 at 04:33:12PM +0100, Maciej S. Szmigiero wrote:
> W dniu 07.03.2015 15:34, Mark Brown pisze:

> > That's true, and it's something it's easy to get away with with board
> > files, but that doesn't mean it's something we should be doing for
> > device tree where we're not just finding an expedient way to load things
> > but rather defining an ABI.

> Do you recommend then putting AC'97 bus/mixer attach in
> simple-card/fsl-asoc-card or controller driver instead?

It's tricky.  For the vast majority of AC'97 systems we probably want to
just do things from the bus controller but there are some funky AC'97
embedded systems out there which need quirks to varying degrees, and in
some cases the ASoC AC'97 drivers do a much better job on things like
power.  For those we'd need to do something different to just doing a
standard AC'97 instantiation but what exactly we do in a DT world (quirk
on machine compatible?) isn't clear.

If the platform you're working on isn't one that commonly got fancy
AC'97 subsystems then it's possibly best to just do the common case and
only worry about what to do about the complex case if anyone has a need.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/sound/ac97-generic-codec.txt b/Documentation/devicetree/bindings/sound/ac97-generic-codec.txt
new file mode 100644
index 0000000..ce98698
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ac97-generic-codec.txt
@@ -0,0 +1,24 @@ 
+Generic ASoC AC97 CODEC driver
+
+Allows using codecs supported by generic ALSA AC97 code as codecs in ASoC.
+
+Required properties:
+
+  - compatible : "linux,ac97-codec"
+
+Optional properties:
+
+  - playback-rates : A list of supported playback rates.
+
+  - capture-rates : A list of supported capture rates.
+
+In case one or both of above properties are missing the relevant rate(s) are assumed
+to be 8000, 11025, 22050, 44100, 48000.
+
+Example:
+
+ac97codec: ac97-hifi {
+	compatible = "linux,ac97-codec";
+	playback-rates = <48000 44100 32000 24000 22050 16000 12000 11025 8000>;
+	capture-rates = <48000>;
+};
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..25125bf 100644
--- a/sound/soc/codecs/ac97.c
+++ b/sound/soc/codecs/ac97.c
@@ -23,6 +23,12 @@ 
 #include <sound/initval.h>
 #include <sound/soc.h>
 
+struct ac97_private {
+	struct snd_ac97 *ac97;
+	struct snd_pcm_hw_constraint_list playback_rate_constraints;
+	struct snd_pcm_hw_constraint_list capture_rate_constraints;
+};
+
 static const struct snd_soc_dapm_widget ac97_widgets[] = {
 	SND_SOC_DAPM_INPUT("RX"),
 	SND_SOC_DAPM_OUTPUT("TX"),
@@ -33,22 +39,41 @@  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 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);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		snd_pcm_hw_constraint_list(substream->runtime, 0,
+			SNDRV_PCM_HW_PARAM_RATE,
+			&ac97->playback_rate_constraints);
+	else
+		snd_pcm_hw_constraint_list(substream->runtime, 0,
+			SNDRV_PCM_HW_PARAM_RATE,
+			&ac97->capture_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 +83,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 +109,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;
 }
@@ -127,8 +150,89 @@  static struct snd_soc_codec_driver soc_codec_dev_ac97 = {
 	.num_dapm_routes = ARRAY_SIZE(ac97_routes),
 };
 
+#ifdef CONFIG_OF
+static int ac97_of_read_rates(struct platform_device *pdev,
+			const char *name,
+			struct snd_pcm_hw_constraint_list *cons)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct property *prop;
+	int len, count, ctr;
+	unsigned int *clist;
+
+	prop = of_find_property(np, name, &len);
+	if (!prop)
+		return 0;
+
+	count = len / sizeof(u32);
+	if (count <= 0)
+		return 0;
+
+	clist = devm_kzalloc(&pdev->dev, count * sizeof(unsigned int),
+				GFP_KERNEL);
+	if (!clist)
+		return -ENOMEM;
+
+	for (ctr = 0; ctr < count; ctr++) {
+		int ret;
+		u32 val;
+
+		ret = of_property_read_u32_index(np, name, ctr, &val);
+		if (ret)
+			return ret;
+
+		clist[ctr] = val;
+	}
+
+	cons->list = clist;
+	cons->count = ctr;
+
+	return 0;
+}
+#endif
+
 static int ac97_probe(struct platform_device *pdev)
 {
+	struct ac97_private *ac97 =
+		devm_kzalloc(&pdev->dev, sizeof(struct ac97_private),
+				GFP_KERNEL);
+#ifdef CONFIG_OF
+	struct device_node *np = pdev->dev.of_node;
+#endif
+
+	if (!ac97)
+		return -ENOMEM;
+
+#ifdef CONFIG_OF
+	if (np) {
+		int ret;
+
+		ret = ac97_of_read_rates(pdev, "playback-rates",
+			&ac97->playback_rate_constraints);
+		if (ret)
+			return ret;
+
+		ret = ac97_of_read_rates(pdev, "capture-rates",
+			&ac97->capture_rate_constraints);
+		if (ret)
+			return ret;
+	}
+#endif
+
+	if (!ac97->playback_rate_constraints.list) {
+		ac97->playback_rate_constraints.list = default_rates;
+		ac97->playback_rate_constraints.count =
+			ARRAY_SIZE(default_rates);
+	}
+
+	if (!ac97->capture_rate_constraints.list) {
+		ac97->capture_rate_constraints.list = default_rates;
+		ac97->capture_rate_constraints.count =
+			ARRAY_SIZE(default_rates);
+	}
+
+	platform_set_drvdata(pdev, ac97);
+
 	return snd_soc_register_codec(&pdev->dev,
 			&soc_codec_dev_ac97, &ac97_dai, 1);
 }
@@ -139,9 +243,16 @@  static int ac97_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id ac97_codec_dt_ids[] = {
+	{ .compatible = "linux,ac97-codec", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ac97_codec_dt_ids);
+
 static struct platform_driver ac97_codec_driver = {
 	.driver = {
 		.name = "ac97-codec",
+		.of_match_table = of_match_ptr(ac97_codec_dt_ids),
 	},
 
 	.probe = ac97_probe,