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
