-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Tuesday, May 19, 2020 1:07 AM To: Sia, Jee Heng jee.heng.sia@intel.com Cc: alsa-devel@alsa-project.org; tiwai@suse.com; pierre-louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com Subject: Re: [PATCH v2 1/4] ASoC: Intel: Add KeemBay platform driver
On Mon, May 18, 2020 at 10:17:19AM +0800, Sia Jee Heng wrote:
+++ b/sound/soc/intel/keembay/kmb_platform.c @@ -0,0 +1,746 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Intel KeemBay Platform driver
- Copyright (C) 2020 Intel Corporation.
Please keep the entire header C++ style so things look more consistent. [>>] Will below format meet the C++ style? [>>] // SPDX-License-Identifier: GPL-2.0-only [>>] // Copyright (C) 2020 Intel Corporation. [>>] /* [>>] * Intel KeemBay Platform driver [>>] */
+static void pcm_operation(struct kmb_i2s_info *kmb_i2s, bool +playback)
Please namespace this function, it looks like a core ALSA call. It'd probably be better to namespace a bunch of the other functions called i2s_ as well. [>>] OK
+static inline void 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));irq = trigger ? irq & ~flag : irq | flag;
Please write this as a normal if statement to improve legibility. [>>] OK
+static int kmb_configure_dai_by_dt(struct kmb_i2s_info *kmb_i2s,
struct snd_soc_dai_driver *kmb_i2s_dai) {- u32 comp1 = readl(kmb_i2s->i2s_base + I2S_COMP_PARAM_1);
- if (COMP1_TX_ENABLED(comp1))
kmb_i2s->capability |= DWC_I2S_PLAY;- if (COMP1_RX_ENABLED(comp1))
kmb_i2s->capability |= DWC_I2S_RECORD;- return kmb_configure_dai(kmb_i2s, kmb_i2s_dai,
I2S_SAMPLE_RATES);+}
This isn't actually reading the DT?
+static void kmb_disable_clk(void *data) {
- clk_disable_unprepare(data);
+}
This function doesn't seem to be adding anything? [>>] It intends to disable the clock during error return.
- ret = of_property_read_string(dev->of_node, "mode", &i2s_mode);
- if (ret < 0) {
dev_err(dev, "Couldn't find the entry\n");return -EINVAL;- }
- dev_dbg(kmb_i2s->dev, "Mode = %s", i2s_mode);
- ret = match_string(i2s_mode_name, ARRAY_SIZE(i2s_mode_name),
+i2s_mode);
This property isn't defined in the DT binding and should be configured by the machine driver through a set_fmt() operation anyway.