Re: [PATCH v7 1/4] ASoC: Intel: add Skylake HDA platform driver

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1448 bytes --]

On Wed, Jul 08, 2015 at 07:56:55PM +0100, Mark Brown wrote:
> On Mon, Jul 06, 2015 at 08:54:23AM +0530, Vinod Koul wrote:
> 
> > +	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
> > +	pm_runtime_get_sync(dai->dev);
> 
> Check the return value please, this can fail.
Yes..

> 
> > +static int skl_get_format(struct snd_pcm_substream *substream,
> > +		struct snd_soc_dai *dai)
> > +{
> > +	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
> > +	struct skl_dma_params *dma_params;
> > +	int format_val = 0;
> > +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > +
> > +	dma_params  = (struct skl_dma_params *)
> > +			snd_soc_dai_get_dma_data(codec_dai, substream);
> 
> Should be no need to cast away from void.
Right.

> 
> > +	if (substream->runtime->dma_area)
> > +		memset(substream->runtime->dma_area, 0,
> > +				params_buffer_bytes(params));
> 
> Why do we need to memset here?
Nope, the buffer is allocated thru snd_pcm_lib_malloc_pages() which IIUC
would for this case do a kzalloc, so we should rmeove this
> 
> > +	dma_params  = (struct skl_dma_params *)
> > +			snd_soc_dai_get_dma_data(dai, substream);
> > +
> > +	pm_runtime_mark_last_busy(dai->dev);
> > +	pm_runtime_put_autosuspend(dai->dev);
> > +	kfree(dma_params);
> 
> We check elsewhere for dma_params being NULL, shouldn't we set it to be
> NULL when we free?
Thanks for pointing, fixed now

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]