2018-02-21 04:35:04

by Katsuhiro Suzuki

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

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



2018-02-21 04:34:36

by Katsuhiro Suzuki

[permalink] [raw]
Subject: [PATCH 2/2] ASoC: support ROHM BD28623 codec

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 <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("ROHM BD28623 speaker amplifier driver");
+MODULE_LICENSE("GPL v2");
--
2.16.1


2018-02-21 04:41:32

by Katsuhiro Suzuki

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

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

Signed-off-by: Katsuhiro Suzuki <[email protected]>
---
.../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


2018-02-21 13:20:49

by Katsuhiro Suzuki

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

Hello Mark,

Thank you for your review.

> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Wednesday, February 21, 2018 9:14 PM
> To: Suzuki, Katsuhiro <[email protected]>
> Cc: [email protected]; Rob Herring <[email protected]>; [email protected]; Masami Hiramatsu
> <[email protected]>; Jassi Brar <[email protected]>; [email protected];
> [email protected]
> 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






2018-02-21 17:05:56

by Mark Brown

[permalink] [raw]
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.

> + 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.


Attachments:
(No filename) (1.55 kB)
signature.asc (499.00 B)
Download all attachments

2018-02-21 17:48:42

by Mark Brown

[permalink] [raw]
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.


Attachments:
(No filename) (387.00 B)
signature.asc (499.00 B)
Download all attachments

2018-02-21 18:40:23

by Katsuhiro Suzuki

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

Hello Mark,

> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Wednesday, February 21, 2018 9:27 PM
> To: Suzuki, Katsuhiro <[email protected]>
> Cc: [email protected]; Rob Herring <[email protected]>; [email protected]; Masami Hiramatsu
> <[email protected]>; Jassi Brar <[email protected]>; [email protected];
> [email protected]
> 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