-----Original Message-----
From: Mark Brown <broonie@xxxxxxxxxx>
Sent: Tuesday, May 19, 2020 1:07 AM
To: Sia, Jee Heng <jee.heng.sia@xxxxxxxxx>
Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; liam.r.girdwood@xxxxxxxxxxxxxxx
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.
