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

[Mark Brown]

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.

> +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.

> +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.

> +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?

> +	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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20200518/212a3e40/attachment.sig>