[PATCH 1/2] ASoC: add generic simple-amplifier support

[PATCH 1/2] ASoC: add generic simple-amplifier support

[Nicolò Veronese]

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>");
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 2/2] ASoC: simple-amplifier: add dt-bindings

[Nicolò Veronese]

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>;
+	...
+};
+
+amp: amplifier {
+	compatible = "simple-audio-amplifier";
+	VCC-supply = <&regulator>
+	shutdown-gpios = <&r_pio 2 GPIO_ACTIVE_LOW>;
+};
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 6269 bytes --]

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 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;

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
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Jerome Brunet]

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 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;
> 
> 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

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 533 bytes --]

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 :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Jerome Brunet]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Nicolò Veronese]

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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Jerome Brunet]

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Chen-Yu Tsai]

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 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;
>>
>> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1148 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Chen-Yu Tsai]

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

Re: [PATCH 1/2] ASoC: add generic simple-amplifier support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 450 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]