[PATCH v2 1/4] ASoC: Intel: Add KeemBay platform driver

[Sia, Jee Heng]

-----Original Message-----
From: Mark Brown <broonie at kernel.org> 
Sent: Tuesday, May 19, 2020 1:07 AM
To: Sia, Jee Heng <jee.heng.sia at intel.com>
Cc: alsa-devel at alsa-project.org; tiwai at suse.com; pierre-louis.bossart at linux.intel.com; liam.r.girdwood at 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.