[PATCH 1/3] sound: Add hikey960 i2s audio driver

[PATCH 1/3] sound: Add hikey960 i2s audio driver

[Pengcheng Li]

From: Youlin Wang <wangyoulin1@hisilicon.com>

Add i2s driver for hisi3660 soc found on the hikey960 board.
Add conpile line in make file.
Technical support by Guangke Ji.

Signed-off-by: Youlin Wang <wangyoulin1@hisilicon.com>
Signed-off-by: Tanglei Han <hantanglei@huawei.com>
Signed-off-by: Guangke Ji <jiguangke@huawei.com>
Signed-off-by: Feng Chen <puck.chen@hisilicon.com>
Signed-off-by: Kaihua Zhong <zhongkaihua@huawei.com>
Signed-off-by: Jun Chen <chenjun14@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Yiping Xu <xuyiping@hisilicon.com>
Signed-off-by: Pengcheng Li <lipengcheng8@huawei.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
---
 sound/soc/hisilicon/Kconfig      |   8 +-
 sound/soc/hisilicon/Makefile     |   1 +
 sound/soc/hisilicon/hi3660-i2s.c | 448 +++++++++++++++++++++++++++++++++++++++
 sound/soc/hisilicon/hi3660-i2s.h |  99 +++++++++
 4 files changed, 555 insertions(+), 1 deletion(-)
 create mode 100644 sound/soc/hisilicon/hi3660-i2s.c
 create mode 100644 sound/soc/hisilicon/hi3660-i2s.h

diff --git a/sound/soc/hisilicon/Kconfig b/sound/soc/hisilicon/Kconfig
index 4356d5a..b023ef9 100644
--- a/sound/soc/hisilicon/Kconfig
+++ b/sound/soc/hisilicon/Kconfig
@@ -1,5 +1,11 @@
 config SND_I2S_HI6210_I2S
-	tristate "Hisilicon I2S controller"
+	tristate "Hisilicon Hi6210 I2S controller"
+	select SND_SOC_GENERIC_DMAENGINE_PCM
+	help
+	  Hisilicon I2S
+
+config SND_I2S_HI3660_I2S
+	tristate "Hisilicon 960 I2S controller"
 	select SND_SOC_GENERIC_DMAENGINE_PCM
 	help
 	  Hisilicon I2S
diff --git a/sound/soc/hisilicon/Makefile b/sound/soc/hisilicon/Makefile
index e8095e2..6800516 100644
--- a/sound/soc/hisilicon/Makefile
+++ b/sound/soc/hisilicon/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_SND_I2S_HI6210_I2S) += hi6210-i2s.o
+obj-$(CONFIG_SND_I2S_HI3660_I2S) += hi3660-i2s.o
diff --git a/sound/soc/hisilicon/hi3660-i2s.c b/sound/soc/hisilicon/hi3660-i2s.c
new file mode 100644
index 0000000..b894007
--- /dev/null
+++ b/sound/soc/hisilicon/hi3660-i2s.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * linux/sound/soc/hisilicon/hi3660-i2s.c
+ *
+ * I2S IP driver for hi3660.
+ *
+ * Copyright (c) 2001-2021, Huawei Tech. Co., Ltd.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/jiffies.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/initval.h>
+#include <sound/soc.h>
+#include <linux/interrupt.h>
+#include <linux/reset.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/reset-controller.h>
+#include <linux/clk.h>
+#include <linux/regulator/consumer.h>
+
+#include "hi3660-i2s.h"
+
+struct hi3660_i2s {
+	struct device *dev;
+	struct reset_control *rc;
+	int clocks;
+	struct regulator *regu_asp;
+	struct pinctrl *pctrl;
+	struct pinctrl_state *pin_default;
+	struct pinctrl_state *pin_idle;
+	struct clk *asp_subsys_clk;
+	struct snd_soc_dai_driver dai;
+	void __iomem *base;
+	void __iomem *base_syscon;
+	phys_addr_t base_phys;
+	struct snd_dmaengine_dai_dma_data dma_data[2];
+	spinlock_t lock;
+	int rate;
+	int format;
+	int bits;
+	int channels;
+	u32 master;
+	u32 status;
+};
+
+static void update_bits(struct hi3660_i2s *i2s, u32 ofs, u32 reset, u32 set)
+{
+	u32 val = readl(i2s->base + ofs) & ~reset;
+
+	writel(val | set, i2s->base + ofs);
+}
+
+static void update_bits_syscon(struct hi3660_i2s *i2s,
+			u32 ofs, u32 reset, u32 set)
+{
+	u32 val = readl(i2s->base_syscon + ofs) & ~reset;
+
+	writel(val | set, i2s->base_syscon + ofs);
+}
+
+static int enable_format(struct hi3660_i2s *i2s,
+			       struct snd_pcm_substream *substream)
+{
+	switch (i2s->format & SND_SOC_DAIFMT_MASTER_MASK) {
+	case SND_SOC_DAIFMT_CBM_CFM:
+		i2s->master = false;
+		update_bits_syscon(i2s, HI_ASP_CFG_R_CLK_SEL_REG,
+				0, HI_ASP_CFG_R_CLK_SEL_EN);
+		break;
+	case SND_SOC_DAIFMT_CBS_CFS:
+		i2s->master = true;
+		update_bits_syscon(i2s, HI_ASP_CFG_R_CLK_SEL_REG,
+				HI_ASP_CFG_R_CLK_SEL_EN, 0);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int startup(struct snd_pcm_substream *substream,
+		     struct snd_soc_dai *cpu_dai)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
+
+	/* deassert reset on sio_bt*/
+	update_bits_syscon(i2s, HI_ASP_CFG_R_RST_CTRLDIS_REG,
+			0, BIT(2)|BIT(6)|BIT(8)|BIT(16));
+
+	/* enable clk before frequency division */
+	update_bits_syscon(i2s, HI_ASP_CFG_R_GATE_EN_REG,
+			0, BIT(5)|BIT(6));
+
+	/* enable frequency division */
+	update_bits_syscon(i2s, HI_ASP_CFG_R_GATE_CLKDIV_EN_REG,
+			0, BIT(2)|BIT(5));
+
+	/* select clk */
+	update_bits_syscon(i2s, HI_ASP_CFG_R_CLK_SEL_REG,
+			HI_ASP_MASK, HI_ASP_CFG_R_CLK_SEL);
+
+	/* select clk_div */
+	update_bits_syscon(i2s, HI_ASP_CFG_R_CLK1_DIV_REG,
+			HI_ASP_MASK, HI_ASP_CFG_R_CLK1_DIV_SEL);
+	update_bits_syscon(i2s, HI_ASP_CFG_R_CLK4_DIV_REG,
+			HI_ASP_MASK, HI_ASP_CFG_R_CLK4_DIV_SEL);
+	update_bits_syscon(i2s, HI_ASP_CFG_R_CLK6_DIV_REG,
+			HI_ASP_MASK, HI_ASP_CFG_R_CLK6_DIV_SEL);
+
+	/* sio config */
+	update_bits(i2s, HI_ASP_SIO_MODE_REG, HI_ASP_MASK, 0x0);
+	update_bits(i2s, HI_ASP_SIO_DATA_WIDTH_SET_REG, HI_ASP_MASK, 0x9);
+	update_bits(i2s, HI_ASP_SIO_I2S_POS_MERGE_EN_REG, HI_ASP_MASK, 0x1);
+	update_bits(i2s, HI_ASP_SIO_I2S_START_POS_REG, HI_ASP_MASK, 0x0);
+
+	return 0;
+}
+
+static void shutdown(struct snd_pcm_substream *substream,
+		       struct snd_soc_dai *cpu_dai)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
+
+	if (!IS_ERR_OR_NULL(i2s->asp_subsys_clk))
+		clk_disable_unprepare(i2s->asp_subsys_clk);
+}
+
+static void txctrl(struct snd_soc_dai *cpu_dai, int on)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
+
+	spin_lock(&i2s->lock);
+
+	if (on) {
+		/* enable SIO TX */
+		update_bits(i2s, HI_ASP_SIO_CT_SET_REG, 0,
+			HI_ASP_SIO_TX_ENABLE |
+			HI_ASP_SIO_TX_DATA_MERGE |
+			HI_ASP_SIO_TX_FIFO_THRESHOLD |
+			HI_ASP_SIO_RX_ENABLE |
+			HI_ASP_SIO_RX_DATA_MERGE |
+			HI_ASP_SIO_RX_FIFO_THRESHOLD);
+	} else {
+		/* disable SIO TX */
+		update_bits(i2s, HI_ASP_SIO_CT_CLR_REG, 0,
+			HI_ASP_SIO_TX_ENABLE | HI_ASP_SIO_RX_ENABLE);
+	}
+	spin_unlock(&i2s->lock);
+}
+
+static void rxctrl(struct snd_soc_dai *cpu_dai, int on)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
+
+	spin_lock(&i2s->lock);
+	if (on)
+		/* enable SIO RX */
+		update_bits(i2s, HI_ASP_SIO_CT_SET_REG, 0,
+			HI_ASP_SIO_TX_ENABLE |
+			HI_ASP_SIO_TX_DATA_MERGE |
+			HI_ASP_SIO_TX_FIFO_THRESHOLD |
+			HI_ASP_SIO_RX_ENABLE |
+			HI_ASP_SIO_RX_DATA_MERGE |
+			HI_ASP_SIO_RX_FIFO_THRESHOLD);
+	else
+		/* disable SIO RX */
+		update_bits(i2s, HI_ASP_SIO_CT_CLR_REG, 0,
+			HI_ASP_SIO_TX_ENABLE | HI_ASP_SIO_RX_ENABLE);
+	spin_unlock(&i2s->lock);
+}
+
+static int set_sysclk(struct snd_soc_dai *cpu_dai,
+			     int clk_id, unsigned int freq, int dir)
+{
+	return 0;
+}
+
+static int set_format(struct snd_soc_dai *cpu_dai, unsigned int fmt)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
+
+	i2s->format = fmt;
+	i2s->master = (i2s->format & SND_SOC_DAIFMT_MASTER_MASK) ==
+		      SND_SOC_DAIFMT_CBS_CFS;
+
+	return 0;
+}
+
+static int hw_params(struct snd_pcm_substream *substream,
+			    struct snd_pcm_hw_params *params,
+			    struct snd_soc_dai *cpu_dai)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
+	struct snd_dmaengine_dai_dma_data *dma_data;
+
+	dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream);
+
+	enable_format(i2s, substream);
+
+	dma_data->maxburst = 4;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		dma_data->addr = i2s->base_phys +
+			HI_ASP_SIO_I2S_DUAL_TX_CHN_REG;
+	else
+		dma_data->addr = i2s->base_phys +
+			HI_ASP_SIO_I2S_DUAL_RX_CHN_REG;
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_U16_LE:
+	case SNDRV_PCM_FORMAT_S16_LE:
+		i2s->bits = 16;
+		dma_data->addr_width = 4;
+		break;
+
+	case SNDRV_PCM_FORMAT_U24_LE:
+	case SNDRV_PCM_FORMAT_S24_LE:
+		i2s->bits = 32;
+		dma_data->addr_width = 4;
+		break;
+	default:
+		dev_err(cpu_dai->dev, "Bad format\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int trigger(struct snd_pcm_substream *substream, int cmd,
+			  struct snd_soc_dai *cpu_dai)
+{
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+			rxctrl(cpu_dai, 1);
+		else
+			txctrl(cpu_dai, 1);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+			rxctrl(cpu_dai, 0);
+		else
+			txctrl(cpu_dai, 0);
+		break;
+	default:
+		dev_err(cpu_dai->dev, "unknown cmd\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int dai_probe(struct snd_soc_dai *dai)
+{
+	struct hi3660_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+	snd_soc_dai_init_dma_data(dai,
+		&i2s->dma_data[SNDRV_PCM_STREAM_PLAYBACK],
+		&i2s->dma_data[SNDRV_PCM_STREAM_CAPTURE]);
+
+	return 0;
+}
+
+
+static struct snd_soc_dai_ops dai_ops = {
+	.trigger	= trigger,
+	.hw_params	= hw_params,
+	.set_fmt	= set_format,
+	.set_sysclk	= set_sysclk,
+	.startup	= startup,
+	.shutdown	= shutdown,
+};
+
+static struct snd_soc_dai_driver dai_init = {
+	.name = "hi3660_i2s",
+	.probe = dai_probe,
+	.playback = {
+		.channels_min = 2,
+		.channels_max = 2,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE |
+			   SNDRV_PCM_FMTBIT_U16_LE,
+		.rates = SNDRV_PCM_RATE_48000,
+	},
+	.capture = {
+		.channels_min = 2,
+		.channels_max = 2,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE |
+			   SNDRV_PCM_FMTBIT_U16_LE,
+		.rates = SNDRV_PCM_RATE_48000,
+	},
+	.ops = &dai_ops,
+};
+
+static const struct snd_soc_component_driver component_driver = {
+	.name = "hi3660_i2s",
+};
+
+#include <sound/dmaengine_pcm.h>
+
+static const struct snd_pcm_hardware sound_hardware = {
+	.info = SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_PAUSE |
+		SNDRV_PCM_INFO_RESUME |
+		SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_HALF_DUPLEX,
+	.period_bytes_min = 4096,
+	.period_bytes_max = 4096,
+	.periods_min = 4,
+	.periods_max = UINT_MAX,
+	.buffer_bytes_max = SIZE_MAX,
+};
+
+static const struct snd_dmaengine_pcm_config dmaengine_pcm_config = {
+	.pcm_hardware = &sound_hardware,
+	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+	.prealloc_buffer_size = 64 * 1024,
+};
+
+static int hi3660_i2s_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hi3660_i2s *i2s;
+	struct resource *res;
+	int ret;
+
+	i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
+	if (!i2s)
+		return -ENOMEM;
+
+	i2s->dev = dev;
+	spin_lock_init(&i2s->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -ENODEV;
+		return ret;
+	}
+	i2s->base_phys = (phys_addr_t)res->start;
+
+	i2s->dai = dai_init;
+	dev_set_drvdata(&pdev->dev, i2s);
+
+	i2s->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(i2s->base)) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		ret = PTR_ERR(i2s->base);
+		return ret;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		ret = -ENODEV;
+		return ret;
+	}
+	i2s->base_syscon = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(i2s->base_syscon)) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		ret = PTR_ERR(i2s->base_syscon);
+		return ret;
+	}
+
+	/* i2s iomux config */
+	i2s->pctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(i2s->pctrl)) {
+		dev_err(dev, "could not get pinctrl\n");
+		ret = -EIO;
+		return ret;
+	}
+
+	i2s->pin_default = pinctrl_lookup_state(i2s->pctrl,
+					PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(i2s->pin_default)) {
+		dev_err(dev,
+			"could not get default state (%li)\n",
+			PTR_ERR(i2s->pin_default));
+		ret = -EIO;
+		return ret;
+	}
+
+	if (pinctrl_select_state(i2s->pctrl, i2s->pin_default)) {
+		dev_err(dev, "could not set pins to default state\n");
+		ret = -EIO;
+		return ret;
+	}
+
+	ret = devm_snd_dmaengine_pcm_register(&pdev->dev,
+				&dmaengine_pcm_config, 0);
+	if (ret)
+		return ret;
+
+	ret = snd_soc_register_component(&pdev->dev, &component_driver,
+				&i2s->dai, 1);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register dai\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int hi3660_i2s_remove(struct platform_device *pdev)
+{
+	struct hi3660_i2s *i2s = dev_get_drvdata(&pdev->dev);
+
+	snd_soc_unregister_component(&pdev->dev);
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	pinctrl_put(i2s->pctrl);
+
+	return 0;
+}
+
+static const struct of_device_id dt_ids[] = {
+	{ .compatible = "hisilicon,hi3660-i2s-1.0" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, dt_ids);
+
+static struct platform_driver local_platform_driver = {
+	.probe = hi3660_i2s_probe,
+	.remove = hi3660_i2s_remove,
+	.driver = {
+		.name = "hi3660_i2s",
+		.owner = THIS_MODULE,
+		.of_match_table = dt_ids,
+	},
+};
+
+module_platform_driver(local_platform_driver);
+
+MODULE_DESCRIPTION("Hisilicon I2S driver");
+MODULE_AUTHOR("Guangke Ji <j00209069@notesmail.huawei.com>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/hisilicon/hi3660-i2s.h b/sound/soc/hisilicon/hi3660-i2s.h
new file mode 100644
index 0000000..1874855
--- /dev/null
+++ b/sound/soc/hisilicon/hi3660-i2s.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0+
+ *
+ * linux/sound/soc/hisilicon/hi3660-i2s.c
+ *
+ * I2S IP driver for hi3660.
+ *
+ * Copyright (c) 2001-2021, Huawei Tech. Co., Ltd.
+ *
+ */
+
+#ifndef _HI3660_I2S_H
+#define _HI3660_I2S_H
+
+enum hisi_bits {
+	HII2S_BITS_16,
+	HII2S_BITS_18,
+	HII2S_BITS_20,
+	HII2S_BITS_24,
+};
+
+enum hisi_i2s_rates {
+	HII2S_FS_RATE_8KHZ = 0,
+	HII2S_FS_RATE_16KHZ = 1,
+	HII2S_FS_RATE_32KHZ = 2,
+	HII2S_FS_RATE_48KHZ = 4,
+	HII2S_FS_RATE_96KHZ = 5,
+	HII2S_FS_RATE_192KHZ = 6,
+};
+
+#define HI_ASP_CFG_R_RST_CTRLEN_REG		0x0
+#define HI_ASP_CFG_R_RST_CTRLDIS_REG		0x4
+#define HI_ASP_CFG_R_GATE_EN_REG		0xC
+#define HI_ASP_CFG_R_GATE_DIS_REG		0x10
+#define HI_ASP_CFG_R_GATE_CLKEN_REG		0x14
+#define HI_ASP_CFG_R_GATE_CLKSTAT_REG		0x18
+#define HI_ASP_CFG_R_GATE_CLKDIV_EN_REG		0x1C
+#define HI_ASP_CFG_R_CLK1_DIV_REG		0x20
+#define HI_ASP_CFG_R_CLK2_DIV_REG		0x24
+#define HI_ASP_CFG_R_CLK3_DIV_REG		0x28
+#define HI_ASP_CFG_R_CLK4_DIV_REG		0x2C
+#define HI_ASP_CFG_R_CLK5_DIV_REG		0x30
+#define HI_ASP_CFG_R_CLK6_DIV_REG		0x34
+#define HI_ASP_CFG_R_CLK_SEL_REG		0x38
+#define HI_ASP_CFG_R_SEC_REG			0x100
+
+
+#define HI_ASP_SIO_VERSION_REG			(0x3C)
+#define HI_ASP_SIO_MODE_REG			(0x40)
+#define HI_ASP_SIO_INTSTATUS_REG		(0x44)
+#define HI_ASP_SIO_INTCLR_REG			(0x48)
+#define HI_ASP_SIO_I2S_LEFT_XD_REG		(0x4C)
+#define HI_ASP_SIO_I2S_RIGHT_XD_REG		(0x50)
+#define HI_ASP_SIO_I2S_LEFT_RD_REG		(0x54)
+#define HI_ASP_SIO_I2S_RIGHT_RD_REG		(0x58)
+#define HI_ASP_SIO_CT_SET_REG			(0x5C)
+#define HI_ASP_SIO_CT_CLR_REG			(0x60)
+#define HI_ASP_SIO_RX_STA_REG			(0x68)
+#define HI_ASP_SIO_TX_STA_REG			(0x6C)
+#define HI_ASP_SIO_DATA_WIDTH_SET_REG		(0x78)
+#define HI_ASP_SIO_I2S_START_POS_REG		(0x7C)
+#define HI_ASP_SIO_I2S_POS_FLAG_REG		(0x80)
+#define HI_ASP_SIO_SIGNED_EXT_REG		(0x84)
+#define HI_ASP_SIO_I2S_POS_MERGE_EN_REG		(0x88)
+#define HI_ASP_SIO_INTMASK_REG			(0x8C)
+#define HI_ASP_SIO_I2S_DUAL_RX_CHN_REG		(0xA0)
+#define HI_ASP_SIO_I2S_DUAL_TX_CHN_REG		(0xC0)
+
+
+#define HI_ASP_CFG_R_CLK_SEL_EN			BIT(2)
+#define HI_ASP_CFG_R_CLK_SEL			0x140010
+#define HI_ASP_CFG_R_CLK1_DIV_SEL		0xbcdc9a
+#define HI_ASP_CFG_R_CLK4_DIV_SEL		0x00ff000f
+#define HI_ASP_CFG_R_CLK6_DIV_SEL		0x00ff003f
+#define HI_ASP_CFG_SIO_MODE			0
+#define HI_ASP_SIO_MODE_SEL_EN			BIT(0)
+#define HI_ASP_MASK				0xffffffff
+
+#define HI_ASP_SIO_RX_ENABLE			BIT(13)
+#define HI_ASP_SIO_TX_ENABLE			BIT(12)
+#define HI_ASP_SIO_RX_FIFO_DISABLE		BIT(11)
+#define HI_ASP_SIO_TX_FIFO_DISABLE		BIT(10)
+#define HI_ASP_SIO_RX_DATA_MERGE		BIT(9)
+#define HI_ASP_SIO_TX_DATA_MERGE		BIT(8)
+#define HI_ASP_SIO_RX_FIFO_THRESHOLD		(0x5 << 4)
+#define HI_ASP_SIO_TX_FIFO_THRESHOLD		(0xB << 0)
+#define HI_ASP_SIO_RX_FIFO_THRESHOLD_CLR	(0xF << 4)
+#define HI_ASP_SIO_TX_FIFO_THRESHOLD_CLR	(0xF << 0)
+#define HI_ASP_SIO_BURST			(0x4)
+
+
+enum hisi_i2s_formats {
+	HII2S_FORMAT_I2S,
+	HII2S_FORMAT_PCM_STD,
+	HII2S_FORMAT_PCM_USER,
+	HII2S_FORMAT_LEFT_JUST,
+	HII2S_FORMAT_RIGHT_JUST,
+};
+
+#endif/* _HI3660_I2S_H */
-- 
2.8.0

Re: [PATCH 1/3] sound: Add hikey960 i2s audio driver

[Mark Brown]

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

On Thu, Feb 28, 2019 at 09:57:00PM +0800, Pengcheng Li wrote:
> From: Youlin Wang <wangyoulin1@hisilicon.com>

Please use subject lines matching the style for the subsystem.  This
makes it easier for people to identify relevant patches.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

>  config SND_I2S_HI6210_I2S
> -	tristate "Hisilicon I2S controller"
> +	tristate "Hisilicon Hi6210 I2S controller"
> +	select SND_SOC_GENERIC_DMAENGINE_PCM
> +	help
> +	  Hisilicon I2S
> +

This should really be a separate patch as it's an update to the
existing driver.

> --- /dev/null
> +++ b/sound/soc/hisilicon/hi3660-i2s.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * linux/sound/soc/hisilicon/hi3660-i2s.c
> + *

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

> + * I2S IP driver for hi3660.
> + *
> + * Copyright (c) 2001-2021, Huawei Tech. Co., Ltd.
> + */

Given that it's currently 2019 I'm not sure that the years copyright is
claimed for here are accurate...

> +static void update_bits(struct hi3660_i2s *i2s, u32 ofs, u32 reset, u32 set)
> +{
> +	u32 val = readl(i2s->base + ofs) & ~reset;
> +
> +	writel(val | set, i2s->base + ofs);
> +}

It'd be better to namespace the functions in the driver to avoid
collisons with anything that gets added to headers.  Look at how other
drivers name their functions for examples.

It's also a bit confusing to have an _update_bits() with the set/reset
pattern given that we've got _update_bits() functions with the
mask/values pattern at both the ASoC and regmap levels in Linux.

> +static void shutdown(struct snd_pcm_substream *substream,
> +		       struct snd_soc_dai *cpu_dai)
> +{
> +	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
> +
> +	if (!IS_ERR_OR_NULL(i2s->asp_subsys_clk))
> +		clk_disable_unprepare(i2s->asp_subsys_clk);
> +}

Can the driver actually function usefully without this clock?

> +static int set_sysclk(struct snd_soc_dai *cpu_dai,
> +			     int clk_id, unsigned int freq, int dir)
> +{
> +	return 0;
> +}

Don't implement empty operations; if it's safe to not have an operation
(like this one) then the framework will handle it being missing.

> +static int set_format(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
> +
> +	i2s->format = fmt;
> +	i2s->master = (i2s->format & SND_SOC_DAIFMT_MASTER_MASK) ==
> +		      SND_SOC_DAIFMT_CBS_CFS;
> +
> +	return 0;
> +}

There should be some validation in this function to make sure that the
format is valid.

> +	case SNDRV_PCM_FORMAT_U24_LE:
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		i2s->bits = 32;
> +		dma_data->addr_width = 4;
> +		break;

Does the hardware not support 32 bit samples?

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		ret = -ENODEV;
> +		return ret;
> +	}
> +	i2s->base_syscon = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(i2s->base_syscon)) {
> +		dev_err(&pdev->dev, "ioremap failed\n");
> +		ret = PTR_ERR(i2s->base_syscon);
> +		return ret;
> +	}

If there's a system controller involved shouldn't we be using the
standard system controller support in drivers/mfd/syscon.c?  Other
HiSilicon devices seem to use it.

> +	i2s->pctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(i2s->pctrl)) {
> +		dev_err(dev, "could not get pinctrl\n");
> +		ret = -EIO;
> +		return ret;
> +	}

This is discarding the error code returned by the API which is going to
make debuggging harder and will break deferred probe - you should use
PTR_ERR() to get the error and both print it and return it as the error
code.

> +static int hi3660_i2s_remove(struct platform_device *pdev)
> +{
> +	struct hi3660_i2s *i2s = dev_get_drvdata(&pdev->dev);
> +
> +	snd_soc_unregister_component(&pdev->dev);
> +	dev_set_drvdata(&pdev->dev, NULL);
> +
> +	pinctrl_put(i2s->pctrl);

The driver used devm_ functions on probe so no need to explicitly undo
things, the devm_ code will handle that for you.

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

Re: [PATCH 1/3] sound: Add hikey960 i2s audio driver

[Daniel Baluta]

Hello Pengcheng,

Make sure you run ./scripts/checkpatch.pl --strict yourpatchfile.patch

On Thu, Feb 28, 2019 at 4:15 PM Pengcheng Li <lipengcheng8@huawei.com> wrote:
>
> From: Youlin Wang <wangyoulin1@hisilicon.com>
>
> Add i2s driver for hisi3660 soc found on the hikey960 board.
> Add conpile line in make file.
> Technical support by Guangke Ji.

Care to add here some documentation pointers to I2S IP on the SOC?

> diff --git a/sound/soc/hisilicon/Kconfig b/sound/soc/hisilicon/Kconfig
> index 4356d5a..b023ef9 100644
> --- a/sound/soc/hisilicon/Kconfig
> +++ b/sound/soc/hisilicon/Kconfig
> @@ -1,5 +1,11 @@
>  config SND_I2S_HI6210_I2S
> -       tristate "Hisilicon I2S controller"
> +       tristate "Hisilicon Hi6210 I2S controller"
> +       select SND_SOC_GENERIC_DMAENGINE_PCM
> +       help
> +         Hisilicon I2S

Can you enahance the help text? Something like "I2S controller driver
for hisi3600 SoC"
<snip>

> + * Copyright (c) 2001-2021, Huawei Tech. Co., Ltd.

2021? :) IANAL but this looks strange :).

> +struct hi3660_i2s {
> +       struct device *dev;
> +       struct reset_control *rc;
> +       int clocks;
> +       struct regulator *regu_asp;
> +       struct pinctrl *pctrl;
> +       struct pinctrl_state *pin_default;
> +       struct pinctrl_state *pin_idle;
> +       struct clk *asp_subsys_clk;
> +       struct snd_soc_dai_driver dai;
> +       void __iomem *base;
> +       void __iomem *base_syscon;
> +       phys_addr_t base_phys;
> +       struct snd_dmaengine_dai_dma_data dma_data[2];
> +       spinlock_t lock;

What is this lock used for? Please add some docs.

> +       int rate;
> +       int format;
> +       int bits;
> +       int channels;
> +       u32 master;
> +       u32 status;
> +};
> +
> +static void update_bits(struct hi3660_i2s *i2s, u32 ofs, u32 reset, u32 set)
> +{
> +       u32 val = readl(i2s->base + ofs) & ~reset;
> +
> +       writel(val | set, i2s->base + ofs);
> +}
> +
> +static void update_bits_syscon(struct hi3660_i2s *i2s,
> +                       u32 ofs, u32 reset, u32 set)
> +{
> +       u32 val = readl(i2s->base_syscon + ofs) & ~reset;
> +
> +       writel(val | set, i2s->base_syscon + ofs);
> +}

Look at the snd_soc_component_update_bits. You can make these two
functions following the same
pattern for parameters.