RE: [PATCH v3 1/3] ASoC: Intel: Add KeemBay platform driver

-----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_ */