* [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver
@ 2020-06-01 9:53 Sia Jee Heng
2020-06-01 9:53 ` [PATCH v3 1/3] ASoC: Intel: Add KeemBay " Sia Jee Heng
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Sia Jee Heng @ 2020-06-01 9:53 UTC (permalink / raw
To: alsa-devel; +Cc: liam.r.girdwood, broonie, tiwai, pierre-louis.bossart
The below series of patches support the KeemBay ASoC platform driver.
The platform driver initialize the i2s to capture and playback the
pcm data on the ARM. The i2s is running in polling mode.
There is no DSP in the KeemBay SoC. Users are rely on the Gstreamer plugin
to perform Audio preprocessing.
Audio graph card is used to connect the platform driver with the
tlv320aic3204 codec.
Change History:
v3:
- Adjusted header format.
- Use Audio graph card instead of custom sound card.
- Use if-else instead of conditional operator.
- Enabled .set_fmt to configure master clock.
v2:
- Corrected I2S naming for DT binding.
v1:
- Initial version.
Sia Jee Heng (3):
ASoC: Intel: Add KeemBay platform driver
ASoC: Intel: Add makefiles and kconfig changes for KeemBay
dt-bindings: sound: Add documentation for KeemBay i2s
.../bindings/sound/intel,keembay-i2s.yaml | 68 +++
sound/soc/intel/Kconfig | 7 +
sound/soc/intel/Makefile | 1 +
sound/soc/intel/keembay/Makefile | 4 +
sound/soc/intel/keembay/kmb_platform.c | 667 +++++++++++++++++++++
sound/soc/intel/keembay/kmb_platform.h | 145 +++++
6 files changed, 892 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml
create mode 100644 sound/soc/intel/keembay/Makefile
create mode 100644 sound/soc/intel/keembay/kmb_platform.c
create mode 100644 sound/soc/intel/keembay/kmb_platform.h
--
1.9.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3 1/3] ASoC: Intel: Add KeemBay platform driver 2020-06-01 9:53 [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Sia Jee Heng @ 2020-06-01 9:53 ` Sia Jee Heng 2020-06-01 21:26 ` Cezary Rojewski 2020-06-01 9:53 ` [PATCH v3 2/3] ASoC: Intel: Add makefiles and kconfig changes for KeemBay Sia Jee Heng ` (3 subsequent siblings) 4 siblings, 1 reply; 9+ messages in thread From: Sia Jee Heng @ 2020-06-01 9:53 UTC (permalink / raw To: alsa-devel; +Cc: liam.r.girdwood, broonie, tiwai, pierre-louis.bossart Add KeemBay ASoC platform driver which initialize the i2s controller and uses i2s to capture and transmit pcm data to external codec. The i2s is running in polling mode. Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> Signed-off-by: Sia Jee Heng <jee.heng.sia@intel.com> --- sound/soc/intel/keembay/Makefile | 4 + sound/soc/intel/keembay/kmb_platform.c | 667 +++++++++++++++++++++++++++++++++ sound/soc/intel/keembay/kmb_platform.h | 145 +++++++ 3 files changed, 816 insertions(+) create mode 100644 sound/soc/intel/keembay/Makefile create mode 100644 sound/soc/intel/keembay/kmb_platform.c create mode 100644 sound/soc/intel/keembay/kmb_platform.h diff --git a/sound/soc/intel/keembay/Makefile b/sound/soc/intel/keembay/Makefile new file mode 100644 index 0000000..9084e8c --- /dev/null +++ b/sound/soc/intel/keembay/Makefile @@ -0,0 +1,4 @@ +snd-soc-kmb_platform-objs := \ + kmb_platform.o + +obj-$(CONFIG_SND_SOC_INTEL_KEEMBAY) += snd-soc-kmb_platform.o diff --git a/sound/soc/intel/keembay/kmb_platform.c b/sound/soc/intel/keembay/kmb_platform.c new file mode 100644 index 0000000..44c34be --- /dev/null +++ b/sound/soc/intel/keembay/kmb_platform.c @@ -0,0 +1,667 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright (C) 2020 Intel Corporation. +// +// Intel KeemBay Platform driver. +// + +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include "kmb_platform.h" + +#define PERIODS_MIN 2 +#define PERIODS_MAX 48 +#define PERIOD_BYTES_MIN 4096 +#define BUFFER_BYTES_MAX (PERIODS_MAX * PERIOD_BYTES_MIN) +#define TDM_OPERATION 1 +#define I2S_OPERATION 0 +#define DATA_WIDTH_CONFIG_BIT 6 +#define TDM_CHANNEL_CONFIG_BIT 3 +#define I2S_SAMPLE_RATES (SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_48000) + +static const struct snd_pcm_hardware kmb_pcm_hardware = { + .info = SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_BLOCK_TRANSFER, + .rates = I2S_SAMPLE_RATES, + .rate_min = 16000, + .rate_max = 48000, + .formats = SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S32_LE, + .channels_min = 2, + .channels_max = 2, + .buffer_bytes_max = BUFFER_BYTES_MAX, + .period_bytes_min = PERIOD_BYTES_MIN, + .period_bytes_max = BUFFER_BYTES_MAX / PERIODS_MIN, + .periods_min = PERIODS_MIN, + .periods_max = PERIODS_MAX, + .fifo_size = 16, +}; + +static unsigned int kmb_pcm_tx_fn(struct kmb_i2s_info *kmb_i2s, + struct snd_pcm_runtime *runtime, + unsigned int tx_ptr, bool *period_elapsed) +{ + unsigned int period_pos = tx_ptr % runtime->period_size; + void __iomem *i2s_base = kmb_i2s->i2s_base; + u16(*p16)[2]; + u32(*p32)[2]; + int i; + + if (kmb_i2s->config.data_width == 16) + p16 = (void *)runtime->dma_area; + else + p32 = (void *)runtime->dma_area; + /* KMB i2s uses two separate L/R FIFO */ + for (i = 0; i < kmb_i2s->fifo_th; i++) { + if (kmb_i2s->config.data_width == 16) { + writel(p16[tx_ptr][0], i2s_base + LRBR_LTHR(0)); + writel(p16[tx_ptr][1], i2s_base + RRBR_RTHR(0)); + } else { + writel(p32[tx_ptr][0], i2s_base + LRBR_LTHR(0)); + writel(p32[tx_ptr][1], i2s_base + RRBR_RTHR(0)); + } + + period_pos++; + + if (++tx_ptr >= runtime->buffer_size) + tx_ptr = 0; + } + + *period_elapsed = period_pos >= runtime->period_size; + + return tx_ptr; +} + +static unsigned int kmb_pcm_rx_fn(struct kmb_i2s_info *kmb_i2s, + struct snd_pcm_runtime *runtime, + unsigned int rx_ptr, bool *period_elapsed) +{ + unsigned int period_pos = rx_ptr % runtime->period_size; + void __iomem *i2s_base = kmb_i2s->i2s_base; + u16(*p16)[2]; + u32(*p32)[2]; + int i; + + if (kmb_i2s->config.data_width == 16) + p16 = (void *)runtime->dma_area; + else + p32 = (void *)runtime->dma_area; + /* KMB i2s uses two separate L/R FIFO */ + for (i = 0; i < kmb_i2s->fifo_th; i++) { + if (kmb_i2s->config.data_width == 16) { + p16[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0)); + p16[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0)); + } else { + p32[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0)); + p32[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0)); + } + + period_pos++; + + if (++rx_ptr >= runtime->buffer_size) + rx_ptr = 0; + } + + *period_elapsed = period_pos >= runtime->period_size; + + return rx_ptr; +} + +static inline void kmb_i2s_disable_channels(struct kmb_i2s_info *kmb_i2s, + u32 stream) +{ + struct i2s_clk_config_data *config = &kmb_i2s->config; + u32 i; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + for (i = 0; i < config->chan_nr / 2; i++) + writel(0, kmb_i2s->i2s_base + TER(i)); + } else { + for (i = 0; i < config->chan_nr / 2; i++) + writel(0, kmb_i2s->i2s_base + RER(i)); + } +} + +static inline void kmb_i2s_clear_irqs(struct kmb_i2s_info *kmb_i2s, u32 stream) +{ + struct i2s_clk_config_data *config = &kmb_i2s->config; + u32 i; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + for (i = 0; i < config->chan_nr / 2; i++) + readl(kmb_i2s->i2s_base + TOR(i)); + } else { + for (i = 0; i < config->chan_nr / 2; i++) + readl(kmb_i2s->i2s_base + ROR(i)); + } +} + +static inline void kmb_i2s_irq_trigger(struct kmb_i2s_info *kmb_i2s, + u32 stream, int chan_nr, bool trigger) +{ + u32 i, irq; + u32 flag; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) + flag = TX_INT_FLAG; + else + flag = RX_INT_FLAG; + + for (i = 0; i < chan_nr / 2; i++) { + irq = readl(kmb_i2s->i2s_base + IMR(i)); + + if (trigger) + irq = irq & ~flag; + else + irq = irq | flag; + + writel(irq, kmb_i2s->i2s_base + IMR(i)); + } +} + +static void kmb_pcm_operation(struct kmb_i2s_info *kmb_i2s, bool playback) +{ + struct snd_pcm_substream *substream; + bool active, period_elapsed; + + if (playback) + substream = kmb_i2s->tx_substream; + else + substream = kmb_i2s->rx_substream; + + active = substream && snd_pcm_running(substream); + + if (active) { + unsigned int ptr; + unsigned int new_ptr; + + if (playback) { + ptr = kmb_i2s->tx_ptr; + new_ptr = kmb_pcm_tx_fn(kmb_i2s, substream->runtime, + ptr, &period_elapsed); + cmpxchg(&kmb_i2s->tx_ptr, ptr, new_ptr); + } else { + ptr = kmb_i2s->rx_ptr; + new_ptr = kmb_pcm_rx_fn(kmb_i2s, substream->runtime, + ptr, &period_elapsed); + cmpxchg(&kmb_i2s->rx_ptr, ptr, new_ptr); + } + + if (period_elapsed) + snd_pcm_period_elapsed(substream); + } +} + +static int kmb_pcm_open(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct kmb_i2s_info *kmb_i2s; + + kmb_i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0)); + snd_soc_set_runtime_hwparams(substream, &kmb_pcm_hardware); + snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); + runtime->private_data = kmb_i2s; + + return 0; +} + +static int kmb_pcm_trigger(struct snd_soc_component *component, + struct snd_pcm_substream *substream, int cmd) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct kmb_i2s_info *kmb_i2s = runtime->private_data; + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + kmb_i2s->tx_ptr = 0; + kmb_i2s->tx_substream = substream; + } else { + kmb_i2s->rx_ptr = 0; + kmb_i2s->rx_substream = substream; + } + break; + case SNDRV_PCM_TRIGGER_STOP: + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + kmb_i2s->tx_substream = NULL; + else + kmb_i2s->rx_substream = NULL; + break; + default: + return -EINVAL; + } + + return 0; +} + +static irqreturn_t kmb_i2s_irq_handler(int irq, void *dev_id) +{ + struct kmb_i2s_info *kmb_i2s = dev_id; + struct i2s_clk_config_data *config = &kmb_i2s->config; + irqreturn_t ret = IRQ_NONE; + u32 isr[4]; + int i; + + for (i = 0; i < config->chan_nr / 2; i++) + isr[i] = readl(kmb_i2s->i2s_base + ISR(i)); + + kmb_i2s_clear_irqs(kmb_i2s, SNDRV_PCM_STREAM_PLAYBACK); + kmb_i2s_clear_irqs(kmb_i2s, SNDRV_PCM_STREAM_CAPTURE); + + for (i = 0; i < config->chan_nr / 2; i++) { + /* + * Check if TX fifo is empty. If empty fill FIFO with samples + */ + if ((isr[i] & ISR_TXFE)) { + kmb_pcm_operation(kmb_i2s, true); + ret = IRQ_HANDLED; + } + /* + * Data available. Retrieve samples from FIFO + */ + if ((isr[i] & ISR_RXDA)) { + kmb_pcm_operation(kmb_i2s, false); + ret = IRQ_HANDLED; + } + /* Error Handling: TX */ + if (isr[i] & ISR_TXFO) { + dev_dbg(kmb_i2s->dev, "TX overrun (ch_id=%d)\n", i); + ret = IRQ_HANDLED; + } + /* Error Handling: RX */ + if (isr[i] & ISR_RXFO) { + dev_dbg(kmb_i2s->dev, "RX overrun (ch_id=%d)\n", i); + ret = IRQ_HANDLED; + } + } + + return ret; +} + +static int kmb_platform_pcm_new(struct snd_soc_component *component, + struct snd_soc_pcm_runtime *soc_runtime) +{ + size_t size = kmb_pcm_hardware.buffer_bytes_max; + /* Use SNDRV_DMA_TYPE_CONTINUOUS as KMB doesn't use PCI sg buffer */ + snd_pcm_set_managed_buffer_all(soc_runtime->pcm, + SNDRV_DMA_TYPE_CONTINUOUS, + NULL, size, size); + return 0; +} + +static snd_pcm_uframes_t kmb_pcm_pointer(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct kmb_i2s_info *kmb_i2s = runtime->private_data; + snd_pcm_uframes_t pos; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + pos = kmb_i2s->tx_ptr; + else + pos = kmb_i2s->rx_ptr; + + return pos < runtime->buffer_size ? pos : 0; +} + +static const struct snd_soc_component_driver kmb_component = { + .name = "kmb", + .pcm_construct = kmb_platform_pcm_new, + .open = kmb_pcm_open, + .trigger = kmb_pcm_trigger, + .pointer = kmb_pcm_pointer, +}; + +static void kmb_i2s_start(struct kmb_i2s_info *kmb_i2s, + struct snd_pcm_substream *substream) +{ + struct i2s_clk_config_data *config = &kmb_i2s->config; + + /* I2S Programming sequence in Keem_Bay_VPU_DB_v1.1 */ + writel(1, kmb_i2s->i2s_base + IER); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + writel(1, kmb_i2s->i2s_base + ITER); + else + writel(1, kmb_i2s->i2s_base + IRER); + + kmb_i2s_irq_trigger(kmb_i2s, substream->stream, config->chan_nr, true); + + if (kmb_i2s->master) + writel(1, kmb_i2s->i2s_base + CER); + else + writel(0, kmb_i2s->i2s_base + CER); +} + +static void kmb_i2s_stop(struct kmb_i2s_info *kmb_i2s, + struct snd_pcm_substream *substream) +{ + /* I2S Programming sequence in Keem_Bay_VPU_DB_v1.1 */ + kmb_i2s_clear_irqs(kmb_i2s, substream->stream); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + writel(0, kmb_i2s->i2s_base + ITER); + else + writel(0, kmb_i2s->i2s_base + IRER); + + kmb_i2s_irq_trigger(kmb_i2s, substream->stream, 8, false); + + if (!kmb_i2s->active) { + writel(0, kmb_i2s->i2s_base + CER); + writel(0, kmb_i2s->i2s_base + IER); + } +} + +static void kmb_disable_clk(void *clk) +{ + clk_disable_unprepare(clk); +} + +static int kmb_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) +{ + struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai); + int ret; + + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBM_CFM: + kmb_i2s->master = false; + ret = 0; + break; + case SND_SOC_DAIFMT_CBS_CFS: + writel(MASTER_MODE, kmb_i2s->pss_base + I2S_GEN_CFG_0); + + ret = clk_prepare_enable(kmb_i2s->clk_i2s); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(kmb_i2s->dev, kmb_disable_clk, + kmb_i2s->clk_i2s); + if (ret) + return ret; + + kmb_i2s->master = true; + break; + default: + return -EINVAL; + } + + return ret; +} + +static int kmb_dai_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *cpu_dai) +{ + struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai); + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + /* Keep track of i2s activity before turn off + * the i2s interface + */ + kmb_i2s->active++; + kmb_i2s_start(kmb_i2s, substream); + break; + case SNDRV_PCM_TRIGGER_STOP: + kmb_i2s->active--; + kmb_i2s_stop(kmb_i2s, substream); + break; + default: + return -EINVAL; + } + + return 0; +} + +static void kmb_i2s_config(struct kmb_i2s_info *kmb_i2s, int stream) +{ + struct i2s_clk_config_data *config = &kmb_i2s->config; + u32 ch_reg; + + kmb_i2s_disable_channels(kmb_i2s, stream); + + for (ch_reg = 0; ch_reg < config->chan_nr / 2; ch_reg++) { + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + writel(kmb_i2s->xfer_resolution, + kmb_i2s->i2s_base + TCR(ch_reg)); + + writel(kmb_i2s->fifo_th - 1, + kmb_i2s->i2s_base + TFCR(ch_reg)); + + writel(1, kmb_i2s->i2s_base + TER(ch_reg)); + } else { + writel(kmb_i2s->xfer_resolution, + kmb_i2s->i2s_base + RCR(ch_reg)); + + writel(kmb_i2s->fifo_th - 1, + kmb_i2s->i2s_base + RFCR(ch_reg)); + + writel(1, kmb_i2s->i2s_base + RER(ch_reg)); + } + } +} + +static int kmb_dai_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *hw_params, + struct snd_soc_dai *cpu_dai) +{ + struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai); + struct i2s_clk_config_data *config = &kmb_i2s->config; + u32 register_val, write_val; + int ret; + + switch (params_format(hw_params)) { + case SNDRV_PCM_FORMAT_S16_LE: + config->data_width = 16; + kmb_i2s->ccr = 0x00; + kmb_i2s->xfer_resolution = 0x02; + break; + case SNDRV_PCM_FORMAT_S24_LE: + config->data_width = 24; + kmb_i2s->ccr = 0x08; + kmb_i2s->xfer_resolution = 0x04; + break; + case SNDRV_PCM_FORMAT_S32_LE: + config->data_width = 32; + kmb_i2s->ccr = 0x10; + kmb_i2s->xfer_resolution = 0x05; + break; + default: + dev_err(kmb_i2s->dev, "kmb: unsupported PCM fmt"); + return -EINVAL; + } + + config->chan_nr = params_channels(hw_params); + + switch (config->chan_nr) { + /* TODO: This switch case will handle up to TDM8 in the near future */ + case TWO_CHANNEL_SUPPORT: + write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | + (config->data_width << DATA_WIDTH_CONFIG_BIT) | + MASTER_MODE | I2S_OPERATION; + + writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0); + + register_val = readl(kmb_i2s->pss_base + I2S_GEN_CFG_0); + dev_dbg(kmb_i2s->dev, "pss register = 0x%X", register_val); + break; + default: + dev_dbg(kmb_i2s->dev, "channel not supported\n"); + return -EINVAL; + } + + kmb_i2s_config(kmb_i2s, substream->stream); + + writel(kmb_i2s->ccr, kmb_i2s->i2s_base + CCR); + + config->sample_rate = params_rate(hw_params); + + if (kmb_i2s->master) { + /* Only 2 ch supported in Master mode */ + u32 bitclk = config->sample_rate * config->data_width * 2; + + ret = clk_set_rate(kmb_i2s->clk_i2s, bitclk); + if (ret) { + dev_err(kmb_i2s->dev, + "Can't set I2S clock rate: %d\n", ret); + return ret; + } + } + + return 0; +} + +static int kmb_dai_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *cpu_dai) +{ + struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + writel(1, kmb_i2s->i2s_base + TXFFR); + else + writel(1, kmb_i2s->i2s_base + RXFFR); + + return 0; +} + +static struct snd_soc_dai_ops kmb_dai_ops = { + .trigger = kmb_dai_trigger, + .hw_params = kmb_dai_hw_params, + .prepare = kmb_dai_prepare, + .set_fmt = kmb_set_dai_fmt, +}; + +static struct snd_soc_dai_driver intel_kmb_platform_dai[] = { + { + .name = "kmb-plat-dai", + .playback = { + .channels_min = 2, + .channels_max = 2, + .rates = I2S_SAMPLE_RATES, + .rate_min = 16000, + .rate_max = 48000, + .formats = (SNDRV_PCM_FMTBIT_S32_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S16_LE), + }, + .capture = { + .channels_min = 2, + .channels_max = 2, + .rates = I2S_SAMPLE_RATES, + .rate_min = 16000, + .rate_max = 48000, + .formats = (SNDRV_PCM_FMTBIT_S32_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S16_LE), + }, + .ops = &kmb_dai_ops, + }, +}; + +static int kmb_plat_dai_probe(struct platform_device *pdev) +{ + struct snd_soc_dai_driver *kmb_i2s_dai; + struct device *dev = &pdev->dev; + struct kmb_i2s_info *kmb_i2s; + int ret, irq; + u32 comp1_reg; + + kmb_i2s = devm_kzalloc(dev, sizeof(*kmb_i2s), GFP_KERNEL); + if (!kmb_i2s) + return -ENOMEM; + + kmb_i2s_dai = devm_kzalloc(dev, sizeof(*kmb_i2s_dai), GFP_KERNEL); + if (!kmb_i2s_dai) + return -ENOMEM; + + kmb_i2s_dai->ops = &kmb_dai_ops; + + /* Prepare the related clocks */ + kmb_i2s->clk_apb = devm_clk_get(dev, "apb_clk"); + if (IS_ERR(kmb_i2s->clk_apb)) { + dev_err(dev, "Failed to get apb clock\n"); + return PTR_ERR(kmb_i2s->clk_apb); + } + + ret = clk_prepare_enable(kmb_i2s->clk_apb); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(dev, kmb_disable_clk, kmb_i2s->clk_apb); + if (ret) { + dev_err(dev, "Failed to add clk_apb reset action\n"); + return ret; + } + + kmb_i2s->clk_i2s = devm_clk_get(dev, "osc"); + if (IS_ERR(kmb_i2s->clk_i2s)) { + dev_err(dev, "Failed to get osc clock\n"); + return PTR_ERR(kmb_i2s->clk_i2s); + } + + kmb_i2s->i2s_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(kmb_i2s->i2s_base)) + return PTR_ERR(kmb_i2s->i2s_base); + + kmb_i2s->pss_base = devm_platform_ioremap_resource(pdev, 1); + if (IS_ERR(kmb_i2s->pss_base)) + return PTR_ERR(kmb_i2s->pss_base); + + kmb_i2s->dev = &pdev->dev; + + irq = platform_get_irq_optional(pdev, 0); + if (irq > 0) { + ret = devm_request_irq(dev, irq, kmb_i2s_irq_handler, 0, + pdev->name, kmb_i2s); + if (ret < 0) { + dev_err(dev, "failed to request irq\n"); + return ret; + } + } + + comp1_reg = readl(kmb_i2s->i2s_base + I2S_COMP_PARAM_1); + + kmb_i2s->fifo_th = (1 << COMP1_FIFO_DEPTH(comp1_reg)) / 2; + + ret = devm_snd_soc_register_component(dev, &kmb_component, + intel_kmb_platform_dai, + ARRAY_SIZE(intel_kmb_platform_dai)); + if (ret) { + dev_err(dev, "not able to register dai\n"); + return ret; + } + + dev_set_drvdata(dev, kmb_i2s); + + return ret; +} + +static const struct of_device_id kmb_plat_of_match[] = { + { .compatible = "intel,keembay-i2s", }, + {} +}; + +static struct platform_driver kmb_plat_dai_driver = { + .driver = { + .name = "kmb-plat-dai", + .of_match_table = kmb_plat_of_match, + }, + .probe = kmb_plat_dai_probe, +}; + +module_platform_driver(kmb_plat_dai_driver); + +MODULE_DESCRIPTION("ASoC Intel KeemBay Platform driver"); +MODULE_AUTHOR("Sia Jee Heng <jee.heng.sia@intel.com>"); +MODULE_AUTHOR("Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:kmb_platform"); diff --git a/sound/soc/intel/keembay/kmb_platform.h b/sound/soc/intel/keembay/kmb_platform.h new file mode 100644 index 0000000..2960065 --- /dev/null +++ b/sound/soc/intel/keembay/kmb_platform.h @@ -0,0 +1,145 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Intel KeemBay Platform driver + * + * Copyright (C) 2020 Intel Corporation. + * + */ + +#ifndef KMB_PLATFORM_H_ +#define KMB_PLATFORMP_H_ + +#include <linux/bits.h> +#include <linux/bitfield.h> +#include <linux/types.h> + +/* Register values with reference to KMB databook v1.1 */ +/* common register for all channel */ +#define IER 0x000 +#define IRER 0x004 +#define ITER 0x008 +#define CER 0x00C +#define CCR 0x010 +#define RXFFR 0x014 +#define TXFFR 0x018 + +/* Interrupt status register fields */ +#define ISR_TXFO BIT(5) +#define ISR_TXFE BIT(4) +#define ISR_RXFO BIT(1) +#define ISR_RXDA BIT(0) + +/* I2S Tx Rx Registers for all channels */ +#define LRBR_LTHR(x) (0x40 * (x) + 0x020) +#define RRBR_RTHR(x) (0x40 * (x) + 0x024) +#define RER(x) (0x40 * (x) + 0x028) +#define TER(x) (0x40 * (x) + 0x02C) +#define RCR(x) (0x40 * (x) + 0x030) +#define TCR(x) (0x40 * (x) + 0x034) +#define ISR(x) (0x40 * (x) + 0x038) +#define IMR(x) (0x40 * (x) + 0x03C) +#define ROR(x) (0x40 * (x) + 0x040) +#define TOR(x) (0x40 * (x) + 0x044) +#define RFCR(x) (0x40 * (x) + 0x048) +#define TFCR(x) (0x40 * (x) + 0x04C) +#define RFF(x) (0x40 * (x) + 0x050) +#define TFF(x) (0x40 * (x) + 0x054) + +/* I2S COMP Registers */ +#define I2S_COMP_PARAM_2 0x01F0 +#define I2S_COMP_PARAM_1 0x01F4 +#define I2S_COMP_VERSION 0x01F8 +#define I2S_COMP_TYPE 0x01FC + +/* PSS_GEN_CTRL_I2S_GEN_CFG_0 Registers */ +#define I2S_GEN_CFG_0 0x000 +#define PSS_CPR_RST_EN 0x010 +#define PSS_CPR_RST_SET 0x014 +#define PSS_CPR_CLK_CLR 0x000 +#define PSS_CPR_AUX_RST_EN 0x070 + +#define MASTER_MODE BIT(13) + +/* Interrupt Flag */ +#define TX_INT_FLAG GENMASK(5, 4) +#define RX_INT_FLAG GENMASK(1, 0) +/* + * Component parameter register fields - define the I2S block's + * configuration. + */ +#define COMP1_TX_WORDSIZE_3(r) FIELD_GET(GENMASK(27, 25), (r)) +#define COMP1_TX_WORDSIZE_2(r) FIELD_GET(GENMASK(24, 22), (r)) +#define COMP1_TX_WORDSIZE_1(r) FIELD_GET(GENMASK(21, 19), (r)) +#define COMP1_TX_WORDSIZE_0(r) FIELD_GET(GENMASK(18, 16), (r)) +#define COMP1_RX_ENABLED(r) FIELD_GET(BIT(6), (r)) +#define COMP1_TX_ENABLED(r) FIELD_GET(BIT(5), (r)) +#define COMP1_MODE_EN(r) FIELD_GET(BIT(4), (r)) +#define COMP1_APB_DATA_WIDTH(r) FIELD_GET(GENMASK(1, 0), (r)) +#define COMP2_RX_WORDSIZE_3(r) FIELD_GET(GENMASK(12, 10), (r)) +#define COMP2_RX_WORDSIZE_2(r) FIELD_GET(GENMASK(9, 7), (r)) +#define COMP2_RX_WORDSIZE_1(r) FIELD_GET(GENMASK(5, 3), (r)) +#define COMP2_RX_WORDSIZE_0(r) FIELD_GET(GENMASK(2, 0), (r)) + +/* Add 1 to the below registers to indicate the actual size */ +#define COMP1_TX_CHANNELS(r) (FIELD_GET(GENMASK(10, 9), (r)) + 1) +#define COMP1_RX_CHANNELS(r) (FIELD_GET(GENMASK(8, 7), (r)) + 1) +#define COMP1_FIFO_DEPTH(r) (FIELD_GET(GENMASK(3, 2), (r)) + 1) + +/* Number of entries in WORDSIZE and DATA_WIDTH parameter registers */ +#define COMP_MAX_WORDSIZE 8 /* 3 bits register width */ + +#define MAX_CHANNEL_NUM 8 +#define MIN_CHANNEL_NUM 2 + +#define TWO_CHANNEL_SUPPORT 2 /* up to 2.0 */ +#define FOUR_CHANNEL_SUPPORT 4 /* up to 3.1 */ +#define SIX_CHANNEL_SUPPORT 6 /* up to 5.1 */ +#define EIGHT_CHANNEL_SUPPORT 8 /* up to 7.1 */ + +#define DWC_I2S_PLAY BIT(0) +#define DWC_I2S_RECORD BIT(1) +#define DW_I2S_SLAVE BIT(2) +#define DW_I2S_MASTER BIT(3) + +#define I2S_RXDMA 0x01C0 +#define I2S_TXDMA 0x01C8 + +/* + * struct i2s_clk_config_data - represent i2s clk configuration data + * @chan_nr: number of channel + * @data_width: number of bits per sample (8/16/24/32 bit) + * @sample_rate: sampling frequency (8Khz, 16Khz, 48Khz) + */ +struct i2s_clk_config_data { + int chan_nr; + u32 data_width; + u32 sample_rate; +}; + +struct kmb_i2s_info { + void __iomem *i2s_base; + void __iomem *pss_base; + struct clk *clk_i2s; + struct clk *clk_apb; + int active; + unsigned int capability; + unsigned int i2s_reg_comp1; + unsigned int i2s_reg_comp2; + struct device *dev; + u32 ccr; + u32 xfer_resolution; + u32 fifo_th; + bool master; + + struct i2s_clk_config_data config; + int (*i2s_clk_cfg)(struct i2s_clk_config_data *config); + + /* data related to PIO transfers */ + bool use_pio; + struct snd_pcm_substream *tx_substream; + struct snd_pcm_substream *rx_substream; + unsigned int tx_ptr; + unsigned int rx_ptr; +}; + +#endif /* KMB_PLATFORM_H_ */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] ASoC: Intel: Add KeemBay platform driver 2020-06-01 9:53 ` [PATCH v3 1/3] ASoC: Intel: Add KeemBay " Sia Jee Heng @ 2020-06-01 21:26 ` Cezary Rojewski 2020-06-02 12:57 ` Sia, Jee Heng 0 siblings, 1 reply; 9+ messages in thread From: Cezary Rojewski @ 2020-06-01 21:26 UTC (permalink / raw To: Sia Jee Heng Cc: Liam Girdwood, alsa-devel, Mark Brown, Takashi Iwai, Pierre-Louis Bossart On 2020-06-01 11:53 AM, Sia Jee Heng wrote: > + > +#define PERIODS_MIN 2 > +#define PERIODS_MAX 48 > +#define PERIOD_BYTES_MIN 4096 > +#define BUFFER_BYTES_MAX (PERIODS_MAX * PERIOD_BYTES_MIN) > +#define TDM_OPERATION 1 Looks as this is unused. > +#define I2S_OPERATION 0 > +#define DATA_WIDTH_CONFIG_BIT 6 > +#define TDM_CHANNEL_CONFIG_BIT 3 > +#define I2S_SAMPLE_RATES (SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_48000) This defines common rates yet .formats are left alone? As .formats, .rate_min/ max and others are left alone I'd stick with this style and specify .rates directly too. > + > +static const struct snd_pcm_hardware kmb_pcm_hardware = { > + .info = SNDRV_PCM_INFO_INTERLEAVED | > + SNDRV_PCM_INFO_MMAP | > + SNDRV_PCM_INFO_MMAP_VALID | > + SNDRV_PCM_INFO_BATCH | > + SNDRV_PCM_INFO_BLOCK_TRANSFER, Several flags have been or'ed here. Have these been verified working for Keembay? > + .rates = I2S_SAMPLE_RATES, > + .rate_min = 16000, > + .rate_max = 48000, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S24_LE | > + SNDRV_PCM_FMTBIT_S32_LE, > + .channels_min = 2, > + .channels_max = 2, > + .buffer_bytes_max = BUFFER_BYTES_MAX, > + .period_bytes_min = PERIOD_BYTES_MIN, > + .period_bytes_max = BUFFER_BYTES_MAX / PERIODS_MIN, > + .periods_min = PERIODS_MIN, > + .periods_max = PERIODS_MAX, > + .fifo_size = 16, > +}; > + > +static unsigned int kmb_pcm_tx_fn(struct kmb_i2s_info *kmb_i2s, > + struct snd_pcm_runtime *runtime, > + unsigned int tx_ptr, bool *period_elapsed) > +{ > + unsigned int period_pos = tx_ptr % runtime->period_size; > + void __iomem *i2s_base = kmb_i2s->i2s_base; > + u16(*p16)[2]; > + u32(*p32)[2]; > + int i; > + > + if (kmb_i2s->config.data_width == 16) > + p16 = (void *)runtime->dma_area; > + else > + p32 = (void *)runtime->dma_area; > + /* KMB i2s uses two separate L/R FIFO */ > + for (i = 0; i < kmb_i2s->fifo_th; i++) { > + if (kmb_i2s->config.data_width == 16) { > + writel(p16[tx_ptr][0], i2s_base + LRBR_LTHR(0)); > + writel(p16[tx_ptr][1], i2s_base + RRBR_RTHR(0)); > + } else { > + writel(p32[tx_ptr][0], i2s_base + LRBR_LTHR(0)); > + writel(p32[tx_ptr][1], i2s_base + RRBR_RTHR(0)); > + } Looks like a refactor candidate. Both, the preceding 'if' and the 'for' - which are the body of this func - are "data_width" dependent. You could redure if-ology by defining separate variants for 16 and non-16. Idk about naming index-variable 'rx_ptr'. This isn't a pointer, that's for sure. > + > + period_pos++; > + > + if (++tx_ptr >= runtime->buffer_size) > + tx_ptr = 0; > + } > + > + *period_elapsed = period_pos >= runtime->period_size; > + > + return tx_ptr; > +} > + > +static unsigned int kmb_pcm_rx_fn(struct kmb_i2s_info *kmb_i2s, > + struct snd_pcm_runtime *runtime, > + unsigned int rx_ptr, bool *period_elapsed) > +{ > + unsigned int period_pos = rx_ptr % runtime->period_size; > + void __iomem *i2s_base = kmb_i2s->i2s_base; > + u16(*p16)[2]; > + u32(*p32)[2]; > + int i; > + > + if (kmb_i2s->config.data_width == 16) > + p16 = (void *)runtime->dma_area; > + else > + p32 = (void *)runtime->dma_area; > + /* KMB i2s uses two separate L/R FIFO */ > + for (i = 0; i < kmb_i2s->fifo_th; i++) { > + if (kmb_i2s->config.data_width == 16) { > + p16[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0)); > + p16[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0)); > + } else { > + p32[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0)); > + p32[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0)); > + } The exact same advice goes here. > + > + period_pos++; > + > + if (++rx_ptr >= runtime->buffer_size) > + rx_ptr = 0; > + } > + > + *period_elapsed = period_pos >= runtime->period_size; > + > + return rx_ptr; > +} > + > + > +static void kmb_pcm_operation(struct kmb_i2s_info *kmb_i2s, bool playback) > +{ > + struct snd_pcm_substream *substream; > + bool active, period_elapsed; > + > + if (playback) > + substream = kmb_i2s->tx_substream; > + else > + substream = kmb_i2s->rx_substream; > + > + active = substream && snd_pcm_running(substream); 'active' serves little to no purpose here, I'd opt for removing it. > + > + if (active) { When deciding between no middle func 'return' or recuded indentation, the later is more readable. Simple: if (!substream || snd_pcm_running(substream)) return; allows rest of the function to be shift-left'ed. > + unsigned int ptr; > + unsigned int new_ptr; 'ptr' and 'new_ptr' declared in local scope yet 'period_elapsed' found its way out of here. To make code look cohesive, either have all declared in function's var declaration block or make sure local scope contains all local variables declarations. Of course this is obsolete if you decide to pursue shift-left suggestion. > + > + if (playback) { > + ptr = kmb_i2s->tx_ptr; > + new_ptr = kmb_pcm_tx_fn(kmb_i2s, substream->runtime, > + ptr, &period_elapsed); > + cmpxchg(&kmb_i2s->tx_ptr, ptr, new_ptr); > + } else { > + ptr = kmb_i2s->rx_ptr; > + new_ptr = kmb_pcm_rx_fn(kmb_i2s, substream->runtime, > + ptr, &period_elapsed); > + cmpxchg(&kmb_i2s->rx_ptr, ptr, new_ptr); > + } > + > + if (period_elapsed) > + snd_pcm_period_elapsed(substream); > + } > +} > + > + > +static int kmb_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) > +{ > + struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai); > + int ret; > + > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBM_CFM: > + kmb_i2s->master = false; > + ret = 0; > + break; Identation is off here. > + case SND_SOC_DAIFMT_CBS_CFS: > + writel(MASTER_MODE, kmb_i2s->pss_base + I2S_GEN_CFG_0); > + > + ret = clk_prepare_enable(kmb_i2s->clk_i2s); > + if (ret < 0) > + return ret; > + > + ret = devm_add_action_or_reset(kmb_i2s->dev, kmb_disable_clk, > + kmb_i2s->clk_i2s); > + if (ret) > + return ret; > + > + kmb_i2s->master = true; > + break; Same here. > + default: > + return -EINVAL; > + } > + > + return ret; > +} > + > + > +/* > + * struct i2s_clk_config_data - represent i2s clk configuration data > + * @chan_nr: number of channel > + * @data_width: number of bits per sample (8/16/24/32 bit) > + * @sample_rate: sampling frequency (8Khz, 16Khz, 48Khz) > + */ > +struct i2s_clk_config_data { > + int chan_nr; > + u32 data_width; > + u32 sample_rate; > +}; > + > +struct kmb_i2s_info { > + void __iomem *i2s_base; > + void __iomem *pss_base; > + struct clk *clk_i2s; > + struct clk *clk_apb; > + int active; > + unsigned int capability; > + unsigned int i2s_reg_comp1; > + unsigned int i2s_reg_comp2; > + struct device *dev; > + u32 ccr; > + u32 xfer_resolution; > + u32 fifo_th; > + bool master; > + > + struct i2s_clk_config_data config; > + int (*i2s_clk_cfg)(struct i2s_clk_config_data *config); > + > + /* data related to PIO transfers */ > + bool use_pio; > + struct snd_pcm_substream *tx_substream; > + struct snd_pcm_substream *rx_substream; > + unsigned int tx_ptr; > + unsigned int rx_ptr; As you 'if' stream's direction in several pcm handlers, how about declaring simple arrays here. You could access members based on SNDRV_PCM_STREAM_PLAYBACK/ CAPTURE i.e. direction, reducing the if-ology. struct snd_pcm_substream *substream[2]; unsigned int ptr[2]; If the number of direction-based 'ifs' cannot be reduced even with this change, drop the suggestion. > +}; > + > +#endif /* KMB_PLATFORM_H_ */ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v3 1/3] ASoC: Intel: Add KeemBay platform driver 2020-06-01 21:26 ` Cezary Rojewski @ 2020-06-02 12:57 ` Sia, Jee Heng 0 siblings, 0 replies; 9+ messages in thread From: Sia, Jee Heng @ 2020-06-02 12:57 UTC (permalink / raw To: Rojewski, Cezary Cc: Liam Girdwood, alsa-devel@alsa-project.org, Mark Brown, Takashi Iwai, Pierre-Louis Bossart -----Original Message----- From: Rojewski, Cezary <cezary.rojewski@intel.com> Sent: Tuesday, June 2, 2020 5:26 AM To: Sia, Jee Heng <jee.heng.sia@intel.com> Cc: alsa-devel@alsa-project.org; Liam Girdwood <liam.r.girdwood@linux.intel.com>; Mark Brown <broonie@kernel.org>; Takashi Iwai <tiwai@suse.com>; Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Subject: Re: [PATCH v3 1/3] ASoC: Intel: Add KeemBay platform driver On 2020-06-01 11:53 AM, Sia Jee Heng wrote: > + > +#define PERIODS_MIN 2 > +#define PERIODS_MAX 48 > +#define PERIOD_BYTES_MIN 4096 > +#define BUFFER_BYTES_MAX (PERIODS_MAX * PERIOD_BYTES_MIN) > +#define TDM_OPERATION 1 Looks as this is unused. [>>] It is needed by the TDM mode and shall submit patches in the near future. > +#define I2S_OPERATION 0 > +#define DATA_WIDTH_CONFIG_BIT 6 > +#define TDM_CHANNEL_CONFIG_BIT 3 > +#define I2S_SAMPLE_RATES (SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_48000) This defines common rates yet .formats are left alone? As .formats, .rate_min/ max and others are left alone I'd stick with this style and specify .rates directly too. [>>] OK, I can specify the rate directly. > + > +static const struct snd_pcm_hardware kmb_pcm_hardware = { > + .info = SNDRV_PCM_INFO_INTERLEAVED | > + SNDRV_PCM_INFO_MMAP | > + SNDRV_PCM_INFO_MMAP_VALID | > + SNDRV_PCM_INFO_BATCH | > + SNDRV_PCM_INFO_BLOCK_TRANSFER, Several flags have been or'ed here. Have these been verified working for Keembay? [>>] Yes, tested with alsa aplay/arecord before submit patches for review. > + .rates = I2S_SAMPLE_RATES, > + .rate_min = 16000, > + .rate_max = 48000, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S24_LE | > + SNDRV_PCM_FMTBIT_S32_LE, > + .channels_min = 2, > + .channels_max = 2, > + .buffer_bytes_max = BUFFER_BYTES_MAX, > + .period_bytes_min = PERIOD_BYTES_MIN, > + .period_bytes_max = BUFFER_BYTES_MAX / PERIODS_MIN, > + .periods_min = PERIODS_MIN, > + .periods_max = PERIODS_MAX, > + .fifo_size = 16, > +}; > + > +static unsigned int kmb_pcm_tx_fn(struct kmb_i2s_info *kmb_i2s, > + struct snd_pcm_runtime *runtime, > + unsigned int tx_ptr, bool *period_elapsed) { > + unsigned int period_pos = tx_ptr % runtime->period_size; > + void __iomem *i2s_base = kmb_i2s->i2s_base; > + u16(*p16)[2]; > + u32(*p32)[2]; > + int i; > + > + if (kmb_i2s->config.data_width == 16) > + p16 = (void *)runtime->dma_area; > + else > + p32 = (void *)runtime->dma_area; > + /* KMB i2s uses two separate L/R FIFO */ > + for (i = 0; i < kmb_i2s->fifo_th; i++) { > + if (kmb_i2s->config.data_width == 16) { > + writel(p16[tx_ptr][0], i2s_base + LRBR_LTHR(0)); > + writel(p16[tx_ptr][1], i2s_base + RRBR_RTHR(0)); > + } else { > + writel(p32[tx_ptr][0], i2s_base + LRBR_LTHR(0)); > + writel(p32[tx_ptr][1], i2s_base + RRBR_RTHR(0)); > + } Looks like a refactor candidate. Both, the preceding 'if' and the 'for' - which are the body of this func - are "data_width" dependent. You could redure if-ology by defining separate variants for 16 and non-16. [>>] Got it. I can refactor the code by reduce the if-ology. Idk about naming index-variable 'rx_ptr'. This isn't a pointer, that's for sure. [>>] I think that tx_ptr or rx_ptr can be easily map to the nature of alsa operation. Could you suggest a better naming if you have concern? > + > + period_pos++; > + > + if (++tx_ptr >= runtime->buffer_size) > + tx_ptr = 0; > + } > + > + *period_elapsed = period_pos >= runtime->period_size; > + > + return tx_ptr; > +} > + > +static unsigned int kmb_pcm_rx_fn(struct kmb_i2s_info *kmb_i2s, > + struct snd_pcm_runtime *runtime, > + unsigned int rx_ptr, bool *period_elapsed) { > + unsigned int period_pos = rx_ptr % runtime->period_size; > + void __iomem *i2s_base = kmb_i2s->i2s_base; > + u16(*p16)[2]; > + u32(*p32)[2]; > + int i; > + > + if (kmb_i2s->config.data_width == 16) > + p16 = (void *)runtime->dma_area; > + else > + p32 = (void *)runtime->dma_area; > + /* KMB i2s uses two separate L/R FIFO */ > + for (i = 0; i < kmb_i2s->fifo_th; i++) { > + if (kmb_i2s->config.data_width == 16) { > + p16[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0)); > + p16[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0)); > + } else { > + p32[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0)); > + p32[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0)); > + } The exact same advice goes here. [>>] I can further re-factor the code by reduce the if usage > + > + period_pos++; > + > + if (++rx_ptr >= runtime->buffer_size) > + rx_ptr = 0; > + } > + > + *period_elapsed = period_pos >= runtime->period_size; > + > + return rx_ptr; > +} > + > + > +static void kmb_pcm_operation(struct kmb_i2s_info *kmb_i2s, bool > +playback) { > + struct snd_pcm_substream *substream; > + bool active, period_elapsed; > + > + if (playback) > + substream = kmb_i2s->tx_substream; > + else > + substream = kmb_i2s->rx_substream; > + > + active = substream && snd_pcm_running(substream); 'active' serves little to no purpose here, I'd opt for removing it. [>>] OK. > + > + if (active) { When deciding between no middle func 'return' or recuded indentation, the later is more readable. Simple: if (!substream || snd_pcm_running(substream)) return; [>>] if (!substream && !snd_pcm_running(substream)) [>>] return; allows rest of the function to be shift-left'ed. > + unsigned int ptr; > + unsigned int new_ptr; 'ptr' and 'new_ptr' declared in local scope yet 'period_elapsed' found its way out of here. To make code look cohesive, either have all declared in function's var declaration block or make sure local scope contains all local variables declarations. Of course this is obsolete if you decide to pursue shift-left suggestion. [>>] Sure, can make it local scope for variable. > + > + if (playback) { > + ptr = kmb_i2s->tx_ptr; > + new_ptr = kmb_pcm_tx_fn(kmb_i2s, substream->runtime, > + ptr, &period_elapsed); > + cmpxchg(&kmb_i2s->tx_ptr, ptr, new_ptr); > + } else { > + ptr = kmb_i2s->rx_ptr; > + new_ptr = kmb_pcm_rx_fn(kmb_i2s, substream->runtime, > + ptr, &period_elapsed); > + cmpxchg(&kmb_i2s->rx_ptr, ptr, new_ptr); > + } > + > + if (period_elapsed) > + snd_pcm_period_elapsed(substream); > + } > +} > + > + > +static int kmb_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int > +fmt) { > + struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai); > + int ret; > + > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBM_CFM: > + kmb_i2s->master = false; > + ret = 0; > + break; Identation is off here. [>>] OK. > + case SND_SOC_DAIFMT_CBS_CFS: > + writel(MASTER_MODE, kmb_i2s->pss_base + I2S_GEN_CFG_0); > + > + ret = clk_prepare_enable(kmb_i2s->clk_i2s); > + if (ret < 0) > + return ret; > + > + ret = devm_add_action_or_reset(kmb_i2s->dev, kmb_disable_clk, > + kmb_i2s->clk_i2s); > + if (ret) > + return ret; > + > + kmb_i2s->master = true; > + break; Same here. [>>] OK. > + default: > + return -EINVAL; > + } > + > + return ret; > +} > + > + > +/* > + * struct i2s_clk_config_data - represent i2s clk configuration data > + * @chan_nr: number of channel > + * @data_width: number of bits per sample (8/16/24/32 bit) > + * @sample_rate: sampling frequency (8Khz, 16Khz, 48Khz) */ struct > +i2s_clk_config_data { > + int chan_nr; > + u32 data_width; > + u32 sample_rate; > +}; > + > +struct kmb_i2s_info { > + void __iomem *i2s_base; > + void __iomem *pss_base; > + struct clk *clk_i2s; > + struct clk *clk_apb; > + int active; > + unsigned int capability; > + unsigned int i2s_reg_comp1; > + unsigned int i2s_reg_comp2; > + struct device *dev; > + u32 ccr; > + u32 xfer_resolution; > + u32 fifo_th; > + bool master; > + > + struct i2s_clk_config_data config; > + int (*i2s_clk_cfg)(struct i2s_clk_config_data *config); > + > + /* data related to PIO transfers */ > + bool use_pio; > + struct snd_pcm_substream *tx_substream; > + struct snd_pcm_substream *rx_substream; > + unsigned int tx_ptr; > + unsigned int rx_ptr; As you 'if' stream's direction in several pcm handlers, how about declaring simple arrays here. You could access members based on SNDRV_PCM_STREAM_PLAYBACK/ CAPTURE i.e. direction, reducing the if-ology. struct snd_pcm_substream *substream[2]; unsigned int ptr[2]; If the number of direction-based 'ifs' cannot be reduced even with this change, drop the suggestion. [>>] Thanks but it doesn't reduce the if usage. > +}; > + > +#endif /* KMB_PLATFORM_H_ */ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] ASoC: Intel: Add makefiles and kconfig changes for KeemBay 2020-06-01 9:53 [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Sia Jee Heng 2020-06-01 9:53 ` [PATCH v3 1/3] ASoC: Intel: Add KeemBay " Sia Jee Heng @ 2020-06-01 9:53 ` Sia Jee Heng 2020-06-01 9:53 ` [PATCH v3 3/3] dt-bindings: sound: Add documentation for KeemBay i2s Sia Jee Heng ` (2 subsequent siblings) 4 siblings, 0 replies; 9+ messages in thread From: Sia Jee Heng @ 2020-06-01 9:53 UTC (permalink / raw To: alsa-devel; +Cc: liam.r.girdwood, broonie, tiwai, pierre-louis.bossart Add makefile and kconfig changes for Intel KeemBay platform driver. Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> Signed-off-by: Sia Jee Heng <jee.heng.sia@intel.com> --- sound/soc/intel/Kconfig | 7 +++++++ sound/soc/intel/Makefile | 1 + 2 files changed, 8 insertions(+) diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index c8de0bb..bc93448 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -244,6 +244,13 @@ config SND_SOC_ACPI_INTEL_MATCH endif ## SND_SOC_INTEL_SST_TOPLEVEL || SND_SOC_SOF_INTEL_TOPLEVEL +config SND_SOC_INTEL_KEEMBAY + tristate "Keembay Platforms" + depends on OF && (ARM64 || COMPILE_TEST) + depends on COMMON_CLK + help + If you have a Intel Keembay platform then enable this option + by saying Y or m. # ASoC codec drivers source "sound/soc/intel/boards/Kconfig" diff --git a/sound/soc/intel/Makefile b/sound/soc/intel/Makefile index 8160520..f5aa32b 100644 --- a/sound/soc/intel/Makefile +++ b/sound/soc/intel/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_SND_SOC_INTEL_HASWELL) += haswell/ obj-$(CONFIG_SND_SOC_INTEL_BAYTRAIL) += baytrail/ obj-$(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM) += atom/ obj-$(CONFIG_SND_SOC_INTEL_SKYLAKE) += skylake/ +obj-$(CONFIG_SND_SOC_INTEL_KEEMBAY) += keembay/ # Machine support obj-$(CONFIG_SND_SOC) += boards/ -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] dt-bindings: sound: Add documentation for KeemBay i2s 2020-06-01 9:53 [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Sia Jee Heng 2020-06-01 9:53 ` [PATCH v3 1/3] ASoC: Intel: Add KeemBay " Sia Jee Heng 2020-06-01 9:53 ` [PATCH v3 2/3] ASoC: Intel: Add makefiles and kconfig changes for KeemBay Sia Jee Heng @ 2020-06-01 9:53 ` Sia Jee Heng 2020-06-01 21:30 ` [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Cezary Rojewski 2020-06-15 23:40 ` Mark Brown 4 siblings, 0 replies; 9+ messages in thread From: Sia Jee Heng @ 2020-06-01 9:53 UTC (permalink / raw To: alsa-devel; +Cc: liam.r.girdwood, broonie, tiwai, pierre-louis.bossart Document Intel KeemBay i2s DT bindings. Signed-off-by: Sia Jee Heng <jee.heng.sia@intel.com> --- .../bindings/sound/intel,keembay-i2s.yaml | 68 ++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml diff --git a/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml b/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml new file mode 100644 index 0000000..175e4fb --- /dev/null +++ b/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2020 Intel Corporation +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/intel,keembay-i2s.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel KeemBay I2S Device Tree Bindings + +maintainers: + - Sia, Jee Heng <jee.heng.sia@intel.com> + +description: | + Intel KeemBay I2S + +properties: + compatible: + enum: + - intel,keembay-i2s + + "#sound-dai-cells": + const: 0 + + reg: + items: + - description: I2S configuration + + reg-names: + items: + - const: i2s-regs + - const: i2s_gen_cfg + + interrupts: + maxItems: 1 + + clocks: + items: + - description: Bus Clock + - description: Module Clock + + clock-names: + items: + - const: osc + - const: apb_clk + +required: + - compatible + - "#sound-dai-cells" + - reg + - clocks + - clock-names + - interrupts + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #define KEEM_BAY_PSS_AUX_I2S3 + #define KEEM_BAY_PSS_I2S3 + i2s3: i2s@20140000 { + compatible = "intel,keembay-i2s"; + #sound-dai-cells = <0>; + reg = <0x20140000 0x200 0x202a00a4 0x4>; + reg-names = "i2s-regs", "i2s_gen_cfg"; + interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "osc", "apb_clk"; + clocks = <&scmi_clk KEEM_BAY_PSS_AUX_I2S3>, <&scmi_clk KEEM_BAY_PSS_I2S3>; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver 2020-06-01 9:53 [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Sia Jee Heng ` (2 preceding siblings ...) 2020-06-01 9:53 ` [PATCH v3 3/3] dt-bindings: sound: Add documentation for KeemBay i2s Sia Jee Heng @ 2020-06-01 21:30 ` Cezary Rojewski 2020-06-02 12:35 ` Sia, Jee Heng 2020-06-15 23:40 ` Mark Brown 4 siblings, 1 reply; 9+ messages in thread From: Cezary Rojewski @ 2020-06-01 21:30 UTC (permalink / raw To: Sia Jee Heng Cc: alsa-devel, Andy Shevchenko, tiwai, pierre-louis.bossart, liam.r.girdwood, broonie On 2020-06-01 11:53 AM, Sia Jee Heng wrote: > The below series of patches support the KeemBay ASoC platform driver. > The platform driver initialize the i2s to capture and playback the > pcm data on the ARM. The i2s is running in polling mode. > > There is no DSP in the KeemBay SoC. Users are rely on the Gstreamer plugin > to perform Audio preprocessing. > > Audio graph card is used to connect the platform driver with the > tlv320aic3204 codec. > > Change History: > v3: > - Adjusted header format. > - Use Audio graph card instead of custom sound card. > - Use if-else instead of conditional operator. > - Enabled .set_fmt to configure master clock. > > v2: > - Corrected I2S naming for DT binding. > > v1: > - Initial version. > > Sia Jee Heng (3): > ASoC: Intel: Add KeemBay platform driver > ASoC: Intel: Add makefiles and kconfig changes for KeemBay > dt-bindings: sound: Add documentation for KeemBay i2s > As these series has been given plenty of comments internally from Andy, Pierre and me, I'd be best to have all reviewers CC'ed here. Czarek ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver 2020-06-01 21:30 ` [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Cezary Rojewski @ 2020-06-02 12:35 ` Sia, Jee Heng 0 siblings, 0 replies; 9+ messages in thread From: Sia, Jee Heng @ 2020-06-02 12:35 UTC (permalink / raw To: Rojewski, Cezary Cc: alsa-devel@alsa-project.org, Shevchenko, Andriy, tiwai@suse.com, pierre-louis.bossart@linux.intel.com, liam.r.girdwood@linux.intel.com, broonie@kernel.org -----Original Message----- From: Rojewski, Cezary <cezary.rojewski@intel.com> Sent: Tuesday, June 2, 2020 5:31 AM To: Sia, Jee Heng <jee.heng.sia@intel.com> Cc: alsa-devel@alsa-project.org; liam.r.girdwood@linux.intel.com; broonie@kernel.org; tiwai@suse.com; pierre-louis.bossart@linux.intel.com; Shevchenko, Andriy <andriy.shevchenko@intel.com> Subject: Re: [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver On 2020-06-01 11:53 AM, Sia Jee Heng wrote: > The below series of patches support the KeemBay ASoC platform driver. > The platform driver initialize the i2s to capture and playback the pcm > data on the ARM. The i2s is running in polling mode. > > There is no DSP in the KeemBay SoC. Users are rely on the Gstreamer > plugin to perform Audio preprocessing. > > Audio graph card is used to connect the platform driver with the > tlv320aic3204 codec. > > Change History: > v3: > - Adjusted header format. > - Use Audio graph card instead of custom sound card. > - Use if-else instead of conditional operator. > - Enabled .set_fmt to configure master clock. > > v2: > - Corrected I2S naming for DT binding. > > v1: > - Initial version. > > Sia Jee Heng (3): > ASoC: Intel: Add KeemBay platform driver > ASoC: Intel: Add makefiles and kconfig changes for KeemBay > dt-bindings: sound: Add documentation for KeemBay i2s > As these series has been given plenty of comments internally from Andy, Pierre and me, I'd be best to have all reviewers CC'ed here. [>>] Noted. Czarek ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver 2020-06-01 9:53 [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Sia Jee Heng ` (3 preceding siblings ...) 2020-06-01 21:30 ` [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Cezary Rojewski @ 2020-06-15 23:40 ` Mark Brown 4 siblings, 0 replies; 9+ messages in thread From: Mark Brown @ 2020-06-15 23:40 UTC (permalink / raw To: alsa-devel, Sia Jee Heng; +Cc: liam.r.girdwood, tiwai, pierre-louis.bossart On Mon, 1 Jun 2020 17:53:18 +0800, Sia Jee Heng wrote: > The below series of patches support the KeemBay ASoC platform driver. > The platform driver initialize the i2s to capture and playback the > pcm data on the ARM. The i2s is running in polling mode. > > There is no DSP in the KeemBay SoC. Users are rely on the Gstreamer plugin > to perform Audio preprocessing. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/3] ASoC: Intel: Add KeemBay platform driver commit: c5477e966728f8e9d1434543d4ee677d515e1078 [2/3] ASoC: Intel: Add makefiles and kconfig changes for KeemBay commit: c544912bcc2dc806ba71d9157ccefca4a4a885f0 [3/3] ASoC: Add documentation for KeemBay i2s commit: e16caedf737145f74e34e45205b15695adf66f38 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-15 23:45 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-01 9:53 [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Sia Jee Heng 2020-06-01 9:53 ` [PATCH v3 1/3] ASoC: Intel: Add KeemBay " Sia Jee Heng 2020-06-01 21:26 ` Cezary Rojewski 2020-06-02 12:57 ` Sia, Jee Heng 2020-06-01 9:53 ` [PATCH v3 2/3] ASoC: Intel: Add makefiles and kconfig changes for KeemBay Sia Jee Heng 2020-06-01 9:53 ` [PATCH v3 3/3] dt-bindings: sound: Add documentation for KeemBay i2s Sia Jee Heng 2020-06-01 21:30 ` [PATCH v3 0/3] ASoC: Intel: Add KeemBay ASoC platform driver Cezary Rojewski 2020-06-02 12:35 ` Sia, Jee Heng 2020-06-15 23:40 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.