RE: [PATCH v9 02/14] ASoC: Intel: catpt: Implement IPC protocol

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

On 2020-09-29 10:46 AM, Andy Shevchenko wrote:
> On Mon, Sep 28, 2020 at 04:52:41PM +0000, Rojewski, Cezary wrote:
>> On 2020-09-28 3:44 PM, Andy Shevchenko wrote:
>>> On Sat, Sep 26, 2020 at 10:58:58AM +0200, Cezary Rojewski wrote:

...

>>>> +	ret = ipc->rx.rsp.status;
>>>> +	if (reply) {
>>>> +		reply->header = ipc->rx.header;
>>>> +		if (!ret && reply->data)
>>>> +			memcpy(reply->data, ipc->rx.data, reply->size);
>>>> +	}
>>>> +
>>>> +	return ret;
>>>
>>> One more looking into this... What about
>>>
>>> 	if (reply)
>>> 		reply->header = ipc->rx.header;
>>>
>>> 	ret = ipc->rx.rsp.status; // or even directly if (status) return status.
>>> 	if (ret)
>>> 		return ret;
>>>
>>> 	if (reply->data)
>>> 		memcpy(reply->data, ipc->rx.data, reply->size);
>>>
>>> 	return 0;
>>>
>>> It may be verbose but I think readability is better here.
>>
>> In your example, last 'if' will cause exception if reply==NULL.
> 
> Yeah, should be reply && reply->data.
> 
>> Got your point, although that's just few lines which already involve
>> 'if' with { } spacing. After removing size-checks you suggested this
>> code is quite thin already.
> 
> Yes, sometimes too thin is not good in terms of readability.
> 

This will cost us additional check (2x reply==NULL instead of 1x). Maybe
a newline between:

	reply->header = ipc->rx.header;

	if (!ret && reply->data)
		memcpy(reply->data, ipc->rx.data, reply->size);

suffices?


...

>>>> +	CATPT_CHANNEL_CONFIG_5_POINT_0	= 7, /* L, C, R, Ls & Rs */
>>>> +	CATPT_CHANNEL_CONFIG_5_POINT_1	= 8, /* L, C, R, Ls, Rs & LFE */
>>>> +	CATPT_CHANNEL_CONFIG_DUAL_MONO	= 9, /* One channel replicated in two */
>>>> +	CATPT_CHANNEL_CONFIG_INVALID	= 10,
>>>
>>> Hmm... I think I got the point why DUAL_MONO was at the end of the switch-case.
>>>
>>
>> Well, well. Indeed we found where Willy is..
> 
> So, we may return to the previous state, up to you.
> 
> ...
> 
>>>> +enum catpt_mclk_frequency {
>>>> +	CATPT_MCLK_OFF = 0,
>>>> +	CATPT_MCLK_FREQ_6_MHZ = 1,
>>>> +	CATPT_MCLK_FREQ_21_MHZ = 2,
>>>> +	CATPT_MCLK_FREQ_24_MHZ = 3,
>>>> +};
>>>
>>> Looks like a 3 MHz as base frequency with multiplicators 0, 2, 7, 8.
>>
>> Naming based on FW enum type equivalent.
> 
> It was just an observation without any AR item :-)
> If you really know the (hardware) background of these choices then perhaps
> comment would be good to have.
> 

In general I'm avoiding that subject here. Quite disappointed with what
I had to face, and that's not "just" existing linux solution but every
other component involved in LPT/WPT ADSP project.

I have plans for many more comments Andy, e.g.: fw image block division
(ascii graph and such). All of that will come in time, eventually. Not
intending to leave catpt without maintenance like other sound/soc/intel/
solutions once were.

>>>> +#define CATPT_STREAM_MSG(msg_type) \
>>>> +{ \
>>>> +	.stream_msg_type = CATPT_STRM_##msg_type, \
>>>> +	.global_msg_type = CATPT_GLB_STREAM_MESSAGE }
>>>> +#define CATPT_STAGE_MSG(msg_type) \
>>>> +{ \
>>>> +	.stage_action = CATPT_STG_##msg_type, \
>>>> +	.stream_msg_type = CATPT_STRM_STAGE_MESSAGE, \
>>>> +	.global_msg_type = CATPT_GLB_STREAM_MESSAGE }
>>>
>>> Hmm... This split is interesting. I would rather move } to a new line.
>>>
>>
>> As basically everything in my code, style is based on existing example -
>> usually stuff from ASoC core - here, it's include/sound/soc.h. It's full
>> of such definitions so because catpt belongs to ASoC subsystem, I've
>> used the exact same style. No problem changing if that's your preference.
> 
> I think it's better to follow the existing style across subsystem. You may
> discard my (style related) comments if they contradict with used style.
> 

If there are no other concerns, either a quick spinoff (v10) or delay
those readability improvements till series with resource_union() re-use?

While catpt is a big upgrade when compared to existing /haswell/
(obviously) there are more fruits of this rewrite: house cleaning -
follow-up series targeting /haswell/, /baytrail/ and /common/ of
sound/soc/intel. Guess everyone would like to see those in 5.10.

Regards,
Czarek