[PATCH 0/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[PATCH 0/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Stephan Gerhold]

This series adds a simple driver and DT schema for the Awinic AW8738 
audio amplifier. It's fairly simple - the main difference to 
simple-amplifier is that there is a "one-wire pulse control" that 
allows configuring the amplifier to one of a few pre-defined modes.

Jonathan Albrieux (1):
  ASoC: codecs: Add Awinic AW8738 audio amplifier driver

Stephan Gerhold (1):
  ASoC: dt-bindings: Add schema for "awinic,aw8738"

 .../bindings/sound/awinic,aw8738.yaml         |  48 ++++++++
 sound/soc/codecs/Kconfig                      |  10 ++
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/aw8738.c                     | 104 ++++++++++++++++++
 4 files changed, 164 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/awinic,aw8738.yaml
 create mode 100644 sound/soc/codecs/aw8738.c

-- 
2.35.1

[PATCH 1/2] ASoC: dt-bindings: Add schema for "awinic,aw8738"

[Stephan Gerhold]

Add a DT schema for describing Awinic AW8738 audio amplifiers. They are
fairly simple and controlled using a single GPIO. The number of pulses
during power up selects one of a few pre-defined amplifier modes.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../bindings/sound/awinic,aw8738.yaml         | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/awinic,aw8738.yaml

diff --git a/Documentation/devicetree/bindings/sound/awinic,aw8738.yaml b/Documentation/devicetree/bindings/sound/awinic,aw8738.yaml
new file mode 100644
index 000000000000..83934a835fd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/awinic,aw8738.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/awinic,aw8738.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Awinic AW8738 Audio Amplifier
+
+maintainers:
+  - Stephan Gerhold <stephan@gerhold.net>
+
+description:
+  The Awinic AW8738 is a simple audio amplifier using
+  one-wire pulse control to set the amplifier mode.
+
+allOf:
+  - $ref: name-prefix.yaml#
+
+properties:
+  compatible:
+    const: awinic,aw8738
+
+  enable-gpios:
+    maxItems: 1
+
+  awinic,mode:
+    description: Amplifier mode (number of pulses for one-wire pulse control)
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+
+  sound-name-prefix: true
+
+required:
+  - compatible
+  - enable-gpios
+  - awinic,mode
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    audio-amplifier {
+        compatible = "awinic,aw8738";
+        enable-gpios = <&msmgpio 114 GPIO_ACTIVE_HIGH>;
+        awinic,mode = <5>;
+        sound-name-prefix = "Speaker Amp";
+    };
-- 
2.35.1

Re: [PATCH 1/2] ASoC: dt-bindings: Add schema for "awinic,aw8738"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Tue, Mar 01, 2022 at 01:37:41PM +0100, Stephan Gerhold wrote:

> +  awinic,mode:
> +    description: Amplifier mode (number of pulses for one-wire pulse control)
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1

No maximum?

> +  sound-name-prefix: true

While it's very likely that someone will want to use this it shouldn't
be *mandatory* that they do so, there may be no other controls for it to
collide with (eg, if none of the other components in the audio path have
controls).

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

Re: [PATCH 1/2] ASoC: dt-bindings: Add schema for "awinic,aw8738"

[Stephan Gerhold]

On Tue, Mar 01, 2022 at 01:36:33PM +0000, Mark Brown wrote:
> On Tue, Mar 01, 2022 at 01:37:41PM +0100, Stephan Gerhold wrote:
> 
> > +  awinic,mode:
> > +    description: Amplifier mode (number of pulses for one-wire pulse control)
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 1
> 
> No maximum?
> 

I wasn't sure if we really want to maintain maxima for several different
Awinic part numbers here. For the AW8738 the maximum is 7, for the
AW87318 (note the additional 1 in the name) it would be 10. The driver
part would be identical for both.

... But it's probably cleaner to have the schema check for the real
maximum of the selected compatible. I will add a "maximum" in v2.

> > +  sound-name-prefix: true
> 
> While it's very likely that someone will want to use this it shouldn't
> be *mandatory* that they do so, there may be no other controls for it to
> collide with (eg, if none of the other components in the audio path have
> controls).

This line just enables the "sound-name-prefix" from the included
name-prefix.yaml. It's not listed in "required" so it's actually
already optional.

Thanks!
Stephan

Re: [PATCH 1/2] ASoC: dt-bindings: Add schema for "awinic,aw8738"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Tue, Mar 01, 2022 at 02:54:06PM +0100, Stephan Gerhold wrote:
> On Tue, Mar 01, 2022 at 01:36:33PM +0000, Mark Brown wrote:
> > On Tue, Mar 01, 2022 at 01:37:41PM +0100, Stephan Gerhold wrote:

> > > +  awinic,mode:
> > > +    description: Amplifier mode (number of pulses for one-wire pulse control)
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    minimum: 1

> > No maximum?

> I wasn't sure if we really want to maintain maxima for several different
> Awinic part numbers here. For the AW8738 the maximum is 7, for the
> AW87318 (note the additional 1 in the name) it would be 10. The driver
> part would be identical for both.

> ... But it's probably cleaner to have the schema check for the real
> maximum of the selected compatible. I will add a "maximum" in v2.

Even if it's just a maximum for any of the supported chips it'd still be
something.

> > > +  sound-name-prefix: true

> > While it's very likely that someone will want to use this it shouldn't
> > be *mandatory* that they do so, there may be no other controls for it to
> > collide with (eg, if none of the other components in the audio path have
> > controls).

> This line just enables the "sound-name-prefix" from the included
> name-prefix.yaml. It's not listed in "required" so it's actually
> already optional.

Of, that's a really confusing way of importing things.

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

Re: [PATCH 1/2] ASoC: dt-bindings: Add schema for "awinic,aw8738"

[Rob Herring]

On Tue, Mar 01, 2022 at 01:37:41PM +0100, Stephan Gerhold wrote:
> Add a DT schema for describing Awinic AW8738 audio amplifiers. They are
> fairly simple and controlled using a single GPIO. The number of pulses
> during power up selects one of a few pre-defined amplifier modes.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../bindings/sound/awinic,aw8738.yaml         | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/awinic,aw8738.yaml

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Stephan Gerhold]

From: Jonathan Albrieux <jonathan.albrieux@gmail.com>

The Awinic AW8738 is a simple audio amplifier using an enable GPIO.
The main difference to simple-amplifier is that there is a "one-wire
pulse control" that allows configuring the amplifier to one of a few
pre-defined modes.

Add a simple driver that allows setting it up in the device tree
with a specified mode number.

Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 sound/soc/codecs/Kconfig  |  10 ++++
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/aw8738.c | 104 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 sound/soc/codecs/aw8738.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 4de029ae377c..0e35d33f8590 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -53,6 +53,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_AK5558
 	imply SND_SOC_ALC5623
 	imply SND_SOC_ALC5632
+	imply SND_SOC_AW8738
 	imply SND_SOC_BT_SCO
 	imply SND_SOC_BD28623
 	imply SND_SOC_CQ0093VC
@@ -579,6 +580,15 @@ config SND_SOC_ALC5632
 	tristate
 	depends on I2C
 
+config SND_SOC_AW8738
+	tristate "Awinic AW8738 Audio Amplifier"
+	select GPIOLIB
+	help
+	  Enable support for the Awinic AW8738 audio amplifier (or similar).
+	  The driver supports simple audio amplifiers similar to
+	  SND_SOC_SIMPLE_AMPLIFIER, but additionally allows setting the
+	  amplifier mode using the Awinic-specific one-wire pulse control.
+
 config SND_SOC_BD28623
 	tristate "ROHM BD28623 CODEC"
 	help
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index c3c6059a5f8a..8637e9e869e3 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -45,6 +45,7 @@ snd-soc-ak4671-objs := ak4671.o
 snd-soc-ak5386-objs := ak5386.o
 snd-soc-ak5558-objs := ak5558.o
 snd-soc-arizona-objs := arizona.o arizona-jack.o
+snd-soc-aw8738-objs := aw8738.o
 snd-soc-bd28623-objs := bd28623.o
 snd-soc-bt-sco-objs := bt-sco.o
 snd-soc-cpcap-objs := cpcap.o
@@ -388,6 +389,7 @@ obj-$(CONFIG_SND_SOC_AK5558)	+= snd-soc-ak5558.o
 obj-$(CONFIG_SND_SOC_ALC5623)    += snd-soc-alc5623.o
 obj-$(CONFIG_SND_SOC_ALC5632)	+= snd-soc-alc5632.o
 obj-$(CONFIG_SND_SOC_ARIZONA)	+= snd-soc-arizona.o
+obj-$(CONFIG_SND_SOC_AW8738)	+= snd-soc-aw8738.o
 obj-$(CONFIG_SND_SOC_BD28623)	+= snd-soc-bd28623.o
 obj-$(CONFIG_SND_SOC_BT_SCO)	+= snd-soc-bt-sco.o
 obj-$(CONFIG_SND_SOC_CQ0093VC) += snd-soc-cq93vc.o
diff --git a/sound/soc/codecs/aw8738.c b/sound/soc/codecs/aw8738.c
new file mode 100644
index 000000000000..b075c7c0efd0
--- /dev/null
+++ b/sound/soc/codecs/aw8738.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <sound/soc.h>
+
+struct aw8738_priv {
+	struct gpio_desc *gpiod_enable;
+	unsigned int mode;
+};
+
+static int aw8738_drv_event(struct snd_soc_dapm_widget *w,
+			    struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_component *c = snd_soc_dapm_to_component(w->dapm);
+	struct aw8738_priv *aw = snd_soc_component_get_drvdata(c);
+	int i;
+
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMU:
+		for (i = 0; i < aw->mode; i++) {
+			gpiod_set_value_cansleep(aw->gpiod_enable, 0);
+			udelay(2);
+			gpiod_set_value_cansleep(aw->gpiod_enable, 1);
+			udelay(2);
+		}
+		msleep(40);
+		break;
+	case SND_SOC_DAPM_PRE_PMD:
+		gpiod_set_value_cansleep(aw->gpiod_enable, 0);
+		usleep_range(1000, 2000);
+		break;
+	default:
+		WARN(1, "Unexpected event");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget aw8738_dapm_widgets[] = {
+	SND_SOC_DAPM_INPUT("IN"),
+	SND_SOC_DAPM_OUT_DRV_E("DRV", SND_SOC_NOPM, 0, 0, NULL, 0, aw8738_drv_event,
+			       SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
+	SND_SOC_DAPM_OUTPUT("OUT"),
+};
+
+static const struct snd_soc_dapm_route aw8738_dapm_routes[] = {
+	{ "DRV", NULL, "IN" },
+	{ "OUT", NULL, "DRV" },
+};
+
+static const struct snd_soc_component_driver aw8738_component_driver = {
+	.dapm_widgets = aw8738_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(aw8738_dapm_widgets),
+	.dapm_routes = aw8738_dapm_routes,
+	.num_dapm_routes = ARRAY_SIZE(aw8738_dapm_routes),
+};
+
+static int aw8738_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct aw8738_priv *aw;
+	int ret;
+
+	aw = devm_kzalloc(dev, sizeof(*aw), GFP_KERNEL);
+	if (!aw)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, aw);
+
+	aw->gpiod_enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(aw->gpiod_enable))
+		return dev_err_probe(dev, PTR_ERR(aw->gpiod_enable),
+				     "Failed to get 'enable' gpio");
+
+	ret = device_property_read_u32(dev, "awinic,mode", &aw->mode);
+	if (ret)
+		return -EINVAL;
+
+	return devm_snd_soc_register_component(&pdev->dev,
+					       &aw8738_component_driver,
+					       NULL, 0);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id aw8738_of_match[] = {
+	{ .compatible = "awinic,aw8738" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aw8738_of_match);
+#endif
+
+static struct platform_driver aw8738_driver = {
+	.probe	= aw8738_probe,
+	.driver = {
+		.name = "aw8738",
+		.of_match_table = of_match_ptr(aw8738_of_match),
+	},
+};
+module_platform_driver(aw8738_driver);
+
+MODULE_DESCRIPTION("Awinic AW8738 Amplifier Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.35.1

Re: [PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

On Tue, Mar 01, 2022 at 01:37:42PM +0100, Stephan Gerhold wrote:
> From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> 
> The Awinic AW8738 is a simple audio amplifier using an enable GPIO.
> The main difference to simple-amplifier is that there is a "one-wire
> pulse control" that allows configuring the amplifier to one of a few
> pre-defined modes.

I only have this patch, nothing else from the rest of the series.
What's the story with dependencies?

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

Re: [PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Stephan Gerhold]

On Tue, Mar 01, 2022 at 12:47:10PM +0000, Mark Brown wrote:
> On Tue, Mar 01, 2022 at 01:37:42PM +0100, Stephan Gerhold wrote:
> > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > 
> > The Awinic AW8738 is a simple audio amplifier using an enable GPIO.
> > The main difference to simple-amplifier is that there is a "one-wire
> > pulse control" that allows configuring the amplifier to one of a few
> > pre-defined modes.
> 
> I only have this patch, nothing else from the rest of the series.
> What's the story with dependencies?

Hmm, I definitely sent the whole series to you. Let's wait a bit longer
to see if it will still arrive, otherwise let me know and I can try to
send it again.

It seems to have arrived fully on the mailing list:
https://lore.kernel.org/alsa-devel/20220301123742.72146-1-stephan@gerhold.net/

Thanks,
Stephan

Re: [PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Tue, Mar 01, 2022 at 01:53:12PM +0100, Stephan Gerhold wrote:
> On Tue, Mar 01, 2022 at 12:47:10PM +0000, Mark Brown wrote:

> > I only have this patch, nothing else from the rest of the series.
> > What's the story with dependencies?

> Hmm, I definitely sent the whole series to you. Let's wait a bit longer
> to see if it will still arrive, otherwise let me know and I can try to
> send it again.

Yes, it's turned up OK now - wonder what delayed the earlier bits.

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

Re: [PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Tue, Mar 01, 2022 at 01:37:42PM +0100, Stephan Gerhold wrote:

> The Awinic AW8738 is a simple audio amplifier using an enable GPIO.
> The main difference to simple-amplifier is that there is a "one-wire
> pulse control" that allows configuring the amplifier to one of a few
> pre-defined modes.

What exactly are the modes here?  Looking at the web site for the part
it seems like it's selecting a power limit for the speaker so it makes
sense that the mode would be fixed in DT but it's not clear from the
driver.

> +	aw->gpiod_enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(aw->gpiod_enable))
> +		return dev_err_probe(dev, PTR_ERR(aw->gpiod_enable),
> +				     "Failed to get 'enable' gpio");

Are we sure that enable is the best name for this pin?  It's more
complex than just an enable since it's the 1 wire data - according to
what's on the awinic web site it looks like the actual label is /SHDN
which is similarly misleading though :/

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

Re: [PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Stephan Gerhold]

On Tue, Mar 01, 2022 at 01:45:01PM +0000, Mark Brown wrote:
> On Tue, Mar 01, 2022 at 01:37:42PM +0100, Stephan Gerhold wrote:
> 
> > The Awinic AW8738 is a simple audio amplifier using an enable GPIO.
> > The main difference to simple-amplifier is that there is a "one-wire
> > pulse control" that allows configuring the amplifier to one of a few
> > pre-defined modes.
> 
> What exactly are the modes here?  Looking at the web site for the part
> it seems like it's selecting a power limit for the speaker so it makes
> sense that the mode would be fixed in DT but it's not clear from the
> driver.
> 

It seems to be mostly a power limit but not only. E.g. on AW8738 mode
3/4 and 5/6 seem to have the same power limit but select between a
"NCN function" or "Multi-Level AGC function", which seems to control
how the amplifier behaves if the power limit is reached.

The exact effect of the modes varies greatly between different Awinic
parts, but since I don't really see a use case for changing those
options dynamically I think it's best to just load it from DT.

> > +	aw->gpiod_enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> > +	if (IS_ERR(aw->gpiod_enable))
> > +		return dev_err_probe(dev, PTR_ERR(aw->gpiod_enable),
> > +				     "Failed to get 'enable' gpio");
> 
> Are we sure that enable is the best name for this pin?  It's more
> complex than just an enable since it's the 1 wire data - according to
> what's on the awinic web site it looks like the actual label is /SHDN
> which is similarly misleading though :/

Yeah, I was considering to call it "shdn" instead but given the negation
that seemed even more confusing. I ended up using "enable" since this is
the name used in the mode table of the datasheet (which will probably be
the main reference when setting up the amplifier in the DT).

Thanks,
Stephan