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

[PATCH 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>
---
 sound/soc/codecs/Kconfig  |   6 +
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/ak4118.c | 449 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 457 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..a88e7d6a65a3
--- /dev/null
+++ b/sound/soc/codecs/ak4118.c
@@ -0,0 +1,449 @@
+// 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, 0,
+			    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_texts[] = {
+	"44100",
+	"Reserved",
+	"48000",
+	"32000",
+	"22050",
+	"11025",
+	"24000",
+	"16000",
+	"88200",
+	"8000",
+	"96000",
+	"64000",
+	"176400",
+	"Unused",
+	"192000",
+};
+
+static const struct soc_enum ak4118_iec958_fs_enum =
+	SOC_ENUM_SINGLE(AK4118_REG_RCV_STATUS1, 4,
+			ARRAY_SIZE(ak4118_iec958_fs_texts),
+			ak4118_iec958_fs_texts);
+
+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;
+	}
+
+	ret = request_threaded_irq(gpiod_to_irq(ak4118->irq), NULL,
+				   ak4118_irq_handler, IRQF_TRIGGER_RISING |
+				   IRQF_ONESHOT, "ak4118-irq", ak4118);
+	if (ret < 0) {
+		dev_err(component->dev, "Fail to request_irq: %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);
+
+	free_irq(gpiod_to_irq(ak4118->irq), ak4118);
+}
+
+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;
+	}
+
+	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 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: [PATCH 1/2] ASoC: ak4118: Add support for AK4118 S/PDIF transceiver

[Mark Brown]

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

On Thu, Nov 08, 2018 at 01:49:33PM +0100, Clément Péron wrote:

This looks mostly good, a few small things below but nothing too major:

> --- /dev/null
> +++ b/sound/soc/codecs/ak4118.c
> @@ -0,0 +1,449 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ak4118.c  --  Asahi Kasei ALSA Soc Audio driver
> + *

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

> +static const char * const ak4118_iec958_fs_texts[] = {
> +	"44100",
> +	"Reserved",

If you use a _VALUE_ENUM_SINGLE you can hide the reserved/invalid values
from userspace which is easier to use.

> +	ret = request_threaded_irq(gpiod_to_irq(ak4118->irq), NULL,
> +				   ak4118_irq_handler, IRQF_TRIGGER_RISING |
> +				   IRQF_ONESHOT, "ak4118-irq", ak4118);
> +	if (ret < 0) {
> +		dev_err(component->dev, "Fail to request_irq: %d\n", ret);
> +		return ret;
> +	}

You should request resources in the device level probe, there's no point
in requesting and releasing the interrupt like this.

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

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

[Clément Péron]

Hi Mark,

On Tue, 13 Nov 2018 at 23:51, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 08, 2018 at 01:49:33PM +0100, Clément Péron wrote:
>
> This looks mostly good, a few small things below but nothing too major:
>
> > --- /dev/null
> > +++ b/sound/soc/codecs/ak4118.c
> > @@ -0,0 +1,449 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ak4118.c  --  Asahi Kasei ALSA Soc Audio driver
> > + *
>
> Please make the entire comment a C++ one so this looks more intentional.

I check from other files and also this article says :
"For normal C source files, the string will be a comment using the
"//" syntax; header files, instead, use traditional (/* */) comments
for reasons related to tooling"
https://lwn.net/Articles/739183/

>
> > +static const char * const ak4118_iec958_fs_texts[] = {
> > +     "44100",
> > +     "Reserved",
>
> If you use a _VALUE_ENUM_SINGLE you can hide the reserved/invalid values
> from userspace which is easier to use.

Ok will change to SOC_VALUE_ENUM_SINGLE_DECL

>
> > +     ret = request_threaded_irq(gpiod_to_irq(ak4118->irq), NULL,
> > +                                ak4118_irq_handler, IRQF_TRIGGER_RISING |
> > +                                IRQF_ONESHOT, "ak4118-irq", ak4118);
> > +     if (ret < 0) {
> > +             dev_err(component->dev, "Fail to request_irq: %d\n", ret);
> > +             return ret;
> > +     }
>
> You should request resources in the device level probe, there's no point
> in requesting and releasing the interrupt like this.

Ok, will change to devm_request_threaded_irq and remove the irq_free

Thanks for the review,
Regards,
Clément

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

[Mark Brown]

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

On Wed, Nov 14, 2018 at 11:55:17AM +0100, Clément Péron wrote:
> On Tue, 13 Nov 2018 at 23:51, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Nov 08, 2018 at 01:49:33PM +0100, Clément Péron wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * ak4118.c  --  Asahi Kasei ALSA Soc Audio driver
> > > + *

> > Please make the entire comment a C++ one so this looks more intentional.

> I check from other files and also this article says :
> "For normal C source files, the string will be a comment using the
> "//" syntax; header files, instead, use traditional (/* */) comments
> for reasons related to tooling"
> https://lwn.net/Articles/739183/

Right, and what I said was that you should make the entire comment a C++
one which means that the magic line will be C++ as required.

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

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

[Clément Péron]

Hi Mark,

On Wed, 14 Nov 2018 at 18:31, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Nov 14, 2018 at 11:55:17AM +0100, Clément Péron wrote:
> > On Tue, 13 Nov 2018 at 23:51, Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Nov 08, 2018 at 01:49:33PM +0100, Clément Péron wrote:
>
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * ak4118.c  --  Asahi Kasei ALSA Soc Audio driver
> > > > + *
>
> > > Please make the entire comment a C++ one so this looks more intentional.
>
> > I check from other files and also this article says :
> > "For normal C source files, the string will be a comment using the
> > "//" syntax; header files, instead, use traditional (/* */) comments
> > for reasons related to tooling"
> > https://lwn.net/Articles/739183/
>
> Right, and what I said was that you should make the entire comment a C++
> one which means that the magic line will be C++ as required.

Yes, sorry I was confused between C++ and C99,
Regards,
Clement