Hi, Mathias Nyman <mathias.ny...@linux.intel.com> writes: > On 20.01.2017 20:18, Felipe Balbi wrote: >> >> Mathias Nyman <mathias.ny...@linux.intel.com> writes: >> >>> Remove duplicate code by using trb_to_noop() when >>> handling Aborted commads >>> >>> Signed-off-by: Mathias Nyman <mathias.ny...@linux.intel.com> >> >> isn't this just [1] ???? >> >> https://marc.info/?i=20161229110109.26372-25-felipe.ba...@linux.intel.com >> > > A simplified version of [1]. > There are only two types of no-op trbs, command and transfer no-ops. > We always know to which one of these we want to turn the trb so just > give it as a parameter.
Right, the only difference is that you're passing NOOP type as argument
instead of figuring it out inside trb_to_noop(). I didn't do it like
that because I find it to be error prone. User has to remember if s/he
is currently dealing with transfer or command TRBs when the function
(totally not a critical path) can easily figure that out.
> Solution [1] takes any trb and turns it into no-op, it's a more generic
> solution but it comes with the expense of code complexity, like the 20-case
> switch statement to check trb type to narrow it down to two options.
yeah, that 20-case switch statement is largely optimized by the
compiler, though. All cases are sequential integer values, so your
switch() gets optimized to something like:
if (type > X && type < Y)
noop = Z;
else if (type > W && type < V)
noop = T;
else
error();
Well, compiler could optimize this ever further if TRB types were held
in an enum instead of just several macros. Compiler would know for sure
which are allowed values for that argument... but let's not go into
that.
> I want xhci code to be as simple as possible.
a very honorable goal; common courtesy, however, would have you
commenting on the original patch and my explanation above could be given
to you. Then we would come to an agreement and I would, gladly, update
original patch.
In any case, it's just a patch ;-) No reason to stop your changes from
going upstream. Just, next time, please have the courtesy of discussing
changes in the list to avoid surprises ;-)
cheers
--
balbi
signature.asc
Description: PGP signature
