[alsa-devel] [PATCH 1/3] sound: Add hikey960 i2s audio driver
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 */
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.
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.
participants (3)
-
Daniel Baluta
-
Mark Brown
-
Pengcheng Li