On Tue, Oct 01, 2013 at 10:05:17AM +0300, Dan Carpenter wrote: > On Tue, Oct 01, 2013 at 01:21:28AM +0000, Paul Zimmerman wrote: > > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > > Sent: Monday, September 30, 2013 6:09 PM > > > > > > Yeah. I guess it's fine... I was going to suggest adding the + 1 in a > > > different place but actually it doesn't matter. > > > > > > The key to understanding dwc2_set_param_host_channels() is that the > > > "val" parameter is always -1. That means it always returns -EINVAL and > > > the caller jumbles the error code in with some other error codes and > > > then ignores any errors. :/ > > > > The intent of this was that the value can be overridden by the platform > > code if required, in which case "val" would not be -1. However, to my > > knowledge, no in-tree platforms do that, so I guess it would be fine to > > redo this as you suggest below. We can always add it back if needed. If we redo the dwc2_set_param_host_channels function, we should probably redo _all_ of the dwc2_set_param_* functions, since they all do this. > Why are we even adding one to the number of channels that the hardware > reports? Because that's how the hardware register is defined. I presume it never makes sense to have 0 host channels, this allows a range of 1-16 instead of 0-15. In any case, for this particular problem, I would think that simply making the host_channels field in dcw2_hw_params bigger is the better solution here. E.g., something like: struct dwc2_hw_params { ... - unsigned host_channels:4; + unsigned host_channels:5; } Moving the +1 calculation reverts the code back to what it was before one of my patches. I moved the +1 to this place, so that the off-by-one detail of the hardware register is only known at the place where hardware registers are read. All other code can simply assume that the "host_channels" variable contains just that: the number of host channels present. However, in that patch I apparently chose the wrong size for the host_channels field, which I think should be problem to fix here. Gr. Matthijs
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
