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
