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