[PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels

[Dinh Nguyen]

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 at oracle.com]
>>>>> 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
>