Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Aug 17, 2020 at 01:12:01PM +0200, Cezary Rojewski wrote:
> On 2020-08-13 8:51 PM, Andy Shevchenko wrote:
> > On Wed, Aug 12, 2020 at 10:57:42PM +0200, Cezary Rojewski wrote:
> > > Implement dsp lifecycle functions such as core RESET and STALL,
> > > SRAM power control and LP clock selection. This also adds functions for
> > > handling transport over DW DMA controller.
> 
> Thanks for your input Andy!

You're welcome!

...

> > > +#define CATPT_DMA_DEVID		1 /* dma engine used */
> > 
> > Not sure I understand what exactly this means.
> > 
> 
> Well, you may choose either engine 0 or 1 for loading images. Reference
> solution which I'm basing catpt on - Windows driver equivalent - makes use
> of engine 1. Goal of this implementation is to align closely to stable
> Windows solution wherever possible to reduce maintainance cost.

Please, give extended comment here.

...

> > > +	status = dma_wait_for_async_tx(desc);
> > 
> > > +	catpt_updatel_shim(cdev, HMDC,
> > > +			   CATPT_HMDC_HDDA(CATPT_DMA_DEVID, chan->chan_id), 0);
> > 
> > Update even in erroneous case?
> > 
> 
> Yes. This is based on stable Windows solution equivalent and get's updated
> even in failure case to disable access to HOST memory in demand more.

I guess this deserves a comment.

> > > +	return (status == DMA_COMPLETE) ? 0 : -EPROTO;

...

> > > +	new <<= __ffs(mask);
> > > +	new = ~(new) & mask;
> > 
> > Unneeded parentheses.
> > And perhaps in one line it will be better to understand:
> > 
> > 	new = ~(new << __ffs(mask)) & mask;
> > 
> 
> Was called out in the past not to combine everything in one-line like if I'm
> to hide something from reviewer.
> 
> No problem with combining these together in v5.

you also may consider to use u32_replace_bits() or so.

...

> > > +	bool lp;
> > > +
> > > +	if (list_empty(&cdev->stream_list))
> > > +		return catpt_dsp_select_lpclock(cdev, true, true);
> > > +
> > > +	lp = true;
> > > +	list_for_each_entry(stream, &cdev->stream_list, node) {
> > > +		if (stream->prepared) {
> > > +			lp = false;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return catpt_dsp_select_lpclock(cdev, lp, true);
> > 
> > Seems too much duplication.
> > 
> > 	struct catpt_stream_runtime *stream;
> > 
> > 	list_for_each_entry(stream, &cdev->stream_list, node) {
> > 		if (stream->prepared)
> > 			return catpt_dsp_select_lpclock(cdev, false, true);
> > 	}
> > 
> > 	return catpt_dsp_select_lpclock(cdev, true, true);
> > 
> > 
> > Better?
> 
> list_first_entry (part of list_for_each_entry) expects list to be non-empty.
> ->streal_list may be empty when invoking catpt_dsp_update_lpclock().

I didn't get this. Can you point out where is exactly problematic place?

-- 
With Best Regards,
Andy Shevchenko