[PATCH 0/2] ASoC: add support for ROHM BD28623 codec

[PATCH 0/2] ASoC: add support for ROHM BD28623 codec

[Katsuhiro Suzuki]

This patch adds support for ROHM BD28623MUV class D speaker
amplifier codec driver.

This driver only refers information of HW specification document
that can be derivered at website of ROHM.

http://www.rohm.com/web/global/products/-/product/BD28623MUV

Katsuhiro Suzuki (2):
  ASoC: add DT bindings documentation for ROHM BD28623 codec
  ASoC: support ROHM BD28623 codec

 .../devicetree/bindings/sound/rohm,bd28623.txt     |  26 +++
 sound/soc/codecs/Kconfig                           |   8 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/bd28623.c                         | 258 +++++++++++++++++++++
 4 files changed, 294 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/rohm,bd28623.txt
 create mode 100644 sound/soc/codecs/bd28623.c

-- 
2.16.1

[PATCH 1/2] ASoC: add DT bindings documentation for ROHM BD28623 codec

[Katsuhiro Suzuki]

This patch adds DT bindings documentation for ROHM BD28623MUV
class D speaker amplifier.

Signed-off-by: Katsuhiro Suzuki <suzuki.katsuhiro@socionext.com>
---
 .../devicetree/bindings/sound/rohm,bd28623.txt     | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/rohm,bd28623.txt

diff --git a/Documentation/devicetree/bindings/sound/rohm,bd28623.txt b/Documentation/devicetree/bindings/sound/rohm,bd28623.txt
new file mode 100644
index 000000000000..954c689b5b08
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/rohm,bd28623.txt
@@ -0,0 +1,26 @@
+ROHM BD28623MUV Class D speaker amplifier for digital input
+
+This codec does not have any control buses such as I2C, it detect format and
+rate of I2S signal automatically. It has two signals that can be connected
+to GPIOs: reset and mute.
+
+Required properties:
+- compatible      : should be "rohm,bd28623"
+- #sound-dai-cells: should be 0.
+- reset-gpios     : GPIO specifier for the active low reset line
+- mute-gpios      : GPIO specifier for the active low mute line
+
+Optional properties:
+- VCCA-supply : regulator phandle for the VCCA supply
+- VCCP1-supply: regulator phandle for the VCCP1 supply
+- VCCP2-supply: regulator phandle for the VCCP2 supply
+
+Example:
+
+	codec {
+		compatible = "rohm,bd28623";
+		#sound-dai-cells = <0>;
+
+		reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>;
+		mute-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
+	};
-- 
2.16.1

Re: [PATCH 1/2] ASoC: add DT bindings documentation for ROHM BD28623 codec

[Mark Brown]

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

On Wed, Feb 21, 2018 at 01:33:10PM +0900, Katsuhiro Suzuki wrote:

> +Optional properties:
> +- VCCA-supply : regulator phandle for the VCCA supply
> +- VCCP1-supply: regulator phandle for the VCCP1 supply
> +- VCCP2-supply: regulator phandle for the VCCP2 supply

These should be documented as mandatory unless the device genuinely
operates without power which seems unlikely.

[-- 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 DT bindings documentation for ROHM BD28623 codec

[Katsuhiro Suzuki]

Hello Mark,

Thank you for your review.

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Wednesday, February 21, 2018 9:14 PM
> To: Suzuki, Katsuhiro <suzuki.katsuhiro@socionext.com>
> Cc: alsa-devel@alsa-project.org; Rob Herring <robh+dt@kernel.org>; devicetree@vger.kernel.org; Masami Hiramatsu
> <masami.hiramatsu@linaro.org>; Jassi Brar <jaswinder.singh@linaro.org>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] ASoC: add DT bindings documentation for ROHM BD28623 codec
> 
> On Wed, Feb 21, 2018 at 01:33:10PM +0900, Katsuhiro Suzuki wrote:
> 
> > +Optional properties:
> > +- VCCA-supply : regulator phandle for the VCCA supply
> > +- VCCP1-supply: regulator phandle for the VCCP1 supply
> > +- VCCP2-supply: regulator phandle for the VCCP2 supply
> 
> These should be documented as mandatory unless the device genuinely
> operates without power which seems unlikely.

Indeed, this IC does not work correctly if VCC power supply is lost.
It's not optional. I'll fix it and send V2.


Regards,
--
Katsuhiro Suzuki

[PATCH 2/2] ASoC: support ROHM BD28623 codec

[Katsuhiro Suzuki]

This patch adds support of the ROHM BD28623MUV
Class D speaker amplifier for Flat-panel TVs.
This IC delivers an output power of 20W + 20W.

Signed-off-by: Katsuhiro Suzuki <suzuki.katsuhiro@socionext.com>
---
 sound/soc/codecs/Kconfig   |   8 ++
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/bd28623.c | 258 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+)
 create mode 100644 sound/soc/codecs/bd28623.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index f72a90104a58..6a53e188ead6 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -47,6 +47,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_ALC5623 if I2C
 	select SND_SOC_ALC5632 if I2C
 	select SND_SOC_BT_SCO
+	select SND_SOC_BD28623
 	select SND_SOC_CQ0093VC
 	select SND_SOC_CS35L32 if I2C
 	select SND_SOC_CS35L33 if I2C
@@ -418,6 +419,13 @@ config SND_SOC_ALC5623
 config SND_SOC_ALC5632
 	tristate
 
+config SND_SOC_BD28623
+	tristate "ROHM BD28623 CODEC"
+	help
+	  Enable support for ROHM BD28623MUV Class D speaker amplifier.
+	  This codec does not have any control buses such as I2C, it
+	  detect format of I2S automatically.
+
 config SND_SOC_BT_SCO
 	tristate "Dummy BT SCO codec driver"
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 56c3252820d2..f2c710e16557 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -37,6 +37,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
+snd-soc-bd28623-objs := bd28623.o
 snd-soc-bt-sco-objs := bt-sco.o
 snd-soc-cq93vc-objs := cq93vc.o
 snd-soc-cs35l32-objs := cs35l32.o
@@ -285,6 +286,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_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
 obj-$(CONFIG_SND_SOC_CS35L32)	+= snd-soc-cs35l32.o
diff --git a/sound/soc/codecs/bd28623.c b/sound/soc/codecs/bd28623.c
new file mode 100644
index 000000000000..672c8e790e24
--- /dev/null
+++ b/sound/soc/codecs/bd28623.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ROHM BD28623MUV class D speaker amplifier codec driver.
+ *
+ * Copyright (c) 2018 Socionext Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <sound/pcm.h>
+#include <sound/soc.h>
+
+#define BD28623_NUM_SUPPLIES    3
+
+static const char *const bd28623_supply_names[BD28623_NUM_SUPPLIES] = {
+	"VCCA",
+	"VCCP1",
+	"VCCP2",
+};
+
+struct bd28623_priv {
+	struct platform_device *pdev;
+	struct regulator_bulk_data supplies[BD28623_NUM_SUPPLIES];
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *mute_gpio;
+
+	int switch_spk;
+};
+
+static const struct snd_soc_dapm_widget bd28623_widgets[] = {
+	SND_SOC_DAPM_DAC("DAC", "Playback", SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_OUTPUT("OUT1P"),
+	SND_SOC_DAPM_OUTPUT("OUT1N"),
+	SND_SOC_DAPM_OUTPUT("OUT2P"),
+	SND_SOC_DAPM_OUTPUT("OUT2N"),
+};
+
+static const struct snd_soc_dapm_route bd28623_routes[] = {
+	{ "OUT1P", NULL, "DAC" },
+	{ "OUT1N", NULL, "DAC" },
+	{ "OUT2P", NULL, "DAC" },
+	{ "OUT2N", NULL, "DAC" },
+};
+
+static int bd28623_power_on(struct bd28623_priv *bd)
+{
+	struct device *dev = &bd->pdev->dev;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(bd->supplies), bd->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	gpiod_set_value(bd->reset_gpio, 0);
+	usleep_range(300000, 400000);
+
+	return 0;
+}
+
+static void bd28623_power_off(struct bd28623_priv *bd)
+{
+	gpiod_set_value(bd->reset_gpio, 1);
+
+	regulator_bulk_disable(ARRAY_SIZE(bd->supplies), bd->supplies);
+}
+
+static int bd28623_update_switch_spk(struct bd28623_priv *bd)
+{
+	if (bd->switch_spk)
+		gpiod_set_value(bd->mute_gpio, 0);
+	else
+		gpiod_set_value(bd->mute_gpio, 1);
+
+	return 0;
+}
+
+static int bd28623_get_switch_spk(struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component =
+		snd_soc_kcontrol_component(kcontrol);
+	struct bd28623_priv *bd = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.integer.value[0] = bd->switch_spk;
+
+	return 0;
+}
+
+static int bd28623_set_switch_spk(struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component =
+		snd_soc_kcontrol_component(kcontrol);
+	struct bd28623_priv *bd = snd_soc_component_get_drvdata(component);
+
+	if (bd->switch_spk == ucontrol->value.integer.value[0])
+		return 0;
+
+	bd->switch_spk = ucontrol->value.integer.value[0];
+
+	return bd28623_update_switch_spk(bd);
+}
+
+static const struct snd_kcontrol_new bd28623_controls[] = {
+	SOC_SINGLE_BOOL_EXT("Speaker Switch", 0,
+			    bd28623_get_switch_spk, bd28623_set_switch_spk),
+};
+
+static int bd28623_codec_probe(struct snd_soc_component *component)
+{
+	struct bd28623_priv *bd = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	bd->switch_spk = 1;
+
+	ret = bd28623_power_on(bd);
+	if (ret)
+		return ret;
+
+	bd28623_update_switch_spk(bd);
+
+	return 0;
+}
+
+static int bd28623_codec_suspend(struct snd_soc_component *component)
+{
+	struct bd28623_priv *bd = snd_soc_component_get_drvdata(component);
+
+	bd28623_power_off(bd);
+
+	return 0;
+}
+
+static int bd28623_codec_resume(struct snd_soc_component *component)
+{
+	struct bd28623_priv *bd = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	ret = bd28623_power_on(bd);
+	if (ret)
+		return ret;
+
+	bd28623_update_switch_spk(bd);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver soc_codec_bd = {
+	.probe			= bd28623_codec_probe,
+	.suspend		= bd28623_codec_suspend,
+	.resume			= bd28623_codec_resume,
+	.dapm_widgets		= bd28623_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(bd28623_widgets),
+	.dapm_routes		= bd28623_routes,
+	.num_dapm_routes	= ARRAY_SIZE(bd28623_routes),
+	.controls		= bd28623_controls,
+	.num_controls		= ARRAY_SIZE(bd28623_controls),
+	.idle_bias_on		= 1,
+	.use_pmdown_time	= 1,
+	.endianness		= 1,
+	.non_legacy_dai_naming	= 1,
+};
+
+static struct snd_soc_dai_driver soc_dai_bd = {
+	.name     = "bd28623-speaker",
+	.playback = {
+		.stream_name  = "Playback",
+		.formats      = SNDRV_PCM_FMTBIT_S32_LE |
+				SNDRV_PCM_FMTBIT_S24_LE |
+				SNDRV_PCM_FMTBIT_S16_LE,
+		.rates        = SNDRV_PCM_RATE_48000 |
+				SNDRV_PCM_RATE_44100 |
+				SNDRV_PCM_RATE_32000,
+		.channels_min = 2,
+		.channels_max = 2,
+	},
+};
+
+static int bd28623_probe(struct platform_device *pdev)
+{
+	struct bd28623_priv *bd;
+	struct device *dev = &pdev->dev;
+	int i, ret;
+
+	bd = devm_kzalloc(&pdev->dev, sizeof(struct bd28623_priv), GFP_KERNEL);
+	if (!bd)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(bd->supplies); i++)
+		bd->supplies[i].supply = bd28623_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(bd->supplies),
+				      bd->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to get supplies: %d\n", ret);
+		return ret;
+	}
+
+	bd->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(bd->reset_gpio)) {
+		dev_err(dev, "Failed to request reset_gpio: %ld\n",
+			PTR_ERR(bd->reset_gpio));
+		return PTR_ERR(bd->reset_gpio);
+	}
+
+	bd->mute_gpio = devm_gpiod_get_optional(dev, "mute",
+						GPIOD_OUT_HIGH);
+	if (IS_ERR(bd->mute_gpio)) {
+		dev_err(dev, "Failed to request mute_gpio: %ld\n",
+			PTR_ERR(bd->mute_gpio));
+		return PTR_ERR(bd->mute_gpio);
+	}
+
+	platform_set_drvdata(pdev, bd);
+	bd->pdev = pdev;
+
+	ret = devm_snd_soc_register_component(dev, &soc_codec_bd,
+					      &soc_dai_bd, 1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int bd28623_remove(struct platform_device *pdev)
+{
+	struct bd28623_priv *bd = platform_get_drvdata(pdev);
+
+	regulator_bulk_disable(ARRAY_SIZE(bd->supplies), bd->supplies);
+
+	return 0;
+}
+
+static const struct of_device_id bd28623_of_match[] = {
+	{ .compatible = "rohm,bd28623", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bd28623_of_match);
+
+static struct platform_driver bd28623_codec_driver = {
+	.driver = {
+		.name = "bd28623",
+		.of_match_table = of_match_ptr(bd28623_of_match),
+	},
+	.probe  = bd28623_probe,
+	.remove = bd28623_remove,
+};
+module_platform_driver(bd28623_codec_driver);
+
+MODULE_AUTHOR("Katsuhiro Suzuki <suzuki.katsuhiro@socionext.com>");
+MODULE_DESCRIPTION("ROHM BD28623 speaker amplifier driver");
+MODULE_LICENSE("GPL v2");
-- 
2.16.1

Re: [PATCH 2/2] ASoC: support ROHM BD28623 codec

[Mark Brown]

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

On Wed, Feb 21, 2018 at 01:33:11PM +0900, Katsuhiro Suzuki wrote:

> +++ b/sound/soc/codecs/bd28623.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BD28623MUV class D speaker amplifier codec driver.
> + *

Please make the entire comment C++ so this looks intentional.

> +		dev_err(dev, "Failed to enable supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	gpiod_set_value(bd->reset_gpio, 0);

Since this GPIO is not needed in atomic contexts you should use the
_cansleep() versions of the GPIO functions - it doesn't cost you
anything and means that if for some reason someone wired this up to a
GPIO that can't be used in atomic context the driver will just work.

> +	bd->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						 GPIOD_OUT_HIGH);

> +	bd->mute_gpio = devm_gpiod_get_optional(dev, "mute",
> +						GPIOD_OUT_HIGH);

These properties were documented as mandatory in the binding but are
optional here.  It's fine that they're optional but I'd expect the
binding to be consistent with this.

> +static int bd28623_remove(struct platform_device *pdev)
> +{
> +	struct bd28623_priv *bd = platform_get_drvdata(pdev);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(bd->supplies), bd->supplies);
> +
> +	return 0;
> +}

We don't enable the supplies explicitly as part of the probe function so
it feels wrong to disable on remove() - I'm sure it is fine in practice
as-is but I'd have to think too hard to confirm that.  I'd put this in a
component level remove function instead so that it's consistent.

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

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

Re: [PATCH 2/2] ASoC: support ROHM BD28623 codec

[Katsuhiro Suzuki]

Hello Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Wednesday, February 21, 2018 9:27 PM
> To: Suzuki, Katsuhiro <suzuki.katsuhiro@socionext.com>
> Cc: alsa-devel@alsa-project.org; Rob Herring <robh+dt@kernel.org>; devicetree@vger.kernel.org; Masami Hiramatsu
> <masami.hiramatsu@linaro.org>; Jassi Brar <jaswinder.singh@linaro.org>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] ASoC: support ROHM BD28623 codec
> 
> On Wed, Feb 21, 2018 at 01:33:11PM +0900, Katsuhiro Suzuki wrote:
> 
> > +++ b/sound/soc/codecs/bd28623.c
> > @@ -0,0 +1,258 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ROHM BD28623MUV class D speaker amplifier codec driver.
> > + *
> 
> Please make the entire comment C++ so this looks intentional.
> 
> > +		dev_err(dev, "Failed to enable supplies: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	gpiod_set_value(bd->reset_gpio, 0);
> 
> Since this GPIO is not needed in atomic contexts you should use the
> _cansleep() versions of the GPIO functions - it doesn't cost you
> anything and means that if for some reason someone wired this up to a
> GPIO that can't be used in atomic context the driver will just work.
> 

Thank you, I'll fix it.


> > +	bd->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +						 GPIOD_OUT_HIGH);
> 
> > +	bd->mute_gpio = devm_gpiod_get_optional(dev, "mute",
> > +						GPIOD_OUT_HIGH);
> 
> These properties were documented as mandatory in the binding but are
> optional here.  It's fine that they're optional but I'd expect the
> binding to be consistent with this.
> 

These GPIO is optional if board vendor connects directly RSTX and MUTEX pins
to VCC. So I think I should fix DT-bindings document.


> > +static int bd28623_remove(struct platform_device *pdev)
> > +{
> > +	struct bd28623_priv *bd = platform_get_drvdata(pdev);
> > +
> > +	regulator_bulk_disable(ARRAY_SIZE(bd->supplies), bd->supplies);
> > +
> > +	return 0;
> > +}
> 
> We don't enable the supplies explicitly as part of the probe function so
> it feels wrong to disable on remove() - I'm sure it is fine in practice
> as-is but I'd have to think too hard to confirm that.  I'd put this in a
> component level remove function instead so that it's consistent.

Ah, indeed. I will use component driver's remove() function instead of platform.

Thank you for review!


Regards,
--
Katsuhiro Suzuki