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

[Jerome Brunet <jbrunet@baylibre.com>]

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