[PATCH v9 02/14] ASoC: Intel: catpt: Implement IPC protocol

[Andy Shevchenko]

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