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!