Received-SPF: pass (google.com: domain of linux-kernel+bounces-85275-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <1f5c1533-5c9f-4cc2-b40b-6e9b22374640@ti.com>
Date: Wed, 28 Feb 2024 09:38:39 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/7] mmc: sdhci_am654: Add tuning algorithm for delay
 chain
To: Adrian Hunter <adrian.hunter@intel.com>
CC: <linux-mmc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Ulf Hansson
	<ulf.hansson@linaro.org>
References: <20240207011520.3128382-1-jm@ti.com>
 <20240207011520.3128382-2-jm@ti.com>
 <461a19cd-36ce-4c34-890e-655a05a81c58@intel.com>
 <6d939482-9a3a-4923-b74b-ceb31b0ba7e9@ti.com>
 <4618f19d-0b7d-4844-83f7-ff2f4be083d9@intel.com>
Content-Language: en-US
From: Judith Mendez <jm@ti.com>
In-Reply-To: <4618f19d-0b7d-4844-83f7-ff2f4be083d9@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

Hello Adrian,

On 2/28/24 7:21 AM, Adrian Hunter wrote:
> 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.

Ok, ITAPDLY_LAST_INDEX / sounds good to me, I will add this instead.

Thanks,
~ Judith