On Fri, Apr 24, 2015 at 06:18:38PM +0100, Mark Brown wrote:
> On Fri, Apr 17, 2015 at 06:46:04PM +0530, Vinod Koul wrote:
> 
> > +void ssth_cldma_int_enable(struct ssth_lib *ctx)
> > +{
> > +	ssth_updatel_bits(ctx, HDA_ADSP_REG_ADSPIC,
> > +			ADSPIC_CL_DMA, 0x2);
> > +}
> > +void ssth_cldma_int_disable(struct ssth_lib *ctx)
> > +{
> > +	ssth_updatel_bits(ctx, HDA_ADSP_REG_ADSPIC,
> > +			ADSPIC_CL_DMA, 0);
> > +}
> 
> Blank lines between functions.  Seems to be an Intel coding style thing?
> :P
sure seems, will fix

> 
> > +/* Code loader helper APIs */
> > +static void ssth_skl_cl_setup_bdle(struct snd_dma_buffer *dmab_data,
> > +		u32 **bdlp, u32 count)
> > +{
> > +	u32 *bdl = *bdlp;
> > +	int i = 0;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		phys_addr_t addr = virt_to_phys(dmab_data->area + i * PAGE_SIZE);
> 
> So this we index by i and...
> 
> > +
> > +		bdl[0] = cpu_to_le32(lower_32_bits(addr));
> > +		bdl[1] = cpu_to_le32(upper_32_bits(addr));
> > +		bdl[2] = cpu_to_le32(PAGE_SIZE);
> > +		bdl[3] = 0;
> > +		bdl += 4;
> > +	}
> 
> ...this we index by stepping through the array with increments in the
> body of the loop.  Consistency would be nice (and more obviously
> correct).
ok

> > +static void ssth_skl_cl_cleaup(struct ssth_lib  *ctx)
> > +{
> 
> Can't we clean it up instead?
yes should have fixed earlier

> 
> > +	if (ctx->cl_dev.bytes_left <= ctx->cl_dev.bufsize &&
> > +			ctx->cl_dev.bytes_left > ctx->cl_dev.period_size) {
> > +
> > +		dev_dbg(ctx->dev, "%s: size less than buffer size: %u\n",
> > +			       __func__, ctx->cl_dev.bytes_left);
> > +		ssth_cldma_int_disable(ctx);
> > +		ctx->cl_dev.curr_spib_pos = ctx->cl_dev.bytes_left;
> > +		ssth_cl_dma_fill_buffer(ctx, size, false, false, 0, false, true);
> > +		do {
> > +			mdelay(5);
> > +			link_pos = ssth_readl(ctx, CL_SD_LPIB);
> > +		} while (link_pos < size);
> 
> Should time out in case the DMA got stuck somehow.
yes will add

> 
> > +		goto cleanup;
> > +	}
> 
> What if the buffer is just too big?  Looks like this would loop for
> ever.
DMA is started, so link_pos get updated and we keep reading it. Since its
DMA a big buffer will get done fairly soon.

Thanks
-- 
~Vinod