2018-11-14 12:17:45

by Clément Péron

[permalink] [raw]
Subject: [PATCH v2 1/2] ASoC: ak4118: Add support for AK4118 S/PDIF transceiver

From: Adrien Charruel <[email protected]>

The AK4118A is a digital audio transceiver supporting 8 input channels
at 192kHz and with 24bits resolution.
It converts the S/PDIF signal to I2S format and is configurable over I2C.

This driver introduce a minimal support of the AK4118, like selecting the
input channel, reading input frequency and detecting some errors.

Datasheet is available here:
https://www.akm.com/akm/en/file/datasheet/AK4118AEQ.pdf

Signed-off-by: Adrien Charruel <[email protected]>
Signed-off-by: Clément Péron <[email protected]>
---

v2:
- request threaded_irq in driver probe
- change fs_enum from VALUE_SINGLE to VALUE_ENUM_SINGLE

sound/soc/codecs/Kconfig | 6 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/ak4118.c | 438 ++++++++++++++++++++++++++++++++++++++
3 files changed, 446 insertions(+)
create mode 100644 sound/soc/codecs/ak4118.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9cc4f1848c9b..62bdb7e333b8 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -35,6 +35,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_ADAU7002
select SND_SOC_ADS117X
select SND_SOC_AK4104 if SPI_MASTER
+ select SND_SOC_AK4118 if I2C
select SND_SOC_AK4458 if I2C
select SND_SOC_AK4535 if I2C
select SND_SOC_AK4554
@@ -392,6 +393,11 @@ config SND_SOC_AK4104
tristate "AKM AK4104 CODEC"
depends on SPI_MASTER

+config SND_SOC_AK4118
+ tristate "AKM AK4118 CODEC"
+ depends on I2C
+ select REGMAP_I2C
+
config SND_SOC_AK4458
tristate "AKM AK4458 CODEC"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 8ffab8c8dbfa..66f55d185620 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -27,6 +27,7 @@ snd-soc-adav801-objs := adav801.o
snd-soc-adav803-objs := adav803.o
snd-soc-ads117x-objs := ads117x.o
snd-soc-ak4104-objs := ak4104.o
+snd-soc-ak4118-objs := ak4118.o
snd-soc-ak4458-objs := ak4458.o
snd-soc-ak4535-objs := ak4535.o
snd-soc-ak4554-objs := ak4554.o
@@ -290,6 +291,7 @@ obj-$(CONFIG_SND_SOC_ADAV801) += snd-soc-adav801.o
obj-$(CONFIG_SND_SOC_ADAV803) += snd-soc-adav803.o
obj-$(CONFIG_SND_SOC_ADS117X) += snd-soc-ads117x.o
obj-$(CONFIG_SND_SOC_AK4104) += snd-soc-ak4104.o
+obj-$(CONFIG_SND_SOC_AK4118) += snd-soc-ak4118.o
obj-$(CONFIG_SND_SOC_AK4458) += snd-soc-ak4458.o
obj-$(CONFIG_SND_SOC_AK4535) += snd-soc-ak4535.o
obj-$(CONFIG_SND_SOC_AK4554) += snd-soc-ak4554.o
diff --git a/sound/soc/codecs/ak4118.c b/sound/soc/codecs/ak4118.c
new file mode 100644
index 000000000000..238ab29f2bf4
--- /dev/null
+++ b/sound/soc/codecs/ak4118.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ak4118.c -- Asahi Kasei ALSA Soc Audio driver
+ *
+ * Copyright 2018 DEVIALET
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include <sound/asoundef.h>
+#include <sound/core.h>
+#include <sound/initval.h>
+#include <sound/soc.h>
+
+#define AK4118_REG_CLK_PWR_CTL 0x00
+#define AK4118_REG_FORMAT_CTL 0x01
+#define AK4118_REG_IO_CTL0 0x02
+#define AK4118_REG_IO_CTL1 0x03
+#define AK4118_REG_INT0_MASK 0x04
+#define AK4118_REG_INT1_MASK 0x05
+#define AK4118_REG_RCV_STATUS0 0x06
+#define AK4118_REG_RCV_STATUS1 0x07
+#define AK4118_REG_RXCHAN_STATUS0 0x08
+#define AK4118_REG_RXCHAN_STATUS1 0x09
+#define AK4118_REG_RXCHAN_STATUS2 0x0a
+#define AK4118_REG_RXCHAN_STATUS3 0x0b
+#define AK4118_REG_RXCHAN_STATUS4 0x0c
+#define AK4118_REG_TXCHAN_STATUS0 0x0d
+#define AK4118_REG_TXCHAN_STATUS1 0x0e
+#define AK4118_REG_TXCHAN_STATUS2 0x0f
+#define AK4118_REG_TXCHAN_STATUS3 0x10
+#define AK4118_REG_TXCHAN_STATUS4 0x11
+#define AK4118_REG_BURST_PREAMB_PC0 0x12
+#define AK4118_REG_BURST_PREAMB_PC1 0x13
+#define AK4118_REG_BURST_PREAMB_PD0 0x14
+#define AK4118_REG_BURST_PREAMB_PD1 0x15
+#define AK4118_REG_QSUB_CTL 0x16
+#define AK4118_REG_QSUB_TRACK 0x17
+#define AK4118_REG_QSUB_INDEX 0x18
+#define AK4118_REG_QSUB_MIN 0x19
+#define AK4118_REG_QSUB_SEC 0x1a
+#define AK4118_REG_QSUB_FRAME 0x1b
+#define AK4118_REG_QSUB_ZERO 0x1c
+#define AK4118_REG_QSUB_ABS_MIN 0x1d
+#define AK4118_REG_QSUB_ABS_SEC 0x1e
+#define AK4118_REG_QSUB_ABS_FRAME 0x1f
+#define AK4118_REG_GPE 0x20
+#define AK4118_REG_GPDR 0x21
+#define AK4118_REG_GPSCR 0x22
+#define AK4118_REG_GPLR 0x23
+#define AK4118_REG_DAT_MASK_DTS 0x24
+#define AK4118_REG_RX_DETECT 0x25
+#define AK4118_REG_STC_DAT_DETECT 0x26
+#define AK4118_REG_RXCHAN_STATUS5 0x27
+#define AK4118_REG_TXCHAN_STATUS5 0x28
+#define AK4118_REG_MAX 0x29
+
+#define AK4118_REG_FORMAT_CTL_DIF0 (1 << 4)
+#define AK4118_REG_FORMAT_CTL_DIF1 (1 << 5)
+#define AK4118_REG_FORMAT_CTL_DIF2 (1 << 6)
+
+struct ak4118_priv {
+ struct regmap *regmap;
+ struct gpio_desc *reset;
+ struct gpio_desc *irq;
+ struct snd_soc_component *component;
+};
+
+static const struct reg_default ak4118_reg_defaults[] = {
+ {AK4118_REG_CLK_PWR_CTL, 0x43},
+ {AK4118_REG_FORMAT_CTL, 0x6a},
+ {AK4118_REG_IO_CTL0, 0x88},
+ {AK4118_REG_IO_CTL1, 0x48},
+ {AK4118_REG_INT0_MASK, 0xee},
+ {AK4118_REG_INT1_MASK, 0xb5},
+ {AK4118_REG_RCV_STATUS0, 0x00},
+ {AK4118_REG_RCV_STATUS1, 0x10},
+ {AK4118_REG_TXCHAN_STATUS0, 0x00},
+ {AK4118_REG_TXCHAN_STATUS1, 0x00},
+ {AK4118_REG_TXCHAN_STATUS2, 0x00},
+ {AK4118_REG_TXCHAN_STATUS3, 0x00},
+ {AK4118_REG_TXCHAN_STATUS4, 0x00},
+ {AK4118_REG_GPE, 0x77},
+ {AK4118_REG_GPDR, 0x00},
+ {AK4118_REG_GPSCR, 0x00},
+ {AK4118_REG_GPLR, 0x00},
+ {AK4118_REG_DAT_MASK_DTS, 0x3f},
+ {AK4118_REG_RX_DETECT, 0x00},
+ {AK4118_REG_STC_DAT_DETECT, 0x00},
+ {AK4118_REG_TXCHAN_STATUS5, 0x00},
+};
+
+static const char * const ak4118_input_select_txt[] = {
+ "RX0", "RX1", "RX2", "RX3", "RX4", "RX5", "RX6", "RX7",
+};
+static SOC_ENUM_SINGLE_DECL(ak4118_insel_enum, AK4118_REG_IO_CTL1, 0x0,
+ ak4118_input_select_txt);
+
+static const struct snd_kcontrol_new ak4118_input_mux_controls =
+ SOC_DAPM_ENUM("Input Select", ak4118_insel_enum);
+
+static const char * const ak4118_iec958_fs_txt[] = {
+ "44100", "48000", "32000", "22050", "11025", "24000", "16000", "88200",
+ "8000", "96000", "64000", "176400", "192000",
+};
+
+static const int ak4118_iec958_fs_val[] = {
+ 0x0, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xA, 0xB, 0xC, 0xE,
+};
+
+static SOC_VALUE_ENUM_SINGLE_DECL(ak4118_iec958_fs_enum, AK4118_REG_RCV_STATUS1,
+ 0x4, 0x4, ak4118_iec958_fs_txt,
+ ak4118_iec958_fs_val);
+
+static struct snd_kcontrol_new ak4118_iec958_controls[] = {
+ SOC_SINGLE("IEC958 Parity Errors", AK4118_REG_RCV_STATUS0, 0, 1, 0),
+ SOC_SINGLE("IEC958 No Audio", AK4118_REG_RCV_STATUS0, 1, 1, 0),
+ SOC_SINGLE("IEC958 PLL Lock", AK4118_REG_RCV_STATUS0, 4, 1, 1),
+ SOC_SINGLE("IEC958 Non PCM", AK4118_REG_RCV_STATUS0, 6, 1, 0),
+ SOC_ENUM("IEC958 Sampling Freq", ak4118_iec958_fs_enum),
+};
+
+static const struct snd_soc_dapm_widget ak4118_dapm_widgets[] = {
+ SND_SOC_DAPM_INPUT("INRX0"),
+ SND_SOC_DAPM_INPUT("INRX1"),
+ SND_SOC_DAPM_INPUT("INRX2"),
+ SND_SOC_DAPM_INPUT("INRX3"),
+ SND_SOC_DAPM_INPUT("INRX4"),
+ SND_SOC_DAPM_INPUT("INRX5"),
+ SND_SOC_DAPM_INPUT("INRX6"),
+ SND_SOC_DAPM_INPUT("INRX7"),
+ SND_SOC_DAPM_MUX("Input Mux", SND_SOC_NOPM, 0, 0,
+ &ak4118_input_mux_controls),
+};
+
+static const struct snd_soc_dapm_route ak4118_dapm_routes[] = {
+ {"Input Mux", "RX0", "INRX0"},
+ {"Input Mux", "RX1", "INRX1"},
+ {"Input Mux", "RX2", "INRX2"},
+ {"Input Mux", "RX3", "INRX3"},
+ {"Input Mux", "RX4", "INRX4"},
+ {"Input Mux", "RX5", "INRX5"},
+ {"Input Mux", "RX6", "INRX6"},
+ {"Input Mux", "RX7", "INRX7"},
+};
+
+
+static int ak4118_set_dai_fmt_master(struct ak4118_priv *ak4118,
+ unsigned int format)
+{
+ int dif;
+
+ switch (format & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_I2S:
+ dif = AK4118_REG_FORMAT_CTL_DIF0 | AK4118_REG_FORMAT_CTL_DIF2;
+ break;
+ case SND_SOC_DAIFMT_RIGHT_J:
+ dif = AK4118_REG_FORMAT_CTL_DIF0 | AK4118_REG_FORMAT_CTL_DIF1;
+ break;
+ case SND_SOC_DAIFMT_LEFT_J:
+ dif = AK4118_REG_FORMAT_CTL_DIF2;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ return dif;
+}
+
+static int ak4118_set_dai_fmt_slave(struct ak4118_priv *ak4118,
+ unsigned int format)
+{
+ int dif;
+
+ switch (format & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_I2S:
+ dif = AK4118_REG_FORMAT_CTL_DIF0 | AK4118_REG_FORMAT_CTL_DIF1 |
+ AK4118_REG_FORMAT_CTL_DIF2;
+ break;
+ case SND_SOC_DAIFMT_LEFT_J:
+ dif = AK4118_REG_FORMAT_CTL_DIF1 | AK4118_REG_FORMAT_CTL_DIF2;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ return dif;
+}
+
+static int ak4118_set_dai_fmt(struct snd_soc_dai *dai,
+ unsigned int format)
+{
+ struct snd_soc_component *component = dai->component;
+ struct ak4118_priv *ak4118 = snd_soc_component_get_drvdata(component);
+ int dif;
+ int ret = 0;
+
+ switch (format & SND_SOC_DAIFMT_MASTER_MASK) {
+ case SND_SOC_DAIFMT_CBM_CFM:
+ /* component is master */
+ dif = ak4118_set_dai_fmt_master(ak4118, format);
+ break;
+ case SND_SOC_DAIFMT_CBS_CFS:
+ /*component is slave */
+ dif = ak4118_set_dai_fmt_slave(ak4118, format);
+ break;
+ default:
+ ret = -ENOTSUPP;
+ goto exit;
+ }
+
+ /* format not supported */
+ if (dif < 0) {
+ ret = dif;
+ goto exit;
+ }
+
+ ret = regmap_update_bits(ak4118->regmap, AK4118_REG_FORMAT_CTL,
+ AK4118_REG_FORMAT_CTL_DIF0 |
+ AK4118_REG_FORMAT_CTL_DIF1 |
+ AK4118_REG_FORMAT_CTL_DIF2, dif);
+ if (ret < 0)
+ goto exit;
+
+exit:
+ return ret;
+}
+
+static int ak4118_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ return 0;
+}
+
+static const struct snd_soc_dai_ops ak4118_dai_ops = {
+ .hw_params = ak4118_hw_params,
+ .set_fmt = ak4118_set_dai_fmt,
+};
+
+static struct snd_soc_dai_driver ak4118_dai = {
+ .name = "ak4118-hifi",
+ .capture = {
+ .stream_name = "Capture",
+ .channels_min = 2,
+ .channels_max = 2,
+ .rates = SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
+ SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
+ SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000 |
+ SNDRV_PCM_RATE_176400 | SNDRV_PCM_RATE_192000,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE |
+ SNDRV_PCM_FMTBIT_S24_3LE |
+ SNDRV_PCM_FMTBIT_S24_LE
+ },
+ .ops = &ak4118_dai_ops,
+};
+
+static irqreturn_t ak4118_irq_handler(int irq, void *data)
+{
+ struct ak4118_priv *ak4118 = data;
+ struct snd_soc_component *component = ak4118->component;
+ struct snd_kcontrol_new *kctl_new;
+ struct snd_kcontrol *kctl;
+ struct snd_ctl_elem_id *id;
+ unsigned int i;
+
+ if (!component)
+ return IRQ_NONE;
+
+ for (i = 0; i < ARRAY_SIZE(ak4118_iec958_controls); i++) {
+ kctl_new = &ak4118_iec958_controls[i];
+ kctl = snd_soc_card_get_kcontrol(component->card,
+ kctl_new->name);
+ if (!kctl)
+ continue;
+ id = &kctl->id;
+ snd_ctl_notify(component->card->snd_card,
+ SNDRV_CTL_EVENT_MASK_VALUE, id);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int ak4118_probe(struct snd_soc_component *component)
+{
+ struct ak4118_priv *ak4118 = snd_soc_component_get_drvdata(component);
+ int ret = 0;
+
+ ak4118->component = component;
+
+ /* release reset */
+ gpiod_set_value(ak4118->reset, 0);
+
+ /* unmask all int1 sources */
+ ret = regmap_write(ak4118->regmap, AK4118_REG_INT1_MASK, 0x00);
+ if (ret < 0) {
+ dev_err(component->dev,
+ "failed to write regmap 0x%x 0x%x: %d\n",
+ AK4118_REG_INT1_MASK, 0x00, ret);
+ return ret;
+ }
+
+ /* rx detect enable on all channels */
+ ret = regmap_write(ak4118->regmap, AK4118_REG_RX_DETECT, 0xff);
+ if (ret < 0) {
+ dev_err(component->dev,
+ "failed to write regmap 0x%x 0x%x: %d\n",
+ AK4118_REG_RX_DETECT, 0xff, ret);
+ return ret;
+ }
+
+ ret = snd_soc_add_component_controls(component, ak4118_iec958_controls,
+ ARRAY_SIZE(ak4118_iec958_controls));
+ if (ret) {
+ dev_err(component->dev,
+ "failed to add component kcontrols: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void ak4118_remove(struct snd_soc_component *component)
+{
+ struct ak4118_priv *ak4118 = snd_soc_component_get_drvdata(component);
+
+ /* hold reset */
+ gpiod_set_value(ak4118->reset, 1);
+}
+
+static const struct snd_soc_component_driver soc_component_drv_ak4118 = {
+ .probe = ak4118_probe,
+ .remove = ak4118_remove,
+ .dapm_widgets = ak4118_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(ak4118_dapm_widgets),
+ .dapm_routes = ak4118_dapm_routes,
+ .num_dapm_routes = ARRAY_SIZE(ak4118_dapm_routes),
+ .idle_bias_on = 1,
+ .use_pmdown_time = 1,
+ .endianness = 1,
+ .non_legacy_dai_naming = 1,
+};
+
+static const struct regmap_config ak4118_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .reg_defaults = ak4118_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(ak4118_reg_defaults),
+
+ .cache_type = REGCACHE_NONE,
+ .max_register = AK4118_REG_MAX - 1,
+};
+
+static int ak4118_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct ak4118_priv *ak4118;
+ int ret;
+
+ ak4118 = devm_kzalloc(&i2c->dev, sizeof(struct ak4118_priv),
+ GFP_KERNEL);
+ if (ak4118 == NULL)
+ return -ENOMEM;
+
+ ak4118->regmap = devm_regmap_init_i2c(i2c, &ak4118_regmap);
+ if (IS_ERR(ak4118->regmap))
+ return PTR_ERR(ak4118->regmap);
+
+ i2c_set_clientdata(i2c, ak4118);
+
+ ak4118->reset = devm_gpiod_get(&i2c->dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ak4118->reset)) {
+ ret = PTR_ERR(ak4118->reset);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&i2c->dev, "Failed to get reset: %d\n", ret);
+ return ret;
+ }
+
+ ak4118->irq = devm_gpiod_get(&i2c->dev, "irq", GPIOD_IN);
+ if (IS_ERR(ak4118->irq)) {
+ ret = PTR_ERR(ak4118->irq);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&i2c->dev, "Failed to get IRQ: %d\n", ret);
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(&i2c->dev, gpiod_to_irq(ak4118->irq),
+ NULL, ak4118_irq_handler,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ "ak4118-irq", ak4118);
+ if (ret < 0) {
+ dev_err(&i2c->dev, "Fail to request_irq: %d\n", ret);
+ return ret;
+ }
+
+ return snd_soc_register_component(&i2c->dev, &soc_component_drv_ak4118,
+ &ak4118_dai, 1);
+}
+
+static int ak4118_i2c_remove(struct i2c_client *i2c)
+{
+ snd_soc_unregister_component(&i2c->dev);
+ return 0;
+}
+
+static const struct of_device_id ak4118_of_match[] = {
+ { .compatible = "asahi-kasei,ak4118", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ak4118_of_match);
+
+static const struct i2c_device_id ak4118_id_table[] = {
+ { "ak4118", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, ak4118_id_table);
+
+static struct i2c_driver ak4118_i2c_driver = {
+ .driver = {
+ .name = "ak4118",
+ .of_match_table = of_match_ptr(ak4118_of_match),
+ },
+ .id_table = ak4118_id_table,
+ .probe = ak4118_i2c_probe,
+ .remove = ak4118_i2c_remove,
+};
+
+module_i2c_driver(ak4118_i2c_driver);
+
+MODULE_DESCRIPTION("Asahi Kasei AK4118 ALSA SoC driver");
+MODULE_AUTHOR("Adrien Charruel <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.19.1



2018-11-14 12:19:48

by Clément Péron

[permalink] [raw]
Subject: [PATCH v2 2/2] ASoC: dt-bindings: add bindings for AK4118 transceiver

Document the bindings for AK4118 S/PDIF transceiver

Signed-off-by: Clément Péron <[email protected]>
---
.../devicetree/bindings/sound/ak4118.txt | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/ak4118.txt

diff --git a/Documentation/devicetree/bindings/sound/ak4118.txt b/Documentation/devicetree/bindings/sound/ak4118.txt
new file mode 100644
index 000000000000..6e11a2f7404c
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ak4118.txt
@@ -0,0 +1,22 @@
+AK4118 S/PDIF transceiver
+
+This device supports I2C mode.
+
+Required properties:
+
+- compatible : "asahi-kasei,ak4118"
+- reg : The I2C address of the device for I2C
+- reset-gpios: A GPIO specifier for the reset pin
+- irq-gpios: A GPIO specifier for the IRQ pin
+
+Example:
+
+&i2c {
+ ak4118: ak4118@13 {
+ #sound-dai-cells = <0>;
+ compatible = "asahi-kasei,ak4118";
+ reg = <0x13>;
+ reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>
+ irq-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+ };
+};
--
2.19.1


2018-11-14 15:08:42

by Ricard Wanderlof

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: ak4118: Add support for AK4118 S/PDIF transceiver


On Wed, 14 Nov 2018, Clément Péron wrote:

> From: Adrien Charruel <[email protected]>
>
> The AK4118A is a digital audio transceiver supporting 8 input channels
> at 192kHz and with 24bits resolution.
> It converts the S/PDIF signal to I2S format and is configurable over I2C.
>
> This driver introduce a minimal support of the AK4118, like selecting the
> input channel, reading input frequency and detecting some errors.

I'm curious, from what it seems, there is no checking that the input
sample rate actually matches the configured sample rate? It's perfectly
understandable for a driver which has 'minimal support', but it's
something a user must be aware of; for instance if someone does

arecord -r 48000 output.wav

when the input data actually has a rate of 44100 Hz then the file will be
written with a header specifying 44100 Hz but the data will actually be
48000 Hz.

It's not an objection on my part, just an observation. I don't know how
ALSA could enforce this in any way; ideally (barring automatic sample rate
conversion) one would want an error message that the sample rate specified
when opening the stream does not match the sample rate of the S/PDIF input
data.

/Ricard
--
Ricard Wolf Wanderlof ricardw(at)axis.com
Axis Communications AB, Lund, Sweden http://www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30

2018-11-14 15:26:48

by Clément Péron

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: ak4118: Add support for AK4118 S/PDIF transceiver

Hi Ricard,

On Wed, 14 Nov 2018 at 16:07, Ricard Wanderlof
<[email protected]> wrote:
>
>
> On Wed, 14 Nov 2018, Clément Péron wrote:
>
> > From: Adrien Charruel <[email protected]>
> >
> > The AK4118A is a digital audio transceiver supporting 8 input channels
> > at 192kHz and with 24bits resolution.
> > It converts the S/PDIF signal to I2S format and is configurable over I2C.
> >
> > This driver introduce a minimal support of the AK4118, like selecting the
> > input channel, reading input frequency and detecting some errors.
>
> I'm curious, from what it seems, there is no checking that the input
> sample rate actually matches the configured sample rate? It's perfectly
> understandable for a driver which has 'minimal support', but it's
> something a user must be aware of; for instance if someone does
>
> arecord -r 48000 output.wav
>
> when the input data actually has a rate of 44100 Hz then the file will be
> written with a header specifying 44100 Hz but the data will actually be
> 48000 Hz.
>
> It's not an objection on my part, just an observation. I don't know how
> ALSA could enforce this in any way; ideally (barring automatic sample rate
> conversion) one would want an error message that the sample rate specified
> when opening the stream does not match the sample rate of the S/PDIF input
> data.

I'm quite new with linux audio driver but adding in ak4118_hw_params a
check like if params_rate(params) == ak4118_rate should do the job no
?

Regards,
Clement

>
> /Ricard
> --
> Ricard Wolf Wanderlof ricardw(at)axis.com
> Axis Communications AB, Lund, Sweden http://www.axis.com
> Phone +46 46 272 2016 Fax +46 46 13 61 30

2018-11-15 16:50:25

by Ricard Wanderlof

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: ak4118: Add support for AK4118 S/PDIF transceiver


On Wed, 14 Nov 2018, Clément Péron wrote:

> > > The AK4118A is a digital audio transceiver supporting 8 input channels
> > > at 192kHz and with 24bits resolution.
> > > It converts the S/PDIF signal to I2S format and is configurable over I2C.
> > >
> > > This driver introduce a minimal support of the AK4118, like selecting the
> > > input channel, reading input frequency and detecting some errors.
> >
> > I'm curious, from what it seems, there is no checking that the input
> > sample rate actually matches the configured sample rate? It's perfectly
> > understandable for a driver which has 'minimal support', but it's
> > something a user must be aware of; for instance if someone does
> >
> > arecord -r 48000 output.wav
> >
> > when the input data actually has a rate of 44100 Hz then the file will be
> > written with a header specifying 44100 Hz but the data will actually be
> > 48000 Hz.
> >
> > It's not an objection on my part, just an observation. I don't know how
> > ALSA could enforce this in any way; ideally (barring automatic sample rate
> > conversion) one would want an error message that the sample rate specified
> > when opening the stream does not match the sample rate of the S/PDIF input
> > data.
>
> I'm quite new with linux audio driver but adding in ak4118_hw_params a
> check like if params_rate(params) == ak4118_rate should do the job no

It will if the SPDIF stream is already running when the driver is started
up, but if for some reason the sample rate changes or the SPDIF source is
not transmitting anything when the stream is started it won't work to do
it hw_params. A changing sample rate might be an unusual usecase, but one
could imagine having an SPDIF source which is silent (e.g. not even
plugged in) when the stream is set up.

Like I said, it might not be a problem in many applications. And I'm not
really sure how to solve it. Perhaps someone else knows. ALSA likes to be
in control of parameters such as the sample rate, but it can't with an
external device controlling the sample rate of the SPDIF receiver.

/Ricard
--
Ricard Wolf Wanderlof ricardw(at)axis.com
Axis Communications AB, Lund, Sweden http://www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30

2018-11-17 15:52:39

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ASoC: dt-bindings: add bindings for AK4118 transceiver

On Wed, Nov 14, 2018 at 01:16:42PM +0100, Cl?ment P?ron wrote:
> Document the bindings for AK4118 S/PDIF transceiver
>
> Signed-off-by: Cl?ment P?ron <[email protected]>
> ---
> .../devicetree/bindings/sound/ak4118.txt | 22 +++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/ak4118.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/ak4118.txt b/Documentation/devicetree/bindings/sound/ak4118.txt
> new file mode 100644
> index 000000000000..6e11a2f7404c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/ak4118.txt
> @@ -0,0 +1,22 @@
> +AK4118 S/PDIF transceiver
> +
> +This device supports I2C mode.
> +
> +Required properties:
> +
> +- compatible : "asahi-kasei,ak4118"
> +- reg : The I2C address of the device for I2C
> +- reset-gpios: A GPIO specifier for the reset pin
> +- irq-gpios: A GPIO specifier for the IRQ pin

This should use 'interrupts' instead.

> +
> +Example:
> +
> +&i2c {
> + ak4118: ak4118@13 {

spdif@13 ? Node names should be based on the class of device, but
there's not really one defined for this.

> + #sound-dai-cells = <0>;
> + compatible = "asahi-kasei,ak4118";
> + reg = <0x13>;
> + reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>
> + irq-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
> + };
> +};
> --
> 2.19.1
>

2018-11-19 10:56:47

by Clément Péron

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] ASoC: dt-bindings: add bindings for AK4118 transceiver

Hi Rob,

On Sat, 17 Nov 2018 at 16:51, Rob Herring <[email protected]> wrote:
>
> On Wed, Nov 14, 2018 at 01:16:42PM +0100, Clément Péron wrote:
> > Document the bindings for AK4118 S/PDIF transceiver
> >
> > Signed-off-by: Clément Péron <[email protected]>
> > ---
> > .../devicetree/bindings/sound/ak4118.txt | 22 +++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/ak4118.txt
> >
> > diff --git a/Documentation/devicetree/bindings/sound/ak4118.txt b/Documentation/devicetree/bindings/sound/ak4118.txt
> > new file mode 100644
> > index 000000000000..6e11a2f7404c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/ak4118.txt
> > @@ -0,0 +1,22 @@
> > +AK4118 S/PDIF transceiver
> > +
> > +This device supports I2C mode.
> > +
> > +Required properties:
> > +
> > +- compatible : "asahi-kasei,ak4118"
> > +- reg : The I2C address of the device for I2C
> > +- reset-gpios: A GPIO specifier for the reset pin
> > +- irq-gpios: A GPIO specifier for the IRQ pin
>
> This should use 'interrupts' instead.
Yes, this will also simplify the driver.

>
> > +
> > +Example:
> > +
> > +&i2c {
> > + ak4118: ak4118@13 {
>
> spdif@13 ? Node names should be based on the class of device, but
> there's not really one defined for this.

Ok I will use spdif.

Thanks for the comments,
Clement

>
> > + #sound-dai-cells = <0>;
> > + compatible = "asahi-kasei,ak4118";
> > + reg = <0x13>;
> > + reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>
> > + irq-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
> > + };
> > +};
> > --
> > 2.19.1
> >