On Tue, Sep 29, 2020 at 09:39:36AM +0000, Rojewski, Cezary wrote:
> 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?
I don't like the !ret piece, TBH.
But I wouldn't object too much if you leave it as is. And double check for
reply at least makes it cleaner in my opinion than compressing in few lines.
...
> If there are no other concerns, either a quick spinoff (v10) or delay
> those readability improvements till series with resource_union() re-use?
I guess we may do v10 w/o waiting. Let me look at the last two patches I
haven't commented yet.
> 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.
Yes!
--
With Best Regards,
Andy Shevchenko
