On 20/02/24 22:10, Judith Mendez wrote:
> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>> On 7/02/24 03:15, Judith Mendez wrote:
>>> +
>>> + if (!num_fails)
>>> + return ITAPDLY_LAST_INDEX >> 1;
>>> +
>>> + if (fail_window->length == ITAPDLY_LENGTH) {
>>> + dev_err(dev, "No passing ITAPDLY, return 0\n");
>>> + return 0;
>>> + }
>>> +
>>> + first_fail_start = fail_window->start;
>>> + last_fail_end = fail_window[num_fails - 1].end;
>>> +
>>> + for (i = 0; i < num_fails; i++) {
>>> + start_fail = fail_window[i].start;
>>> + end_fail = fail_window[i].end;
>>> + pass_length = start_fail - (prev_fail_end + 1);
>>> +
>>> + if (pass_length > pass_window.length) {
>>> + pass_window.start = prev_fail_end + 1;
>>> + pass_window.length = pass_length;
>>> + }
>>> + prev_fail_end = end_fail;
>>> + }
>>> +
>>> + if (!circular_buffer)
>>> + pass_length = ITAPDLY_LAST_INDEX - last_fail_end;
>>> + else
>>> + pass_length = ITAPDLY_LAST_INDEX - last_fail_end + first_fail_start;
>>> +
>>> + if (pass_length > pass_window.length) {
>>> + pass_window.start = last_fail_end + 1;
>>> + pass_window.length = pass_length;
>>> + }
>>> +
>>> + if (!circular_buffer)
>>> + itap = pass_window.start + (pass_window.length >> 1);
>>> + else
>>> + itap = (pass_window.start + (pass_window.length >> 1)) % ITAPDLY_LENGTH;
>>> +
>>> + return (itap < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : itap);
>>
>> Parentheses are not needed where they are but putting
>> them around the condition would make it more readable e.g.
>>
>> return (itap < 0 || itap > ITAPDLY_LAST_INDEX) ? 0 : itap;
>>
>> However (itap < 0) is not possible because itap is an unsigned type
>> and if (itap > ITAPDLY_LAST_INDEX) then maybe it would be better
>> to return ITAPDLY_LAST_INDEX
>
> You are right about itap < 0, thanks will fix.
>
> About itap > ITAPDLY_LAST_INDEX, this is an error. Why
> return ITAPDLY_LAST_INDEX instead of 0?
It doesn't matter. Just if a value has a better chance to work
if the calculation fails, like maybe ITAPDLY_LAST_INDEX / 2, but
presumably it should not fail.
