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

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

[Clément Péron]

From: Adrien Charruel <adrien.charruel@devialet.com>

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 <adrien.charruel@devialet.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---

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 <adrien.charruel@devialet.com>");
+MODULE_LICENSE("GPL");
-- 
2.19.1

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

[Clément Péron]

Document the bindings for AK4118 S/PDIF transceiver

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 .../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

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

[Ricard Wanderlof]

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

> From: Adrien Charruel <adrien.charruel@devialet.com>
> 
> 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            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

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

[Clément Péron]

Hi Ricard,

On Wed, 14 Nov 2018 at 16:07, Ricard Wanderlof
<ricard.wanderlof@axis.com> wrote:
>
>
> On Wed, 14 Nov 2018, Clément Péron wrote:
>
> > From: Adrien Charruel <adrien.charruel@devialet.com>
> >
> > 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            www.axis.com
> Phone +46 46 272 2016                           Fax +46 46 13 61 30

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

[Ricard Wanderlof]

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            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

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

[Rob Herring]

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 <peron.clem@gmail.com>
> ---
>  .../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
> 

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

[Clément Péron]

Hi Rob,

On Sat, 17 Nov 2018 at 16:51, Rob Herring <robh@kernel.org> 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 <peron.clem@gmail.com>
> > ---
> >  .../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
> >