Hi Dan,
On 10/1/13 3:23 AM, Dan Carpenter wrote:
On Tue, Oct 01, 2013 at 09:51:07AM +0200, Matthijs Kooijman wrote:
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.
Yeah... Generally one of the early steps in staging drivers is to
delete as much code as possible. The rule in the kernel is to not
include any functionality which doesn't have a user.
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.
Somehow I assumed that was fixed by the hardware, but I see now that you
are right. Yes, making the definition larger is better than moving the
+ 1.
This was my original fix to the problem, but I thought that it would be
confusing when reading the code. I also thought about the "+1" for
host_channels was strange too. For debug outputs, it would be more
accurate to display 16 channels, in code-wise, I see that host_channels
is used in 2 for loops. Does it make sense to just fix the for loops to
include channels 0-15?
Dinh, do you want to do that? The other option is that Matthijs could
fix it and give you the Reported-by credit.
I'm fine with that, if Matthijs wants to submit the fix. I can test it
on my hardware too.
Dinh
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
