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

[Andy Shevchenko]

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.

...

> >> +	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.

...

> >> +#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.

-- 
With Best Regards,
Andy Shevchenko