[alsa-devel] [PATCH 1/2] ASoC: add generic simple-amplifier support
Many boards have speaker amplifier attached on an analog output. Most of them are only managed by a mute or shutdown GPIO. In order to reduce duplicate driver, this patch adds generic/simple-amplifier.
Signed-off-by: Nicolò Veronese nicveronese@gmail.com --- sound/soc/generic/Kconfig | 5 ++ sound/soc/generic/Makefile | 2 + sound/soc/generic/simple-amplifier.c | 109 +++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 sound/soc/generic/simple-amplifier.c
diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig index c954be0..ace17c3a 100644 --- a/sound/soc/generic/Kconfig +++ b/sound/soc/generic/Kconfig @@ -7,6 +7,11 @@ config SND_SIMPLE_CARD help This option enables generic simple sound card support
+config SND_SIMPLE_AMPLIFIER + tristate "ASoC Simple Amplifier support" + help + This option enables generic simple amplifier support + config SND_SIMPLE_SCU_CARD tristate "ASoC Simple SCU sound card support" depends on OF diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile index 9dec293..805a746 100644 --- a/sound/soc/generic/Makefile +++ b/sound/soc/generic/Makefile @@ -1,12 +1,14 @@ # SPDX-License-Identifier: GPL-2.0 snd-soc-simple-card-utils-objs := simple-card-utils.o snd-soc-simple-card-objs := simple-card.o +snd-soc-simple-amplifier-objs := simple-amplifier.o snd-soc-simple-scu-card-objs := simple-scu-card.o snd-soc-audio-graph-card-objs := audio-graph-card.o snd-soc-audio-graph-scu-card-objs := audio-graph-scu-card.o
obj-$(CONFIG_SND_SIMPLE_CARD_UTILS) += snd-soc-simple-card-utils.o obj-$(CONFIG_SND_SIMPLE_CARD) += snd-soc-simple-card.o +obj-$(CONFIG_SND_SIMPLE_AMPLIFIER) += snd-soc-simple-amplifier.o obj-$(CONFIG_SND_SIMPLE_SCU_CARD) += snd-soc-simple-scu-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_CARD) += snd-soc-audio-graph-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_SCU_CARD) += snd-soc-audio-graph-scu-card.o diff --git a/sound/soc/generic/simple-amplifier.c b/sound/soc/generic/simple-amplifier.c new file mode 100644 index 0000000..e135a43 --- /dev/null +++ b/sound/soc/generic/simple-amplifier.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +// ALSA SoC simple amplifier driver +// +// Copyright 2018 +// Author: Nicolò Veronese nicveronese@gmail.com + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h> + +#include <sound/soc.h> +#include <sound/soc-dapm.h> + +/* This struct is used to save the context */ +struct simple_amp_data { + struct device *dev; + struct regmap *regmap; + struct gpio_desc *power_gpio; +}; + +static int simple_amp_power_event(struct snd_soc_dapm_widget *widget, + struct snd_kcontrol *kctrl, int event) +{ + struct snd_soc_component *c = snd_soc_dapm_to_component(widget->dapm); + struct simple_amp_data *data = snd_soc_component_get_drvdata(c); + + /* Shutdown GPIO */ + if (data->power_gpio) + gpiod_set_value_cansleep(data->power_gpio, SND_SOC_DAPM_EVENT_ON(event)); + + return 0; +} + +static const struct snd_soc_dapm_widget simple_amp_dapm_widgets[] = { + SND_SOC_DAPM_INPUT("LEFT AMP IN"), + SND_SOC_DAPM_INPUT("RIGHT AMP IN"), + + SND_SOC_DAPM_OUTPUT("SPK LEFT"), + SND_SOC_DAPM_OUTPUT("SPK RIGHT"), + + SND_SOC_DAPM_REGULATOR_SUPPLY("VCC", 20, 0), + SND_SOC_DAPM_SPK("MUTE", simple_amp_power_event), +}; + +static const struct snd_soc_dapm_route simple_amp_dapm_routes[] = { + { "MUTE", NULL, "LEFT AMP IN" }, + { "MUTE", NULL, "RIGHT AMP IN" }, + + { "SPK LEFT", NULL, "VCC" }, + { "SPK RIGHT", NULL, "VCC" }, + + { "SPK LEFT", NULL, "MUTE" }, + { "SPK RIGHT", NULL, "MUTE" }, +}; + +static const struct snd_soc_component_driver simple_amp_component_driver = { + .name = "simple-amplifier", + .dapm_widgets = simple_amp_dapm_widgets, + .num_dapm_widgets = ARRAY_SIZE(simple_amp_dapm_widgets), + .dapm_routes = simple_amp_dapm_routes, + .num_dapm_routes = ARRAY_SIZE(simple_amp_dapm_routes), +}; + +static int asoc_simple_amp_probe(struct platform_device *pdev) +{ + struct simple_amp_data *data; + struct device *dev = &pdev->dev; + int ret; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + data->dev = &pdev->dev; + platform_set_drvdata(pdev, data); + + if(of_property_read_bool(pdev->dev.of_node,"shutdown-gpios")) { + data->power_gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW); + + if (IS_ERR(data->power_gpio)) { + ret = PTR_ERR(data->power_gpio); + dev_err(&pdev->dev, "Failed to get shutdown gpio: %d\n", ret); + return ret; + } + } + + return devm_snd_soc_register_component(dev, + &simple_amp_component_driver, NULL, 0); +} + +static const struct of_device_id asoc_simple_of_match[] = { + { .compatible = "simple-audio-amplifier", }, + {}, +}; +MODULE_DEVICE_TABLE(of, asoc_simple_of_match); + +static struct platform_driver asoc_simple_amp = { + .driver = { + .name = "asoc-simple-amplifier", + .of_match_table = of_match_ptr(asoc_simple_of_match), + }, + .probe = asoc_simple_amp_probe, +}; + +module_platform_driver(asoc_simple_amp); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ASoC Simple Amplifier"); +MODULE_AUTHOR("Nicolò Veronese nicveronese@gmail.com");
Signed-off-by: Nicolò Veronese nicveronese@gmail.com --- .../devicetree/bindings/sound/simple-amplifier.txt | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/simple-amplifier.txt
diff --git a/Documentation/devicetree/bindings/sound/simple-amplifier.txt b/Documentation/devicetree/bindings/sound/simple-amplifier.txt new file mode 100644 index 0000000..1c2b57c --- /dev/null +++ b/Documentation/devicetree/bindings/sound/simple-amplifier.txt @@ -0,0 +1,36 @@ +Simple-Amplifier: + +Simple-Amplifier specifies a simple aux device for your codec. +Most aplifiers are managed by a single GPIO. The main function +of this driver is to toggle the MUTE/SHUTDOWN GPIO. + +Required properties: + +- compatible : "simple-audio-amplifier" + +- VCC-supply : power supply for the device, as covered +in Documentation/devicetree/bindings/regulator/regulator.txt + +Optional properties: + +- shutdown-gpios : The gpio to be toggled on and off +to shutdown the amplifier when not in use. as covered +in Documentation/gpio/board.txt + +Example: + +sound { + compatible = "simple-audio-card"; + ... + simple-audio-card,routing = + "LEFT AMP IN", "LINE_OUT_LEFT", + "RIGHT AMP IN", "LINE_OUT_RIGHT"; + simple-audio-card,aux-devs = <&>; + ... +}; + +amp: amplifier { + compatible = "simple-audio-amplifier"; + VCC-supply = <®ulator> + shutdown-gpios = <&r_pio 2 GPIO_ACTIVE_LOW>; +};
On Mon, Jun 25, 2018 at 01:06:16PM +0200, Nicolò Veronese wrote:
Many boards have speaker amplifier attached on an analog output. Most of them are only managed by a mute or shutdown GPIO. In order to reduce duplicate driver, this patch adds generic/simple-amplifier.
Copying in Hans who has a machine that needs something like this, not deleting context for his benefit.
This looks good to me, one coding style thing below but otherwise the main thing I'm thinking is that we might want to add a mono channel as well for simplicity when wiring up a system using a mono amplifier. That could always be done later though.
Signed-off-by: Nicolò Veronese nicveronese@gmail.com
sound/soc/generic/Kconfig | 5 ++ sound/soc/generic/Makefile | 2 + sound/soc/generic/simple-amplifier.c | 109 +++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 sound/soc/generic/simple-amplifier.c
diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig index c954be0..ace17c3a 100644 --- a/sound/soc/generic/Kconfig +++ b/sound/soc/generic/Kconfig @@ -7,6 +7,11 @@ config SND_SIMPLE_CARD help This option enables generic simple sound card support
+config SND_SIMPLE_AMPLIFIER
- tristate "ASoC Simple Amplifier support"
- help
This option enables generic simple amplifier supportconfig SND_SIMPLE_SCU_CARD tristate "ASoC Simple SCU sound card support" depends on OF diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile index 9dec293..805a746 100644 --- a/sound/soc/generic/Makefile +++ b/sound/soc/generic/Makefile @@ -1,12 +1,14 @@ # SPDX-License-Identifier: GPL-2.0 snd-soc-simple-card-utils-objs := simple-card-utils.o snd-soc-simple-card-objs := simple-card.o +snd-soc-simple-amplifier-objs := simple-amplifier.o snd-soc-simple-scu-card-objs := simple-scu-card.o snd-soc-audio-graph-card-objs := audio-graph-card.o snd-soc-audio-graph-scu-card-objs := audio-graph-scu-card.o
obj-$(CONFIG_SND_SIMPLE_CARD_UTILS) += snd-soc-simple-card-utils.o obj-$(CONFIG_SND_SIMPLE_CARD) += snd-soc-simple-card.o +obj-$(CONFIG_SND_SIMPLE_AMPLIFIER) += snd-soc-simple-amplifier.o obj-$(CONFIG_SND_SIMPLE_SCU_CARD) += snd-soc-simple-scu-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_CARD) += snd-soc-audio-graph-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_SCU_CARD) += snd-soc-audio-graph-scu-card.o diff --git a/sound/soc/generic/simple-amplifier.c b/sound/soc/generic/simple-amplifier.c new file mode 100644 index 0000000..e135a43 --- /dev/null +++ b/sound/soc/generic/simple-amplifier.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +// ALSA SoC simple amplifier driver +// +// Copyright 2018 +// Author: Nicolò Veronese nicveronese@gmail.com
+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>
+#include <sound/soc.h> +#include <sound/soc-dapm.h>
+/* This struct is used to save the context */ +struct simple_amp_data {
- struct device *dev;
- struct regmap *regmap;
You don't need a regmap here.
- struct gpio_desc *power_gpio;
+};
+static int simple_amp_power_event(struct snd_soc_dapm_widget *widget,
struct snd_kcontrol *kctrl, int event)+{
- struct snd_soc_component *c = snd_soc_dapm_to_component(widget->dapm);
- struct simple_amp_data *data = snd_soc_component_get_drvdata(c);
- /* Shutdown GPIO */
- if (data->power_gpio)
gpiod_set_value_cansleep(data->power_gpio, SND_SOC_DAPM_EVENT_ON(event));- return 0;
+}
+static const struct snd_soc_dapm_widget simple_amp_dapm_widgets[] = {
- SND_SOC_DAPM_INPUT("LEFT AMP IN"),
- SND_SOC_DAPM_INPUT("RIGHT AMP IN"),
- SND_SOC_DAPM_OUTPUT("SPK LEFT"),
- SND_SOC_DAPM_OUTPUT("SPK RIGHT"),
- SND_SOC_DAPM_REGULATOR_SUPPLY("VCC", 20, 0),
- SND_SOC_DAPM_SPK("MUTE", simple_amp_power_event),
+};
+static const struct snd_soc_dapm_route simple_amp_dapm_routes[] = {
- { "MUTE", NULL, "LEFT AMP IN" },
- { "MUTE", NULL, "RIGHT AMP IN" },
- { "SPK LEFT", NULL, "VCC" },
- { "SPK RIGHT", NULL, "VCC" },
- { "SPK LEFT", NULL, "MUTE" },
- { "SPK RIGHT", NULL, "MUTE" },
+};
+static const struct snd_soc_component_driver simple_amp_component_driver = {
- .name = "simple-amplifier",
- .dapm_widgets = simple_amp_dapm_widgets,
- .num_dapm_widgets = ARRAY_SIZE(simple_amp_dapm_widgets),
- .dapm_routes = simple_amp_dapm_routes,
- .num_dapm_routes = ARRAY_SIZE(simple_amp_dapm_routes),
+};
+static int asoc_simple_amp_probe(struct platform_device *pdev) +{
- struct simple_amp_data *data;
- struct device *dev = &pdev->dev;
- int ret;
- data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
- if (!data)
return -ENOMEM;- data->dev = &pdev->dev;
- platform_set_drvdata(pdev, data);
- if(of_property_read_bool(pdev->dev.of_node,"shutdown-gpios")) {
Coding style: should be if (
data->power_gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW);if (IS_ERR(data->power_gpio)) {ret = PTR_ERR(data->power_gpio);dev_err(&pdev->dev, "Failed to get shutdown gpio: %d\n", ret);return ret;}- }
- return devm_snd_soc_register_component(dev,
&simple_amp_component_driver, NULL, 0);+}
+static const struct of_device_id asoc_simple_of_match[] = {
- { .compatible = "simple-audio-amplifier", },
- {},
+}; +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+static struct platform_driver asoc_simple_amp = {
- .driver = {
.name = "asoc-simple-amplifier",.of_match_table = of_match_ptr(asoc_simple_of_match),- },
- .probe = asoc_simple_amp_probe,
+};
+module_platform_driver(asoc_simple_amp);
+MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ASoC Simple Amplifier");
+MODULE_AUTHOR("Nicolò Veronese nicveronese@gmail.com");
2.7.4
On Mon, 2018-06-25 at 12:21 +0100, Mark Brown wrote:
On Mon, Jun 25, 2018 at 01:06:16PM +0200, Nicolò Veronese wrote:
Many boards have speaker amplifier attached on an analog output. Most of them are only managed by a mute or shutdown GPIO. In order to reduce duplicate driver, this patch adds generic/simple-amplifier.
Copying in Hans who has a machine that needs something like this, not deleting context for his benefit.
This looks good to me, one coding style thing below but otherwise the main thing I'm thinking is that we might want to add a mono channel as well for simplicity when wiring up a system using a mono amplifier. That could always be done later though.
Signed-off-by: Nicolò Veronese nicveronese@gmail.com
Hi Nicolo,
Same comment as on IRC, you driver looks very similar to the one I posted a while back (sound/soc/codecs/dio2125.c)
Could you just add your compatible to the list in the existing driver ? Is there anything I missed which makes them incompatible ?
Regards Jerome
sound/soc/generic/Kconfig | 5 ++ sound/soc/generic/Makefile | 2 + sound/soc/generic/simple-amplifier.c | 109 +++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 sound/soc/generic/simple-amplifier.c
diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig index c954be0..ace17c3a 100644 --- a/sound/soc/generic/Kconfig +++ b/sound/soc/generic/Kconfig @@ -7,6 +7,11 @@ config SND_SIMPLE_CARD help This option enables generic simple sound card support
+config SND_SIMPLE_AMPLIFIER
- tristate "ASoC Simple Amplifier support"
- help
This option enables generic simple amplifier supportconfig SND_SIMPLE_SCU_CARD tristate "ASoC Simple SCU sound card support" depends on OF diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile index 9dec293..805a746 100644 --- a/sound/soc/generic/Makefile +++ b/sound/soc/generic/Makefile @@ -1,12 +1,14 @@ # SPDX-License-Identifier: GPL-2.0 snd-soc-simple-card-utils-objs := simple-card-utils.o snd-soc-simple-card-objs := simple-card.o +snd-soc-simple-amplifier-objs := simple-amplifier.o snd-soc-simple-scu-card-objs := simple-scu-card.o snd-soc-audio-graph-card-objs := audio-graph-card.o snd-soc-audio-graph-scu-card-objs := audio-graph-scu-card.o
obj-$(CONFIG_SND_SIMPLE_CARD_UTILS) += snd-soc-simple-card-utils.o obj-$(CONFIG_SND_SIMPLE_CARD) += snd-soc-simple-card.o +obj-$(CONFIG_SND_SIMPLE_AMPLIFIER) += snd-soc-simple-amplifier.o obj-$(CONFIG_SND_SIMPLE_SCU_CARD) += snd-soc-simple-scu-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_CARD) += snd-soc-audio-graph-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_SCU_CARD) += snd-soc-audio-graph-scu-card.o diff --git a/sound/soc/generic/simple-amplifier.c b/sound/soc/generic/simple-amplifier.c new file mode 100644 index 0000000..e135a43 --- /dev/null +++ b/sound/soc/generic/simple-amplifier.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +// ALSA SoC simple amplifier driver +// +// Copyright 2018 +// Author: Nicolò Veronese nicveronese@gmail.com
+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>
+#include <sound/soc.h> +#include <sound/soc-dapm.h>
+/* This struct is used to save the context */ +struct simple_amp_data {
- struct device *dev;
- struct regmap *regmap;
You don't need a regmap here.
- struct gpio_desc *power_gpio;
+};
+static int simple_amp_power_event(struct snd_soc_dapm_widget *widget,
struct snd_kcontrol *kctrl, int event)+{
- struct snd_soc_component *c = snd_soc_dapm_to_component(widget->dapm);
- struct simple_amp_data *data = snd_soc_component_get_drvdata(c);
- /* Shutdown GPIO */
- if (data->power_gpio)
gpiod_set_value_cansleep(data->power_gpio, SND_SOC_DAPM_EVENT_ON(event));- return 0;
+}
+static const struct snd_soc_dapm_widget simple_amp_dapm_widgets[] = {
- SND_SOC_DAPM_INPUT("LEFT AMP IN"),
- SND_SOC_DAPM_INPUT("RIGHT AMP IN"),
- SND_SOC_DAPM_OUTPUT("SPK LEFT"),
- SND_SOC_DAPM_OUTPUT("SPK RIGHT"),
- SND_SOC_DAPM_REGULATOR_SUPPLY("VCC", 20, 0),
- SND_SOC_DAPM_SPK("MUTE", simple_amp_power_event),
+};
+static const struct snd_soc_dapm_route simple_amp_dapm_routes[] = {
- { "MUTE", NULL, "LEFT AMP IN" },
- { "MUTE", NULL, "RIGHT AMP IN" },
- { "SPK LEFT", NULL, "VCC" },
- { "SPK RIGHT", NULL, "VCC" },
- { "SPK LEFT", NULL, "MUTE" },
- { "SPK RIGHT", NULL, "MUTE" },
+};
+static const struct snd_soc_component_driver simple_amp_component_driver = {
- .name = "simple-amplifier",
- .dapm_widgets = simple_amp_dapm_widgets,
- .num_dapm_widgets = ARRAY_SIZE(simple_amp_dapm_widgets),
- .dapm_routes = simple_amp_dapm_routes,
- .num_dapm_routes = ARRAY_SIZE(simple_amp_dapm_routes),
+};
+static int asoc_simple_amp_probe(struct platform_device *pdev) +{
- struct simple_amp_data *data;
- struct device *dev = &pdev->dev;
- int ret;
- data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
- if (!data)
return -ENOMEM;- data->dev = &pdev->dev;
- platform_set_drvdata(pdev, data);
- if(of_property_read_bool(pdev->dev.of_node,"shutdown-gpios")) {
Coding style: should be if (
data->power_gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW);if (IS_ERR(data->power_gpio)) {ret = PTR_ERR(data->power_gpio);dev_err(&pdev->dev, "Failed to get shutdown gpio: %d\n", ret);return ret;}- }
- return devm_snd_soc_register_component(dev,
&simple_amp_component_driver, NULL, 0);+}
+static const struct of_device_id asoc_simple_of_match[] = {
- { .compatible = "simple-audio-amplifier", },
- {},
+}; +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+static struct platform_driver asoc_simple_amp = {
- .driver = {
.name = "asoc-simple-amplifier",.of_match_table = of_match_ptr(asoc_simple_of_match),- },
- .probe = asoc_simple_amp_probe,
+};
+module_platform_driver(asoc_simple_amp);
+MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ASoC Simple Amplifier");
+MODULE_AUTHOR("Nicolò Veronese nicveronese@gmail.com");
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, Jun 25, 2018 at 01:31:29PM +0200, Jerome Brunet wrote:
Same comment as on IRC, you driver looks very similar to the one I posted a while back (sound/soc/codecs/dio2125.c)
Could you just add your compatible to the list in the existing driver ? Is there anything I missed which makes them incompatible ?
It's probably a good idea to rename that driver so it's a bit more discoverable - you're absolutely right that it's very similar and doing basically the same thing but I'd completely forgotten that it existed :/
On Mon, 2018-06-25 at 12:44 +0100, Mark Brown wrote:
On Mon, Jun 25, 2018 at 01:31:29PM +0200, Jerome Brunet wrote:
Same comment as on IRC, you driver looks very similar to the one I posted a while back (sound/soc/codecs/dio2125.c) Could you just add your compatible to the list in the existing driver ? Is there anything I missed which makes them incompatible ?
It's probably a good idea to rename that driver so it's a bit more discoverable - you're absolutely right that it's very similar and doing basically the same thing but I'd completely forgotten that it existed :/
Agreed , the name is poorly chosen. I did not thought it was a generic need at the time.
Nicolò, do you want to do the remame ? or would you prefer me to do it ?
Cheers Jerome
If you can do it, it's better. I'm still learning, take care that the "simple-amplifier" can support supplys.
Regards, Nicolò
Il giorno lun 25 giu 2018 alle ore 13:51 Jerome Brunet jbrunet@baylibre.com ha scritto:
On Mon, 2018-06-25 at 12:44 +0100, Mark Brown wrote:
On Mon, Jun 25, 2018 at 01:31:29PM +0200, Jerome Brunet wrote:
Same comment as on IRC, you driver looks very similar to the one I
posted a
while back (sound/soc/codecs/dio2125.c) Could you just add your compatible to the list in the existing driver ? Is there anything I missed which makes them incompatible ?
It's probably a good idea to rename that driver so it's a bit more discoverable - you're absolutely right that it's very similar and doing basically the same thing but I'd completely forgotten that it existed :/
Agreed , the name is poorly chosen. I did not thought it was a generic need at the time.
Nicolò, do you want to do the remame ? or would you prefer me to do it ?
Cheers Jerome
On Mon, 2018-06-25 at 14:07 +0200, Nicolò Veronese wrote:
If you can do it, it's better. I'm still learning, take care that the "simple-amplifier" can support supplys.
supplies support makes sense.
If I may suggest the following: * 1) Just perform a simple rename/sed first (Don't forget the DT documentation) * 2) Add your compatible to the list * 3) Add optional supply support if you need it
Cheers Jerome
Regards, Nicolò
Il giorno lun 25 giu 2018 alle ore 13:51 Jerome Brunet jbrunet@baylibre.com ha scritto:
On Mon, 2018-06-25 at 12:44 +0100, Mark Brown wrote:
On Mon, Jun 25, 2018 at 01:31:29PM +0200, Jerome Brunet wrote:
Same comment as on IRC, you driver looks very similar to the one I posted a while back (sound/soc/codecs/dio2125.c) Could you just add your compatible to the list in the existing driver ? Is there anything I missed which makes them incompatible ?
It's probably a good idea to rename that driver so it's a bit more discoverable - you're absolutely right that it's very similar and doing basically the same thing but I'd completely forgotten that it existed :/
Agreed , the name is poorly chosen. I did not thought it was a generic need at the time.
Nicolò, do you want to do the remame ? or would you prefer me to do it ?
Cheers Jerome
On Mon, Jun 25, 2018 at 7:31 PM, Jerome Brunet jbrunet@baylibre.com wrote:
On Mon, 2018-06-25 at 12:21 +0100, Mark Brown wrote:
On Mon, Jun 25, 2018 at 01:06:16PM +0200, Nicolò Veronese wrote:
Many boards have speaker amplifier attached on an analog output. Most of them are only managed by a mute or shutdown GPIO. In order to reduce duplicate driver, this patch adds generic/simple-amplifier.
Copying in Hans who has a machine that needs something like this, not deleting context for his benefit.
This looks good to me, one coding style thing below but otherwise the main thing I'm thinking is that we might want to add a mono channel as well for simplicity when wiring up a system using a mono amplifier. That could always be done later though.
Signed-off-by: Nicolò Veronese nicveronese@gmail.com
Hi Nicolo,
Same comment as on IRC, you driver looks very similar to the one I posted a while back (sound/soc/codecs/dio2125.c)
Could you just add your compatible to the list in the existing driver ? Is there anything I missed which makes them incompatible ?
I think slightly different semantics apply for "shutdown" vs "mute" pins / controls. "shutdown" implies a low power, if not powered off altogether, mode which might require a slight delay to charge up when the amp is enabled. "mute" is just that, disconnecting the output, which would be useful in masking pops or other noises, but does nothing to help power consumption.
Not sure if the subtle differences can be handled in DAPM, but it would be nice to have. I've seen devices that have both control pins.
Regards ChenYu
Regards Jerome
sound/soc/generic/Kconfig | 5 ++ sound/soc/generic/Makefile | 2 + sound/soc/generic/simple-amplifier.c | 109 +++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 sound/soc/generic/simple-amplifier.c
diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig index c954be0..ace17c3a 100644 --- a/sound/soc/generic/Kconfig +++ b/sound/soc/generic/Kconfig @@ -7,6 +7,11 @@ config SND_SIMPLE_CARD help This option enables generic simple sound card support
+config SND_SIMPLE_AMPLIFIER
- tristate "ASoC Simple Amplifier support"
- help
This option enables generic simple amplifier supportconfig SND_SIMPLE_SCU_CARD tristate "ASoC Simple SCU sound card support" depends on OF diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile index 9dec293..805a746 100644 --- a/sound/soc/generic/Makefile +++ b/sound/soc/generic/Makefile @@ -1,12 +1,14 @@ # SPDX-License-Identifier: GPL-2.0 snd-soc-simple-card-utils-objs := simple-card-utils.o snd-soc-simple-card-objs := simple-card.o +snd-soc-simple-amplifier-objs := simple-amplifier.o snd-soc-simple-scu-card-objs := simple-scu-card.o snd-soc-audio-graph-card-objs := audio-graph-card.o snd-soc-audio-graph-scu-card-objs := audio-graph-scu-card.o
obj-$(CONFIG_SND_SIMPLE_CARD_UTILS) += snd-soc-simple-card-utils.o obj-$(CONFIG_SND_SIMPLE_CARD) += snd-soc-simple-card.o +obj-$(CONFIG_SND_SIMPLE_AMPLIFIER) += snd-soc-simple-amplifier.o obj-$(CONFIG_SND_SIMPLE_SCU_CARD) += snd-soc-simple-scu-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_CARD) += snd-soc-audio-graph-card.o obj-$(CONFIG_SND_AUDIO_GRAPH_SCU_CARD) += snd-soc-audio-graph-scu-card.o diff --git a/sound/soc/generic/simple-amplifier.c b/sound/soc/generic/simple-amplifier.c new file mode 100644 index 0000000..e135a43 --- /dev/null +++ b/sound/soc/generic/simple-amplifier.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +// ALSA SoC simple amplifier driver +// +// Copyright 2018 +// Author: Nicolò Veronese nicveronese@gmail.com
+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>
+#include <sound/soc.h> +#include <sound/soc-dapm.h>
+/* This struct is used to save the context */ +struct simple_amp_data {
- struct device *dev;
- struct regmap *regmap;
You don't need a regmap here.
- struct gpio_desc *power_gpio;
+};
+static int simple_amp_power_event(struct snd_soc_dapm_widget *widget,
struct snd_kcontrol *kctrl, int event)+{
- struct snd_soc_component *c = snd_soc_dapm_to_component(widget->dapm);
- struct simple_amp_data *data = snd_soc_component_get_drvdata(c);
- /* Shutdown GPIO */
- if (data->power_gpio)
gpiod_set_value_cansleep(data->power_gpio, SND_SOC_DAPM_EVENT_ON(event));- return 0;
+}
+static const struct snd_soc_dapm_widget simple_amp_dapm_widgets[] = {
- SND_SOC_DAPM_INPUT("LEFT AMP IN"),
- SND_SOC_DAPM_INPUT("RIGHT AMP IN"),
- SND_SOC_DAPM_OUTPUT("SPK LEFT"),
- SND_SOC_DAPM_OUTPUT("SPK RIGHT"),
- SND_SOC_DAPM_REGULATOR_SUPPLY("VCC", 20, 0),
- SND_SOC_DAPM_SPK("MUTE", simple_amp_power_event),
+};
+static const struct snd_soc_dapm_route simple_amp_dapm_routes[] = {
- { "MUTE", NULL, "LEFT AMP IN" },
- { "MUTE", NULL, "RIGHT AMP IN" },
- { "SPK LEFT", NULL, "VCC" },
- { "SPK RIGHT", NULL, "VCC" },
- { "SPK LEFT", NULL, "MUTE" },
- { "SPK RIGHT", NULL, "MUTE" },
+};
+static const struct snd_soc_component_driver simple_amp_component_driver = {
- .name = "simple-amplifier",
- .dapm_widgets = simple_amp_dapm_widgets,
- .num_dapm_widgets = ARRAY_SIZE(simple_amp_dapm_widgets),
- .dapm_routes = simple_amp_dapm_routes,
- .num_dapm_routes = ARRAY_SIZE(simple_amp_dapm_routes),
+};
+static int asoc_simple_amp_probe(struct platform_device *pdev) +{
- struct simple_amp_data *data;
- struct device *dev = &pdev->dev;
- int ret;
- data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
- if (!data)
return -ENOMEM;- data->dev = &pdev->dev;
- platform_set_drvdata(pdev, data);
- if(of_property_read_bool(pdev->dev.of_node,"shutdown-gpios")) {
Coding style: should be if (
data->power_gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW);if (IS_ERR(data->power_gpio)) {ret = PTR_ERR(data->power_gpio);dev_err(&pdev->dev, "Failed to get shutdown gpio: %d\n", ret);return ret;}- }
- return devm_snd_soc_register_component(dev,
&simple_amp_component_driver, NULL, 0);+}
+static const struct of_device_id asoc_simple_of_match[] = {
- { .compatible = "simple-audio-amplifier", },
- {},
+}; +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+static struct platform_driver asoc_simple_amp = {
- .driver = {
.name = "asoc-simple-amplifier",.of_match_table = of_match_ptr(asoc_simple_of_match),- },
- .probe = asoc_simple_amp_probe,
+};
+module_platform_driver(asoc_simple_amp);
+MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ASoC Simple Amplifier");
+MODULE_AUTHOR("Nicolò Veronese nicveronese@gmail.com");
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, Jun 25, 2018 at 08:28:32PM +0800, Chen-Yu Tsai wrote:
I think slightly different semantics apply for "shutdown" vs "mute" pins / controls. "shutdown" implies a low power, if not powered off altogether, mode which might require a slight delay to charge up when the amp is enabled. "mute" is just that, disconnecting the output, which would be useful in masking pops or other noises, but does nothing to help power consumption.
Not sure if the subtle differences can be handled in DAPM, but it would be nice to have. I've seen devices that have both control pins.
It's certainly potentially useful to have pins for both and treat them differently - ideally something that's a mute would get autodisable type treatment so we mask out any pops in the input path but also make it available to userspace if it needs it. That's something that could be done when the need arises though, until something that actually has both controls appears handling a mute pin like a power control is probably going to result in the most reliable integration.
Having the binding specify both differently would be good for future proofing.
On Mon, Jun 25, 2018 at 8:39 PM, Mark Brown broonie@kernel.org wrote:
On Mon, Jun 25, 2018 at 08:28:32PM +0800, Chen-Yu Tsai wrote:
I think slightly different semantics apply for "shutdown" vs "mute" pins / controls. "shutdown" implies a low power, if not powered off altogether, mode which might require a slight delay to charge up when the amp is enabled. "mute" is just that, disconnecting the output, which would be useful in masking pops or other noises, but does nothing to help power consumption.
Not sure if the subtle differences can be handled in DAPM, but it would be nice to have. I've seen devices that have both control pins.
It's certainly potentially useful to have pins for both and treat them differently - ideally something that's a mute would get autodisable type treatment so we mask out any pops in the input path but also make it available to userspace if it needs it. That's something that could be done when the need arises though, until something that actually has both controls appears handling a mute pin like a power control is probably going to result in the most reliable integration.
Agreed. I'll dig around to see which one of my devices has this.
Having the binding specify both differently would be good for future proofing.
Ack.
Also, would it be possible to specify the name or prefix of the widgets from the device tree, such that it would be possible to have multiple instances? I'm thinking one amp for speakers, and one for headphones, each individually controlled.
ChenYu
On Tue, Jun 26, 2018 at 11:20:37AM +0800, Chen-Yu Tsai wrote:
Also, would it be possible to specify the name or prefix of the widgets from the device tree, such that it would be possible to have multiple instances? I'm thinking one amp for speakers, and one for headphones, each individually controlled.
There's already a general facility for doing this - name_prefix. I don't think it's got a DT binding yet but no reason not to add one.
participants (4)
-
Chen-Yu Tsai -
Jerome Brunet -
Mark Brown -
Nicolò Veronese